make return of 0 explicit
Subsystems (13)
cover make return of 0 explicit
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 != &retreturn 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
From: Julia Lawall <[email protected]>
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>
---
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(-)
drivers/net/wireless/libertas/rx.c +2 -5
@@ -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);
Re: [PATCH 1/13] libertas: make return of 0 explicit
Am 19.05.2014 06:31, schrieb Julia Lawall:
Quoted 53 lines
From: Julia Lawall <[email protected]>
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 <[email protected]>---
Alternatively, was an error code intended in the bad length case, as is done inprocess_brxed_802_11_packet?drivers/net/wireless/libertas/rx.c | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-)drivers/net/wireless/libertas/rx.c +0
-2@@ -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; }
drivers/net/wireless/libertas/rx.c int lbs_process_rxed_packet(struct lbs_private *priv, struct sk_buff *skb) · line 158
Quoted 10 lines
drivers/net/wireless/libertas/rx.c +2
-3@@ -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
Quoted 8 lines
drivers/net/wireless/libertas/rx.c +0
-1EXPORT_SYMBOL_GPL (lbs_process_rxed_packet); --To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/13] libertas: make return of 0 explicit
In reply to Julia LawallHello.
Quoted 36 lines · Julia Lawall
From: Julia Lawall <[email protected]>
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 <[email protected]>---
Alternatively, was an error code intended in the bad length case, as is done inprocess_brxed_802_11_packet?drivers/net/wireless/libertas/rx.c | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-)drivers/net/wireless/libertas/rx.c +0
-0
[...]
drivers/net/wireless/libertas/rx.c int lbs_process_rxed_packet(struct lbs_private *priv, struct sk_buff *skb) · line 156
Quoted 9 lines
drivers/net/wireless/libertas/rx.c +1
-3@@ -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"?
drivers/net/wireless/libertas/rx.c int lbs_process_rxed_packet(struct lbs_private *priv, struct sk_buff *skb) · line 157
Quoted 1 line
drivers/net/wireless/libertas/rx.c +1
-0+return 0 ;
WBR, Sergei
Re: [PATCH 1/13] libertas: make return of 0 explicit
Quoted 45 lines · Sergei Shtylyov
Hello.
Quoted 36 lines · Julia Lawall
From: Julia Lawall <[email protected]>
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 <[email protected]>---
Alternatively, was an error code intended in the bad length case, as is done inprocess_brxed_802_11_packet?drivers/net/wireless/libertas/rx.c | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-)drivers/net/wireless/libertas/rx.c +0
-0[...]
Quoted 4 lines
drivers/net/wireless/libertas/rx.c +0
-0@@ -154,10 +152,9 @@ int lbs_process_rxed_packet(struct lbs_private *priv, struct sk_buff *skb)else netif_rx_ni (skb);
Quoted 7 lines
drivers/net/wireless/libertas/rx.c int lbs_process_rxed_packet(struct lbs_private *priv, struct sk_buff *skb) · line 156
Quoted 5 lines
drivers/net/wireless/libertas/rx.c +1
-3- 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:
drivers/net/wireless/libertas/rx.c +5 -3
@@ -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
PATCH 1/1 libertas: fix return value when processing invalid packet
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.
--- drivers/net/wireless/libertas/Makefile | 4 ++-- drivers/net/wireless/libertas/rx.c | 8 +++++--- 2 files changed, 7 insertions(+), 5 deletions(-)
drivers/net/wireless/libertas/rx.c +5 -3
@@ -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));
Signature · 1 line
1.9.0
Re: [PATCH] libertas: fix return value when processing invalid packet
Quoted 7 lines · 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 thusprocess_rxed_802_11_packet()is called) that the debugging enter/leave functions are balanced.Signed-off-by Dan Williams <[email protected]>
Signature · 2 lines
James Cameron http://quozl.linux.org.au/
Re: [PATCH 1/13] libertas: make return of 0 explicit
In reply to Sergei ShtylyovQuoted 47 lines · Sergei Shtylyov
Hello.
Quoted 37 lines · Julia Lawall
From: Julia Lawall <[email protected]>
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 <[email protected]>---
Alternatively, was an error code intended in the bad length case, as is done inprocess_brxed_802_11_packet?drivers/net/wireless/libertas/rx.c | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-)drivers/net/wireless/libertas/rx.c +0
-0[...]
Quoted 5 lines
drivers/net/wireless/libertas/rx.c +0
-0@@ -154,10 +152,9 @@ int lbs_process_rxed_packet(struct lbs_private *priv, struct sk_buff *skb) elsenetif_rx_ni (skb );
Quoted 7 lines
drivers/net/wireless/libertas/rx.c int lbs_process_rxed_packet(struct lbs_private *priv, struct sk_buff *skb) · line 156
Quoted 5 lines
drivers/net/wireless/libertas/rx.c +1
-3- 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
In reply to Sergei ShtylyovQuoted 47 lines · Sergei Shtylyov
Hello.
Quoted 37 lines · Julia Lawall
From: Julia Lawall <[email protected]>
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 <[email protected]>---
Alternatively, was an error code intended in the bad length case, as is done inprocess_brxed_802_11_packet?drivers/net/wireless/libertas/rx.c | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-)drivers/net/wireless/libertas/rx.c +0
-0[...]
Quoted 5 lines
drivers/net/wireless/libertas/rx.c +0
-0@@ -154,10 +152,9 @@ int lbs_process_rxed_packet(struct lbs_private *priv, struct sk_buff *skb) elsenetif_rx_ni (skb );
Quoted 7 lines
drivers/net/wireless/libertas/rx.c int lbs_process_rxed_packet(struct lbs_private *priv, struct sk_buff *skb) · line 156
Quoted 5 lines
drivers/net/wireless/libertas/rx.c +1
-3- 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 3/13 CLK: TI: DRA7: make return of 0 explicit
In reply to Julia LawallFrom: Julia Lawall <[email protected]>
Delete unnecessary local variable whose value is always 0 and that hides the fact that the result is always 0.
Additionally dropped some unneeded braces in an affected if.
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>
---
Alternatively, is an error code wanted in either of the MAX_APLL_WAIT_TRIES cases?
drivers/clk/ti/apll.c | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-)
drivers/clk/ti/apll.c +5 -8
@@ -37,7 +37,7 @@static int dra7_apll_enable (struct clk_hw *hw) {struct clk_hw_omap *clk =to_clk_hw_omap (hw); -int r =0 , i =0 ; +int i =0 ;struct dpll_data *ad;const char *clk_name;u8 state =1 ;
@@ -55,7 +55,7 @@ static int dra7_apll_enable(struct clk_hw *hw) v = ti_clk_ll_ops->clk_readl (ad->idlest_reg);if ((v & ad->idlest_mask) = state) -return r; +return 0 ; v = ti_clk_ll_ops->clk_readl (ad->control_reg); v &= ~ad->enable_mask;
@@ -74,17 +74,14 @@ static int dra7_apll_enable(struct clk_hw *hw)udelay (1 ); } -if (i =MAX_APLL_WAIT_TRIES ) { +if (i =MAX_APLL_WAIT_TRIES )pr_warn ("clock: %s failed transition to '%s'\n" , clk_name, (state) ?"locked" :"bypassed" ); - }else { +else pr_debug ("clock: %s transition to '%s' in %d loops\n" , clk_name, (state) ?"locked" :"bypassed" , i); - r =0 ; - } - -return r; +return 0 ; }static void dra7_apll_disable (struct clk_hw *hw)
Re: [PATCH 3/13] CLK: TI: DRA7: make return of 0 explicit
This one does feel like a bug in the original code as you mention. I have added the TI devs to the CC list so they can help us.
regards,
dan carpenter
Quoted 82 lines · Julia Lawall
From: Julia Lawall <[email protected]>
Delete unnecessary local variable whose value is always 0 and that hides the fact that the result is always 0.
Additionally dropped some unneeded braces in an affected if.
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 <[email protected]>---
Alternatively, is an error code wanted in either of theMAX_APLL_WAIT_TRIEScases?drivers/clk/ti/apll.c | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-)drivers/clk/ti/apll.c +5
-9@@ -37,7 +37,7 @@static int dra7_apll_enable (struct clk_hw *hw) {struct clk_hw_omap *clk =to_clk_hw_omap (hw); -int r =0 , i =0 ; +int i =0 ;struct dpll_data *ad;const char *clk_name;u8 state =1 ;@@ -55,7 +55,7 @@ static int dra7_apll_enable(struct clk_hw *hw) v = ti_clk_ll_ops->clk_readl (ad->idlest_reg);if ((v & ad->idlest_mask) = state) -return r; +return 0 ; v = ti_clk_ll_ops->clk_readl (ad->control_reg); v &= ~ad->enable_mask;@@ -74,17 +74,14 @@ static int dra7_apll_enable(struct clk_hw *hw)udelay (1 ); } -if (i =MAX_APLL_WAIT_TRIES ) { +if (i =MAX_APLL_WAIT_TRIES )pr_warn ("clock: %s failed transition to '%s'\n" , clk_name, (state) ?"locked" :"bypassed" ); - }else { +else pr_debug ("clock: %s transition to '%s' in %d loops\n" , clk_name, (state) ?"locked" :"bypassed" , i); - r =0 ; - } - -return r; +return 0 ; }static void dra7_apll_disable (struct clk_hw *hw) --To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/13] CLK: TI: DRA7: make return of 0 explicit
Quoted 2 lines · Dan Carpenter
This one does feel like a bug in the original code as you mention. I have added the TI devs to the CC list so they can help us.
Yes this is a bug, the dra7_apll_enable() should return some sort of error code if the lock fails. EBUSY maybe?
-Tero
Quoted 87 lines
regards,
dan carpenterQuoted 82 lines · Julia Lawall
From: Julia Lawall <[email protected]>
Delete unnecessary local variable whose value is always 0 and that hides the fact that the result is always 0.
Additionally dropped some unneeded braces in an affected if.
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 <[email protected]>---
Alternatively, is an error code wanted in either of theMAX_APLL_WAIT_TRIEScases?drivers/clk/ti/apll.c | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-)drivers/clk/ti/apll.c +5
-9@@ -37,7 +37,7 @@static int dra7_apll_enable (struct clk_hw *hw) {struct clk_hw_omap *clk =to_clk_hw_omap (hw); -int r =0 , i =0 ; +int i =0 ;struct dpll_data *ad;const char *clk_name;u8 state =1 ;@@ -55,7 +55,7 @@ static int dra7_apll_enable(struct clk_hw *hw) v = ti_clk_ll_ops->clk_readl (ad->idlest_reg);if ((v & ad->idlest_mask) == state) -return r; +return 0 ; v = ti_clk_ll_ops->clk_readl (ad->control_reg); v &= ~ad->enable_mask;@@ -74,17 +74,14 @@ static int dra7_apll_enable(struct clk_hw *hw)udelay (1 ); } -if (i ==MAX_APLL_WAIT_TRIES ) { +if (i ==MAX_APLL_WAIT_TRIES )pr_warn ("clock: %s failed transition to '%s'\n" , clk_name, (state) ?"locked" :"bypassed" ); - }else { +else pr_debug ("clock: %s transition to '%s' in %d loops\n" , clk_name, (state) ?"locked" :"bypassed" , i); - r =0 ; - } - -return r; +return 0 ; }static void dra7_apll_disable (struct clk_hw *hw) --To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
PATCH 1/1 CLK: TI: DRA7: return error code in failure case
From: Julia Lawall <[email protected]>
Add a returned error code in the MAX_APLL_WAIT_TRIES case. Remove the updating of the return variable r to 0 if MAX_APLL_WAIT_TRIES is not yet reached, because r is already 0 at this point.
---
drivers/clk/ti/apll.c +2 -4
@@ -77,13 +77,11 @@ static int dra7_apll_enable(struct clk_hw *hw)if (i =MAX_APLL_WAIT_TRIES ) {pr_warn ("clock: %s failed transition to '%s'\n" , clk_name, (state) ?"locked" :"bypassed" ); - }else { + r = -EBUSY ; + }else pr_debug ("clock: %s transition to '%s' in %d loops\n" , clk_name, (state) ?"locked" :"bypassed" , i); - r =0 ; - } -return r; }
Re: [PATCH] CLK: TI: DRA7: return error code in failure case
Quoted 7 lines · Julia Lawall
From: Julia Lawall <[email protected]>
Add a returned error code in the
MAX_APLL_WAIT_TRIEScase. Remove the updating of the return variable r to 0 ifMAX_APLL_WAIT_TRIESis not yet reached, because r is already 0 at this point.Signed-off-by Julia Lawall <[email protected]>
Mike, do you want to queue this as a fix or shall I add this to be queued for 3.16?
-Tero
Quoted 22 lines
---
drivers/clk/ti/apll.c +2
-4@@ -77,13 +77,11 @@ static int dra7_apll_enable(struct clk_hw *hw)if (i ==MAX_APLL_WAIT_TRIES ) {pr_warn ("clock: %s failed transition to '%s'\n" , clk_name, (state) ?"locked" :"bypassed" ); - }else { + r = -EBUSY ; + }else pr_debug ("clock: %s transition to '%s' in %d loops\n" , clk_name, (state) ?"locked" :"bypassed" , i); - r =0 ; - } -return r; }
Re: [PATCH] CLK: TI: DRA7: return error code in failure case
Quoting Tero Kristo (2014-05-19 05:23:10)
Quoted 7 lines · Julia Lawall
Quoted 6 lines
From: Julia Lawall <[email protected]>
Add a returned error code in the
MAX_APLL_WAIT_TRIEScase. Remove the updating of the return variable r to 0 ifMAX_APLL_WAIT_TRIESis not yet reached, because r is already 0 at this point.
Quoted 6 lines
Quoted 1 line
Signed-off-by Julia Lawall <[email protected]>Acked-by Tero Kristo <[email protected]>Mike, do you want to queue this as a fix or shall I add this to be queued for 3.16?
It's not a visible regression, so let's go with 3.16.
Regards,
Mike
Quoted 26 lines
-Tero
Quoted 22 lines
---
drivers/clk/ti/apll.c +2
-4@@ -77,13 +77,11 @@ static int dra7_apll_enable(struct clk_hw *hw)if (i =MAX_APLL_WAIT_TRIES ) {pr_warn ("clock: %s failed transition to '%s'\n" , clk_name, (state) ?"locked" :"bypassed" ); - }else { + r = -EBUSY ; + }else pr_debug ("clock: %s transition to '%s' in %d loops\n" , clk_name, (state) ?"locked" :"bypassed" , i); - r =0 ; - } -return r; }
Re: [PATCH] CLK: TI: DRA7: return error code in failure case
Quoted 8 lines · Mike Turquette
Quoting Tero Kristo (2014-05-19 05:23:10)
Quoted 7 lines · Julia Lawall
Quoted 6 lines
From: Julia Lawall <[email protected]>
Add a returned error code in the
MAX_APLL_WAIT_TRIEScase. Remove the updating of the return variable r to 0 ifMAX_APLL_WAIT_TRIESis not yet reached, because r is already 0 at this point.
Quoted 8 lines
Quoted 6 lines
Quoted 1 line
Signed-off-by Julia Lawall <[email protected]>Acked-by Tero Kristo <[email protected]>Mike, do you want to queue this as a fix or shall I add this to be queued for 3.16?
It's not a visible regression, so let's go with 3.16.
Thanks, queued for 3.16-rc fixes now, sorry about the latency.
-Tero
Quoted 30 lines
Regards,
MikeQuoted 26 lines
-Tero
Quoted 22 lines
---
drivers/clk/ti/apll.c +2
-4@@ -77,13 +77,11 @@ static int dra7_apll_enable(struct clk_hw *hw)if (i ==MAX_APLL_WAIT_TRIES ) {pr_warn ("clock: %s failed transition to '%s'\n" , clk_name, (state) ?"locked" :"bypassed" ); - }else { + r = -EBUSY ; + }else pr_debug ("clock: %s transition to '%s' in %d loops\n" , clk_name, (state) ?"locked" :"bypassed" , i); - r =0 ; - } -return r; }
PATCH 4/13 wimax/i2400m: make return of 0 explicit
In reply to Julia LawallFrom: Julia Lawall <[email protected]>
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>
---
Alternatively, is an error code wanted under the if?
drivers/net/wimax/i2400m/driver.c | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-)
drivers/net/wimax/i2400m/driver.c +2 -5
@@ -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);
Re: [PATCH 4/13] wimax/i2400m: make return of 0 explicit
Am 19.05.2014 06:31, schrieb Julia Lawall:
Quoted 50 lines
From: Julia Lawall <[email protected]>
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 <[email protected]>---
Alternatively, is an error code wanted under the if?drivers/net/wimax/i2400m/driver.c | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-)drivers/net/wimax/i2400m/driver.c +0
-2@@ -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);
drivers/net/wimax/i2400m/driver.c int i2400m_pm_notifier(struct notifier_block *notifier, · line 518
Quoted 10 lines
drivers/net/wimax/i2400m/driver.c +1
-3- 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
Quoted 10 lines
drivers/net/wimax/i2400m/driver.c +1
-1+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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 4/13] wimax/i2400m: make return of 0 explicit
Quoted 58 lines · walter harms
Am 19.05.2014 06:31, schrieb Julia Lawall:
Quoted 55 lines
From: Julia Lawall <[email protected]>
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 <[email protected]>---
Alternatively, is an error code wanted under the if?drivers/net/wimax/i2400m/driver.c | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-)drivers/net/wimax/i2400m/driver.c +0
-3@@ -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);
Quoted 8 lines
drivers/net/wimax/i2400m/driver.c int i2400m_pm_notifier(struct notifier_block *notifier, · line 518
Quoted 5 lines
drivers/net/wimax/i2400m/driver.c +1
-2if (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
Quoted 15 lines
or something like that
re,
whQuoted 10 lines
drivers/net/wimax/i2400m/driver.c +1
-1+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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 4/13] wimax/i2400m: make return of 0 explicit
Am 19.05.2014 13:30, schrieb Julia Lawall:
Quoted 71 lines · walter harms
Quoted 59 lines
Am 19.05.2014 06:31, schrieb Julia Lawall:
Quoted 55 lines
From: Julia Lawall <[email protected]>
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 <[email protected]>---
Alternatively, is an error code wanted under the if?drivers/net/wimax/i2400m/driver.c | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-)drivers/net/wimax/i2400m/driver.c +0
-3@@ -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);Quoted 8 lines
drivers/net/wimax/i2400m/driver.c int i2400m_pm_notifier(struct notifier_block *notifier, · line 518
Quoted 5 lines
drivers/net/wimax/i2400m/driver.c +1
-2if (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);
Quoted 5 lines
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
Quoted 18 lines
julia
Quoted 15 lines
or something like that
re,
whQuoted 10 lines
drivers/net/wimax/i2400m/driver.c +1
-1+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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 4/13] wimax/i2400m: make return of 0 explicit
In reply to Julia LawallQuoted 50 lines · Julia Lawall
From: Julia Lawall <[email protected]>
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 <[email protected]>---
Alternatively, is an error code wanted under the if?drivers/net/wimax/i2400m/driver.c | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-)drivers/net/wimax/i2400m/driver.c +0
-2@@ -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);
drivers/net/wimax/i2400m/driver.c int i2400m_pm_notifier(struct notifier_block *notifier, · line 518
Quoted 10 lines
drivers/net/wimax/i2400m/driver.c +1
-3- 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"?
drivers/net/wimax/i2400m/driver.c int i2400m_pm_notifier(struct notifier_block *notifier, · line 519
Quoted 1 line
drivers/net/wimax/i2400m/driver.c +1
-0+return 0 ;
WBR, Sergei
PATCH v2 4/13 wimax/i2400m: make return of 0 explicit
From: Julia Lawall <[email protected]>
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>
---
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(-)
drivers/net/wimax/i2400m/driver.c +2 -5
@@ -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
From: Julia Lawall <[email protected]>
Date: Tue, 20 May 2014 08:44:49 +0800 (SGT)
Quoted 7 lines
From: Julia Lawall <[email protected]>
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/)
...
Quoted 6 lines
Signed-off-by Julia Lawall <[email protected]>---
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 5/13 usb: gadget: make return of 0 explicit
In reply to Julia LawallFrom: Julia Lawall <[email protected]>
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>
--- drivers/usb/gadget/composite.c | 6 ++---- drivers/usb/gadget/dummy_hcd.c | 4 +--- 2 files changed, 3 insertions(+), 7 deletions(-)
drivers/usb/gadget/composite.c +2 -4
@@ -1312,9 +1312,7 @@ static void fill_ext_compat(struct usb_configuration *c, u8 *buf)static int count_ext_prop (struct usb_configuration *c,int interface) {struct usb_function *f; -int j, res; - - res =0 ; +int j; f = c->interface[interface];for (j =0 ; j < f->os_desc_n; ++j) {
@@ -1326,7 +1324,7 @@ static int count_ext_prop(struct usb_configuration *c, int interface)if (d && d->ext_compat_id)return d->ext_prop_count; } -return res; +return 0 ; }static int len_ext_prop (struct usb_configuration *c,int interface)
drivers/usb/gadget/dummy_hcd.c +1 -3
@@ -561,7 +561,6 @@ static int dummy_disable(struct usb_ep *_ep)struct dummy_ep *ep;struct dummy *dum;unsigned long flags; -int retval; ep =usb_ep_to_dummy_ep (_ep);if (!_ep || !ep->desc || _ep->name = ep0name)
@@ -571,12 +570,11 @@ static int dummy_disable(struct usb_ep *_ep)spin_lock_irqsave (&dum->lock, flags); ep->desc =NULL ; ep->stream_en =0 ; - retval =0 ;nuke (dum, ep);spin_unlock_irqrestore (&dum->lock, flags);dev_dbg (udc_dev (dum),"disabled %s\n" , _ep->name); -return retval; +return 0 ; } static struct usb_request *dummy_alloc_request(struct usb_ep *_ep,
PATCH 6/13 : arp_tables: make return of 0 explicit
In reply to Julia LawallFrom: Julia Lawall <[email protected]>
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>
--- net/ipv4/netfilter/arp_tables.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)
net/ipv4/netfilter/arp_tables.c +2 -3
@@ -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,
Pablo Neira Ayuso
maintainerRe: [PATCH 6/13] [NETFILTER]: arp_tables: make return of 0 explicit
Hi Julia,
Quoted 45 lines · Julia Lawall
From: Julia Lawall <[email protected]>
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 <[email protected]>--- net/ipv4/netfilter/arp_tables.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)net/ipv4/netfilter/arp_tables.c +1
-2@@ -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 ));
net/ipv4/netfilter/arp_tables.c compat_copy_entry_from_user(struct compat_arpt_entry *e, void **dstptr, · line 1315
Quoted 6 lines
net/ipv4/netfilter/arp_tables.c +1
-1@@ -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 ; }
This looks good, but we can simplify this a bit further, given that compat_copy_entry_from_user could be made void.
This branch below never happens.
xt_entry_foreach(iter0, entry0, total_size) {
ret = compat_copy_entry_from_user(iter0, &pos, &size,
name, newinfo, entry1);
if (ret != 0)
break;
}Thanks for your patch anyway!
Re: [PATCH 6/13] [NETFILTER]: arp_tables: make return of 0 explicit
Quoted 58 lines · Pablo Neira Ayuso
Hi Julia,
Quoted 45 lines · Julia Lawall
From: Julia Lawall <[email protected]>
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 <[email protected]>--- net/ipv4/netfilter/arp_tables.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)net/ipv4/netfilter/arp_tables.c +1
-2@@ -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 ));net/ipv4/netfilter/arp_tables.c compat_copy_entry_from_user(struct compat_arpt_entry *e, void **dstptr, · line 1315
Quoted 6 lines
net/ipv4/netfilter/arp_tables.c +1
-1@@ -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 ; }This looks good, but we can simplify this a bit further, given that
compat_copy_entry_from_usercould be made void.
Quoted 8 lines
This branch below never happens.
xt_entry_foreach(iter0, entry0, total_size) { ret = compat_copy_entry_from_user(iter0, &pos, &size, name, newinfo, entry1); if (ret != 0) break; }
Are you taking care of this., or should I do something?
thanks,
julia
PATCH 7/13 staging: wlags49_h2: make return of 0 explicit
In reply to Julia LawallFrom: Julia Lawall <[email protected]>
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>
---
I'm not sure to understand this code. There seem to be two cases where ret was undefined at the point of the return (on failure of the case 0 test and in the default case).
drivers/staging/wlags49_h2/wl_wext.c | 11 ++--------- 1 file changed, 2 insertions(+), 9 deletions(-)
drivers/staging/wlags49_h2/wl_wext.c +2 -9
@@ -159,15 +159,12 @@ static int hermes_set_tkip_keys(ltv_t *ltv, u16 key_idx, u8 *addr,/* Set up the LTV to clear the appropriate key */ static int hermes_clear_tkip_keys (ltv_t *ltv,u16 key_idx,u8 *addr) { -int ret; -switch (key_idx) {case 0 :if (!is_broadcast_ether_addr (addr)) { ltv->len =7 ; ltv->typ =CFG_REMOVE_TKIP_MAPPED_KEY ;memcpy (<v->u.u8[0 ], addr,ETH_ALEN ); - ret =0 ; }break ;case 1 :
@@ -178,13 +175,12 @@ static int hermes_clear_tkip_keys(ltv_t *ltv, u16 key_idx, u8 *addr) ltv->typ =CFG_REMOVE_TKIP_DEFAULT_KEY ; ltv->u.u16[0 ] =cpu_to_le16 (key_idx); - ret =0 ;break ;default :break ; } -return ret; +return 0 ; }/* Set the WEP keys in the wl_private structure */
@@ -3027,13 +3023,10 @@ static int wireless_set_genie(struct net_device *dev, struct iw_point *data, char *extra) { - int ret = 0; -/* We can't write this to the card, but apparently this * operation needs to succeed */ - ret =0 ; -return ret; +return 0 ; }/*======================================*/
PATCH 8/13 sound: mpu401.c: make return of 0 explicit
In reply to Julia LawallFrom: Julia Lawall <[email protected]>
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>
--- sound/oss/mpu401.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-)
sound/oss/mpu401.c +1 -3
@@ -567,7 +567,6 @@ static int mpu401_out(int dev, unsigned char midi_byte)static int mpu401_command (int dev,mpu_command_rec * cmd) {int i, timeout, ok; -int ret =0 ;unsigned long flags;struct mpu_config *devc;
@@ -644,7 +643,6 @@ retry: } } } - ret =0 ; cmd->data[0 ] =0 ;if (cmd->nr_returns)
@@ -666,7 +664,7 @@ retry: } }spin_unlock_irqrestore (&devc->lock,flags); -return ret; +return 0 ; }static int mpu_cmd (int dev,int cmd,int data)
Takashi Iwai
AppliedRe: [PATCH 8/13] sound: mpu401.c: make return of 0 explicit
At Mon, 19 May 2014 06:31:10 +0200,
Quoted 24 lines · Julia Lawall
From: Julia Lawall <[email protected]>
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>
Quoted 1 line
Signed-off-by Julia Lawall <[email protected]>
Thanks, applied.
Takashi
Quoted 35 lines
--- sound/oss/mpu401.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-)sound/oss/mpu401.c +1
-3@@ -567,7 +567,6 @@ static int mpu401_out(int dev, unsigned char midi_byte)static int mpu401_command (int dev,mpu_command_rec * cmd) {int i, timeout, ok; -int ret =0 ;unsigned long flags;struct mpu_config *devc;@@ -644,7 +643,6 @@ retry: } } } - ret =0 ; cmd->data[0 ] =0 ;if (cmd->nr_returns)@@ -666,7 +664,7 @@ retry: } }spin_unlock_irqrestore (&devc->lock,flags); -return ret; +return 0 ; }static int mpu_cmd (int dev,int cmd,int data)
PATCH 9/13 md: make return of 0 explicit
In reply to Julia LawallFrom: Julia Lawall <[email protected]>
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>
---
Alternatively, was an error code intended in the MD_MAX_BADBLOCKS case?
drivers/md/md.c | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-)
drivers/md/md.c +2 -5
@@ -8297,7 +8297,6 @@ static int md_clear_badblocks(struct badblocks *bb, sector_t s, int sectors)u64 *p;int lo, hi;sector_t target = s + sectors; -int rv =0 ;if (bb->shift >0 ) {/* When clearing we round the start up and the end down.
@@ -8339,10 +8338,8 @@ static int md_clear_badblocks(struct badblocks *bb, sector_t s, int sectors)if (a < s) {/* we need to split this range */ -if (bb->count >=MD_MAX_BADBLOCKS ) { - rv =0 ; +if (bb->count >=MD_MAX_BADBLOCKS ) goto out; - }memmove (p+lo+1 , p+lo, (bb->count - lo) *8 ); bb->count++; p[lo] =BB_MAKE (a, s-a, ack);
@@ -8378,7 +8375,7 @@ static int md_clear_badblocks(struct badblocks *bb, sector_t s, int sectors) bb->changed =1 ; out:write_sequnlock_irq (&bb->lock); -return rv; +return 0 ; } int rdev_clear_badblocks(struct md_rdev *rdev, sector_t s, int sectors,
Re: [PATCH 9/13] md: make return of 0 explicit
Quoted 23 lines · On Mon, 19 May 2014 06:31:11 +0200 Julia Lawall <[email protected]> wrote:
From: Julia Lawall <[email protected]>
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>
Quoted 4 lines
Signed-off-by Julia Lawall <[email protected]>---
Alternatively, was an error code intended in theMD_MAX_BADBLOCKScase?
Yes, I did want an error code in that case - thanks! I'll make a patch.
NeilBrown
Quoted 37 lines
drivers/md/md.c | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-)drivers/md/md.c +2
-5@@ -8297,7 +8297,6 @@ static int md_clear_badblocks(struct badblocks *bb, sector_t s, int sectors)u64 *p;int lo, hi;sector_t target = s + sectors; -int rv =0 ;if (bb->shift >0 ) {/* When clearing we round the start up and the end down. @@ -8339,10 +8338,8 @@ static int md_clear_badblocks(struct badblocks *bb, sector_t s, int sectors)if (a < s) {/* we need to split this range */ -if (bb->count >=MD_MAX_BADBLOCKS ) { - rv =0 ; +if (bb->count >=MD_MAX_BADBLOCKS ) goto out; - }memmove (p+lo+1 , p+lo, (bb->count - lo) *8 ); bb->count++; p[lo] =BB_MAKE (a, s-a, ack);@@ -8378,7 +8375,7 @@ static int md_clear_badblocks(struct badblocks *bb, sector_t s, int sectors) bb->changed =1 ; out:write_sequnlock_irq (&bb->lock); -return rv; +return 0 ; } int rdev_clear_badblocks(struct md_rdev *rdev, sector_t s, int sectors,
PATCH 10/13 staging: rtl8192e: make return of 0 explicit
In reply to Julia LawallFrom: Julia Lawall <[email protected]>
Delete unnecessary use of a local variable to immediately return 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>
--- drivers/staging/rtl8192e/rtl8192e/rtl_core.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
drivers/staging/rtl8192e/rtl8192e/rtl_core.c +1 -2
@@ -1915,8 +1915,7 @@ int rtl8192_hard_start_xmit(struct sk_buff *skb, struct net_device *dev)memcpy ((unsigned char *)(skb->cb), &dev,sizeof (dev));if (queue_index =TXCMD_QUEUE ) {rtl8192_tx_cmd (dev, skb); - ret =0 ; -return ret; +return 0 ; }else { tcb_desc->RATRIndex =7 ; tcb_desc->bTxDisableRateFallBack =1 ;
PATCH 11/13 ufs: make return of 0 explicit
In reply to Julia LawallFrom: Julia Lawall <[email protected]>
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>
--- fs/ufs/truncate.c | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-)
fs/ufs/truncate.c +2 -6
@@ -72,7 +72,6 @@ static int ufs_trunc_direct(struct inode *inode)u64 frag1, frag2, frag3, frag4, block1, block2;unsigned frag_to_free, free_count;unsigned i, tmp; -int retry;UFSD ("ENTER: ino %lu\n" , inode->i_ino);
@@ -81,7 +80,6 @@ static int ufs_trunc_direct(struct inode *inode) frag_to_free =0 ; free_count =0 ; - retry =0 ; frag1 =DIRECT_FRAGMENT ; frag4 =min_t (u64,UFS_NDIR_FRAGMENT , ufsi->i_lastfrag);
@@ -164,7 +162,7 @@ next1: next3:UFSD ("EXIT: ino %lu\n" , inode->i_ino); -return retry; +return 0 ; }
@@ -176,7 +174,6 @@ static int ufs_trunc_indirect(struct inode *inode, u64 offset, void *p)void *ind;u64 tmp, indirect_block, i, frag_to_free;unsigned free_count; -int retry;UFSD ("ENTER: ino %lu, offset %llu, p: %p\n" , inode->i_ino, (unsigned long long )offset, p);
@@ -188,7 +185,6 @@ static int ufs_trunc_indirect(struct inode *inode, u64 offset, void *p) frag_to_free =0 ; free_count =0 ; - retry =0 ; tmp =ufs_data_ptr_to_cpu (sb, p);if (!tmp)
@@ -248,7 +244,7 @@ static int ufs_trunc_indirect(struct inode *inode, u64 offset, void *p)UFSD ("EXIT: ino %lu\n" , inode->i_ino); -return retry; +return 0 ; }static int ufs_trunc_dindirect (struct inode *inode,u64 offset,void *p)
PATCH 12/13 brcmsmac: make return of 0 explicit
In reply to Julia LawallFrom: Julia Lawall <[email protected]>
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>
--- drivers/net/wireless/brcm80211/brcmsmac/main.c | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-)
drivers/net/wireless/brcm80211/brcmsmac/main.c +1 -4
@@ -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 ; }
PATCH 13/13 s390/irq: make return of 0 explicit
In reply to Julia LawallFrom: Julia Lawall <[email protected]>
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>
--- arch/s390/oprofile/hwsampler.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-)
arch/s390/oprofile/hwsampler.c +2 -4
@@ -212,10 +212,8 @@ static void init_all_cpu_buffers(void)static int prepare_cpu_buffers (void ) {int cpu; -int rc;struct hws_cpu_buffer *cb; - rc =0 ;for_each_online_cpu (cpu) { cb = &per_cpu (sampler_cpu_buffer, cpu);atomic_set (&cb->ext_params,0 );
@@ -231,7 +229,7 @@ static int prepare_cpu_buffers(void) cb->stop_mode =0 ; } -return rc; +return 0 ; }/*
@@ -1156,7 +1154,7 @@ int hwsampler_stop_all(void) rc =0 ;if (hws_state =HWS_INIT ) {mutex_unlock (&hws_sem); -return rc; +return 0 ; } hws_state =HWS_STOPPING ;mutex_unlock (&hws_sem);
Re: [PATCH 13/13] s390/irq: make return of 0 explicit
Quoted 30 lines · Julia Lawall
From: Julia Lawall <[email protected]>
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 <[email protected]>--- arch/s390/oprofile/hwsampler.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-)arch/s390/oprofile/hwsampler.c +0
-2@@ -212,10 +212,8 @@ static void init_all_cpu_buffers(void)static int prepare_cpu_buffers (void ) {int cpu; -int rc;struct hws_cpu_buffer *cb; - rc =0 ;for_each_online_cpu (cpu) { cb = &per_cpu (sampler_cpu_buffer, cpu);atomic_set (&cb->ext_params,0 );
arch/s390/oprofile/hwsampler.c static int prepare_cpu_buffers(void) · line 233
Quoted 6 lines
arch/s390/oprofile/hwsampler.c +1
-1@@ -231,7 +229,7 @@ static int prepare_cpu_buffers(void) cb->stop_mode =0 ; } -return rc; +return 0 ; }
Thanks Julia,
I applied a slightly different version which also turns prepare_cpu_buffers into a void returning function.