[U-Boot] [PATCH] nios2: convert cache flush to use dm cpu data

[Marek Vasut]

On Tuesday, October 13, 2015 at 03:04:44 AM, Thomas Chou wrote:
> Hi Marek,

Hi!

> On 10/12/2015 09:29 PM, Marek Vasut wrote:
> > On Monday, October 12, 2015 at 03:12:18 PM, Thomas Chou wrote:
> >> Hi Marek,
> >> 
> >> On 10/12/2015 06:30 PM, Marek Vasut wrote:
> >>> There are also DEFINE_CACHE_ALIGN_BUFFER() and
> >>> ALLOC_CACHE_ALIGN_BUFFER() macros which can be used to allocate such
> >>> stuff on stack. And you sometimes do want to allocate things on stack
> >>> instead of using malloc().
> >> 
> >> Thanks for sharing this.
> >> 
> >>> Sometimes you might want to allocate DMA buffers on stack, for example
> >>> if you don't have mallocator running yet or if it's more convenient
> >>> for some reason. So forcing everyone to allocate DMA buffers using
> >>> malloc is not gonna slide I'm afraid.
> >> 
> >> The same rule can be applied to buffer allocated on stack, with the
> >> macro you mentioned above. In all, cache line aware allocation on heap
> >> or on stack must be used for DMA buffer.
> > 
> > That's correct, they must be used. But sadly, this is not yet the case in
> > all the drivers, which we need to rectify. And how best to rectify this
> > than to scream when someone does such a thing, right ?
> 
> Given that there are not so many drivers using DMA in u-boot, as
> compared to Linux. I would suggest we can walk through and rectify the
> allocation of DMA buffers.

That's what we're pretty much trying to do -- fix all the DMA-using drivers
to behave correctly.

> >>> The cache flush ops is the best place to scream death and murder if
> >>> someone tries such unaligned cache operation, so maybe you should even
> >>> do a printf() there to weed such crappy drivers out for the 2016.01
> >>> release.
> >>> 
> >>> I agree it's the responsibility of the driver, so if the driver doesn't
> >>> do things right, it's a bug and the behavior of cache ops is undefined,
> >>> which might as well be that we do the safer thing here and flush
> >>> nothing.
> >> 
> >> It won't be safer to flush nothing. Sooner or later the cache will be
> >> flushed due to data access, even if the cache flush ops is skip.
> > 
> > That is bad bad bad, that's even nastier. We really need to fix the
> > drivers, not paper over it in the cache ops.
> 
> I know how bad it is, with over 35 years work with DMA. grin..

I can feel your pain here ;-/

> >> To solve problem like this, the only solution is to enforce the rule to
> >> allocate DMA buffer. It is wrong to skip the flush.
> > 
> > I absolutelly agree we need aligned allocations for DMA memory areas.
> > But, we also shouldn't hide bugs. And I believe aligning the incorrect
> > arguments to cache functions is not the way to go. We should check the
> > arguments and if someone tries an unaligned cache op, we should scream.
> > What do you think?
> > 
> > btw. I think you won't get way too many cache warnings nowadays and we
> > can fix those few remaining way before the 2016.01 is out.
> 
> I would suggest the "cache alignment check and skip" be removed from
> cache flush ops, and say out the DMA buffer allocation rule loudly in
> README, and enforce it by guardianship.

What exactly do you envision by this "guardianship" ?

> Please allow me to restate the reasons,
> 
> 1. The cache flush ops are commonly used. Please refer to the "Cache and
> TLB Flushing Under Linux" doc, linux/Documentation/cachetlb.txt.
> Violating the defined interface is much worse than violating coding
> style. It will certainly impact the portability of u-boot. And might
> introduce more bug than resolve.

I agree with this one.

> 2. We all agree that enforcing DMA buffer allocation to cache aligned is
> the only real solution. Adding such "check and skip" to cache flush ops
> cannot prevent the flush or solve the problem.

We should probably check-scream-skip here.

> 3. Though the flush size of block device are usually aligned, the size
> of packet are not. Asking the packet drivers to adjust the flush size
> does not make sense. It is the job of cache flush ops. The debug probe
> should not override the original purpose. It should be spelled for
> common understanding.

The socket buffer(s) should be aligned, so network packets should be fine.

> It is free to your consideration. As it is free and open software. :)
> 
> Best regards,
> Thomas