[PATCH] ACPI: APEI: move edac_init ahead of ghes platform drv register

[PATCH] ACPI: APEI: move edac_init ahead of ghes platform drv register

[Jia He]

Commit dc4e8c07e9e2 ("ACPI: APEI: explicit init of HEST and GHES in
apci_init()") introduced a bug that invoking ghes_edac_register()
before edac_init(). Because at that time, the bus "edac" hasn't been
registered, this created sysfs /devices/mc0 instead of
/sys/devices/system/edac/mc/mc0 and caused a sysfs dup splat on an
Ampere eMag server:
 sysfs: cannot create duplicate filename '/devices/mc0'
 CPU: 19 PID: 1 Comm: swapper/0 Not tainted 5.19.0+ #138
 random: crng init done
 Hardware name: MiTAC RAPTOR EV-883832-X3-0001/RAPTOR, BIOS 0.14 02/22/2019
 Call trace:
  sysfs_warn_dup+0x6c/0x88
  sysfs_create_dir_ns+0xec/0x108
  kobject_add_internal+0xc0/0x328
  kobject_add+0x94/0x108
  device_add+0x104/0x8b0
  pmu_dev_alloc+0xb4/0x128
  perf_pmu_register+0x308/0x438
  xgene_pmu_dev_add+0x168/0x2c8
  acpi_pmu_dev_add+0x1f0/0x370
  acpi_ns_walk_namespace+0x16c/0x1ec
  acpi_walk_namespace+0xb0/0xf8
  xgene_pmu_probe+0x6b8/0x8a0
  platform_probe+0x70/0xe0
  really_probe+0x164/0x3b0
  __driver_probe_device+0x11c/0x190
  driver_probe_device+0x44/0xf8
  __driver_attach+0xc4/0x1b8
  bus_for_each_dev+0x78/0xd0
  driver_attach+0x2c/0x38
  bus_add_driver+0x150/0x240
  driver_register+0x6c/0x128
  __platform_driver_register+0x30/0x40
  xgene_pmu_driver_init+0x24/0x30
  do_one_initcall+0x50/0x248
  kernel_init_freeable+0x284/0x328
  kernel_init+0x2c/0x140
  ret_from_fork+0x10/0x20
 kobject_add_internal failed for mc0 with -EEXIST, don't try to register things with the same name in the same

This patch fixes it by moving edac_init() into acpi_ghes_init() and ahead of
platform_driver_register().

Fixes: dc4e8c07e9e2 ("ACPI: APEI: explicit init of HEST and GHES in apci_init()")
Signed-off-by: Jia He <justin.he@arm.com>
Cc: Shuai Xue <xueshuai@linux.alibaba.com>
---
 drivers/acpi/apei/ghes.c   | 1 +
 drivers/edac/edac_module.c | 3 +--
 include/linux/edac.h       | 1 +
 3 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
index d91ad378c00d..1127dfffeeb0 100644
--- a/drivers/acpi/apei/ghes.c
+++ b/drivers/acpi/apei/ghes.c
@@ -1462,6 +1462,7 @@ void __init acpi_ghes_init(void)
 	int rc;
 
 	sdei_init();
+	edac_init();
 
 	if (acpi_disabled)
 		return;
diff --git a/drivers/edac/edac_module.c b/drivers/edac/edac_module.c
index 32a931d0cb71..34ada2064b36 100644
--- a/drivers/edac/edac_module.c
+++ b/drivers/edac/edac_module.c
@@ -99,7 +99,7 @@ EXPORT_SYMBOL_GPL(edac_get_sysfs_subsys);
  * edac_init
  *      module initialization entry point
  */
-static int __init edac_init(void)
+int __init edac_init(void)
 {
 	int err = 0;
 
@@ -160,7 +160,6 @@ static void __exit edac_exit(void)
 /*
  * Inform the kernel of our entry and exit points
  */
-subsys_initcall(edac_init);
 module_exit(edac_exit);
 
 MODULE_LICENSE("GPL");
diff --git a/include/linux/edac.h b/include/linux/edac.h
index e730b3468719..104b22c2c177 100644
--- a/include/linux/edac.h
+++ b/include/linux/edac.h
@@ -30,6 +30,7 @@ struct device;
 
 extern int edac_op_state;
 
+int __init edac_init(void);
 struct bus_type *edac_get_sysfs_subsys(void);
 
 static inline void opstate_init(void)
-- 
2.25.1

Re: [PATCH] ACPI: APEI: move edac_init ahead of ghes platform drv register

[Rafael J. Wysocki]

On Fri, Aug 5, 2022 at 4:32 AM Jia He <justin.he@arm.com> wrote:
>
> Commit dc4e8c07e9e2 ("ACPI: APEI: explicit init of HEST and GHES in
> apci_init()") introduced a bug that invoking ghes_edac_register()
> before edac_init(). Because at that time, the bus "edac" hasn't been
> registered, this created sysfs /devices/mc0 instead of
> /sys/devices/system/edac/mc/mc0 and caused a sysfs dup splat on an
> Ampere eMag server:
>  sysfs: cannot create duplicate filename '/devices/mc0'
>  CPU: 19 PID: 1 Comm: swapper/0 Not tainted 5.19.0+ #138
>  random: crng init done
>  Hardware name: MiTAC RAPTOR EV-883832-X3-0001/RAPTOR, BIOS 0.14 02/22/2019
>  Call trace:
>   sysfs_warn_dup+0x6c/0x88
>   sysfs_create_dir_ns+0xec/0x108
>   kobject_add_internal+0xc0/0x328
>   kobject_add+0x94/0x108
>   device_add+0x104/0x8b0
>   pmu_dev_alloc+0xb4/0x128
>   perf_pmu_register+0x308/0x438
>   xgene_pmu_dev_add+0x168/0x2c8
>   acpi_pmu_dev_add+0x1f0/0x370
>   acpi_ns_walk_namespace+0x16c/0x1ec
>   acpi_walk_namespace+0xb0/0xf8
>   xgene_pmu_probe+0x6b8/0x8a0
>   platform_probe+0x70/0xe0
>   really_probe+0x164/0x3b0
>   __driver_probe_device+0x11c/0x190
>   driver_probe_device+0x44/0xf8
>   __driver_attach+0xc4/0x1b8
>   bus_for_each_dev+0x78/0xd0
>   driver_attach+0x2c/0x38
>   bus_add_driver+0x150/0x240
>   driver_register+0x6c/0x128
>   __platform_driver_register+0x30/0x40
>   xgene_pmu_driver_init+0x24/0x30
>   do_one_initcall+0x50/0x248
>   kernel_init_freeable+0x284/0x328
>   kernel_init+0x2c/0x140
>   ret_from_fork+0x10/0x20
>  kobject_add_internal failed for mc0 with -EEXIST, don't try to register things with the same name in the same
>
> This patch fixes it by moving edac_init() into acpi_ghes_init() and ahead of
> platform_driver_register().
>
> Fixes: dc4e8c07e9e2 ("ACPI: APEI: explicit init of HEST and GHES in apci_init()")
> Signed-off-by: Jia He <justin.he@arm.com>
> Cc: Shuai Xue <xueshuai@linux.alibaba.com>
> ---
>  drivers/acpi/apei/ghes.c   | 1 +
>  drivers/edac/edac_module.c | 3 +--
>  include/linux/edac.h       | 1 +
>  3 files changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
> index d91ad378c00d..1127dfffeeb0 100644
> --- a/drivers/acpi/apei/ghes.c
> +++ b/drivers/acpi/apei/ghes.c
> @@ -1462,6 +1462,7 @@ void __init acpi_ghes_init(void)
>         int rc;
>
>         sdei_init();
> +       edac_init();
>
>         if (acpi_disabled)
>                 return;
> diff --git a/drivers/edac/edac_module.c b/drivers/edac/edac_module.c
> index 32a931d0cb71..34ada2064b36 100644
> --- a/drivers/edac/edac_module.c
> +++ b/drivers/edac/edac_module.c
> @@ -99,7 +99,7 @@ EXPORT_SYMBOL_GPL(edac_get_sysfs_subsys);
>   * edac_init
>   *      module initialization entry point
>   */
> -static int __init edac_init(void)
> +int __init edac_init(void)
>  {
>         int err = 0;
>
> @@ -160,7 +160,6 @@ static void __exit edac_exit(void)
>  /*
>   * Inform the kernel of our entry and exit points
>   */
> -subsys_initcall(edac_init);

This effectively makes EDAC depend on GHES which may not be always valid AFAICS.

>  module_exit(edac_exit);
>
>  MODULE_LICENSE("GPL");
> diff --git a/include/linux/edac.h b/include/linux/edac.h
> index e730b3468719..104b22c2c177 100644
> --- a/include/linux/edac.h
> +++ b/include/linux/edac.h
> @@ -30,6 +30,7 @@ struct device;
>
>  extern int edac_op_state;
>
> +int __init edac_init(void);
>  struct bus_type *edac_get_sysfs_subsys(void);
>
>  static inline void opstate_init(void)
> --

Re: [PATCH] ACPI: APEI: move edac_init ahead of ghes platform drv register

[Borislav Petkov]

+ Toshi.

On Mon, Aug 08, 2022 at 08:17:58PM +0200, Rafael J. Wysocki wrote:
> This effectively makes EDAC depend on GHES which may not be always
> valid AFAICS.

Yes, and this has been getting on my nerves since forever.

The GHES code which does collect all those errors *forces* the
registration of an EDAC module which does only the reporting.

Which cannot be any more backwards.

What should happen is, GHES inits and starts working on the errors.
Then, at some point later, ghes_edac loads and starts reporting whatever
it gets. If there's no EDAC module, it doesn't report them. The same way
MCA works.

That's it.

And then ghes_edac can be made a normal module again and we can get rid
of this insanity.

Jia, willing to try it?

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

RE: [PATCH] ACPI: APEI: move edac_init ahead of ghes platform drv register

[Kani, Toshi]

On Monday, August 8, 2022 12:38 PM, Borislav Petkov wrote:
> On Mon, Aug 08, 2022 at 08:17:58PM +0200, Rafael J. Wysocki wrote:
> > This effectively makes EDAC depend on GHES which may not be always
> > valid AFAICS.
> 
> Yes, and this has been getting on my nerves since forever.
> 
> The GHES code which does collect all those errors *forces* the registration of
> an EDAC module which does only the reporting.
> 
> Which cannot be any more backwards.
> 
> What should happen is, GHES inits and starts working on the errors.
> Then, at some point later, ghes_edac loads and starts reporting whatever it
> gets. If there's no EDAC module, it doesn't report them. The same way MCA
> works.
> 
> That's it.
> 
> And then ghes_edac can be made a normal module again and we can get rid
> of this insanity.

The following approach may be considerable:
- Separate ghes_edac_register() into two functions, e.g., ghes_edac_register()
and ghes_edac_init().
- ghes_edac_register() only takes the first if-block with IS_ENABLED() & force_load
check, and then calls a new function, edac_set_owner(mod_name), which simply
sets mod_name to edac_mc_owner.  This allows ghes_edac_register() to run
before edac_init(), and sets edac_mc_owner to prevent chipset-specific edac driver
to be loaded before ghes_edac.
- ghes_edac_init() first calls edac_get_owner() to match with its mod_name.  If so,
it performs the rest of the original ghes_edac_register() procedure.  This
ghes_edac_init() is called from the normal module init path, e.g., module_init().

Thanks,
Toshi

Re: [PATCH] ACPI: APEI: move edac_init ahead of ghes platform drv register

[Borislav Petkov]

On Mon, Aug 08, 2022 at 08:40:18PM +0000, Kani, Toshi wrote:
> This allows ghes_edac_register() to run before edac_init(), and sets
> edac_mc_owner to prevent chipset-specific edac driver

So this is the important part: how does it get decided which EDAC driver
to load? The chipset-specific one or the FW glue one?

User, policy, etc?

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

RE: [PATCH] ACPI: APEI: move edac_init ahead of ghes platform drv register

[Kani, Toshi]

On Monday, August 8, 2022 2:57 PM, Borislav Petkov wrote:
> On Mon, Aug 08, 2022 at 08:40:18PM +0000, Kani, Toshi wrote:
> > This allows ghes_edac_register() to run before edac_init(), and sets
> > edac_mc_owner to prevent chipset-specific edac driver
> 
> So this is the important part: how does it get decided which EDAC driver
> to load? The chipset-specific one or the FW glue one?
> 
> User, policy, etc?

Whichever loaded first wins.

Chipset-specific one checks with chipset IDs but does not check with GHES.  
Hence, ghes_edac_register() needs to run before chipset-specific one.

Toshi

Re: [PATCH] ACPI: APEI: move edac_init ahead of ghes platform drv register

[Borislav Petkov]

On Mon, Aug 08, 2022 at 09:11:52PM +0000, Kani, Toshi wrote:
> Whichever loaded first wins.

That I know.

The question is, which one *should* win each time.

IOW, on which platforms should ghes_edac load and on which the
chipset-one?

I'm guessing the answer is the plat_list...

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

RE: [PATCH] ACPI: APEI: move edac_init ahead of ghes platform drv register

[Kani, Toshi]

On Monday, August 8, 2022 3:21 PM, Borislav Petkov wrote:
> On Mon, Aug 08, 2022 at 09:11:52PM +0000, Kani, Toshi wrote:
> > Whichever loaded first wins.
> 
> That I know.
> 
> The question is, which one *should* win each time.
> 
> IOW, on which platforms should ghes_edac load and on which the chipset-
> one?

Platforms with ACPI GHES support should use ghes_edac.  This is the case
on Arm.  The x86 side has additional platform ID check to protect from legacy
buggy GHES FW existed before ghes_edac enablement. 

Toshi

Re: [PATCH] ACPI: APEI: move edac_init ahead of ghes platform drv register

[Borislav Petkov]

On Mon, Aug 08, 2022 at 09:36:13PM +0000, Kani, Toshi wrote:
> Platforms with ACPI GHES support should use ghes_edac. This is the
> case on Arm. The x86 side has additional platform ID check to protect
> from legacy buggy GHES FW existed before ghes_edac enablement.

Sounds to me like we should put all that logic in ghes.c and out of
ghes_edac and the EDAC drivers will query it by doing upon load:

<edac_driver>_init:

	if (ghes_edac_driver_is_preferred())
		return -ENODEV;

And then ghes_edac can become a normal driver module again.

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

回复: [PATCH] ACPI: APEI: move edac_init ahead of ghes platform drv register

[Justin He]

> -----邮件原件-----
> 发件人: Borislav Petkov <bp@alien8.de>
> 发送时间: Tuesday, August 9, 2022 3:41 PM
> 收件人: Kani, Toshi <toshi.kani@hpe.com>
> 抄送: Rafael J. Wysocki <rafael@kernel.org>; Justin He
> <Justin.He@arm.com>; Len Brown <lenb@kernel.org>; James Morse
> <James.Morse@arm.com>; Tony Luck <tony.luck@intel.com>; Mauro
> Carvalho Chehab <mchehab@kernel.org>; Robert Richter <rric@kernel.org>;
> Shuai Xue <xueshuai@linux.alibaba.com>; Jarkko Sakkinen
> <jarkko@kernel.org>; ACPI Devel Maling List <linux-acpi@vger.kernel.org>;
> Linux Kernel Mailing List <linux-kernel@vger.kernel.org>; open
> list:EDAC-CORE <linux-edac@vger.kernel.org>
> 主题: Re: [PATCH] ACPI: APEI: move edac_init ahead of ghes platform drv
> register
>
> On Mon, Aug 08, 2022 at 09:36:13PM +0000, Kani, Toshi wrote:
> > Platforms with ACPI GHES support should use ghes_edac. This is the
> > case on Arm. The x86 side has additional platform ID check to protect
> > from legacy buggy GHES FW existed before ghes_edac enablement.
>
> Sounds to me like we should put all that logic in ghes.c and out of ghes_edac
> and the EDAC drivers will query it by doing upon load:
>
> <edac_driver>_init:
>
>       if (ghes_edac_driver_is_preferred())
>               return -ENODEV;
>
Do you mean the similar logic as what Kani Toshi suggested?
e.g.
bool ghes_edac_driver_is_preferred()
{
        If (mod_name is "ghes_edac")
                return true;

        return false;
}
If no, what is the detail logic of ghes_edac_driver_is_preferred()?
Thanks for the clarification 😊
Because I notice that lots of other edac drivers are probing like:
...
        owner = edac_get_owner();
        if (owner && strncmp(owner, EDAC_MOD_STR, sizeof(EDAC_MOD_STR)))
                return -EBUSY;
...

> And then ghes_edac can become a normal driver module again.
>

--
Cheers,
Justin (Jia He)
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

Re: 回复: [PATCH] ACPI: APEI: move edac_init ahead of ghes platform drv register

[Borislav Petkov]

On Tue, Aug 09, 2022 at 09:24:33AM +0000, Justin He wrote:
> If no, what is the detail logic of ghes_edac_driver_is_preferred()?

That should be moved from ghes_edac.c to ghes.c:

/*
 * Known systems that are safe to enable this module.
 */
static struct acpi_platform_list plat_list[] = {
        {"HPE   ", "Server  ", 0, ACPI_SIG_FADT, all_versions},
        { } /* End */
};

and then

bool ghes_edac_driver_is_preferred()
{
	if (IS_ENABLED(CONFIG_X86)) {
                /* Check if safe to enable on this system */
                idx = acpi_match_platform_list(plat_list);
	} else if (ARM) {
		/* insert ARM logic here */
	}
}

That function should be called by the EDAC modules which compete with
ghes_edac.

In the x86 case, that's sb_edac, skx_edac and amd64_edac, I guess.

It all depends on what platforms Toshi wants to load it - I'm guessing
HPE has both Intel and AMD platforms where they prefer ghes_edac.

On ARM, that's up to ARM folks.

> Because I notice that lots of other edac drivers are probing like:
> ...
>         owner = edac_get_owner();
>         if (owner && strncmp(owner, EDAC_MOD_STR, sizeof(EDAC_MOD_STR)))
>                 return -EBUSY;

No, that's a silly mechanism to allow a single EDAC driver to load on
the system. But your test will go before it, at the very beginning of
the init function.

HTH.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

RE: 回复: [PATCH] ACPI: APEI: move edac_init ahead of ghes platform drv register

[Kani, Toshi]

On Tuesday, August 9, 2022 5:16 AM, Borislav Petkov wrote:
> bool ghes_edac_driver_is_preferred()
> {
> 	if (IS_ENABLED(CONFIG_X86)) {
>                 /* Check if safe to enable on this system */
>                 idx = acpi_match_platform_list(plat_list);
> 	} else if (ARM) {
> 		/* insert ARM logic here */
> 	}
> }
> 
> That function should be called by the EDAC modules which compete with
> ghes_edac.

The logic needs to be latched on ghes presence, i.e., the condition is
(ghes-present) && (ghes-preferred).  Currently, ghes_probe() calls
ghes_edac_register() for this.

> In the x86 case, that's sb_edac, skx_edac and amd64_edac, I guess.

Agree that changing all edac drivers to check with GHES is an option.
In this approach, though, they will need to check with foo_preferred()
when a new FW interface FOO is introduced.
 
> It all depends on what platforms Toshi wants to load it - I'm guessing
> HPE has both Intel and AMD platforms where they prefer ghes_edac.

Yes, and HPE will keep the same platform ID for GHES/FF model.

> On ARM, that's up to ARM folks.
> 
> > Because I notice that lots of other edac drivers are probing like:
> > ...
> >         owner = edac_get_owner();
> >         if (owner && strncmp(owner, EDAC_MOD_STR,
> sizeof(EDAC_MOD_STR)))
> >                 return -EBUSY;
> 
> No, that's a silly mechanism to allow a single EDAC driver to load on
> the system. But your test will go before it, at the very beginning of
> the init function.

Well, this check being interface independent (like GHES) is good at least.

Toshi

Re: 回复: [PATCH] ACPI: APEI: move edac_init ahead of ghes platform drv register

[Borislav Petkov]

On Tue, Aug 09, 2022 at 02:36:33PM +0000, Kani, Toshi wrote:
> The logic needs to be latched on ghes presence, i.e., the condition is

If GHES is not enabled, there'll of course be a stub which returns
false.

> Agree that changing all edac drivers to check with GHES is an option.

Not all - all relevant drivers for your - HPE - use case. We don't load
ghes_edac on anything else. Known-good platforms only, remember?

> In this approach, though, they will need to check with foo_preferred()
> when a new FW interface FOO is introduced.

I'm afraid I don't understand what you mean here.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

RE: 回复: [PATCH] ACPI: APEI: move edac_init ahead of ghes platform drv register

[Kani, Toshi]

On Sent: Tuesday, August 9, 2022 9:14 AM, Borislav Petkov wrote:
> On Tue, Aug 09, 2022 at 02:36:33PM +0000, Kani, Toshi wrote:
> > The logic needs to be latched on ghes presence, i.e., the condition is
> 
> If GHES is not enabled, there'll of course be a stub which returns
> false.

Right.  Just checking since the example pseudo code did not have it.
 
> > Agree that changing all edac drivers to check with GHES is an option.
> 
> Not all - all relevant drivers for your - HPE - use case. We don't load
> ghes_edac on anything else. Known-good platforms only, remember?

I think this should be all edac drivers on x86.

> > In this approach, though, they will need to check with foo_preferred()
> > when a new FW interface FOO is introduced.
> 
> I'm afraid I don't understand what you mean here.

I was referring a hypothetical future case that ACPI GHES might not be
the only FW interface for FF-based memory error reporting table going
forward.

Toshi

Re: 回复: [PATCH] ACPI: APEI: move edac_init ahead of ghes platform drv register

[Borislav Petkov]

On Tue, Aug 09, 2022 at 03:39:43PM +0000, Kani, Toshi wrote:
> I think this should be all edac drivers on x86.

Because?

> I was referring a hypothetical future case that ACPI GHES might not be
> the only FW interface for FF-based memory error reporting table going
> forward.

Don't tell me you have something else in the works which will override
GHES...

Can you guys make up your mind and stick with it?!

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

RE: 回复: [PATCH] ACPI: APEI: move edac_init ahead of ghes platform drv register

[Kani, Toshi]

On Tuesday, August 9, 2022 11:12 AM, Borislav Petkov wrote:
> On Tue, Aug 09, 2022 at 03:39:43PM +0000, Kani, Toshi wrote:
> > I think this should be all edac drivers on x86.
> 
> Because?

ghes_edac.force_load option skips the platform ID check on x86. 

I am not familiar with Arm platforms, but Arm edac drivers may
need this change as well.

> > I was referring a hypothetical future case that ACPI GHES might not be
> > the only FW interface for FF-based memory error reporting table going
> > forward.
> 
> Don't tell me you have something else in the works which will override
> GHES...
> 
> Can you guys make up your mind and stick with it?!

I am not aware of any such effort.

Toshi

RE: 回复: [PATCH] ACPI: APEI: move edac_init ahead of ghes platform drv register

[Justin He]

Hi Borislav

> -----Original Message-----
> From: Borislav Petkov <bp@alien8.de>
> Sent: Tuesday, August 9, 2022 7:16 PM
> To: Justin He <Justin.He@arm.com>
> Cc: Kani, Toshi <toshi.kani@hpe.com>; Rafael J. Wysocki <rafael@kernel.org>;
> Len Brown <lenb@kernel.org>; James Morse <James.Morse@arm.com>; Tony
> Luck <tony.luck@intel.com>; Mauro Carvalho Chehab <mchehab@kernel.org>;
> Robert Richter <rric@kernel.org>; Shuai Xue <xueshuai@linux.alibaba.com>;
> Jarkko Sakkinen <jarkko@kernel.org>; ACPI Devel Maling List
> <linux-acpi@vger.kernel.org>; Linux Kernel Mailing List
> <linux-kernel@vger.kernel.org>; open list:EDAC-CORE
> <linux-edac@vger.kernel.org>
> Subject: Re: 回复: [PATCH] ACPI: APEI: move edac_init ahead of ghes platform
> drv register
>
> On Tue, Aug 09, 2022 at 09:24:33AM +0000, Justin He wrote:
> > If no, what is the detail logic of ghes_edac_driver_is_preferred()?
>
> That should be moved from ghes_edac.c to ghes.c:
>
> /*
>  * Known systems that are safe to enable this module.
>  */
> static struct acpi_platform_list plat_list[] = {
>         {"HPE   ", "Server  ", 0, ACPI_SIG_FADT, all_versions},
>         { } /* End */
> };
>
> and then
>
> bool ghes_edac_driver_is_preferred()
> {
>       if (IS_ENABLED(CONFIG_X86)) {
>                 /* Check if safe to enable on this system */
>                 idx = acpi_match_platform_list(plat_list);
>       } else if (ARM) {
>               /* insert ARM logic here */
>       }
> }
>
> That function should be called by the EDAC modules which compete with
> ghes_edac.
>
> In the x86 case, that's sb_edac, skx_edac and amd64_edac, I guess.

Ok, thanks a lot for elaborating it more.
Let me summarize it before sending v2 (maybe tomorrow):
1. implement a ghes_edac_driver_is_preferred() to replace the old ghes_edac_register()
and move it to ghes.c together with plat_list
2. save the ghes and dev to a global structure and pass it to the new module_init
3.move the remain logic of old ghes_edac_register() to a new XX_init() which is under the
module_init path.
4. introduce a stub ghes_edac_driver_is_preferred() for sb_edac, skx_edac and amd64_edac
to check.
5. add the check condition in the XXX_init of sb_edac, skx_edac and amd64_edac

Please let me know if I missed anything above.


--
Cheers,
Justin (Jia He)


>
> It all depends on what platforms Toshi wants to load it - I'm guessing HPE has
> both Intel and AMD platforms where they prefer ghes_edac.
>
> On ARM, that's up to ARM folks.
>
> > Because I notice that lots of other edac drivers are probing like:
> > ...
> >         owner = edac_get_owner();
> >         if (owner && strncmp(owner, EDAC_MOD_STR,
> sizeof(EDAC_MOD_STR)))
> >                 return -EBUSY;
>
> No, that's a silly mechanism to allow a single EDAC driver to load on the
> system. But your test will go before it, at the very beginning of the init
> function.
>
> HTH.
>
> --
> Regards/Gruss,
>     Boris.
>
> https://people.kernel.org/tglx/notes-about-netiquette
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

Re: 回复: [PATCH] ACPI: APEI: move edac_init ahead of ghes platform drv register

[Borislav Petkov]

On Tue, Aug 09, 2022 at 04:51:45PM +0000, Justin He wrote:
> Let me summarize it before sending v2 (maybe tomorrow):
> 1. implement a ghes_edac_driver_is_preferred() to replace the old ghes_edac_register()
> and move it to ghes.c together with plat_list

No, not replace.

In ghes.c:

- add ghes_edac_driver_is_preferred()
- remove ghes_edac_register() and _unregister() calls from there. Make them proper module
init/exit functions.
- Make ghes_edac.c a proper module - i.e., get rid of this thing:

config EDAC_GHES
        bool "Output ACPI APEI/GHES BIOS detected errors via EDAC"
        depends on ACPI_APEI_GHES && (EDAC=y)
				     ^^^^^^^^

> 2. save the ghes and dev to a global structure and pass it to the new module_init

Not at all...

> 3.move the remain logic of old ghes_edac_register() to a new XX_init() which is under the
> module_init path.
> 4. introduce a stub ghes_edac_driver_is_preferred() for sb_edac, skx_edac and amd64_edac
> to check.
> 5. add the check condition in the XXX_init of sb_edac, skx_edac and amd64_edac
> 
> Please let me know if I missed anything above.

Yes, I think you do. Lemme write something and you can finish it/test
it.

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: 回复: [PATCH] ACPI: APEI: move edac_init ahead of ghes platform drv register

[Borislav Petkov]

On Tue, Aug 09, 2022 at 07:15:43PM +0200, Borislav Petkov wrote:
> Yes, I think you do. Lemme write something and you can finish it/test
> it.

Here's something to only show what I'm thinking of. It doesn't build
because of:

drivers/acpi/apei/ghes.c: In function ‘ghes_do_proc’:
drivers/acpi/apei/ghes.c:651:25: error: implicit declaration of function ‘ghes_edac_report_mem_error’; did you mean ‘arch_apei_report_mem_error’? [-Werror=implicit-function-declaration]
  651 |                         ghes_edac_report_mem_error(sev, mem_err);


and that needs more thinking what to do. My idea currently is to do a
notifier like we do for MCA...

And I'm not really happy about returning a list of ghes devices just so
ghes_edac instances can get their struct device * pointers. Maybe we'll
think of something better.

But this is the general goal: not call module code from builtin code -
i.e., call ghes_edac.c code from ghes.c. It works now because ghes_edac
is forced builtin which is ugly as hell.

Anyway, enough for today...

---
diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
index d91ad378c00d..0919317b8313 100644
--- a/drivers/acpi/apei/ghes.c
+++ b/drivers/acpi/apei/ghes.c
@@ -118,6 +118,9 @@ module_param_named(disable, ghes_disable, bool, 0);
 static LIST_HEAD(ghes_hed);
 static DEFINE_MUTEX(ghes_list_mutex);
 
+static LIST_HEAD(ghes_devs);
+static DEFINE_MUTEX(ghes_devs_mutex);
+
 /*
  * Because the memory area used to transfer hardware error information
  * from BIOS to Linux can be determined only in NMI, IRQ or timer
@@ -1376,7 +1379,11 @@ static int ghes_probe(struct platform_device *ghes_dev)
 
 	platform_set_drvdata(ghes_dev, ghes);
 
-	ghes_edac_register(ghes, &ghes_dev->dev);
+	ghes->dev = &ghes_dev->dev;
+
+	mutex_lock(&ghes_devs_mutex);
+	list_add_tail(&ghes->elist, &ghes_devs);
+	mutex_unlock(&ghes_devs_mutex);
 
 	/* Handle any pending errors right away */
 	spin_lock_irqsave(&ghes_notify_lock_irq, flags);
@@ -1440,8 +1447,6 @@ static int ghes_remove(struct platform_device *ghes_dev)
 
 	ghes_fini(ghes);
 
-	ghes_edac_unregister(ghes);
-
 	kfree(ghes);
 
 	platform_set_drvdata(ghes_dev, NULL);
@@ -1497,3 +1502,25 @@ void __init acpi_ghes_init(void)
 	else
 		pr_info(GHES_PFX "Failed to enable APEI firmware first mode.\n");
 }
+
+/*
+ * Known x86 systems that prefer GHES error reporting:
+ */
+static struct acpi_platform_list plat_list[] = {
+	{"HPE   ", "Server  ", 0, ACPI_SIG_FADT, all_versions},
+	{ } /* End */
+};
+
+struct list_head *ghes_get_devices(bool force)
+{
+	int idx = -1;
+
+	if (IS_ENABLED(CONFIG_X86)) {
+		idx = acpi_match_platform_list(plat_list);
+		if (idx < 0 && !force)
+			return NULL;
+	}
+
+	return &ghes_devs;
+}
+EXPORT_SYMBOL_GPL(ghes_get_devices);
diff --git a/drivers/edac/Kconfig b/drivers/edac/Kconfig
index 17562cf1fe97..df45db81858b 100644
--- a/drivers/edac/Kconfig
+++ b/drivers/edac/Kconfig
@@ -53,8 +53,8 @@ config EDAC_DECODE_MCE
 	  has been initialized.
 
 config EDAC_GHES
-	bool "Output ACPI APEI/GHES BIOS detected errors via EDAC"
-	depends on ACPI_APEI_GHES && (EDAC=y)
+	tristate "Output ACPI APEI/GHES BIOS detected errors via EDAC"
+	depends on ACPI_APEI_GHES
 	select UEFI_CPER
 	help
 	  Not all machines support hardware-driven error report. Some of those
diff --git a/drivers/edac/ghes_edac.c b/drivers/edac/ghes_edac.c
index c8fa7dcfdbd0..da6d1a9e107d 100644
--- a/drivers/edac/ghes_edac.c
+++ b/drivers/edac/ghes_edac.c
@@ -59,6 +59,8 @@ module_param(force_load, bool, 0);
 
 static bool system_scanned;
 
+static struct list_head *ghes_devs;
+
 /* Memory Device - Type 17 of SMBIOS spec */
 struct memdev_dmi_entry {
 	u8 type;
@@ -376,34 +378,15 @@ void ghes_edac_report_mem_error(int sev, struct cper_sec_mem_err *mem_err)
 	spin_unlock_irqrestore(&ghes_lock, flags);
 }
 
-/*
- * Known systems that are safe to enable this module.
- */
-static struct acpi_platform_list plat_list[] = {
-	{"HPE   ", "Server  ", 0, ACPI_SIG_FADT, all_versions},
-	{ } /* End */
-};
-
-int ghes_edac_register(struct ghes *ghes, struct device *dev)
+static int ghes_edac_register(struct device *dev)
 {
 	bool fake = false;
 	struct mem_ctl_info *mci;
 	struct ghes_pvt *pvt;
 	struct edac_mc_layer layers[1];
 	unsigned long flags;
-	int idx = -1;
 	int rc = 0;
 
-	if (IS_ENABLED(CONFIG_X86)) {
-		/* Check if safe to enable on this system */
-		idx = acpi_match_platform_list(plat_list);
-		if (!force_load && idx < 0)
-			return -ENODEV;
-	} else {
-		force_load = true;
-		idx = 0;
-	}
-
 	/* finish another registration/unregistration instance first */
 	mutex_lock(&ghes_reg_mutex);
 
@@ -447,7 +430,7 @@ int ghes_edac_register(struct ghes *ghes, struct device *dev)
 		pr_info("This system has a very crappy BIOS: It doesn't even list the DIMMS.\n");
 		pr_info("Its SMBIOS info is wrong. It is doubtful that the error report would\n");
 		pr_info("work on such system. Use this driver with caution\n");
-	} else if (idx < 0) {
+	} else if (force_load) {
 		pr_info("This EDAC driver relies on BIOS to enumerate memory and get error reports.\n");
 		pr_info("Unfortunately, not all BIOSes reflect the memory layout correctly.\n");
 		pr_info("So, the end result of using this driver varies from vendor to vendor.\n");
@@ -517,7 +500,7 @@ int ghes_edac_register(struct ghes *ghes, struct device *dev)
 	return rc;
 }
 
-void ghes_edac_unregister(struct ghes *ghes)
+static void ghes_edac_unregister(struct ghes *ghes)
 {
 	struct mem_ctl_info *mci;
 	unsigned long flags;
@@ -551,3 +534,30 @@ void ghes_edac_unregister(struct ghes *ghes)
 unlock:
 	mutex_unlock(&ghes_reg_mutex);
 }
+
+static int __init ghes_edac_init(void)
+{
+	struct ghes *g, *g_tmp;
+
+	ghes_devs = ghes_get_devices(force_load);
+	if (ghes_devs)
+		return -ENODEV;
+
+	list_for_each_entry_safe(g, g_tmp, ghes_devs, elist) {
+		ghes_edac_register(g->dev);
+	}
+
+	return 0;
+}
+module_init(ghes_edac_init);
+
+static void __exit ghes_edac_exit(void)
+{
+	struct ghes *g, *g_tmp;
+
+
+	list_for_each_entry_safe(g, g_tmp, ghes_devs, elist) {
+		ghes_edac_unregister(g);
+	}
+}
+module_exit(ghes_edac_exit);
diff --git a/include/acpi/ghes.h b/include/acpi/ghes.h
index 34fb3431a8f3..f39b75c3f9c6 100644
--- a/include/acpi/ghes.h
+++ b/include/acpi/ghes.h
@@ -27,6 +27,8 @@ struct ghes {
 		struct timer_list timer;
 		unsigned int irq;
 	};
+	struct device *dev;
+	struct list_head elist;
 };
 
 struct ghes_estatus_node {
@@ -69,35 +71,13 @@ int ghes_register_vendor_record_notifier(struct notifier_block *nb);
  * @nb: pointer to the notifier_block structure of the vendor record handler.
  */
 void ghes_unregister_vendor_record_notifier(struct notifier_block *nb);
+struct list_head *ghes_get_devices(bool force);
+#else
+static inline struct list_head *ghes_get_devices(bool force) { return NULL; }
 #endif
 
 int ghes_estatus_pool_init(int num_ghes);
 
-/* From drivers/edac/ghes_edac.c */
-
-#ifdef CONFIG_EDAC_GHES
-void ghes_edac_report_mem_error(int sev, struct cper_sec_mem_err *mem_err);
-
-int ghes_edac_register(struct ghes *ghes, struct device *dev);
-
-void ghes_edac_unregister(struct ghes *ghes);
-
-#else
-static inline void ghes_edac_report_mem_error(int sev,
-				       struct cper_sec_mem_err *mem_err)
-{
-}
-
-static inline int ghes_edac_register(struct ghes *ghes, struct device *dev)
-{
-	return -ENODEV;
-}
-
-static inline void ghes_edac_unregister(struct ghes *ghes)
-{
-}
-#endif
-
 static inline int acpi_hest_get_version(struct acpi_hest_generic_data *gdata)
 {
 	return gdata->revision >> 8;

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

RE: 回复: [PATCH] ACPI: APEI: move edac_init ahead of ghes platform drv register

[Justin He]

Hi Borislav

> -----Original Message-----
> From: Borislav Petkov <bp@alien8.de>
> Sent: Wednesday, August 10, 2022 2:46 AM
> To: Justin He <Justin.He@arm.com>
> Cc: Kani, Toshi <toshi.kani@hpe.com>; Rafael J. Wysocki <rafael@kernel.org>;
> Len Brown <lenb@kernel.org>; James Morse <James.Morse@arm.com>;
> Tony Luck <tony.luck@intel.com>; Mauro Carvalho Chehab
> <mchehab@kernel.org>; Robert Richter <rric@kernel.org>; Shuai Xue
> <xueshuai@linux.alibaba.com>; Jarkko Sakkinen <jarkko@kernel.org>; ACPI
> Devel Maling List <linux-acpi@vger.kernel.org>; Linux Kernel Mailing List
> <linux-kernel@vger.kernel.org>; open list:EDAC-CORE
> <linux-edac@vger.kernel.org>
> Subject: Re: 回复: [PATCH] ACPI: APEI: move edac_init ahead of ghes
> platform drv register
>
> On Tue, Aug 09, 2022 at 07:15:43PM +0200, Borislav Petkov wrote:
> > Yes, I think you do. Lemme write something and you can finish it/test
> > it.
>
> Here's something to only show what I'm thinking of. It doesn't build because
> of:
>
> drivers/acpi/apei/ghes.c: In function ‘ghes_do_proc’:
> drivers/acpi/apei/ghes.c:651:25: error: implicit declaration of function
> ‘ghes_edac_report_mem_error’; did you mean
> ‘arch_apei_report_mem_error’? [-Werror=implicit-function-declaration]
>   651 |                         ghes_edac_report_mem_error(sev,
> mem_err);
>
>
> and that needs more thinking what to do. My idea currently is to do a notifier
> like we do for MCA...

There is still a small gap even if we use notifier for ghes edac report mem error.

The notifier will be registered only when calling module_init() of ghes_edac module.
Then there will be different behavior for ghes_do_proc:
1. if ghes_do_proc is invoked in the acpi_init code path, the notifier has NOT been registered,
ghes_edac_report_mem_err() does nothing.
2.If ghes_do_proc is invoked after module_init(), the notifier is registered, everything is fine.

Is this strange or any other side effects?

--
Cheers,
Justin (Jia He)
>
> And I'm not really happy about returning a list of ghes devices just so
> ghes_edac instances can get their struct device * pointers. Maybe we'll think
> of something better.
>
> But this is the general goal: not call module code from builtin code - i.e., call
> ghes_edac.c code from ghes.c. It works now because ghes_edac is forced
> builtin which is ugly as hell.
>
> Anyway, enough for today...
>
> ---
> diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c index
> d91ad378c00d..0919317b8313 100644
> --- a/drivers/acpi/apei/ghes.c
> +++ b/drivers/acpi/apei/ghes.c
> @@ -118,6 +118,9 @@ module_param_named(disable, ghes_disable, bool,
> 0);  static LIST_HEAD(ghes_hed);  static DEFINE_MUTEX(ghes_list_mutex);
>
> +static LIST_HEAD(ghes_devs);
> +static DEFINE_MUTEX(ghes_devs_mutex);
> +
>  /*
>   * Because the memory area used to transfer hardware error information
>   * from BIOS to Linux can be determined only in NMI, IRQ or timer @@
> -1376,7 +1379,11 @@ static int ghes_probe(struct platform_device
> *ghes_dev)
>
>       platform_set_drvdata(ghes_dev, ghes);
>
> -     ghes_edac_register(ghes, &ghes_dev->dev);
> +     ghes->dev = &ghes_dev->dev;
> +
> +     mutex_lock(&ghes_devs_mutex);
> +     list_add_tail(&ghes->elist, &ghes_devs);
> +     mutex_unlock(&ghes_devs_mutex);
>
>       /* Handle any pending errors right away */
>       spin_lock_irqsave(&ghes_notify_lock_irq, flags); @@ -1440,8 +1447,6
> @@ static int ghes_remove(struct platform_device *ghes_dev)
>
>       ghes_fini(ghes);
>
> -     ghes_edac_unregister(ghes);
> -
>       kfree(ghes);
>
>       platform_set_drvdata(ghes_dev, NULL);
> @@ -1497,3 +1502,25 @@ void __init acpi_ghes_init(void)
>       else
>               pr_info(GHES_PFX "Failed to enable APEI firmware first
> mode.\n");  }
> +
> +/*
> + * Known x86 systems that prefer GHES error reporting:
> + */
> +static struct acpi_platform_list plat_list[] = {
> +     {"HPE   ", "Server  ", 0, ACPI_SIG_FADT, all_versions},
> +     { } /* End */
> +};
> +
> +struct list_head *ghes_get_devices(bool force) {
> +     int idx = -1;
> +
> +     if (IS_ENABLED(CONFIG_X86)) {
> +             idx = acpi_match_platform_list(plat_list);
> +             if (idx < 0 && !force)
> +                     return NULL;
> +     }
> +
> +     return &ghes_devs;
> +}
> +EXPORT_SYMBOL_GPL(ghes_get_devices);
> diff --git a/drivers/edac/Kconfig b/drivers/edac/Kconfig index
> 17562cf1fe97..df45db81858b 100644
> --- a/drivers/edac/Kconfig
> +++ b/drivers/edac/Kconfig
> @@ -53,8 +53,8 @@ config EDAC_DECODE_MCE
>         has been initialized.
>
>  config EDAC_GHES
> -     bool "Output ACPI APEI/GHES BIOS detected errors via EDAC"
> -     depends on ACPI_APEI_GHES && (EDAC=y)
> +     tristate "Output ACPI APEI/GHES BIOS detected errors via EDAC"
> +     depends on ACPI_APEI_GHES
>       select UEFI_CPER
>       help
>         Not all machines support hardware-driven error report. Some of those
> diff --git a/drivers/edac/ghes_edac.c b/drivers/edac/ghes_edac.c index
> c8fa7dcfdbd0..da6d1a9e107d 100644
> --- a/drivers/edac/ghes_edac.c
> +++ b/drivers/edac/ghes_edac.c
> @@ -59,6 +59,8 @@ module_param(force_load, bool, 0);
>
>  static bool system_scanned;
>
> +static struct list_head *ghes_devs;
> +
>  /* Memory Device - Type 17 of SMBIOS spec */  struct memdev_dmi_entry
> {
>       u8 type;
> @@ -376,34 +378,15 @@ void ghes_edac_report_mem_error(int sev, struct
> cper_sec_mem_err *mem_err)
>       spin_unlock_irqrestore(&ghes_lock, flags);  }
>
> -/*
> - * Known systems that are safe to enable this module.
> - */
> -static struct acpi_platform_list plat_list[] = {
> -     {"HPE   ", "Server  ", 0, ACPI_SIG_FADT, all_versions},
> -     { } /* End */
> -};
> -
> -int ghes_edac_register(struct ghes *ghes, struct device *dev)
> +static int ghes_edac_register(struct device *dev)
>  {
>       bool fake = false;
>       struct mem_ctl_info *mci;
>       struct ghes_pvt *pvt;
>       struct edac_mc_layer layers[1];
>       unsigned long flags;
> -     int idx = -1;
>       int rc = 0;
>
> -     if (IS_ENABLED(CONFIG_X86)) {
> -             /* Check if safe to enable on this system */
> -             idx = acpi_match_platform_list(plat_list);
> -             if (!force_load && idx < 0)
> -                     return -ENODEV;
> -     } else {
> -             force_load = true;
> -             idx = 0;
> -     }
> -
>       /* finish another registration/unregistration instance first */
>       mutex_lock(&ghes_reg_mutex);
>
> @@ -447,7 +430,7 @@ int ghes_edac_register(struct ghes *ghes, struct
> device *dev)
>               pr_info("This system has a very crappy BIOS: It doesn't even list the
> DIMMS.\n");
>               pr_info("Its SMBIOS info is wrong. It is doubtful that the error report
> would\n");
>               pr_info("work on such system. Use this driver with caution\n");
> -     } else if (idx < 0) {
> +     } else if (force_load) {
>               pr_info("This EDAC driver relies on BIOS to enumerate memory and
> get error reports.\n");
>               pr_info("Unfortunately, not all BIOSes reflect the memory layout
> correctly.\n");
>               pr_info("So, the end result of using this driver varies from vendor to
> vendor.\n"); @@ -517,7 +500,7 @@ int ghes_edac_register(struct ghes *ghes,
> struct device *dev)
>       return rc;
>  }
>
> -void ghes_edac_unregister(struct ghes *ghes)
> +static void ghes_edac_unregister(struct ghes *ghes)
>  {
>       struct mem_ctl_info *mci;
>       unsigned long flags;
> @@ -551,3 +534,30 @@ void ghes_edac_unregister(struct ghes *ghes)
>  unlock:
>       mutex_unlock(&ghes_reg_mutex);
>  }
> +
> +static int __init ghes_edac_init(void)
> +{
> +     struct ghes *g, *g_tmp;
> +
> +     ghes_devs = ghes_get_devices(force_load);
> +     if (ghes_devs)
> +             return -ENODEV;
> +
> +     list_for_each_entry_safe(g, g_tmp, ghes_devs, elist) {
> +             ghes_edac_register(g->dev);
> +     }
> +
> +     return 0;
> +}
> +module_init(ghes_edac_init);
> +
> +static void __exit ghes_edac_exit(void) {
> +     struct ghes *g, *g_tmp;
> +
> +
> +     list_for_each_entry_safe(g, g_tmp, ghes_devs, elist) {
> +             ghes_edac_unregister(g);
> +     }
> +}
> +module_exit(ghes_edac_exit);
> diff --git a/include/acpi/ghes.h b/include/acpi/ghes.h index
> 34fb3431a8f3..f39b75c3f9c6 100644
> --- a/include/acpi/ghes.h
> +++ b/include/acpi/ghes.h
> @@ -27,6 +27,8 @@ struct ghes {
>               struct timer_list timer;
>               unsigned int irq;
>       };
> +     struct device *dev;
> +     struct list_head elist;
>  };
>
>  struct ghes_estatus_node {
> @@ -69,35 +71,13 @@ int ghes_register_vendor_record_notifier(struct
> notifier_block *nb);
>   * @nb: pointer to the notifier_block structure of the vendor record handler.
>   */
>  void ghes_unregister_vendor_record_notifier(struct notifier_block *nb);
> +struct list_head *ghes_get_devices(bool force); #else static inline
> +struct list_head *ghes_get_devices(bool force) { return NULL; }
>  #endif
>
>  int ghes_estatus_pool_init(int num_ghes);
>
> -/* From drivers/edac/ghes_edac.c */
> -
> -#ifdef CONFIG_EDAC_GHES
> -void ghes_edac_report_mem_error(int sev, struct cper_sec_mem_err
> *mem_err);
> -
> -int ghes_edac_register(struct ghes *ghes, struct device *dev);
> -
> -void ghes_edac_unregister(struct ghes *ghes);
> -
> -#else
> -static inline void ghes_edac_report_mem_error(int sev,
> -                                    struct cper_sec_mem_err *mem_err)
> -{
> -}
> -
> -static inline int ghes_edac_register(struct ghes *ghes, struct device *dev) -{
> -     return -ENODEV;
> -}
> -
> -static inline void ghes_edac_unregister(struct ghes *ghes) -{ -} -#endif
> -
>  static inline int acpi_hest_get_version(struct acpi_hest_generic_data *gdata)
> {
>       return gdata->revision >> 8;
>
> --
> Regards/Gruss,
>     Boris.
>
> https://people.kernel.org/tglx/notes-about-netiquette
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

Re: 回复: [PATCH] ACPI: APEI: move edac_init ahead of ghes platform drv register

[Borislav Petkov]

On Wed, Aug 10, 2022 at 06:00:49AM +0000, Justin He wrote:
> Is this strange or any other side effects?

This is no different than what we do now on x86 MCA.

If the logging machinery is modules, they consume error records only
when loaded.

> IMPORTANT NOTICE: The contents of this email and any attachments are
> confidential and may also be privileged. If you are not the intended
> recipient, please notify the sender immediately and do not disclose
> the contents to any other person, use it for any purpose, or store or
> copy the information in any medium. Thank you.

Btw, pls get rid of this if you want to be posting on public mailing
lists. You can ask your other ARM colleagues how they solved it. :)

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

RE: 回复: [PATCH] ACPI: APEI: move edac_init ahead of ghes platform drv register

[Justin He]

> -----Original Message-----
> From: Borislav Petkov <bp@alien8.de>
> Sent: Wednesday, August 10, 2022 4:28 PM
> To: Justin He <Justin.He@arm.com>
> Cc: Kani, Toshi <toshi.kani@hpe.com>; Rafael J. Wysocki <rafael@kernel.org>;
> Len Brown <lenb@kernel.org>; James Morse <James.Morse@arm.com>;
> Tony Luck <tony.luck@intel.com>; Mauro Carvalho Chehab
> <mchehab@kernel.org>; Robert Richter <rric@kernel.org>; Shuai Xue
> <xueshuai@linux.alibaba.com>; Jarkko Sakkinen <jarkko@kernel.org>; ACPI
> Devel Maling List <linux-acpi@vger.kernel.org>; Linux Kernel Mailing List
> <linux-kernel@vger.kernel.org>; open list:EDAC-CORE
> <linux-edac@vger.kernel.org>
> Subject: Re: 回复: [PATCH] ACPI: APEI: move edac_init ahead of ghes
> platform drv register
> 
> On Wed, Aug 10, 2022 at 06:00:49AM +0000, Justin He wrote:
> > Is this strange or any other side effects?
> 
> This is no different than what we do now on x86 MCA.
> 
> If the logging machinery is modules, they consume error records only when
> loaded.
Ok, got it, thanks a lot
From my local testing, after the provided patch from you + notifier register.
It works after I resolved a few other issues.

When I verify different building kconfig(X86/Arm64, ghes_edac[M]/[*]), and booting, I will send out
the new version.


> 
> Btw, pls get rid of this if you want to be posting on public mailing lists. You can
> ask your other ARM colleagues how they solved it. :)
> 
Sorry, missed one configuration in the mailbox ☹


--
Cheers,
Justin (Jia He)

RE: 回复: [PATCH] ACPI: APEI: move edac_init ahead of ghes platform drv register

[Justin He]

Hi Borislav

> -----Original Message-----
> From: Borislav Petkov <bp@alien8.de>
> Sent: Wednesday, August 10, 2022 2:46 AM
> To: Justin He <Justin.He@arm.com>
> Cc: Kani, Toshi <toshi.kani@hpe.com>; Rafael J. Wysocki <rafael@kernel.org>;
> Len Brown <lenb@kernel.org>; James Morse <James.Morse@arm.com>;
> Tony Luck <tony.luck@intel.com>; Mauro Carvalho Chehab
> <mchehab@kernel.org>; Robert Richter <rric@kernel.org>; Shuai Xue
> <xueshuai@linux.alibaba.com>; Jarkko Sakkinen <jarkko@kernel.org>; ACPI
> Devel Maling List <linux-acpi@vger.kernel.org>; Linux Kernel Mailing List
> <linux-kernel@vger.kernel.org>; open list:EDAC-CORE
> <linux-edac@vger.kernel.org>
> Subject: Re: 回复: [PATCH] ACPI: APEI: move edac_init ahead of ghes
> platform drv register
> 
[...]
> ---
> diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c index
> d91ad378c00d..0919317b8313 100644
> --- a/drivers/acpi/apei/ghes.c
> +++ b/drivers/acpi/apei/ghes.c
> @@ -118,6 +118,9 @@ module_param_named(disable, ghes_disable, bool,
> 0);  static LIST_HEAD(ghes_hed);  static DEFINE_MUTEX(ghes_list_mutex);
> 
> +static LIST_HEAD(ghes_devs);
> +static DEFINE_MUTEX(ghes_devs_mutex);
> +
>  /*
>   * Because the memory area used to transfer hardware error information
>   * from BIOS to Linux can be determined only in NMI, IRQ or timer @@
> -1376,7 +1379,11 @@ static int ghes_probe(struct platform_device
> *ghes_dev)
> 
>  	platform_set_drvdata(ghes_dev, ghes);
> 
> -	ghes_edac_register(ghes, &ghes_dev->dev);
> +	ghes->dev = &ghes_dev->dev;
> +
> +	mutex_lock(&ghes_devs_mutex);
> +	list_add_tail(&ghes->elist, &ghes_devs);
> +	mutex_unlock(&ghes_devs_mutex);
> 
>  	/* Handle any pending errors right away */
>  	spin_lock_irqsave(&ghes_notify_lock_irq, flags); @@ -1440,8 +1447,6
> @@ static int ghes_remove(struct platform_device *ghes_dev)
> 
>  	ghes_fini(ghes);
> 
> -	ghes_edac_unregister(ghes);
> -
>  	kfree(ghes);
> 
>  	platform_set_drvdata(ghes_dev, NULL);
> @@ -1497,3 +1502,25 @@ void __init acpi_ghes_init(void)
>  	else
>  		pr_info(GHES_PFX "Failed to enable APEI firmware first
> mode.\n");  }
> +
> +/*
> + * Known x86 systems that prefer GHES error reporting:
> + */
> +static struct acpi_platform_list plat_list[] = {
> +	{"HPE   ", "Server  ", 0, ACPI_SIG_FADT, all_versions},
> +	{ } /* End */
> +};
> +
> +struct list_head *ghes_get_devices(bool force) {
> +	int idx = -1;
> +
> +	if (IS_ENABLED(CONFIG_X86)) {
> +		idx = acpi_match_platform_list(plat_list);
> +		if (idx < 0 && !force)
> +			return NULL;
> +	}
> +
> +	return &ghes_devs;
> +}
> +EXPORT_SYMBOL_GPL(ghes_get_devices);
> diff --git a/drivers/edac/Kconfig b/drivers/edac/Kconfig index
> 17562cf1fe97..df45db81858b 100644
> --- a/drivers/edac/Kconfig
> +++ b/drivers/edac/Kconfig
> @@ -53,8 +53,8 @@ config EDAC_DECODE_MCE
>  	  has been initialized.
> 
>  config EDAC_GHES
> -	bool "Output ACPI APEI/GHES BIOS detected errors via EDAC"
> -	depends on ACPI_APEI_GHES && (EDAC=y)
> +	tristate "Output ACPI APEI/GHES BIOS detected errors via EDAC"
> +	depends on ACPI_APEI_GHES
>  	select UEFI_CPER
>  	help
>  	  Not all machines support hardware-driven error report. Some of those
> diff --git a/drivers/edac/ghes_edac.c b/drivers/edac/ghes_edac.c index
> c8fa7dcfdbd0..da6d1a9e107d 100644
> --- a/drivers/edac/ghes_edac.c
> +++ b/drivers/edac/ghes_edac.c
> @@ -59,6 +59,8 @@ module_param(force_load, bool, 0);
> 
>  static bool system_scanned;
> 
> +static struct list_head *ghes_devs;
> +
>  /* Memory Device - Type 17 of SMBIOS spec */  struct memdev_dmi_entry
> {
>  	u8 type;
> @@ -376,34 +378,15 @@ void ghes_edac_report_mem_error(int sev, struct
> cper_sec_mem_err *mem_err)
>  	spin_unlock_irqrestore(&ghes_lock, flags);  }
> 
> -/*
> - * Known systems that are safe to enable this module.
> - */
> -static struct acpi_platform_list plat_list[] = {
> -	{"HPE   ", "Server  ", 0, ACPI_SIG_FADT, all_versions},
> -	{ } /* End */
> -};
> -
> -int ghes_edac_register(struct ghes *ghes, struct device *dev)
> +static int ghes_edac_register(struct device *dev)
>  {
>  	bool fake = false;
>  	struct mem_ctl_info *mci;
>  	struct ghes_pvt *pvt;
>  	struct edac_mc_layer layers[1];
>  	unsigned long flags;
> -	int idx = -1;
>  	int rc = 0;
> 
> -	if (IS_ENABLED(CONFIG_X86)) {
> -		/* Check if safe to enable on this system */
> -		idx = acpi_match_platform_list(plat_list);
> -		if (!force_load && idx < 0)
> -			return -ENODEV;
> -	} else {
> -		force_load = true;
> -		idx = 0;
> -	}
> -
>  	/* finish another registration/unregistration instance first */
>  	mutex_lock(&ghes_reg_mutex);
> 
> @@ -447,7 +430,7 @@ int ghes_edac_register(struct ghes *ghes, struct
> device *dev)
>  		pr_info("This system has a very crappy BIOS: It doesn't even list the
> DIMMS.\n");
>  		pr_info("Its SMBIOS info is wrong. It is doubtful that the error report
> would\n");
>  		pr_info("work on such system. Use this driver with caution\n");
> -	} else if (idx < 0) {
> +	} else if (force_load) {
>  		pr_info("This EDAC driver relies on BIOS to enumerate memory and
> get error reports.\n");
>  		pr_info("Unfortunately, not all BIOSes reflect the memory layout
> correctly.\n");
>  		pr_info("So, the end result of using this driver varies from vendor to
> vendor.\n"); @@ -517,7 +500,7 @@ int ghes_edac_register(struct ghes *ghes,
> struct device *dev)
>  	return rc;
>  }
> 
> -void ghes_edac_unregister(struct ghes *ghes)
> +static void ghes_edac_unregister(struct ghes *ghes)
>  {
>  	struct mem_ctl_info *mci;
>  	unsigned long flags;
> @@ -551,3 +534,30 @@ void ghes_edac_unregister(struct ghes *ghes)
>  unlock:
>  	mutex_unlock(&ghes_reg_mutex);
>  }
> +
> +static int __init ghes_edac_init(void)
> +{
> +	struct ghes *g, *g_tmp;
> +
> +	ghes_devs = ghes_get_devices(force_load);
> +	if (ghes_devs)
Should it be changed to if(!ghes_devs) ?
Otherwise, this ghes_edac_init() on Arm will always return with ENODEV
because ! ghes_get_devices().

What do you think of it?

--
Cheers,
Justin (Jia He)


> +		return -ENODEV;
> +
> +	list_for_each_entry_safe(g, g_tmp, ghes_devs, elist) {
> +		ghes_edac_register(g->dev);
> +	}
> +
> +	return 0;
> +}
> +module_init(ghes_edac_init);
> +
> +static void __exit ghes_edac_exit(void) {
> +	struct ghes *g, *g_tmp;
> +
> +
> +	list_for_each_entry_safe(g, g_tmp, ghes_devs, elist) {
> +		ghes_edac_unregister(g);
> +	}
> +}
> +module_exit(ghes_edac_exit);
> diff --git a/include/acpi/ghes.h b/include/acpi/ghes.h index
> 34fb3431a8f3..f39b75c3f9c6 100644
> --- a/include/acpi/ghes.h
> +++ b/include/acpi/ghes.h
> @@ -27,6 +27,8 @@ struct ghes {
>  		struct timer_list timer;
>  		unsigned int irq;
>  	};
> +	struct device *dev;
> +	struct list_head elist;
>  };
> 
>  struct ghes_estatus_node {
> @@ -69,35 +71,13 @@ int ghes_register_vendor_record_notifier(struct
> notifier_block *nb);
>   * @nb: pointer to the notifier_block structure of the vendor record handler.
>   */
>  void ghes_unregister_vendor_record_notifier(struct notifier_block *nb);
> +struct list_head *ghes_get_devices(bool force); #else static inline
> +struct list_head *ghes_get_devices(bool force) { return NULL; }
>  #endif
> 
>  int ghes_estatus_pool_init(int num_ghes);
> 
> -/* From drivers/edac/ghes_edac.c */
> -
> -#ifdef CONFIG_EDAC_GHES
> -void ghes_edac_report_mem_error(int sev, struct cper_sec_mem_err
> *mem_err);
> -
> -int ghes_edac_register(struct ghes *ghes, struct device *dev);
> -
> -void ghes_edac_unregister(struct ghes *ghes);
> -
> -#else
> -static inline void ghes_edac_report_mem_error(int sev,
> -				       struct cper_sec_mem_err *mem_err)
> -{
> -}
> -
> -static inline int ghes_edac_register(struct ghes *ghes, struct device *dev) -{
> -	return -ENODEV;
> -}
> -
> -static inline void ghes_edac_unregister(struct ghes *ghes) -{ -} -#endif
> -
>  static inline int acpi_hest_get_version(struct acpi_hest_generic_data *gdata)
> {
>  	return gdata->revision >> 8;
> 
> --
> Regards/Gruss,
>     Boris.
> 
> https://people.kernel.org/tglx/notes-about-netiquette

Re: 回复: [PATCH] ACPI: APEI: move edac_init ahead of ghes platform drv register

[Borislav Petkov]

On Thu, Aug 11, 2022 at 12:39:10AM +0000, Justin He wrote:
> > +static int __init ghes_edac_init(void)
> > +{
> > +	struct ghes *g, *g_tmp;
> > +
> > +	ghes_devs = ghes_get_devices(force_load);
> > +	if (ghes_devs)
> Should it be changed to if(!ghes_devs) ?
> Otherwise, this ghes_edac_init() on Arm will always return with ENODEV
> because ! ghes_get_devices().

Yes, of course, sorry about that.

I mean, for a lack of a better idea, it returns a list of devices... ;-\

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

回复: [PATCH] ACPI: APEI: move edac_init ahead of ghes platform drv register

[Justin He]

Hi Kani,

> -----邮件原件-----
> 发件人: Kani, Toshi <toshi.kani@hpe.com>
> 发送时间: Tuesday, August 9, 2022 4:40 AM
> 收件人: Borislav Petkov <bp@alien8.de>; Rafael J. Wysocki
> <rafael@kernel.org>
> 抄送: Justin He <Justin.He@arm.com>; Len Brown <lenb@kernel.org>; James
> Morse <James.Morse@arm.com>; Tony Luck <tony.luck@intel.com>; Mauro
> Carvalho Chehab <mchehab@kernel.org>; Robert Richter <rric@kernel.org>;
> Shuai Xue <xueshuai@linux.alibaba.com>; Jarkko Sakkinen
> <jarkko@kernel.org>; ACPI Devel Maling List <linux-acpi@vger.kernel.org>;
> Linux Kernel Mailing List <linux-kernel@vger.kernel.org>; open
> list:EDAC-CORE <linux-edac@vger.kernel.org>
> 主题: RE: [PATCH] ACPI: APEI: move edac_init ahead of ghes platform drv
> register
>
> On Monday, August 8, 2022 12:38 PM, Borislav Petkov wrote:
> > On Mon, Aug 08, 2022 at 08:17:58PM +0200, Rafael J. Wysocki wrote:
> > > This effectively makes EDAC depend on GHES which may not be always
> > > valid AFAICS.
> >
> > Yes, and this has been getting on my nerves since forever.
> >
> > The GHES code which does collect all those errors *forces* the
> > registration of an EDAC module which does only the reporting.
> >
> > Which cannot be any more backwards.
> >
> > What should happen is, GHES inits and starts working on the errors.
> > Then, at some point later, ghes_edac loads and starts reporting
> > whatever it gets. If there's no EDAC module, it doesn't report them.
> > The same way MCA works.
> >
> > That's it.
> >
> > And then ghes_edac can be made a normal module again and we can get
> > rid of this insanity.
>
> The following approach may be considerable:
> - Separate ghes_edac_register() into two functions, e.g., ghes_edac_register()
> and ghes_edac_init().
> - ghes_edac_register() only takes the first if-block with IS_ENABLED() &
> force_load check, and then calls a new function,
> edac_set_owner(mod_name), which simply sets mod_name to
> edac_mc_owner.  This allows ghes_edac_register() to run before edac_init(),
> and sets edac_mc_owner to prevent chipset-specific edac driver to be loaded
> before ghes_edac.
> - ghes_edac_init() first calls edac_get_owner() to match with its mod_name.
> If so, it performs the rest of the original ghes_edac_register() procedure.
> This
> ghes_edac_init() is called from the normal module init path, e.g.,
> module_init().
Thanks for the suggestion, one gap is that under module_init path, how can we pass
 the 2nd parameter of ghes_edac_register (struct device *dev) to the new
* ghes_edac_init()?

IIUC, the parameter of any functions under module_init() path should be void.


--
Cheers,
Justin (Jia He)


IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

RE: [PATCH] ACPI: APEI: move edac_init ahead of ghes platform drv register

[Kani, Toshi]

On Tuesday, August 9, 2022 4:06 AM, Justin He wrote:
> > ghes_edac_init() is called from the normal module init path, e.g.,
> > module_init().
> Thanks for the suggestion, one gap is that under module_init path, how can
> we pass the 2nd parameter of ghes_edac_register (struct device *dev) to
> the new * ghes_edac_init()?
> 
> IIUC, the parameter of any functions under module_init() path should be
> void.

Since simpler version of ghes_edac_register() is still called with 
the parameters first, it can save *dev to a ghes_edac structure for
ghes_edac_init() to use.

Toshi