* Re: [Outreachy kernel] [PATCH] Staging: dgnc: constify attribute_group structures
2016-09-29 14:48 [PATCH] Staging: dgnc: constify attribute_group structures Bhumika Goyal
@ 2016-09-29 18:16 ` Julia Lawall
2016-09-29 18:20 ` Julia Lawall
2016-10-02 15:22 ` Greg KH
2 siblings, 0 replies; 5+ messages in thread
From: Julia Lawall @ 2016-09-29 18:16 UTC (permalink / raw)
To: Bhumika Goyal; +Cc: outreachy-kernel, markh, gregkh, lidza.louina
On Thu, 29 Sep 2016, Bhumika Goyal wrote:
> Check for attribute_group structures that are only passed as a second
> argument to the functions sysfs_remove_group and sysfs_create_group. As
> these arguments are constant so, attribute_group structures having this
> property can also be made constant.
> Done using coccinelle:
>
> @r1 disable optional_qualifier @
> identifier i;
> position p;
> @@
> static struct attribute_group i@p = {...};
>
> @ok1@
> identifier r1.i;
> position p;
> expression e1;
> @@
> (
> sysfs_remove_group(e1,&i@p)
> |
> sysfs_create_group(e1,&i@p)
> )
>
> @bad@
> position p!={r1.p,ok1.p};
> identifier r1.i;
> @@
> i@p
>
> @depends on !bad disable optional_qualifier@
> identifier r1.i;
> @@
> static
> +const
> struct attribute_group i={...};
>
> @depends on !bad disable optional_qualifier@
> identifier r1.i;
> @@
> +const
> struct attribute_group i;
>
> File size before:
> text data bss dec hex filename
> 6248 1024 0 7272 1c68
> drivers/staging/dgnc/dgnc_sysfs.o
>
> File size after:
> text data bss dec hex filename
> 6288 960 0 7248 1c50
> drivers/staging/dgnc/dgnc_sysfs.o
>
> Signed-off-by: Bhumika Goyal <bhumirks@gmail.com>
Acked-by: Julia Lawall <julia.lawall@lip6.fr>
> ---
> drivers/staging/dgnc/dgnc_sysfs.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/staging/dgnc/dgnc_sysfs.c b/drivers/staging/dgnc/dgnc_sysfs.c
> index a83e0e4..290bf6e 100644
> --- a/drivers/staging/dgnc/dgnc_sysfs.c
> +++ b/drivers/staging/dgnc/dgnc_sysfs.c
> @@ -677,7 +677,7 @@ static struct attribute *dgnc_sysfs_tty_entries[] = {
> NULL
> };
>
> -static struct attribute_group dgnc_tty_attribute_group = {
> +static const struct attribute_group dgnc_tty_attribute_group = {
> .name = NULL,
> .attrs = dgnc_sysfs_tty_entries,
> };
> --
> 1.9.1
>
> --
> You received this message because you are subscribed to the Google Groups "outreachy-kernel" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to outreachy-kernel+unsubscribe@googlegroups.com.
> To post to this group, send email to outreachy-kernel@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/1475160534-23482-1-git-send-email-bhumirks%40gmail.com.
> For more options, visit https://groups.google.com/d/optout.
>
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [Outreachy kernel] [PATCH] Staging: dgnc: constify attribute_group structures
2016-09-29 14:48 [PATCH] Staging: dgnc: constify attribute_group structures Bhumika Goyal
2016-09-29 18:16 ` [Outreachy kernel] " Julia Lawall
@ 2016-09-29 18:20 ` Julia Lawall
2016-10-02 15:22 ` Greg KH
2 siblings, 0 replies; 5+ messages in thread
From: Julia Lawall @ 2016-09-29 18:20 UTC (permalink / raw)
To: Bhumika Goyal; +Cc: outreachy-kernel, markh, gregkh, lidza.louina
On Thu, 29 Sep 2016, Bhumika Goyal wrote:
> Check for attribute_group structures that are only passed as a second
> argument to the functions sysfs_remove_group and sysfs_create_group. As
> these arguments are constant so, attribute_group structures having this
> property can also be made constant.
The structure is used in the following context:
ret = sysfs_create_group(&c->kobj, &dgnc_tty_attribute_group);
if (ret) {
dev_err(c, "dgnc: failed to create sysfs tty device attributes.\n");
sysfs_remove_group(&c->kobj, &dgnc_tty_attribute_group);
return;
}
I'm not familiar with these functions, but it is unusual to have a cleanup
function for a resource in the error handling code of the same code that
creates the resource. You also sent a patch on most/hdm-dim2/dim2_sysfs.c
which doesn't do this. Perhaps both are correct, but it could be worth
looking into. One approach is to unfold the definitions of the function.
Another is to find other calls to sysfs_create_group and see what its
errror handling code contains. When you unfold the definitions of the
functions, you would want to see if sysfs_create_group can fail when it
has partially completed its work, but it does not do the cleanup itself.
This would again be unusual.
julia
> Done using coccinelle:
>
> @r1 disable optional_qualifier @
> identifier i;
> position p;
> @@
> static struct attribute_group i@p = {...};
>
> @ok1@
> identifier r1.i;
> position p;
> expression e1;
> @@
> (
> sysfs_remove_group(e1,&i@p)
> |
> sysfs_create_group(e1,&i@p)
> )
>
> @bad@
> position p!={r1.p,ok1.p};
> identifier r1.i;
> @@
> i@p
>
> @depends on !bad disable optional_qualifier@
> identifier r1.i;
> @@
> static
> +const
> struct attribute_group i={...};
>
> @depends on !bad disable optional_qualifier@
> identifier r1.i;
> @@
> +const
> struct attribute_group i;
>
> File size before:
> text data bss dec hex filename
> 6248 1024 0 7272 1c68
> drivers/staging/dgnc/dgnc_sysfs.o
>
> File size after:
> text data bss dec hex filename
> 6288 960 0 7248 1c50
> drivers/staging/dgnc/dgnc_sysfs.o
>
> Signed-off-by: Bhumika Goyal <bhumirks@gmail.com>
> ---
> drivers/staging/dgnc/dgnc_sysfs.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/staging/dgnc/dgnc_sysfs.c b/drivers/staging/dgnc/dgnc_sysfs.c
> index a83e0e4..290bf6e 100644
> --- a/drivers/staging/dgnc/dgnc_sysfs.c
> +++ b/drivers/staging/dgnc/dgnc_sysfs.c
> @@ -677,7 +677,7 @@ static struct attribute *dgnc_sysfs_tty_entries[] = {
> NULL
> };
>
> -static struct attribute_group dgnc_tty_attribute_group = {
> +static const struct attribute_group dgnc_tty_attribute_group = {
> .name = NULL,
> .attrs = dgnc_sysfs_tty_entries,
> };
> --
> 1.9.1
>
> --
> You received this message because you are subscribed to the Google Groups "outreachy-kernel" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to outreachy-kernel+unsubscribe@googlegroups.com.
> To post to this group, send email to outreachy-kernel@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/1475160534-23482-1-git-send-email-bhumirks%40gmail.com.
> For more options, visit https://groups.google.com/d/optout.
>
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] Staging: dgnc: constify attribute_group structures
2016-09-29 14:48 [PATCH] Staging: dgnc: constify attribute_group structures Bhumika Goyal
2016-09-29 18:16 ` [Outreachy kernel] " Julia Lawall
2016-09-29 18:20 ` Julia Lawall
@ 2016-10-02 15:22 ` Greg KH
2016-10-02 20:10 ` Bhumika Goyal
2 siblings, 1 reply; 5+ messages in thread
From: Greg KH @ 2016-10-02 15:22 UTC (permalink / raw)
To: Bhumika Goyal; +Cc: outreachy-kernel, markh, lidza.louina
On Thu, Sep 29, 2016 at 08:18:54PM +0530, Bhumika Goyal wrote:
> Check for attribute_group structures that are only passed as a second
> argument to the functions sysfs_remove_group and sysfs_create_group. As
> these arguments are constant so, attribute_group structures having this
> property can also be made constant.
> Done using coccinelle:
>
> @r1 disable optional_qualifier @
> identifier i;
> position p;
> @@
> static struct attribute_group i@p = {...};
>
> @ok1@
> identifier r1.i;
> position p;
> expression e1;
> @@
> (
> sysfs_remove_group(e1,&i@p)
> |
> sysfs_create_group(e1,&i@p)
> )
>
> @bad@
> position p!={r1.p,ok1.p};
> identifier r1.i;
> @@
> i@p
>
> @depends on !bad disable optional_qualifier@
> identifier r1.i;
> @@
> static
> +const
> struct attribute_group i={...};
>
> @depends on !bad disable optional_qualifier@
> identifier r1.i;
> @@
> +const
> struct attribute_group i;
>
> File size before:
> text data bss dec hex filename
> 6248 1024 0 7272 1c68
> drivers/staging/dgnc/dgnc_sysfs.o
>
> File size after:
> text data bss dec hex filename
> 6288 960 0 7248 1c50
> drivers/staging/dgnc/dgnc_sysfs.o
>
> Signed-off-by: Bhumika Goyal <bhumirks@gmail.com>
> ---
> drivers/staging/dgnc/dgnc_sysfs.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/staging/dgnc/dgnc_sysfs.c b/drivers/staging/dgnc/dgnc_sysfs.c
> index a83e0e4..290bf6e 100644
> --- a/drivers/staging/dgnc/dgnc_sysfs.c
> +++ b/drivers/staging/dgnc/dgnc_sysfs.c
> @@ -677,7 +677,7 @@ static struct attribute *dgnc_sysfs_tty_entries[] = {
> NULL
> };
>
> -static struct attribute_group dgnc_tty_attribute_group = {
> +static const struct attribute_group dgnc_tty_attribute_group = {
> .name = NULL,
> .attrs = dgnc_sysfs_tty_entries,
> };
I'll take this patch, but all of these sysfs files should be removed
from the driver, as there are standard ways of getting this information
from a tty device that is not using this interface.
So if you want to send a follow-on patch, removing all of them, I would
really appreciate it.
thanks,
greg k-h
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] Staging: dgnc: constify attribute_group structures
2016-10-02 15:22 ` Greg KH
@ 2016-10-02 20:10 ` Bhumika Goyal
0 siblings, 0 replies; 5+ messages in thread
From: Bhumika Goyal @ 2016-10-02 20:10 UTC (permalink / raw)
To: Greg KH; +Cc: outreachy-kernel, markh, Lidza Louina
On Sun, Oct 2, 2016 at 8:52 PM, Greg KH <gregkh@linuxfoundation.org> wrote:
> On Thu, Sep 29, 2016 at 08:18:54PM +0530, Bhumika Goyal wrote:
>> Check for attribute_group structures that are only passed as a second
>> argument to the functions sysfs_remove_group and sysfs_create_group. As
>> these arguments are constant so, attribute_group structures having this
>> property can also be made constant.
>> Done using coccinelle:
>>
>> @r1 disable optional_qualifier @
>> identifier i;
>> position p;
>> @@
>> static struct attribute_group i@p = {...};
>>
>> @ok1@
>> identifier r1.i;
>> position p;
>> expression e1;
>> @@
>> (
>> sysfs_remove_group(e1,&i@p)
>> |
>> sysfs_create_group(e1,&i@p)
>> )
>>
>> @bad@
>> position p!={r1.p,ok1.p};
>> identifier r1.i;
>> @@
>> i@p
>>
>> @depends on !bad disable optional_qualifier@
>> identifier r1.i;
>> @@
>> static
>> +const
>> struct attribute_group i={...};
>>
>> @depends on !bad disable optional_qualifier@
>> identifier r1.i;
>> @@
>> +const
>> struct attribute_group i;
>>
>> File size before:
>> text data bss dec hex filename
>> 6248 1024 0 7272 1c68
>> drivers/staging/dgnc/dgnc_sysfs.o
>>
>> File size after:
>> text data bss dec hex filename
>> 6288 960 0 7248 1c50
>> drivers/staging/dgnc/dgnc_sysfs.o
>>
>> Signed-off-by: Bhumika Goyal <bhumirks@gmail.com>
>> ---
>> drivers/staging/dgnc/dgnc_sysfs.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/staging/dgnc/dgnc_sysfs.c b/drivers/staging/dgnc/dgnc_sysfs.c
>> index a83e0e4..290bf6e 100644
>> --- a/drivers/staging/dgnc/dgnc_sysfs.c
>> +++ b/drivers/staging/dgnc/dgnc_sysfs.c
>> @@ -677,7 +677,7 @@ static struct attribute *dgnc_sysfs_tty_entries[] = {
>> NULL
>> };
>>
>> -static struct attribute_group dgnc_tty_attribute_group = {
>> +static const struct attribute_group dgnc_tty_attribute_group = {
>> .name = NULL,
>> .attrs = dgnc_sysfs_tty_entries,
>> };
>
> I'll take this patch, but all of these sysfs files should be removed
> from the driver, as there are standard ways of getting this information
> from a tty device that is not using this interface.
>
> So if you want to send a follow-on patch, removing all of them, I would
> really appreciate it.
>
Yes, I will surely work on this and send a follow-on patch addressing
the issue that you mentioned.
Thanks,
Bhumika
> thanks,
>
> greg k-h
^ permalink raw reply [flat|nested] 5+ messages in thread