[RFC PATCH 0/2] Enable DMA transfers for SAM9 and fix cache aliasing

[RFC PATCH 0/2] Enable DMA transfers for SAM9 and fix cache aliasing

[Radu Pirea]

This patches are an attempt to fix the following problem:
On SAM9 SoCs the cache model is VIVT thus 2 different cache may exist for
the same physical address, and because of that, when we allocate memory
with vmalloc and try to use DMA to read or write from/to this region it's
possible to get cache coherency problems.

To solve this, I have investigated the mailing list for possible solutions
and other drivers as well. The buffers coming from the filesystem allocated
with vmalloc are not in a DMA coherent area in this scenario. MTDblock
layer solve this problem for the other file system, but for file systems
based on MTD directly like UBIFS, this is not happening.

One solution is to use bounce buffers to copy the data in a DMA coherent
area. This will add a certain delay though and impact the performance. To
have best possible outcome we need to pass the same buffer to the DMA
engine, but insure that the memory is cache coherent before we read from
it, and the cache is invalidated after we write to the memory. To achieve
that I have added a cache flush on the buffer before starting the DMA
operation and a cache invalidate after DMA operation completes.

>From my point of view this is the best and most simple approach.

I based these patches on Russell King idea
https://lkml.org/lkml/2017/6/23/509

I welcome any feedback about this solution.

Thanks.



Radu Pirea (2):
  Revert "spi: atmel: fix corrupted data issue on SAM9 family SoCs"
  spi: atmel: Fix DMA transfers data corruption

 drivers/spi/spi-atmel.c | 44 +++++++++++++++++++++-----------------------
 1 file changed, 21 insertions(+), 23 deletions(-)

-- 
2.7.4

--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[RFC PATCH 1/2] Revert "spi: atmel: fix corrupted data issue on SAM9 family SoCs"

[Radu Pirea]

This reverts commit 7094576ccdc3acfe1e06a1e2ab547add375baf7f.

A better fix was found and DMA for SAM9 SoCs must be enabled.

Signed-off-by: Radu Pirea <radu.pirea-UWL1GkI3JZL3oGB3hsPCZA@public.gmane.org>
---
 drivers/spi/spi-atmel.c | 24 +-----------------------
 1 file changed, 1 insertion(+), 23 deletions(-)

diff --git a/drivers/spi/spi-atmel.c b/drivers/spi/spi-atmel.c
index f95da36..4e5e51f 100644
--- a/drivers/spi/spi-atmel.c
+++ b/drivers/spi/spi-atmel.c
@@ -269,7 +269,6 @@ struct atmel_spi_caps {
 	bool	is_spi2;
 	bool	has_wdrbt;
 	bool	has_dma_support;
-	bool	has_pdc_support;
 };
 
 /*
@@ -1426,28 +1425,7 @@ static void atmel_get_caps(struct atmel_spi *as)
 
 	as->caps.is_spi2 = version > 0x121;
 	as->caps.has_wdrbt = version >= 0x210;
-#ifdef CONFIG_SOC_SAM_V4_V5
-	/*
-	 * Atmel SoCs based on ARM9 (SAM9x) cores should not use spi_map_buf()
-	 * since this later function tries to map buffers with dma_map_sg()
-	 * even if they have not been allocated inside DMA-safe areas.
-	 * On SoCs based on Cortex A5 (SAMA5Dx), it works anyway because for
-	 * those ARM cores, the data cache follows the PIPT model.
-	 * Also the L2 cache controller of SAMA5D2 uses the PIPT model too.
-	 * In case of PIPT caches, there cannot be cache aliases.
-	 * However on ARM9 cores, the data cache follows the VIVT model, hence
-	 * the cache aliases issue can occur when buffers are allocated from
-	 * DMA-unsafe areas, by vmalloc() for instance, where cache coherency is
-	 * not taken into account or at least not handled completely (cache
-	 * lines of aliases are not invalidated).
-	 * This is not a theorical issue: it was reproduced when trying to mount
-	 * a UBI file-system on a at91sam9g35ek board.
-	 */
-	as->caps.has_dma_support = false;
-#else
 	as->caps.has_dma_support = version >= 0x212;
-#endif
-	as->caps.has_pdc_support = version < 0x212;
 }
 
 /*-------------------------------------------------------------------------*/
@@ -1588,7 +1566,7 @@ static int atmel_spi_probe(struct platform_device *pdev)
 		} else if (ret == -EPROBE_DEFER) {
 			return ret;
 		}
-	} else if (as->caps.has_pdc_support) {
+	} else {
 		as->use_pdc = true;
 	}
 
-- 
2.7.4

--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[RFC PATCH 2/2] spi: atmel: Fix DMA transfers data corruption

[Radu Pirea]

If the cache model is VIVT, DMA data transfers may not be valid and to
ensure the validity of the data cache must be flushed and invalidated.

Signed-off-by: Radu Pirea <radu.pirea-UWL1GkI3JZL3oGB3hsPCZA@public.gmane.org>
---
 drivers/spi/spi-atmel.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/drivers/spi/spi-atmel.c b/drivers/spi/spi-atmel.c
index 4e5e51f..cda6d0f 100644
--- a/drivers/spi/spi-atmel.c
+++ b/drivers/spi/spi-atmel.c
@@ -21,6 +21,8 @@
 #include <linux/slab.h>
 #include <linux/platform_data/dma-atmel.h>
 #include <linux/of.h>
+#include <asm/cacheflush.h>
+#include <asm/cachetype.h>
 
 #include <linux/io.h>
 #include <linux/gpio.h>
@@ -742,6 +744,18 @@ static int atmel_spi_next_xfer_dma_submit(struct spi_master *master,
 				       xfer->bits_per_word))
 		goto err_exit;
 
+#ifdef CONFIG_SOC_SAM_V4_V5
+	/*
+	 * On Atmel SoCs based on ARM9 cores, the data cache follows the VIVT
+	 * model, hence the cache aliases issue can occur when buffers are
+	 * allocated from DMA-unsafe areas, by vmalloc() for instance, where
+	 * cache coherency is not taken into account or at least not handled
+	 * completely (cache lines of aliases are not flushed and invalidated).
+	 * This is not a theorical issue: it was reproduced when trying to mount
+	 * a UBI file-system on a at91sam9g35ek board.
+	 */
+	flush_kernel_vmap_range((void *)xfer->rx_buf, xfer->len);
+#endif
 	/* Send both scatterlists */
 	rxdesc = dmaengine_prep_slave_sg(rxchan,
 					 xfer->rx_sg.sgl, xfer->rx_sg.nents,
@@ -750,6 +764,9 @@ static int atmel_spi_next_xfer_dma_submit(struct spi_master *master,
 	if (!rxdesc)
 		goto err_dma;
 
+#ifdef CONFIG_SOC_SAM_V4_V5
+	flush_kernel_vmap_range((void *)xfer->tx_buf, xfer->len);
+#endif
 	txdesc = dmaengine_prep_slave_sg(txchan,
 					 xfer->tx_sg.sgl, xfer->tx_sg.nents,
 					 DMA_TO_DEVICE,
@@ -779,6 +796,9 @@ static int atmel_spi_next_xfer_dma_submit(struct spi_master *master,
 	rxchan->device->device_issue_pending(rxchan);
 	txchan->device->device_issue_pending(txchan);
 
+#ifdef CONFIG_SOC_SAM_V4_V5
+	invalidate_kernel_vmap_range((void *)xfer->rx_buf, xfer->len);
+#endif
 	/* take back lock */
 	atmel_spi_lock(as);
 	return 0;
-- 
2.7.4

--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH 2/2] spi: atmel: Fix DMA transfers data corruption

[Trent Piepho]

On Wed, 2017-11-15 at 18:35 +0200, Radu Pirea wrote:
> If the cache model is VIVT, DMA data transfers may not be valid and to
> ensure the validity of the data cache must be flushed and invalidated.
> 
> Signed-off-by: Radu Pirea <radu.pirea@microchip.com>
>  
> +#ifdef CONFIG_SOC_SAM_V4_V5
> +	/*
> +	 * On Atmel SoCs based on ARM9 cores, the data cache follows the VIVT
> +	 * model, hence the cache aliases issue can occur when buffers are
> +	 * allocated from DMA-unsafe areas, by vmalloc() for instance, where
> +	 * cache coherency is not taken into account or at least not handled
> +	 * completely (cache lines of aliases are not flushed and invalidated).
> +	 * This is not a theorical issue: it was reproduced when trying to mount
> +	 * a UBI file-system on a at91sam9g35ek board.
> +	 */
> +	flush_kernel_vmap_range((void *)xfer->rx_buf, xfer->len);
> +#endif

Does this call need to be inside an ifdef for a specific SOC?  I
believe that flush_kernel_vmap_range should expand to a no-op if the
cache is not VIVT or aliasing VIPT.  So it should be safe to always
call it.

It also doesn't seem right that the SPI driver needs to know which SoCs
have what kind of cache.  If there are more socs with more cache
details, does the spi driver need to expand the list?  If another
driver does DMA, does it need the list too?


>  	/* Send both scatterlists */
>  	rxdesc = dmaengine_prep_slave_sg(rxchan,
>  					 xfer->rx_sg.sgl, xfer->rx_sg.nents,

Does this problem also possibly affect all other users of dmaengine on
these SoCs?  I2C, serial, crpto, etc. I couldn't find any other driver
that need to make flush_kernel_vmap_range().  Which seems odd, since
there are many drivers that use DMA and run on ARM9 cores.  E.g., I
believe spi-mxs runs on ARM9 based iMX.23 and iMX.28 which also have
VIVT caches.  It doesn't make flush_kernel_vmap_range calls.  Does it
have this same bug?

Re: [RFC PATCH 2/2] spi: atmel: Fix DMA transfers data corruption

[Radu Nicolae Pirea]

On 15.11.2017 21:01, Trent Piepho wrote:
> On Wed, 2017-11-15 at 18:35 +0200, Radu Pirea wrote:
>> If the cache model is VIVT, DMA data transfers may not be valid and to
>> ensure the validity of the data cache must be flushed and invalidated.
>>
>> Signed-off-by: Radu Pirea <radu.pirea@microchip.com>
>>   
>> +#ifdef CONFIG_SOC_SAM_V4_V5
>> +	/*
>> +	 * On Atmel SoCs based on ARM9 cores, the data cache follows the VIVT
>> +	 * model, hence the cache aliases issue can occur when buffers are
>> +	 * allocated from DMA-unsafe areas, by vmalloc() for instance, where
>> +	 * cache coherency is not taken into account or at least not handled
>> +	 * completely (cache lines of aliases are not flushed and invalidated).
>> +	 * This is not a theorical issue: it was reproduced when trying to mount
>> +	 * a UBI file-system on a at91sam9g35ek board.
>> +	 */
>> +	flush_kernel_vmap_range((void *)xfer->rx_buf, xfer->len);
>> +#endif
> 
> Does this call need to be inside an ifdef for a specific SOC?  I
> believe that flush_kernel_vmap_range should expand to a no-op if the
> cache is not VIVT or aliasing VIPT.  So it should be safe to always
> call it.
> 
> It also doesn't seem right that the SPI driver needs to know which SoCs
> have what kind of cache.  If there are more socs with more cache
> details, does the spi driver need to expand the list?  If another
> driver does DMA, does it need the list too?

Understood. I will find another way to call the function.

> 
> 
>>   	/* Send both scatterlists */
>>   	rxdesc = dmaengine_prep_slave_sg(rxchan,
>>   					 xfer->rx_sg.sgl, xfer->rx_sg.nents,
> 
> Does this problem also possibly affect all other users of dmaengine on
> these SoCs?  I2C, serial, crpto, etc. I couldn't find any other driver
> that need to make flush_kernel_vmap_range().  Which seems odd, since
> there are many drivers that use DMA and run on ARM9 cores.  E.g., I
> believe spi-mxs runs on ARM9 based iMX.23 and iMX.28 which also have
> VIVT caches.  It doesn't make flush_kernel_vmap_range calls.  Does it
> have this same bug?\x13��칻\x1c�&�~�&�\x18��+-��ݶ\x17��w��˛���m�b��l�(��\x17��ܨ}���Ơz�&j:+v���\a����zZ+��+zf���h���~����i���z�\x1e�w���?����&�)ߢ^[f
> 

Other users of dmaengine are not affected by this problem. I know nothing about 
spi-mxs, but i know spi-davinci have the same bug.

Re: [RFC PATCH 1/2] Revert "spi: atmel: fix corrupted data issue on SAM9 family SoCs"

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 571 bytes --]

On Wed, Nov 15, 2017 at 06:35:31PM +0200, Radu Pirea wrote:
> This reverts commit 7094576ccdc3acfe1e06a1e2ab547add375baf7f.
> 
> A better fix was found and DMA for SAM9 SoCs must be enabled.

Why can't this better fix be done incrementally, why do we have to
revert this?

Please submit patches using subject lines reflecting the style for the
subsystem.  This makes it easier for people to identify relevant
patches.  Look at what existing commits in the area you're changing are
doing and make sure your subject lines visually resemble what they're
doing.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [RFC PATCH 2/2] spi: atmel: Fix DMA transfers data corruption

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 898 bytes --]

On Wed, Nov 15, 2017 at 06:35:32PM +0200, Radu Pirea wrote:

> +#ifdef CONFIG_SOC_SAM_V4_V5
> +	/*
> +	 * On Atmel SoCs based on ARM9 cores, the data cache follows the VIVT
> +	 * model, hence the cache aliases issue can occur when buffers are
> +	 * allocated from DMA-unsafe areas, by vmalloc() for instance, where
> +	 * cache coherency is not taken into account or at least not handled
> +	 * completely (cache lines of aliases are not flushed and invalidated).
> +	 * This is not a theorical issue: it was reproduced when trying to mount
> +	 * a UBI file-system on a at91sam9g35ek board.
> +	 */
> +	flush_kernel_vmap_range((void *)xfer->rx_buf, xfer->len);
> +#endif

Shouldn't we be fixing this in the DMA mapping operations for the SoC,
won't this affect everything that does DMA on this platform and not just
this driver?  I'd expect that dma_map_sg() and so on would do the right
thing.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [RFC PATCH 2/2] spi: atmel: Fix DMA transfers data corruption

[Radu Nicolae Pirea]

On 16.11.2017 12:45, Mark Brown wrote:
> On Wed, Nov 15, 2017 at 06:35:32PM +0200, Radu Pirea wrote:
> 
>> +#ifdef CONFIG_SOC_SAM_V4_V5
>> +	/*
>> +	 * On Atmel SoCs based on ARM9 cores, the data cache follows the VIVT
>> +	 * model, hence the cache aliases issue can occur when buffers are
>> +	 * allocated from DMA-unsafe areas, by vmalloc() for instance, where
>> +	 * cache coherency is not taken into account or at least not handled
>> +	 * completely (cache lines of aliases are not flushed and invalidated).
>> +	 * This is not a theorical issue: it was reproduced when trying to mount
>> +	 * a UBI file-system on a at91sam9g35ek board.
>> +	 */
>> +	flush_kernel_vmap_range((void *)xfer->rx_buf, xfer->len);
>> +#endif
> 
> Shouldn't we be fixing this in the DMA mapping operations for the SoC,
> won't this affect everything that does DMA on this platform and not just
> this driver?  I'd expect that dma_map_sg() and so on would do the right
> thing.
> 

I didn't find a bug like this in other drivers and the only way I can reproduce 
the bug is with UBIFS on top of a spi-nor memory. dma_map_sg() does the right 
thing, but data from cache are not written-back. If I enable 
CONFIG_CPU_DCACHE_WRITETHROUGH bug disappears. Anyway, enabling 
CONFIG_CPU_DCACHE_WRITETHROUGH is not an option because performance will drop.

Fixing the bug in DMA driver is not an option because other DMA operations are 
not affected and the bug comes from the fact that UBIFS allocates memory with 
vmalloc.

Until now I have two solutions for this bug:
1. This one with cache flushing.
2. Another solution, based on ti-qspi driver, is to transfer the data whit a 
bounce buffer allocated with dma_alloc_coherent when rx_buf or tx_buf is 
allocated with vmalloc.

So, witch solution do you think is suitable to solve this bug?