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

[Boris Brezillon]

On Wed, 1 Mar 2017 15:21:24 +0100
Cyrille Pitchen <cyrille.pitchen@xxxxxxxxx> wrote:

> + Mark
> 
> Le 01/03/2017 Ã 12:46, Vignesh R a Ãcrit :
> > 
> > 
> > On Wednesday 01 March 2017 04:13 PM, 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.
> >>
> >> 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 have atleast one evidence of VIVT aliasing causing problem. Please see
> > this thread on DMA issues with davinci-spi driver
> > https://www.spinics.net/lists/arm-kernel/msg563420.html
> > https://www.spinics.net/lists/arm-kernel/msg563445.html
> >   
> >> Then my next question: is spi_map_msg() enough in every case, even with
> >> VIPT or VIVT caches?
> >>  
> > 
> > Not really, I am debugging another issue with UBIFS on DRA74 EVM (ARM
> > cortex-a15) wherein pages allocated by vmalloc are in highmem region
> > that are not addressable using 32 bit addresses and is backed by LPAE.
> > So, a 32 bit DMA cannot access these buffers at all.
> > When dma_map_sg() is called to map these pages by spi_map_buf() the
> > physical address is just truncated to 32 bit in pfn_to_dma() (as part of
> > dma_map_sg() call). This results in random crashes as DMA starts
> > accessing random memory during SPI read.
> > 
> > IMO, there may be more undiscovered caveat with using dma_map_sg() for
> > non kmalloc'd buffers and its better that spi-nor starts handling these
> > buffers instead of relying on spi_map_msg() and working around every
> > time something pops up.
> >   
> 
> Both Frode and you confirmed that the alias issue does occur at least
> with VIVT caches, hence we can't rely on spi_map_msg() in that case.
> So I agree with you: adding a bounce buffer in spi-nor seems to be a
> good solution at least till some rework is done in the ubifs layer, as
> proposed by Boris, to replace vmalloc'ed buffers by kmalloc'ed memory.

We should keep it even after reworking UBI/UBIFS, because UBI is just
one user of MTD, and other users might pass vmalloc-ed or kmap-ed bufs.