Subject: Re: [RFT/PATCH] mwl8k: replace rateinfo bitfields with mask and
	shift macros
From: Nishant Sarmukadam <nishants@marvell.com>
Reply-To: <nishants@marvell.com>
To: "John W. Linville" <linville@tuxdriver.com>
CC: "linux-wireless@vger.kernel.org" <linux-wireless@vger.kernel.org>,
	Pradeep Nemavat <pnemavat@marvell.com>,
	Yogesh Powar <yogeshp@marvell.com>,
	Lennert Buytenhek <buytenh@wantstofly.org>
In-Reply-To: <1303931068-5922-1-git-send-email-linville@tuxdriver.com>
References: <1303931068-5922-1-git-send-email-linville@tuxdriver.com>
Content-Type: text/plain
Date: Thu, 28 Apr 2011 10:58:45 +0530
Message-ID: <1303968525.10316.5.camel@localhost.localdomain> (sfid-20110428_073409_144930_C364AB22)
MIME-Version: 1.0
Sender: linux-wireless-owner@vger.kernel.org

Hi John,

Thanks for looking into this. I have tested this patch and it is working
fine.  

Regards,
Nishant

On Wed, 2011-04-27 at 12:04 -0700, John W. Linville wrote:
> AFAICT, this driver is claiming that 24 bits of rate info fit into a
> 16-bit field in the Tx descriptor.  Anyway, the use of bitfields is
> frowned-upon for a variety of well-documented reasons...
> 
> Signed-off-by: John W. Linville <linville@tuxdriver.com>
> ---
> These masks could be backwards -- someone with the hardware please let
> me know if that is the case or if this version works?
> 
>  drivers/net/wireless/mwl8k.c |   25 +++++--------------------
>  1 files changed, 5 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/net/wireless/mwl8k.c b/drivers/net/wireless/mwl8k.c
> index b8f2b12..8a1b262 100644
> --- a/drivers/net/wireless/mwl8k.c
> +++ b/drivers/net/wireless/mwl8k.c
> @@ -1562,24 +1562,11 @@ static int mwl8k_tid_queue_mapping(u8 tid)
>  
>  /* The firmware will fill in the rate information
>   * for each packet that gets queued in the hardware
> - * in this structure
> + * and these macros will interpret that info.
>   */
>  
> -struct rateinfo {
> -	__le16  format:1;
> -	__le16  short_gi:1;
> -	__le16  band_width:1;
> -	__le16  rate_id_mcs:6;
> -	__le16  adv_coding:2;
> -	__le16  antenna:2;
> -	__le16  act_sub_chan:2;
> -	__le16  preamble_type:1;
> -	__le16  power_id:4;
> -	__le16  antenna2:1;
> -	__le16  reserved:1;
> -	__le16  tx_bf_frame:1;
> -	__le16  green_field:1;
> -} __packed;
> +#define RI_FORMAT(a)		  (a & 0x0001)
> +#define RI_RATE_ID_MCS(a)	 ((a & 0x01f8) >> 3)
>  
>  static int
>  mwl8k_txq_reclaim(struct ieee80211_hw *hw, int index, int limit, int force)
> @@ -1600,7 +1587,6 @@ mwl8k_txq_reclaim(struct ieee80211_hw *hw, int index, int limit, int force)
>  		struct ieee80211_sta *sta;
>  		struct mwl8k_sta *sta_info = NULL;
>  		u16 rate_info;
> -		struct rateinfo *rate;
>  		struct ieee80211_hdr *wh;
>  
>  		tx = txq->head;
> @@ -1643,14 +1629,13 @@ mwl8k_txq_reclaim(struct ieee80211_hw *hw, int index, int limit, int force)
>  				sta_info = MWL8K_STA(sta);
>  				BUG_ON(sta_info == NULL);
>  				rate_info = le16_to_cpu(tx_desc->rate_info);
> -				rate = (struct rateinfo *)&rate_info;
>  				/* If rate is < 6.5 Mpbs for an ht station
>  				 * do not form an ampdu. If the station is a
>  				 * legacy station (format = 0), do not form an
>  				 * ampdu
>  				 */
> -				if (rate->rate_id_mcs < 1 ||
> -				    rate->format == 0) {
> +				if (RI_RATE_ID_MCS(rate_info) < 1 ||
> +				    RI_FORMAT(rate_info) == 0) {
>  					sta_info->is_ampdu_allowed = false;
>  				} else {
>  					sta_info->is_ampdu_allowed = true;