Re: [PATCH 11/12] ras: add DeviceTree estatus provider driver

[Will Deacon <will@kernel.org>]

On Fri, Dec 19, 2025 at 09:02:35AM +0000, Ahmed Tiba wrote:
> On Thu, 18 Dec 2025 03:19:17PM +0000, Will Deacon wrote:
> > On Thu, Dec 18, 2025 at 01:42:47PM +0000, Ahmed Tiba wrote:
> >> On Thu, 18 Dec 2025 12:13:25PM +0000, Will Deacon wrote:
> >> >> Introduce a platform driver that maps the CPER status block described
> >> >> in DeviceTree, feeds it into the estatus core and handles either IRQ- or
> >> >> poll-driven notifications. Arm64 gains a FIX_ESTATUS_IRQ slot so the
> >> >> driver can safely map the shared buffer while copying records.
> >> >>
> >> >> Signed-off-by: Ahmed Tiba <ahmed.tiba@arm.com>
> >> >> ---
> >> >>  MAINTAINERS                     |   1 +
> >> >>  arch/arm64/include/asm/fixmap.h |   5 +
> >> >>  drivers/ras/Kconfig             |  14 ++
> >> >>  drivers/ras/Makefile            |   1 +
> >> >>  drivers/ras/estatus-dt.c        | 318 ++++++++++++++++++++++++++++++++
> >> >>  include/linux/estatus.h         |   3 +-
> >> >>  6 files changed, 341 insertions(+), 1 deletion(-)
> >> >>  create mode 100644 drivers/ras/estatus-dt.c
> >> >>
> >> >> diff --git a/MAINTAINERS b/MAINTAINERS
> >> >> index 6b2ef2ddc0c7..5567d5e82053 100644
> >> >> --- a/MAINTAINERS
> >> >> +++ b/MAINTAINERS
> >> >> @@ -21761,6 +21761,7 @@ RAS ERROR STATUS
> >> >>  M:   Ahmed Tiba <ahmed.tiba@arm.com>
> >> >>  S:   Maintained
> >> >>  F:   Documentation/devicetree/bindings/ras/arm,ras-ffh.yaml
> >> >> +F:   drivers/ras/estatus-dt.c
> >> >>  F:   drivers/firmware/efi/estatus.c
> >> >>  F:   include/linux/estatus.h
> >> >>
> >> >> diff --git a/arch/arm64/include/asm/fixmap.h b/arch/arm64/include/asm/fixmap.h
> >> >> index 65555284446e..85ffba87bab9 100644
> >> >> --- a/arch/arm64/include/asm/fixmap.h
> >> >> +++ b/arch/arm64/include/asm/fixmap.h
> >> >> @@ -64,6 +64,11 @@ enum fixed_addresses {
> >> >>  #endif
> >> >>  #endif /* CONFIG_ACPI_APEI_GHES */
> >> >>
> >> >> +#ifdef CONFIG_RAS_ESTATUS_DT
> >> >> +     /* Used for ESTATUS mapping from assorted contexts */
> >> >> +     FIX_ESTATUS_IRQ,
> >> >> +#endif /* CONFIG_RAS_ESTATUS_DT */
> >> >
> >> > Why do we need this in addition to the four existing GHES slots? The DT
> >> > code doesn't use it and I was assuming that the ACPI code would continue
> >> > to use the existing irq; is that not the case?
> >>
> >>
> >> We still need a dedicated slot when only the DT provider is built.
> >> All four GHES slots are defined as part of the ACPI implementation,
> >> so they are not present in a DT-only configuration.
> >>
> >> The estatus core always requests a fixmap index from each provider
> >> before copying a CPER record. As a result, the DT driver must supply
> >> its own slot to return a valid enum value to satisfy the common code.
> >
> > Sorry, but I still don't follow this. The DT code doesn't use the fixmap,
> > does it? It looks like it maps the buffer ahead of time using
> > devm_ioremap_resource() and then the accessors don't use the fixmap
> > index at all, hence the horrible '(void)fixmap_idx;' cast which presumably
> > stops the compiler from complaining about an unused variable.
> 
> Correct. The current DT driver keeps the CPER buffer permanently mapped with
> devm_ioremap_resource() and that (void)fixmap_idx; line is just silencing
> the warning. I’ll fix that by dropping the permanent mapping and copying the
> status block via the fixmap entry, so the DT implementation mirrors GHES. That
> gets rid of the cast and makes FIX_ESTATUS_IRQ do real work.

Why can't you just drop FIX_ESTATUS_IRQ entirely? Your original
justification was:

> We still need a dedicated slot when only the DT provider is built.

but as above, the DT driver doesn't actually need it.

Will