On Thu, 19 Dec 2019 09:04:32 +0100
Marek Vasut <[email protected]> wrote:
Hi Marek,
> On 12/19/19 2:23 AM, André Przywara wrote:
> > On 19/12/2019 00:55, Marek Vasut wrote:
> >
> > Hi Marek,
>
> Hi,
>
> >> On 12/19/19 1:52 AM, Andre Przywara wrote:
> >>> According to commit 11aa6a32eb5f ("arm: cache: Implement cache range
> >>> check for v7"), which introduced check_cache_range(), this was meant
> >>> as a pure debugging feature, only to be compiled in when a developer
> >>> explicitly #defined DEBUG in cache.c. Presumably the intention was to
> >>> help with finding *certain* alignment issues with DMA buffers.
> >>>
> >>> Commit bcc53bf09589 ("arm: Show cache warnings in U-Boot proper only")
> >>> compiled this in *unconditionally* into U-Boot proper.
> >>>
> >>> This has the annoying side effect of producing tons of somewhat
> >>> pointless warnings about non-aligned clean&invalidate operations, which
> >>> tend to be appeased by even more pointless rounding operations in many
> >>> drivers (mostly those used on ARM boards).
> >>>
> >>> Bring back the old behaviour, of only compiling this in for DEBUG
> >>> situations, but staying silent otherwise.
> >>>
> >>> This reverts commit bcc53bf095893fbdae531a9a7b5d4ef4a125a7fc.
> >>>
> >>> Signed-off-by: Andre Przywara <[email protected]>
> >>> ---
> >>> Hi,
> >>>
> >>> if the intention was indeed to always force cache maintenance range
> >>> alignments, I would like to open a discussion on this, because I believe
> >>> it is not useful, especially in the clean&invalidate case.
> >>
> >> Why don't you rather fix the cache op alignment bugs ?
> >
> > Which bugs do you mean?
>
> Well, the ones reported by this warning.
As mentioned, most of them are just silenced in drivers, though not actually
fixed.
This patch here was actually triggered by me cleaning up sun8i-emac.c, which
then made those warnings appear again.
I will try to send out this series, which might make it more clear what I am
after.
> > Those that would currently trigger those warnings?
> > I don't think they are actual bugs, besides I don't think they are any
> > cases left for 32-bit ARM boards (leave alone the new RPi4 Ethernet
> > driver in the rpi-4-32b_defconfig).
>
> If your cache isn't flushed or invalidated properly, it's a bug.
> On ARM32, this is (was, before this warning) a real problem.
The actual cache maintenance is fine in the drivers, it's the over-strict
alignment requirements that bothers me and makes things actually worse:
Architecturally (ARM ARM) there is no alignment requirement for VA operations.
Walking over a range of addresses does get easier if you start on a cache line
boundary, so this is what we do in the implementations for v7 and v8.
Alignment gets critical when it comes to a pure invalidate call. If you have
other dirty data in this same cache line, you might lose the latest stores,
that's why it is important to align those *buffers*. So the strict check we
have in v7_dcache_inval_range() is the right thing (at least for the base
address). What is not right is to deliberately align just the *parameters* of
the invalidate call, as seens in net/sun8i_emac.c or net/dwc_eth_qos.c, for
instance. This could actually hide bugs. Drivers should always pass on the
actual buffer address to invalidate().
Then there is no real need to align the range for flush (clean&invalidate)
calls. Overshooting here is not critical, and again we do align them anyway in
the implementation. This
forced check only leads to drivers aligning the *parameters* of these calls, to
no actual effect, probably given a wrong impression to driver authors and
readers.
So in preparation for fixing the drivers, I ask for those checks to become pure
DEBUG warnings. Another alternative would be to relax those checks, to just
require the start address of invalidate to be aligned - but this is what we
check separately already.
> > Or those that are currently hidden because we *force* an alignment on
> > the *arguments* passed to invalidate_dcache_range, for instance?
> > These are quite numerous, so I would rather get some input first before
> > spending a lot of time on this.
>
> Make sure your buffers are properly aligned and you have no cache
> issues. It's really that easy. If you see numerous, then your platform
> has a real problem which must be fixed ; I didn't see any for quite a
> while on platforms I have access to.
Yes, I think the buffers are correctly aligned in the drivers, as least the
ones I checked. But then we should always pass this address to invalidate, and
not round it just before calling invalidate. Same goes to some degree for the
end address, although is a somewhat separate topic.
And that's why I think having check_cache_range() is useful for development and
debugging, but only then, because not all warnings are legit (end addresses).
Cheers,
Andre.
> > Starting a discussion on this topic and getting some feedback was the
> > actual reason for this patch - even though it is still valid, IMHO.
>
> [...]
