| Message ID | fc2440c6f52877f28286d89691049e5cba10e6a7.1514087323.git.cyrille.pitchen@wedev4u.fr |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | mtd: spi-nor: fix DMA-unsafe buffer issue between MTD and SPI | expand |
Hi Cyrille, Thanks for doing this series! One comment below. On 24-Dec-17 10:06 AM, Cyrille Pitchen wrote: [...] > diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c > index 8bafd462f0ae..59f9fbd45234 100644 > --- a/drivers/mtd/spi-nor/spi-nor.c > +++ b/drivers/mtd/spi-nor/spi-nor.c > @@ -14,8 +14,10 @@ > #include <linux/errno.h> > #include <linux/module.h> > #include <linux/device.h> > +#include <linux/highmem.h> > #include <linux/mutex.h> > #include <linux/math64.h> > +#include <linux/mm.h> > #include <linux/sizes.h> > #include <linux/slab.h> > > @@ -1232,6 +1234,56 @@ static const struct flash_info spi_nor_ids[] = { > { }, > }; > > +static bool spi_nor_is_dma_safe(const void *buf) > +{ > + if (is_vmalloc_addr(buf)) > + return false; > + > +#ifdef CONFIG_HIGHMEM > + if ((unsigned long)buf >= PKMAP_BASE && > + (unsigned long)buf < (PKMAP_BASE + (LAST_PKMAP * PAGE_SIZE))) > + return false; > +#endif > + > + return true; > +} > + Better way would be to use virt_addr_valid(): static bool spi_nor_is_dma_safe(const void *buf) { return virt_addr_valid(buf); } Regards Vignesh
Hi Vignesh Le 26/12/2017 à 14:42, Vignesh R a écrit : > Hi Cyrille, > > Thanks for doing this series! One comment below. > > On 24-Dec-17 10:06 AM, Cyrille Pitchen wrote: > [...] >> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c >> index 8bafd462f0ae..59f9fbd45234 100644 >> --- a/drivers/mtd/spi-nor/spi-nor.c >> +++ b/drivers/mtd/spi-nor/spi-nor.c >> @@ -14,8 +14,10 @@ >> #include <linux/errno.h> >> #include <linux/module.h> >> #include <linux/device.h> >> +#include <linux/highmem.h> >> #include <linux/mutex.h> >> #include <linux/math64.h> >> +#include <linux/mm.h> >> #include <linux/sizes.h> >> #include <linux/slab.h> >> >> @@ -1232,6 +1234,56 @@ static const struct flash_info spi_nor_ids[] = { >> { }, >> }; >> >> +static bool spi_nor_is_dma_safe(const void *buf) >> +{ >> + if (is_vmalloc_addr(buf)) >> + return false; >> + >> +#ifdef CONFIG_HIGHMEM >> + if ((unsigned long)buf >= PKMAP_BASE && >> + (unsigned long)buf < (PKMAP_BASE + (LAST_PKMAP * PAGE_SIZE))) >> + return false; >> +#endif >> + >> + return true; >> +} >> + > > Better way would be to use virt_addr_valid(): > static bool spi_nor_is_dma_safe(const void *buf) > { > return virt_addr_valid(buf); > } > > Regards > Vignesh > Thanks for the advice :) https://patchwork.kernel.org/patch/9768341/ Maybe I could check both virt_addr_valid() and object_is_on_stack() too ? Best regards, Cyrille
On Sun, 2017-12-24 at 05:36 +0100, Cyrille Pitchen wrote: > > Then the patch adds two hardware capabilities for SPI flash controllers, > SNOR_HWCAPS_WR_BOUNCE and SNOR_HWCAPS_RD_BOUNCE. Are there any drivers for which a bounce buffer is NOT needed when the tx/rx buffer is not in DMA safe memory? Maybe it would make more sense to invert the sense of these flags, so that they indicate the driver does not need DMA safe buffers, if that is the uncommon/non-existent case, so that fewer drivers need to be modified to to be fixed? > +static bool spi_nor_is_dma_safe(const void *buf) > +{ > + if (is_vmalloc_addr(buf)) > + return false; > + > +#ifdef CONFIG_HIGHMEM > + if ((unsigned long)buf >= PKMAP_BASE && > + (unsigned long)buf < (PKMAP_BASE + (LAST_PKMAP * PAGE_SIZE))) > + return false; > +#endif It looks like: (unsigned long)addr >= PKMAP_ADDR(0) && (unsigned long)addr < PKMAP_ADDR(LAST_PKMAP) is the expression used in the highmem code. But really, isn't this begging for is_highmem_addr() in include/linux/mm.h that can always return false when highmem is not enabled? In order to be safe, this must be called when nor->lock is held, right? Otherwise it could race against two callers allocating the buffer at the same time. That should probably be noted in the kerneldoc comments for this function, which should also be written. > +static int spi_nor_get_bounce_buffer(struct spi_nor *nor, > + u_char **buffer, > + size_t *buffer_size) > +{ > + > + *buffer = nor->bounce_buffer; > + *buffer_size = size; So the buffer is returned via the parameter, and also via a field inside nor. Seems redundant. Consider address could be returned via the function return value coupled with PTR_ERR() for the error cases. Or not return address at all since it's available via nor- >bounce_buffer. > { > struct spi_nor *nor = mtd_to_spi_nor(mtd); > + bool use_bounce = (nor->flags & SNOR_F_USE_RD_BOUNCE) && > + !spi_nor_is_dma_safe(buf); > + u_char *buffer = buf; > + size_t buffer_size = 0; > int ret; > > dev_dbg(nor->dev, "from 0x%08x, len %zd\n", (u32)from, len); > @@ -1268,13 +1324,23 @@ static int spi_nor_read(struct mtd_info *mtd, loff_t from, size_t len, > if (ret) > return ret; > > + if (use_bounce) { > + ret = spi_nor_get_bounce_buffer(nor, &buffer, &buffer_size); > + if (ret < 0) > + goto read_err; > + } This pattern, check if bounce is enabled, check if address is dma- unsafe, get bounce buffer, seems to be very common. Could it be refactored into one helper? u_char *buffer = spi_nor_check_bounce(nor, buf, len, &buffer_size); if (IS_ERR(buffer)) return PTR_ERR(buffer); // buffer = nor->bounce_buffer or buf, whichever is correct // buffer_size = len or bounce buffer size, whichever is correct Could spi_nor_read_sfdp_dma_unsafe() also use this buffer?
Hi Trent, Le 26/12/2017 à 20:43, Trent Piepho a écrit : > On Sun, 2017-12-24 at 05:36 +0100, Cyrille Pitchen wrote: >> >> Then the patch adds two hardware capabilities for SPI flash controllers, >> SNOR_HWCAPS_WR_BOUNCE and SNOR_HWCAPS_RD_BOUNCE. > > Are there any drivers for which a bounce buffer is NOT needed when the > tx/rx buffer is not in DMA safe memory? Maybe it would make more sense > to invert the sense of these flags, so that they indicate the driver > does not need DMA safe buffers, if that is the uncommon/non-existent > case, so that fewer drivers need to be modified to to be fixed? > It doesn't sound safe for a first step. I don't know if some of the spi-flash controllers are embedded inside systems with small memory and don't care about DMA transfers. Maybe they don't plan to use anything else but PIO transfers. Then why taking the risk to exhaust the memory on systems that would not use the bounce buffer anyway? Later, if we see that most spi-flash drivers actually need this bounce buffer, I agree with you: we could invert the logic of the flags but currently I guess this is too soon and to be safe I would have to add the negative flags to all other spi-flash drivers. I would like a smooth and safe transition to avoid unexpected and unwanted side effects. About the memory loss when forcing the SNOR_HWCAPS_*_BOUNCE in m25p80.c, I justify it because the m25p80 has to be compliant with the SPI sub-system requirements but currently is not. However I've taken care not to allocate the bounce buffer as long as we use only DMA-safe buffers. Here at the MTD side, the main (only ?) source of DMA-unsafe buffers is the UBIFS (JFFS2 too ?) layer. Then I've assumed that systems using such a file-system should already have enough system memory. However I wanted to take all other use cases into account. >> +static bool spi_nor_is_dma_safe(const void *buf) >> +{ >> + if (is_vmalloc_addr(buf)) >> + return false; >> + >> +#ifdef CONFIG_HIGHMEM >> + if ((unsigned long)buf >= PKMAP_BASE && >> + (unsigned long)buf < (PKMAP_BASE + (LAST_PKMAP * PAGE_SIZE))) >> + return false; >> +#endif > > It looks like: > > (unsigned long)addr >= PKMAP_ADDR(0) && > (unsigned long)addr < PKMAP_ADDR(LAST_PKMAP) > > is the expression used in the highmem code. But really, isn't this > begging for is_highmem_addr() in include/linux/mm.h that can always > return false when highmem is not enabled? > Vignesh has suggested to call virt_addr_valid() instead. I think Boris has also told me about this function. So it might be the right solution. What do you think about their proposal? > In order to be safe, this must be called when nor->lock is held, right? Indeed, in all cases (spi_nor_read(), sst_write() and spi_nor_write()), spi_nor_get_bounce_buffer() is always called with nor->lock held, precisely to avoid races and allocating the bounce buffer twice by mistake. > Otherwise it could race against two callers allocating the buffer at > the same time. That should probably be noted in the kerneldoc comments > for this function, which should also be written. > I can add kernel-doc. >> +static int spi_nor_get_bounce_buffer(struct spi_nor *nor, >> + u_char **buffer, >> + size_t *buffer_size) >> +{ > >> + >> + *buffer = nor->bounce_buffer; >> + *buffer_size = size; > > So the buffer is returned via the parameter, and also via a field > inside nor. Seems redundant. Consider address could be returned via > the function return value coupled with PTR_ERR() for the error cases. > Or not return address at all since it's available via nor- >> bounce_buffer. > Why not. It would require more lines though. I guess it's purely a matter of taste. u_char *buffer = buf; if (use_bounce) { buffer = spi_nor_get_bounce_buffer(nor, &buffer_size); if (IS_ERR(buffer)) { err = PTR_ERR(buffer); goto read_err; } } >> { >> struct spi_nor *nor = mtd_to_spi_nor(mtd); >> + bool use_bounce = (nor->flags & SNOR_F_USE_RD_BOUNCE) && >> + !spi_nor_is_dma_safe(buf); >> + u_char *buffer = buf; >> + size_t buffer_size = 0; >> int ret; >> >> dev_dbg(nor->dev, "from 0x%08x, len %zd\n", (u32)from, len); >> @@ -1268,13 +1324,23 @@ static int spi_nor_read(struct mtd_info *mtd, loff_t from, size_t len, >> if (ret) >> return ret; >> >> + if (use_bounce) { >> + ret = spi_nor_get_bounce_buffer(nor, &buffer, &buffer_size); >> + if (ret < 0) >> + goto read_err; >> + } > > This pattern, check if bounce is enabled, check if address is dma- > unsafe, get bounce buffer, seems to be very common. Could it be > refactored into one helper? > > u_char *buffer = spi_nor_check_bounce(nor, buf, len, &buffer_size); The conditions that define the value of 'use_bounce' also depend on the type of operation, read or write, hence on the two different flags SNOR_F_USE_RD_BOUNCE and SNOR_F_USE_WR_BOUNCE. Besides, 'use_bounce' is also tested later in spi_nor_read(), sst_write() and sst_write(). So I don't really see how the spi_nor_check_bounce() function you propose could be that different from spi_nor_get_bounce_buffer(). > if (IS_ERR(buffer)) > return PTR_ERR(buffer); > // buffer = nor->bounce_buffer or buf, whichever is correct > // buffer_size = len or bounce buffer size, whichever is correct > > Could spi_nor_read_sfdp_dma_unsafe() also use this buffer? > I didn't use the bounce buffer in spi_nor_read_sfdp_dma_unsafe() on purpose: the bounce buffer, when needed, is allocated once for all to limit performance loss. However, to avoid increasing the memory footprint, if not absolutely needed the bounce buffer is not allocated at all. For a SFDP compliant memory, spi_nor_parse_sfdp() is always called during spi_nor_scan() even if later, only DMA-safe buffers are provided. In such a case, allocating the bounce buffer would have been a waste of memory. Best regards, Cyrille
On Thu, 2017-12-28 at 11:39 +0100, Cyrille Pitchen wrote: > Le 26/12/2017 à 20:43, Trent Piepho a écrit : > > On Sun, 2017-12-24 at 05:36 +0100, Cyrille Pitchen wrote: > > > > > > Then the patch adds two hardware capabilities for SPI flash controllers, > > > SNOR_HWCAPS_WR_BOUNCE and SNOR_HWCAPS_RD_BOUNCE. > > > > Are there any drivers for which a bounce buffer is NOT needed when the > > tx/rx buffer is not in DMA safe memory? Maybe it would make more sense > > to invert the sense of these flags, so that they indicate the driver > > does not need DMA safe buffers, if that is the uncommon/non-existent > > case, so that fewer drivers need to be modified to to be fixed? > > > > It doesn't sound safe for a first step. I don't know if some of the > spi-flash controllers are embedded inside systems with small memory and > don't care about DMA transfers. Maybe they don't plan to use anything else > but PIO transfers. Then why taking the risk to exhaust the memory on systems > that would not use the bounce buffer anyway? This would certainly be the case when the driver does not even support DMA in the first place. This also makes me wonder, how inefficient does this become when it uses a bounce buffer for small transfer that would not use DMA anyway? In the spi_flash_read() interface for spi masters, there is a master method spi_flash_can_dma() callback used by the spi core to tell if each transfer can be DMAed. Should something like that be used here, to ask the master if it would use dma on the buffer? This might also prevent allocation of the bounce buffer if the only DMA unsafe transfers are tiny control ops with stack variables that wouldn't use DMA, e.g. the stuff spi_nor_read_sfdp_dma_unsafe() does. > About the memory loss when forcing the SNOR_HWCAPS_*_BOUNCE in m25p80.c, I > justify it because the m25p80 has to be compliant with the SPI sub-system > requirements but currently is not. However I've taken care not to allocate > the bounce buffer as long as we use only DMA-safe buffers. Another possibility to reduce memory usage: make the buffer smaller when first allocated by being just enough for the needed space. Grow it (by powers of two?) until it reaches the max allowed size. No reason to use a 256 kB buffer if DMA unsafe operations never get that big. > Here at the MTD side, the main (only ?) source of DMA-unsafe buffers is > the UBIFS (JFFS2 too ?) layer. Then I've assumed that systems using such a > file-system should already have enough system memory. I saw a note in one of the existing DMA fixes that JFFS2 was the source of the unsafe buffers, so probably there too. > > Vignesh has suggested to call virt_addr_valid() instead. > I think Boris has also told me about this function. > So it might be the right solution. What do you think about their proposal? Not sure what exactly the differences are between these methods. The fact that each of the many existing DMA fixes uses slightly different code to detect what is unsafe speaks to the difficulty of this problem! virt_addr_valid() is already used by spi-ti-qspi. spi core uses for the buffer map helper, but that code path is for buffers which are NOT vmalloc or highmem, but are still not virt_addr_valid() for some other reason. > > > +static int spi_nor_get_bounce_buffer(struct spi_nor *nor, > > > + u_char **buffer, > > > + size_t *buffer_size) > > > +{ > > > + > > > + *buffer = nor->bounce_buffer; > > > + *buffer_size = size; > > > > So the buffer is returned via the parameter, and also via a field > > inside nor. Seems redundant. Consider address could be returned via > > the function return value coupled with PTR_ERR() for the error cases. > > Or not return address at all since it's available via nor- > > > bounce_buffer. > > Why not. It would require more lines though. I guess it's purely a matter of taste. Well, also consider that you don't need to even return the buffer pointer at all, since it's available as nor->bounce_buffer. Which it is used as in spi_nor_write() and spi_nor_read(). > > This pattern, check if bounce is enabled, check if address is dma- > > unsafe, get bounce buffer, seems to be very common. Could it be > > refactored into one helper? > > > > u_char *buffer = spi_nor_check_bounce(nor, buf, len, &buffer_size); > > The conditions that define the value of 'use_bounce' also depend on the type > of operation, read or write, hence on the two different flags > SNOR_F_USE_RD_BOUNCE and SNOR_F_USE_WR_BOUNCE. Just pass one of those flags as an argument to tell what direction it is in. Though I wonder if using a bounce buffer for only one direction will ever be used. > > Besides, 'use_bounce' is also tested later in spi_nor_read(), sst_write() > and sst_write(). > > So I don't really see how the spi_nor_check_bounce() function you propose > could be that different from spi_nor_get_bounce_buffer(). > > > if (IS_ERR(buffer)) > > return PTR_ERR(buffer); > > // buffer = nor->bounce_buffer or buf, whichever is correct > > // buffer_size = len or bounce buffer size, whichever is correct > > > > Could spi_nor_read_sfdp_dma_unsafe() also use this buffer? > > > > I didn't use the bounce buffer in spi_nor_read_sfdp_dma_unsafe() on > purpose: the bounce buffer, when needed, is allocated once for all to limit > performance loss. However, to avoid increasing the memory footprint, if not > absolutely needed the bounce buffer is not allocated at all. spi-nor tries to provide a common implementation of DMA bounce buffers, yet spi-nor itself has two different DMA bounce buffer implementations. I think the real answer for spi_nor_read_sfdp_dma_unsafe() is that it shouldn't be written that way and the function should go away. The two call sites should just kmalloc the struct they read into instead of placing it on the stack. The dma_unsafe wrapper kmallocs a buffer anyway, so it's not like there is any more allocation going on.
On Friday 29 December 2017 12:24 AM, Trent Piepho wrote: > On Thu, 2017-12-28 at 11:39 +0100, Cyrille Pitchen wrote: >> Le 26/12/2017 à 20:43, Trent Piepho a écrit : >> > On Sun, 2017-12-24 at 05:36 +0100, Cyrille Pitchen wrote: >> > > >> > > Then the patch adds two hardware capabilities for SPI flash controllers, >> > > SNOR_HWCAPS_WR_BOUNCE and SNOR_HWCAPS_RD_BOUNCE. >> > >> > Are there any drivers for which a bounce buffer is NOT needed when the >> > tx/rx buffer is not in DMA safe memory? Maybe it would make more sense >> > to invert the sense of these flags, so that they indicate the driver >> > does not need DMA safe buffers, if that is the uncommon/non-existent >> > case, so that fewer drivers need to be modified to to be fixed? >> > >> >> It doesn't sound safe for a first step. I don't know if some of the >> spi-flash controllers are embedded inside systems with small memory and >> don't care about DMA transfers. Maybe they don't plan to use anything else >> but PIO transfers. Then why taking the risk to exhaust the memory on systems >> that would not use the bounce buffer anyway? > > This would certainly be the case when the driver does not even support > DMA in the first place. > > This also makes me wonder, how inefficient does this become when it > uses a bounce buffer for small transfer that would not use DMA anyway? > In the spi_flash_read() interface for spi masters, there is a master > method spi_flash_can_dma() callback used by the spi core to tell if > each transfer can be DMAed. > > Should something like that be used here, to ask the master if it would > use dma on the buffer? > > This might also prevent allocation of the bounce buffer if the only DMA > unsafe transfers are tiny control ops with stack variables that > wouldn't use DMA, e.g. the stuff spi_nor_read_sfdp_dma_unsafe() does. > > >> About the memory loss when forcing the SNOR_HWCAPS_*_BOUNCE in m25p80.c, I >> justify it because the m25p80 has to be compliant with the SPI sub-system >> requirements but currently is not. However I've taken care not to allocate >> the bounce buffer as long as we use only DMA-safe buffers. > > Another possibility to reduce memory usage: make the buffer smaller > when first allocated by being just enough for the needed space. Grow > it (by powers of two?) until it reaches the max allowed size. No > reason to use a 256 kB buffer if DMA unsafe operations never get that > big. > > >> Here at the MTD side, the main (only ?) source of DMA-unsafe buffers is >> the UBIFS (JFFS2 too ?) layer. Then I've assumed that systems using such a >> file-system should already have enough system memory. > > I saw a note in one of the existing DMA fixes that JFFS2 was the source > of the unsafe buffers, so probably there too. > > >> >> Vignesh has suggested to call virt_addr_valid() instead. >> I think Boris has also told me about this function. >> So it might be the right solution. What do you think about their proposal? > > Not sure what exactly the differences are between these methods. The > fact that each of the many existing DMA fixes uses slightly different > code to detect what is unsafe speaks to the difficulty of this problem! My understanding based on Documentation/DMA-API-HOWTO.txt and Documentation/arm/memory.txt is that virt_addr_valid() will guarantee that address is in range of PAGE_OFFSET to high_memory-1 (Kernel direct-mapped RAM region) which is address range of buffers that are DMA'able. > virt_addr_valid() is already used by spi-ti-qspi. spi core uses for > the buffer map helper, but that code path is for buffers which are NOT > vmalloc or highmem, but are still not virt_addr_valid() for some other > reason. > if (vmalloced_buf || kmap_buf) { /* Handle vmalloc'd or kmap'd buffers */ ... } else if (virt_addr_valid(buf)) { /* Handle kmalloc'd and such buffers */ ... } else { /* Error if none of the above */ return -EINVAL; }
On Fri, 2017-12-29 at 15:46 +0530, Vignesh R wrote: > On Friday 29 December 2017 12:24 AM, Trent Piepho wrote: > > > > > Vignesh has suggested to call virt_addr_valid() instead. > > > I think Boris has also told me about this function. > > > So it might be the right solution. What do you think about their proposal? > > > > Not sure what exactly the differences are between these methods. The > > fact that each of the many existing DMA fixes uses slightly different > > code to detect what is unsafe speaks to the difficulty of this problem! > > My understanding based on Documentation/DMA-API-HOWTO.txt and > Documentation/arm/memory.txt is that > virt_addr_valid() will guarantee that address is in range of > PAGE_OFFSET to high_memory-1 (Kernel direct-mapped RAM region) which is > address range of buffers that are DMA'able. There's code in gpmi-nand.c that does: /* first try to map the upper buffer directly */ if (virt_addr_valid(this->upper_buf) && !object_is_on_stack(this->upper_buf)) { sg_init_one(sgl, this->upper_buf, this->upper_len); So whoever wrote that thought that stack objects needed an additional test beyond virt_addr_valid. But it does appear to be far more common to depend on just virt_addr_valid, so perhaps the code in gpmi-nand is in error. > > virt_addr_valid() is already used by spi-ti-qspi. spi core uses for > > the buffer map helper, but that code path is for buffers which are NOT > > vmalloc or highmem, but are still not virt_addr_valid() for some other > > reason. > > > > if (vmalloced_buf || kmap_buf) { > /* Handle vmalloc'd or kmap'd buffers */ > ... This stuff does get DMAed. So I have to wonder, if spi.c thinks it can use DMA with vmalloc or highmem, couldn't spi-not do the same instead of the bounce buffer? > } else if (virt_addr_valid(buf)) { > /* Handle kmalloc'd and such buffers */ > ... > } else { > /* Error if none of the above */ So what is this case here for? It's some class that does not have a valid virtual address and yet is not vmalloc or highmem. > return -EINVAL; > } >
On Friday 29 December 2017 11:33 PM, Trent Piepho wrote: > On Fri, 2017-12-29 at 15:46 +0530, Vignesh R wrote: >> On Friday 29 December 2017 12:24 AM, Trent Piepho wrote: >> > >> > > Vignesh has suggested to call virt_addr_valid() instead. >> > > I think Boris has also told me about this function. >> > > So it might be the right solution. What do you think about their proposal? >> > >> > Not sure what exactly the differences are between these methods. The >> > fact that each of the many existing DMA fixes uses slightly different >> > code to detect what is unsafe speaks to the difficulty of this problem! >> >> My understanding based on Documentation/DMA-API-HOWTO.txt and >> Documentation/arm/memory.txt is that >> virt_addr_valid() will guarantee that address is in range of >> PAGE_OFFSET to high_memory-1 (Kernel direct-mapped RAM region) which is >> address range of buffers that are DMA'able. > > There's code in gpmi-nand.c that does: > > /* first try to map the upper buffer directly */ > if (virt_addr_valid(this->upper_buf) && > !object_is_on_stack(this->upper_buf)) { > sg_init_one(sgl, this->upper_buf, this->upper_len); > > So whoever wrote that thought that stack objects needed an additional > test beyond virt_addr_valid. But it does appear to be far more common > to depend on just virt_addr_valid, so perhaps the code in gpmi-nand is > in error. > The test in gpmi-nand.c is right. Enabling CONFIG_DMA_API_DEBUG does warn about DMA'ing into stack objects (in lib/dma-debug.c). So other places needs to be fixed, I guess. >> > virt_addr_valid() is already used by spi-ti-qspi. spi core uses for >> > the buffer map helper, but that code path is for buffers which are NOT >> > vmalloc or highmem, but are still not virt_addr_valid() for some other >> > reason. >> > >> >> if (vmalloced_buf || kmap_buf) { >> /* Handle vmalloc'd or kmap'd buffers */ >> ... > This stuff does get DMAed. So I have to wonder, if spi.c thinks it can > use DMA with vmalloc or highmem, couldn't spi-not do the same instead > of the bounce buffer? SPI core does try to DMA into underlying physical pages of vmalloc'd buffer. But this has two problems: 1. Does not work well with VIVT caches[1]. 2. On ARM LPAE systems, vmalloc'd buffers can be from highmem region that are not addressable using 32 bit addresses and is backed by LPAE. So, a 32 bit DMA cannot access these buffers. Both these issues lead to random crashes and errors with UBIFS and JFFS2 flash file systems which this patch series tries to address using bounce buffer [1] https://patchwork.kernel.org/patch/9579553/
On Tue, 26 Dec 2017 14:59:00 +0100 Cyrille Pitchen <cyrille.pitchen@wedev4u.fr> wrote: > Hi Vignesh > > Le 26/12/2017 à 14:42, Vignesh R a écrit : > > Hi Cyrille, > > > > Thanks for doing this series! One comment below. > > > > On 24-Dec-17 10:06 AM, Cyrille Pitchen wrote: > > [...] > >> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c > >> index 8bafd462f0ae..59f9fbd45234 100644 > >> --- a/drivers/mtd/spi-nor/spi-nor.c > >> +++ b/drivers/mtd/spi-nor/spi-nor.c > >> @@ -14,8 +14,10 @@ > >> #include <linux/errno.h> > >> #include <linux/module.h> > >> #include <linux/device.h> > >> +#include <linux/highmem.h> > >> #include <linux/mutex.h> > >> #include <linux/math64.h> > >> +#include <linux/mm.h> > >> #include <linux/sizes.h> > >> #include <linux/slab.h> > >> > >> @@ -1232,6 +1234,56 @@ static const struct flash_info spi_nor_ids[] = { > >> { }, > >> }; > >> > >> +static bool spi_nor_is_dma_safe(const void *buf) > >> +{ > >> + if (is_vmalloc_addr(buf)) > >> + return false; > >> + > >> +#ifdef CONFIG_HIGHMEM > >> + if ((unsigned long)buf >= PKMAP_BASE && > >> + (unsigned long)buf < (PKMAP_BASE + (LAST_PKMAP * PAGE_SIZE))) > >> + return false; > >> +#endif > >> + > >> + return true; > >> +} > >> + > > > > Better way would be to use virt_addr_valid(): > > static bool spi_nor_is_dma_safe(const void *buf) > > { > > return virt_addr_valid(buf); > > } > > > > Regards > > Vignesh > > > > Thanks for the advice :) > > https://patchwork.kernel.org/patch/9768341/ > Maybe I could check both virt_addr_valid() and object_is_on_stack() too ? Yep, see the explanation given here [1]. [1]http://elixir.free-electrons.com/linux/v4.15-rc6/source/Documentation/DMA-API-HOWTO.txt#L132
Hi Cyrille, On Sun, 24 Dec 2017 05:36:04 +0100 Cyrille Pitchen <cyrille.pitchen@wedev4u.fr> wrote: > This patch has two purposes: > > 1 - To fix the compatible issue between the MTD and SPI sub-systems > > The MTD sub-system has no particular requirement about the memory areas it > uses. Especially, ubifs is well known for using vmalloc'ed buffers, which > then are not DMA-safe. There are reasons behind that, so we have to deal > with it. Well, the only reason is that it's easier to deal with virtually contiguous memory region, and since the LEB/PEB size can be quite big (especially for NAND devices) we have to allocate it with vmalloc() to prevent memory fragmentation. The solution would be to allocate an array of ubi->min_io_size buffers using kzalloc() and write/read to/from the MTD device using this granularity, but this approach would require quite a few changes and that's not the topic here. > > On the other hand, the SPI sub-system clearly states in the kernel doc for > 'struct spi-transfer' (include/linux/spi/spi.h) that both .tx_buf and > .rx_buf must point into "dma-safe memory". This requirement has not been > taken into account by the m25p80.c driver, at the border between MTD and > SPI, for a long time now. So it's high time to fix this issue. I agree, even if I guess the MTD layer is not the only offender and having this bounce buffer logic at the SPI level would be even better IMO. But let's solve the problem in the SPI-NOR layer for now. > > 2 - To help other SPI flash controller drivers to perform DMA transfers > > Those controller drivers suffer the same issue as those behind the > m25p80.c driver in the SPI sub-system: They may be provided by the MTD > sub-system with buffers not suited to DMA operations. We want to avoid > each controller to implement its own bounce buffer so we offer them some > optional bounce buffer, allocated and managed by the spi-nor framework > itself, as an helper to add support to DMA transfers. > > Then the patch adds two hardware capabilities for SPI flash controllers, > SNOR_HWCAPS_WR_BOUNCE and SNOR_HWCAPS_RD_BOUNCE. Do you have good reasons to handle the read/write path independently? I mean, if you need a bounce buffer for one of them it's likely that you'll need it for both. > > SPI flash controller drivers are supposed to use them to request the > spi-nor framework to allocate an optional bounce buffer in some > DMA-safe memory area then to use it in some cases during (Fast) Read > and/or Page Program operations. > > More precisely, the bounce buffer is used if and only if two conditions > are met: > 1 - The SPI flash controller driver has declared the > SNOR_HWCAPS_RD_BOUNCE, resp. SNOR_HWCAPS_WR_BOUNCE for (Fast) Read, > resp. Page Program operations. > 2 - The buffer provided by the above MTD layer is not already in a > DMA-safe area. > > This policy avoid using the bounce buffer when not explicitly requested > or when not needed, hence limiting the performance penalty. > > Besides, the bounce buffer is allocated once for all at the very first > time it is actually needed. Hm, I think it would be better to allocate the buffer at detection/probe time, when you have all the information you need (sector_size?). My fear is that you might not be able to kmalloc() a large buffer the first time a read/write operation is performed, and that means the operation might randomly fail/succeed depending on when the first IO operation is done. If you do it at probe time, you'll be able to detect the allocation failure early and stop the MTD dev registration when this is the case. > This means that as long as all buffers > provided by the above MTD layer are allocated in some DMA-safe areas, the > bounce buffer itself is never allocated. > > Finally, the bounce buffer size is limited to 256KiB, the currently known > maximum erase sector size. This tradeoff should still provide good > performances even if new memory parts come with even larger erase sector > sizes, limiting the memory footprint at the same time. > > Signed-off-by: Cyrille Pitchen <cyrille.pitchen@wedev4u.fr> > --- > drivers/mtd/devices/m25p80.c | 4 +- > drivers/mtd/spi-nor/spi-nor.c | 136 +++++++++++++++++++++++++++++++++++++++--- > include/linux/mtd/spi-nor.h | 8 +++ > 3 files changed, 139 insertions(+), 9 deletions(-) > > diff --git a/drivers/mtd/devices/m25p80.c b/drivers/mtd/devices/m25p80.c > index a4e18f6aaa33..60878c62a654 100644 > --- a/drivers/mtd/devices/m25p80.c > +++ b/drivers/mtd/devices/m25p80.c > @@ -239,7 +239,9 @@ static int m25p_probe(struct spi_device *spi) > struct spi_nor_hwcaps hwcaps = { > .mask = SNOR_HWCAPS_READ | > SNOR_HWCAPS_READ_FAST | > - SNOR_HWCAPS_PP, > + SNOR_HWCAPS_PP | > + SNOR_HWCAPS_RD_BOUNCE | > + SNOR_HWCAPS_WR_BOUNCE, > }; > char *flash_name; > int ret; > diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c > index 8bafd462f0ae..59f9fbd45234 100644 > --- a/drivers/mtd/spi-nor/spi-nor.c > +++ b/drivers/mtd/spi-nor/spi-nor.c > @@ -14,8 +14,10 @@ > #include <linux/errno.h> > #include <linux/module.h> > #include <linux/device.h> > +#include <linux/highmem.h> > #include <linux/mutex.h> > #include <linux/math64.h> > +#include <linux/mm.h> > #include <linux/sizes.h> > #include <linux/slab.h> > > @@ -1232,6 +1234,56 @@ static const struct flash_info spi_nor_ids[] = { > { }, > }; > > +static bool spi_nor_is_dma_safe(const void *buf) > +{ > + if (is_vmalloc_addr(buf)) > + return false; > + > +#ifdef CONFIG_HIGHMEM > + if ((unsigned long)buf >= PKMAP_BASE && > + (unsigned long)buf < (PKMAP_BASE + (LAST_PKMAP * PAGE_SIZE))) > + return false; > +#endif > + > + return true; > +} > + > +static int spi_nor_get_bounce_buffer(struct spi_nor *nor, > + u_char **buffer, > + size_t *buffer_size) > +{ > + const struct flash_info *info = nor->info; > + /* > + * Limit the size of the bounce buffer to 256KB: this is currently > + * the largest known erase sector size (> page size) and should be > + * enough to still reach good performances if some day new memory > + * parts use even larger erase sector sizes. > + */ > + size_t size = min_t(size_t, info->sector_size, SZ_256K); Wow! That's a huge max size for a buffer allocated with kmalloc. Are you sure you shouldn't shrink it a bit? Don't know what the usual sector_size is, but AFAIU sector_size is the ->erasesize, and read/write operations are done at a ->writesize or ->writebufsize granularity. > + > + /* > + * Allocate the bounce buffer once for all at the first time it is > + * actually needed. This prevents wasting some precious memory > + * in cases where it would never be needed. > + */ > + if (unlikely(!nor->bounce_buffer)) { I've been told that unlikely/likely() specifiers are not so useful these days and are sometime doing worse than when nothing is specified. I must admit I never went as far as evaluating it myself, but I don't think it's really needed here (the time spent doing this check is likely to be negligible compared to the IO operation). > + nor->bounce_buffer = devm_kmalloc(nor->dev, size, GFP_KERNEL); Nope, devm_kmalloc() is not DMA-safe (see this thread [1]). > + > + /* > + * The SPI flash controller driver has required and expects to > + * use the DMA-safe bounce buffer, so we can't recover from > + * this allocation failure. > + */ > + if (!nor->bounce_buffer) > + return -ENOMEM; > + } > + > + *buffer = nor->bounce_buffer; > + *buffer_size = size; > + > + return 0; > +} > + Regards, Boris [1]http://linux-arm-kernel.infradead.narkive.com/vyJqy0RQ/question-devm-kmalloc-for-dma
diff --git a/drivers/mtd/devices/m25p80.c b/drivers/mtd/devices/m25p80.c index a4e18f6aaa33..60878c62a654 100644 --- a/drivers/mtd/devices/m25p80.c +++ b/drivers/mtd/devices/m25p80.c @@ -239,7 +239,9 @@ static int m25p_probe(struct spi_device *spi) struct spi_nor_hwcaps hwcaps = { .mask = SNOR_HWCAPS_READ | SNOR_HWCAPS_READ_FAST | - SNOR_HWCAPS_PP, + SNOR_HWCAPS_PP | + SNOR_HWCAPS_RD_BOUNCE | + SNOR_HWCAPS_WR_BOUNCE, }; char *flash_name; int ret; diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c index 8bafd462f0ae..59f9fbd45234 100644 --- a/drivers/mtd/spi-nor/spi-nor.c +++ b/drivers/mtd/spi-nor/spi-nor.c @@ -14,8 +14,10 @@ #include <linux/errno.h> #include <linux/module.h> #include <linux/device.h> +#include <linux/highmem.h> #include <linux/mutex.h> #include <linux/math64.h> +#include <linux/mm.h> #include <linux/sizes.h> #include <linux/slab.h> @@ -1232,6 +1234,56 @@ static const struct flash_info spi_nor_ids[] = { { }, }; +static bool spi_nor_is_dma_safe(const void *buf) +{ + if (is_vmalloc_addr(buf)) + return false; + +#ifdef CONFIG_HIGHMEM + if ((unsigned long)buf >= PKMAP_BASE && + (unsigned long)buf < (PKMAP_BASE + (LAST_PKMAP * PAGE_SIZE))) + return false; +#endif + + return true; +} + +static int spi_nor_get_bounce_buffer(struct spi_nor *nor, + u_char **buffer, + size_t *buffer_size) +{ + const struct flash_info *info = nor->info; + /* + * Limit the size of the bounce buffer to 256KB: this is currently + * the largest known erase sector size (> page size) and should be + * enough to still reach good performances if some day new memory + * parts use even larger erase sector sizes. + */ + size_t size = min_t(size_t, info->sector_size, SZ_256K); + + /* + * Allocate the bounce buffer once for all at the first time it is + * actually needed. This prevents wasting some precious memory + * in cases where it would never be needed. + */ + if (unlikely(!nor->bounce_buffer)) { + nor->bounce_buffer = devm_kmalloc(nor->dev, size, GFP_KERNEL); + + /* + * The SPI flash controller driver has required and expects to + * use the DMA-safe bounce buffer, so we can't recover from + * this allocation failure. + */ + if (!nor->bounce_buffer) + return -ENOMEM; + } + + *buffer = nor->bounce_buffer; + *buffer_size = size; + + return 0; +} + static const struct flash_info *spi_nor_read_id(struct spi_nor *nor) { int tmp; @@ -1260,6 +1312,10 @@ static int spi_nor_read(struct mtd_info *mtd, loff_t from, size_t len, size_t *retlen, u_char *buf) { struct spi_nor *nor = mtd_to_spi_nor(mtd); + bool use_bounce = (nor->flags & SNOR_F_USE_RD_BOUNCE) && + !spi_nor_is_dma_safe(buf); + u_char *buffer = buf; + size_t buffer_size = 0; int ret; dev_dbg(nor->dev, "from 0x%08x, len %zd\n", (u32)from, len); @@ -1268,13 +1324,23 @@ static int spi_nor_read(struct mtd_info *mtd, loff_t from, size_t len, if (ret) return ret; + if (use_bounce) { + ret = spi_nor_get_bounce_buffer(nor, &buffer, &buffer_size); + if (ret < 0) + goto read_err; + } + while (len) { loff_t addr = from; + size_t length = len; if (nor->flags & SNOR_F_S3AN_ADDR_DEFAULT) addr = spi_nor_s3an_addr_convert(nor, addr); - ret = nor->read(nor, addr, len, buf); + if (use_bounce && length > buffer_size) + length = buffer_size; + + ret = nor->read(nor, addr, length, buffer); if (ret == 0) { /* We shouldn't see 0-length reads */ ret = -EIO; @@ -1283,7 +1349,11 @@ static int spi_nor_read(struct mtd_info *mtd, loff_t from, size_t len, if (ret < 0) goto read_err; - WARN_ON(ret > len); + WARN_ON(ret > length); + if (use_bounce) + memcpy(buf, nor->bounce_buffer, ret); + else + buffer += ret; *retlen += ret; buf += ret; from += ret; @@ -1300,7 +1370,10 @@ static int sst_write(struct mtd_info *mtd, loff_t to, size_t len, size_t *retlen, const u_char *buf) { struct spi_nor *nor = mtd_to_spi_nor(mtd); - size_t actual; + bool use_bounce = (nor->flags & SNOR_F_USE_WR_BOUNCE) && + !spi_nor_is_dma_safe(buf); + u_char *buffer = NULL; + size_t actual = 0, buffer_size = 0; int ret; dev_dbg(nor->dev, "to 0x%08x, len %zd\n", (u32)to, len); @@ -1309,6 +1382,12 @@ static int sst_write(struct mtd_info *mtd, loff_t to, size_t len, if (ret) return ret; + if (use_bounce) { + ret = spi_nor_get_bounce_buffer(nor, &buffer, &buffer_size); + if (ret < 0) + goto sst_write_err; + } + write_enable(nor); nor->sst_write_second = false; @@ -1318,8 +1397,13 @@ static int sst_write(struct mtd_info *mtd, loff_t to, size_t len, if (actual) { nor->program_opcode = SPINOR_OP_BP; + if (use_bounce) + buffer[0] = buf[0]; + else + buffer = (u_char *)buf; + /* write one byte. */ - ret = nor->write(nor, to, 1, buf); + ret = nor->write(nor, to, 1, buffer); if (ret < 0) goto sst_write_err; WARN(ret != 1, "While writing 1 byte written %i bytes\n", @@ -1334,8 +1418,15 @@ static int sst_write(struct mtd_info *mtd, loff_t to, size_t len, for (; actual < len - 1; actual += 2) { nor->program_opcode = SPINOR_OP_AAI_WP; + if (use_bounce) { + buffer[0] = buf[actual]; + buffer[1] = buf[actual + 1]; + } else { + buffer = (u_char *)buf + actual; + } + /* write two bytes. */ - ret = nor->write(nor, to, 2, buf + actual); + ret = nor->write(nor, to, 2, buffer); if (ret < 0) goto sst_write_err; WARN(ret != 2, "While writing 2 bytes written %i bytes\n", @@ -1358,7 +1449,13 @@ static int sst_write(struct mtd_info *mtd, loff_t to, size_t len, write_enable(nor); nor->program_opcode = SPINOR_OP_BP; - ret = nor->write(nor, to, 1, buf + actual); + + if (use_bounce) + buffer[0] = buf[actual]; + else + buffer = (u_char *)buf + actual; + + ret = nor->write(nor, to, 1, buffer); if (ret < 0) goto sst_write_err; WARN(ret != 1, "While writing 1 byte written %i bytes\n", @@ -1384,7 +1481,10 @@ static int spi_nor_write(struct mtd_info *mtd, loff_t to, size_t len, size_t *retlen, const u_char *buf) { struct spi_nor *nor = mtd_to_spi_nor(mtd); - size_t page_offset, page_remain, i; + bool use_bounce = (nor->flags & SNOR_F_USE_WR_BOUNCE) && + !spi_nor_is_dma_safe(buf); + u_char *buffer = NULL; + size_t page_offset, page_remain, i, buffer_size = 0; ssize_t ret; dev_dbg(nor->dev, "to 0x%08x, len %zd\n", (u32)to, len); @@ -1393,6 +1493,12 @@ static int spi_nor_write(struct mtd_info *mtd, loff_t to, size_t len, if (ret) return ret; + if (use_bounce) { + ret = spi_nor_get_bounce_buffer(nor, &buffer, &buffer_size); + if (ret < 0) + goto write_err; + } + for (i = 0; i < len; ) { ssize_t written; loff_t addr = to + i; @@ -1419,8 +1525,17 @@ static int spi_nor_write(struct mtd_info *mtd, loff_t to, size_t len, if (nor->flags & SNOR_F_S3AN_ADDR_DEFAULT) addr = spi_nor_s3an_addr_convert(nor, addr); + if (use_bounce) { + if (page_remain > buffer_size) + page_remain = buffer_size; + + memcpy(nor->bounce_buffer, buf + i, page_remain); + } else { + buffer = (u_char *)buf + i; + } + write_enable(nor); - ret = nor->write(nor, addr, page_remain, buf + i); + ret = nor->write(nor, addr, page_remain, buffer); if (ret < 0) goto write_err; written = ret; @@ -2814,6 +2929,11 @@ int spi_nor_scan(struct spi_nor *nor, const char *name, if (info->flags & SPI_S3AN) nor->flags |= SNOR_F_READY_XSR_RDY; + if (hwcaps->mask & SNOR_HWCAPS_RD_BOUNCE) + nor->flags |= SNOR_F_USE_RD_BOUNCE; + if (hwcaps->mask & SNOR_HWCAPS_WR_BOUNCE) + nor->flags |= SNOR_F_USE_WR_BOUNCE; + /* Parse the Serial Flash Discoverable Parameters table. */ ret = spi_nor_init_params(nor, info, ¶ms); if (ret) diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h index de36969eb359..9f4218990fc7 100644 --- a/include/linux/mtd/spi-nor.h +++ b/include/linux/mtd/spi-nor.h @@ -233,6 +233,8 @@ enum spi_nor_option_flags { SNOR_F_S3AN_ADDR_DEFAULT = BIT(3), SNOR_F_READY_XSR_RDY = BIT(4), SNOR_F_USE_CLSR = BIT(5), + SNOR_F_USE_RD_BOUNCE = BIT(6), + SNOR_F_USE_WR_BOUNCE = BIT(7), }; /** @@ -259,6 +261,7 @@ struct flash_info; * @write_proto: the SPI protocol for write operations * @reg_proto the SPI protocol for read_reg/write_reg/erase operations * @cmd_buf: used by the write_reg + * @bounce_buffer: an optional kmalloc'ed buffer as DMA transfer helper * @prepare: [OPTIONAL] do some preparations for the * read/write/erase/lock/unlock operations * @unprepare: [OPTIONAL] do some post work after the @@ -294,6 +297,7 @@ struct spi_nor { bool sst_write_second; u32 flags; u8 cmd_buf[SPI_NOR_MAX_CMD_SIZE]; + void *bounce_buffer; int (*prepare)(struct spi_nor *nor, enum spi_nor_ops ops); void (*unprepare)(struct spi_nor *nor, enum spi_nor_ops ops); @@ -386,6 +390,10 @@ struct spi_nor_hwcaps { #define SNOR_HWCAPS_PP_1_8_8 BIT(21) #define SNOR_HWCAPS_PP_8_8_8 BIT(22) +/* Bounce buffer helper, for DMA transfer for instance. */ +#define SNOR_HWCAPS_WR_BOUNCE BIT(30) +#define SNOR_HWCAPS_RD_BOUNCE BIT(31) + /** * spi_nor_scan() - scan the SPI NOR * @nor: the spi_nor structure
This patch has two purposes: 1 - To fix the compatible issue between the MTD and SPI sub-systems The MTD sub-system has no particular requirement about the memory areas it uses. Especially, ubifs is well known for using vmalloc'ed buffers, which then are not DMA-safe. There are reasons behind that, so we have to deal with it. On the other hand, the SPI sub-system clearly states in the kernel doc for 'struct spi-transfer' (include/linux/spi/spi.h) that both .tx_buf and .rx_buf must point into "dma-safe memory". This requirement has not been taken into account by the m25p80.c driver, at the border between MTD and SPI, for a long time now. So it's high time to fix this issue. 2 - To help other SPI flash controller drivers to perform DMA transfers Those controller drivers suffer the same issue as those behind the m25p80.c driver in the SPI sub-system: They may be provided by the MTD sub-system with buffers not suited to DMA operations. We want to avoid each controller to implement its own bounce buffer so we offer them some optional bounce buffer, allocated and managed by the spi-nor framework itself, as an helper to add support to DMA transfers. Then the patch adds two hardware capabilities for SPI flash controllers, SNOR_HWCAPS_WR_BOUNCE and SNOR_HWCAPS_RD_BOUNCE. SPI flash controller drivers are supposed to use them to request the spi-nor framework to allocate an optional bounce buffer in some DMA-safe memory area then to use it in some cases during (Fast) Read and/or Page Program operations. More precisely, the bounce buffer is used if and only if two conditions are met: 1 - The SPI flash controller driver has declared the SNOR_HWCAPS_RD_BOUNCE, resp. SNOR_HWCAPS_WR_BOUNCE for (Fast) Read, resp. Page Program operations. 2 - The buffer provided by the above MTD layer is not already in a DMA-safe area. This policy avoid using the bounce buffer when not explicitly requested or when not needed, hence limiting the performance penalty. Besides, the bounce buffer is allocated once for all at the very first time it is actually needed. This means that as long as all buffers provided by the above MTD layer are allocated in some DMA-safe areas, the bounce buffer itself is never allocated. Finally, the bounce buffer size is limited to 256KiB, the currently known maximum erase sector size. This tradeoff should still provide good performances even if new memory parts come with even larger erase sector sizes, limiting the memory footprint at the same time. Signed-off-by: Cyrille Pitchen <cyrille.pitchen@wedev4u.fr> --- drivers/mtd/devices/m25p80.c | 4 +- drivers/mtd/spi-nor/spi-nor.c | 136 +++++++++++++++++++++++++++++++++++++++--- include/linux/mtd/spi-nor.h | 8 +++ 3 files changed, 139 insertions(+), 9 deletions(-)