diff mbox

[OPW,kernel] staging:wlan-ng:Fix sparse warning

Message ID 20140310103527.GA7606@himangi-Inspiron-N5110gmail.com
State New, archived
Headers show

Commit Message

HIMANGI SARAOGI March 10, 2014, 10:35 a.m. UTC
This patch fixes the sparse warning -
drivers/staging/wlan-ng/p80211netdev.c:289:38: warning: cast to restricted __le16
by eliminating the variable fc as it is not required.

Signed-off-by: Himangi Saraogi <himangi774@gmail.com>
---
 drivers/staging/wlan-ng/p80211netdev.c |    4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

Comments

Bob Copeland March 10, 2014, 5:01 p.m. UTC | #1
On Mon, Mar 10, 2014 at 04:05:27PM +0530, Himangi Saraogi wrote:
>  			} else {
>  				hdr = (struct p80211_hdr_a3 *) skb->data;
> -				fc = le16_to_cpu(hdr->fc);
> -				if (p80211_rx_typedrop(wlandev, fc)) {
> +				if (p80211_rx_typedrop(wlandev, hdr->fc)) {

Are you sure?  It seems like p80211_rx_typedrop() wants to operate
on CPU-endian values.  Since hdr->fc is coming straight from the
packet (skb->data) it's most likely supposed to be __le16, just
missing an annotation in struct p80211_hdr_a3.
HIMANGI SARAOGI March 10, 2014, 5:54 p.m. UTC | #2
On Monday, March 10, 2014 10:31:13 PM UTC+5:30, Bob Copeland wrote:
>
> On Mon, Mar 10, 2014 at 04:05:27PM +0530, Himangi Saraogi wrote: 
> >                          } else { 
> >                                  hdr = (struct p80211_hdr_a3 *) 
> skb->data; 
> > -                                fc = le16_to_cpu(hdr->fc); 
> > -                                if (p80211_rx_typedrop(wlandev, fc)) { 
> > +                                if (p80211_rx_typedrop(wlandev, 
> hdr->fc)) { 
>
> Are you sure?  It seems like p80211_rx_typedrop() wants to operate 
> on CPU-endian values.  Since hdr->fc is coming straight from the 
> packet (skb->data) it's most likely supposed to be __le16, just 
> missing an annotation in struct p80211_hdr_a3. 
>
> hdr->fc is of type u16. Here is the definition  from p80211hdr.h.
struct p80211_hdr_a3 {
    u16 fc;
    u16 dur;
    u8 a1[ETH_ALEN];
    u8 a2[ETH_ALEN];
    u8 a3[ETH_ALEN];
    u16 seq;
} __packed;
Also, the function p80211_rx_typedrop() operates on CPU-endian values
as it is declared as follow in p80211netdev.c.
static int p80211_rx_typedrop(wlandevice_t *wlandev, u16 fc);

So, I suppose the fix is fine. 

> -- 
> Bob Copeland %% www.bobcopeland.com 
>

Thanks
Himangi
Bob Copeland March 10, 2014, 6:10 p.m. UTC | #3
On Mon, Mar 10, 2014 at 11:14:32PM +0530, Himangi Saraogi wrote:
> hdr->fc is of type u16. Here is the definition  from p80211hdr.h.
> struct p80211_hdr_a3 {
>     u16 fc;
>     u16 dur;
>     u8 a1[ETH_ALEN];
>     u8 a2[ETH_ALEN];
>     u8 a3[ETH_ALEN];
>     u16 seq;
> } __packed;
> Also, the function p80211_rx_typedrop() operates on CPU-endian values
> as it is declared as follow in p80211netdev.c.
> static int p80211_rx_typedrop(wlandevice_t *wlandev, u16 fc);

I'm suggesting that struct p80211_hdr_a3 really should be:

struct p80211_hdr_a3 {
    __le16 fc;
    __le16 dur;
    u8 a1[ETH_ALEN];
    u8 a2[ETH_ALEN];
    u8 a3[ETH_ALEN];
    __le16 seq;
} __packed;

Because:

> > >                               hdr = (struct p80211_hdr_a3 *) skb->data;

That is casting a packet buffer pointer to the structure (probably
for a frame received over the air given the function name), without
any conversion to CPU-endianness.

Most fields in 802.11 frames are little-endian (I just happen to know
this, but you could look at the 802.11 standard to be sure, or other
similar structures in include/linux/ieee80211.h).

As such I don't think your change is correct: it is changing
functionality for BE platforms.  Also, generally speaking it's a
bit more likely that the original author would have forgotten an
endianness conversion or annotation than to have gone to the trouble
of adding one that is wrong.
Waskiewicz Jr, Peter P March 13, 2014, 10:46 p.m. UTC | #4
On Mon, 2014-03-10 at 16:05 +0530, Himangi Saraogi wrote:
> This patch fixes the sparse warning -
> drivers/staging/wlan-ng/p80211netdev.c:289:38: warning: cast to restricted __le16
> by eliminating the variable fc as it is not required.
> 
> Signed-off-by: Himangi Saraogi <himangi774@gmail.com>

Applied.

> ---
>  drivers/staging/wlan-ng/p80211netdev.c |    4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/drivers/staging/wlan-ng/p80211netdev.c b/drivers/staging/wlan-ng/p80211netdev.c
> index 0039e08..e3ae802 100644
> --- a/drivers/staging/wlan-ng/p80211netdev.c
> +++ b/drivers/staging/wlan-ng/p80211netdev.c
> @@ -262,7 +262,6 @@ static void p80211netdev_rx_bh(unsigned long arg)
>  	struct sk_buff *skb = NULL;
>  	netdevice_t *dev = wlandev->netdev;
>  	struct p80211_hdr_a3 *hdr;
> -	u16 fc;
>  
>  	/* Let's empty our our queue */
>  	while ((skb = skb_dequeue(&wlandev->nsd_rxq))) {
> @@ -286,8 +285,7 @@ static void p80211netdev_rx_bh(unsigned long arg)
>  				continue;
>  			} else {
>  				hdr = (struct p80211_hdr_a3 *) skb->data;
> -				fc = le16_to_cpu(hdr->fc);
> -				if (p80211_rx_typedrop(wlandev, fc)) {
> +				if (p80211_rx_typedrop(wlandev, hdr->fc)) {
>  					dev_kfree_skb(skb);
>  					continue;
>  				}
> -- 
> 1.7.9.5
>
diff mbox

Patch

diff --git a/drivers/staging/wlan-ng/p80211netdev.c b/drivers/staging/wlan-ng/p80211netdev.c
index 0039e08..e3ae802 100644
--- a/drivers/staging/wlan-ng/p80211netdev.c
+++ b/drivers/staging/wlan-ng/p80211netdev.c
@@ -262,7 +262,6 @@  static void p80211netdev_rx_bh(unsigned long arg)
 	struct sk_buff *skb = NULL;
 	netdevice_t *dev = wlandev->netdev;
 	struct p80211_hdr_a3 *hdr;
-	u16 fc;
 
 	/* Let's empty our our queue */
 	while ((skb = skb_dequeue(&wlandev->nsd_rxq))) {
@@ -286,8 +285,7 @@  static void p80211netdev_rx_bh(unsigned long arg)
 				continue;
 			} else {
 				hdr = (struct p80211_hdr_a3 *) skb->data;
-				fc = le16_to_cpu(hdr->fc);
-				if (p80211_rx_typedrop(wlandev, fc)) {
+				if (p80211_rx_typedrop(wlandev, hdr->fc)) {
 					dev_kfree_skb(skb);
 					continue;
 				}