Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [PATCH] spi: spi-cadence-quadspi: Fix mapping of buffers for DMA
 reads
To:     Jan Kiszka <jan.kiszka@siemens.com>,
        Mark Brown <broonie@kernel.org>
CC:     Pratyush Yadav <p.yadav@ti.com>, <linux-spi@vger.kernel.org>,
        <linux-kernel@vger.kernel.org>
References: <20200828074726.24546-1-vigneshr@ti.com>
 <8828e301-a7b7-d837-dc60-6c5101cdac90@siemens.com>
From:   Vignesh Raghavendra <vigneshr@ti.com>
Message-ID: <d011f755-421a-292f-25f6-a88568f09798@ti.com>
Date:   Mon, 31 Aug 2020 18:28:19 +0530
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101
 Thunderbird/68.10.0
MIME-Version: 1.0
In-Reply-To: <8828e301-a7b7-d837-dc60-6c5101cdac90@siemens.com>
Content-Type: text/plain; charset="utf-8"
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk


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@ti.com>
>> ---
>>
>> 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@siemens.com>

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;
>>  }
>>
>