[PATCH v3 1/3] wifi: rtl8xxxu: Add __packed to struct rtl8723bu_c2h

[PATCH v3 1/3] wifi: rtl8xxxu: Add __packed to struct rtl8723bu_c2h

[Bitterblue Smith]

This struct is used to access a sequence of bytes received from the
wifi chip. It must not have any padding bytes between the members.

This doesn't change anything on my system, possibly because currently
none of the members need more than byte alignment.

Fixes: b2b43b7837ba ("rtl8xxxu: Initial functionality to handle C2H events for 8723bu")
Signed-off-by: Bitterblue Smith <rtl8821cerfe2@gmail.com>
---
v3:
 - No change.
 
v2:
 - Patch is new in v2.
---
 drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu.h b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu.h
index 136992f0200c..d26df4095da0 100644
--- a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu.h
+++ b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu.h
@@ -1240,7 +1240,7 @@ struct rtl8723bu_c2h {
 			u8 bw;
 		} __packed ra_report;
 	};
-};
+} __packed;
 
 struct rtl8xxxu_fileops;
 
-- 
2.38.0

[PATCH v3 2/3] wifi: rtl8xxxu: Fix the channel width reporting

[Bitterblue Smith]

The gen 2 chips RTL8192EU and RTL8188FU periodically send the driver
reports about the TX rate, and the driver passes these reports to
sta_statistics. The reports from RTL8192EU may or may not include the
channel width. The reports from RTL8188FU do not include it.

Only access the c2h->ra_report.bw field if the report (skb) is big
enough.

The other problem fixed here is that the code was actually never
changing the channel width initially reported by
rtl8xxxu_bss_info_changed because the value of RATE_INFO_BW_20 is 0.

Fixes: 0985d3a410ac ("rtl8xxxu: Feed current txrate information for mac80211")
Signed-off-by: Bitterblue Smith <rtl8821cerfe2@gmail.com>
---
v3:
 - Don't assume bw will always be 1 byte.
 
v2:
 - Eliminate the magic numbers.
---
 drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c
index 28f136064297..9799bc5ed60a 100644
--- a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c
+++ b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c
@@ -5569,7 +5569,6 @@ static void rtl8xxxu_c2hcmd_callback(struct work_struct *work)
 			rarpt->txrate.flags = 0;
 			rate = c2h->ra_report.rate;
 			sgi = c2h->ra_report.sgi;
-			bw = c2h->ra_report.bw;
 
 			if (rate < DESC_RATE_MCS0) {
 				rarpt->txrate.legacy =
@@ -5586,8 +5585,14 @@ static void rtl8xxxu_c2hcmd_callback(struct work_struct *work)
 						RATE_INFO_FLAGS_SHORT_GI;
 				}
 
-				if (bw == RATE_INFO_BW_20)
-					rarpt->txrate.bw |= RATE_INFO_BW_20;
+				if (skb->len >= offsetof(typeof(*c2h), ra_report.bw) +
+						sizeof(c2h->ra_report.bw)) {
+					if (c2h->ra_report.bw == RTL8XXXU_CHANNEL_WIDTH_40)
+						bw = RATE_INFO_BW_40;
+					else
+						bw = RATE_INFO_BW_20;
+					rarpt->txrate.bw = bw;
+				}
 			}
 			bit_rate = cfg80211_calculate_bitrate(&rarpt->txrate);
 			rarpt->bit_rate = bit_rate;
-- 
2.38.0

[PATCH v3 3/3] wifi: rtl8xxxu: Introduce rtl8xxxu_update_ra_report

[Bitterblue Smith]

The ra_report struct is used for reporting the TX rate via
sta_statistics. The code which fills it out is duplicated in two
places, and the RTL8188EU will need it in a third place. Move this
code into a new function rtl8xxxu_update_ra_report.

Signed-off-by: Bitterblue Smith <rtl8821cerfe2@gmail.com>
---
v3:
 - No change.

v2:
 - Compare against DESC_RATE_54M instead of DESC_RATE_MCS0.
---
 .../wireless/realtek/rtl8xxxu/rtl8xxxu_core.c | 107 ++++++++----------
 1 file changed, 46 insertions(+), 61 deletions(-)

diff --git a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c
index 9799bc5ed60a..7580ec8a5562 100644
--- a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c
+++ b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c
@@ -4598,6 +4598,32 @@ static void rtl8xxxu_set_aifs(struct rtl8xxxu_priv *priv, u8 slot_time)
 	}
 }
 
+static void rtl8xxxu_update_ra_report(struct rtl8xxxu_ra_report *rarpt,
+				      u8 rate, u8 sgi, u8 bw)
+{
+	u8 mcs, nss;
+
+	rarpt->txrate.flags = 0;
+
+	if (rate <= DESC_RATE_54M) {
+		rarpt->txrate.legacy = rtl8xxxu_legacy_ratetable[rate].bitrate;
+	} else {
+		rtl8xxxu_desc_to_mcsrate(rate, &mcs, &nss);
+		rarpt->txrate.flags |= RATE_INFO_FLAGS_MCS;
+
+		rarpt->txrate.mcs = mcs;
+		rarpt->txrate.nss = nss;
+
+		if (sgi)
+			rarpt->txrate.flags |= RATE_INFO_FLAGS_SHORT_GI;
+
+		rarpt->txrate.bw = bw;
+	}
+
+	rarpt->bit_rate = cfg80211_calculate_bitrate(&rarpt->txrate);
+	rarpt->desc_rate = rate;
+}
+
 static void
 rtl8xxxu_bss_info_changed(struct ieee80211_hw *hw, struct ieee80211_vif *vif,
 			  struct ieee80211_bss_conf *bss_conf, u64 changed)
@@ -4620,9 +4646,7 @@ rtl8xxxu_bss_info_changed(struct ieee80211_hw *hw, struct ieee80211_vif *vif,
 			u32 ramask;
 			int sgi = 0;
 			u8 highest_rate;
-			u8 mcs = 0, nss = 0;
-			u32 bit_rate;
-
+			u8 bw;
 
 			rcu_read_lock();
 			sta = ieee80211_find_sta(vif, bss_conf->bssid);
@@ -4647,37 +4671,19 @@ rtl8xxxu_bss_info_changed(struct ieee80211_hw *hw, struct ieee80211_vif *vif,
 				sgi = 1;
 
 			highest_rate = fls(ramask) - 1;
-			if (highest_rate < DESC_RATE_MCS0) {
-				rarpt->txrate.legacy =
-				rtl8xxxu_legacy_ratetable[highest_rate].bitrate;
-			} else {
-				rtl8xxxu_desc_to_mcsrate(highest_rate,
-							 &mcs, &nss);
-				rarpt->txrate.flags |= RATE_INFO_FLAGS_MCS;
-
-				rarpt->txrate.mcs = mcs;
-				rarpt->txrate.nss = nss;
-
-				if (sgi) {
-					rarpt->txrate.flags |=
-						RATE_INFO_FLAGS_SHORT_GI;
-				}
-
-				if (rtl8xxxu_ht40_2g &&
-				    (sta->deflink.ht_cap.cap & IEEE80211_HT_CAP_SUP_WIDTH_20_40))
-					rarpt->txrate.bw = RATE_INFO_BW_40;
-				else
-					rarpt->txrate.bw = RATE_INFO_BW_20;
-			}
+			if (rtl8xxxu_ht40_2g &&
+			    (sta->deflink.ht_cap.cap & IEEE80211_HT_CAP_SUP_WIDTH_20_40))
+				bw = RATE_INFO_BW_40;
+			else
+				bw = RATE_INFO_BW_20;
 			rcu_read_unlock();
-			bit_rate = cfg80211_calculate_bitrate(&rarpt->txrate);
-			rarpt->bit_rate = bit_rate;
-			rarpt->desc_rate = highest_rate;
+
+			rtl8xxxu_update_ra_report(rarpt, highest_rate, sgi, bw);
 
 			priv->vif = vif;
 			priv->rssi_level = RTL8XXXU_RATR_STA_INIT;
 
-			priv->fops->update_rate_mask(priv, ramask, 0, sgi, rarpt->txrate.bw == RATE_INFO_BW_40);
+			priv->fops->update_rate_mask(priv, ramask, 0, sgi, bw == RATE_INFO_BW_40);
 
 			rtl8xxxu_write8(priv, REG_BCN_MAX_ERR, 0xff);
 
@@ -5538,9 +5544,7 @@ static void rtl8xxxu_c2hcmd_callback(struct work_struct *work)
 	u8 bt_info = 0;
 	struct rtl8xxxu_btcoex *btcoex;
 	struct rtl8xxxu_ra_report *rarpt;
-	u8 rate, sgi, bw;
-	u32 bit_rate;
-	u8 mcs = 0, nss = 0;
+	u8 bw;
 
 	priv = container_of(work, struct rtl8xxxu_priv, c2hcmd_work);
 	btcoex = &priv->bt_coex;
@@ -5566,37 +5570,18 @@ static void rtl8xxxu_c2hcmd_callback(struct work_struct *work)
 			rtl8723bu_handle_bt_info(priv);
 			break;
 		case C2H_8723B_RA_REPORT:
-			rarpt->txrate.flags = 0;
-			rate = c2h->ra_report.rate;
-			sgi = c2h->ra_report.sgi;
-
-			if (rate < DESC_RATE_MCS0) {
-				rarpt->txrate.legacy =
-					rtl8xxxu_legacy_ratetable[rate].bitrate;
-			} else {
-				rtl8xxxu_desc_to_mcsrate(rate, &mcs, &nss);
-				rarpt->txrate.flags |= RATE_INFO_FLAGS_MCS;
+			bw = rarpt->txrate.bw;
 
-				rarpt->txrate.mcs = mcs;
-				rarpt->txrate.nss = nss;
-
-				if (sgi) {
-					rarpt->txrate.flags |=
-						RATE_INFO_FLAGS_SHORT_GI;
-				}
-
-				if (skb->len >= offsetof(typeof(*c2h), ra_report.bw) +
-						sizeof(c2h->ra_report.bw)) {
-					if (c2h->ra_report.bw == RTL8XXXU_CHANNEL_WIDTH_40)
-						bw = RATE_INFO_BW_40;
-					else
-						bw = RATE_INFO_BW_20;
-					rarpt->txrate.bw = bw;
-				}
+			if (skb->len >= offsetof(typeof(*c2h), ra_report.bw) +
+					sizeof(c2h->ra_report.bw)) {
+				if (c2h->ra_report.bw == RTL8XXXU_CHANNEL_WIDTH_40)
+					bw = RATE_INFO_BW_40;
+				else
+					bw = RATE_INFO_BW_20;
 			}
-			bit_rate = cfg80211_calculate_bitrate(&rarpt->txrate);
-			rarpt->bit_rate = bit_rate;
-			rarpt->desc_rate = rate;
+
+			rtl8xxxu_update_ra_report(rarpt, c2h->ra_report.rate,
+						  c2h->ra_report.sgi, bw);
 			break;
 		default:
 			break;
-- 
2.38.0

RE: [PATCH v3 2/3] wifi: rtl8xxxu: Fix the channel width reporting

[Ping-Ke Shih]

> -----Original Message-----
> From: Bitterblue Smith <rtl8821cerfe2@gmail.com>
> Sent: Wednesday, November 30, 2022 3:06 AM
> To: linux-wireless@vger.kernel.org
> Cc: Jes Sorensen <Jes.Sorensen@gmail.com>; Ping-Ke Shih <pkshih@realtek.com>
> Subject: [PATCH v3 2/3] wifi: rtl8xxxu: Fix the channel width reporting
> 
> The gen 2 chips RTL8192EU and RTL8188FU periodically send the driver
> reports about the TX rate, and the driver passes these reports to
> sta_statistics. The reports from RTL8192EU may or may not include the
> channel width. The reports from RTL8188FU do not include it.
> 
> Only access the c2h->ra_report.bw field if the report (skb) is big
> enough.
> 
> The other problem fixed here is that the code was actually never
> changing the channel width initially reported by
> rtl8xxxu_bss_info_changed because the value of RATE_INFO_BW_20 is 0.
> 
> Fixes: 0985d3a410ac ("rtl8xxxu: Feed current txrate information for mac80211")
> Signed-off-by: Bitterblue Smith <rtl8821cerfe2@gmail.com>
> ---
> v3:
>  - Don't assume bw will always be 1 byte.
> 
> v2:
>  - Eliminate the magic numbers.
> ---
>  drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c | 11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c
> b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c
> index 28f136064297..9799bc5ed60a 100644
> --- a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c
> +++ b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c
> @@ -5569,7 +5569,6 @@ static void rtl8xxxu_c2hcmd_callback(struct work_struct *work)
>  			rarpt->txrate.flags = 0;
>  			rate = c2h->ra_report.rate;
>  			sgi = c2h->ra_report.sgi;
> -			bw = c2h->ra_report.bw;
> 
>  			if (rate < DESC_RATE_MCS0) {
>  				rarpt->txrate.legacy =
> @@ -5586,8 +5585,14 @@ static void rtl8xxxu_c2hcmd_callback(struct work_struct *work)
>  						RATE_INFO_FLAGS_SHORT_GI;
>  				}
> 
> -				if (bw == RATE_INFO_BW_20)
> -					rarpt->txrate.bw |= RATE_INFO_BW_20;
> +				if (skb->len >= offsetof(typeof(*c2h), ra_report.bw) +
> +						sizeof(c2h->ra_report.bw)) {

Today, I look up if we have a better choice, and offsetofend() can make thing simple.

if (skb->len >= offsetofend(typeof(*c2h), ra_report.bw))

Sorry for the late.

> +					if (c2h->ra_report.bw == RTL8XXXU_CHANNEL_WIDTH_40)
> +						bw = RATE_INFO_BW_40;
> +					else
> +						bw = RATE_INFO_BW_20;
> +					rarpt->txrate.bw = bw;
> +				}
>  			}
>  			bit_rate = cfg80211_calculate_bitrate(&rarpt->txrate);
>  			rarpt->bit_rate = bit_rate;
> --
> 2.38.0
> 
> ------Please consider the environment before printing this e-mail.

Re: [PATCH v3 3/3] wifi: rtl8xxxu: Introduce rtl8xxxu_update_ra_report

[JunASAKA]

Hi,
I am trying to merge this patch into the linux source tree (Mr. Torvalds' updated source tree) and find a confliection in patch 3/3.
I think you should move the rcu_read_unlock() that originally located in line 4657 to line 4682(right after the if-else section)
to make this patch work normally.

Jun ASAKA.

Re: [PATCH v3 2/3] wifi: rtl8xxxu: Fix the channel width reporting

[Bitterblue Smith]

On 30/11/2022 02:38, Ping-Ke Shih wrote:
> 
> 
>> -----Original Message-----
>> From: Bitterblue Smith <rtl8821cerfe2@gmail.com>
>> Sent: Wednesday, November 30, 2022 3:06 AM
>> To: linux-wireless@vger.kernel.org
>> Cc: Jes Sorensen <Jes.Sorensen@gmail.com>; Ping-Ke Shih <pkshih@realtek.com>
>> Subject: [PATCH v3 2/3] wifi: rtl8xxxu: Fix the channel width reporting
>>
>> The gen 2 chips RTL8192EU and RTL8188FU periodically send the driver
>> reports about the TX rate, and the driver passes these reports to
>> sta_statistics. The reports from RTL8192EU may or may not include the
>> channel width. The reports from RTL8188FU do not include it.
>>
>> Only access the c2h->ra_report.bw field if the report (skb) is big
>> enough.
>>
>> The other problem fixed here is that the code was actually never
>> changing the channel width initially reported by
>> rtl8xxxu_bss_info_changed because the value of RATE_INFO_BW_20 is 0.
>>
>> Fixes: 0985d3a410ac ("rtl8xxxu: Feed current txrate information for mac80211")
>> Signed-off-by: Bitterblue Smith <rtl8821cerfe2@gmail.com>
>> ---
>> v3:
>>  - Don't assume bw will always be 1 byte.
>>
>> v2:
>>  - Eliminate the magic numbers.
>> ---
>>  drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c | 11 ++++++++---
>>  1 file changed, 8 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c
>> b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c
>> index 28f136064297..9799bc5ed60a 100644
>> --- a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c
>> +++ b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c
>> @@ -5569,7 +5569,6 @@ static void rtl8xxxu_c2hcmd_callback(struct work_struct *work)
>>  			rarpt->txrate.flags = 0;
>>  			rate = c2h->ra_report.rate;
>>  			sgi = c2h->ra_report.sgi;
>> -			bw = c2h->ra_report.bw;
>>
>>  			if (rate < DESC_RATE_MCS0) {
>>  				rarpt->txrate.legacy =
>> @@ -5586,8 +5585,14 @@ static void rtl8xxxu_c2hcmd_callback(struct work_struct *work)
>>  						RATE_INFO_FLAGS_SHORT_GI;
>>  				}
>>
>> -				if (bw == RATE_INFO_BW_20)
>> -					rarpt->txrate.bw |= RATE_INFO_BW_20;
>> +				if (skb->len >= offsetof(typeof(*c2h), ra_report.bw) +
>> +						sizeof(c2h->ra_report.bw)) {
> 
> Today, I look up if we have a better choice, and offsetofend() can make thing simple.
> 
> if (skb->len >= offsetofend(typeof(*c2h), ra_report.bw))
> 
> Sorry for the late.
> 

Even better. Thank you.

Re: [PATCH v3 3/3] wifi: rtl8xxxu: Introduce rtl8xxxu_update_ra_report

[Bitterblue Smith]

On 30/11/2022 15:47, JunASAKA wrote:
> Hi,
> I am trying to merge this patch into the linux source tree (Mr. Torvalds' updated source tree) and find a confliection in patch 3/3.
> I think you should move the rcu_read_unlock() that originally located in line 4657 to line 4682(right after the if-else section)
> to make this patch work normally.
> 
> Jun ASAKA.

This patch set is meant to be applied on top of the wireless-next tree:
https://git.kernel.org/pub/scm/linux/kernel/git/wireless/wireless-next.git
That tree contains a commit which is not yet found in other trees:
7927afb5e27b ("wifi: rtl8xxxu: Fix use after rcu_read_unlock in rtl8xxxu_bss_info_changed")

Re: [PATCH v3 3/3] wifi: rtl8xxxu: Introduce rtl8xxxu_update_ra_report

[Jun ASAKA]

On 30/11/2022 11:39 pm, Bitterblue Smith wrote:
> On 30/11/2022 15:47, JunASAKA wrote:
>> Hi,
>> I am trying to merge this patch into the linux source tree (Mr. Torvalds' updated source tree) and find a confliection in patch 3/3.
>> I think you should move the rcu_read_unlock() that originally located in line 4657 to line 4682(right after the if-else section)
>> to make this patch work normally.
>>
>> Jun ASAKA.
> This patch set is meant to be applied on top of the wireless-next tree:
> https://git.kernel.org/pub/scm/linux/kernel/git/wireless/wireless-next.git
> That tree contains a commit which is not yet found in other trees:
> 7927afb5e27b ("wifi: rtl8xxxu: Fix use after rcu_read_unlock in rtl8xxxu_bss_info_changed")

Oh I see, sorry for my ignorant.


Jun ASAKA.