[PATCH 1/3] mwifiex: debugfs: correct histogram spacing, formatting

[PATCH 1/3] mwifiex: debugfs: correct histogram spacing, formatting

[Brian Norris]

Currently, snippets of this file look like:

rx rates (in Mbps): 0=1M   1=2M2=5.5M  3=11M   4=6M   5=9M  6=12M
7=18M  8=24M  9=36M  10=48M  11=54M12-27=MCS0-15(BW20) 28-43=MCS0-15(BW40)
44-53=MCS0-9(VHT:BW20)54-63=MCS0-9(VHT:BW40)64-73=MCS0-9(VHT:BW80)
...
noise_flr[--96dBm] = 22
noise_flr[--95dBm] = 149
noise_flr[--94dBm] = 9
noise_flr[--93dBm] = 2

We're missing some spaces, and we're adding a minus sign ('-') on values
that are already negative signed integers.

Signed-off-by: Brian Norris <briannorris@chromium.org>
---
 drivers/net/wireless/marvell/mwifiex/debugfs.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/net/wireless/marvell/mwifiex/debugfs.c b/drivers/net/wireless/marvell/mwifiex/debugfs.c
index cce70252fd96..65e48e16cb49 100644
--- a/drivers/net/wireless/marvell/mwifiex/debugfs.c
+++ b/drivers/net/wireless/marvell/mwifiex/debugfs.c
@@ -273,14 +273,14 @@ mwifiex_histogram_read(struct file *file, char __user *ubuf,
 		     "total samples = %d\n",
 		     atomic_read(&phist_data->num_samples));
 
-	p += sprintf(p, "rx rates (in Mbps): 0=1M   1=2M");
+	p += sprintf(p, "rx rates (in Mbps): 0=1M   1=2M ");
 	p += sprintf(p, "2=5.5M  3=11M   4=6M   5=9M  6=12M\n");
-	p += sprintf(p, "7=18M  8=24M  9=36M  10=48M  11=54M");
+	p += sprintf(p, "7=18M  8=24M  9=36M  10=48M  11=54M ");
 	p += sprintf(p, "12-27=MCS0-15(BW20) 28-43=MCS0-15(BW40)\n");
 
 	if (ISSUPP_11ACENABLED(priv->adapter->fw_cap_info)) {
-		p += sprintf(p, "44-53=MCS0-9(VHT:BW20)");
-		p += sprintf(p, "54-63=MCS0-9(VHT:BW40)");
+		p += sprintf(p, "44-53=MCS0-9(VHT:BW20) ");
+		p += sprintf(p, "54-63=MCS0-9(VHT:BW40) ");
 		p += sprintf(p, "64-73=MCS0-9(VHT:BW80)\n\n");
 	} else {
 		p += sprintf(p, "\n");
@@ -310,7 +310,7 @@ mwifiex_histogram_read(struct file *file, char __user *ubuf,
 	for (i = 0; i < MWIFIEX_MAX_NOISE_FLR; i++) {
 		value = atomic_read(&phist_data->noise_flr[i]);
 		if (value)
-			p += sprintf(p, "noise_flr[-%02ddBm] = %d\n",
+			p += sprintf(p, "noise_flr[%02ddBm] = %d\n",
 				(int)(i-128), value);
 	}
 	for (i = 0; i < MWIFIEX_MAX_SIG_STRENGTH; i++) {
-- 
2.20.0.rc1.387.gf8505762e3-goog

[PATCH 2/3] mwifiex: refactor mwifiex_parse_htinfo() for reuse

[Brian Norris]

This function converts some firmware-specific parameters into cfg80211
'rate_info' structures. It currently assumes it's dealing only with TX
bitrate, but the RX bitrate looks to be the same, so refactor this
function to be reusable.

Signed-off-by: Brian Norris <briannorris@chromium.org>
---
 .../net/wireless/marvell/mwifiex/cfg80211.c   | 36 ++++++++++---------
 1 file changed, 19 insertions(+), 17 deletions(-)

diff --git a/drivers/net/wireless/marvell/mwifiex/cfg80211.c b/drivers/net/wireless/marvell/mwifiex/cfg80211.c
index adc88433faa8..02b80ea232a7 100644
--- a/drivers/net/wireless/marvell/mwifiex/cfg80211.c
+++ b/drivers/net/wireless/marvell/mwifiex/cfg80211.c
@@ -1275,27 +1275,27 @@ mwifiex_cfg80211_change_virtual_intf(struct wiphy *wiphy,
 }
 
 static void
-mwifiex_parse_htinfo(struct mwifiex_private *priv, u8 tx_htinfo,
+mwifiex_parse_htinfo(struct mwifiex_private *priv, u8 rateinfo, u8 htinfo,
 		     struct rate_info *rate)
 {
 	struct mwifiex_adapter *adapter = priv->adapter;
 
 	if (adapter->is_hw_11ac_capable) {
 		/* bit[1-0]: 00=LG 01=HT 10=VHT */
-		if (tx_htinfo & BIT(0)) {
+		if (htinfo & BIT(0)) {
 			/* HT */
-			rate->mcs = priv->tx_rate;
+			rate->mcs = rateinfo;
 			rate->flags |= RATE_INFO_FLAGS_MCS;
 		}
-		if (tx_htinfo & BIT(1)) {
+		if (htinfo & BIT(1)) {
 			/* VHT */
-			rate->mcs = priv->tx_rate & 0x0F;
+			rate->mcs = rateinfo & 0x0F;
 			rate->flags |= RATE_INFO_FLAGS_VHT_MCS;
 		}
 
-		if (tx_htinfo & (BIT(1) | BIT(0))) {
+		if (htinfo & (BIT(1) | BIT(0))) {
 			/* HT or VHT */
-			switch (tx_htinfo & (BIT(3) | BIT(2))) {
+			switch (htinfo & (BIT(3) | BIT(2))) {
 			case 0:
 				rate->bw = RATE_INFO_BW_20;
 				break;
@@ -1310,26 +1310,26 @@ mwifiex_parse_htinfo(struct mwifiex_private *priv, u8 tx_htinfo,
 				break;
 			}
 
-			if (tx_htinfo & BIT(4))
+			if (htinfo & BIT(4))
 				rate->flags |= RATE_INFO_FLAGS_SHORT_GI;
 
-			if ((priv->tx_rate >> 4) == 1)
+			if ((rateinfo >> 4) == 1)
 				rate->nss = 2;
 			else
 				rate->nss = 1;
 		}
 	} else {
 		/*
-		 * Bit 0 in tx_htinfo indicates that current Tx rate
-		 * is 11n rate. Valid MCS index values for us are 0 to 15.
+		 * Bit 0 in htinfo indicates that current rate is 11n. Valid
+		 * MCS index values for us are 0 to 15.
 		 */
-		if ((tx_htinfo & BIT(0)) && (priv->tx_rate < 16)) {
-			rate->mcs = priv->tx_rate;
+		if ((htinfo & BIT(0)) && (rateinfo < 16)) {
+			rate->mcs = rateinfo;
 			rate->flags |= RATE_INFO_FLAGS_MCS;
 			rate->bw = RATE_INFO_BW_20;
-			if (tx_htinfo & BIT(1))
+			if (htinfo & BIT(1))
 				rate->bw = RATE_INFO_BW_40;
-			if (tx_htinfo & BIT(2))
+			if (htinfo & BIT(2))
 				rate->flags |= RATE_INFO_FLAGS_SHORT_GI;
 		}
 	}
@@ -1375,7 +1375,8 @@ mwifiex_dump_station_info(struct mwifiex_private *priv,
 		sinfo->tx_packets = node->stats.tx_packets;
 		sinfo->tx_failed = node->stats.tx_failed;
 
-		mwifiex_parse_htinfo(priv, node->stats.last_tx_htinfo,
+		mwifiex_parse_htinfo(priv, priv->tx_rate,
+				     node->stats.last_tx_htinfo,
 				     &sinfo->txrate);
 		sinfo->txrate.legacy = node->stats.last_tx_rate * 5;
 
@@ -1401,7 +1402,8 @@ mwifiex_dump_station_info(struct mwifiex_private *priv,
 			 HostCmd_ACT_GEN_GET, DTIM_PERIOD_I,
 			 &priv->dtim_period, true);
 
-	mwifiex_parse_htinfo(priv, priv->tx_htinfo, &sinfo->txrate);
+	mwifiex_parse_htinfo(priv, priv->tx_rate, priv->tx_htinfo,
+			     &sinfo->txrate);
 
 	sinfo->signal_avg = priv->bcn_rssi_avg;
 	sinfo->rx_bytes = priv->stats.rx_bytes;
-- 
2.20.0.rc1.387.gf8505762e3-goog

[PATCH 3/3] mwifiex: add NL80211_STA_INFO_RX_BITRATE support

[Brian Norris]

Comparing the existing TX_BITRATE parsing code (in
mwifiex_parse_htinfo()) with the RX bitrate histograms in debugfs.c, it
appears that the rxpd_rate and rxpd_htinfo fields have the same format.
At least, they give reasonable results when I parse them this way.

So this patch adds support for RX_BITRATE to our station info dump.

Along the way, I add legacy bitrate parsing into the same function,
using the debugfs code (mwifiex_histogram_read() and
mwifiex_adjust_data_rate()) as reference.

Additionally, to satisfy the requirements of
NL80211_STA_INFO_RX_BITRATE, I skip logging the bitrate of multicast
packets. This shouldn't add a lot of overhead to the RX path, as there
are already several similar 802.3 header checks in this same codepath.
We can also bias the branch behavior to favor unicast, as that's the
common performance-sensitive case.

I'd consider this support somewhat experimental, as I have zero
documentation from Marvell. But the existing driver code gives me good
reason to think this is correct.

I've tested this on a few different 802.11{a,b,g,n,ac} networks, and the
reported bitrates look good to me.

Signed-off-by: Brian Norris <briannorris@chromium.org>
---
I'd appreciate it if someone from Marvell could double check my work
here.

 .../net/wireless/marvell/mwifiex/cfg80211.c   | 26 +++++++++++++++++++
 drivers/net/wireless/marvell/mwifiex/sta_rx.c | 13 ++++++----
 2 files changed, 34 insertions(+), 5 deletions(-)

diff --git a/drivers/net/wireless/marvell/mwifiex/cfg80211.c b/drivers/net/wireless/marvell/mwifiex/cfg80211.c
index 02b80ea232a7..1467af22e394 100644
--- a/drivers/net/wireless/marvell/mwifiex/cfg80211.c
+++ b/drivers/net/wireless/marvell/mwifiex/cfg80211.c
@@ -1333,6 +1333,28 @@ mwifiex_parse_htinfo(struct mwifiex_private *priv, u8 rateinfo, u8 htinfo,
 				rate->flags |= RATE_INFO_FLAGS_SHORT_GI;
 		}
 	}
+
+	/* Decode legacy rates for non-HT. */
+	if (!(htinfo & (BIT(0) | BIT(1)))) {
+		/* Bitrates in multiples of 100kb/s. */
+		static const int legacy_rates[] = {
+			[0] = 10,
+			[1] = 20,
+			[2] = 55,
+			[3] = 110,
+			[4] = 60, /* MWIFIEX_RATE_INDEX_OFDM0 */
+			[5] = 60,
+			[6] = 90,
+			[7] = 120,
+			[8] = 180,
+			[9] = 240,
+			[10] = 360,
+			[11] = 480,
+			[12] = 540,
+		};
+		if (rateinfo < ARRAY_SIZE(legacy_rates))
+			rate->legacy = legacy_rates[rateinfo];
+	}
 }
 
 /*
@@ -1414,6 +1436,10 @@ mwifiex_dump_station_info(struct mwifiex_private *priv,
 	/* bit rate is in 500 kb/s units. Convert it to 100kb/s units */
 	sinfo->txrate.legacy = rate * 5;
 
+	sinfo->filled |= BIT(NL80211_STA_INFO_RX_BITRATE);
+	mwifiex_parse_htinfo(priv, priv->rxpd_rate, priv->rxpd_htinfo,
+			     &sinfo->rxrate);
+
 	if (priv->bss_mode == NL80211_IFTYPE_STATION) {
 		sinfo->filled |= BIT_ULL(NL80211_STA_INFO_BSS_PARAM);
 		sinfo->bss_param.flags = 0;
diff --git a/drivers/net/wireless/marvell/mwifiex/sta_rx.c b/drivers/net/wireless/marvell/mwifiex/sta_rx.c
index 00fcbda09349..fb28a5c7f441 100644
--- a/drivers/net/wireless/marvell/mwifiex/sta_rx.c
+++ b/drivers/net/wireless/marvell/mwifiex/sta_rx.c
@@ -152,14 +152,17 @@ int mwifiex_process_rx_packet(struct mwifiex_private *priv,
 		mwifiex_process_tdls_action_frame(priv, offset, rx_pkt_len);
 	}
 
-	priv->rxpd_rate = local_rx_pd->rx_rate;
-
-	priv->rxpd_htinfo = local_rx_pd->ht_info;
+	/* Only stash RX bitrate for unicast packets. */
+	if (likely(!is_multicast_ether_addr(rx_pkt_hdr->eth803_hdr.h_dest))) {
+		priv->rxpd_rate = local_rx_pd->rx_rate;
+		priv->rxpd_htinfo = local_rx_pd->ht_info;
+	}
 
 	if (GET_BSS_ROLE(priv) == MWIFIEX_BSS_ROLE_STA ||
 	    GET_BSS_ROLE(priv) == MWIFIEX_BSS_ROLE_UAP) {
-		adj_rx_rate = mwifiex_adjust_data_rate(priv, priv->rxpd_rate,
-						       priv->rxpd_htinfo);
+		adj_rx_rate = mwifiex_adjust_data_rate(priv,
+						       local_rx_pd->rx_rate,
+						       local_rx_pd->ht_info);
 		mwifiex_hist_data_add(priv, adj_rx_rate, local_rx_pd->snr,
 				      local_rx_pd->nf);
 	}
-- 
2.20.0.rc1.387.gf8505762e3-goog

Re: [PATCH 1/3] mwifiex: debugfs: correct histogram spacing, formatting

[Joe Perches]

On Fri, 2018-11-30 at 14:53 -0800, Brian Norris wrote:
> Currently, snippets of this file look like:
> 
> rx rates (in Mbps): 0=1M   1=2M2=5.5M  3=11M   4=6M   5=9M  6=12M
> 7=18M  8=24M  9=36M  10=48M  11=54M12-27=MCS0-15(BW20) 28-43=MCS0-15(BW40)
> 44-53=MCS0-9(VHT:BW20)54-63=MCS0-9(VHT:BW40)64-73=MCS0-9(VHT:BW80)
> ...
> noise_flr[--96dBm] = 22
> noise_flr[--95dBm] = 149
> noise_flr[--94dBm] = 9
> noise_flr[--93dBm] = 2
> 
> We're missing some spaces, and we're adding a minus sign ('-') on values
> that are already negative signed integers.
> 
> Signed-off-by: Brian Norris <briannorris@chromium.org>
> ---
>  drivers/net/wireless/marvell/mwifiex/debugfs.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/net/wireless/marvell/mwifiex/debugfs.c b/drivers/net/wireless/marvell/mwifiex/debugfs.c
> index cce70252fd96..65e48e16cb49 100644
> --- a/drivers/net/wireless/marvell/mwifiex/debugfs.c
> +++ b/drivers/net/wireless/marvell/mwifiex/debugfs.c
> @@ -273,14 +273,14 @@ mwifiex_histogram_read(struct file *file, char __user *ubuf,
>  		     "total samples = %d\n",
>  		     atomic_read(&phist_data->num_samples));
>  
> -	p += sprintf(p, "rx rates (in Mbps): 0=1M   1=2M");
> +	p += sprintf(p, "rx rates (in Mbps): 0=1M   1=2M ");
>  	p += sprintf(p, "2=5.5M  3=11M   4=6M   5=9M  6=12M\n");
> -	p += sprintf(p, "7=18M  8=24M  9=36M  10=48M  11=54M");
> +	p += sprintf(p, "7=18M  8=24M  9=36M  10=48M  11=54M ");
>  	p += sprintf(p, "12-27=MCS0-15(BW20) 28-43=MCS0-15(BW40)\n");
> 

It's be smaller object code and more intelligible source to use
a single coalesced string with one sprintf

>  	if (ISSUPP_11ACENABLED(priv->adapter->fw_cap_info)) {
> -		p += sprintf(p, "44-53=MCS0-9(VHT:BW20)");
> -		p += sprintf(p, "54-63=MCS0-9(VHT:BW40)");
> +		p += sprintf(p, "44-53=MCS0-9(VHT:BW20) ");
> +		p += sprintf(p, "54-63=MCS0-9(VHT:BW40) ");
>  		p += sprintf(p, "64-73=MCS0-9(VHT:BW80)\n\n");
>  	} else {
>  		p += sprintf(p, "\n");
> @@ -310,7 +310,7 @@ mwifiex_histogram_read(struct file *file, char __user *ubuf,y 
>  	for (i = 0; i < MWIFIEX_MAX_NOISE_FLR; i++) {
>  		value = atomic_read(&phist_data->noise_flr[i]);
>  		if (value)
> -			p += sprintf(p, "noise_flr[-%02ddBm] = %d\n",
> +			p += sprintf(p, "noise_flr[%02ddBm] = %d\n",
>  				(int)(i-128), value);
>  	}
>  	for (i = 0; i < MWIFIEX_MAX_SIG_STRENGTH; i++) {

[PATCH v2] mwifiex: debugfs: correct histogram spacing, formatting

[Brian Norris]

Currently, snippets of this file look like:

rx rates (in Mbps): 0=1M   1=2M2=5.5M  3=11M   4=6M   5=9M  6=12M
7=18M  8=24M  9=36M  10=48M  11=54M12-27=MCS0-15(BW20) 28-43=MCS0-15(BW40)
44-53=MCS0-9(VHT:BW20)54-63=MCS0-9(VHT:BW40)64-73=MCS0-9(VHT:BW80)
...
noise_flr[--96dBm] = 22
noise_flr[--95dBm] = 149
noise_flr[--94dBm] = 9
noise_flr[--93dBm] = 2

We're missing some spaces, and we're adding a minus sign ('-') on values
that are already negative signed integers.

Signed-off-by: Brian Norris <briannorris@chromium.org>
---
v2:
 * combine strings to improve readability and save a tiny bit of code

On Mon, Dec 03, 2018 at 10:32:52AM -0800, Joe Perches wrote:
> It's be smaller object code and more intelligible source to use
> a single coalesced string with one sprintf

Sure, I can do that.

Here's a v2 that combines both sets of strings in that way. I'm not
resending the other patches, since they were only related in concept
(since I was referring to debugfs for implementing the nl80211 stuff),
but have no real dependency.

@Kalle: I can resend the others as a new series if that helps.

 drivers/net/wireless/marvell/mwifiex/debugfs.c | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

diff --git a/drivers/net/wireless/marvell/mwifiex/debugfs.c b/drivers/net/wireless/marvell/mwifiex/debugfs.c
index cce70252fd96..cbe4493b3266 100644
--- a/drivers/net/wireless/marvell/mwifiex/debugfs.c
+++ b/drivers/net/wireless/marvell/mwifiex/debugfs.c
@@ -273,15 +273,13 @@ mwifiex_histogram_read(struct file *file, char __user *ubuf,
 		     "total samples = %d\n",
 		     atomic_read(&phist_data->num_samples));
 
-	p += sprintf(p, "rx rates (in Mbps): 0=1M   1=2M");
-	p += sprintf(p, "2=5.5M  3=11M   4=6M   5=9M  6=12M\n");
-	p += sprintf(p, "7=18M  8=24M  9=36M  10=48M  11=54M");
-	p += sprintf(p, "12-27=MCS0-15(BW20) 28-43=MCS0-15(BW40)\n");
+	p += sprintf(p,
+		     "rx rates (in Mbps): 0=1M   1=2M 2=5.5M  3=11M   4=6M   5=9M  6=12M\n"
+		     "7=18M  8=24M  9=36M  10=48M  11=54M 12-27=MCS0-15(BW20) 28-43=MCS0-15(BW40)\n");
 
 	if (ISSUPP_11ACENABLED(priv->adapter->fw_cap_info)) {
-		p += sprintf(p, "44-53=MCS0-9(VHT:BW20)");
-		p += sprintf(p, "54-63=MCS0-9(VHT:BW40)");
-		p += sprintf(p, "64-73=MCS0-9(VHT:BW80)\n\n");
+		p += sprintf(p,
+			     "44-53=MCS0-9(VHT:BW20) 54-63=MCS0-9(VHT:BW40) 64-73=MCS0-9(VHT:BW80)\n\n");
 	} else {
 		p += sprintf(p, "\n");
 	}
@@ -310,7 +308,7 @@ mwifiex_histogram_read(struct file *file, char __user *ubuf,
 	for (i = 0; i < MWIFIEX_MAX_NOISE_FLR; i++) {
 		value = atomic_read(&phist_data->noise_flr[i]);
 		if (value)
-			p += sprintf(p, "noise_flr[-%02ddBm] = %d\n",
+			p += sprintf(p, "noise_flr[%02ddBm] = %d\n",
 				(int)(i-128), value);
 	}
 	for (i = 0; i < MWIFIEX_MAX_SIG_STRENGTH; i++) {
-- 
2.20.0.rc1.387.gf8505762e3-goog

Re: [PATCH v2] mwifiex: debugfs: correct histogram spacing, formatting

[Kalle Valo]

Brian Norris <briannorris@chromium.org> writes:

> Here's a v2 that combines both sets of strings in that way. I'm not
> resending the other patches, since they were only related in concept
> (since I was referring to debugfs for implementing the nl80211 stuff),
> but have no real dependency.
>
> @Kalle: I can resend the others as a new series if that helps.

Yeah, it does. Trying to pick patches from different places in patchwork
is just too much of a hassle for me. So please do resend the whole
series.

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches#resubmit_the_whole_patchset

-- 
Kalle Valo

Re: [PATCH v2] mwifiex: debugfs: correct histogram spacing, formatting

[Brian Norris]

On Tue, Dec 04, 2018 at 08:37:30AM +0200, Kalle Valo wrote:
> Brian Norris <briannorris@chromium.org> writes:
> 
> > Here's a v2 that combines both sets of strings in that way. I'm not
> > resending the other patches, since they were only related in concept
> > (since I was referring to debugfs for implementing the nl80211 stuff),
> > but have no real dependency.
> >
> > @Kalle: I can resend the others as a new series if that helps.
> 
> Yeah, it does. Trying to pick patches from different places in patchwork
> is just too much of a hassle for me. So please do resend the whole
> series.

I've split them up and resent the other 2 unmodified now (as 'RFC PATCH
v2 X/2'), as this patch is trivial and not related to the others
directly. They can be applied in any order. Hopefully that's sufficient.

Brian

Re: [PATCH v2] mwifiex: debugfs: correct histogram spacing, formatting

[Kalle Valo]

Brian Norris <briannorris@chromium.org> wrote:

> Currently, snippets of this file look like:
> 
> rx rates (in Mbps): 0=1M   1=2M2=5.5M  3=11M   4=6M   5=9M  6=12M
> 7=18M  8=24M  9=36M  10=48M  11=54M12-27=MCS0-15(BW20) 28-43=MCS0-15(BW40)
> 44-53=MCS0-9(VHT:BW20)54-63=MCS0-9(VHT:BW40)64-73=MCS0-9(VHT:BW80)
> ...
> noise_flr[--96dBm] = 22
> noise_flr[--95dBm] = 149
> noise_flr[--94dBm] = 9
> noise_flr[--93dBm] = 2
> 
> We're missing some spaces, and we're adding a minus sign ('-') on values
> that are already negative signed integers.
> 
> Signed-off-by: Brian Norris <briannorris@chromium.org>

Patch applied to wireless-drivers-next.git, thanks.

4cb777c64e03 mwifiex: debugfs: correct histogram spacing, formatting

-- 
https://patchwork.kernel.org/patch/10710795/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches