[PATCH 14/30] net: Kill some unneeded allocation return value casts in libertas

[PATCH 14/30] net: Kill some unneeded allocation return value casts in libertas

[Jesper Juhl]

kmalloc() and friends return void*, no need to cast it.

Signed-off-by: Jesper Juhl <jesper.juhl@gmail.com>
---
 drivers/net/wireless/libertas/debugfs.c |    2 +-
 drivers/net/wireless/libertas/ethtool.c |    3 +--
 2 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/net/wireless/libertas/debugfs.c b/drivers/net/wireless/libertas/debugfs.c
index 715cbda..6ade63e 100644
--- a/drivers/net/wireless/libertas/debugfs.c
+++ b/drivers/net/wireless/libertas/debugfs.c
@@ -1839,7 +1839,7 @@ static ssize_t wlan_debugfs_write(struct file *f, const char __user *buf,
 	char *p2;
 	struct debug_data *d = (struct debug_data *)f->private_data;
 
-	pdata = (char *)kmalloc(cnt, GFP_KERNEL);
+	pdata = kmalloc(cnt, GFP_KERNEL);
 	if (pdata == NULL)
 		return 0;
 
diff --git a/drivers/net/wireless/libertas/ethtool.c b/drivers/net/wireless/libertas/ethtool.c
index 96f1974..7dad493 100644
--- a/drivers/net/wireless/libertas/ethtool.c
+++ b/drivers/net/wireless/libertas/ethtool.c
@@ -60,8 +60,7 @@ static int libertas_ethtool_get_eeprom(struct net_device *dev,
 
 //      mutex_lock(&priv->mutex);
 
-	adapter->prdeeprom =
-		    (char *)kmalloc(eeprom->len+sizeof(regctrl), GFP_KERNEL);
+	adapter->prdeeprom = kmalloc(eeprom->len+sizeof(regctrl), GFP_KERNEL);
 	if (!adapter->prdeeprom)
 		return -ENOMEM;
 	memcpy(adapter->prdeeprom, &regctrl, sizeof(regctrl));
-- 
1.5.2.2

Re: [PATCH 14/30] net: Kill some unneeded allocation return value casts in libertas

[Dan Williams]

On Fri, 2007-08-24 at 02:03 +0200, Jesper Juhl wrote:
> kmalloc() and friends return void*, no need to cast it.

Applied to libertas-2.6 'for-linville' branch, thanks.

Dan

> Signed-off-by: Jesper Juhl <jesper.juhl@gmail.com>
> ---
>  drivers/net/wireless/libertas/debugfs.c |    2 +-
>  drivers/net/wireless/libertas/ethtool.c |    3 +--
>  2 files changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/wireless/libertas/debugfs.c b/drivers/net/wireless/libertas/debugfs.c
> index 715cbda..6ade63e 100644
> --- a/drivers/net/wireless/libertas/debugfs.c
> +++ b/drivers/net/wireless/libertas/debugfs.c
> @@ -1839,7 +1839,7 @@ static ssize_t wlan_debugfs_write(struct file *f, const char __user *buf,
>  	char *p2;
>  	struct debug_data *d = (struct debug_data *)f->private_data;
>  
> -	pdata = (char *)kmalloc(cnt, GFP_KERNEL);
> +	pdata = kmalloc(cnt, GFP_KERNEL);
>  	if (pdata == NULL)
>  		return 0;
>  
> diff --git a/drivers/net/wireless/libertas/ethtool.c b/drivers/net/wireless/libertas/ethtool.c
> index 96f1974..7dad493 100644
> --- a/drivers/net/wireless/libertas/ethtool.c
> +++ b/drivers/net/wireless/libertas/ethtool.c
> @@ -60,8 +60,7 @@ static int libertas_ethtool_get_eeprom(struct net_device *dev,
>  
>  //      mutex_lock(&priv->mutex);
>  
> -	adapter->prdeeprom =
> -		    (char *)kmalloc(eeprom->len+sizeof(regctrl), GFP_KERNEL);
> +	adapter->prdeeprom = kmalloc(eeprom->len+sizeof(regctrl), GFP_KERNEL);
>  	if (!adapter->prdeeprom)
>  		return -ENOMEM;
>  	memcpy(adapter->prdeeprom, &regctrl, sizeof(regctrl));

Re: [PATCH 13/30] net: Don't do pointless kmalloc return value casts in zd1211 driver

[Daniel Drake]

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 <jesper.juhl@gmail.com>
> ---
>  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");

Re: [PATCH 13/30] net: Don't do pointless kmalloc return value casts in zd1211 driver

[Jesper Juhl]

On 30/08/2007, Daniel Drake <dsd@gentoo.org> 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 <jesper.juhl@gmail.com>
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

Re: [PATCH 13/30] net: Don't do pointless kmalloc return value casts in zd1211 driver

[Joe Perches]

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

[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)

[Jesper Juhl]

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 <jesper.juhl@gmail.com>
---

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;
 

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)

[Randy Dunlap]

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 <jesper.juhl@gmail.com>
> ---
> 
> 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 ***

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)

[Jesper Juhl]

On 31/08/2007, Randy Dunlap <randy.dunlap@oracle.com> 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 <jesper.juhl@gmail.com>
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

Re: [PATCH 13/30] net: Don't do pointless kmalloc return value casts in zd1211 driver

[Daniel Drake]

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

Re: [PATCH 13/30] net: Don't do pointless kmalloc return value casts in zd1211 driver

[Herbert Xu]

Jesper Juhl <jesper.juhl@gmail.com> wrote:
> On 30/08/2007, Daniel Drake <dsd@gentoo.org> 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~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt