Re: [RFC PATCH 2/2] mtd: devices: m25p80: Enable spi-nor bounce buffer support

[Frode Isaksen]

On 01/03/2017 11:43, Cyrille Pitchen wrote:
> Le 01/03/2017 à 05:54, Vignesh R a écrit :
>>
>> On Wednesday 01 March 2017 03:11 AM, Richard Weinberger wrote:
>>> Vignesh,
>>>
>>> Am 27.02.2017 um 13:08 schrieb Vignesh R:
>>>> Many SPI controller drivers use DMA to read/write from m25p80 compatible
>>>> flashes. Therefore enable bounce buffers support provided by spi-nor
>>>> framework to take care of handling vmalloc'd buffers which may not be
>>>> DMA'able.
>>>>
>>>> Signed-off-by: Vignesh R <vigneshr@xxxxxx>
>>>> ---
>>>>  drivers/mtd/devices/m25p80.c | 1 +
>>>>  1 file changed, 1 insertion(+)
>>>>
>>>> diff --git a/drivers/mtd/devices/m25p80.c b/drivers/mtd/devices/m25p80.c
>>>> index c4df3b1bded0..d05acf22eadf 100644
>>>> --- a/drivers/mtd/devices/m25p80.c
>>>> +++ b/drivers/mtd/devices/m25p80.c
>>>> @@ -241,6 +241,7 @@ static int m25p_probe(struct spi_device *spi)
>>>>  	else
>>>>  		flash_name = spi->modalias;
>>>>  
>>>> +	nor->flags |= SNOR_F_USE_BOUNCE_BUFFER;
>>> Isn't there a better way to detect whether a bounce buffer is needed or not?
> I agree with Richard: the bounce buffer should be enabled only if needed
> by the SPI controller.
>
>> Yes, I can poke the spi->master struct to see of dma channels are
>> populated and request SNOR_F_USE_BOUNCE_BUFFER accordingly:
>>
>> -       nor->flags |= SNOR_F_USE_BOUNCE_BUFFER;
>> +       if (spi->master->dma_tx || spi->master->dma_rx)
>> +               nor->flags |= SNOR_F_USE_BOUNCE_BUFFER;
>> +
>>
> However I don't agree with this solution: master->dma_{tx|rx} can be set
> for SPI controllers which already rely on spi_map_msg() to handle
> vmalloc'ed memory during DMA transfers.
> Such SPI controllers don't need the spi-nor bounce buffer.
I tried to push a patch handling the VIVT cache flush in the daVinci  SPI driver, but this was NACK'ed. So, in general terms, the upper layers do not know if the SPI controller will correctly handle vmalloc'ed buffers.
>
> spi_map_msg() can build a scatter-gather list from vmalloc'ed buffer
> then map this sg list with dma_map_sg(). AFAIK, It is safe to do so for
> architectures using PIPT caches since the possible cache aliases issue
> present for VIPT or VIVT caches is always avoided for PIPT caches.
>
> For instance, the drivers/spi/spi-atmel.c driver relies on spi_map_sg()
> to be called from the SPI sub-system to handle vmalloc'ed buffers and
> both master->dma_tx and master->dma_rx are set by the this driver.
>
>
> By the way, Is there any case where the same physical page is actually
> mapped into two different virtual addresses for the buffers allocated by
> the MTD sub-system? Because for a long time now I wonder whether the
> cache aliases issue is a real or only theoretical issue but I have no
> answer to that question.
I can reproduce the VIVT cache bug with 100% reliability either using spi-loopback-test (with patches) or mounting/reading/writing to a UBIFS volume. The physical address of the vmalloc'ed buffers is mapped to both vmap and lowmem virtual address, and when doing DMA, only the lowmem virtual address is used when invalidating/flushing the cache.

Thanks,
Frode
>
> Then my next question: is spi_map_msg() enough in every case, even with
> VIPT or VIVT caches?
>
> Best regards,
>
> Cyrille