| Message ID | 20240325183755.776-1-bp@alien8.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | RAS/AMD/FMPM: Fix build when debugfs is not enabled | expand |
On 3/25/24 14:37, Borislav Petkov wrote: > From: "Borislav Petkov (AMD)" <bp@alien8.de> > > Have the driver depend on DEBUG_FS as it is useless without it. This isn't true which is why the module doesn't fail to load if debugfs is not available. > > Fixes: 6f15e617cc99 ("RAS: Introduce a FRU memory poison manager") > Closes: https://bugzilla.kernel.org/show_bug.cgi?id=218640 > Reported-by: anthony s.k. <akira.2020@protonmail.com> > Signed-off-by: Borislav Petkov (AMD) <bp@alien8.de> > Cc: Yazen Ghannam <yazen.ghannam@amd.com> > --- > drivers/ras/Kconfig | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/ras/Kconfig b/drivers/ras/Kconfig > index fc4f4bb94a4c..41697e326fa6 100644 > --- a/drivers/ras/Kconfig > +++ b/drivers/ras/Kconfig > @@ -37,7 +37,7 @@ source "drivers/ras/amd/atl/Kconfig" > config RAS_FMPM > tristate "FRU Memory Poison Manager" > default m > - depends on AMD_ATL && ACPI_APEI > + depends on AMD_ATL && ACPI_APEI && DEBUG_FS This was my first thought too. However, besides not true as stated above, this also leaves the issue open for others to hit. I think the fix below (not tested) would be more appropriate. What do you think? Thanks, Yazen diff --git a/drivers/ras/debugfs.h b/drivers/ras/debugfs.h index 4749ccdeeba1..ab95831e7710 100644 --- a/drivers/ras/debugfs.h +++ b/drivers/ras/debugfs.h @@ -4,6 +4,10 @@ #include <linux/debugfs.h> +#if IS_ENABLED(DEBUG_FS) struct dentry *ras_get_debugfs_root(void); +#else +static inline struct dentry *ras_get_debugfs_root(void) { return NULL; } +#endif /* DEBUG_FS */ #endif /* __RAS_DEBUGFS_H__ */
On Tue, Mar 26, 2024 at 09:41:41AM -0400, Yazen Ghannam wrote: > This isn't true which is why the module doesn't fail to load if debugfs > is not available. How useful is this thing really without debugfs to dump records? > This was my first thought too. However, besides not true as stated > above, this also leaves the issue open for others to hit. The others to hit? I don't get that part. > I think the fix below (not tested) would be more appropriate. > > What do you think? Sure, remove my fix and send me a tested version of yours and I'll swap them if the driver is useful without debugfs... Thx.
On Tuesday, March 26th, 2024 at 13:41, Yazen Ghannam <yazen.ghannam@amd.com> wrote: > > > > On 3/25/24 14:37, Borislav Petkov wrote: > > > From: "Borislav Petkov (AMD)" bp@alien8.de > > > > Have the driver depend on DEBUG_FS as it is useless without it. > > > This isn't true which is why the module doesn't fail to load if debugfs > is not available. > > > Fixes: 6f15e617cc99 ("RAS: Introduce a FRU memory poison manager") > > Closes: https://bugzilla.kernel.org/show_bug.cgi?id=218640 > > Reported-by: anthony s.k. akira.2020@protonmail.com > > Signed-off-by: Borislav Petkov (AMD) bp@alien8.de > > Cc: Yazen Ghannam yazen.ghannam@amd.com > > --- > > drivers/ras/Kconfig | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/ras/Kconfig b/drivers/ras/Kconfig > > index fc4f4bb94a4c..41697e326fa6 100644 > > --- a/drivers/ras/Kconfig > > +++ b/drivers/ras/Kconfig > > @@ -37,7 +37,7 @@ source "drivers/ras/amd/atl/Kconfig" > > config RAS_FMPM > > tristate "FRU Memory Poison Manager" > > default m > > - depends on AMD_ATL && ACPI_APEI > > + depends on AMD_ATL && ACPI_APEI && DEBUG_FS > > > This was my first thought too. However, besides not true as stated > above, this also leaves the issue open for others to hit. > > I think the fix below (not tested) would be more appropriate. > > What do you think? > > Thanks, > Yazen > > diff --git a/drivers/ras/debugfs.h b/drivers/ras/debugfs.h > index 4749ccdeeba1..ab95831e7710 100644 > --- a/drivers/ras/debugfs.h > +++ b/drivers/ras/debugfs.h > @@ -4,6 +4,10 @@ > > #include <linux/debugfs.h> > > > +#if IS_ENABLED(DEBUG_FS) > struct dentry *ras_get_debugfs_root(void); > +#else > +static inline struct dentry ras_get_debugfs_root(void) { return NULL; } > +#endif / DEBUG_FS / > > #endif / RAS_DEBUGFS_H */ this also works, just tested this time round, `make oldconfig` asks if i want to build RAS_FMPM, i built it as module, compilation succeeds. thanks kind regards, anthony s.k.
On 3/26/24 10:07, Borislav Petkov wrote: > On Tue, Mar 26, 2024 at 09:41:41AM -0400, Yazen Ghannam wrote: >> This isn't true which is why the module doesn't fail to load if debugfs >> is not available. > > How useful is this thing really without debugfs to dump records? > The goal of the module is to save and restore records across reboots. The debugfs thing came later and the commit message states that it is optional. 7d19eea51757 ("RAS/AMD/FMPM: Add debugfs interface to print record entries") >> This was my first thought too. However, besides not true as stated >> above, this also leaves the issue open for others to hit. > > The others to hit? I don't get that part. > Sorry, I mean that if there's another user of ras_get_debugfs_root() that doesn't depend on CONFIG_DEBUG_FS. >> I think the fix below (not tested) would be more appropriate. >> >> What do you think? > > Sure, remove my fix and send me a tested version of yours and I'll swap > them if the driver is useful without debugfs... Okay, will do. Thanks, Yazen
On Tue, Mar 26, 2024 at 10:22:05AM -0400, Yazen Ghannam wrote: > The goal of the module is to save and restore records across reboots. I guess... although when you think about it, you'd want to dump records and see what got restored. But ok, strictly speaking, I guess debugfs is optional... > Sorry, I mean that if there's another user of ras_get_debugfs_root() > that doesn't depend on CONFIG_DEBUG_FS. That is true. > Okay, will do. Thx.
On 3/26/24 10:20, A wrote: [...] >> >> diff --git a/drivers/ras/debugfs.h b/drivers/ras/debugfs.h >> index 4749ccdeeba1..ab95831e7710 100644 >> --- a/drivers/ras/debugfs.h >> +++ b/drivers/ras/debugfs.h >> @@ -4,6 +4,10 @@ >> >> #include <linux/debugfs.h> >> >> >> +#if IS_ENABLED(DEBUG_FS) >> struct dentry *ras_get_debugfs_root(void); >> +#else >> +static inline struct dentry ras_get_debugfs_root(void) { return NULL; } >> +#endif / DEBUG_FS / >> >> #endif / RAS_DEBUGFS_H */ > > this also works, just tested > > this time round, `make oldconfig` asks if i want to build RAS_FMPM, i built it as module, compilation succeeds. > Thanks for testing! Would you mind including your "Tested-by" tag? I can include this when I send a proper patch. Thanks, Yazen
On Tuesday, March 26th, 2024 at 14:29, Yazen Ghannam <yazen.ghannam@amd.com> wrote: > > > On 3/26/24 10:20, A wrote: > [...] > > > > diff --git a/drivers/ras/debugfs.h b/drivers/ras/debugfs.h > > > index 4749ccdeeba1..ab95831e7710 100644 > > > --- a/drivers/ras/debugfs.h > > > +++ b/drivers/ras/debugfs.h > > > @@ -4,6 +4,10 @@ > > > > > > #include <linux/debugfs.h> > > > > > > +#if IS_ENABLED(DEBUG_FS) > > > struct dentry *ras_get_debugfs_root(void); > > > +#else > > > +static inline struct dentry ras_get_debugfs_root(void) { return NULL; } > > > +#endif / DEBUG_FS / > > > > > > #endif / RAS_DEBUGFS_H */ > > > > this also works, just tested > > > > this time round, `make oldconfig` asks if i want to build RAS_FMPM, i built it as module, compilation succeeds. > > > Thanks for testing! > > Would you mind including your "Tested-by" tag? I can include this when I > send a proper patch. > > Thanks, > Yazen sure, tested-by: anthony s. knowles <akira.2020@protonmail.com> kind regards, a.s.k
diff --git a/drivers/ras/Kconfig b/drivers/ras/Kconfig index fc4f4bb94a4c..41697e326fa6 100644 --- a/drivers/ras/Kconfig +++ b/drivers/ras/Kconfig @@ -37,7 +37,7 @@ source "drivers/ras/amd/atl/Kconfig" config RAS_FMPM tristate "FRU Memory Poison Manager" default m - depends on AMD_ATL && ACPI_APEI + depends on AMD_ATL && ACPI_APEI && DEBUG_FS help Support saving and restoring memory error information across reboot using ACPI ERST as persistent storage. Error information is saved with