[RESEND PATCH v2 50/53] mtd: nand: allocate aligned buffers if NAND_OWN_BUFFERS is unset

[yamada.masahiro at socionext.com]

Hi Boris

> -----Original Message-----
> > The current situation violates [A].
> 
> Do you have a real failure that is proven to be caused by mis cache
> line alignment, or are you just speculating?

No real problem.
Rather, I am following the line 226 of DMA-API.txt
"Warnings: Memory coherency ...".


In my case, the cache line is 64 byte (for ARM),
But this does not really matter for the reason
nand_buffers is only touched at initialization as you mentioned.


[B] is a real problem for me
because Denali DMA engine requires >4byte alignment.




So, I can replace the git-log in the next version:

"
Some NAND controllers are using DMA engine requiring a specific buffer
alignment. The core provides no guarantee on the nand_buffers pointers,
which forces some drivers to allocate their own buffers and pass the
NAND_OWN_BUFFERS flag.

Rework the nand_buffers allocation logic to allocate each buffer
independently. This should make most NAND controllers/DMA engine
happy, and allow us to get rid of these custom buf allocation in NAND
controller drivers.
"

Masahiro













> >
> > Usually [B] is less than [A].
> 
> Yep, it's likely the case.
> 
> > So, if we meet [A] (by using kmalloc), [B] will be met as well.
> 
> Sure.
> 
> >
> >
> >
> > > Regarding the cache line alignment issue, in this case it shouldn't
> be
> > > a problem, because the driver and the core shouldn't touch the
> > > chip->buffers object during a DMA transfer, so dma_map/unmap_single()
> > > should work fine (they may flush/invalidate one cache line entry that
> > > contains non-payload data, but that's not a problem as long as nothing
> > > is modified during the DMA transfer).
> >
> >
> > This is related to 52/53:
> > http://patchwork.ozlabs.org/patch/742409/
> >
> > Can you also read this?
> > https://lkml.org/lkml/2017/3/8/693
> >
> > Your comment is very similar to what was discussed in the thread.
> 
> I read it.
> 
> >
> >
> >
> > > >
> > > > So, drivers using DMA are very likely to end up with setting the
> > > > NAND_OWN_BUFFERS flag and allocate buffers by themselves.  Drivers
> > > > tend to use devm_k*alloc to simplify the probe failure path, but
> > > > devm_k*alloc() does not return a cache line aligned buffer.
> > >
> > > Oh, I didn't know that. I had a closer look at the code, and indeed,
> > > devm_kmalloc() does not guarantee any alignment.
> > >
> > > >
> > > > If used, it could violate the DMA-API requirement stated in
> > > > Documentation/DMA-API.txt:
> > > >   Warnings:  Memory coherency operates at a granularity called the
> > > >   cache line width.  In order for memory mapped by this API to
> > > >   operate correctly, the mapped region must begin exactly on a cache
> > > >   line boundary and end exactly on one (to prevent two separately
> > > >   mapped regions from sharing a single cache line).
> > > >
> > > > To sum up, NAND_OWN_BUFFERS is not very convenient for drivers.
> > > > nand_scan_tail() can give a separate buffer for each of ecccalc,
> > > > ecccode, databuf.  It is guaranteed kmalloc'ed memory is aligned
> > > > with ARCH_DMA_MINALIGN.
> > >
> > > Maybe I'm wrong, but AFAIU, kmalloc&co do not guarantee cache line
> > > alignment for small buffers (< cache line), so even kmalloc can return
> > > a buffer that is not cache line aligned.
> > > This being said, we should be good because most NAND controllers are
> > > only manipulating the page buffer (->databuf) which is large enough
> to
> > > be cache line aligned.
> >
> >
> > In my understanding kmalloc() returns cache aligned address even for 1
> byte memory.
> 
> After digging into the SLAB code I found the calculate_alignment()
> function [1] which is used to calculate the required alignment of
> objects provided by a SLAB cache. For kmalloc caches SLAB_HWCACHE_ALIGN
> is set, but if you look at the code, if the object size is smaller
> than half a cache line the alignment constraint is relaxed, meaning that
> elements smaller than cache_line/2 are not guaranteed to be aligned on
> a cache line.
> 
> I didn't test, but that should be pretty easy to verify (kmalloc a
> bunch of 1byte bufs and check their alignment).
> 
> [1]http://lxr.free-electrons.com/source/mm/slab_common.c#L301