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