| Message ID | 20241023070508.275-1-thunder.leizhen@huawei.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [1/1] RAS/AMD/FMPM: Fix return value check in setup_debugfs() | expand |
Hi Zhen Lei, Thanks for the patch. The subject line does not need to include 1/1 for a single patch. On Wed, Oct 23, 2024 at 03:05:08PM +0800, Zhen Lei wrote: > Fix the incorrect return value check for debugfs_create_dir() and > debugfs_create_file(), which returns ERR_PTR(-ERROR) instead of NULL Yes, the return value check is incorrect. But I think the solution is simpler. Please see the patch at the end. > when it fails. In addition, fmpm_dfs_dir should be set to NULL after > being reclaimed. > Why is this needed? > Fixes: 7d19eea51757 ("RAS/AMD/FMPM: Add debugfs interface to print record entries") > Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com> > --- > drivers/ras/amd/fmpm.c | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/drivers/ras/amd/fmpm.c b/drivers/ras/amd/fmpm.c > index 90de737fbc90978..b1cffbde6d319ed 100644 > --- a/drivers/ras/amd/fmpm.c > +++ b/drivers/ras/amd/fmpm.c > @@ -956,12 +956,14 @@ static void setup_debugfs(void) > return; > > fmpm_dfs_dir = debugfs_create_dir("fmpm", dfs); > - if (!fmpm_dfs_dir) > + if (IS_ERR(fmpm_dfs_dir)) > return; > > fmpm_dfs_entries = debugfs_create_file("entries", 0400, fmpm_dfs_dir, NULL, &fmpm_fops); > - if (!fmpm_dfs_entries) > + if (IS_ERR(fmpm_dfs_entries)) { > debugfs_remove(fmpm_dfs_dir); > + fmpm_dfs_dir = NULL; > + } > } > I think the intention here is correct. But the solution is to just remove the error checks entirely. Thanks, Yazen From 75869583bec8eb95d062cecedc4278bead8a293a Mon Sep 17 00:00:00 2001 From: Yazen Ghannam <yazen.ghannam@amd.com> Date: Thu, 24 Oct 2024 10:06:54 -0400 Subject: [PATCH] RAS/AMD/ATL: Simplify debugfs init Drop return value checks as debugfs API says most drivers should ignore errors. Issues between creation steps are automatically handled by the debugfs code. Also, add a code comment highlighting why a valid ras debugfs dentry is needed. Signed-off-by: Yazen Ghannam <yazen.ghannam@amd.com> --- drivers/ras/amd/fmpm.c | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/drivers/ras/amd/fmpm.c b/drivers/ras/amd/fmpm.c index 90de737fbc90..3965963d9164 100644 --- a/drivers/ras/amd/fmpm.c +++ b/drivers/ras/amd/fmpm.c @@ -952,16 +952,15 @@ static void setup_debugfs(void) { struct dentry *dfs = ras_get_debugfs_root(); + /* + * Ensure there's a ras debugfs directory so the fmpm files aren't populated in + * the root debugfs directory. + */ if (!dfs) return; fmpm_dfs_dir = debugfs_create_dir("fmpm", dfs); - if (!fmpm_dfs_dir) - return; - fmpm_dfs_entries = debugfs_create_file("entries", 0400, fmpm_dfs_dir, NULL, &fmpm_fops); - if (!fmpm_dfs_entries) - debugfs_remove(fmpm_dfs_dir); } static const struct x86_cpu_id fmpm_cpuids[] = {
On 2024/10/24 23:55, Yazen Ghannam wrote: > Hi Zhen Lei, > > Thanks for the patch. > > The subject line does not need to include 1/1 for a single patch. OK > > On Wed, Oct 23, 2024 at 03:05:08PM +0800, Zhen Lei wrote: >> Fix the incorrect return value check for debugfs_create_dir() and >> debugfs_create_file(), which returns ERR_PTR(-ERROR) instead of NULL > > Yes, the return value check is incorrect. But I think the solution is > simpler. Please see the patch at the end. According to the description of debugfs_create_dir(), the modification scheme you provide is more appropriate. By the way, fmpm_dfs_entries is unused now, should be removed. > >> when it fails. In addition, fmpm_dfs_dir should be set to NULL after >> being reclaimed. >> > > Why is this needed? Habitual, sorry, I did not analyze the code carefully before, this operation is not needed here. > >> Fixes: 7d19eea51757 ("RAS/AMD/FMPM: Add debugfs interface to print record entries") >> Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com> >> --- >> drivers/ras/amd/fmpm.c | 6 ++++-- >> 1 file changed, 4 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/ras/amd/fmpm.c b/drivers/ras/amd/fmpm.c >> index 90de737fbc90978..b1cffbde6d319ed 100644 >> --- a/drivers/ras/amd/fmpm.c >> +++ b/drivers/ras/amd/fmpm.c >> @@ -956,12 +956,14 @@ static void setup_debugfs(void) >> return; >> >> fmpm_dfs_dir = debugfs_create_dir("fmpm", dfs); >> - if (!fmpm_dfs_dir) >> + if (IS_ERR(fmpm_dfs_dir)) >> return; >> >> fmpm_dfs_entries = debugfs_create_file("entries", 0400, fmpm_dfs_dir, NULL, &fmpm_fops); >> - if (!fmpm_dfs_entries) >> + if (IS_ERR(fmpm_dfs_entries)) { >> debugfs_remove(fmpm_dfs_dir); >> + fmpm_dfs_dir = NULL; >> + } >> } >> > > I think the intention here is correct. But the solution is to just > remove the error checks entirely. > > Thanks, > Yazen > > >>From 75869583bec8eb95d062cecedc4278bead8a293a Mon Sep 17 00:00:00 2001 > From: Yazen Ghannam <yazen.ghannam@amd.com> > Date: Thu, 24 Oct 2024 10:06:54 -0400 > Subject: [PATCH] RAS/AMD/ATL: Simplify debugfs init > > Drop return value checks as debugfs API says most drivers should ignore > errors. Issues between creation steps are automatically handled by the > debugfs code. > > Also, add a code comment highlighting why a valid ras debugfs dentry is > needed. > > Signed-off-by: Yazen Ghannam <yazen.ghannam@amd.com> > --- > drivers/ras/amd/fmpm.c | 9 ++++----- > 1 file changed, 4 insertions(+), 5 deletions(-) > > diff --git a/drivers/ras/amd/fmpm.c b/drivers/ras/amd/fmpm.c > index 90de737fbc90..3965963d9164 100644 > --- a/drivers/ras/amd/fmpm.c > +++ b/drivers/ras/amd/fmpm.c > @@ -952,16 +952,15 @@ static void setup_debugfs(void) > { > struct dentry *dfs = ras_get_debugfs_root(); > > + /* > + * Ensure there's a ras debugfs directory so the fmpm files aren't populated in > + * the root debugfs directory. > + */ > if (!dfs) > return; > > fmpm_dfs_dir = debugfs_create_dir("fmpm", dfs); > - if (!fmpm_dfs_dir) > - return; > - > fmpm_dfs_entries = debugfs_create_file("entries", 0400, fmpm_dfs_dir, NULL, &fmpm_fops); > - if (!fmpm_dfs_entries) > - debugfs_remove(fmpm_dfs_dir); > } > > static const struct x86_cpu_id fmpm_cpuids[] = { >
diff --git a/drivers/ras/amd/fmpm.c b/drivers/ras/amd/fmpm.c index 90de737fbc90978..b1cffbde6d319ed 100644 --- a/drivers/ras/amd/fmpm.c +++ b/drivers/ras/amd/fmpm.c @@ -956,12 +956,14 @@ static void setup_debugfs(void) return; fmpm_dfs_dir = debugfs_create_dir("fmpm", dfs); - if (!fmpm_dfs_dir) + if (IS_ERR(fmpm_dfs_dir)) return; fmpm_dfs_entries = debugfs_create_file("entries", 0400, fmpm_dfs_dir, NULL, &fmpm_fops); - if (!fmpm_dfs_entries) + if (IS_ERR(fmpm_dfs_entries)) { debugfs_remove(fmpm_dfs_dir); + fmpm_dfs_dir = NULL; + } } static const struct x86_cpu_id fmpm_cpuids[] = {
Fix the incorrect return value check for debugfs_create_dir() and debugfs_create_file(), which returns ERR_PTR(-ERROR) instead of NULL when it fails. In addition, fmpm_dfs_dir should be set to NULL after being reclaimed. Fixes: 7d19eea51757 ("RAS/AMD/FMPM: Add debugfs interface to print record entries") Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com> --- drivers/ras/amd/fmpm.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)