From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751628AbbFYMHE (ORCPT ); Thu, 25 Jun 2015 08:07:04 -0400 Received: from mout.kundenserver.de ([212.227.17.13]:49989 "EHLO mout.kundenserver.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750993AbbFYMGz (ORCPT ); Thu, 25 Jun 2015 08:06:55 -0400 From: Arnd Bergmann To: Gaston Gonzalez Cc: gregkh@linuxfoundation.org, paul.gortmaker@windriver.com, dilekuzulmez@gmail.com, gdonald@gmail.com, cristina.opriceana@gmail.com, hamohammed.sa@gmail.com, devel@driverdev.osuosl.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH] staging: rtl8192u: ieee80211_rx: Fix incorrect type in assignments Date: Thu, 25 Jun 2015 14:06:44 +0200 Message-ID: <13543765.eFokhJjyZi@wuerfel> User-Agent: KMail/4.11.5 (Linux/3.16.0-10-generic; KDE/4.11.5; x86_64; ; ) In-Reply-To: <20150624163458.GA1613@debsktop1> References: <1434924729-21086-1-git-send-email-gascoar@gmail.com> <1775494.ZyQRebyBWF@wuerfel> <20150624163458.GA1613@debsktop1> MIME-Version: 1.0 Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="us-ascii" X-Provags-ID: V03:K0:8NtJA88VUI+tNpZBVFe0oV7vknIt62m3PD1h4ZR66t4o9Ls74K+ cQPsiXd0apNQBE8cXhZe4h3a2WkxMC8i6Xg43Hwxy+85jHr9f66HPqnKW7zX7sFVeC5y7kC QnxVaMuRdbTCyamwppWzk94tO7GPLCke7S/TBsnL8e/60866lSyacp1LBc3hESdZsCvyKf9 +rPABibQDhTI9NzDeEBKA== X-UI-Out-Filterresults: notjunk:1;V01:K0:/64ThynT5D0=:sEcZhjqAiNkZq8BtAHG3XS LEfAFVbW4VypYj07xsQwxPsYDd0hcUeRZVCNRJ01YXt1BtGPo5KB1lR/eAciYuApeot0do8k7 e/FRVRJTzm3zFcCHsM6F66wHS9cdKH830Ulse60YfvW+gXNxEcHDyy2pnQfxqXR4qJK2bW6/l NRexTjuDrcuEGxPjZh5bJba7QOzF1BnqThIr+Vk1Lam0/rqJOmD993cox2C/X8XwK8zuRWYI9 jQdjmjv7HODGB3oJFnKnd5d4CGvudYnJYGV/dAucBuFEAqeKVoiw3cMoIz5cX0Sh5lJh5AGHT 4Pp/WypnNEx9m9kJxZJk1pyYgMkWr7301xFd6vul4YNlCKQroaablCyEH42O4DmsTznXr0nUp 4eKuDdsNopendStcOdcowpnKaZFa/UylBgxrVp1dsc3KtjwVMcsq6xAyC2Gq6+6oqIfydIPZZ k9SVG3SToDPllFbXERgDx7V8etEkmXpHXueGbxU7pKDoB+QGqpx8tRu3XwIkhvgcsfIQXhyLo w/TZ/JVQqfCm/+ymYMI0+MlW81r1feeU04WPkXuLc0VBMsuFCRcoFhafEBt/ID6fqqVBUlyVZ 6EktjwjCbFnLqWjc/G1GB+mZWEXwEavBVoGUJKByHnz6PP0bkrkLmuTYBhw1ZgUbESap44vZ2 0BlLMLaiDGKpdCrgXP19UbAh8ED6nLlHlwDm0Ev9eLu6wlA== Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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