* Re: [PATCH] wil6210: fix debugfs_simple_attr.cocci warnings
2018-10-05 11:07 [PATCH] wil6210: fix debugfs_simple_attr.cocci warnings YueHaibing
@ 2018-10-05 11:04 ` Kalle Valo
2018-10-05 11:51 ` YueHaibing
2018-10-05 14:29 ` Julia Lawall
2018-10-13 17:29 ` Kalle Valo
1 sibling, 2 replies; 8+ messages in thread
From: Kalle Valo @ 2018-10-05 11:04 UTC (permalink / raw)
To: YueHaibing; +Cc: Maya Erez, linux-wireless, wil6210, kernel-janitors, netdev
YueHaibing <yuehaibing@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().
>
> Generated by: scripts/coccinelle/api/debugfs/debugfs_simple_attr.cocci
Just out of curiosity, what kind of overhead are we talking about here?
--
Kalle Valo
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH] wil6210: fix debugfs_simple_attr.cocci warnings
@ 2018-10-05 11:07 YueHaibing
2018-10-05 11:04 ` Kalle Valo
2018-10-13 17:29 ` Kalle Valo
0 siblings, 2 replies; 8+ messages in thread
From: YueHaibing @ 2018-10-05 11:07 UTC (permalink / raw)
To: Maya Erez, Kalle Valo
Cc: YueHaibing, linux-wireless, wil6210, kernel-janitors, netdev
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().
Generated by: scripts/coccinelle/api/debugfs/debugfs_simple_attr.cocci
Signed-off-by: YueHaibing <yuehaibing@huawei.com>
---
drivers/net/wireless/ath/wil6210/debugfs.c | 14 ++++++++------
1 file changed, 8 insertions(+), 6 deletions(-)
diff --git a/drivers/net/wireless/ath/wil6210/debugfs.c b/drivers/net/wireless/ath/wil6210/debugfs.c
index 66ffae2..aa50813 100644
--- a/drivers/net/wireless/ath/wil6210/debugfs.c
+++ b/drivers/net/wireless/ath/wil6210/debugfs.c
@@ -416,8 +416,8 @@ static int wil_debugfs_iomem_x32_get(void *data, u64 *val)
return 0;
}
-DEFINE_SIMPLE_ATTRIBUTE(fops_iomem_x32, wil_debugfs_iomem_x32_get,
- wil_debugfs_iomem_x32_set, "0x%08llx\n");
+DEFINE_DEBUGFS_ATTRIBUTE(fops_iomem_x32, wil_debugfs_iomem_x32_get,
+ wil_debugfs_iomem_x32_set, "0x%08llx\n");
static struct dentry *wil_debugfs_create_iomem_x32(const char *name,
umode_t mode,
@@ -432,7 +432,8 @@ static struct dentry *wil_debugfs_create_iomem_x32(const char *name,
data->wil = wil;
data->offset = value;
- file = debugfs_create_file(name, mode, parent, data, &fops_iomem_x32);
+ file = debugfs_create_file_unsafe(name, mode, parent, data,
+ &fops_iomem_x32);
if (!IS_ERR_OR_NULL(file))
wil->dbg_data.iomem_data_count++;
@@ -451,14 +452,15 @@ static int wil_debugfs_ulong_get(void *data, u64 *val)
return 0;
}
-DEFINE_SIMPLE_ATTRIBUTE(wil_fops_ulong, wil_debugfs_ulong_get,
- wil_debugfs_ulong_set, "0x%llx\n");
+DEFINE_DEBUGFS_ATTRIBUTE(wil_fops_ulong, wil_debugfs_ulong_get,
+ wil_debugfs_ulong_set, "0x%llx\n");
static struct dentry *wil_debugfs_create_ulong(const char *name, umode_t mode,
struct dentry *parent,
ulong *value)
{
- return debugfs_create_file(name, mode, parent, value, &wil_fops_ulong);
+ return debugfs_create_file_unsafe(name, mode, parent, value,
+ &wil_fops_ulong);
}
/**
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] wil6210: fix debugfs_simple_attr.cocci warnings
2018-10-05 11:04 ` Kalle Valo
@ 2018-10-05 11:51 ` YueHaibing
2018-10-05 14:29 ` Julia Lawall
1 sibling, 0 replies; 8+ messages in thread
From: YueHaibing @ 2018-10-05 11:51 UTC (permalink / raw)
To: Kalle Valo; +Cc: Maya Erez, linux-wireless, wil6210, kernel-janitors, netdev
On 2018/10/5 19:04, Kalle Valo wrote:
> YueHaibing <yuehaibing@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().
>>
>> Generated by: scripts/coccinelle/api/debugfs/debugfs_simple_attr.cocci
>
> Just out of curiosity, what kind of overhead are we talking about here?
commit 5103068eaca2 ("debugfs, coccinelle: check for obsolete DEFINE_SIMPLE_ATTRIBUTE() usage")
said this:
In order to protect against file removal races, debugfs files created via
debugfs_create_file() now get wrapped by a struct file_operations at their
opening.
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().
In order to help debugfs users who use the common
DEFINE_SIMPLE_ATTRIBUTE() + debugfs_create_file()
idiom to transition to removal safe struct file_operations, the helper
macro DEFINE_DEBUGFS_ATTRIBUTE() has been introduced.
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] wil6210: fix debugfs_simple_attr.cocci warnings
2018-10-05 11:04 ` Kalle Valo
2018-10-05 11:51 ` YueHaibing
@ 2018-10-05 14:29 ` Julia Lawall
2018-10-06 12:11 ` Kalle Valo
1 sibling, 1 reply; 8+ messages in thread
From: Julia Lawall @ 2018-10-05 14:29 UTC (permalink / raw)
To: Kalle Valo
Cc: YueHaibing, Maya Erez, linux-wireless, wil6210, kernel-janitors, netdev
On Fri, 5 Oct 2018, Kalle Valo wrote:
> YueHaibing <yuehaibing@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().
> >
> > Generated by: scripts/coccinelle/api/debugfs/debugfs_simple_attr.cocci
>
> Just out of curiosity, what kind of overhead are we talking about here?
The log message on the commit introducing the semantic patch says the
following:
In order to protect against file removal races, debugfs files created via
debugfs_create_file() now get wrapped by a struct file_operations at their
opening.
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().
In order to help debugfs users who use the common
DEFINE_SIMPLE_ATTRIBUTE() + debugfs_create_file()
idiom to transition to removal safe struct file_operations, the helper
macro DEFINE_DEBUGFS_ATTRIBUTE() has been introduced.
Thus, the preferred strategy is to use
DEFINE_DEBUGFS_ATTRIBUTE() + debugfs_create_file_unsafe()
now.
julia
>
> --
> Kalle Valo
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] wil6210: fix debugfs_simple_attr.cocci warnings
2018-10-05 14:29 ` Julia Lawall
@ 2018-10-06 12:11 ` Kalle Valo
2018-10-06 12:22 ` Julia Lawall
0 siblings, 1 reply; 8+ messages in thread
From: Kalle Valo @ 2018-10-06 12:11 UTC (permalink / raw)
To: Julia Lawall
Cc: YueHaibing, Maya Erez, linux-wireless, wil6210, kernel-janitors, netdev
Julia Lawall <julia.lawall@lip6.fr> writes:
> On Fri, 5 Oct 2018, Kalle Valo wrote:
>
>> YueHaibing <yuehaibing@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().
>> >
>> > Generated by: scripts/coccinelle/api/debugfs/debugfs_simple_attr.cocci
>>
>> Just out of curiosity, what kind of overhead are we talking about here?
>
> The log message on the commit introducing the semantic patch says the
> following:
>
> In order to protect against file removal races, debugfs files created via
> debugfs_create_file() now get wrapped by a struct file_operations at their
> opening.
>
> 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().
>
> In order to help debugfs users who use the common
> DEFINE_SIMPLE_ATTRIBUTE() + debugfs_create_file()
> idiom to transition to removal safe struct file_operations, the helper
> macro DEFINE_DEBUGFS_ATTRIBUTE() has been introduced.
>
> Thus, the preferred strategy is to use
> DEFINE_DEBUGFS_ATTRIBUTE() + debugfs_create_file_unsafe()
> now.
I admit that I didn't have time to investigate this is detail but I'm
still not understanding where is that "significant overhead" coming from
and how big of overhead are we talking about? I guess it has something
to do with full_proxy_open() vs open_proxy_open()?
Not that I'm against this patch, just curious when I see someone
claiming "significant overhead" which is not obvious for me.
--
Kalle Valo
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] wil6210: fix debugfs_simple_attr.cocci warnings
2018-10-06 12:11 ` Kalle Valo
@ 2018-10-06 12:22 ` Julia Lawall
2018-10-06 12:54 ` Kalle Valo
0 siblings, 1 reply; 8+ messages in thread
From: Julia Lawall @ 2018-10-06 12:22 UTC (permalink / raw)
To: Kalle Valo
Cc: Julia Lawall, YueHaibing, Maya Erez, linux-wireless, wil6210,
kernel-janitors, netdev
On Sat, 6 Oct 2018, Kalle Valo wrote:
> Julia Lawall <julia.lawall@lip6.fr> writes:
>
> > On Fri, 5 Oct 2018, Kalle Valo wrote:
> >
> >> YueHaibing <yuehaibing@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().
> >> >
> >> > Generated by: scripts/coccinelle/api/debugfs/debugfs_simple_attr.cocci
> >>
> >> Just out of curiosity, what kind of overhead are we talking about here?
> >
> > The log message on the commit introducing the semantic patch says the
> > following:
> >
> > In order to protect against file removal races, debugfs files created via
> > debugfs_create_file() now get wrapped by a struct file_operations at their
> > opening.
> >
> > 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().
> >
> > In order to help debugfs users who use the common
> > DEFINE_SIMPLE_ATTRIBUTE() + debugfs_create_file()
> > idiom to transition to removal safe struct file_operations, the helper
> > macro DEFINE_DEBUGFS_ATTRIBUTE() has been introduced.
> >
> > Thus, the preferred strategy is to use
> > DEFINE_DEBUGFS_ATTRIBUTE() + debugfs_create_file_unsafe()
> > now.
>
> I admit that I didn't have time to investigate this is detail but I'm
> still not understanding where is that "significant overhead" coming from
> and how big of overhead are we talking about? I guess it has something
> to do with full_proxy_open() vs open_proxy_open()?
>
> Not that I'm against this patch, just curious when I see someone
> claiming "significant overhead" which is not obvious for me.
The message with the semantic patch doesn't really talk about significant
overhead. Maybe YueHaibing can discuss with the person who proposed the
semantic patch what the actual issue is, and when the proposed change is
actually applicable.
julia
>
> --
> Kalle Valo
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] wil6210: fix debugfs_simple_attr.cocci warnings
2018-10-06 12:22 ` Julia Lawall
@ 2018-10-06 12:54 ` Kalle Valo
0 siblings, 0 replies; 8+ messages in thread
From: Kalle Valo @ 2018-10-06 12:54 UTC (permalink / raw)
To: Julia Lawall
Cc: YueHaibing, Maya Erez, linux-wireless, wil6210, kernel-janitors, netdev
Julia Lawall <julia.lawall@lip6.fr> writes:
> On Sat, 6 Oct 2018, Kalle Valo wrote:
>
>> Julia Lawall <julia.lawall@lip6.fr> writes:
>>
>> > On Fri, 5 Oct 2018, Kalle Valo wrote:
>> >
>> >> YueHaibing <yuehaibing@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().
>> >> >
>> >> > Generated by: scripts/coccinelle/api/debugfs/debugfs_simple_attr.cocci
>> >>
>> >> Just out of curiosity, what kind of overhead are we talking about here?
>> >
>> > The log message on the commit introducing the semantic patch says the
>> > following:
>> >
>> > In order to protect against file removal races, debugfs files created via
>> > debugfs_create_file() now get wrapped by a struct file_operations at their
>> > opening.
>> >
>> > 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().
>> >
>> > In order to help debugfs users who use the common
>> > DEFINE_SIMPLE_ATTRIBUTE() + debugfs_create_file()
>> > idiom to transition to removal safe struct file_operations, the helper
>> > macro DEFINE_DEBUGFS_ATTRIBUTE() has been introduced.
>> >
>> > Thus, the preferred strategy is to use
>> > DEFINE_DEBUGFS_ATTRIBUTE() + debugfs_create_file_unsafe()
>> > now.
>>
>> I admit that I didn't have time to investigate this is detail but I'm
>> still not understanding where is that "significant overhead" coming from
>> and how big of overhead are we talking about? I guess it has something
>> to do with full_proxy_open() vs open_proxy_open()?
>>
>> Not that I'm against this patch, just curious when I see someone
>> claiming "significant overhead" which is not obvious for me.
>
> The message with the semantic patch doesn't really talk about significant
> overhead. Maybe YueHaibing can discuss with the person who proposed the
> semantic patch what the actual issue is, and when the proposed change is
> actually applicable.
Actually commit 5103068eaca2 mentions "significant overhead":
--- /dev/null
+++ b/scripts/coccinelle/api/debugfs/debugfs_simple_attr.cocci
@@ -0,0 +1,67 @@
+/// Use DEFINE_DEBUGFS_ATTRIBUTE rather than DEFINE_SIMPLE_ATTRIBUTE
+/// for debugfs files.
+///
+//# Rationale: DEFINE_SIMPLE_ATTRIBUTE + debugfs_create_file()
+//# imposes some significant overhead as compared to
+//# DEFINE_DEBUGFS_ATTRIBUTE + debugfs_create_file_unsafe().
But I'll anyway apply this patch as I don't see anything wrong with it.
I was just trying to learn where this overhead is :)
--
Kalle Valo
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] wil6210: fix debugfs_simple_attr.cocci warnings
2018-10-05 11:07 [PATCH] wil6210: fix debugfs_simple_attr.cocci warnings YueHaibing
2018-10-05 11:04 ` Kalle Valo
@ 2018-10-13 17:29 ` Kalle Valo
1 sibling, 0 replies; 8+ messages in thread
From: Kalle Valo @ 2018-10-13 17:29 UTC (permalink / raw)
To: YueHaibing
Cc: Maya Erez, YueHaibing, linux-wireless, wil6210, kernel-janitors, netdev
YueHaibing <yuehaibing@huawei.com> wrote:
> 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().
>
> Generated by: scripts/coccinelle/api/debugfs/debugfs_simple_attr.cocci
>
> Signed-off-by: YueHaibing <yuehaibing@huawei.com>
> Signed-off-by: Kalle Valo <kvalo@codeaurora.org>
Patch applied to ath-next branch of ath.git, thanks.
f9dca154a4e4 wil6210: fix debugfs_simple_attr.cocci warnings
--
https://patchwork.kernel.org/patch/10627883/
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2018-10-13 17:29 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-10-05 11:07 [PATCH] wil6210: fix debugfs_simple_attr.cocci warnings YueHaibing
2018-10-05 11:04 ` Kalle Valo
2018-10-05 11:51 ` YueHaibing
2018-10-05 14:29 ` Julia Lawall
2018-10-06 12:11 ` Kalle Valo
2018-10-06 12:22 ` Julia Lawall
2018-10-06 12:54 ` Kalle Valo
2018-10-13 17:29 ` Kalle Valo
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).