| Message ID | cover.1514087323.git.cyrille.pitchen@wedev4u.fr |
|---|---|
| Headers | show |
| Series | mtd: spi-nor: fix DMA-unsafe buffer issue between MTD and SPI | expand |
On Sun, 2017-12-24 at 05:36 +0100, Cyrille Pitchen wrote: > this series tries to solve a long time issue of compatibility between the > MTD and SPI sub-systems about whether we should use DMA-safe memory. Can this should replace SoC specific fixes like: c687c46e9e45 spi: spi-ti-qspi: Use bounce buffer if read buffer is not DMA'ble 7094576ccdc3 spi: atmel: fix corrupted data issue on SAM9 family SoCs Or, since this only fixes instances of DMA-unsafe buffers used in access to SPI NOR flash chips, and since there are other SPI master interface users, those chip specific fixes in some/all spi master drivers are still needed to fix transfers not originated via spi-nor? Or are all the previous fixes necessary because of spi-nor flash (via ubifs or jffs2) and once that's fixed via this series there are no more originators of dma-unsafe buffers? > The SPI sub-system has already implemented a work-around on its side, > based on the spi_map_buf() function. However this function has its own > limitation too. Especially, even if it builds a 'struct scatterlist' from > a vmalloc'ed buffer, calling dma_map_sg() is still not safe on all > architectures. Especially, on ARM cores using either VIPT or VIVT data > caches, dma_map_sg() doesn't take the cache aliases issue into account. > Then numerous crashes were reported for such architectures. Could the above issue also be fixed via calls to flush_kernel_vmap_range() and invalidate_kernel_vmap_range() to keep the data cache valid? Or is there a reason that won't work?
On Tue, Dec 26, 2017 at 06:45:28PM +0000, Trent Piepho wrote: > Or, since this only fixes instances of DMA-unsafe buffers used in > access to SPI NOR flash chips, and since there are other SPI master > interface users, those chip specific fixes in some/all spi master > drivers are still needed to fix transfers not originated via spi-nor? SPI client drivers are *supposed* to use DMA safe memory already. How often that happens in cases where it matters is a separate question, we definitely have users with smaller transfers that don't do the right thing but they're normally done using PIO anyway.
On Wed, 2017-12-27 at 10:36 +0000, Mark Brown wrote: > On Tue, Dec 26, 2017 at 06:45:28PM +0000, Trent Piepho wrote: > > > Or, since this only fixes instances of DMA-unsafe buffers used in > > access to SPI NOR flash chips, and since there are other SPI master > > interface users, those chip specific fixes in some/all spi master > > drivers are still needed to fix transfers not originated via spi-nor? > > SPI client drivers are *supposed* to use DMA safe memory already. How > often that happens in cases where it matters is a separate question, we > definitely have users with smaller transfers that don't do the right > thing but they're normally done using PIO anyway. I wonder what the end goal is here? A random collection of spi master drivers will accept DMA-unsafe buffers in some way. In some cases a framework like spi-nor provides the fixup to spi-nor master drivers (none so far) and in other cases (atmel-quadspi), the spi-nor master driver has its own fixes. Generic spi masters like spi-atmel, spi-ti-qspi, and spi-davinci will have their fixes for certain cases. Perhaps spi flash drivers like m25p80 will have fixes too? Some spi clients, like spidev, will have internal bounce buffers, rather than userspace addresses or commands in stack variables, so that they follow the rules about DMA safe buffers. What exactly is caught as DMA unsafe and what is not will of course vary greatly from driver to driver. Some drivers will catch highmem memory while other drivers will only detect vmalloc memory. Some will only catch an unsafe buffer if a specific SoC known to the driver to have an aliasing cache is enabled. Some will check buffers that arrive via the spi_flash_read interface but not via generic spi transfers, while others will check all spi transfer buffers. Obviously, I don't think this path will lead to a desirable end. Maybe the basic assumption, that clients should provide DMA safe buffers, should be revisited. Experience has shown that it's too much to ask for and spi clients will never get it right. It would be better to try to fix this at some common point between the clients and masters so it can be done once and for all.
Hi Trent, Le 27/12/2017 à 21:15, Trent Piepho a écrit : > On Wed, 2017-12-27 at 10:36 +0000, Mark Brown wrote: >> On Tue, Dec 26, 2017 at 06:45:28PM +0000, Trent Piepho wrote: >> >>> Or, since this only fixes instances of DMA-unsafe buffers used in >>> access to SPI NOR flash chips, and since there are other SPI master >>> interface users, those chip specific fixes in some/all spi master >>> drivers are still needed to fix transfers not originated via spi-nor? >> >> SPI client drivers are *supposed* to use DMA safe memory already. How >> often that happens in cases where it matters is a separate question, we >> definitely have users with smaller transfers that don't do the right >> thing but they're normally done using PIO anyway. > > I wonder what the end goal is here? > > A random collection of spi master drivers will accept DMA-unsafe > buffers in some way. In some cases a framework like spi-nor provides > the fixup to spi-nor master drivers (none so far) and in other cases > (atmel-quadspi), the spi-nor master driver has its own fixes. > At the spi-nor side, atmel-quadspi is also an example showing how the bounce buffer feature should be used by other spi-flash drivers. Actually, the m25p80 driver taken aside, no spi-flash driver is currently able to perform DMA safe transfers. Some patches were proposed but all rejected because they were doing wrong things: calling dma_map_single() even if the buffer is not guaranteed to be contiguous in physical memory or not being aware of the data cache aliasing issue on some architectures. So this series offers a common helper solution for all drivers in spi-nor. I don't want each spi-flash driver to implement its own custom solution. > Generic spi masters like spi-atmel, spi-ti-qspi, and spi-davinci will > have their fixes for certain cases. > If UBIFS was the only reason for those drivers to implement their own fixes then those fixes would no longer be needed with this series. However if other spi-clients also provide the SPI sub-system with DMA-unsafe buffers then maybe those fixes are still needed. I think Mark knows better than anyone else about the SPI sub-system. Besides, another reason to allocate the bounce buffer from spi-nor is that we can choose a suited size as a trade-off between performance and memory footprint. > Perhaps spi flash drivers like m25p80 will have fixes too? > patch 1 of the series enables the bounce buffer for both read and write operations in the m25p80 driver, in order to be compliant with buffer constraints expressed in the kernel-doc of 'struct spi_transfer'. > Some spi clients, like spidev, will have internal bounce buffers, > rather than userspace addresses or commands in stack variables, so that > they follow the rules about DMA safe buffers. > > What exactly is caught as DMA unsafe and what is not will of course > vary greatly from driver to driver. Some drivers will catch highmem > memory while other drivers will only detect vmalloc memory. Some will > only catch an unsafe buffer if a specific SoC known to the driver to > have an aliasing cache is enabled. Some will check buffers that arrive > via the spi_flash_read interface but not via generic spi transfers, > while others will check all spi transfer buffers. > That's why I've asked for pieces of advice on how to implement a relevant [spi_nor_]is_dma_safe() function and Vignesh has provided good suggestion! > Obviously, I don't think this path will lead to a desirable end. Maybe Here you seem to only take the m25p80 and SPI sub-system cases into account. However, at the spi-nor side, we currently have to other solution to let spi-nor flash drivers implement DMA transfers safely. Best regards, Cyrille > the basic assumption, that clients should provide DMA safe buffers, > should be revisited. Experience has shown that it's too much to ask > for and spi clients will never get it right. It would be better to try > to fix this at some common point between the clients and masters so it > can be done once and for all. >
On Wednesday 27 December 2017 12:15 AM, Trent Piepho wrote: > On Sun, 2017-12-24 at 05:36 +0100, Cyrille Pitchen wrote: >> this series tries to solve a long time issue of compatibility between the >> MTD and SPI sub-systems about whether we should use DMA-safe memory. > > Can this should replace SoC specific fixes like: > > c687c46e9e45 > spi: spi-ti-qspi: Use bounce buffer if read buffer is not DMA'ble > Yes, I plan to revert this patch once m25p80 starts using bounce buffers. I am interested in knowing which other SPI clients end up passing non DMA'able buffers. AFAIK, its UBIFS and JFFS2. Most other SPI devices on TI boards that I have dealt with like Touch, Sensors (IIO), GPIO expanders etc all provide DMA'able buffers.
On Wed, Dec 27, 2017 at 08:15:11PM +0000, Trent Piepho wrote: > A random collection of spi master drivers will accept DMA-unsafe > buffers in some way. In some cases a framework like spi-nor provides > the fixup to spi-nor master drivers (none so far) and in other cases > (atmel-quadspi), the spi-nor master driver has its own fixes. Everything really should be DMA safe. The thing with skimping on the DMA safety is that things generally won't explode every single time but they might explode. > What exactly is caught as DMA unsafe and what is not will of course > vary greatly from driver to driver. Some drivers will catch highmem > memory while other drivers will only detect vmalloc memory. Some will > only catch an unsafe buffer if a specific SoC known to the driver to > have an aliasing cache is enabled. Some will check buffers that arrive > via the spi_flash_read interface but not via generic spi transfers, > while others will check all spi transfer buffers. As I keep saying here the real fix would be to have a simple API that we could use to tell if we're dealing with something DMA safe. Right now it's a mess because you need to pass that information around through every layer that references memory which is a pain.
Hi all, this series tries to solve a long time issue of compatibility between the MTD and SPI sub-systems about whether we should use DMA-safe memory. This issue is visible espcecially when using a UBI file-system on a SPI NOR memory accessed through a SPI controller behind the m25p80 driver. The SPI sub-system has already implemented a work-around on its side, based on the spi_map_buf() function. However this function has its own limitation too. Especially, even if it builds a 'struct scatterlist' from a vmalloc'ed buffer, calling dma_map_sg() is still not safe on all architectures. Especially, on ARM cores using either VIPT or VIVT data caches, dma_map_sg() doesn't take the cache aliases issue into account. Then numerous crashes were reported for such architectures. If think it's high time to provide a reliable solution. So let's try to work together! The proposed solution here is based on an former series from Vignesh R and relies on a bounce buffer. For this series, I will need some pieces of advice on how to implement a reliable [spi_nor_]is_dma_safe() function. The proposed implementation in patch 1 is based on the tests performed by spi_map_buf() but I was wondring whether it would be more cautious to consider: DMA-safe <=> allocated by kmalloc'ed. Actually, it's better using the bounce buffer when not needed than not using it when we should. Also the bounce buffer solution in spi-nor is designed so it could be used as an helper so spi flash controller drivers other than m25p80.c could now easily add support to DMA transfers for (Fast) Read and/or Page Program operations. I've implemented and tested it on a sama5d2 xplained board + Macronix mx25l25673g SPI NOR memory reading from and writing into some UBI file-system. I've also tested with mtd_debug to write then read back a raw data into the SPI NOR memory, later checking the data integrity with sha1sum. For the atmel-quadspi.c driver, DMA memcpy() transfers are enabled only if the "dmacap,mempcy" boolean property is set in the device-tree. I found this name in some other device-trees already using it for a boolean property. Best regards, Cyrille Cyrille Pitchen (3): mtd: spi-nor: add optional DMA-safe bounce buffer for data transfer dt-bindings: mtd: atmel-quadspi: add an optional property 'dmacap,memcpy' mtd: atmel-quadspi: add support of DMA memcpy() .../devicetree/bindings/mtd/atmel-quadspi.txt | 5 + drivers/mtd/devices/m25p80.c | 4 +- drivers/mtd/spi-nor/atmel-quadspi.c | 132 +++++++++++++++++++- drivers/mtd/spi-nor/spi-nor.c | 136 +++++++++++++++++++-- include/linux/mtd/spi-nor.h | 8 ++ 5 files changed, 273 insertions(+), 12 deletions(-)