Re: [PATCH 2/2] mmc: fix integer assignments to pointer

Re: [PATCH 2/2] mmc: fix integer assignments to pointer

[Chris Ball]

Hi,

[Adding linux-sparse@ to CC]

On Tue, Aug 23 2011, Venkatraman S wrote:
> Fix the sparse warning output
> "warning: Using plain integer as NULL pointer"
>
> Signed-off-by: Venkatraman S <svenkatr@ti.com>
> ---
>  drivers/mmc/card/block.c    |    4 ++--
>  drivers/mmc/core/core.c     |    2 +-
>  drivers/mmc/core/mmc_ops.c  |    4 ++--
>  drivers/mmc/core/sdio_ops.c |    2 +-
>  4 files changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/mmc/card/block.c b/drivers/mmc/card/block.c
> index 1ff5486..e702c61 100644
> --- a/drivers/mmc/card/block.c
> +++ b/drivers/mmc/card/block.c
> @@ -291,7 +291,7 @@ static int mmc_blk_ioctl_cmd(struct block_device *bdev,
>  	struct mmc_card *card;
>  	struct mmc_command cmd = {0};
>  	struct mmc_data data = {0};
> -	struct mmc_request mrq = {0};
> +	struct mmc_request mrq = {NULL};
>  	struct scatterlist sg;
>  	int err;
> [...]

The sparse warning is mistaken.  Or I'm mistaken.  But I suspect it's
the sparse warning.

The {0} syntax is covered by:

   [6.7.8.21] If there are fewer initializers in a brace-enclosed list
   than there are elements or members of an aggregate, or fewer
   characters in a string literal used to initialize an array of known
   size than there are elements in the array, the remainder of the
   aggregate shall be initialized implicitly the same as objects that
   have static storage duration.

So we're not assigning 0 to a pointer, or whatever sparse thinks we're
doing -- we're initializing every member of the struct with 0, which is
a good and safe way to initialize it.

Sparse folks, any comment?

Thanks,

- Chris.
-- 
Chris Ball   <cjb@laptop.org>   <http://printf.net/>
One Laptop Per Child

Re: [PATCH 2/2] mmc: fix integer assignments to pointer

[Sam Ravnborg]

On Tue, Aug 23, 2011 at 12:31:55PM -0400, Chris Ball wrote:
> Hi,
> 
> [Adding linux-sparse@ to CC]
> 
> On Tue, Aug 23 2011, Venkatraman S wrote:
> > Fix the sparse warning output
> > "warning: Using plain integer as NULL pointer"
> >
> > Signed-off-by: Venkatraman S <svenkatr@ti.com>
> > ---
> >  drivers/mmc/card/block.c    |    4 ++--
> >  drivers/mmc/core/core.c     |    2 +-
> >  drivers/mmc/core/mmc_ops.c  |    4 ++--
> >  drivers/mmc/core/sdio_ops.c |    2 +-
> >  4 files changed, 6 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/mmc/card/block.c b/drivers/mmc/card/block.c
> > index 1ff5486..e702c61 100644
> > --- a/drivers/mmc/card/block.c
> > +++ b/drivers/mmc/card/block.c
> > @@ -291,7 +291,7 @@ static int mmc_blk_ioctl_cmd(struct block_device *bdev,
> >  	struct mmc_card *card;
> >  	struct mmc_command cmd = {0};
> >  	struct mmc_data data = {0};
> > -	struct mmc_request mrq = {0};
> > +	struct mmc_request mrq = {NULL};
> >  	struct scatterlist sg;
> >  	int err;
> > [...]
> 
> The sparse warning is mistaken.  Or I'm mistaken.  But I suspect it's
> the sparse warning.
> 
> The {0} syntax is covered by:
> 
>    [6.7.8.21] If there are fewer initializers in a brace-enclosed list
>    than there are elements or members of an aggregate, or fewer
>    characters in a string literal used to initialize an array of known
>    size than there are elements in the array, the remainder of the
>    aggregate shall be initialized implicitly the same as objects that
>    have static storage duration.
> 
> So we're not assigning 0 to a pointer, or whatever sparse thinks we're
> doing -- we're initializing every member of the struct with 0, which is
> a good and safe way to initialize it.
> 
> Sparse folks, any comment?

The struct looks like this:
struct mmc_request {
        struct mmc_command      *sbc;           /* SET_BLOCK_COUNT for multiblock */
        struct mmc_command      *cmd;
        struct mmc_data         *data;
        struct mmc_command      *stop;

        struct completion       completion;
        void                    (*done)(struct mmc_request *);/* completion function */
};


So you assing '0' to sbc - which is a pointer.
So sparse warning is correct.

	Sam

Re: [PATCH 2/2] mmc: fix integer assignments to pointer

[Chris Ball]

Hi,

On Tue, Aug 23 2011, Sam Ravnborg wrote:
>> So we're not assigning 0 to a pointer, or whatever sparse thinks we're
>> doing -- we're initializing every member of the struct with 0, which is
>> a good and safe way to initialize it.
>> 
>> Sparse folks, any comment?
>
> The struct looks like this:
> struct mmc_request {
>         struct mmc_command      *sbc;           /* SET_BLOCK_COUNT for multiblock */
>         struct mmc_command      *cmd;
>         struct mmc_data         *data;
>         struct mmc_command      *stop;
>
>         struct completion       completion;
>         void (*done)(struct mmc_request *);/* completion function */
> };
>
>
> So you assing '0' to sbc - which is a pointer.
> So sparse warning is correct.

Oops, thanks Sam, I should have realized this was the complaint --
sorry for wasting your time.

- Chris.
-- 
Chris Ball   <cjb@laptop.org>   <http://printf.net/>
One Laptop Per Child

Re: [PATCH 2/2] mmc: fix integer assignments to pointer

[J Freyensee]

On 08/23/2011 09:56 AM, Sam Ravnborg wrote:
> On Tue, Aug 23, 2011 at 12:31:55PM -0400, Chris Ball wrote:
>> Hi,
>>
>> [Adding linux-sparse@ to CC]
>>
>> On Tue, Aug 23 2011, Venkatraman S wrote:
>>> Fix the sparse warning output
>>> "warning: Using plain integer as NULL pointer"
>>>
>>> Signed-off-by: Venkatraman S<svenkatr@ti.com>
>>> ---
>>>   drivers/mmc/card/block.c    |    4 ++--
>>>   drivers/mmc/core/core.c     |    2 +-
>>>   drivers/mmc/core/mmc_ops.c  |    4 ++--
>>>   drivers/mmc/core/sdio_ops.c |    2 +-
>>>   4 files changed, 6 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/mmc/card/block.c b/drivers/mmc/card/block.c
>>> index 1ff5486..e702c61 100644
>>> --- a/drivers/mmc/card/block.c
>>> +++ b/drivers/mmc/card/block.c
>>> @@ -291,7 +291,7 @@ static int mmc_blk_ioctl_cmd(struct block_device *bdev,
>>>   	struct mmc_card *card;
>>>   	struct mmc_command cmd = {0};
>>>   	struct mmc_data data = {0};
>>> -	struct mmc_request mrq = {0};
>>> +	struct mmc_request mrq = {NULL};
>>>   	struct scatterlist sg;
>>>   	int err;
>>> [...]
>>
>> The sparse warning is mistaken.  Or I'm mistaken.  But I suspect it's
>> the sparse warning.
>>

Okay, stupid question times:

 >>> -	struct mmc_request mrq = {0};

this is 'struct mmc_request mrq' and not 'struct mmc_request *mrq', right?

Then I think the sparse warning is correct.  From my experience, sparse 
is very clear: if any variable is defined as a pointer, you use NULL to 
reset the pointer instead of '0', and if any variable is a normal 
variable, you use '0' to reset the variable. I don't think sparse is 
smart enough to look at the underlying variables, which in this case, 
mrq contains members that are pointers to something.

in this case, the defined variable is not a pointer, rather a variable 
named mrq that is of type 'struct mmc_request'.  Therefore, 0 is correct 
to use and NULL is incorrect.

If you want it to be sparse-correct as well as human-readable correct, 
you probably should write the code that explicitly shows assigning each 
pointer member of mrq to NULL, ie:

struct mmc_request mrq;
mrq.sbc = NULL;
mrq.cmd = NULL;
mrq.data = NULL;
.
.
etc.



>> The {0} syntax is covered by:
>>
>>     [6.7.8.21] If there are fewer initializers in a brace-enclosed list
>>     than there are elements or members of an aggregate, or fewer
>>     characters in a string literal used to initialize an array of known
>>     size than there are elements in the array, the remainder of the
>>     aggregate shall be initialized implicitly the same as objects that
>>     have static storage duration.
>>
>> So we're not assigning 0 to a pointer, or whatever sparse thinks we're
>> doing -- we're initializing every member of the struct with 0, which is
>> a good and safe way to initialize it.
>>
>> Sparse folks, any comment?
>
> The struct looks like this:
> struct mmc_request {
>          struct mmc_command      *sbc;           /* SET_BLOCK_COUNT for multiblock */
>          struct mmc_command      *cmd;
>          struct mmc_data         *data;
>          struct mmc_command      *stop;
>
>          struct completion       completion;
>          void                    (*done)(struct mmc_request *);/* completion function */
> };
>
>
> So you assing '0' to sbc - which is a pointer.
> So sparse warning is correct.
>
> 	Sam
> --
> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html


-- 
J (James/Jay) Freyensee
Storage Technology Group
Intel Corporation

Re: [PATCH 2/2] mmc: fix integer assignments to pointer

[S, Venkatraman]

On Wed, Aug 24, 2011 at 4:10 AM, J Freyensee
<james_p_freyensee@linux.intel.com> wrote:
> On 08/23/2011 09:56 AM, Sam Ravnborg wrote:
>>
>> On Tue, Aug 23, 2011 at 12:31:55PM -0400, Chris Ball wrote:
>>>
>>> Hi,
>>>
>>> [Adding linux-sparse@ to CC]
>>>
>>> On Tue, Aug 23 2011, Venkatraman S wrote:
>>>>
>>>> Fix the sparse warning output
>>>> "warning: Using plain integer as NULL pointer"
>>>>
>>>> Signed-off-by: Venkatraman S<svenkatr@ti.com>
>>>> ---
>>>>  drivers/mmc/card/block.c    |    4 ++--
>>>>  drivers/mmc/core/core.c     |    2 +-
>>>>  drivers/mmc/core/mmc_ops.c  |    4 ++--
>>>>  drivers/mmc/core/sdio_ops.c |    2 +-
>>>>  4 files changed, 6 insertions(+), 6 deletions(-)
>>>>
>>>> diff --git a/drivers/mmc/card/block.c b/drivers/mmc/card/block.c
>>>> index 1ff5486..e702c61 100644
>>>> --- a/drivers/mmc/card/block.c
>>>> +++ b/drivers/mmc/card/block.c
>>>> @@ -291,7 +291,7 @@ static int mmc_blk_ioctl_cmd(struct block_device
>>>> *bdev,
>>>>        struct mmc_card *card;
>>>>        struct mmc_command cmd = {0};
>>>>        struct mmc_data data = {0};
>>>> -       struct mmc_request mrq = {0};
>>>> +       struct mmc_request mrq = {NULL};
>>>>        struct scatterlist sg;
>>>>        int err;
>>>> [...]
>>>
>>> The sparse warning is mistaken.  Or I'm mistaken.  But I suspect it's
>>> the sparse warning.
>>>
>
> Okay, stupid question times:
>
>>>> -   struct mmc_request mrq = {0};
>
> this is 'struct mmc_request mrq' and not 'struct mmc_request *mrq', right?
>
> Then I think the sparse warning is correct.  From my experience, sparse is
> very clear: if any variable is defined as a pointer, you use NULL to reset
> the pointer instead of '0', and if any variable is a normal variable, you
> use '0' to reset the variable. I don't think sparse is smart enough to look
> at the underlying variables, which in this case, mrq contains members that
> are pointers to something.
>
> in this case, the defined variable is not a pointer, rather a variable named
> mrq that is of type 'struct mmc_request'.  Therefore, 0 is correct to use
> and NULL is incorrect.

Please tolerate my misunderstanding here - but aren't the above two statements
contradictory ?
My understanding was the '0' or NULL would be assigned to the first member of
struct mmc_request, which happens to be a pointer. Hence NULL is more correct
than '0', right ?

The type of the structure variable is irrelevant here. i.e.
struct mmc_request mrq = {0};
and
struct mmc_request *mrq = {0};

would generate the same warning, isn't it ?

>
> If you want it to be sparse-correct as well as human-readable correct, you
> probably should write the code that explicitly shows assigning each pointer
> member of mrq to NULL, ie:
>
> struct mmc_request mrq;
> mrq.sbc = NULL;
> mrq.cmd = NULL;
> mrq.data = NULL;
> .
> .
> etc.
>
--
To unsubscribe from this list: send the line "unsubscribe linux-sparse" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] mmc: fix integer assignments to pointer

[Josh Triplett]

On Tue, Aug 23, 2011 at 12:31:55PM -0400, Chris Ball wrote:
> Hi,
> 
> [Adding linux-sparse@ to CC]
> 
> On Tue, Aug 23 2011, Venkatraman S wrote:
> > Fix the sparse warning output
> > "warning: Using plain integer as NULL pointer"
> >
> > Signed-off-by: Venkatraman S <svenkatr@ti.com>
> > ---
> >  drivers/mmc/card/block.c    |    4 ++--
> >  drivers/mmc/core/core.c     |    2 +-
> >  drivers/mmc/core/mmc_ops.c  |    4 ++--
> >  drivers/mmc/core/sdio_ops.c |    2 +-
> >  4 files changed, 6 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/mmc/card/block.c b/drivers/mmc/card/block.c
> > index 1ff5486..e702c61 100644
> > --- a/drivers/mmc/card/block.c
> > +++ b/drivers/mmc/card/block.c
> > @@ -291,7 +291,7 @@ static int mmc_blk_ioctl_cmd(struct block_device *bdev,
> >  	struct mmc_card *card;
> >  	struct mmc_command cmd = {0};
> >  	struct mmc_data data = {0};
> > -	struct mmc_request mrq = {0};
> > +	struct mmc_request mrq = {NULL};
> >  	struct scatterlist sg;
> >  	int err;
> > [...]
> 
> The sparse warning is mistaken.  Or I'm mistaken.  But I suspect it's
> the sparse warning.
> 
> The {0} syntax is covered by:
> 
>    [6.7.8.21] If there are fewer initializers in a brace-enclosed list
>    than there are elements or members of an aggregate, or fewer
>    characters in a string literal used to initialize an array of known
>    size than there are elements in the array, the remainder of the
>    aggregate shall be initialized implicitly the same as objects that
>    have static storage duration.
> 
> So we're not assigning 0 to a pointer, or whatever sparse thinks we're
> doing -- we're initializing every member of the struct with 0, which is
> a good and safe way to initialize it.
> 
> Sparse folks, any comment?

Notice that it says "the remainder of the aggregate".  The first field
still gets initialized with the 0 you supplied, and the first field of
struct mmc_request has a pointer type.

- Josh Triplett

Re: [PATCH 2/2] mmc: fix integer assignments to pointer

[Ben Pfaff]

Josh Triplett <josh@joshtriplett.org> writes:

> On Tue, Aug 23, 2011 at 12:31:55PM -0400, Chris Ball wrote:
>> On Tue, Aug 23 2011, Venkatraman S wrote:
>> > -	struct mmc_request mrq = {0};
>> > +	struct mmc_request mrq = {NULL};
>> 
>> The sparse warning is mistaken.  Or I'm mistaken.  But I suspect it's
>> the sparse warning.
>
> Notice that it says "the remainder of the aggregate".  The first field
> still gets initialized with the 0 you supplied, and the first field of
> struct mmc_request has a pointer type.

That's an understandable position, but I think it would also be
reasonable for sparse to special case using {0} as an
initializer.  {0} is a valid initializer for every type and so
it's sometimes used as an initializer for a local variable to get
the same effect that one would have for a static variable without
specifying an initializer.
-- 
Ben Pfaff 
http://benpfaff.org

Re: [PATCH 2/2] mmc: fix integer assignments to pointer

[Josh Triplett]

On Tue, Aug 23, 2011 at 11:04:08AM -0700, Ben Pfaff wrote:
> Josh Triplett <josh@joshtriplett.org> writes:
> 
> > On Tue, Aug 23, 2011 at 12:31:55PM -0400, Chris Ball wrote:
> >> On Tue, Aug 23 2011, Venkatraman S wrote:
> >> > -	struct mmc_request mrq = {0};
> >> > +	struct mmc_request mrq = {NULL};
> >> 
> >> The sparse warning is mistaken.  Or I'm mistaken.  But I suspect it's
> >> the sparse warning.
> >
> > Notice that it says "the remainder of the aggregate".  The first field
> > still gets initialized with the 0 you supplied, and the first field of
> > struct mmc_request has a pointer type.
> 
> That's an understandable position, but I think it would also be
> reasonable for sparse to special case using {0} as an
> initializer.  {0} is a valid initializer for every type and so
> it's sometimes used as an initializer for a local variable to get
> the same effect that one would have for a static variable without
> specifying an initializer.

{} produces the same effect, as far as I know.

- Josh Triplett

Re: [PATCH 2/2] mmc: fix integer assignments to pointer

[Chris Ball]

Hi,

On Tue, Aug 23 2011, Josh Triplett wrote:
> {} produces the same effect, as far as I know.

Yeah.  I prefer {0}, because {} is a gcc-ism (the ANSI grammar demands
initializer-lists be non-empty) and is less readable for people who
haven't seen the idiom before and are wondering what's going on.

I'm still a little confused -- the {0} or memset(0, struct ..);
formations are used often in the kernel, even with pointers involved.
Is the warning (Wnon_pointer_null) run against the kernel by default,
or did Venkatraman add it manually?  If default, is it catching bugs?

Thanks,

- Chris.
-- 
Chris Ball   <cjb@laptop.org>   <http://printf.net/>
One Laptop Per Child

Re: [PATCH 2/2] mmc: fix integer assignments to pointer

[Sam Ravnborg]

On Tue, Aug 23, 2011 at 02:28:42PM -0400, Chris Ball wrote:
> Hi,
> 
> On Tue, Aug 23 2011, Josh Triplett wrote:
> > {} produces the same effect, as far as I know.
> 
> Yeah.  I prefer {0}, because {} is a gcc-ism (the ANSI grammar demands
> initializer-lists be non-empty) and is less readable for people who
> haven't seen the idiom before and are wondering what's going on.
> 
> I'm still a little confused -- the {0} or memset(0, struct ..);
> formations are used often in the kernel, even with pointers involved.
> Is the warning (Wnon_pointer_null) run against the kernel by default,
> or did Venkatraman add it manually?  If default, is it catching bugs?

It is default enabled - and trigger a lot of warnings.
But the kernel idiom is to use NULL for a null pointer - and not to use 0.

Which is why the warning was introduced.

	Sam

Re: [PATCH 2/2] mmc: fix integer assignments to pointer

[Sam Ravnborg]

> 
> I'm still a little confused -- the {0} or memset(0, struct ..);
> formations are used often in the kernel, even with pointers involved.
> Is the warning (Wnon_pointer_null) run against the kernel by default,
> or did Venkatraman add it manually?  If default, is it catching bugs?

Did a quick "allnoconfig" build. With this minimal build the warning only triggered three times.
So a lot of these has been fixed already.

	Sam

Re: [PATCH 2/2] mmc: fix integer assignments to pointer

[S, Venkatraman]

On Wed, Aug 24, 2011 at 2:08 AM, Sam Ravnborg <sam@ravnborg.org> wrote:
>>
>> I'm still a little confused -- the {0} or memset(0, struct ..);
>> formations are used often in the kernel, even with pointers involved.
>> Is the warning (Wnon_pointer_null) run against the kernel by default,
>> or did Venkatraman add it manually?  If default, is it catching bugs?
>
> Did a quick "allnoconfig" build. With this minimal build the warning only triggered three times.
> So a lot of these has been fixed already.
>
>        Sam
>
I did my typical omap build on mainline and mmc-next and see that the warnings
have increased, and not fixed.
I will post upated patch series.

Re: [PATCH 2/2] mmc: fix integer assignments to pointer

[Ben Pfaff]

Josh Triplett <josh@joshtriplett.org> writes:

> On Tue, Aug 23, 2011 at 11:04:08AM -0700, Ben Pfaff wrote:
>> Josh Triplett <josh@joshtriplett.org> writes:
>> 
>> > On Tue, Aug 23, 2011 at 12:31:55PM -0400, Chris Ball wrote:
>> >> On Tue, Aug 23 2011, Venkatraman S wrote:
>> >> > -	struct mmc_request mrq = {0};
>> >> > +	struct mmc_request mrq = {NULL};
>> >> 
>> >> The sparse warning is mistaken.  Or I'm mistaken.  But I suspect it's
>> >> the sparse warning.
>> >
>> > Notice that it says "the remainder of the aggregate".  The first field
>> > still gets initialized with the 0 you supplied, and the first field of
>> > struct mmc_request has a pointer type.
>> 
>> That's an understandable position, but I think it would also be
>> reasonable for sparse to special case using {0} as an
>> initializer.  {0} is a valid initializer for every type and so
>> it's sometimes used as an initializer for a local variable to get
>> the same effect that one would have for a static variable without
>> specifying an initializer.
>
> {} produces the same effect, as far as I know.

{} is not a valid initializer for every type in the same way as
{0}, e.g.:

    blp@hardrock:~/db$ cat > tmp.c
    int x = {};
    int y = {0};
    blp@hardrock:~/db$ gcc tmp.c
    tmp.c:1:1: error: empty scalar initializer
    tmp.c:1:1: error: (near initialization for 'x')

-- 
Ben Pfaff 
http://benpfaff.org