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");
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 ***
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
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
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
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;
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
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