[PATCH] staging: rtl8192u: ieee80211_rx: Fix incorrect type in assignments

[PATCH] staging: rtl8192u: ieee80211_rx: Fix incorrect type in assignments

[Gaston Gonzalez]

Fix the following incorrect type in assignments detected by sparse:

drivers/staging/rtl8192u/ieee80211/ieee80211_rx.c:571:37: warning: incorrect type in assignment (different base types)
drivers/staging/rtl8192u/ieee80211/ieee80211_rx.c:571:37:    expected unsigned short [unsigned] [usertype] len
drivers/staging/rtl8192u/ieee80211/ieee80211_rx.c:571:37:    got restricted __be16 [usertype] <noident>

drivers/staging/rtl8192u/ieee80211/ieee80211_rx.c:1330:45: warning: incorrect type in assignment (different base types)
drivers/staging/rtl8192u/ieee80211/ieee80211_rx.c:1330:45:    expected unsigned short [unsigned] [usertype] len
drivers/staging/rtl8192u/ieee80211/ieee80211_rx.c:1330:45:    got restricted __be16 [usertype] <noident>

drivers/staging/rtl8192u/ieee80211/ieee80211_rx.c:1495:40: warning: incorrect type in assignment (different base types)
drivers/staging/rtl8192u/ieee80211/ieee80211_rx.c:1495:40:    expected restricted __le16 <noident>
drivers/staging/rtl8192u/ieee80211/ieee80211_rx.c:1495:40:    got int

drivers/staging/rtl8192u/ieee80211/ieee80211_rx.c:1497:40: warning: incorrect type in assignment (different base types)
drivers/staging/rtl8192u/ieee80211/ieee80211_rx.c:1497:40:    expected restricted __le16 <noident>
drivers/staging/rtl8192u/ieee80211/ieee80211_rx.c:1497:40:    got int

drivers/staging/rtl8192u/ieee80211/ieee80211_rx.c:1501:45: warning: incorrect type in assignment (different base types)
drivers/staging/rtl8192u/ieee80211/ieee80211_rx.c:1501:45:    expected restricted __le16 <noident>
drivers/staging/rtl8192u/ieee80211/ieee80211_rx.c:1501:45:    got unsigned short [unsigned] [usertype] <noident>


Signed-off-by: Gaston Gonzalez <gascoar@gmail.com>
---
 drivers/staging/rtl8192u/ieee80211/ieee80211_rx.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/drivers/staging/rtl8192u/ieee80211/ieee80211_rx.c b/drivers/staging/rtl8192u/ieee80211/ieee80211_rx.c
index b374088..15bcf7e 100644
--- a/drivers/staging/rtl8192u/ieee80211/ieee80211_rx.c
+++ b/drivers/staging/rtl8192u/ieee80211/ieee80211_rx.c
@@ -566,7 +566,7 @@ void ieee80211_indicate_packets(struct ieee80211_device *ieee, struct ieee80211_
 				memcpy(skb_push(sub_skb, ETH_ALEN), prxb->src, ETH_ALEN);
 				memcpy(skb_push(sub_skb, ETH_ALEN), prxb->dst, ETH_ALEN);
 			} else {
-				u16 len;
+				__be16 len;
 			/* Leave Ethernet header part of hdr and full payload */
 				len = htons(sub_skb->len);
 				memcpy(skb_push(sub_skb, 2), &len, 2);
@@ -1325,7 +1325,7 @@ int ieee80211_rx(struct ieee80211_device *ieee, struct sk_buff *skb,
 					memcpy(skb_push(sub_skb, ETH_ALEN), src, ETH_ALEN);
 					memcpy(skb_push(sub_skb, ETH_ALEN), dst, ETH_ALEN);
 				} else {
-					u16 len;
+					__be16 len;
 					/* Leave Ethernet header part of hdr and full payload */
 					len = htons(sub_skb->len);
 					memcpy(skb_push(sub_skb, 2), &len, 2);
@@ -1492,13 +1492,15 @@ static int ieee80211_qos_convert_ac_to_parameters(struct
 		/* WMM spec P.11: The minimum value for AIFSN shall be 2 */
 		qos_param->aifs[aci] = (qos_param->aifs[aci] < 2) ? 2:qos_param->aifs[aci];
 
-		qos_param->cw_min[aci] = ac_params->ecw_min_max & 0x0F;
+		qos_param->cw_min[aci] =
+			cpu_to_le16(ac_params->ecw_min_max & 0x0F);
 
-		qos_param->cw_max[aci] = (ac_params->ecw_min_max & 0xF0) >> 4;
+		qos_param->cw_max[aci] =
+			cpu_to_le16((ac_params->ecw_min_max & 0xF0) >> 4);
 
 		qos_param->flag[aci] =
 		    (ac_params->aci_aifsn & 0x10) ? 0x01 : 0x00;
-		qos_param->tx_op_limit[aci] = le16_to_cpu(ac_params->tx_op_limit);
+		qos_param->tx_op_limit[aci] = ac_params->tx_op_limit;
 	}
 	return 0;
 }
-- 
2.4.2

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] staging: rtl8192u: ieee80211_rx: Fix incorrect type in assignments

[Arnd Bergmann]

On Sunday 21 June 2015 19:12:09 Gaston Gonzalez wrote:
>                 /* WMM spec P.11: The minimum value for AIFSN shall be 2 */
>                 qos_param->aifs[aci] = (qos_param->aifs[aci] < 2) ? 2:qos_param->aifs[aci];
>  
> -               qos_param->cw_min[aci] = ac_params->ecw_min_max & 0x0F;
> +               qos_param->cw_min[aci] =
> +                       cpu_to_le16(ac_params->ecw_min_max & 0x0F);
>  
> -               qos_param->cw_max[aci] = (ac_params->ecw_min_max & 0xF0) >> 4;
> +               qos_param->cw_max[aci] =
> +                       cpu_to_le16((ac_params->ecw_min_max & 0xF0) >> 4);
>  
>                 qos_param->flag[aci] =
>                     (ac_params->aci_aifsn & 0x10) ? 0x01 : 0x00;
> -               qos_param->tx_op_limit[aci] = le16_to_cpu(ac_params->tx_op_limit);
> +               qos_param->tx_op_limit[aci] = ac_params->tx_op_limit;
>         }
>         return 0;

This certainly needs a more thorough description of how you determined that
the byte swaps that you add are in fact required. Did you test it on
a big-endian machine?

	Arnd

Re: [PATCH] staging: rtl8192u: ieee80211_rx: Fix incorrect type in assignments

[Gaston Gonzalez]

On Tue, Jun 23, 2015 at 12:13:47PM +0200, Arnd Bergmann wrote:
> On Sunday 21 June 2015 19:12:09 Gaston Gonzalez wrote:
> >                 /* WMM spec P.11: The minimum value for AIFSN shall be 2 */
> >                 qos_param->aifs[aci] = (qos_param->aifs[aci] < 2) ? 2:qos_param->aifs[aci];
> >  
> > -               qos_param->cw_min[aci] = ac_params->ecw_min_max & 0x0F;
> > +               qos_param->cw_min[aci] =
> > +                       cpu_to_le16(ac_params->ecw_min_max & 0x0F);
> >  
> > -               qos_param->cw_max[aci] = (ac_params->ecw_min_max & 0xF0) >> 4;
> > +               qos_param->cw_max[aci] =
> > +                       cpu_to_le16((ac_params->ecw_min_max & 0xF0) >> 4);
> >  
> >                 qos_param->flag[aci] =
> >                     (ac_params->aci_aifsn & 0x10) ? 0x01 : 0x00;
> > -               qos_param->tx_op_limit[aci] = le16_to_cpu(ac_params->tx_op_limit);
> > +               qos_param->tx_op_limit[aci] = ac_params->tx_op_limit;
> >         }
> >         return 0;
> 
> This certainly needs a more thorough description of how you determined that
> the byte swaps that you add are in fact required. Did you test it on
> a big-endian machine?
> 
> 	Arnd

Hello Arnd,

Thank you for reviewing this.
After your email and reviwing this again I'm getting a bit suspicious
myself, but this is what I saw:

-- First warning:

qos_param->cw_min[aci] is defined as __le16() in ieee80211.h
(ieee80211_qos_parameters structure)

ac_params-> ecw_min_max is defined as u8 in ieee80211.h
(ieee80211_qos_ac_parameter structure)

So the assignment is: __le16 = u8 & 0x0F;

-- Second warning:

qos_param->cw_max[aci] is __le16()
ac_params-> ecw_min_max is u8

The assignment is: __le16 = (u8 & 0xF0) >> 4;

Thus, for the warning 1 and 2, I understand that the result won't be the
same if the machine is big-endian or little-endian, and that's why we
need a cpu_to_le16. Am I missing something?

-- Third warning:

In this case both sides of the assignment are already defined as __le16:

qos_param->tx_op_limit[aci] (ieee80211_qos_parameters structure defined
in ieee80211.h))

ac_params->tx_op_limit (ieee80211_qos_ac_parameter structure defined in
ieee80211.h)

So the assignment is: __le16() = le16_to_cpu(__le16)

Im getting suspicious now, but it sounded wrong to me.
In the case the right part is correct, I guess the left part should be
u16 type?

Regarding the test: I tested it on my machine, but is of course little-
endian :( I could built a qemu virtual machine to test it on a
big-endian emulated platform. Should that work?

Regards,

Gaston

Re: [PATCH] staging: rtl8192u: ieee80211_rx: Fix incorrect type in assignments

[Arnd Bergmann]

On Wednesday 24 June 2015 13:34:58 Gaston Gonzalez wrote:
> On Tue, Jun 23, 2015 at 12:13:47PM +0200, Arnd Bergmann wrote:
> > On Sunday 21 June 2015 19:12:09 Gaston Gonzalez wrote:
> > >                 /* WMM spec P.11: The minimum value for AIFSN shall be 2 */
> > >                 qos_param->aifs[aci] = (qos_param->aifs[aci] < 2) ? 2:qos_param->aifs[aci];
> > >  
> > > -               qos_param->cw_min[aci] = ac_params->ecw_min_max & 0x0F;
> > > +               qos_param->cw_min[aci] =
> > > +                       cpu_to_le16(ac_params->ecw_min_max & 0x0F);
> > >  
> > > -               qos_param->cw_max[aci] = (ac_params->ecw_min_max & 0xF0) >> 4;
> > > +               qos_param->cw_max[aci] =
> > > +                       cpu_to_le16((ac_params->ecw_min_max & 0xF0) >> 4);
> > >  
> > >                 qos_param->flag[aci] =
> > >                     (ac_params->aci_aifsn & 0x10) ? 0x01 : 0x00;
> > > -               qos_param->tx_op_limit[aci] = le16_to_cpu(ac_params->tx_op_limit);
> > > +               qos_param->tx_op_limit[aci] = ac_params->tx_op_limit;
> > >         }
> > >         return 0;
> > 
> > This certainly needs a more thorough description of how you determined that
> > the byte swaps that you add are in fact required. Did you test it on
> > a big-endian machine?
> > 
> > 	Arnd
> 
> Hello Arnd,
> 
> Thank you for reviewing this.
> After your email and reviwing this again I'm getting a bit suspicious
> myself, but this is what I saw:
> 
> -- First warning:
> 
> qos_param->cw_min[aci] is defined as __le16() in ieee80211.h
> (ieee80211_qos_parameters structure)
> 
> ac_params-> ecw_min_max is defined as u8 in ieee80211.h
> (ieee80211_qos_ac_parameter structure)
> 
> So the assignment is: __le16 = u8 & 0x0F;
> 
> -- Second warning:
> 
> qos_param->cw_max[aci] is __le16()
> ac_params-> ecw_min_max is u8
> 
> The assignment is: __le16 = (u8 & 0xF0) >> 4;
> 
> Thus, for the warning 1 and 2, I understand that the result won't be the
> same if the machine is big-endian or little-endian, and that's why we
> need a cpu_to_le16. Am I missing something?

I think your analysis is right, as long as the __le16 annotation is
actually correct. It usually helps to look at the git history to
see what the intent of the patch was that introduced the assignment
and the patch that introduced the __le16 type. Presumably one of them
was incorrect, and it would be good to figure out where it went wrong,
and to add a 'Fixes:' tag in your patch description that pinpoints
the exact mistake.

> -- Third warning:
> 
> In this case both sides of the assignment are already defined as __le16:
> 
> qos_param->tx_op_limit[aci] (ieee80211_qos_parameters structure defined
> in ieee80211.h))
> 
> ac_params->tx_op_limit (ieee80211_qos_ac_parameter structure defined in
> ieee80211.h)
> 
> So the assignment is: __le16() = le16_to_cpu(__le16)
> 
> Im getting suspicious now, but it sounded wrong to me.
> In the case the right part is correct, I guess the left part should be
> u16 type?

Again, your logic sounds good: there is clearly something wrong here, but
it's not obvious to conclude whether it is an incorrect annotation or
an extraneous byte swap. Besides looking at the git history, it also
helps to look at all other uses of the two sides of the assignment:

See if qos_param->tx_op_limit is in fact used as a little-endian
value (e.g. by copying to memory or a register), and if the value that
got written to ac_params->tx_op_limit was byte-swapped already at
the time of assignment.

> Regarding the test: I tested it on my machine, but is of course little-
> endian :( I could built a qemu virtual machine to test it on a
> big-endian emulated platform. Should that work?

Yes, that would work: you can assign the USB device to the qemu machine
and run a kernel in there. The easiest emulation to try is probably
a PowerPC PAPR machine with a file system from
https://people.debian.org/~aurel32/qemu/powerpc/.
MIPS should work as well.

	Arnd

Re: [PATCH] staging: rtl8192u: ieee80211_rx: Fix incorrect type in assignments

[Gaston Gonzalez]

On Thu, Jun 25, 2015 at 02:06:44PM +0200, Arnd Bergmann wrote:
> On Wednesday 24 June 2015 13:34:58 Gaston Gonzalez wrote:
> > On Tue, Jun 23, 2015 at 12:13:47PM +0200, Arnd Bergmann wrote:
> > > On Sunday 21 June 2015 19:12:09 Gaston Gonzalez wrote:
> > > >                 /* WMM spec P.11: The minimum value for AIFSN shall be 2 */
> > > >                 qos_param->aifs[aci] = (qos_param->aifs[aci] < 2) ? 2:qos_param->aifs[aci];
> > > >  
> > > > -               qos_param->cw_min[aci] = ac_params->ecw_min_max & 0x0F;
> > > > +               qos_param->cw_min[aci] =
> > > > +                       cpu_to_le16(ac_params->ecw_min_max & 0x0F);
> > > >  
> > > > -               qos_param->cw_max[aci] = (ac_params->ecw_min_max & 0xF0) >> 4;
> > > > +               qos_param->cw_max[aci] =
> > > > +                       cpu_to_le16((ac_params->ecw_min_max & 0xF0) >> 4);
> > > >  
> > > >                 qos_param->flag[aci] =
> > > >                     (ac_params->aci_aifsn & 0x10) ? 0x01 : 0x00;
> > > > -               qos_param->tx_op_limit[aci] = le16_to_cpu(ac_params->tx_op_limit);
> > > > +               qos_param->tx_op_limit[aci] = ac_params->tx_op_limit;
> > > >         }
> > > >         return 0;
> > > 
> > > This certainly needs a more thorough description of how you determined that
> > > the byte swaps that you add are in fact required. Did you test it on
> > > a big-endian machine?
> > > 
> > > 	Arnd
> > 
> > Hello Arnd,
> > 
> > Thank you for reviewing this.
> > After your email and reviwing this again I'm getting a bit suspicious
> > myself, but this is what I saw:
> > 
> > -- First warning:
> > 
> > qos_param->cw_min[aci] is defined as __le16() in ieee80211.h
> > (ieee80211_qos_parameters structure)
> > 
> > ac_params-> ecw_min_max is defined as u8 in ieee80211.h
> > (ieee80211_qos_ac_parameter structure)
> > 
> > So the assignment is: __le16 = u8 & 0x0F;
> > 
> > -- Second warning:
> > 
> > qos_param->cw_max[aci] is __le16()
> > ac_params-> ecw_min_max is u8
> > 
> > The assignment is: __le16 = (u8 & 0xF0) >> 4;
> > 
> > Thus, for the warning 1 and 2, I understand that the result won't be the
> > same if the machine is big-endian or little-endian, and that's why we
> > need a cpu_to_le16. Am I missing something?
> 
> I think your analysis is right, as long as the __le16 annotation is
> actually correct. It usually helps to look at the git history to
> see what the intent of the patch was that introduced the assignment
> and the patch that introduced the __le16 type. Presumably one of them
> was incorrect, and it would be good to figure out where it went wrong,
> and to add a 'Fixes:' tag in your patch description that pinpoints
> the exact mistake.
> 

Ok, will do.

> > -- Third warning:
> > 
> > In this case both sides of the assignment are already defined as __le16:
> > 
> > qos_param->tx_op_limit[aci] (ieee80211_qos_parameters structure defined
> > in ieee80211.h))
> > 
> > ac_params->tx_op_limit (ieee80211_qos_ac_parameter structure defined in
> > ieee80211.h)
> > 
> > So the assignment is: __le16() = le16_to_cpu(__le16)
> > 
> > Im getting suspicious now, but it sounded wrong to me.
> > In the case the right part is correct, I guess the left part should be
> > u16 type?
> 
> Again, your logic sounds good: there is clearly something wrong here, but
> it's not obvious to conclude whether it is an incorrect annotation or
> an extraneous byte swap. Besides looking at the git history, it also
> helps to look at all other uses of the two sides of the assignment:
> 
> See if qos_param->tx_op_limit is in fact used as a little-endian
> value (e.g. by copying to memory or a register), and if the value that
> got written to ac_params->tx_op_limit was byte-swapped already at
> the time of assignment.
> 
Ok, I'll do it too.

> > Regarding the test: I tested it on my machine, but is of course little-
> > endian :( I could built a qemu virtual machine to test it on a
> > big-endian emulated platform. Should that work?
> 
> Yes, that would work: you can assign the USB device to the qemu machine
> and run a kernel in there. The easiest emulation to try is probably
> a PowerPC PAPR machine with a file system from
> https://people.debian.org/~aurel32/qemu/powerpc/.
> MIPS should work as well.
> 

Ok, thanks a lot for all the pointers.

Gaston
> 	Arnd

Re: [PATCH] staging: rtl8192u: ieee80211_rx: Fix incorrect type in assignments

[Gaston Gonzalez]

On Fri, Jun 26, 2015 at 01:36:14PM -0300, Gaston Gonzalez wrote:
> On Thu, Jun 25, 2015 at 02:06:44PM +0200, Arnd Bergmann wrote:
> > On Wednesday 24 June 2015 13:34:58 Gaston Gonzalez wrote:
> > > On Tue, Jun 23, 2015 at 12:13:47PM +0200, Arnd Bergmann wrote:
> > > > On Sunday 21 June 2015 19:12:09 Gaston Gonzalez wrote:
> > > > >                 /* WMM spec P.11: The minimum value for AIFSN shall be 2 */
> > > > >                 qos_param->aifs[aci] = (qos_param->aifs[aci] < 2) ? 2:qos_param->aifs[aci];
> > > > >  
> > > > > -               qos_param->cw_min[aci] = ac_params->ecw_min_max & 0x0F;
> > > > > +               qos_param->cw_min[aci] =
> > > > > +                       cpu_to_le16(ac_params->ecw_min_max & 0x0F);
> > > > >  
> > > > > -               qos_param->cw_max[aci] = (ac_params->ecw_min_max & 0xF0) >> 4;
> > > > > +               qos_param->cw_max[aci] =
> > > > > +                       cpu_to_le16((ac_params->ecw_min_max & 0xF0) >> 4);
> > > > >  
> > > > >                 qos_param->flag[aci] =
> > > > >                     (ac_params->aci_aifsn & 0x10) ? 0x01 : 0x00;
> > > > > -               qos_param->tx_op_limit[aci] = le16_to_cpu(ac_params->tx_op_limit);
> > > > > +               qos_param->tx_op_limit[aci] = ac_params->tx_op_limit;
> > > > >         }
> > > > >         return 0;
> > > > 
> > > > This certainly needs a more thorough description of how you determined that
> > > > the byte swaps that you add are in fact required. Did you test it on
> > > > a big-endian machine?
> > > > 
> > > > 	Arnd
> > > 
> > > Hello Arnd,
> > > 
> > > Thank you for reviewing this.
> > > After your email and reviwing this again I'm getting a bit suspicious
> > > myself, but this is what I saw:
> > > 
> > > -- First warning:
> > > 
> > > qos_param->cw_min[aci] is defined as __le16() in ieee80211.h
> > > (ieee80211_qos_parameters structure)
> > > 
> > > ac_params-> ecw_min_max is defined as u8 in ieee80211.h
> > > (ieee80211_qos_ac_parameter structure)
> > > 
> > > So the assignment is: __le16 = u8 & 0x0F;
> > > 
> > > -- Second warning:
> > > 
> > > qos_param->cw_max[aci] is __le16()
> > > ac_params-> ecw_min_max is u8
> > > 
> > > The assignment is: __le16 = (u8 & 0xF0) >> 4;
> > > 
> > > Thus, for the warning 1 and 2, I understand that the result won't be the
> > > same if the machine is big-endian or little-endian, and that's why we
> > > need a cpu_to_le16. Am I missing something?
> > 
> > I think your analysis is right, as long as the __le16 annotation is
> > actually correct. It usually helps to look at the git history to
> > see what the intent of the patch was that introduced the assignment
> > and the patch that introduced the __le16 type. Presumably one of them
> > was incorrect, and it would be good to figure out where it went wrong,
> > and to add a 'Fixes:' tag in your patch description that pinpoints
> > the exact mistake.
> > 
> 
> Ok, will do.
> 
> > > -- Third warning:
> > > 
> > > In this case both sides of the assignment are already defined as __le16:
> > > 
> > > qos_param->tx_op_limit[aci] (ieee80211_qos_parameters structure defined
> > > in ieee80211.h))
> > > 
> > > ac_params->tx_op_limit (ieee80211_qos_ac_parameter structure defined in
> > > ieee80211.h)
> > > 
> > > So the assignment is: __le16() = le16_to_cpu(__le16)
> > > 
> > > Im getting suspicious now, but it sounded wrong to me.
> > > In the case the right part is correct, I guess the left part should be
> > > u16 type?
> > 
> > Again, your logic sounds good: there is clearly something wrong here, but
> > it's not obvious to conclude whether it is an incorrect annotation or
> > an extraneous byte swap. Besides looking at the git history, it also
> > helps to look at all other uses of the two sides of the assignment:
> > 
> > See if qos_param->tx_op_limit is in fact used as a little-endian
> > value (e.g. by copying to memory or a register), and if the value that
> > got written to ac_params->tx_op_limit was byte-swapped already at
> > the time of assignment.
> > 
> Ok, I'll do it too.
> 
> > > Regarding the test: I tested it on my machine, but is of course little-
> > > endian :( I could built a qemu virtual machine to test it on a
> > > big-endian emulated platform. Should that work?
> > 
> > Yes, that would work: you can assign the USB device to the qemu machine
> > and run a kernel in there. The easiest emulation to try is probably
> > a PowerPC PAPR machine with a file system from
> > https://people.debian.org/~aurel32/qemu/powerpc/.
> > MIPS should work as well.
> > 
> 
> Ok, thanks a lot for all the pointers.
> 
> Gaston
> > 	Arnd

So more than two months have passed without any reply from my side.

The thing is I'm struggling to get a new hardware with this chipset. Today I
received my fourth rtl8192cu device to my collection of wrong devices, which
also comprises one ralink and some other chip. All of them specified as
rtl8192u on the sellers site...

I have one or two options left to try to get a replacement rtl8192u device,
but for the moment I don't have the hardware.

Though I know this 'change'is almost insignificant I wanted to give some 
life signal and not leave this thread as abandoned.

regards,

Gaston