[PATCH 1/1] RAS/AMD/FMPM: Fix return value check in setup_debugfs()

[PATCH 1/1] RAS/AMD/FMPM: Fix return value check in setup_debugfs()

[Zhen Lei]

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(-)

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[] = {
-- 
2.34.1

Re: [PATCH 1/1] RAS/AMD/FMPM: Fix return value check in setup_debugfs()

[Yazen Ghannam]

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[] = {
-- 
2.43.0

Re: [PATCH 1/1] RAS/AMD/FMPM: Fix return value check in setup_debugfs()

[Leizhen (ThunderTown)]

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[] = {
> 

-- 
Regards,
  Zhen Lei