Re: [PATCH] spi: spi-cadence-quadspi: Fix mapping of buffers for DMA reads

[Vignesh Raghavendra <vigneshr@xxxxxx>]

On 8/28/20 1:56 PM, Jan Kiszka wrote:
> On 28.08.20 09:47, Vignesh Raghavendra wrote:
>> Buffers need to mapped to DMA channel's device pointer instead of SPI
>> controller's device pointer as its system DMA that actually does data
>> transfer.
>> Data inconsistencies have been reported when reading from flash
>> without this fix.
>>
>> Fixes: 31fb632b5d43c ("spi: Move cadence-quadspi driver to drivers/spi/")
> 
> This looks wrong, ...
> 
>> Signed-off-by: Vignesh Raghavendra <vigneshr@xxxxxx>
>> ---
>>
>> This issue was present in the original driver under SPI NOR framework as well.
>> But only got exposed as driver started handling probe deferral for DMA channel
>> request and thus uses DMA almost always unlike before.
> 
> ...you rather want 935da5e5100f57d843cac4781b21f1c235059aa0 then.
> 

Actually, the offending code is actually from 
commit ffa639e069fb ("mtd: spi-nor: cadence-quadspi: Add DMA support for direct mode reads)
which adds dma_{un}map_single() calls.

Will send v2 with Fixes tag updated to point to above commit 


> Other than that:
> 
> Tested-by: Jan Kiszka <jan.kiszka@xxxxxxxxxxx>

Thanks!

Regards
Vignesh

> 
> Thanks!
> Jan
> 
>>
>>  drivers/spi/spi-cadence-quadspi.c | 8 +++++---
>>  1 file changed, 5 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/spi/spi-cadence-quadspi.c b/drivers/spi/spi-cadence-quadspi.c
>> index 508b219eabf8..c6795c684b16 100644
>> --- a/drivers/spi/spi-cadence-quadspi.c
>> +++ b/drivers/spi/spi-cadence-quadspi.c
>> @@ -907,14 +907,16 @@ static int cqspi_direct_read_execute(struct cqspi_flash_pdata *f_pdata,
>>  	struct dma_async_tx_descriptor *tx;
>>  	dma_cookie_t cookie;
>>  	dma_addr_t dma_dst;
>> +	struct device *ddev;
>>  
>>  	if (!cqspi->rx_chan || !virt_addr_valid(buf)) {
>>  		memcpy_fromio(buf, cqspi->ahb_base + from, len);
>>  		return 0;
>>  	}
>>  
>> -	dma_dst = dma_map_single(dev, buf, len, DMA_FROM_DEVICE);
>> -	if (dma_mapping_error(dev, dma_dst)) {
>> +	ddev = cqspi->rx_chan->device->dev;
>> +	dma_dst = dma_map_single(ddev, buf, len, DMA_FROM_DEVICE);
>> +	if (dma_mapping_error(ddev, dma_dst)) {
>>  		dev_err(dev, "dma mapping failed\n");
>>  		return -ENOMEM;
>>  	}
>> @@ -948,7 +950,7 @@ static int cqspi_direct_read_execute(struct cqspi_flash_pdata *f_pdata,
>>  	}
>>  
>>  err_unmap:
>> -	dma_unmap_single(dev, dma_dst, len, DMA_FROM_DEVICE);
>> +	dma_unmap_single(ddev, dma_dst, len, DMA_FROM_DEVICE);
>>  
>>  	return ret;
>>  }
>>
>