[PATCH] powerpc/setup_64: use DEFINE_DEBUGFS_ATTRIBUTE to define fops_rfi_flush

[PATCH] powerpc/setup_64: use DEFINE_DEBUGFS_ATTRIBUTE to define fops_rfi_flush

[Chen Zhou]

Use DEFINE_DEBUGFS_ATTRIBUTE rather than DEFINE_SIMPLE_ATTRIBUTE for
debugfs files.

Semantic patch information:
Rationale: DEFINE_SIMPLE_ATTRIBUTE + debugfs_create_file()
imposes some significant overhead as compared to
DEFINE_DEBUGFS_ATTRIBUTE + debugfs_create_file_unsafe().

Signed-off-by: Chen Zhou <chenzhou10@huawei.com>
---
 arch/powerpc/kernel/setup_64.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/powerpc/kernel/setup_64.c b/arch/powerpc/kernel/setup_64.c
index 6104917..4b9fbb2 100644
--- a/arch/powerpc/kernel/setup_64.c
+++ b/arch/powerpc/kernel/setup_64.c
@@ -956,11 +956,11 @@ static int rfi_flush_get(void *data, u64 *val)
 	return 0;
 }
 
-DEFINE_SIMPLE_ATTRIBUTE(fops_rfi_flush, rfi_flush_get, rfi_flush_set, "%llu\n");
+DEFINE_DEBUGFS_ATTRIBUTE(fops_rfi_flush, rfi_flush_get, rfi_flush_set, "%llu\n");
 
 static __init int rfi_flush_debugfs_init(void)
 {
-	debugfs_create_file("rfi_flush", 0600, powerpc_debugfs_root, NULL, &fops_rfi_flush);
+	debugfs_create_file_unsafe("rfi_flush", 0600, powerpc_debugfs_root, NULL, &fops_rfi_flush);
 	return 0;
 }
 device_initcall(rfi_flush_debugfs_init);
-- 
2.7.4

Re: [PATCH] powerpc/setup_64: use DEFINE_DEBUGFS_ATTRIBUTE to define fops_rfi_flush

[Michael Ellerman]

Chen Zhou <chenzhou10@huawei.com> writes:
> Use DEFINE_DEBUGFS_ATTRIBUTE rather than DEFINE_SIMPLE_ATTRIBUTE for
> debugfs files.
>
> Semantic patch information:
> Rationale: DEFINE_SIMPLE_ATTRIBUTE + debugfs_create_file()
> imposes some significant overhead as compared to
> DEFINE_DEBUGFS_ATTRIBUTE + debugfs_create_file_unsafe().

I know you didn't write this text, but these change logs are not great.
It doesn't really explain why you're doing it. And it is alarming that
you're converting *to* a function with "unsafe" in the name.

The commit that added the script:

  5103068eaca2 ("debugfs, coccinelle: check for obsolete DEFINE_SIMPLE_ATTRIBUTE() usage")

Has a bit more explanation.

Maybe something like this:

  In order to protect against file removal races, debugfs files created via
  debugfs_create_file() are wrapped by a struct file_operations at their
  opening.
  
  If the original struct file_operations is known to be safe against removal
  races already, the proxy creation may be bypassed by creating the files
  using DEFINE_DEBUGFS_ATTRIBUTE() and debugfs_create_file_unsafe().


The part that's not explained is why this file is "known to be safe
against removal races already"?

It also seems this conversion will make the file no longer seekable,
because DEFINE_SIMPLE_ATTRIBUTE() uses generic_file_llseek() whereas
DEFINE_DEBUGFS_ATTRIBUTE() uses no_llseek.

That is probably fine, but should be mentioned.

cheers

> Signed-off-by: Chen Zhou <chenzhou10@huawei.com>
> ---
>  arch/powerpc/kernel/setup_64.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/arch/powerpc/kernel/setup_64.c b/arch/powerpc/kernel/setup_64.c
> index 6104917..4b9fbb2 100644
> --- a/arch/powerpc/kernel/setup_64.c
> +++ b/arch/powerpc/kernel/setup_64.c
> @@ -956,11 +956,11 @@ static int rfi_flush_get(void *data, u64 *val)
>  	return 0;
>  }
>  
> -DEFINE_SIMPLE_ATTRIBUTE(fops_rfi_flush, rfi_flush_get, rfi_flush_set, "%llu\n");
> +DEFINE_DEBUGFS_ATTRIBUTE(fops_rfi_flush, rfi_flush_get, rfi_flush_set, "%llu\n");
>  
>  static __init int rfi_flush_debugfs_init(void)
>  {
> -	debugfs_create_file("rfi_flush", 0600, powerpc_debugfs_root, NULL, &fops_rfi_flush);
> +	debugfs_create_file_unsafe("rfi_flush", 0600, powerpc_debugfs_root, NULL, &fops_rfi_flush);
>  	return 0;
>  }
>  device_initcall(rfi_flush_debugfs_init);
> -- 
> 2.7.4

Re: [PATCH] powerpc/setup_64: use DEFINE_DEBUGFS_ATTRIBUTE to define fops_rfi_flush

[Chen Zhou]

Hi Michael,

On 2019/12/18 19:02, Michael Ellerman wrote:
> Chen Zhou <chenzhou10@huawei.com> writes:
>> Use DEFINE_DEBUGFS_ATTRIBUTE rather than DEFINE_SIMPLE_ATTRIBUTE for
>> debugfs files.
>>
>> Semantic patch information:
>> Rationale: DEFINE_SIMPLE_ATTRIBUTE + debugfs_create_file()
>> imposes some significant overhead as compared to
>> DEFINE_DEBUGFS_ATTRIBUTE + debugfs_create_file_unsafe().
> 
> I know you didn't write this text, but these change logs are not great.
> It doesn't really explain why you're doing it. And it is alarming that
> you're converting *to* a function with "unsafe" in the name.
> 
> The commit that added the script:
> 
>   5103068eaca2 ("debugfs, coccinelle: check for obsolete DEFINE_SIMPLE_ATTRIBUTE() usage")
> 
> Has a bit more explanation.
> 
> Maybe something like this:
> 
>   In order to protect against file removal races, debugfs files created via
>   debugfs_create_file() are wrapped by a struct file_operations at their
>   opening.
>   
>   If the original struct file_operations is known to be safe against removal
>   races already, the proxy creation may be bypassed by creating the files
>   using DEFINE_DEBUGFS_ATTRIBUTE() and debugfs_create_file_unsafe().
> 
> 
> The part that's not explained is why this file is "known to be safe
> against removal races already"?
> 
> It also seems this conversion will make the file no longer seekable,
> because DEFINE_SIMPLE_ATTRIBUTE() uses generic_file_llseek() whereas
> DEFINE_DEBUGFS_ATTRIBUTE() uses no_llseek.
> 
> That is probably fine, but should be mentioned.

Thanks for your explanation. This part indeed should be mentioned.

Chen Zhou

> 
> cheers
> 
>> Signed-off-by: Chen Zhou <chenzhou10@huawei.com>
>> ---
>>  arch/powerpc/kernel/setup_64.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/powerpc/kernel/setup_64.c b/arch/powerpc/kernel/setup_64.c
>> index 6104917..4b9fbb2 100644
>> --- a/arch/powerpc/kernel/setup_64.c
>> +++ b/arch/powerpc/kernel/setup_64.c
>> @@ -956,11 +956,11 @@ static int rfi_flush_get(void *data, u64 *val)
>>  	return 0;
>>  }
>>  
>> -DEFINE_SIMPLE_ATTRIBUTE(fops_rfi_flush, rfi_flush_get, rfi_flush_set, "%llu\n");
>> +DEFINE_DEBUGFS_ATTRIBUTE(fops_rfi_flush, rfi_flush_get, rfi_flush_set, "%llu\n");
>>  
>>  static __init int rfi_flush_debugfs_init(void)
>>  {
>> -	debugfs_create_file("rfi_flush", 0600, powerpc_debugfs_root, NULL, &fops_rfi_flush);
>> +	debugfs_create_file_unsafe("rfi_flush", 0600, powerpc_debugfs_root, NULL, &fops_rfi_flush);
>>  	return 0;
>>  }
>>  device_initcall(rfi_flush_debugfs_init);
>> -- 
>> 2.7.4
> 
> .
>