| Message ID | 1314114363-6767-3-git-send-email-svenkatr@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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.
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 -- 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
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 -- 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
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.
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.
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 -- 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
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.
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 -- 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
> > 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 -- 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
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')
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
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-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- 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
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; @@ -466,7 +466,7 @@ static u32 mmc_sd_num_wr_blocks(struct mmc_card *card) u32 result; __be32 *blocks; - struct mmc_request mrq = {0}; + struct mmc_request mrq = {NULL}; struct mmc_command cmd = {0}; struct mmc_data data = {0}; unsigned int timeout_us; diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c index 89bdeae..11fcf20 100644 --- a/drivers/mmc/core/core.c +++ b/drivers/mmc/core/core.c @@ -330,7 +330,7 @@ EXPORT_SYMBOL(mmc_wait_for_req); */ int mmc_wait_for_cmd(struct mmc_host *host, struct mmc_command *cmd, int retries) { - struct mmc_request mrq = {0}; + struct mmc_request mrq = {NULL}; WARN_ON(!host->claimed); diff --git a/drivers/mmc/core/mmc_ops.c b/drivers/mmc/core/mmc_ops.c index 845ce7c..8f1d372 100644 --- a/drivers/mmc/core/mmc_ops.c +++ b/drivers/mmc/core/mmc_ops.c @@ -233,7 +233,7 @@ static int mmc_send_cxd_data(struct mmc_card *card, struct mmc_host *host, u32 opcode, void *buf, unsigned len) { - struct mmc_request mrq = {0}; + struct mmc_request mrq = {NULL}; struct mmc_command cmd = {0}; struct mmc_data data = {0}; struct scatterlist sg; @@ -454,7 +454,7 @@ static int mmc_send_bus_test(struct mmc_card *card, struct mmc_host *host, u8 opcode, u8 len) { - struct mmc_request mrq = {0}; + struct mmc_request mrq = {NULL}; struct mmc_command cmd = {0}; struct mmc_data data = {0}; struct scatterlist sg; diff --git a/drivers/mmc/core/sdio_ops.c b/drivers/mmc/core/sdio_ops.c index f087d87..4addbe9 100644 --- a/drivers/mmc/core/sdio_ops.c +++ b/drivers/mmc/core/sdio_ops.c @@ -121,7 +121,7 @@ int mmc_io_rw_direct(struct mmc_card *card, int write, unsigned fn, int mmc_io_rw_extended(struct mmc_card *card, int write, unsigned fn, unsigned addr, int incr_addr, u8 *buf, unsigned blocks, unsigned blksz) { - struct mmc_request mrq = {0}; + struct mmc_request mrq = {NULL}; struct mmc_command cmd = {0}; struct mmc_data data = {0}; struct scatterlist sg;
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(-)