[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