[PATCH 0/13] make return of 0 explicit

[PATCH 0/13] make return of 0 explicit

[Julia Lawall]

Sometimes a local variable is used as a return value in a case where the
return value is always 0.  The result is more apparent if this variable is
just replaced by 0.  This is done by the following semantic patch, although
some cleanups may be needed. (http://coccinelle.lip6.fr/)

// <smpl>
@r exists@
local idexpression ret;
identifier reti;
expression e;
position p;
@@

ret@reti = 0;
... when != \(ret = e\|ret &= e\|ret |= e\|ret += e\|ret -= e\|ret *= e\|ret ^= e\)
    when != \(++ret\|--ret\|ret++\|ret--\)
    when != &ret
return ret;@p

@bad1 exists@
expression e != 0;
local idexpression r.ret;
position r.p;
@@

 \(ret = e\|ret &= e\|ret |= e\|ret += e\|ret -= e\|ret *= e\|ret ^= e\|++ret\|--ret\|ret++\|ret--\|&ret\)
 ...
 return ret;@p

@bad2 exists@
identifier r.reti;
position r.p;
identifier f;
type T;
@@

f(...,T reti,...) {
 ...
 return reti;@p
}

@change depends on !bad1 && !bad2@
position r.p;
local idexpression r.ret;
identifier f;
@@

f(...) { <+...
return 
-ret
+0
;@p
...+> }

@rewrite@
local idexpression r.ret;
expression e;
position p;
identifier change.f;
@@

f(...) { <+...
 \(ret@p = e\|&ret@p\)
...+> }

@depends on change@
local idexpression r.ret;
position p != rewrite.p;
identifier change.f;
@@

f(...) { <+...
(
break;
|
ret = 0;
... when exists
ret@p
... when any
|
- ret = 0;
)
...+> }

@depends on change@
identifier r.reti;
type T;
constant C;
identifier change.f;
@@

f(...) { ... when any
-T reti = C;
 ... when != reti
     when strict
 }

@depends on change@
identifier r.reti;
type T;
identifier change.f;
@@

f(...) { ... when any
-T reti;
 ... when != reti
     when strict
 }
// </smpl>

The first four rules detect cases where only 0 reaches a return.  The
remaining rules clean up any variable initializations or declarations that
are made unnecessary by this change.

[PATCH 1/13] libertas: make return of 0 explicit

[Julia Lawall]

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

Delete unnecessary local variable whose value is always 0 and that hides
the fact that the result is always 0.

A simplified version of the semantic patch that fixes this problem is as
follows: (http://coccinelle.lip6.fr/)

// <smpl>
@r exists@
local idexpression ret;
expression e;
position p;
@@

-ret = 0;
... when != ret = e
return 
- ret
+ 0
  ;
// </smpl>

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

---
Alternatively, was an error code intended in the bad length case, as is
done in process_brxed_802_11_packet?

 drivers/net/wireless/libertas/rx.c |    7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/drivers/net/wireless/libertas/rx.c b/drivers/net/wireless/libertas/rx.c
index c7366b0..807c5b8 100644
--- a/drivers/net/wireless/libertas/rx.c
+++ b/drivers/net/wireless/libertas/rx.c
@@ -55,7 +55,6 @@ static int process_rxed_802_11_packet(struct lbs_private *priv,
  */
 int lbs_process_rxed_packet(struct lbs_private *priv, struct sk_buff *skb)
 {
-	int ret = 0;
 	struct net_device *dev = priv->dev;
 	struct rxpackethdr *p_rx_pkt;
 	struct rxpd *p_rx_pd;
@@ -86,7 +85,6 @@ int lbs_process_rxed_packet(struct lbs_private *priv, struct sk_buff *skb)
 	if (skb->len < (ETH_HLEN + 8 + sizeof(struct rxpd))) {
 		lbs_deb_rx("rx err: frame received with bad length\n");
 		dev->stats.rx_length_errors++;
-		ret = 0;
 		dev_kfree_skb(skb);
 		goto done;
 	}
@@ -154,10 +152,9 @@ int lbs_process_rxed_packet(struct lbs_private *priv, struct sk_buff *skb)
 	else
 		netif_rx_ni(skb);
 
-	ret = 0;
 done:
-	lbs_deb_leave_args(LBS_DEB_RX, "ret %d", ret);
-	return ret;
+	lbs_deb_leave_args(LBS_DEB_RX, "ret %d", 0);
+	return 0;
 }
 EXPORT_SYMBOL_GPL(lbs_process_rxed_packet);
 

[PATCH 4/13] wimax/i2400m: make return of 0 explicit

[Julia Lawall]

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

Delete unnecessary local variable whose value is always 0 and that hides
the fact that the result is always 0.

A simplified version of the semantic patch that fixes this problem is as
follows: (http://coccinelle.lip6.fr/)

// <smpl>
@r exists@
local idexpression ret;
expression e;
position p;
@@

-ret = 0;
... when != ret = e
return 
- ret
+ 0
  ;
// </smpl>

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

---
Alternatively, is an error code wanted under the if?

 drivers/net/wimax/i2400m/driver.c |    7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/drivers/net/wimax/i2400m/driver.c b/drivers/net/wimax/i2400m/driver.c
index 9c34d2f..901a534 100644
--- a/drivers/net/wimax/i2400m/driver.c
+++ b/drivers/net/wimax/i2400m/driver.c
@@ -500,26 +500,23 @@ int i2400m_pm_notifier(struct notifier_block *notifier,
  */
 int i2400m_pre_reset(struct i2400m *i2400m)
 {
-	int result;
 	struct device *dev = i2400m_dev(i2400m);
 
 	d_fnstart(3, dev, "(i2400m %p)\n", i2400m);
 	d_printf(1, dev, "pre-reset shut down\n");
 
-	result = 0;
 	mutex_lock(&i2400m->init_mutex);
 	if (i2400m->updown) {
 		netif_tx_disable(i2400m->wimax_dev.net_dev);
 		__i2400m_dev_stop(i2400m);
-		result = 0;
 		/* down't set updown to zero -- this way
 		 * post_reset can restore properly */
 	}
 	mutex_unlock(&i2400m->init_mutex);
 	if (i2400m->bus_release)
 		i2400m->bus_release(i2400m);
-	d_fnend(3, dev, "(i2400m %p) = %d\n", i2400m, result);
-	return result;
+	d_fnend(3, dev, "(i2400m %p) = %d\n", i2400m, 0);
+	return 0;
 }
 EXPORT_SYMBOL_GPL(i2400m_pre_reset);
 

[PATCH 6/13] [NETFILTER]: arp_tables: make return of 0 explicit

[Julia Lawall]

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

Delete unnecessary local variable whose value is always 0 and that hides
the fact that the result is always 0.

A simplified version of the semantic patch that fixes this problem is as
follows: (http://coccinelle.lip6.fr/)

// <smpl>
@r exists@
local idexpression ret;
expression e;
position p;
@@

-ret = 0;
... when != ret = e
return 
- ret
+ 0
  ;
// </smpl>

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

---
 net/ipv4/netfilter/arp_tables.c |    5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/net/ipv4/netfilter/arp_tables.c b/net/ipv4/netfilter/arp_tables.c
index f95b6f9..89f6737 100644
--- a/net/ipv4/netfilter/arp_tables.c
+++ b/net/ipv4/netfilter/arp_tables.c
@@ -1289,9 +1289,8 @@ compat_copy_entry_from_user(struct compat_arpt_entry *e, void **dstptr,
 	struct xt_target *target;
 	struct arpt_entry *de;
 	unsigned int origsize;
-	int ret, h;
+	int h;
 
-	ret = 0;
 	origsize = *size;
 	de = (struct arpt_entry *)*dstptr;
 	memcpy(de, e, sizeof(struct arpt_entry));
@@ -1312,7 +1311,7 @@ compat_copy_entry_from_user(struct compat_arpt_entry *e, void **dstptr,
 		if ((unsigned char *)de - base < newinfo->underflow[h])
 			newinfo->underflow[h] -= origsize - *size;
 	}
-	return ret;
+	return 0;
 }
 
 static int translate_compat_table(const char *name,

[PATCH 12/13] brcmsmac: make return of 0 explicit

[Julia Lawall]

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

Delete unnecessary local variable whose value is always 0 and that hides
the fact that the result is always 0.

A simplified version of the semantic patch that fixes this problem is as
follows: (http://coccinelle.lip6.fr/)

// <smpl>
@r exists@
local idexpression ret;
expression e;
position p;
@@

-ret = 0;
... when != ret = e
return 
- ret
+ 0
  ;
// </smpl>

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

---
 drivers/net/wireless/brcm80211/brcmsmac/main.c |    5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/net/wireless/brcm80211/brcmsmac/main.c b/drivers/net/wireless/brcm80211/brcmsmac/main.c
index 9417cb5..3054725 100644
--- a/drivers/net/wireless/brcm80211/brcmsmac/main.c
+++ b/drivers/net/wireless/brcm80211/brcmsmac/main.c
@@ -4875,9 +4875,6 @@ static int brcms_b_detach(struct brcms_c_info *wlc)
 	uint i;
 	struct brcms_hw_band *band;
 	struct brcms_hardware *wlc_hw = wlc->hw;
-	int callbacks;
-
-	callbacks = 0;
 
 	brcms_b_detach_dmapio(wlc_hw);
 
@@ -4901,7 +4898,7 @@ static int brcms_b_detach(struct brcms_c_info *wlc)
 		wlc_hw->sih = NULL;
 	}
 
-	return callbacks;
+	return 0;
 
 }
 

Re: [PATCH 1/13] libertas: make return of 0 explicit

[walter harms]

Am 19.05.2014 06:31, schrieb Julia Lawall:
> From: Julia Lawall <Julia.Lawall@lip6.fr>
> 
> Delete unnecessary local variable whose value is always 0 and that hides
> the fact that the result is always 0.
> 
> A simplified version of the semantic patch that fixes this problem is as
> follows: (http://coccinelle.lip6.fr/)
> 
> // <smpl>
> @r exists@
> local idexpression ret;
> expression e;
> position p;
> @@
> 
> -ret = 0;
> ... when != ret = e
> return 
> - ret
> + 0
>   ;
> // </smpl>
> 
> Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr>
> 
> ---
> Alternatively, was an error code intended in the bad length case, as is
> done in process_brxed_802_11_packet?
> 
>  drivers/net/wireless/libertas/rx.c |    7 ++-----
>  1 file changed, 2 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/net/wireless/libertas/rx.c b/drivers/net/wireless/libertas/rx.c
> index c7366b0..807c5b8 100644
> --- a/drivers/net/wireless/libertas/rx.c
> +++ b/drivers/net/wireless/libertas/rx.c
> @@ -55,7 +55,6 @@ static int process_rxed_802_11_packet(struct lbs_private *priv,
>   */
>  int lbs_process_rxed_packet(struct lbs_private *priv, struct sk_buff *skb)
>  {
> -	int ret = 0;
>  	struct net_device *dev = priv->dev;
>  	struct rxpackethdr *p_rx_pkt;
>  	struct rxpd *p_rx_pd;
> @@ -86,7 +85,6 @@ int lbs_process_rxed_packet(struct lbs_private *priv, struct sk_buff *skb)
>  	if (skb->len < (ETH_HLEN + 8 + sizeof(struct rxpd))) {
>  		lbs_deb_rx("rx err: frame received with bad length\n");
>  		dev->stats.rx_length_errors++;
> -		ret = 0;
>  		dev_kfree_skb(skb);
>  		goto done;
>  	}
> @@ -154,10 +152,9 @@ int lbs_process_rxed_packet(struct lbs_private *priv, struct sk_buff *skb)
>  	else
>  		netif_rx_ni(skb);
>  
> -	ret = 0;
>  done:
> -	lbs_deb_leave_args(LBS_DEB_RX, "ret %d", ret);
> -	return ret;
> +	lbs_deb_leave_args(LBS_DEB_RX, "ret %d", 0);
> +	return 0;
>  }

i guess lbs_deb_leave_args() is obsolet now.

re,
 wh


>  EXPORT_SYMBOL_GPL(lbs_process_rxed_packet);
>  
> 
> --
> To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH 4/13] wimax/i2400m: make return of 0 explicit

[walter harms]

Am 19.05.2014 06:31, schrieb Julia Lawall:
> From: Julia Lawall <Julia.Lawall@lip6.fr>
> 
> Delete unnecessary local variable whose value is always 0 and that hides
> the fact that the result is always 0.
> 
> A simplified version of the semantic patch that fixes this problem is as
> follows: (http://coccinelle.lip6.fr/)
> 
> // <smpl>
> @r exists@
> local idexpression ret;
> expression e;
> position p;
> @@
> 
> -ret = 0;
> ... when != ret = e
> return 
> - ret
> + 0
>   ;
> // </smpl>
> 
> Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr>
> 
> ---
> Alternatively, is an error code wanted under the if?
> 
>  drivers/net/wimax/i2400m/driver.c |    7 ++-----
>  1 file changed, 2 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/net/wimax/i2400m/driver.c b/drivers/net/wimax/i2400m/driver.c
> index 9c34d2f..901a534 100644
> --- a/drivers/net/wimax/i2400m/driver.c
> +++ b/drivers/net/wimax/i2400m/driver.c
> @@ -500,26 +500,23 @@ int i2400m_pm_notifier(struct notifier_block *notifier,
>   */
>  int i2400m_pre_reset(struct i2400m *i2400m)
>  {
> -	int result;
>  	struct device *dev = i2400m_dev(i2400m);
>  
>  	d_fnstart(3, dev, "(i2400m %p)\n", i2400m);
>  	d_printf(1, dev, "pre-reset shut down\n");
>  
> -	result = 0;
>  	mutex_lock(&i2400m->init_mutex);
>  	if (i2400m->updown) {
>  		netif_tx_disable(i2400m->wimax_dev.net_dev);
>  		__i2400m_dev_stop(i2400m);
> -		result = 0;
>  		/* down't set updown to zero -- this way
>  		 * post_reset can restore properly */
>  	}
>  	mutex_unlock(&i2400m->init_mutex);
>  	if (i2400m->bus_release)
>  		i2400m->bus_release(i2400m);
> -	d_fnend(3, dev, "(i2400m %p) = %d\n", i2400m, result);
> -	return result;
> +	d_fnend(3, dev, "(i2400m %p) = %d\n", i2400m, 0);


	d_fnend(3, dev, "i2400m=%p\n", i2400m);

or something like that
re,
 wh

> +	return 0;
>  }
>  EXPORT_SYMBOL_GPL(i2400m_pre_reset);
>  
> 
> --
> To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH 4/13] wimax/i2400m: make return of 0 explicit

[Julia Lawall]

On Mon, 19 May 2014, walter harms wrote:

>
>
> Am 19.05.2014 06:31, schrieb Julia Lawall:
> > From: Julia Lawall <Julia.Lawall@lip6.fr>
> >
> > Delete unnecessary local variable whose value is always 0 and that hides
> > the fact that the result is always 0.
> >
> > A simplified version of the semantic patch that fixes this problem is as
> > follows: (http://coccinelle.lip6.fr/)
> >
> > // <smpl>
> > @r exists@
> > local idexpression ret;
> > expression e;
> > position p;
> > @@
> >
> > -ret = 0;
> > ... when != ret = e
> > return
> > - ret
> > + 0
> >   ;
> > // </smpl>
> >
> > Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr>
> >
> > ---
> > Alternatively, is an error code wanted under the if?
> >
> >  drivers/net/wimax/i2400m/driver.c |    7 ++-----
> >  1 file changed, 2 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/net/wimax/i2400m/driver.c b/drivers/net/wimax/i2400m/driver.c
> > index 9c34d2f..901a534 100644
> > --- a/drivers/net/wimax/i2400m/driver.c
> > +++ b/drivers/net/wimax/i2400m/driver.c
> > @@ -500,26 +500,23 @@ int i2400m_pm_notifier(struct notifier_block *notifier,
> >   */
> >  int i2400m_pre_reset(struct i2400m *i2400m)
> >  {
> > -	int result;
> >  	struct device *dev = i2400m_dev(i2400m);
> >
> >  	d_fnstart(3, dev, "(i2400m %p)\n", i2400m);
> >  	d_printf(1, dev, "pre-reset shut down\n");
> >
> > -	result = 0;
> >  	mutex_lock(&i2400m->init_mutex);
> >  	if (i2400m->updown) {
> >  		netif_tx_disable(i2400m->wimax_dev.net_dev);
> >  		__i2400m_dev_stop(i2400m);
> > -		result = 0;
> >  		/* down't set updown to zero -- this way
> >  		 * post_reset can restore properly */
> >  	}
> >  	mutex_unlock(&i2400m->init_mutex);
> >  	if (i2400m->bus_release)
> >  		i2400m->bus_release(i2400m);
> > -	d_fnend(3, dev, "(i2400m %p) = %d\n", i2400m, result);
> > -	return result;
> > +	d_fnend(3, dev, "(i2400m %p) = %d\n", i2400m, 0);
>
>
> 	d_fnend(3, dev, "i2400m=%p\n", i2400m);

All the other logging messages in this file seem to have the form
(i2400m %p) = %d
or (i2400m %p) = void in the case of a function that has no return value.
I don't know if a return value is wanted here, but since the function is
exported, perhaps it is best not to change its type.

julia

> or something like that
> re,
>  wh
>
> > +	return 0;
> >  }
> >  EXPORT_SYMBOL_GPL(i2400m_pre_reset);
> >
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> >
>

Re: [PATCH 1/13] libertas: make return of 0 explicit

[Sergei Shtylyov]

Hello.

On 19-05-2014 8:31, Julia Lawall wrote:

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

> Delete unnecessary local variable whose value is always 0 and that hides
> the fact that the result is always 0.

> A simplified version of the semantic patch that fixes this problem is as
> follows: (http://coccinelle.lip6.fr/)

> // <smpl>
> @r exists@
> local idexpression ret;
> expression e;
> position p;
> @@
>
> -ret = 0;
> ... when != ret = e
> return
> - ret
> + 0
>    ;
> // </smpl>

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

> ---
> Alternatively, was an error code intended in the bad length case, as is
> done in process_brxed_802_11_packet?

>   drivers/net/wireless/libertas/rx.c |    7 ++-----
>   1 file changed, 2 insertions(+), 5 deletions(-)

> diff --git a/drivers/net/wireless/libertas/rx.c b/drivers/net/wireless/libertas/rx.c
> index c7366b0..807c5b8 100644
> --- a/drivers/net/wireless/libertas/rx.c
> +++ b/drivers/net/wireless/libertas/rx.c
[...]
> @@ -154,10 +152,9 @@ int lbs_process_rxed_packet(struct lbs_private *priv, struct sk_buff *skb)
>   	else
>   		netif_rx_ni(skb);
>
> -	ret = 0;
>   done:
> -	lbs_deb_leave_args(LBS_DEB_RX, "ret %d", ret);
> -	return ret;
> +	lbs_deb_leave_args(LBS_DEB_RX, "ret %d", 0);

    Why not just "ret 0"?

> +	return 0;

WBR, Sergei

Re: [PATCH 4/13] wimax/i2400m: make return of 0 explicit

[Sergei Shtylyov]

On 19-05-2014 8:31, Julia Lawall wrote:

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

> Delete unnecessary local variable whose value is always 0 and that hides
> the fact that the result is always 0.

> A simplified version of the semantic patch that fixes this problem is as
> follows: (http://coccinelle.lip6.fr/)

> // <smpl>
> @r exists@
> local idexpression ret;
> expression e;
> position p;
> @@
>
> -ret = 0;
> ... when != ret = e
> return
> - ret
> + 0
>    ;
> // </smpl>

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

> ---
> Alternatively, is an error code wanted under the if?

>   drivers/net/wimax/i2400m/driver.c |    7 ++-----
>   1 file changed, 2 insertions(+), 5 deletions(-)

> diff --git a/drivers/net/wimax/i2400m/driver.c b/drivers/net/wimax/i2400m/driver.c
> index 9c34d2f..901a534 100644
> --- a/drivers/net/wimax/i2400m/driver.c
> +++ b/drivers/net/wimax/i2400m/driver.c
> @@ -500,26 +500,23 @@ int i2400m_pm_notifier(struct notifier_block *notifier,
>    */
>   int i2400m_pre_reset(struct i2400m *i2400m)
>   {
> -	int result;
>   	struct device *dev = i2400m_dev(i2400m);
>
>   	d_fnstart(3, dev, "(i2400m %p)\n", i2400m);
>   	d_printf(1, dev, "pre-reset shut down\n");
>
> -	result = 0;
>   	mutex_lock(&i2400m->init_mutex);
>   	if (i2400m->updown) {
>   		netif_tx_disable(i2400m->wimax_dev.net_dev);
>   		__i2400m_dev_stop(i2400m);
> -		result = 0;
>   		/* down't set updown to zero -- this way
>   		 * post_reset can restore properly */
>   	}
>   	mutex_unlock(&i2400m->init_mutex);
>   	if (i2400m->bus_release)
>   		i2400m->bus_release(i2400m);
> -	d_fnend(3, dev, "(i2400m %p) = %d\n", i2400m, result);
> -	return result;
> +	d_fnend(3, dev, "(i2400m %p) = %d\n", i2400m, 0);

    Again, why not just "(i2400m %p) = 0\n"?

> +	return 0;

WBR, Sergei

Re: [PATCH 4/13] wimax/i2400m: make return of 0 explicit

[walter harms]

Am 19.05.2014 13:30, schrieb Julia Lawall:
> 
> 
> On Mon, 19 May 2014, walter harms wrote:
> 
>>
>>
>> Am 19.05.2014 06:31, schrieb Julia Lawall:
>>> From: Julia Lawall <Julia.Lawall@lip6.fr>
>>>
>>> Delete unnecessary local variable whose value is always 0 and that hides
>>> the fact that the result is always 0.
>>>
>>> A simplified version of the semantic patch that fixes this problem is as
>>> follows: (http://coccinelle.lip6.fr/)
>>>
>>> // <smpl>
>>> @r exists@
>>> local idexpression ret;
>>> expression e;
>>> position p;
>>> @@
>>>
>>> -ret = 0;
>>> ... when != ret = e
>>> return
>>> - ret
>>> + 0
>>>   ;
>>> // </smpl>
>>>
>>> Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr>
>>>
>>> ---
>>> Alternatively, is an error code wanted under the if?
>>>
>>>  drivers/net/wimax/i2400m/driver.c |    7 ++-----
>>>  1 file changed, 2 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/drivers/net/wimax/i2400m/driver.c b/drivers/net/wimax/i2400m/driver.c
>>> index 9c34d2f..901a534 100644
>>> --- a/drivers/net/wimax/i2400m/driver.c
>>> +++ b/drivers/net/wimax/i2400m/driver.c
>>> @@ -500,26 +500,23 @@ int i2400m_pm_notifier(struct notifier_block *notifier,
>>>   */
>>>  int i2400m_pre_reset(struct i2400m *i2400m)
>>>  {
>>> -	int result;
>>>  	struct device *dev = i2400m_dev(i2400m);
>>>
>>>  	d_fnstart(3, dev, "(i2400m %p)\n", i2400m);
>>>  	d_printf(1, dev, "pre-reset shut down\n");
>>>
>>> -	result = 0;
>>>  	mutex_lock(&i2400m->init_mutex);
>>>  	if (i2400m->updown) {
>>>  		netif_tx_disable(i2400m->wimax_dev.net_dev);
>>>  		__i2400m_dev_stop(i2400m);
>>> -		result = 0;
>>>  		/* down't set updown to zero -- this way
>>>  		 * post_reset can restore properly */
>>>  	}
>>>  	mutex_unlock(&i2400m->init_mutex);
>>>  	if (i2400m->bus_release)
>>>  		i2400m->bus_release(i2400m);
>>> -	d_fnend(3, dev, "(i2400m %p) = %d\n", i2400m, result);
>>> -	return result;
>>> +	d_fnend(3, dev, "(i2400m %p) = %d\n", i2400m, 0);
>>
>>
>> 	d_fnend(3, dev, "i2400m=%p\n", i2400m);
> 
> All the other logging messages in this file seem to have the form
> (i2400m %p) = %d
> or (i2400m %p) = void in the case of a function that has no return value.
> I don't know if a return value is wanted here, but since the function is
> exported, perhaps it is best not to change its type.


I got my "inspiration" from d_fnstart().
As i see it the whole function is void so printing a debug msg with a bogus
return value may confuse some people.

re,
 wh


> julia
> 
>> or something like that
>> re,
>>  wh
>>
>>> +	return 0;
>>>  }
>>>  EXPORT_SYMBOL_GPL(i2400m_pre_reset);
>>>
>>>
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
>>> the body of a message to majordomo@vger.kernel.org
>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>>
>>
> 

Re: [PATCH 1/13] libertas: make return of 0 explicit

[Dan Williams]

On Mon, 2014-05-19 at 16:45 +0400, Sergei Shtylyov wrote:
> Hello.
> 
> On 19-05-2014 8:31, Julia Lawall wrote:
> 
> > From: Julia Lawall <Julia.Lawall@lip6.fr>
> 
> > Delete unnecessary local variable whose value is always 0 and that hides
> > the fact that the result is always 0.
> 
> > A simplified version of the semantic patch that fixes this problem is as
> > follows: (http://coccinelle.lip6.fr/)
> 
> > // <smpl>
> > @r exists@
> > local idexpression ret;
> > expression e;
> > position p;
> > @@
> >
> > -ret = 0;
> > ... when != ret = e
> > return
> > - ret
> > + 0
> >    ;
> > // </smpl>
> 
> > Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr>
> 
> > ---
> > Alternatively, was an error code intended in the bad length case, as is
> > done in process_brxed_802_11_packet?
> 
> >   drivers/net/wireless/libertas/rx.c |    7 ++-----
> >   1 file changed, 2 insertions(+), 5 deletions(-)
> 
> > diff --git a/drivers/net/wireless/libertas/rx.c b/drivers/net/wireless/libertas/rx.c
> > index c7366b0..807c5b8 100644
> > --- a/drivers/net/wireless/libertas/rx.c
> > +++ b/drivers/net/wireless/libertas/rx.c
> [...]
> > @@ -154,10 +152,9 @@ int lbs_process_rxed_packet(struct lbs_private *priv, struct sk_buff *skb)
> >   	else
> >   		netif_rx_ni(skb);
> >
> > -	ret = 0;
> >   done:
> > -	lbs_deb_leave_args(LBS_DEB_RX, "ret %d", ret);
> > -	return ret;
> > +	lbs_deb_leave_args(LBS_DEB_RX, "ret %d", 0);
> 
>     Why not just "ret 0"?

I think instead we want:

diff --git a/drivers/net/wireless/libertas/rx.c b/drivers/net/wireless/libertas/rx.c
index c7366b0..e446fed 100644
--- a/drivers/net/wireless/libertas/rx.c
+++ b/drivers/net/wireless/libertas/rx.c
@@ -67,30 +67,32 @@ int lbs_process_rxed_packet(struct lbs_private *priv, struct sk_buff *skb)
 
 	lbs_deb_enter(LBS_DEB_RX);
 
 	BUG_ON(!skb);
 
 	skb->ip_summed = CHECKSUM_NONE;
 
-	if (priv->wdev->iftype == NL80211_IFTYPE_MONITOR)
-		return process_rxed_802_11_packet(priv, skb);
+	if (priv->wdev->iftype == NL80211_IFTYPE_MONITOR) {
+		ret = process_rxed_802_11_packet(priv, skb);
+		goto done;
+	}
 
 	p_rx_pd = (struct rxpd *) skb->data;
 	p_rx_pkt = (struct rxpackethdr *) ((u8 *)p_rx_pd +
 		le32_to_cpu(p_rx_pd->pkt_ptr));
 
 	dev = lbs_mesh_set_dev(priv, dev, p_rx_pd);
 
 	lbs_deb_hex(LBS_DEB_RX, "RX Data: Before chop rxpd", skb->data,
 		 min_t(unsigned int, skb->len, 100));
 
 	if (skb->len < (ETH_HLEN + 8 + sizeof(struct rxpd))) {
 		lbs_deb_rx("rx err: frame received with bad length\n");
 		dev->stats.rx_length_errors++;
-		ret = 0;
+		ret = -EINVAL;
 		dev_kfree_skb(skb);
 		goto done;
 	}
 
 	lbs_deb_rx("rx data: skb->len - pkt_ptr = %d-%zd = %zd\n",
 		skb->len, (size_t)le32_to_cpu(p_rx_pd->pkt_ptr),
 		skb->len - (size_t)le32_to_cpu(p_rx_pd->pkt_ptr));

Since that (a) preserves the enter/leave balance when monitor mode is
enabled, and (b) fixes the return code that has been 0 since the driver
was added to the tree in early 2007.  Dave Woodhouse fixed the
process_rxed_802_11_packet() return code for EINVAL case in commit
7bf02c29 from late 2007, and I think the normal codepath should do
EINVAL too.  (though it turns out nothing cares about the return code
anyway, we should still fix it)

Dan

Re: [PATCH 1/13] libertas: make return of 0 explicit

[Julia Lawall]

On Mon, 19 May 2014, Sergei Shtylyov wrote:

> Hello.
>
> On 19-05-2014 8:31, Julia Lawall wrote:
>
> > From: Julia Lawall <Julia.Lawall@lip6.fr>
>
> > Delete unnecessary local variable whose value is always 0 and that hides
> > the fact that the result is always 0.
>
> > A simplified version of the semantic patch that fixes this problem is as
> > follows: (http://coccinelle.lip6.fr/)
>
> > // <smpl>
> > @r exists@
> > local idexpression ret;
> > expression e;
> > position p;
> > @@
> >
> > -ret = 0;
> > ... when != ret = e
> > return
> > - ret
> > + 0
> >    ;
> > // </smpl>
>
> > Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr>
>
> > ---
> > Alternatively, was an error code intended in the bad length case, as is
> > done in process_brxed_802_11_packet?
>
> >   drivers/net/wireless/libertas/rx.c |    7 ++-----
> >   1 file changed, 2 insertions(+), 5 deletions(-)
>
> > diff --git a/drivers/net/wireless/libertas/rx.c
> > b/drivers/net/wireless/libertas/rx.c
> > index c7366b0..807c5b8 100644
> > --- a/drivers/net/wireless/libertas/rx.c
> > +++ b/drivers/net/wireless/libertas/rx.c
> [...]
> > @@ -154,10 +152,9 @@ int lbs_process_rxed_packet(struct lbs_private *priv,
> > struct sk_buff *skb)
> >   	else
> >   		netif_rx_ni(skb);
> >
> > -	ret = 0;
> >   done:
> > -	lbs_deb_leave_args(LBS_DEB_RX, "ret %d", ret);
> > -	return ret;
> > +	lbs_deb_leave_args(LBS_DEB_RX, "ret %d", 0);
>
>    Why not just "ret 0"?

OK, I wasn't sure if it was useful to keep the same number of arguments.
But I will update it, since from the definition of lbs_deb_leave_args it
seems ok.

julia

Re: [PATCH 1/13] libertas: make return of 0 explicit

[Julia Lawall]

On Mon, 19 May 2014, Sergei Shtylyov wrote:

> Hello.
>
> On 19-05-2014 8:31, Julia Lawall wrote:
>
> > From: Julia Lawall <Julia.Lawall@lip6.fr>
>
> > Delete unnecessary local variable whose value is always 0 and that hides
> > the fact that the result is always 0.
>
> > A simplified version of the semantic patch that fixes this problem is as
> > follows: (http://coccinelle.lip6.fr/)
>
> > // <smpl>
> > @r exists@
> > local idexpression ret;
> > expression e;
> > position p;
> > @@
> >
> > -ret = 0;
> > ... when != ret = e
> > return
> > - ret
> > + 0
> >    ;
> > // </smpl>
>
> > Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr>
>
> > ---
> > Alternatively, was an error code intended in the bad length case, as is
> > done in process_brxed_802_11_packet?
>
> >   drivers/net/wireless/libertas/rx.c |    7 ++-----
> >   1 file changed, 2 insertions(+), 5 deletions(-)
>
> > diff --git a/drivers/net/wireless/libertas/rx.c
> > b/drivers/net/wireless/libertas/rx.c
> > index c7366b0..807c5b8 100644
> > --- a/drivers/net/wireless/libertas/rx.c
> > +++ b/drivers/net/wireless/libertas/rx.c
> [...]
> > @@ -154,10 +152,9 @@ int lbs_process_rxed_packet(struct lbs_private *priv,
> > struct sk_buff *skb)
> >   	else
> >   		netif_rx_ni(skb);
> >
> > -	ret = 0;
> >   done:
> > -	lbs_deb_leave_args(LBS_DEB_RX, "ret %d", ret);
> > -	return ret;
> > +	lbs_deb_leave_args(LBS_DEB_RX, "ret %d", 0);
>
>    Why not just "ret 0"?

Oops, I won't make any change here, because Dan Williams has proposed
another patch that makes the ret variable useful.

julia

[PATCH 4/13 v2] wimax/i2400m: make return of 0 explicit

[Julia Lawall]

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

Delete unnecessary local variable whose value is always 0 and that hides
the fact that the result is always 0.

A simplified version of the semantic patch that fixes this problem is as
follows: (http://coccinelle.lip6.fr/)

// <smpl>
@r exists@
local idexpression ret;
expression e;
position p;
@@

-ret = 0;
... when != ret = e
return
- ret
+ 0
  ;
// </smpl>

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

---
Alternatively, is an error code wanted under the if?

v2: remove the use of 0 as an argument in the call to d_fnend

 drivers/net/wimax/i2400m/driver.c |    7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/drivers/net/wimax/i2400m/driver.c b/drivers/net/wimax/i2400m/driver.c
index 9c34d2f..9c78090 100644
--- a/drivers/net/wimax/i2400m/driver.c
+++ b/drivers/net/wimax/i2400m/driver.c
@@ -500,26 +500,23 @@ int i2400m_pm_notifier(struct notifier_block *notifier,
  */
 int i2400m_pre_reset(struct i2400m *i2400m)
 {
-	int result;
 	struct device *dev = i2400m_dev(i2400m);

 	d_fnstart(3, dev, "(i2400m %p)\n", i2400m);
 	d_printf(1, dev, "pre-reset shut down\n");

-	result = 0;
 	mutex_lock(&i2400m->init_mutex);
 	if (i2400m->updown) {
 		netif_tx_disable(i2400m->wimax_dev.net_dev);
 		__i2400m_dev_stop(i2400m);
-		result = 0;
 		/* down't set updown to zero -- this way
 		 * post_reset can restore properly */
 	}
 	mutex_unlock(&i2400m->init_mutex);
 	if (i2400m->bus_release)
 		i2400m->bus_release(i2400m);
-	d_fnend(3, dev, "(i2400m %p) = %d\n", i2400m, result);
-	return result;
+	d_fnend(3, dev, "(i2400m %p) = 0\n", i2400m);
+	return 0;
 }
 EXPORT_SYMBOL_GPL(i2400m_pre_reset);

Re: [PATCH 4/13 v2] wimax/i2400m: make return of 0 explicit

[David Miller]

From: Julia Lawall <julia.lawall@lip6.fr>
Date: Tue, 20 May 2014 08:44:49 +0800 (SGT)

> From: Julia Lawall <Julia.Lawall@lip6.fr>
> 
> Delete unnecessary local variable whose value is always 0 and that hides
> the fact that the result is always 0.
> 
> A simplified version of the semantic patch that fixes this problem is as
> follows: (http://coccinelle.lip6.fr/)
 ...
> Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr>
> 
> ---
> Alternatively, is an error code wanted under the if?
> 
> v2: remove the use of 0 as an argument in the call to d_fnend

Applied to net-next, thanks.

[PATCH] libertas: fix return value when processing invalid packet

[Dan Williams]

Nothing actually uses the return value yet, but we might as well
make it correct, like process_rxed_802_11_packet() does for the
same case.  Also ensure that if monitor mode is enabled (and
thus process_rxed_802_11_packet() is called) that the debugging
enter/leave functions are balanced.

Signed-off-by: Dan Williams <dcbw@redhat.com>
---
 drivers/net/wireless/libertas/Makefile | 4 ++--
 drivers/net/wireless/libertas/rx.c     | 8 +++++---
 2 files changed, 7 insertions(+), 5 deletions(-)

diff --git a/drivers/net/wireless/libertas/rx.c b/drivers/net/wireless/libertas/rx.c
index c7366b0..e446fed 100644
--- a/drivers/net/wireless/libertas/rx.c
+++ b/drivers/net/wireless/libertas/rx.c
@@ -67,30 +67,32 @@ int lbs_process_rxed_packet(struct lbs_private *priv, struct sk_buff *skb)
 
 	lbs_deb_enter(LBS_DEB_RX);
 
 	BUG_ON(!skb);
 
 	skb->ip_summed = CHECKSUM_NONE;
 
-	if (priv->wdev->iftype == NL80211_IFTYPE_MONITOR)
-		return process_rxed_802_11_packet(priv, skb);
+	if (priv->wdev->iftype == NL80211_IFTYPE_MONITOR) {
+		ret = process_rxed_802_11_packet(priv, skb);
+		goto done;
+	}
 
 	p_rx_pd = (struct rxpd *) skb->data;
 	p_rx_pkt = (struct rxpackethdr *) ((u8 *)p_rx_pd +
 		le32_to_cpu(p_rx_pd->pkt_ptr));
 
 	dev = lbs_mesh_set_dev(priv, dev, p_rx_pd);
 
 	lbs_deb_hex(LBS_DEB_RX, "RX Data: Before chop rxpd", skb->data,
 		 min_t(unsigned int, skb->len, 100));
 
 	if (skb->len < (ETH_HLEN + 8 + sizeof(struct rxpd))) {
 		lbs_deb_rx("rx err: frame received with bad length\n");
 		dev->stats.rx_length_errors++;
-		ret = 0;
+		ret = -EINVAL;
 		dev_kfree_skb(skb);
 		goto done;
 	}
 
 	lbs_deb_rx("rx data: skb->len - pkt_ptr = %d-%zd = %zd\n",
 		skb->len, (size_t)le32_to_cpu(p_rx_pd->pkt_ptr),
 		skb->len - (size_t)le32_to_cpu(p_rx_pd->pkt_ptr));
-- 
1.9.0

Re: [PATCH] libertas: fix return value when processing invalid packet

[James Cameron]

On Thu, May 22, 2014 at 07:32:41AM -0500, Dan Williams wrote:
> Nothing actually uses the return value yet, but we might as well
> make it correct, like process_rxed_802_11_packet() does for the
> same case.  Also ensure that if monitor mode is enabled (and
> thus process_rxed_802_11_packet() is called) that the debugging
> enter/leave functions are balanced.
> 
> Signed-off-by: Dan Williams <dcbw@redhat.com>

Reviewed-by: James Cameron <quozl@laptop.org>

-- 
James Cameron
http://quozl.linux.org.au/