2022-11-23 10:56:28

by zhang.songyi

[permalink] [raw]
Subject: [PATCH linux-next] powerpc/cell/axon_msi: replace DEFINE_SIMPLE_ATTRIBUTE with DEFINE_DEBUGFS_ATTRIBUTE

From: zhang songyi <[email protected]>

Fix the following coccicheck warning:
/arch/powerpc/platforms/cell/axon_msi.c:457:0-23: WARNING:
fops_msic should be defined with DEFINE_DEBUGFS_ATTRIBUTE

Signed-off-by: zhang songyi <[email protected]>
---
arch/powerpc/platforms/cell/axon_msi.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/arch/powerpc/platforms/cell/axon_msi.c b/arch/powerpc/platforms/cell/axon_msi.c
index 5b012abca773..ab080b5022ff 100644
--- a/arch/powerpc/platforms/cell/axon_msi.c
+++ b/arch/powerpc/platforms/cell/axon_msi.c
@@ -454,7 +454,7 @@ static int msic_get(void *data, u64 *val)
return 0;
}

-DEFINE_SIMPLE_ATTRIBUTE(fops_msic, msic_get, msic_set, "%llu\n");
+DEFINE_DEBUGFS_ATTRIBUTE(fops_msic, msic_get, msic_set, "%llu\n");

void axon_msi_debug_setup(struct device_node *dn, struct axon_msic *msic)
{
@@ -475,6 +475,7 @@ void axon_msi_debug_setup(struct device_node *dn, struct axon_msic *msic)

snprintf(name, sizeof(name), "msic_%d", of_node_to_nid(dn));

- debugfs_create_file(name, 0600, arch_debugfs_dir, msic, &fops_msic);
+ debugfs_create_file_unsafe(name, 0600, arch_debugfs_dir,
+ msic, &fops_msic);
}
#endif /* DEBUG */
--
2.15.2


2022-11-23 14:16:30

by Christophe Leroy

[permalink] [raw]
Subject: Re: [PATCH linux-next] powerpc/cell/axon_msi: replace DEFINE_SIMPLE_ATTRIBUTE with DEFINE_DEBUGFS_ATTRIBUTE

Hi,

Le 23/11/2022 à 10:06, [email protected] a écrit :
> From: zhang songyi <[email protected]>
>
> Fix the following coccicheck warning:
> /arch/powerpc/platforms/cell/axon_msi.c:457:0-23: WARNING:
> fops_msic should be defined with DEFINE_DEBUGFS_ATTRIBUTE

What's the difference between this new patch and the one that is already
awaiting application here :
https://patchwork.ozlabs.org/project/linuxppc-dev/patch/[email protected]/
?

The only difference I see it that the already existing patch has a more
complete description of the change, so unless I'm missing something it
would be nice to avoid sending the same changes again and again.

Thanks
Christophe



>
> Signed-off-by: zhang songyi <[email protected]>
> ---
> arch/powerpc/platforms/cell/axon_msi.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/arch/powerpc/platforms/cell/axon_msi.c b/arch/powerpc/platforms/cell/axon_msi.c
> index 5b012abca773..ab080b5022ff 100644
> --- a/arch/powerpc/platforms/cell/axon_msi.c
> +++ b/arch/powerpc/platforms/cell/axon_msi.c
> @@ -454,7 +454,7 @@ static int msic_get(void *data, u64 *val)
> return 0;
> }
>
> -DEFINE_SIMPLE_ATTRIBUTE(fops_msic, msic_get, msic_set, "%llu\n");
> +DEFINE_DEBUGFS_ATTRIBUTE(fops_msic, msic_get, msic_set, "%llu\n");
>
> void axon_msi_debug_setup(struct device_node *dn, struct axon_msic *msic)
> {
> @@ -475,6 +475,7 @@ void axon_msi_debug_setup(struct device_node *dn, struct axon_msic *msic)
>
> snprintf(name, sizeof(name), "msic_%d", of_node_to_nid(dn));
>
> - debugfs_create_file(name, 0600, arch_debugfs_dir, msic, &fops_msic);
> + debugfs_create_file_unsafe(name, 0600, arch_debugfs_dir,
> + msic, &fops_msic);
> }
> #endif /* DEBUG */
> --
> 2.15.2

2022-11-25 05:55:44

by Michael Ellerman

[permalink] [raw]
Subject: Re: [PATCH linux-next] powerpc/cell/axon_msi: replace DEFINE_SIMPLE_ATTRIBUTE with DEFINE_DEBUGFS_ATTRIBUTE

Christophe Leroy <[email protected]> writes:
> Hi,
>
> Le 23/11/2022 à 10:06, [email protected] a écrit :
>> From: zhang songyi <[email protected]>
>>
>> Fix the following coccicheck warning:
>> /arch/powerpc/platforms/cell/axon_msi.c:457:0-23: WARNING:
>> fops_msic should be defined with DEFINE_DEBUGFS_ATTRIBUTE
>
> What's the difference between this new patch and the one that is already
> awaiting application here :
> https://patchwork.ozlabs.org/project/linuxppc-dev/patch/[email protected]/
> ?
>
> The only difference I see it that the already existing patch has a more
> complete description of the change, so unless I'm missing something it
> would be nice to avoid sending the same changes again and again.

Both patches switch the code to use a function called "unsafe" without
adequately explaining why that is OK.

The commit that added the cocci check script says:

If the original struct file_operations are known to be safe against removal
races by themselves already, the proxy creation may be bypassed by creating
the files through debugfs_create_file_unsafe().

None of these conversion patches ever contain any explanation which
speaks to that.

In this case I *think* the change is OK and there is no race because the
debugfs file is never removed. But I really wish the submitter would
tell me that in the change log.

cheers