| Message ID | 1432802414-12355-3-git-send-email-thomas.petazzoni@free-electrons.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thursday 28 May 2015 10:40:13 Thomas Petazzoni wrote: > Fixes: 1737cac69369 ("bus: mvebu-mbus: make sure SDRAM CS for DMA don't overlap the MBus bridge window") > Cc: <stable@vger.kernel.org> # v4.0+ > --- > drivers/bus/mvebu-mbus.c | 105 ++++++++--------------------------------------- > 1 file changed, 16 insertions(+), 89 deletions(-) Hmm, the stable kernel rules say that a patch cannot exceed 100 lines with context, so this one is technically too large. Maybe Greg has a suggestion about what to do here. Is it possible to make an exception for a revert? In theory you could make a smaller version of the patch that adds an #if 0 instead of removing some of the code that was added, in order to get below the limit, but that seems counterproductive for minimizing the possible risk. Arnd
Arnd, On Thu, 28 May 2015 10:49:46 +0200, Arnd Bergmann wrote: > On Thursday 28 May 2015 10:40:13 Thomas Petazzoni wrote: > > Fixes: 1737cac69369 ("bus: mvebu-mbus: make sure SDRAM CS for DMA don't overlap the MBus bridge window") > > Cc: <stable@vger.kernel.org> # v4.0+ > > --- > > drivers/bus/mvebu-mbus.c | 105 ++++++++--------------------------------------- > > 1 file changed, 16 insertions(+), 89 deletions(-) > > Hmm, the stable kernel rules say that a patch cannot exceed 100 > lines with context, so this one is technically too large. Ah, okay, I didn't know about this specific rule. > Maybe Greg has a suggestion about what to do here. Is it possible > to make an exception for a revert? In theory you could make a > smaller version of the patch that adds an #if 0 instead of removing > some of the code that was added, in order to get below the limit, > but that seems counterproductive for minimizing the possible risk. In the specific case of such an exact revert, isn't it possible to make an exception? I guess the very reason we have rules is to have exceptions for such rules, no? :-) It would really be more logical to have a revert than a different patch just disabling the change, since it would actually be more risky than just reverting to the previous situation. Thanks, Thomas
Hi Thomas, On 28/05/2015 10:40, Thomas Petazzoni wrote: > This reverts commit 1737cac69369 ("bus: mvebu-mbus: make sure SDRAM CS > for DMA don't overlap the MBus bridge window"), because it breaks DMA > on platforms having more than 2 GB of RAM. > > This commit changed the information reported to DMA masters device > drivers through the mv_mbus_dram_info() function so that the returned > DRAM ranges do not overlap with I/O windows. > > This was necessary as a preparation to support the new CESA Crypto > Engine driver, which will use DMA for cryptographic operations. But > since it does DMA with the SRAM which is mapped as an I/O window, > having DRAM ranges overlapping with I/O windows was problematic. > > To solve this, the above mentioned commit changed the mvebu-mbus to > adjust the DRAM ranges so that they don't overlap with the I/O > windows. However, by doing this, we re-adjust the DRAM ranges in a way > that makes them have a size that is no longer a power of two. While > this is perfectly fine for the Crypto Engine, which supports DRAM > ranges with a granularity of 64 KB, it breaks basically all other DMA > masters, which expect power of two sizes for the DRAM ranges. > > Due to this, if the installed system memory is 4 GB, in two > chip-selects of 2 GB, the second DRAM range will be reduced from 2 GB > to a little bit less than 2 GB to not overlap with the I/O windows, in > a way that results in a DRAM range that doesn't have a power of two > size. This means that whenever you do a DMA transfer with an address > located in the [ 2 GB ; 4 GB ] area, it will freeze the system. Any > serious DMA activity like simply running: > > for i in $(seq 1 64) ; do dd if=/dev/urandom of=file$i bs=1M count=16 ; done > > in an ext3 partition mounted over a SATA drive will freeze the system. > > Since the new CESA crypto driver that uses DMA has not been merged > yet, the easiest fix is to simply revert this commit. A follow-up > commit will introduce a different solution for the CESA crypto driver. > > Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com> > Fixes: 1737cac69369 ("bus: mvebu-mbus: make sure SDRAM CS for DMA don't overlap the MBus bridge window") > Cc: <stable@vger.kernel.org> # v4.0+ applied on mvebu/fixes I hope that the 100 lines rules will be not a problem for this case Thanks, Gregory > --- > drivers/bus/mvebu-mbus.c | 105 ++++++++--------------------------------------- > 1 file changed, 16 insertions(+), 89 deletions(-) > > diff --git a/drivers/bus/mvebu-mbus.c b/drivers/bus/mvebu-mbus.c > index 7fa4510..6f047dc 100644 > --- a/drivers/bus/mvebu-mbus.c > +++ b/drivers/bus/mvebu-mbus.c > @@ -58,7 +58,6 @@ > #include <linux/debugfs.h> > #include <linux/log2.h> > #include <linux/syscore_ops.h> > -#include <linux/memblock.h> > > /* > * DDR target is the same on all platforms. > @@ -103,9 +102,7 @@ > > /* Relative to mbusbridge_base */ > #define MBUS_BRIDGE_CTRL_OFF 0x0 > -#define MBUS_BRIDGE_SIZE_MASK 0xffff0000 > #define MBUS_BRIDGE_BASE_OFF 0x4 > -#define MBUS_BRIDGE_BASE_MASK 0xffff0000 > > /* Maximum number of windows, for all known platforms */ > #define MBUS_WINS_MAX 20 > @@ -579,106 +576,36 @@ static unsigned int armada_xp_mbus_win_remap_offset(int win) > return MVEBU_MBUS_NO_REMAP; > } > > -/* > - * Use the memblock information to find the MBus bridge hole in the > - * physical address space. > - */ > -static void __init > -mvebu_mbus_find_bridge_hole(uint64_t *start, uint64_t *end) > -{ > - struct memblock_region *r; > - uint64_t s = 0; > - > - for_each_memblock(memory, r) { > - /* > - * This part of the memory is above 4 GB, so we don't > - * care for the MBus bridge hole. > - */ > - if (r->base >= 0x100000000) > - continue; > - > - /* > - * The MBus bridge hole is at the end of the RAM under > - * the 4 GB limit. > - */ > - if (r->base + r->size > s) > - s = r->base + r->size; > - } > - > - *start = s; > - *end = 0x100000000; > -} > - > static void __init > mvebu_mbus_default_setup_cpu_target(struct mvebu_mbus_state *mbus) > { > int i; > int cs; > - uint64_t mbus_bridge_base, mbus_bridge_end; > > mvebu_mbus_dram_info.mbus_dram_target_id = TARGET_DDR; > > - mvebu_mbus_find_bridge_hole(&mbus_bridge_base, &mbus_bridge_end); > - > for (i = 0, cs = 0; i < 4; i++) { > - u64 base = readl(mbus->sdramwins_base + DDR_BASE_CS_OFF(i)); > - u64 size = readl(mbus->sdramwins_base + DDR_SIZE_CS_OFF(i)); > - u64 end; > - struct mbus_dram_window *w; > - > - /* Ignore entries that are not enabled */ > - if (!(size & DDR_SIZE_ENABLED)) > - continue; > - > - /* > - * Ignore entries whose base address is above 2^32, > - * since devices cannot DMA to such high addresses > - */ > - if (base & DDR_BASE_CS_HIGH_MASK) > - continue; > - > - base = base & DDR_BASE_CS_LOW_MASK; > - size = (size | ~DDR_SIZE_MASK) + 1; > - end = base + size; > - > - /* > - * Adjust base/size of the current CS to make sure it > - * doesn't overlap with the MBus bridge hole. This is > - * particularly important for devices that do DMA from > - * DRAM to a SRAM mapped in a MBus window, such as the > - * CESA cryptographic engine. > - */ > + u32 base = readl(mbus->sdramwins_base + DDR_BASE_CS_OFF(i)); > + u32 size = readl(mbus->sdramwins_base + DDR_SIZE_CS_OFF(i)); > > /* > - * The CS is fully enclosed inside the MBus bridge > - * area, so ignore it. > + * We only take care of entries for which the chip > + * select is enabled, and that don't have high base > + * address bits set (devices can only access the first > + * 32 bits of the memory). > */ > - if (base >= mbus_bridge_base && end <= mbus_bridge_end) > - continue; > + if ((size & DDR_SIZE_ENABLED) && > + !(base & DDR_BASE_CS_HIGH_MASK)) { > + struct mbus_dram_window *w; > > - /* > - * Beginning of CS overlaps with end of MBus, raise CS > - * base address, and shrink its size. > - */ > - if (base >= mbus_bridge_base && end > mbus_bridge_end) { > - size -= mbus_bridge_end - base; > - base = mbus_bridge_end; > + w = &mvebu_mbus_dram_info.cs[cs++]; > + w->cs_index = i; > + w->mbus_attr = 0xf & ~(1 << i); > + if (mbus->hw_io_coherency) > + w->mbus_attr |= ATTR_HW_COHERENCY; > + w->base = base & DDR_BASE_CS_LOW_MASK; > + w->size = (size | ~DDR_SIZE_MASK) + 1; > } > - > - /* > - * End of CS overlaps with beginning of MBus, shrink > - * CS size. > - */ > - if (base < mbus_bridge_base && end > mbus_bridge_base) > - size -= end - mbus_bridge_base; > - > - w = &mvebu_mbus_dram_info.cs[cs++]; > - w->cs_index = i; > - w->mbus_attr = 0xf & ~(1 << i); > - if (mbus->hw_io_coherency) > - w->mbus_attr |= ATTR_HW_COHERENCY; > - w->base = base; > - w->size = size; > } > mvebu_mbus_dram_info.num_cs = cs; > } >
On Thu, May 28, 2015 at 10:49:46AM +0200, Arnd Bergmann wrote: > On Thursday 28 May 2015 10:40:13 Thomas Petazzoni wrote: > > Fixes: 1737cac69369 ("bus: mvebu-mbus: make sure SDRAM CS for DMA don't overlap the MBus bridge window") > > Cc: <stable@vger.kernel.org> # v4.0+ > > --- > > drivers/bus/mvebu-mbus.c | 105 ++++++++--------------------------------------- > > 1 file changed, 16 insertions(+), 89 deletions(-) > > Hmm, the stable kernel rules say that a patch cannot exceed 100 > lines with context, so this one is technically too large. "Technically" yes, but really, it's a guideline, if this is a revert, this should be fine, I don't have an objection to taking it. thanks, greg k-h
On Thu, May 28, 2015 at 11:17:10AM +0200, Gregory CLEMENT wrote: > Hi Thomas, > > On 28/05/2015 10:40, Thomas Petazzoni wrote: > > This reverts commit 1737cac69369 ("bus: mvebu-mbus: make sure SDRAM CS > > for DMA don't overlap the MBus bridge window"), because it breaks DMA > > on platforms having more than 2 GB of RAM. > > > > This commit changed the information reported to DMA masters device > > drivers through the mv_mbus_dram_info() function so that the returned > > DRAM ranges do not overlap with I/O windows. > > > > This was necessary as a preparation to support the new CESA Crypto > > Engine driver, which will use DMA for cryptographic operations. But > > since it does DMA with the SRAM which is mapped as an I/O window, > > having DRAM ranges overlapping with I/O windows was problematic. > > > > To solve this, the above mentioned commit changed the mvebu-mbus to > > adjust the DRAM ranges so that they don't overlap with the I/O > > windows. However, by doing this, we re-adjust the DRAM ranges in a way > > that makes them have a size that is no longer a power of two. While > > this is perfectly fine for the Crypto Engine, which supports DRAM > > ranges with a granularity of 64 KB, it breaks basically all other DMA > > masters, which expect power of two sizes for the DRAM ranges. > > > > Due to this, if the installed system memory is 4 GB, in two > > chip-selects of 2 GB, the second DRAM range will be reduced from 2 GB > > to a little bit less than 2 GB to not overlap with the I/O windows, in > > a way that results in a DRAM range that doesn't have a power of two > > size. This means that whenever you do a DMA transfer with an address > > located in the [ 2 GB ; 4 GB ] area, it will freeze the system. Any > > serious DMA activity like simply running: > > > > for i in $(seq 1 64) ; do dd if=/dev/urandom of=file$i bs=1M count=16 ; done > > > > in an ext3 partition mounted over a SATA drive will freeze the system. > > > > Since the new CESA crypto driver that uses DMA has not been merged > > yet, the easiest fix is to simply revert this commit. A follow-up > > commit will introduce a different solution for the CESA crypto driver. > > > > Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com> > > Fixes: 1737cac69369 ("bus: mvebu-mbus: make sure SDRAM CS for DMA don't overlap the MBus bridge window") > > Cc: <stable@vger.kernel.org> # v4.0+ > > applied on mvebu/fixes > > I hope that the 100 lines rules will be not a problem for this case It will not be, don't worry. thanks, greg k-h
diff --git a/drivers/bus/mvebu-mbus.c b/drivers/bus/mvebu-mbus.c index 7fa4510..6f047dc 100644 --- a/drivers/bus/mvebu-mbus.c +++ b/drivers/bus/mvebu-mbus.c @@ -58,7 +58,6 @@ #include <linux/debugfs.h> #include <linux/log2.h> #include <linux/syscore_ops.h> -#include <linux/memblock.h> /* * DDR target is the same on all platforms. @@ -103,9 +102,7 @@ /* Relative to mbusbridge_base */ #define MBUS_BRIDGE_CTRL_OFF 0x0 -#define MBUS_BRIDGE_SIZE_MASK 0xffff0000 #define MBUS_BRIDGE_BASE_OFF 0x4 -#define MBUS_BRIDGE_BASE_MASK 0xffff0000 /* Maximum number of windows, for all known platforms */ #define MBUS_WINS_MAX 20 @@ -579,106 +576,36 @@ static unsigned int armada_xp_mbus_win_remap_offset(int win) return MVEBU_MBUS_NO_REMAP; } -/* - * Use the memblock information to find the MBus bridge hole in the - * physical address space. - */ -static void __init -mvebu_mbus_find_bridge_hole(uint64_t *start, uint64_t *end) -{ - struct memblock_region *r; - uint64_t s = 0; - - for_each_memblock(memory, r) { - /* - * This part of the memory is above 4 GB, so we don't - * care for the MBus bridge hole. - */ - if (r->base >= 0x100000000) - continue; - - /* - * The MBus bridge hole is at the end of the RAM under - * the 4 GB limit. - */ - if (r->base + r->size > s) - s = r->base + r->size; - } - - *start = s; - *end = 0x100000000; -} - static void __init mvebu_mbus_default_setup_cpu_target(struct mvebu_mbus_state *mbus) { int i; int cs; - uint64_t mbus_bridge_base, mbus_bridge_end; mvebu_mbus_dram_info.mbus_dram_target_id = TARGET_DDR; - mvebu_mbus_find_bridge_hole(&mbus_bridge_base, &mbus_bridge_end); - for (i = 0, cs = 0; i < 4; i++) { - u64 base = readl(mbus->sdramwins_base + DDR_BASE_CS_OFF(i)); - u64 size = readl(mbus->sdramwins_base + DDR_SIZE_CS_OFF(i)); - u64 end; - struct mbus_dram_window *w; - - /* Ignore entries that are not enabled */ - if (!(size & DDR_SIZE_ENABLED)) - continue; - - /* - * Ignore entries whose base address is above 2^32, - * since devices cannot DMA to such high addresses - */ - if (base & DDR_BASE_CS_HIGH_MASK) - continue; - - base = base & DDR_BASE_CS_LOW_MASK; - size = (size | ~DDR_SIZE_MASK) + 1; - end = base + size; - - /* - * Adjust base/size of the current CS to make sure it - * doesn't overlap with the MBus bridge hole. This is - * particularly important for devices that do DMA from - * DRAM to a SRAM mapped in a MBus window, such as the - * CESA cryptographic engine. - */ + u32 base = readl(mbus->sdramwins_base + DDR_BASE_CS_OFF(i)); + u32 size = readl(mbus->sdramwins_base + DDR_SIZE_CS_OFF(i)); /* - * The CS is fully enclosed inside the MBus bridge - * area, so ignore it. + * We only take care of entries for which the chip + * select is enabled, and that don't have high base + * address bits set (devices can only access the first + * 32 bits of the memory). */ - if (base >= mbus_bridge_base && end <= mbus_bridge_end) - continue; + if ((size & DDR_SIZE_ENABLED) && + !(base & DDR_BASE_CS_HIGH_MASK)) { + struct mbus_dram_window *w; - /* - * Beginning of CS overlaps with end of MBus, raise CS - * base address, and shrink its size. - */ - if (base >= mbus_bridge_base && end > mbus_bridge_end) { - size -= mbus_bridge_end - base; - base = mbus_bridge_end; + w = &mvebu_mbus_dram_info.cs[cs++]; + w->cs_index = i; + w->mbus_attr = 0xf & ~(1 << i); + if (mbus->hw_io_coherency) + w->mbus_attr |= ATTR_HW_COHERENCY; + w->base = base & DDR_BASE_CS_LOW_MASK; + w->size = (size | ~DDR_SIZE_MASK) + 1; } - - /* - * End of CS overlaps with beginning of MBus, shrink - * CS size. - */ - if (base < mbus_bridge_base && end > mbus_bridge_base) - size -= end - mbus_bridge_base; - - w = &mvebu_mbus_dram_info.cs[cs++]; - w->cs_index = i; - w->mbus_attr = 0xf & ~(1 << i); - if (mbus->hw_io_coherency) - w->mbus_attr |= ATTR_HW_COHERENCY; - w->base = base; - w->size = size; } mvebu_mbus_dram_info.num_cs = cs; }
This reverts commit 1737cac69369 ("bus: mvebu-mbus: make sure SDRAM CS for DMA don't overlap the MBus bridge window"), because it breaks DMA on platforms having more than 2 GB of RAM. This commit changed the information reported to DMA masters device drivers through the mv_mbus_dram_info() function so that the returned DRAM ranges do not overlap with I/O windows. This was necessary as a preparation to support the new CESA Crypto Engine driver, which will use DMA for cryptographic operations. But since it does DMA with the SRAM which is mapped as an I/O window, having DRAM ranges overlapping with I/O windows was problematic. To solve this, the above mentioned commit changed the mvebu-mbus to adjust the DRAM ranges so that they don't overlap with the I/O windows. However, by doing this, we re-adjust the DRAM ranges in a way that makes them have a size that is no longer a power of two. While this is perfectly fine for the Crypto Engine, which supports DRAM ranges with a granularity of 64 KB, it breaks basically all other DMA masters, which expect power of two sizes for the DRAM ranges. Due to this, if the installed system memory is 4 GB, in two chip-selects of 2 GB, the second DRAM range will be reduced from 2 GB to a little bit less than 2 GB to not overlap with the I/O windows, in a way that results in a DRAM range that doesn't have a power of two size. This means that whenever you do a DMA transfer with an address located in the [ 2 GB ; 4 GB ] area, it will freeze the system. Any serious DMA activity like simply running: for i in $(seq 1 64) ; do dd if=/dev/urandom of=file$i bs=1M count=16 ; done in an ext3 partition mounted over a SATA drive will freeze the system. Since the new CESA crypto driver that uses DMA has not been merged yet, the easiest fix is to simply revert this commit. A follow-up commit will introduce a different solution for the CESA crypto driver. Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com> Fixes: 1737cac69369 ("bus: mvebu-mbus: make sure SDRAM CS for DMA don't overlap the MBus bridge window") Cc: <stable@vger.kernel.org> # v4.0+ --- drivers/bus/mvebu-mbus.c | 105 ++++++++--------------------------------------- 1 file changed, 16 insertions(+), 89 deletions(-)