[PATCH 4/4] drivers/net/wireless/mwifiex/scan.c: test the just-initialized value

[PATCH 4/4] drivers/net/wireless/mwifiex/scan.c: test the just-initialized value

[Julia Lawall]

From: Julia Lawall <julia@diku.dk>

Test the just-initialized value rather than some other one.

The semantic match that finds this problem is as follows:
(http://coccinelle.lip6.fr/)

// <smpl>
@r@
identifier x,y,f!={PTR_ERR,ERR_PTR,ERR_CAST};
statement S;
@@

x = f(...);
(
if (\(x = NULL\|IS_ERR(x)\)) S
|
*if (\(y = NULL\|IS_ERR(y)\))
 { ... when != x
   return ...; }
)
// </smpl>

Signed-off-by: Julia Lawall <julia@diku.dk>

---
 drivers/net/wireless/mwifiex/scan.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/wireless/mwifiex/scan.c b/drivers/net/wireless/mwifiex/scan.c
index b28241c..d3111c9 100644
--- a/drivers/net/wireless/mwifiex/scan.c
+++ b/drivers/net/wireless/mwifiex/scan.c
@@ -1480,7 +1480,7 @@ mwifiex_update_curr_bss_params(struct mwifiex_private *priv,
 		return -ENOMEM;
 	}
 	beacon_ie = kzalloc(ie_len, GFP_KERNEL);
-	if (!bss_desc) {
+	if (!beacon_ie) {
 		dev_err(priv->adapter->dev, " failed to alloc bss_desc\n");
 		return -ENOMEM;
 	}

Re: [PATCH 4/4] drivers/net/wireless/mwifiex/scan.c: test the just-initialized

[Pierre Louis Aublin]

Hello all

On 08/22/2011 04:00 PM, Julia Lawall wrote:
> -	if (!bss_desc) {
> +	if (!beacon_ie) {
>   		dev_err(priv->adapter->dev, " failed to alloc bss_desc\n");
Shouldn't we also modify the error message, from "failed to alloc 
bss_desc" to "failed to alloc beacon_ie" ?

Sincerely
Pierre Louis Aublin

Re: [PATCH 4/4] drivers/net/wireless/mwifiex/scan.c: test the

[Julia Lawall]

On Mon, 22 Aug 2011, Pierre Louis Aublin wrote:

> Hello all
> 
> On 08/22/2011 04:00 PM, Julia Lawall wrote:
> > -	if (!bss_desc) {
> > +	if (!beacon_ie) {
> >     dev_err(priv->adapter->dev, " failed to alloc bss_desc\n");
> Shouldn't we also modify the error message, from "failed to alloc bss_desc" to
> "failed to alloc beacon_ie" ?

Sure :)

julia

[PATCH 4/4] drivers/net/wireless/mwifiex/scan.c: test the just-initialized

[Julia Lawall]

From: Julia Lawall <julia@diku.dk>

Test the just-initialized value rather than some other one.

The semantic match that finds this problem is as follows:
(http://coccinelle.lip6.fr/)

// <smpl>
@r@
identifier x,y,f!={PTR_ERR,ERR_PTR,ERR_CAST};
statement S;
@@

x = f(...);
(
if (\(x = NULL\|IS_ERR(x)\)) S
|
*if (\(y = NULL\|IS_ERR(y)\))
 { ... when != x
   return ...; }
)
// </smpl>

Signed-off-by: Julia Lawall <julia@diku.dk>

---
 drivers/net/wireless/mwifiex/scan.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/wireless/mwifiex/scan.c b/drivers/net/wireless/mwifiex/scan.c
index b28241c..37ca2f9 100644
--- a/drivers/net/wireless/mwifiex/scan.c
+++ b/drivers/net/wireless/mwifiex/scan.c
@@ -1480,8 +1480,8 @@ mwifiex_update_curr_bss_params(struct mwifiex_private *priv,
 		return -ENOMEM;
 	}
 	beacon_ie = kzalloc(ie_len, GFP_KERNEL);
-	if (!bss_desc) {
-		dev_err(priv->adapter->dev, " failed to alloc bss_desc\n");
+	if (!beacon_ie) {
+		dev_err(priv->adapter->dev, " failed to alloc beacon_ie\n");
 		return -ENOMEM;
 	}
 	memcpy(beacon_ie, ie_buf, ie_len);

RE: [PATCH 4/4] drivers/net/wireless/mwifiex/scan.c: test the

[Bing Zhao]

Hi Julia,

Thanks for the patch.

> -----Original Message-----
> From: Julia Lawall [mailto:julia@diku.dk]
> Sent: Monday, August 22, 2011 7:16 AM
> To: Pierre Louis Aublin
> Cc: Bing Zhao; kernel-janitors@vger.kernel.org; John W. Linville; linux-wireless@vger.kernel.org;
> netdev@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: [PATCH 4/4] drivers/net/wireless/mwifiex/scan.c: test the just-initialized value
> 
> From: Julia Lawall <julia@diku.dk>
> 
> Test the just-initialized value rather than some other one.
> 
> The semantic match that finds this problem is as follows:
> (http://coccinelle.lip6.fr/)
> 
> // <smpl>
> @r@
> identifier x,y,f!={PTR_ERR,ERR_PTR,ERR_CAST};
> statement S;
> @@
> 
> x = f(...);
> (
> if (\(x = NULL\|IS_ERR(x)\)) S
> |
> *if (\(y = NULL\|IS_ERR(y)\))
>  { ... when != x
>    return ...; }
> )
> // </smpl>
> 
> Signed-off-by: Julia Lawall <julia@diku.dk>

Acked-by: Bing Zhao <bzhao@marvell.com>


Thanks,
Bing

> 
> ---
>  drivers/net/wireless/mwifiex/scan.c |    4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/wireless/mwifiex/scan.c b/drivers/net/wireless/mwifiex/scan.c
> index b28241c..37ca2f9 100644
> --- a/drivers/net/wireless/mwifiex/scan.c
> +++ b/drivers/net/wireless/mwifiex/scan.c
> @@ -1480,8 +1480,8 @@ mwifiex_update_curr_bss_params(struct mwifiex_private *priv,
>  		return -ENOMEM;
>  	}
>  	beacon_ie = kzalloc(ie_len, GFP_KERNEL);
> -	if (!bss_desc) {
> -		dev_err(priv->adapter->dev, " failed to alloc bss_desc\n");
> +	if (!beacon_ie) {
> +		dev_err(priv->adapter->dev, " failed to alloc beacon_ie\n");
>  		return -ENOMEM;
>  	}
>  	memcpy(beacon_ie, ie_buf, ie_len);

Re: [PATCH 4/4] drivers/net/wireless/mwifiex/scan.c: test the just-initialized

[walter harms]

Am 22.08.2011 21:02, schrieb Bing Zhao:
> Hi Julia,
> 
> Thanks for the patch.
> 
>> -----Original Message-----
>> From: Julia Lawall [mailto:julia@diku.dk]
>> Sent: Monday, August 22, 2011 7:16 AM
>> To: Pierre Louis Aublin
>> Cc: Bing Zhao; kernel-janitors@vger.kernel.org; John W. Linville; linux-wireless@vger.kernel.org;
>> netdev@vger.kernel.org; linux-kernel@vger.kernel.org
>> Subject: [PATCH 4/4] drivers/net/wireless/mwifiex/scan.c: test the just-initialized value
>>
>> From: Julia Lawall <julia@diku.dk>
>>
>> Test the just-initialized value rather than some other one.
>>
>> The semantic match that finds this problem is as follows:
>> (http://coccinelle.lip6.fr/)
>>
>> // <smpl>
>> @r@
>> identifier x,y,f!={PTR_ERR,ERR_PTR,ERR_CAST};
>> statement S;
>> @@
>>
>> x = f(...);
>> (
>> if (\(x = NULL\|IS_ERR(x)\)) S
>> |
>> *if (\(y = NULL\|IS_ERR(y)\))
>>  { ... when != x
>>    return ...; }
>> )
>> // </smpl>
>>
>> Signed-off-by: Julia Lawall <julia@diku.dk>
> 
> Acked-by: Bing Zhao <bzhao@marvell.com>
> 
> 
> Thanks,
> Bing
> 
>>
>> ---
>>  drivers/net/wireless/mwifiex/scan.c |    4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/net/wireless/mwifiex/scan.c b/drivers/net/wireless/mwifiex/scan.c
>> index b28241c..37ca2f9 100644
>> --- a/drivers/net/wireless/mwifiex/scan.c
>> +++ b/drivers/net/wireless/mwifiex/scan.c
>> @@ -1480,8 +1480,8 @@ mwifiex_update_curr_bss_params(struct mwifiex_private *priv,
>>  		return -ENOMEM;
>>  	}
>>  	beacon_ie = kzalloc(ie_len, GFP_KERNEL);
>> -	if (!bss_desc) {
>> -		dev_err(priv->adapter->dev, " failed to alloc bss_desc\n");
>> +	if (!beacon_ie) {
>> +		dev_err(priv->adapter->dev, " failed to alloc beacon_ie\n");
>>  		return -ENOMEM;
>>  	}
>>  	memcpy(beacon_ie, ie_buf, ie_len);
> --


this looks like a case for kmemdup()

just my 2 cents,

re,
 wh

RE: [PATCH 4/4] drivers/net/wireless/mwifiex/scan.c: test the

[Bing Zhao]

> Am 22.08.2011 21:02, schrieb Bing Zhao:
> > Hi Julia,
> >
> > Thanks for the patch.
> >
> >> -----Original Message-----
> >> From: Julia Lawall [mailto:julia@diku.dk]
> >> Sent: Monday, August 22, 2011 7:16 AM
> >> To: Pierre Louis Aublin
> >> Cc: Bing Zhao; kernel-janitors@vger.kernel.org; John W. Linville; linux-wireless@vger.kernel.org;
> >> netdev@vger.kernel.org; linux-kernel@vger.kernel.org
> >> Subject: [PATCH 4/4] drivers/net/wireless/mwifiex/scan.c: test the just-initialized value
> >>
> >> From: Julia Lawall <julia@diku.dk>
> >>
> >> Test the just-initialized value rather than some other one.
> >>
> >> The semantic match that finds this problem is as follows:
> >> (http://coccinelle.lip6.fr/)
> >>
> >> // <smpl>
> >> @r@
> >> identifier x,y,f!={PTR_ERR,ERR_PTR,ERR_CAST};
> >> statement S;
> >> @@
> >>
> >> x = f(...);
> >> (
> >> if (\(x = NULL\|IS_ERR(x)\)) S
> >> |
> >> *if (\(y = NULL\|IS_ERR(y)\))
> >>  { ... when != x
> >>    return ...; }
> >> )
> >> // </smpl>
> >>
> >> Signed-off-by: Julia Lawall <julia@diku.dk>
> >
> > Acked-by: Bing Zhao <bzhao@marvell.com>
> >
> >
> > Thanks,
> > Bing
> >
> >>
> >> ---
> >>  drivers/net/wireless/mwifiex/scan.c |    4 ++--
> >>  1 file changed, 2 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/net/wireless/mwifiex/scan.c b/drivers/net/wireless/mwifiex/scan.c
> >> index b28241c..37ca2f9 100644
> >> --- a/drivers/net/wireless/mwifiex/scan.c
> >> +++ b/drivers/net/wireless/mwifiex/scan.c
> >> @@ -1480,8 +1480,8 @@ mwifiex_update_curr_bss_params(struct mwifiex_private *priv,
> >>  		return -ENOMEM;
> >>  	}
> >>  	beacon_ie = kzalloc(ie_len, GFP_KERNEL);
> >> -	if (!bss_desc) {
> >> -		dev_err(priv->adapter->dev, " failed to alloc bss_desc\n");
> >> +	if (!beacon_ie) {
> >> +		dev_err(priv->adapter->dev, " failed to alloc beacon_ie\n");
> >>  		return -ENOMEM;
> >>  	}
> >>  	memcpy(beacon_ie, ie_buf, ie_len);
> > --
> 
> 
> this looks like a case for kmemdup()

Hi Walter,

You are right. I believe there are some more cases where kmemdup can be used.

Hi John,

Please apply Julia's patch as I will send a separate patch later to address Walter's comment. Thanks.

> 
> just my 2 cents,

Thanks a lot for your 2 cents.

Bing

> 
> re,
>  wh
> 

Re: [PATCH 4/4] drivers/net/wireless/mwifiex/scan.c: test the just-initialized

[Larry Finger]

On 08/22/2011 09:00 AM, Julia Lawall wrote:
> From: Julia Lawall<julia@diku.dk>
>
> Test the just-initialized value rather than some other one.
>
> The semantic match that finds this problem is as follows:
> (http://coccinelle.lip6.fr/)
>
> //<smpl>
> @r@
> identifier x,y,f!={PTR_ERR,ERR_PTR,ERR_CAST};
> statement S;
> @@
>
> x = f(...);
> (
> if (\(x = NULL\|IS_ERR(x)\)) S
> |
> *if (\(y = NULL\|IS_ERR(y)\))
>   { ... when != x
>     return ...; }
> )
> //</smpl>
>
> Signed-off-by: Julia Lawall<julia@diku.dk>
>
> ---
>   drivers/net/wireless/mwifiex/scan.c |    2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/net/wireless/mwifiex/scan.c b/drivers/net/wireless/mwifiex/scan.c
> index b28241c..d3111c9 100644
> --- a/drivers/net/wireless/mwifiex/scan.c
> +++ b/drivers/net/wireless/mwifiex/scan.c
> @@ -1480,7 +1480,7 @@ mwifiex_update_curr_bss_params(struct mwifiex_private *priv,
>   		return -ENOMEM;
>   	}
>   	beacon_ie = kzalloc(ie_len, GFP_KERNEL);
> -	if (!bss_desc) {
> +	if (!beacon_ie) {
>   		dev_err(priv->adapter->dev, " failed to alloc bss_desc\n");
>   		return -ENOMEM;
>   	}

The error message should also get the bss_desc => beacon_ie chang.

Larry

Re: [PATCH 4/4] drivers/net/wireless/mwifiex/scan.c: test the

[Julia Lawall]

On Mon, 22 Aug 2011, Larry Finger wrote:

> On 08/22/2011 09:00 AM, Julia Lawall wrote:
> > From: Julia Lawall<julia@diku.dk>
> >
> > Test the just-initialized value rather than some other one.
> >
> > The semantic match that finds this problem is as follows:
> > (http://coccinelle.lip6.fr/)
> >
> > //<smpl>
> > @r@
> > identifier x,y,f!={PTR_ERR,ERR_PTR,ERR_CAST};
> > statement S;
> > @@
> >
> > x = f(...);
> > (
> > if (\(x = NULL\|IS_ERR(x)\)) S
> > |
> > *if (\(y = NULL\|IS_ERR(y)\))
> >   { ... when != x
> >     return ...; }
> > )
> > //</smpl>
> >
> > Signed-off-by: Julia Lawall<julia@diku.dk>
> >
> > ---
> >   drivers/net/wireless/mwifiex/scan.c |    2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/net/wireless/mwifiex/scan.c
> > b/drivers/net/wireless/mwifiex/scan.c
> > index b28241c..d3111c9 100644
> > --- a/drivers/net/wireless/mwifiex/scan.c
> > +++ b/drivers/net/wireless/mwifiex/scan.c
> > @@ -1480,7 +1480,7 @@ mwifiex_update_curr_bss_params(struct mwifiex_private
> > *priv,
> >    	return -ENOMEM;
> >    }
> >    beacon_ie = kzalloc(ie_len, GFP_KERNEL);
> > -	if (!bss_desc) {
> > +	if (!beacon_ie) {
> >     dev_err(priv->adapter->dev, " failed to alloc bss_desc\n");
> >     return -ENOMEM;
> >    }
> 
> The error message should also get the bss_desc => beacon_ie chang.

Thanks.  Pierre Louis Aublin made the same observation and I resumitted 
the patch.

julia

[PATCH 4/4] drivers/net/wireless/mwifiex/scan.c: convert GFP_KERNEL to GFP_ATOMIC

[Julia Lawall]

From: Julia Lawall <julia.lawall@lip6.fr>

The function is called with locks held and thus should not use GFP_KERNEL.

The semantic patch that makes this report is available
in scripts/coccinelle/locks/call_kern.cocci.

More information about semantic patching is available at
http://coccinelle.lip6.fr/

Signed-off-by: Julia Lawall <julia.lawall@lip6.fr>

---
 drivers/net/wireless/mwifiex/scan.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/wireless/mwifiex/scan.c b/drivers/net/wireless/mwifiex/scan.c
index 6396d33..98f1ca9 100644
--- a/drivers/net/wireless/mwifiex/scan.c
+++ b/drivers/net/wireless/mwifiex/scan.c
@@ -2001,7 +2001,7 @@ mwifiex_save_curr_bcn(struct mwifiex_private *priv)
 
 		kfree(priv->curr_bcn_buf);
 		priv->curr_bcn_buf = kmalloc(curr_bss->beacon_buf_size,
-						GFP_KERNEL);
+						GFP_ATOMIC);
 		if (!priv->curr_bcn_buf) {
 			dev_err(priv->adapter->dev,
 					"failed to alloc curr_bcn_buf\n");

RE: [PATCH 4/4] drivers/net/wireless/mwifiex/scan.c: convert

[Bing Zhao]

Hi Julia,

Thanks for the patch.

> Subject: [PATCH 4/4] drivers/net/wireless/mwifiex/scan.c: convert GFP_KERNEL to GFP_ATOMIC
> 
> From: Julia Lawall <julia.lawall@lip6.fr>
> 
> The function is called with locks held and thus should not use GFP_KERNEL.
> 
> The semantic patch that makes this report is available
> in scripts/coccinelle/locks/call_kern.cocci.
> 
> More information about semantic patching is available at
> http://coccinelle.lip6.fr/
> 
> Signed-off-by: Julia Lawall <julia.lawall@lip6.fr>

Acked-by: Bing Zhao <bzhao@marvell.com>

Regards,
Bing

> 
> ---
>  drivers/net/wireless/mwifiex/scan.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/wireless/mwifiex/scan.c b/drivers/net/wireless/mwifiex/scan.c
> index 6396d33..98f1ca9 100644
> --- a/drivers/net/wireless/mwifiex/scan.c
> +++ b/drivers/net/wireless/mwifiex/scan.c
> @@ -2001,7 +2001,7 @@ mwifiex_save_curr_bcn(struct mwifiex_private *priv)
> 
>  		kfree(priv->curr_bcn_buf);
>  		priv->curr_bcn_buf = kmalloc(curr_bss->beacon_buf_size,
> -						GFP_KERNEL);
> +						GFP_ATOMIC);
>  		if (!priv->curr_bcn_buf) {
>  			dev_err(priv->adapter->dev,
>  					"failed to alloc curr_bcn_buf\n");