* [PATCH 1/13] libertas: make return of 0 explicit
2014-05-19 4:31 [PATCH 0/13] make return of 0 explicit Julia Lawall
@ 2014-05-19 4:31 ` Julia Lawall
2014-05-19 7:43 ` walter harms
2014-05-19 12:45 ` Sergei Shtylyov
2014-05-19 4:31 ` [PATCH 4/13] wimax/i2400m: " Julia Lawall
` (2 subsequent siblings)
3 siblings, 2 replies; 18+ messages in thread
From: Julia Lawall @ 2014-05-19 4:31 UTC (permalink / raw)
To: John W. Linville
Cc: kernel-janitors, libertas-dev, linux-wireless, netdev,
linux-kernel
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);
^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH 1/13] libertas: make return of 0 explicit
2014-05-19 4:31 ` [PATCH 1/13] libertas: " Julia Lawall
@ 2014-05-19 7:43 ` walter harms
2014-05-19 12:45 ` Sergei Shtylyov
1 sibling, 0 replies; 18+ messages in thread
From: walter harms @ 2014-05-19 7:43 UTC (permalink / raw)
To: Julia Lawall
Cc: John W. Linville, kernel-janitors, libertas-dev, linux-wireless,
netdev, linux-kernel
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
>
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/13] libertas: make return of 0 explicit
2014-05-19 4:31 ` [PATCH 1/13] libertas: " Julia Lawall
2014-05-19 7:43 ` walter harms
@ 2014-05-19 12:45 ` Sergei Shtylyov
2014-05-19 17:25 ` Dan Williams
` (2 more replies)
1 sibling, 3 replies; 18+ messages in thread
From: Sergei Shtylyov @ 2014-05-19 12:45 UTC (permalink / raw)
To: Julia Lawall, John W. Linville
Cc: kernel-janitors, libertas-dev, linux-wireless, netdev,
linux-kernel
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
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/13] libertas: make return of 0 explicit
2014-05-19 12:45 ` Sergei Shtylyov
@ 2014-05-19 17:25 ` Dan Williams
2014-05-22 12:32 ` [PATCH] libertas: fix return value when processing invalid packet Dan Williams
2014-05-20 0:30 ` [PATCH 1/13] libertas: make return of 0 explicit Julia Lawall
2014-05-20 0:36 ` Julia Lawall
2 siblings, 1 reply; 18+ messages in thread
From: Dan Williams @ 2014-05-19 17:25 UTC (permalink / raw)
To: Sergei Shtylyov
Cc: Julia Lawall, John W. Linville, kernel-janitors, libertas-dev,
linux-wireless, netdev, linux-kernel
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
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH] libertas: fix return value when processing invalid packet
2014-05-19 17:25 ` Dan Williams
@ 2014-05-22 12:32 ` Dan Williams
2014-05-22 21:57 ` James Cameron
0 siblings, 1 reply; 18+ messages in thread
From: Dan Williams @ 2014-05-22 12:32 UTC (permalink / raw)
To: Sergei Shtylyov
Cc: kernel-janitors, libertas-dev, netdev, linux-wireless,
John W. Linville, linux-kernel, Julia Lawall
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
^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH] libertas: fix return value when processing invalid packet
2014-05-22 12:32 ` [PATCH] libertas: fix return value when processing invalid packet Dan Williams
@ 2014-05-22 21:57 ` James Cameron
0 siblings, 0 replies; 18+ messages in thread
From: James Cameron @ 2014-05-22 21:57 UTC (permalink / raw)
To: Dan Williams
Cc: Sergei Shtylyov, kernel-janitors, libertas-dev, netdev,
linux-wireless, John W. Linville, linux-kernel, Julia Lawall
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/
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/13] libertas: make return of 0 explicit
2014-05-19 12:45 ` Sergei Shtylyov
2014-05-19 17:25 ` Dan Williams
@ 2014-05-20 0:30 ` Julia Lawall
2014-05-20 0:36 ` Julia Lawall
2 siblings, 0 replies; 18+ messages in thread
From: Julia Lawall @ 2014-05-20 0:30 UTC (permalink / raw)
To: Sergei Shtylyov
Cc: John W. Linville, kernel-janitors, libertas-dev, linux-wireless,
netdev, linux-kernel
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
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/13] libertas: make return of 0 explicit
2014-05-19 12:45 ` Sergei Shtylyov
2014-05-19 17:25 ` Dan Williams
2014-05-20 0:30 ` [PATCH 1/13] libertas: make return of 0 explicit Julia Lawall
@ 2014-05-20 0:36 ` Julia Lawall
2 siblings, 0 replies; 18+ messages in thread
From: Julia Lawall @ 2014-05-20 0:36 UTC (permalink / raw)
To: Sergei Shtylyov
Cc: John W. Linville, kernel-janitors, libertas-dev, linux-wireless,
netdev, linux-kernel
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
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH 4/13] wimax/i2400m: make return of 0 explicit
2014-05-19 4:31 [PATCH 0/13] make return of 0 explicit Julia Lawall
2014-05-19 4:31 ` [PATCH 1/13] libertas: " Julia Lawall
@ 2014-05-19 4:31 ` Julia Lawall
2014-05-19 7:47 ` walter harms
2014-05-19 12:46 ` Sergei Shtylyov
2014-05-19 4:31 ` [PATCH 6/13] [NETFILTER]: arp_tables: " Julia Lawall
2014-05-19 4:31 ` [PATCH 12/13] brcmsmac: " Julia Lawall
3 siblings, 2 replies; 18+ messages in thread
From: Julia Lawall @ 2014-05-19 4:31 UTC (permalink / raw)
To: Inaky Perez-Gonzalez; +Cc: kernel-janitors, linux-wimax, netdev, linux-kernel
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);
^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH 4/13] wimax/i2400m: make return of 0 explicit
2014-05-19 4:31 ` [PATCH 4/13] wimax/i2400m: " Julia Lawall
@ 2014-05-19 7:47 ` walter harms
2014-05-19 11:30 ` Julia Lawall
2014-05-19 12:46 ` Sergei Shtylyov
1 sibling, 1 reply; 18+ messages in thread
From: walter harms @ 2014-05-19 7:47 UTC (permalink / raw)
To: Julia Lawall
Cc: Inaky Perez-Gonzalez, kernel-janitors, linux-wimax, netdev,
linux-kernel
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
>
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 4/13] wimax/i2400m: make return of 0 explicit
2014-05-19 7:47 ` walter harms
@ 2014-05-19 11:30 ` Julia Lawall
2014-05-19 13:22 ` walter harms
0 siblings, 1 reply; 18+ messages in thread
From: Julia Lawall @ 2014-05-19 11:30 UTC (permalink / raw)
To: walter harms
Cc: Inaky Perez-Gonzalez, kernel-janitors, linux-wimax, netdev,
linux-kernel
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
> >
>
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 4/13] wimax/i2400m: make return of 0 explicit
2014-05-19 11:30 ` Julia Lawall
@ 2014-05-19 13:22 ` walter harms
0 siblings, 0 replies; 18+ messages in thread
From: walter harms @ 2014-05-19 13:22 UTC (permalink / raw)
To: Julia Lawall
Cc: Inaky Perez-Gonzalez, kernel-janitors, linux-wimax, netdev,
linux-kernel
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
>>>
>>
>
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 4/13] wimax/i2400m: make return of 0 explicit
2014-05-19 4:31 ` [PATCH 4/13] wimax/i2400m: " Julia Lawall
2014-05-19 7:47 ` walter harms
@ 2014-05-19 12:46 ` Sergei Shtylyov
2014-05-20 0:44 ` [PATCH 4/13 v2] " Julia Lawall
1 sibling, 1 reply; 18+ messages in thread
From: Sergei Shtylyov @ 2014-05-19 12:46 UTC (permalink / raw)
To: Julia Lawall, Inaky Perez-Gonzalez
Cc: kernel-janitors, linux-wimax, netdev, linux-kernel
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
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH 4/13 v2] wimax/i2400m: make return of 0 explicit
2014-05-19 12:46 ` Sergei Shtylyov
@ 2014-05-20 0:44 ` Julia Lawall
2014-05-21 21:19 ` David Miller
0 siblings, 1 reply; 18+ messages in thread
From: Julia Lawall @ 2014-05-20 0:44 UTC (permalink / raw)
To: Sergei Shtylyov
Cc: Inaky Perez-Gonzalez, kernel-janitors, linux-wimax, netdev,
linux-kernel
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);
^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH 4/13 v2] wimax/i2400m: make return of 0 explicit
2014-05-20 0:44 ` [PATCH 4/13 v2] " Julia Lawall
@ 2014-05-21 21:19 ` David Miller
0 siblings, 0 replies; 18+ messages in thread
From: David Miller @ 2014-05-21 21:19 UTC (permalink / raw)
To: julia.lawall
Cc: sergei.shtylyov, inaky.perez-gonzalez, kernel-janitors,
linux-wimax, netdev, linux-kernel
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.
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH 6/13] [NETFILTER]: arp_tables: make return of 0 explicit
2014-05-19 4:31 [PATCH 0/13] make return of 0 explicit Julia Lawall
2014-05-19 4:31 ` [PATCH 1/13] libertas: " Julia Lawall
2014-05-19 4:31 ` [PATCH 4/13] wimax/i2400m: " Julia Lawall
@ 2014-05-19 4:31 ` Julia Lawall
2014-05-19 4:31 ` [PATCH 12/13] brcmsmac: " Julia Lawall
3 siblings, 0 replies; 18+ messages in thread
From: Julia Lawall @ 2014-05-19 4:31 UTC (permalink / raw)
To: Pablo Neira Ayuso
Cc: kernel-janitors, Patrick McHardy, Jozsef Kadlecsik,
David S. Miller, Alexey Kuznetsov, James Morris,
Hideaki YOSHIFUJI, netfilter-devel, netfilter, coreteam, netdev,
linux-kernel
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,
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH 12/13] brcmsmac: make return of 0 explicit
2014-05-19 4:31 [PATCH 0/13] make return of 0 explicit Julia Lawall
` (2 preceding siblings ...)
2014-05-19 4:31 ` [PATCH 6/13] [NETFILTER]: arp_tables: " Julia Lawall
@ 2014-05-19 4:31 ` Julia Lawall
3 siblings, 0 replies; 18+ messages in thread
From: Julia Lawall @ 2014-05-19 4:31 UTC (permalink / raw)
To: Brett Rudley
Cc: kernel-janitors, Arend van Spriel, Franky (Zhenhui) Lin,
Hante Meuleman, John W. Linville, linux-wireless,
brcm80211-dev-list, netdev, linux-kernel
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;
}
^ permalink raw reply related [flat|nested] 18+ messages in thread