From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932300AbbFXQch (ORCPT ); Wed, 24 Jun 2015 12:32:37 -0400 Received: from mail-qc0-f178.google.com ([209.85.216.178]:33305 "EHLO mail-qc0-f178.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932285AbbFXQcZ (ORCPT ); Wed, 24 Jun 2015 12:32:25 -0400 Date: Wed, 24 Jun 2015 13:34:58 -0300 From: Gaston Gonzalez To: Arnd Bergmann 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, gascoar@gmail.com Subject: Re: [PATCH] staging: rtl8192u: ieee80211_rx: Fix incorrect type in assignments Message-ID: <20150624163458.GA1613@debsktop1> References: <1434924729-21086-1-git-send-email-gascoar@gmail.com> <1775494.ZyQRebyBWF@wuerfel> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1775494.ZyQRebyBWF@wuerfel> User-Agent: Mutt/1.5.23 (2014-03-12) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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