2011-04-27 19:15:49

by John W. Linville

[permalink] [raw]
Subject: [RFT/PATCH] mwl8k: replace rateinfo bitfields with mask and shift macros

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 <[email protected]>
---
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;
--
1.7.4.4



2011-04-28 05:34:05

by Nishant Sarmukadam

[permalink] [raw]
Subject: Re: [RFT/PATCH] mwl8k: replace rateinfo bitfields with mask and shift macros

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 <[email protected]>
> ---
> 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;