[RFC PATCH] Revert "arm: Show cache warnings in U-Boot proper only"

[Marek Vasut]

On 12/19/19 12:36 PM, Andre Przywara wrote:
> On Thu, 19 Dec 2019 09:04:32 +0100
> Marek Vasut <marex at denx.de> wrote:
> 
> Hi Marek,

Hi,

>> 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 <andre.przywara at arm.com>
>>>>> ---
>>>>> 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.

Do you have specific details here ? They should be 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.

U-Boot is not only V7 and V8, but also all the earlier ones (back to V4).

> 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().

Sure, but that's a driver bug. That's a separate topic from this patch.

> 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.

Wrong, you may interfere with some DMA engine which might be accessing
the area which you just overshot.

> 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.

Again, driver bug, that's unrelated and must be fixed.

> 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.

See above.

>>> 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.

Of course the address passed to cache ops should not be rounded up.
That's a bug (unless the buffer actually covers that rounded address,
which sometimes is the case, and might or might not thus be harmless.
That's up to the driver author to determine)

> 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).

This implication makes no sense. How do you infer from "drivers have
bugs" that a warning about such bugs should be turned into a DEBUG()?