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

[Ahmed Tiba <ahmed.tiba@arm.com>]

On Fri, 19 Dec 2025 13:00:08 +0000, Will Deacon wrote:
>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.

The DT provider is intended to mirror the GHES path, so both need to supply a
fixmap slot to satisfy the estatus core interface.

I could drop FIX_ESTATUS_IRQ entirely, but that would require relaxing the
estatus core so a provider can explicitly indicate that it does not use a
fixmap and instead relies on a permanent mapping. If we want to stay aligned
with the ACPI/GHES model, keeping the fixmap-based approach seems preferable.

Thanks,
Ahmed