On Wed, May 16, 2007 at 05:01:27PM -0400, Florin Malita wrote:
> In libertas_process_rxed_packet() and process_rxed_802_11_packet() the
> skb is dereferenced after being passed to netif_rx (called from
> libertas_upload_rx_packet). Spotted by Coverity (1658, 1659).
Relocating the libertas_upload_rx_packet call is fine, but...
> Also, libertas_upload_rx_packet() unconditionally returns 0 so the error
> check is dead code - might as well take it out.
Is this merely an implementation detail? Or an absolute fact?
If the former is true, then we should preserve the error
checking. If the latter, then we should change the signature of
libertas_upload_rx_packet to return void.
> Signed-off-by: Florin Malita <[email protected]>
> lbs_pr_debug(1, "RX Data: size of actual packet = %d\n", skb->len);
> - if (libertas_upload_rx_packet(priv, skb)) {
> - lbs_pr_debug(1, "RX error: libertas_upload_rx_packet"
> - " returns failure\n");
> - ret = -1;
> - goto done;
> - }
> priv->stats.rx_bytes += skb->len;
> priv->stats.rx_packets++;
>
> + libertas_upload_rx_packet(priv, skb);
> +
> ret = 0;
> done:
> LEAVE();
Another potential patch is to remove the "ret = 0" line before the
"done" label, since ret is initialized at the head of the function.
Come to think of it, you can probably remove the "= 0" part of ret's
declaration as well (in both functions).
Hth!
John
P.S. Also, please make sure to send wireless patches to
[email protected] and CC me.
--
John W. Linville
[email protected]
John W. Linville wrote:
>> Also, libertas_upload_rx_packet() unconditionally returns 0 so the error
>> check is dead code - might as well take it out.
>>
>
> Is this merely an implementation detail? Or an absolute fact?
>
I believe it's an absolute fact that got lost among implementation
details ;)
All libertas_upload_rx_packet does is set a few fields in the skb, then
pass it up to the stack via netif_rx:
139 int libertas_upload_rx_packet(wlan_private * priv, struct sk_buff *skb)
140 {
141 lbs_pr_debug(1, "skb->data=%p\n", skb->data);
142
143 if(IS_MESH_FRAME(skb))
144 skb->dev = priv->mesh_dev;
145 else
146 skb->dev = priv->wlan_dev.netdev;
147 skb->protocol = eth_type_trans(skb, priv->wlan_dev.netdev);
148 skb->ip_summed = CHECKSUM_UNNECESSARY;
149
150 netif_rx(skb);
151
152 return 0;
153 }
Since netif_rx always succeeds, so should libertas_upload_rx_packet -
there's no reason for passing back a success code (especially one that's
hardcoded to 0).
> If the latter, then we should change the signature of
> libertas_upload_rx_packet to return void.
>
Makes sense, updated patch below.
> Another potential patch is to remove the "ret = 0" line before the
> "done" label, since ret is initialized at the head of the function.
> Come to think of it, you can probably remove the "= 0" part of ret's
> declaration as well (in both functions).
>
Right, even more: looks like both process_rxed_802_11_packet &
libertas_process_rxed_packet can only return 0 so we could drop the
return code altogether and change their signature to void too (nobody
seems to care about their return code anyway). I will send a separate
cleanup patch but this might be leaning more on the implementation
detail side (planning to extend the functions and make the return code
meaningful in the future?) so somebody familiar with the driver should
make the call.
Thanks,
Florin
Signed-off-by: Florin Malita <[email protected]>
---
decl.h | 2 +-
rx.c | 22 +++++-----------------
2 files changed, 6 insertions(+), 18 deletions(-)
diff --git a/drivers/net/wireless/libertas/decl.h b/drivers/net/wireless/libertas/decl.h
index 606bdd0..dfe2764 100644
--- a/drivers/net/wireless/libertas/decl.h
+++ b/drivers/net/wireless/libertas/decl.h
@@ -46,7 +46,7 @@ u32 libertas_index_to_data_rate(u8 index);
u8 libertas_data_rate_to_index(u32 rate);
void libertas_get_fwversion(wlan_adapter * adapter, char *fwversion, int maxlen);
-int libertas_upload_rx_packet(wlan_private * priv, struct sk_buff *skb);
+void libertas_upload_rx_packet(wlan_private * priv, struct sk_buff *skb);
/** The proc fs interface */
int libertas_process_rx_command(wlan_private * priv);
diff --git a/drivers/net/wireless/libertas/rx.c b/drivers/net/wireless/libertas/rx.c
index d17924f..b19b5aa 100644
--- a/drivers/net/wireless/libertas/rx.c
+++ b/drivers/net/wireless/libertas/rx.c
@@ -136,7 +136,7 @@ static void wlan_compute_rssi(wlan_private * priv, struct rxpd *p_rx_pd)
LEAVE();
}
-int libertas_upload_rx_packet(wlan_private * priv, struct sk_buff *skb)
+void libertas_upload_rx_packet(wlan_private * priv, struct sk_buff *skb)
{
lbs_pr_debug(1, "skb->data=%p\n", skb->data);
@@ -148,8 +148,6 @@ int libertas_upload_rx_packet(wlan_private * priv, struct sk_buff *skb)
skb->ip_summed = CHECKSUM_UNNECESSARY;
netif_rx(skb);
-
- return 0;
}
/**
@@ -269,15 +267,11 @@ int libertas_process_rxed_packet(wlan_private * priv, struct sk_buff *skb)
wlan_compute_rssi(priv, p_rx_pd);
lbs_pr_debug(1, "RX Data: size of actual packet = %d\n", skb->len);
- if (libertas_upload_rx_packet(priv, skb)) {
- lbs_pr_debug(1, "RX error: libertas_upload_rx_packet"
- " returns failure\n");
- ret = -1;
- goto done;
- }
priv->stats.rx_bytes += skb->len;
priv->stats.rx_packets++;
+ libertas_upload_rx_packet(priv, skb);
+
ret = 0;
done:
LEAVE();
@@ -438,17 +432,11 @@ static int process_rxed_802_11_packet(wlan_private * priv, struct sk_buff *skb)
wlan_compute_rssi(priv, prxpd);
lbs_pr_debug(1, "RX Data: size of actual packet = %d\n", skb->len);
-
- if (libertas_upload_rx_packet(priv, skb)) {
- lbs_pr_debug(1, "RX error: libertas_upload_rx_packet "
- "returns failure\n");
- ret = -1;
- goto done;
- }
-
priv->stats.rx_bytes += skb->len;
priv->stats.rx_packets++;
+ libertas_upload_rx_packet(priv, skb);
+
ret = 0;
done:
LEAVE();
Stephen Hemminger wrote:
> The skb is always consumed so the the return value is informational only.
Yes, this is true. Handing off an skb to netif_rx() relieves the driver
of further responsibility for it.
Jeff
On Fri, 2007-05-18 at 14:09 -0400, John W. Linville wrote:
> On Wed, May 16, 2007 at 05:01:27PM -0400, Florin Malita wrote:
> > In libertas_process_rxed_packet() and process_rxed_802_11_packet() the
> > skb is dereferenced after being passed to netif_rx (called from
> > libertas_upload_rx_packet). Spotted by Coverity (1658, 1659).
>
> Relocating the libertas_upload_rx_packet call is fine, but...
>
> > Also, libertas_upload_rx_packet() unconditionally returns 0 so the error
> > check is dead code - might as well take it out.
>
> Is this merely an implementation detail? Or an absolute fact?
> If the former is true, then we should preserve the error
> checking. If the latter, then we should change the signature of
> libertas_upload_rx_packet to return void.
According to the comments, netif_rx always succeeds. I think we should
just change the return type to void since there's nothing else in that
function that can fail.
Dan
> > Signed-off-by: Florin Malita <[email protected]>
>
> > lbs_pr_debug(1, "RX Data: size of actual packet = %d\n", skb->len);
> > - if (libertas_upload_rx_packet(priv, skb)) {
> > - lbs_pr_debug(1, "RX error: libertas_upload_rx_packet"
> > - " returns failure\n");
> > - ret = -1;
> > - goto done;
> > - }
> > priv->stats.rx_bytes += skb->len;
> > priv->stats.rx_packets++;
> >
> > + libertas_upload_rx_packet(priv, skb);
> > +
> > ret = 0;
> > done:
> > LEAVE();
>
> Another potential patch is to remove the "ret = 0" line before the
> "done" label, since ret is initialized at the head of the function.
> Come to think of it, you can probably remove the "= 0" part of ret's
> declaration as well (in both functions).
>
> Hth!
>
> John
>
> P.S. Also, please make sure to send wireless patches to
> [email protected] and CC me.
From: Jeff Garzik <[email protected]>
Date: Sat, 19 May 2007 21:47:00 -0400
> Dan Williams wrote:
> > On Fri, 2007-05-18 at 14:09 -0400, John W. Linville wrote:
> >> On Wed, May 16, 2007 at 05:01:27PM -0400, Florin Malita wrote:
> >>> In libertas_process_rxed_packet() and process_rxed_802_11_packet() the
> >>> skb is dereferenced after being passed to netif_rx (called from
> >>> libertas_upload_rx_packet). Spotted by Coverity (1658, 1659).
> >>
> >> Relocating the libertas_upload_rx_packet call is fine, but...
> >>
> >>> Also, libertas_upload_rx_packet() unconditionally returns 0 so the error
> >>> check is dead code - might as well take it out.
> >> Is this merely an implementation detail? Or an absolute fact?
> >> If the former is true, then we should preserve the error
> >> checking. If the latter, then we should change the signature of
> >> libertas_upload_rx_packet to return void.
> >
> > According to the comments, netif_rx always succeeds. I think we should
> > just change the return type to void since there's nothing else in that
> > function that can fail.
>
> According to the implementation, netif_rx() can fail.
It doesn't exactly "fail", but it does give return values
which indicate RX congestion.
On Fri, 18 May 2007 14:09:03 -0400
"John W. Linville" <[email protected]> wrote:
> On Wed, May 16, 2007 at 05:01:27PM -0400, Florin Malita wrote:
> > In libertas_process_rxed_packet() and process_rxed_802_11_packet() the
> > skb is dereferenced after being passed to netif_rx (called from
> > libertas_upload_rx_packet). Spotted by Coverity (1658, 1659).
>
> Relocating the libertas_upload_rx_packet call is fine, but...
>
> > Also, libertas_upload_rx_packet() unconditionally returns 0 so the error
> > check is dead code - might as well take it out.
>
> Is this merely an implementation detail? Or an absolute fact?
> If the former is true, then we should preserve the error
> checking. If the latter, then we should change the signature of
> libertas_upload_rx_packet to return void.
netif_rx() used to return a value in older kernels.
David Miller wrote:
> From: Jeff Garzik <[email protected]>
> Date: Sat, 19 May 2007 21:47:00 -0400
>
>> According to the implementation, netif_rx() can fail.
>>
>
> It doesn't exactly "fail", but it does give return values
> which indicate RX congestion.
>
Assuming you're referring to NET_RX_CN_*, this doesn't seem to be the
case anymore: it appears netif_rx can only return NET_RX_SUCCESS or
NET_RX_DROP.
The congestion level constants are hardly used at all - if they are
left-overs, would it make sense ripping them out completely?
Either way I believe the following should be OK:
Remove inaccurate netif_rx() return value comments.
Signed-off-by: Florin Malita <[email protected]>
---
dev.c | 3 ---
1 file changed, 3 deletions(-)
diff --git a/net/core/dev.c b/net/core/dev.c
index f2b6111..79f5d90 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -1628,9 +1628,6 @@ DEFINE_PER_CPU(struct netif_rx_stats, netdev_rx_stat) = { 0, };
*
* return values:
* NET_RX_SUCCESS (no congestion)
- * NET_RX_CN_LOW (low congestion)
- * NET_RX_CN_MOD (moderate congestion)
- * NET_RX_CN_HIGH (high congestion)
* NET_RX_DROP (packet was dropped)
*
*/
Dan Williams wrote:
> On Fri, 2007-05-18 at 14:09 -0400, John W. Linville wrote:
>> On Wed, May 16, 2007 at 05:01:27PM -0400, Florin Malita wrote:
>>> In libertas_process_rxed_packet() and process_rxed_802_11_packet() the
>>> skb is dereferenced after being passed to netif_rx (called from
>>> libertas_upload_rx_packet). Spotted by Coverity (1658, 1659).
>>
>> Relocating the libertas_upload_rx_packet call is fine, but...
>>
>>> Also, libertas_upload_rx_packet() unconditionally returns 0 so the error
>>> check is dead code - might as well take it out.
>> Is this merely an implementation detail? Or an absolute fact?
>> If the former is true, then we should preserve the error
>> checking. If the latter, then we should change the signature of
>> libertas_upload_rx_packet to return void.
>
> According to the comments, netif_rx always succeeds. I think we should
> just change the return type to void since there's nothing else in that
> function that can fail.
According to the implementation, netif_rx() can fail.
Jeff
On Sat, 19 May 2007 21:47:00 -0400
Jeff Garzik <[email protected]> wrote:
> Dan Williams wrote:
> > On Fri, 2007-05-18 at 14:09 -0400, John W. Linville wrote:
> >> On Wed, May 16, 2007 at 05:01:27PM -0400, Florin Malita wrote:
> >>> In libertas_process_rxed_packet() and process_rxed_802_11_packet() the
> >>> skb is dereferenced after being passed to netif_rx (called from
> >>> libertas_upload_rx_packet). Spotted by Coverity (1658, 1659).
> >>
> >> Relocating the libertas_upload_rx_packet call is fine, but...
> >>
> >>> Also, libertas_upload_rx_packet() unconditionally returns 0 so the error
> >>> check is dead code - might as well take it out.
> >> Is this merely an implementation detail? Or an absolute fact?
> >> If the former is true, then we should preserve the error
> >> checking. If the latter, then we should change the signature of
> >> libertas_upload_rx_packet to return void.
> >
> > According to the comments, netif_rx always succeeds. I think we should
> > just change the return type to void since there's nothing else in that
> > function that can fail.
>
> According to the implementation, netif_rx() can fail.
>
> Jeff
Yeah, it was the old congestion levels that got dropped.
The skb is always consumed so the the return value is informational only.
--
Stephen Hemminger <[email protected]>
Stephen Hemminger wrote:
> netif_rx() used to return a value in older kernels.
netif_rx() returns a value in current kernels.
Jeff