Re: [PATCH 13/30] net: Don't do pointless kmalloc return value casts in zd1211 driver
Jesper Juhl <[EMAIL PROTECTED]> wrote:
> On 30/08/2007, Daniel Drake <[EMAIL PROTECTED]> wrote:
>> Jesper Juhl wrote:
>> > Since kmalloc() returns a void pointer there is no reason to cast
>> > its return value.
>> > This patch also removes a pointless initialization of a variable.
>>
>> NAK: adds a sparse warning
>> zd_chip.c:116:15: warning: implicit cast to nocast type
>>
> Ok, I must admit I didn't check with sparse since it seemed pointless
> - we usually never cast void pointers to other pointer types,
> specifically because the C language nicely guarantees that the right
> thing will happen without the cast. Sometimes we have to cast them to
> integer types, su sure we need the cast there. But what on earth
> makes a "zd_addr_t *" so special that we have to explicitly cast a
> "void *" to that type?
>
> I see it's defined as
> typedef u32 __nocast zd_addr_t;
> in drivers/net/wireless/zd1211rw/zd_types.h , but why the __nocast ?
Nevermind the __nocast, this looks like a bug in sparse. Just
because a base type is __nocast, sparse shouldn't infer that a
pointer to it should also be __nocast.
Cheers,
--
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <[EMAIL PROTECTED]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
-
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 13/30] net: Don't do pointless kmalloc return value casts in zd1211 driver
Jesper Juhl wrote: What would be wrong in applying my patch that removes the cast of the kmalloc() return value and then also remove the "__nocast" here? We use it as a safety measure when coding. For example the write register function takes an address and a value. We got one of these the wrong way round once, and had a non-obvious bug. nocast and sparse helps us prevent this. Daniel - To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Don't needlessly initialize variable to NULL in zd_chip (was: Re: [PATCH 13/30] net: Don't do pointless kmalloc return value casts in zd1211 driver)
On 31/08/2007, Randy Dunlap <[EMAIL PROTECTED]> wrote: ... > > BTW: http://marc.info/?l=linux-wireless&m=118831813500769&w=2 > ... Heh, thanks Randy. All too often patches get missed since I don't happen to include the right magic person to Cc. So I generally take a "better to have one Cc too many than miss one" approach when sending patches - otherwise I just end up resending it several times and eventually have to bother Andrew to move it through -mm. I see the point of people not getting things twice, but too many patches slip through the cracks already (and not just my patches, that's a general problem) so I'd rather inconvenience a few people with one extra email than missing the proper maintainer by not Cc'ing the right list.Picking the right list/set of people to Cc is hard! (whoops, mistakenly didn't do a reply-to-all initially; sorry Randy, now you'll get this mail twice ;) -- Jesper Juhl <[EMAIL PROTECTED]> Don't top-post http://www.catb.org/~esr/jargon/html/T/top-post.html Plain text mails only, please http://www.expita.com/nomime.html - To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Don't needlessly initialize variable to NULL in zd_chip (was: Re: [PATCH 13/30] net: Don't do pointless kmalloc return value casts in zd1211 driver)
On Fri, 31 Aug 2007 00:30:31 +0200 Jesper Juhl wrote:
> On Friday 31 August 2007 00:19:53 Joe Perches wrote:
> > On Thu, 2007-08-30 at 22:20 +0200, Jesper Juhl wrote:
> > > Ok, I must admit I didn't check with sparse since it seemed pointless
> > > - we usually never cast void pointers to other pointer types,
> > > specifically because the C language nicely guarantees that the right
> > > thing will happen without the cast. Sometimes we have to cast them to
> > > integer types, su sure we need the cast there. But what on earth
> > > makes a "zd_addr_t *" so special that we have to explicitly cast a
> > > "void *" to that type?
> >
> > http://marc.info/?l=linux-netdev&m=117113743902549&w=1
> >
>
> Thank you for that link Joe.
>
> I'm not sure I agree with the __nocast, but I respect that this is
> the maintainers choice.
>
> But, I still think the first chunk of the patch that removes the
> initial variable initialization makes sense.
> Initializing the variable to NULL is pointless since it'll never be
> used before the kmalloc() call. So here's a revised patch that just
> gets rid of that little detail.
BTW: http://marc.info/?l=linux-wireless&m=118831813500769&w=2
> No need to initialize to NULL when variable is never used before
> it's assigned the return value of a kmalloc() call.
>
> Signed-off-by: Jesper Juhl <[EMAIL PROTECTED]>
> ---
>
> diff --git a/drivers/net/wireless/zd1211rw/zd_chip.c
> b/drivers/net/wireless/zd1211rw/zd_chip.c
> index c39f198..30872fe 100644
> --- a/drivers/net/wireless/zd1211rw/zd_chip.c
> +++ b/drivers/net/wireless/zd1211rw/zd_chip.c
> @@ -106,7 +106,7 @@ int zd_ioread32v_locked(struct zd_chip *chip, u32
> *values, const zd_addr_t *addr
> {
> int r;
> int i;
> - zd_addr_t *a16 = (zd_addr_t *)NULL;
> + zd_addr_t *a16;
> u16 *v16;
> unsigned int count16;
---
~Randy
*** Remember to use Documentation/SubmitChecklist when testing your code ***
-
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] Don't needlessly initialize variable to NULL in zd_chip (was: Re: [PATCH 13/30] net: Don't do pointless kmalloc return value casts in zd1211 driver)
On Friday 31 August 2007 00:19:53 Joe Perches wrote:
> On Thu, 2007-08-30 at 22:20 +0200, Jesper Juhl wrote:
> > Ok, I must admit I didn't check with sparse since it seemed pointless
> > - we usually never cast void pointers to other pointer types,
> > specifically because the C language nicely guarantees that the right
> > thing will happen without the cast. Sometimes we have to cast them to
> > integer types, su sure we need the cast there. But what on earth
> > makes a "zd_addr_t *" so special that we have to explicitly cast a
> > "void *" to that type?
>
> http://marc.info/?l=linux-netdev&m=117113743902549&w=1
>
Thank you for that link Joe.
I'm not sure I agree with the __nocast, but I respect that this is
the maintainers choice.
But, I still think the first chunk of the patch that removes the
initial variable initialization makes sense.
Initializing the variable to NULL is pointless since it'll never be
used before the kmalloc() call. So here's a revised patch that just
gets rid of that little detail.
No need to initialize to NULL when variable is never used before
it's assigned the return value of a kmalloc() call.
Signed-off-by: Jesper Juhl <[EMAIL PROTECTED]>
---
diff --git a/drivers/net/wireless/zd1211rw/zd_chip.c
b/drivers/net/wireless/zd1211rw/zd_chip.c
index c39f198..30872fe 100644
--- a/drivers/net/wireless/zd1211rw/zd_chip.c
+++ b/drivers/net/wireless/zd1211rw/zd_chip.c
@@ -106,7 +106,7 @@ int zd_ioread32v_locked(struct zd_chip *chip, u32 *values,
const zd_addr_t *addr
{
int r;
int i;
- zd_addr_t *a16 = (zd_addr_t *)NULL;
+ zd_addr_t *a16;
u16 *v16;
unsigned int count16;
-
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 13/30] net: Don't do pointless kmalloc return value casts in zd1211 driver
On Thu, 2007-08-30 at 22:20 +0200, Jesper Juhl wrote: > Ok, I must admit I didn't check with sparse since it seemed pointless > - we usually never cast void pointers to other pointer types, > specifically because the C language nicely guarantees that the right > thing will happen without the cast. Sometimes we have to cast them to > integer types, su sure we need the cast there. But what on earth > makes a "zd_addr_t *" so special that we have to explicitly cast a > "void *" to that type? http://marc.info/?l=linux-netdev&m=117113743902549&w=1 - To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 13/30] net: Don't do pointless kmalloc return value casts in zd1211 driver
On 30/08/2007, Daniel Drake <[EMAIL PROTECTED]> wrote: > Jesper Juhl wrote: > > Since kmalloc() returns a void pointer there is no reason to cast > > its return value. > > This patch also removes a pointless initialization of a variable. > > NAK: adds a sparse warning > zd_chip.c:116:15: warning: implicit cast to nocast type > Ok, I must admit I didn't check with sparse since it seemed pointless - we usually never cast void pointers to other pointer types, specifically because the C language nicely guarantees that the right thing will happen without the cast. Sometimes we have to cast them to integer types, su sure we need the cast there. But what on earth makes a "zd_addr_t *" so special that we have to explicitly cast a "void *" to that type? I see it's defined as typedef u32 __nocast zd_addr_t; in drivers/net/wireless/zd1211rw/zd_types.h , but why the __nocast ? What would be wrong in applying my patch that removes the cast of the kmalloc() return value and then also remove the "__nocast" here? -- Jesper Juhl <[EMAIL PROTECTED]> Don't top-post http://www.catb.org/~esr/jargon/html/T/top-post.html Plain text mails only, please http://www.expita.com/nomime.html - To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 13/30] net: Don't do pointless kmalloc return value casts in zd1211 driver
Jesper Juhl wrote:
Since kmalloc() returns a void pointer there is no reason to cast
its return value.
This patch also removes a pointless initialization of a variable.
NAK: adds a sparse warning
zd_chip.c:116:15: warning: implicit cast to nocast type
Signed-off-by: Jesper Juhl <[EMAIL PROTECTED]>
---
drivers/net/wireless/zd1211rw/zd_chip.c |5 ++---
1 files changed, 2 insertions(+), 3 deletions(-)
diff --git a/drivers/net/wireless/zd1211rw/zd_chip.c
b/drivers/net/wireless/zd1211rw/zd_chip.c
index c39f198..4942e5c 100644
--- a/drivers/net/wireless/zd1211rw/zd_chip.c
+++ b/drivers/net/wireless/zd1211rw/zd_chip.c
@@ -106,8 +106,8 @@ int zd_ioread32v_locked(struct zd_chip *chip, u32 *values,
const zd_addr_t *addr
{
int r;
int i;
- zd_addr_t *a16 = (zd_addr_t *)NULL;
u16 *v16;
+ zd_addr_t *a16;
unsigned int count16;
if (count > USB_MAX_IOREAD32_COUNT)
@@ -115,8 +115,7 @@ int zd_ioread32v_locked(struct zd_chip *chip, u32 *values,
const zd_addr_t *addr
/* Allocate a single memory block for values and addresses. */
count16 = 2*count;
- a16 = (zd_addr_t *) kmalloc(count16 * (sizeof(zd_addr_t) + sizeof(u16)),
- GFP_KERNEL);
+ a16 = kmalloc(count16 * (sizeof(zd_addr_t) + sizeof(u16)), GFP_KERNEL);
if (!a16) {
dev_dbg_f(zd_chip_dev(chip),
"error ENOMEM in allocation of a16\n");
-
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
