On 12/19/19 12:36 PM, Andre Przywara wrote:
> On Thu, 19 Dec 2019 09:04:32 +0100
> Marek Vasut <[email protected]> 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 <[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.
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()?
