| Message ID | 20230519201249.31147-1-leoyang.li@nxp.com |
|---|---|
| State | Accepted |
| Commit | 9368aa1882ac7178adcd936cee5f0899dbf76dc4 |
| Headers | show |
| Series | [v2] apei/ghes: correctly return NULL for ghes_get_devices() | expand |
Hi Li, > -----Original Message----- > From: Li Yang <leoyang.li@nxp.com> > Sent: Saturday, May 20, 2023 4:13 AM > To: Rafael J. Wysocki <rafael@kernel.org>; Len Brown <lenb@kernel.org>; > James Morse <James.Morse@arm.com>; Tony Luck <tony.luck@intel.com>; > Borislav Petkov <bp@alien8.de>; Justin He <Justin.He@arm.com> > Cc: Li Yang <leoyang.li@nxp.com>; linux-acpi@vger.kernel.org; > linux-kernel@vger.kernel.org > Subject: [PATCH v2] apei/ghes: correctly return NULL for ghes_get_devices() > > Since 315bada690e0 ("EDAC: Check for GHES preference in the chipset-specific > EDAC drivers"), vendor specific EDAC driver will not probe correctly when > CONFIG_ACPI_APEI_GHES is enabled but no GHES device is present. Make > ghes_get_devices() return NULL when the GHES device list is empty to fix the > problem. > > Fixes: 9057a3f7ac36 ("EDAC/ghes: Prepare to make ghes_edac a proper > module") > Signed-off-by: Li Yang <leoyang.li@nxp.com> > Cc: Jia He <justin.he@arm.com> > --- > > V2: fix the fallthrough case in x86 path > > drivers/acpi/apei/ghes.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c index > 34ad071a64e9..4382fe13ee3e 100644 > --- a/drivers/acpi/apei/ghes.c > +++ b/drivers/acpi/apei/ghes.c > @@ -1544,6 +1544,8 @@ struct list_head *ghes_get_devices(void) > > pr_warn_once("Force-loading ghes_edac on an unsupported > platform. You're on your own!\n"); > } > + } else if (list_empty(&ghes_devs)) { > + return NULL; > } I have no major objections to the fix. Just curious about the edac driver in you platform. IIUC, in your case, CONFIG_ACPI_APEI_GHES is enabled and edac_ghes driver is either not loaded or fails to load. Is my understanding correct? I wonder whether ghes_get_devices() can unblock such check condition and let other chipset-specific edac drivers continue with the initialization. @Toshi, What do u think of it? -- 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.
> -----Original Message----- > From: Justin He <Justin.He@arm.com> > Sent: Thursday, May 25, 2023 10:18 AM > To: Leo Li <leoyang.li@nxp.com>; Toshi Kani <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>; Borislav > Petkov <bp@alien8.de> > Cc: linux-acpi@vger.kernel.org; linux-kernel@vger.kernel.org > Subject: RE: [PATCH v2] apei/ghes: correctly return NULL for > ghes_get_devices() > > Hi Li, > > > -----Original Message----- > > From: Li Yang <leoyang.li@nxp.com> > > Sent: Saturday, May 20, 2023 4:13 AM > > To: Rafael J. Wysocki <rafael@kernel.org>; Len Brown > > <lenb@kernel.org>; James Morse <James.Morse@arm.com>; Tony Luck > > <tony.luck@intel.com>; Borislav Petkov <bp@alien8.de>; Justin He > > <Justin.He@arm.com> > > Cc: Li Yang <leoyang.li@nxp.com>; linux-acpi@vger.kernel.org; > > linux-kernel@vger.kernel.org > > Subject: [PATCH v2] apei/ghes: correctly return NULL for > > ghes_get_devices() > > > > Since 315bada690e0 ("EDAC: Check for GHES preference in the > > chipset-specific EDAC drivers"), vendor specific EDAC driver will not > > probe correctly when CONFIG_ACPI_APEI_GHES is enabled but no GHES > > device is present. Make > > ghes_get_devices() return NULL when the GHES device list is empty to > > fix the problem. > > > > Fixes: 9057a3f7ac36 ("EDAC/ghes: Prepare to make ghes_edac a proper > > module") > > Signed-off-by: Li Yang <leoyang.li@nxp.com> > > Cc: Jia He <justin.he@arm.com> > > --- > > > > V2: fix the fallthrough case in x86 path > > > > drivers/acpi/apei/ghes.c | 2 ++ > > 1 file changed, 2 insertions(+) > > > > diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c index > > 34ad071a64e9..4382fe13ee3e 100644 > > --- a/drivers/acpi/apei/ghes.c > > +++ b/drivers/acpi/apei/ghes.c > > @@ -1544,6 +1544,8 @@ struct list_head *ghes_get_devices(void) > > > > pr_warn_once("Force-loading ghes_edac on an > > unsupported platform. You're on your own!\n"); > > } > > + } else if (list_empty(&ghes_devs)) { > > + return NULL; > > } > I have no major objections to the fix. Just curious about the edac driver in > you platform. > IIUC, in your case, CONFIG_ACPI_APEI_GHES is enabled and edac_ghes > driver is either not loaded or fails to load. Is my understanding correct? Right. ghes_edac is loaded but since ghes_devs is empty due to this platform not using ACPI, it bails out with -ENODEV very quickly. I would expect the original platform EDAC driver should continue to work in this scenario. > I wonder whether ghes_get_devices() can unblock such check condition and > let other chipset-specific edac drivers continue with the initialization. @Toshi, > What do u think of it? > > > -- > 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.
On Fri, May 19, 2023 at 10:13 PM Li Yang <leoyang.li@nxp.com> wrote: > > Since 315bada690e0 ("EDAC: Check for GHES preference in the > chipset-specific EDAC drivers"), vendor specific EDAC driver will not > probe correctly when CONFIG_ACPI_APEI_GHES is enabled but no GHES device > is present. Make ghes_get_devices() return NULL when the GHES device > list is empty to fix the problem. > > Fixes: 9057a3f7ac36 ("EDAC/ghes: Prepare to make ghes_edac a proper module") > Signed-off-by: Li Yang <leoyang.li@nxp.com> > Cc: Jia He <justin.he@arm.com> Boris, Tony, any comments? > --- > > V2: fix the fallthrough case in x86 path > > drivers/acpi/apei/ghes.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c > index 34ad071a64e9..4382fe13ee3e 100644 > --- a/drivers/acpi/apei/ghes.c > +++ b/drivers/acpi/apei/ghes.c > @@ -1544,6 +1544,8 @@ struct list_head *ghes_get_devices(void) > > pr_warn_once("Force-loading ghes_edac on an unsupported platform. You're on your own!\n"); > } > + } else if (list_empty(&ghes_devs)) { > + return NULL; > } > > return &ghes_devs; > -- > 2.38.0 >
> > Since 315bada690e0 ("EDAC: Check for GHES preference in the > > chipset-specific EDAC drivers"), vendor specific EDAC driver will not > > probe correctly when CONFIG_ACPI_APEI_GHES is enabled but no GHES device > > is present. Make ghes_get_devices() return NULL when the GHES device > > list is empty to fix the problem. > > > > Fixes: 9057a3f7ac36 ("EDAC/ghes: Prepare to make ghes_edac a proper module") > > Signed-off-by: Li Yang <leoyang.li@nxp.com> > > Cc: Jia He <justin.he@arm.com> > > Boris, Tony, any comments? All of the callers are expecting NULL for a failure, not an empty list. So this looks OK to me. Reviewed-by: Tony Luck <tony.luck@intel.com> -Tony
diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c index 34ad071a64e9..4382fe13ee3e 100644 --- a/drivers/acpi/apei/ghes.c +++ b/drivers/acpi/apei/ghes.c @@ -1544,6 +1544,8 @@ struct list_head *ghes_get_devices(void) pr_warn_once("Force-loading ghes_edac on an unsupported platform. You're on your own!\n"); } + } else if (list_empty(&ghes_devs)) { + return NULL; } return &ghes_devs;
Since 315bada690e0 ("EDAC: Check for GHES preference in the chipset-specific EDAC drivers"), vendor specific EDAC driver will not probe correctly when CONFIG_ACPI_APEI_GHES is enabled but no GHES device is present. Make ghes_get_devices() return NULL when the GHES device list is empty to fix the problem. Fixes: 9057a3f7ac36 ("EDAC/ghes: Prepare to make ghes_edac a proper module") Signed-off-by: Li Yang <leoyang.li@nxp.com> Cc: Jia He <justin.he@arm.com> --- V2: fix the fallthrough case in x86 path drivers/acpi/apei/ghes.c | 2 ++ 1 file changed, 2 insertions(+)