diff mbox series

[v2] apei/ghes: correctly return NULL for ghes_get_devices()

Message ID 20230519201249.31147-1-leoyang.li@nxp.com
State New
Headers
Series [v2] apei/ghes: correctly return NULL for ghes_get_devices() |

Commit Message

Li Yang May 19, 2023, 8:12 p.m. UTC 
  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(+)

Comments

Justin He May 25, 2023, 3:17 p.m. UTC | #1 
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.
Li Yang May 25, 2023, 8:33 p.m. UTC | #2 
> -----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.
Rafael J. Wysocki June 5, 2023, 5:11 p.m. UTC | #3 
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
>
Luck, Tony June 5, 2023, 5:26 p.m. UTC | #4 
> > 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
Rafael J. Wysocki June 12, 2023, 5:33 p.m. UTC | #5 
On Mon, Jun 5, 2023 at 7:26 PM Luck, Tony <tony.luck@intel.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?
>
> 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>

Applied as 6.5 material, thanks!
diff mbox series

Patch

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;