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 _______________________________________________ devel mailing list devel@xxxxxxxxxxxxxxxxxxxxxx http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
