[PATCH] Staging: dgnc: constify attribute_group structures

[PATCH] Staging: dgnc: constify attribute_group structures

[Bhumika Goyal]

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,
 };
-- 
1.9.1

Re: [Outreachy kernel] [PATCH] Staging: dgnc: constify attribute_group structures

[Julia Lawall]

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.
>

Re: [Outreachy kernel] [PATCH] Staging: dgnc: constify attribute_group structures

[Julia Lawall]

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.
>

Re: [PATCH] Staging: dgnc: constify attribute_group structures

[Greg KH]

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

Re: [PATCH] Staging: dgnc: constify attribute_group structures

[Bhumika Goyal]

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