Re: [PATCH v2 2/5] irqchip/gic-v3: Disable pseudo NMIs on Mediatek devices w/ firmware issues

[Doug Anderson]

Hi,

On Tue, May 16, 2023 at 6:23 AM AngeloGioacchino Del Regno
<angelogioacchino.delregno@xxxxxxxxxxxxx> wrote:
>
> Il 15/05/23 22:13, Douglas Anderson ha scritto:
> > Some Chromebooks with Mediatek SoCs have a problem where the firmware
> > doesn't properly save/restore certain GICR registers. Newer
> > Chromebooks should fix this issue and we may be able to do firmware
> > updates for old Chromebooks. At the moment, the only known issue with
> > these Chromebooks is that we can't enable "pseudo NMIs" since the
> > priority register can be lost. Enabling "pseudo NMIs" on Chromebooks
> > with the problematic firmware causes crashes and freezes.
> >
> > Let's detect devices with this problem and then disable "pseudo NMIs"
> > on them. We'll detect the problem by looking for the presence of the
> > "mediatek,broken-save-restore-fw" property in the GIC device tree
> > node. Any devices with fixed firmware will not have this property.
> >
> > Our detection plan works because we never bake a Chromebook's device
> > tree into firmware. Instead, device trees are always bundled with the
> > kernel. We'll update the device trees of all affected Chromebooks and
> > then we'll never enable "pseudo NMI" on a kernel that is bundled with
> > old device trees. When a firmware update is shipped that fixes this
> > issue it will know to patch the device tree to remove the property.
> >
> > In order to make this work, the quick detection mechanism of the GICv3
> > code is extended to be able to look for properties in addition to
> > looking at "compatible".
> >
> > Reviewed-by: Julius Werner <jwerner@xxxxxxxxxxxx>
> > Signed-off-by: Douglas Anderson <dianders@xxxxxxxxxxxx>
>
> I don't like firmware removing properties from my devicetrees and I'd like this
> issue to get addressed in another way (use a scratch register? and check it in
> Linux drivers to determine if the issue is not present: if scratch contains BIT(x),
> do not parse the quirk) but that's a different discussion which is a bit out of
> context for this patch, so:

Any particular reason why? IMO it's actually a fair bit cleaner to
have firmware remove a property that's specifically documented for the
firmware to remove compared to having firmware adding properties to or
otherwise messing with the device tree. For the removal case, it's
easy from the device tree git history to find out about the property,
when it was added, and that it is expected that some versions of
firmware will remove it. IMO having firmware add properties can be a
little more mysterious, though that has its place too. In general,
though, firmware is expected to be able to be able to touch up the
device tree. It puts things in "chosen", adds bits describing the
firmware, can add things to the device tree to describe components it
is uniquely able to probe (like SDRAM), could enable/disable a
component if it has info about their presence, etc.

I'm happy to hear other opinions on it, but in my mind having a
sideband bit telling us to ignore the quirk is more confusing instead
of less confusing.


> Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@xxxxxxxxxxxxx>

Thanks!