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

[Geert Uytterhoeven]

Hi Douglas,

On Mon, May 15, 2023 at 10:16 PM Douglas Anderson <dianders@xxxxxxxxxxxx> wrote:
> 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>
> ---
>
> Changes in v2:
> - mediatek,gicr-save-quirk => mediatek,broken-save-restore-fw

Thanks for your patch, which is now commit 44bd78dd2b8897f5
("irqchip/gic-v3: Disable pseudo NMIs on Mediatek devices w/
firmware issues") in v6.4-rc4.

This causes enabling an unrelated workaround on R-Car V4H:

    GIC: enabling workaround for GICv3: Cavium erratum 38539

> --- a/drivers/irqchip/irq-gic-common.c
> +++ b/drivers/irqchip/irq-gic-common.c
> @@ -16,7 +16,11 @@ void gic_enable_of_quirks(const struct device_node *np,
>                           const struct gic_quirk *quirks, void *data)
>  {
>         for (; quirks->desc; quirks++) {
> -               if (!of_device_is_compatible(np, quirks->compatible))
> +               if (quirks->compatible &&
> +                   !of_device_is_compatible(np, quirks->compatible))
> +                       continue;
> +               if (quirks->property &&
> +                   !of_property_read_bool(np, quirks->property))
>                         continue;

Presumably the loop should continue if none of quirks-compatible
or quirks->property is set?

>                 if (quirks->init(data))
>                         pr_info("GIC: enabling workaround for %s\n",
> @@ -28,7 +32,7 @@ void gic_enable_quirks(u32 iidr, const struct gic_quirk *quirks,
>                 void *data)
>  {
>         for (; quirks->desc; quirks++) {
> -               if (quirks->compatible)
> +               if (quirks->compatible || quirks->property)
>                         continue;
>                 if (quirks->iidr != (quirks->mask & iidr))
>                         continue;
> diff --git a/drivers/irqchip/irq-gic-common.h b/drivers/irqchip/irq-gic-common.h
> index 27e3d4ed4f32..3db4592cda1c 100644
> --- a/drivers/irqchip/irq-gic-common.h
> +++ b/drivers/irqchip/irq-gic-common.h
> @@ -13,6 +13,7 @@
>  struct gic_quirk {
>         const char *desc;
>         const char *compatible;
> +       const char *property;
>         bool (*init)(void *data);
>         u32 iidr;
>         u32 mask;
> diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
> index 6fcee221f201..a605aa79435a 100644
> --- a/drivers/irqchip/irq-gic-v3.c
> +++ b/drivers/irqchip/irq-gic-v3.c
> @@ -39,6 +39,7 @@
>
>  #define FLAGS_WORKAROUND_GICR_WAKER_MSM8996    (1ULL << 0)
>  #define FLAGS_WORKAROUND_CAVIUM_ERRATUM_38539  (1ULL << 1)
> +#define FLAGS_WORKAROUND_MTK_GICR_SAVE         (1ULL << 2)
>
>  #define GIC_IRQ_TYPE_PARTITION (GIC_IRQ_TYPE_LPI + 1)
>
> @@ -1720,6 +1721,15 @@ static bool gic_enable_quirk_msm8996(void *data)
>         return true;
>  }
>
> +static bool gic_enable_quirk_mtk_gicr(void *data)
> +{
> +       struct gic_chip_data *d = data;
> +
> +       d->flags |= FLAGS_WORKAROUND_MTK_GICR_SAVE;
> +
> +       return true;
> +}
> +
>  static bool gic_enable_quirk_cavium_38539(void *data)
>  {
>         struct gic_chip_data *d = data;
> @@ -1792,6 +1802,11 @@ static const struct gic_quirk gic_quirks[] = {
>                 .compatible = "qcom,msm8996-gic-v3",
>                 .init   = gic_enable_quirk_msm8996,
>         },
> +       {
> +               .desc   = "GICv3: Mediatek Chromebook GICR save problem",
> +               .property = "mediatek,broken-save-restore-fw",
> +               .init   = gic_enable_quirk_mtk_gicr,
> +       },
>         {
>                 .desc   = "GICv3: HIP06 erratum 161010803",
>                 .iidr   = 0x0204043b,
> @@ -1834,6 +1849,11 @@ static void gic_enable_nmi_support(void)
>         if (!gic_prio_masking_enabled())
>                 return;
>
> +       if (gic_data.flags & FLAGS_WORKAROUND_MTK_GICR_SAVE) {
> +               pr_warn("Skipping NMI enable due to firmware issues\n");
> +               return;
> +       }
> +
>         ppi_nmi_refs = kcalloc(gic_data.ppi_nr, sizeof(*ppi_nmi_refs), GFP_KERNEL);
>         if (!ppi_nmi_refs)
>                 return;
> --
> 2.40.1.606.ga4b1b128d6-goog

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds