[PATCH v2 1/2] rtw88: follow the AP basic rates for tx mgmt frame

[PATCH v2 1/2] rtw88: follow the AP basic rates for tx mgmt frame

[Ping-Ke Shih]

From: Yu-Yen Ting <steventing@realtek.com>

By default the driver uses the 1M and 6M rate for managemnt frames
in 2G and 5G bands respectively. But when the basic rates is
configured from the mac80211, we need to send the management frames
according to the basic rates.

This commit makes the driver use the lowest basic rates to send
the management frames.

Signed-off-by: Yu-Yen Ting <steventing@realtek.com>
Signed-off-by: Ping-Ke Shih <pkshih@realtek.com>
---
v2: move debugfs as a separated patch
v1: the original patch is "[PATCH 2/7] rtw88: follow the AP basic rates for tx mgmt frame"
---
 drivers/net/wireless/realtek/rtw88/tx.c | 26 ++++++++++++++++++++-----
 1 file changed, 21 insertions(+), 5 deletions(-)

diff --git a/drivers/net/wireless/realtek/rtw88/tx.c b/drivers/net/wireless/realtek/rtw88/tx.c
index 0193708fc013..e5949a775283 100644
--- a/drivers/net/wireless/realtek/rtw88/tx.c
+++ b/drivers/net/wireless/realtek/rtw88/tx.c
@@ -233,17 +233,33 @@ void rtw_tx_report_handle(struct rtw_dev *rtwdev, struct sk_buff *skb, int src)
 	spin_unlock_irqrestore(&tx_report->q_lock, flags);
 }
 
+static u8 rtw_get_mgmt_rate(struct rtw_dev *rtwdev, struct sk_buff *skb,
+			    u8 lowest_rate, bool ignore_rate)
+{
+	struct ieee80211_tx_info *tx_info = IEEE80211_SKB_CB(skb);
+	struct ieee80211_vif *vif = tx_info->control.vif;
+
+	if (!vif || !vif->bss_conf.basic_rates || ignore_rate)
+		return lowest_rate;
+
+	return __ffs(vif->bss_conf.basic_rates) + lowest_rate;
+}
+
 static void rtw_tx_pkt_info_update_rate(struct rtw_dev *rtwdev,
 					struct rtw_tx_pkt_info *pkt_info,
-					struct sk_buff *skb)
+					struct sk_buff *skb,
+					bool ignore_rate)
 {
 	if (rtwdev->hal.current_band_type == RTW_BAND_2G) {
 		pkt_info->rate_id = RTW_RATEID_B_20M;
-		pkt_info->rate = DESC_RATE1M;
+		pkt_info->rate = rtw_get_mgmt_rate(rtwdev, skb, DESC_RATE1M,
+						   ignore_rate);
 	} else {
 		pkt_info->rate_id = RTW_RATEID_G;
-		pkt_info->rate = DESC_RATE6M;
+		pkt_info->rate = rtw_get_mgmt_rate(rtwdev, skb, DESC_RATE6M,
+						   ignore_rate);
 	}
+
 	pkt_info->use_rate = true;
 	pkt_info->dis_rate_fallback = true;
 }
@@ -280,7 +296,7 @@ static void rtw_tx_mgmt_pkt_info_update(struct rtw_dev *rtwdev,
 					struct ieee80211_sta *sta,
 					struct sk_buff *skb)
 {
-	rtw_tx_pkt_info_update_rate(rtwdev, pkt_info, skb);
+	rtw_tx_pkt_info_update_rate(rtwdev, pkt_info, skb, false);
 	pkt_info->dis_qselseq = true;
 	pkt_info->en_hwseq = true;
 	pkt_info->hw_ssn_sel = 0;
@@ -404,7 +420,7 @@ void rtw_tx_rsvd_page_pkt_info_update(struct rtw_dev *rtwdev,
 	if (type != RSVD_BEACON && type != RSVD_DUMMY)
 		pkt_info->qsel = TX_DESC_QSEL_MGMT;
 
-	rtw_tx_pkt_info_update_rate(rtwdev, pkt_info, skb);
+	rtw_tx_pkt_info_update_rate(rtwdev, pkt_info, skb, true);
 
 	bmc = is_broadcast_ether_addr(hdr->addr1) ||
 	      is_multicast_ether_addr(hdr->addr1);
-- 
2.21.0

[PATCH v2 2/2] rtw88: add debugfs to force lowest basic rate

[Ping-Ke Shih]

From: Yu-Yen Ting <steventing@realtek.com>

The management frame with high rate e.g. 24M may not be transmitted
smoothly in long range environment.
Add a debugfs to force to use the lowest basic rate
in order to debug the reachability of transmitting management frame.

obtain current setting
cat /sys/kernel/debug/ieee80211/phyX/rtw88/basic_rates

force lowest rate:
echo 1 > /sys/kernel/debug/ieee80211/phyX/rtw88/basic_rates

Signed-off-by: Yu-Yen Ting <steventing@realtek.com>
Signed-off-by: Ping-Ke Shih <pkshih@realtek.com>
---
 drivers/net/wireless/realtek/rtw88/debug.c | 39 ++++++++++++++++++++++
 drivers/net/wireless/realtek/rtw88/main.h  |  1 +
 drivers/net/wireless/realtek/rtw88/tx.c    |  3 +-
 3 files changed, 42 insertions(+), 1 deletion(-)

diff --git a/drivers/net/wireless/realtek/rtw88/debug.c b/drivers/net/wireless/realtek/rtw88/debug.c
index 18ab472ea46c..a99a3dc6f9f9 100644
--- a/drivers/net/wireless/realtek/rtw88/debug.c
+++ b/drivers/net/wireless/realtek/rtw88/debug.c
@@ -861,6 +861,39 @@ static int rtw_debugfs_get_fw_crash(struct seq_file *m, void *v)
 	return 0;
 }
 
+static ssize_t rtw_debugfs_set_basic_rates(struct file *filp,
+					   const char __user *buffer,
+					   size_t count, loff_t *loff)
+{
+	struct seq_file *seqpriv = (struct seq_file *)filp->private_data;
+	struct rtw_debugfs_priv *debugfs_priv = seqpriv->private;
+	struct rtw_dev *rtwdev = debugfs_priv->rtwdev;
+	bool input;
+	int err;
+
+	err = kstrtobool_from_user(buffer, count, &input);
+	if (err)
+		return err;
+
+	if (input)
+		set_bit(RTW_FLAG_USE_LOWEST_RATE, rtwdev->flags);
+	else
+		clear_bit(RTW_FLAG_USE_LOWEST_RATE, rtwdev->flags);
+
+	return count;
+}
+
+static int rtw_debugfs_get_basic_rates(struct seq_file *m, void *v)
+{
+	struct rtw_debugfs_priv *debugfs_priv = m->private;
+	struct rtw_dev *rtwdev = debugfs_priv->rtwdev;
+
+	seq_printf(m, "use lowest: %d\n",
+		   test_bit(RTW_FLAG_USE_LOWEST_RATE, rtwdev->flags));
+
+	return 0;
+}
+
 static ssize_t rtw_debugfs_set_dm_cap(struct file *filp,
 				      const char __user *buffer,
 				      size_t count, loff_t *loff)
@@ -1046,6 +1079,11 @@ static struct rtw_debugfs_priv rtw_debug_priv_fw_crash = {
 	.cb_read = rtw_debugfs_get_fw_crash,
 };
 
+static struct rtw_debugfs_priv rtw_debug_priv_basic_rates = {
+	.cb_write = rtw_debugfs_set_basic_rates,
+	.cb_read = rtw_debugfs_get_basic_rates,
+};
+
 static struct rtw_debugfs_priv rtw_debug_priv_dm_cap = {
 	.cb_write = rtw_debugfs_set_dm_cap,
 	.cb_read = rtw_debugfs_get_dm_cap,
@@ -1125,6 +1163,7 @@ void rtw_debugfs_init(struct rtw_dev *rtwdev)
 	rtw_debugfs_add_r(rf_dump);
 	rtw_debugfs_add_r(tx_pwr_tbl);
 	rtw_debugfs_add_rw(fw_crash);
+	rtw_debugfs_add_rw(basic_rates);
 	rtw_debugfs_add_rw(dm_cap);
 }
 
diff --git a/drivers/net/wireless/realtek/rtw88/main.h b/drivers/net/wireless/realtek/rtw88/main.h
index dc3744847ba9..0df5df3a62ab 100644
--- a/drivers/net/wireless/realtek/rtw88/main.h
+++ b/drivers/net/wireless/realtek/rtw88/main.h
@@ -362,6 +362,7 @@ enum rtw_flags {
 	RTW_FLAG_BUSY_TRAFFIC,
 	RTW_FLAG_WOWLAN,
 	RTW_FLAG_RESTARTING,
+	RTW_FLAG_USE_LOWEST_RATE,
 
 	NUM_OF_RTW_FLAGS,
 };
diff --git a/drivers/net/wireless/realtek/rtw88/tx.c b/drivers/net/wireless/realtek/rtw88/tx.c
index e5949a775283..0aeed15736c8 100644
--- a/drivers/net/wireless/realtek/rtw88/tx.c
+++ b/drivers/net/wireless/realtek/rtw88/tx.c
@@ -238,8 +238,9 @@ static u8 rtw_get_mgmt_rate(struct rtw_dev *rtwdev, struct sk_buff *skb,
 {
 	struct ieee80211_tx_info *tx_info = IEEE80211_SKB_CB(skb);
 	struct ieee80211_vif *vif = tx_info->control.vif;
+	bool use_lowest = test_bit(RTW_FLAG_USE_LOWEST_RATE, rtwdev->flags);
 
-	if (!vif || !vif->bss_conf.basic_rates || ignore_rate)
+	if (!vif || !vif->bss_conf.basic_rates || ignore_rate || use_lowest)
 		return lowest_rate;
 
 	return __ffs(vif->bss_conf.basic_rates) + lowest_rate;
-- 
2.21.0

RE: [PATCH v2 1/2] rtw88: follow the AP basic rates for tx mgmt frame

[Pkshih]

> -----Original Message-----
> From: Ping-Ke Shih [mailto:pkshih@realtek.com]
> Sent: Thursday, April 22, 2021 11:04 AM
> To: tony0620emma@gmail.com; kvalo@codeaurora.org
> Cc: linux-wireless@vger.kernel.org; Steven Ting
> Subject: [PATCH v2 1/2] rtw88: follow the AP basic rates for tx mgmt frame
> 
> From: Yu-Yen Ting <steventing@realtek.com>
> 
> By default the driver uses the 1M and 6M rate for managemnt frames
> in 2G and 5G bands respectively. But when the basic rates is
> configured from the mac80211, we need to send the management frames
> according to the basic rates.
> 
> This commit makes the driver use the lowest basic rates to send
> the management frames.
> 
> Signed-off-by: Yu-Yen Ting <steventing@realtek.com>
> Signed-off-by: Ping-Ke Shih <pkshih@realtek.com>
> ---
> v2: move debugfs as a separated patch
> v1: the original patch is "[PATCH 2/7] rtw88: follow the AP basic rates for tx mgmt frame"
> ---

[...]

Gentle ping.
The patchset v2 is separated into two patches. Is there any further suggestion?

--
Ping-Ke

Re: [PATCH v2 1/2] rtw88: follow the AP basic rates for tx mgmt frame

[Brian Norris]

On Thu, Apr 22, 2021 at 11:04:12AM +0800, Ping-Ke Shih wrote:
> From: Yu-Yen Ting <steventing@realtek.com>
> 
> By default the driver uses the 1M and 6M rate for managemnt frames
> in 2G and 5G bands respectively. But when the basic rates is
> configured from the mac80211, we need to send the management frames
> according to the basic rates.
> 
> This commit makes the driver use the lowest basic rates to send
> the management frames.
> 
> Signed-off-by: Yu-Yen Ting <steventing@realtek.com>
> Signed-off-by: Ping-Ke Shih <pkshih@realtek.com>
> ---
> v2: move debugfs as a separated patch
> v1: the original patch is "[PATCH 2/7] rtw88: follow the AP basic rates for tx mgmt frame"

FWIW:

Reviewed-by: Brian Norris <briannorris@chromium.org>
Tested-by: Brian Norris <briannorris@chromium.org>

Re: [PATCH v2 2/2] rtw88: add debugfs to force lowest basic rate

[Brian Norris]

On Thu, Apr 22, 2021 at 11:04:13AM +0800, Ping-Ke Shih wrote:
> From: Yu-Yen Ting <steventing@realtek.com>
> 
> The management frame with high rate e.g. 24M may not be transmitted
> smoothly in long range environment.
> Add a debugfs to force to use the lowest basic rate
> in order to debug the reachability of transmitting management frame.
> 
> obtain current setting
> cat /sys/kernel/debug/ieee80211/phyX/rtw88/basic_rates
> 
> force lowest rate:
> echo 1 > /sys/kernel/debug/ieee80211/phyX/rtw88/basic_rates
> 
> Signed-off-by: Yu-Yen Ting <steventing@realtek.com>
> Signed-off-by: Ping-Ke Shih <pkshih@realtek.com>

I believe some initial objection to this was because it was unclear if
this is for "production" use (e.g., recommending distros to play with
this) or for debugging. I'll admit, I requested the feature for patch 1,
because I've seen that for those networks where people *do* configure
odd Basic Rates, they intend for stations to follow those, and not use
the lowest (and most airtime-hogging) rates.

And I can say, I don't see why distributions should be turning that back
off. If the Basic Rates setting is wrong, then the that's up to the
network admin to fix.

All that is to say: I agree that this patch is purely for debugging, as
stated, and that it belongs in debugfs. I also maintain a distribution,
and I don't plan on using this beyond debugging.

Therefore:

Reviewed-by: Brian Norris <briannorris@chromium.org>

BTW, if we have clear guidelines on debugfs, module parameters, etc.,
maybe those should be going on the wiki? I know this came up before:

https://lore.kernel.org/linux-wireless/87d09u7tyr.fsf@codeaurora.org/

At this point, I'm willing to write such guidelines, if I get an ack
from the relevant folks (I guess that's just Kalle?). It probably
belongs somewhere in this tree:

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

similar to this:
https://wireless.wiki.kernel.org/en/developers/documentation/nl80211#vendor-specific_api
except it's not really an nl80211 thing. Suggestions welcome.

Side note: it could really use some cleanup -- like this page:
https://wireless.wiki.kernel.org/en/developers/process

Brian

wireless: guidelines for user space interfaces

[Kalle Valo]

(changing subject, was "Re: [PATCH v2 2/2] rtw88: add debugfs to force lowest basic rate")

Brian Norris <briannorris@chromium.org> writes:

> BTW, if we have clear guidelines on debugfs, module parameters, etc.,
> maybe those should be going on the wiki? I know this came up before:
>
> https://lore.kernel.org/linux-wireless/87d09u7tyr.fsf@codeaurora.org/
>
> At this point, I'm willing to write such guidelines, if I get an ack
> from the relevant folks (I guess that's just Kalle?). It probably
> belongs somewhere in this tree:
>
> https://wireless.wiki.kernel.org/en/developers/documentation
>
> similar to this:
> https://wireless.wiki.kernel.org/en/developers/documentation/nl80211#vendor-specific_api
> except it's not really an nl80211 thing. Suggestions welcome.

I think this is a very good idea. Having general guidelines for wireless
drivers using user space interfaces would help both people submitting
patches and also people like me reviewing the patches.

We should try to get an ack for the guidelines at least from Johannes,
but I would prefer also involve Jakub and Dave (CCed) as they might have
some input from the network subsystem point of view.

Just to get this started, here's a draft list I came up of different
user space interfaces upstream wireless drivers are using:

* generic nl80211 (excluding testmode and vendor commands)

* nl80211 testmode commands

* nl80211 vendor commands

* sysfs[1]

* debugfs

* relayfs

* configfs[1]

* module parameters

* thermal subsystem

* firmware_request()

I'm not saying that we need to document all these in the first version,
I'm just trying to come up a comprehensive overview how wireless drivers
interact with the user space. And I'm sure I missed something. so please
do fill in.

> Side note: it could really use some cleanup -- like this page:
> https://wireless.wiki.kernel.org/en/developers/process

Heh, that is old information. TBH in practise I maintain only the
submittingpatches page (link in the signature), other pages I rarely
touch. And naturally I also look after ath10k and ath11k pages.

Any volunteers to clean that up?

[1] Actually I don't know if there are any valid use cases for sysfs and
    configfs at the moment, but I'll include them in the list for
    completeness.

-- 
https://patchwork.kernel.org/project/linux-wireless/list/

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

Re: wireless: guidelines for user space interfaces

[Arend van Spriel]

[-- Attachment #1: Type: text/plain, Size: 3501 bytes --]

On 10/11/2021 11:23 AM, Kalle Valo wrote:
> (changing subject, was "Re: [PATCH v2 2/2] rtw88: add debugfs to force lowest basic rate")
> 
> Brian Norris <briannorris@chromium.org> writes:
> 
>> BTW, if we have clear guidelines on debugfs, module parameters, etc.,
>> maybe those should be going on the wiki? I know this came up before:
>>
>> https://lore.kernel.org/linux-wireless/87d09u7tyr.fsf@codeaurora.org/
>>
>> At this point, I'm willing to write such guidelines, if I get an ack
>> from the relevant folks (I guess that's just Kalle?). It probably
>> belongs somewhere in this tree:
>>
>> https://wireless.wiki.kernel.org/en/developers/documentation
>>
>> similar to this:
>> https://wireless.wiki.kernel.org/en/developers/documentation/nl80211#vendor-specific_api
>> except it's not really an nl80211 thing. Suggestions welcome.
> 
> I think this is a very good idea. Having general guidelines for wireless
> drivers using user space interfaces would help both people submitting
> patches and also people like me reviewing the patches.
> 
> We should try to get an ack for the guidelines at least from Johannes,
> but I would prefer also involve Jakub and Dave (CCed) as they might have
> some input from the network subsystem point of view.
> 
> Just to get this started, here's a draft list I came up of different
> user space interfaces upstream wireless drivers are using:
> 
> * generic nl80211 (excluding testmode and vendor commands)
> 
> * nl80211 testmode commands
> 
> * nl80211 vendor commands
> 
> * sysfs[1]
> 
> * debugfs
> 
> * relayfs
> 
> * configfs[1]
> 
> * module parameters
> 
> * thermal subsystem
> 
> * firmware_request()
> 
> I'm not saying that we need to document all these in the first version,
> I'm just trying to come up a comprehensive overview how wireless drivers
> interact with the user space. And I'm sure I missed something. so please
> do fill in.

Not sure if all of the above can be considered user-space interfaces, 
but wireless driver developers could benefit from guidelines for them 
regardless. Maybe following needs to be considered as well although I 
think cfg80211 is taking care of it:

* rfkill

Regards,
Arend

>> Side note: it could really use some cleanup -- like this page:
>> https://wireless.wiki.kernel.org/en/developers/process
> 
> Heh, that is old information. TBH in practise I maintain only the
> submittingpatches page (link in the signature), other pages I rarely
> touch. And naturally I also look after ath10k and ath11k pages.
> 
> Any volunteers to clean that up?
> 
> [1] Actually I don't know if there are any valid use cases for sysfs and
>      configfs at the moment, but I'll include them in the list for
>      completeness.
> 

-- 
This electronic communication and the information and any files transmitted 
with it, or attached to it, are confidential and are intended solely for 
the use of the individual or entity to whom it is addressed and may contain 
information that is confidential, legally privileged, protected by privacy 
laws, or otherwise restricted from disclosure to anyone else. If you are 
not the intended recipient or the person responsible for delivering the 
e-mail to the intended recipient, you are hereby notified that any use, 
copying, distributing, dissemination, forwarding, printing, or copying of 
this e-mail is strictly prohibited. If you received this e-mail in error, 
please return the e-mail to the sender, delete it from your computer, and 
destroy any printed copy of it.

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4219 bytes --]

Re: wireless: guidelines for user space interfaces

[Kalle Valo]

Arend van Spriel <arend.vanspriel@broadcom.com> writes:

> On 10/11/2021 11:23 AM, Kalle Valo wrote:
>> (changing subject, was "Re: [PATCH v2 2/2] rtw88: add debugfs to
>> force lowest basic rate")
>>
>> Brian Norris <briannorris@chromium.org> writes:
>>
>>> BTW, if we have clear guidelines on debugfs, module parameters, etc.,
>>> maybe those should be going on the wiki? I know this came up before:
>>>
>>> https://lore.kernel.org/linux-wireless/87d09u7tyr.fsf@codeaurora.org/
>>>
>>> At this point, I'm willing to write such guidelines, if I get an ack
>>> from the relevant folks (I guess that's just Kalle?). It probably
>>> belongs somewhere in this tree:
>>>
>>> https://wireless.wiki.kernel.org/en/developers/documentation
>>>
>>> similar to this:
>>> https://wireless.wiki.kernel.org/en/developers/documentation/nl80211#vendor-specific_api
>>> except it's not really an nl80211 thing. Suggestions welcome.
>>
>> I think this is a very good idea. Having general guidelines for wireless
>> drivers using user space interfaces would help both people submitting
>> patches and also people like me reviewing the patches.
>>
>> We should try to get an ack for the guidelines at least from Johannes,
>> but I would prefer also involve Jakub and Dave (CCed) as they might have
>> some input from the network subsystem point of view.
>>
>> Just to get this started, here's a draft list I came up of different
>> user space interfaces upstream wireless drivers are using:
>>
>> * generic nl80211 (excluding testmode and vendor commands)
>>
>> * nl80211 testmode commands
>>
>> * nl80211 vendor commands
>>
>> * sysfs[1]
>>
>> * debugfs
>>
>> * relayfs
>>
>> * configfs[1]
>>
>> * module parameters
>>
>> * thermal subsystem
>>
>> * firmware_request()
>>
>> I'm not saying that we need to document all these in the first version,
>> I'm just trying to come up a comprehensive overview how wireless drivers
>> interact with the user space. And I'm sure I missed something. so please
>> do fill in.
>
> Not sure if all of the above can be considered user-space interfaces,
> but wireless driver developers could benefit from guidelines for them
> regardless.

Maybe the name should be "Guidelines for wireless drivers" without being
too specific?

> Maybe following needs to be considered as well although I think
> cfg80211 is taking care of it:
>
> * rfkill

Yeah, it would be good to include rfkill.

-- 
https://patchwork.kernel.org/project/linux-wireless/list/

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

Re: [PATCH v2 2/2] rtw88: add debugfs to force lowest basic rate

[Kalle Valo]

(replying to an old thread:

https://patchwork.kernel.org/project/linux-wireless/patch/20210422030413.9738-2-pkshih@realtek.com/ )


Brian Norris <briannorris@chromium.org> writes:

> On Thu, Apr 22, 2021 at 11:04:13AM +0800, Ping-Ke Shih wrote:
>> From: Yu-Yen Ting <steventing@realtek.com>
>> 
>> The management frame with high rate e.g. 24M may not be transmitted
>> smoothly in long range environment.
>> Add a debugfs to force to use the lowest basic rate
>> in order to debug the reachability of transmitting management frame.
>> 
>> obtain current setting
>> cat /sys/kernel/debug/ieee80211/phyX/rtw88/basic_rates
>> 
>> force lowest rate:
>> echo 1 > /sys/kernel/debug/ieee80211/phyX/rtw88/basic_rates
>> 
>> Signed-off-by: Yu-Yen Ting <steventing@realtek.com>
>> Signed-off-by: Ping-Ke Shih <pkshih@realtek.com>
>
> I believe some initial objection to this was because it was unclear if
> this is for "production" use (e.g., recommending distros to play with
> this) or for debugging. I'll admit, I requested the feature for patch 1,
> because I've seen that for those networks where people *do* configure
> odd Basic Rates, they intend for stations to follow those, and not use
> the lowest (and most airtime-hogging) rates.
>
> And I can say, I don't see why distributions should be turning that back
> off. If the Basic Rates setting is wrong, then the that's up to the
> network admin to fix.
>
> All that is to say: I agree that this patch is purely for debugging, as
> stated, and that it belongs in debugfs. I also maintain a distribution,
> and I don't plan on using this beyond debugging.
>
> Therefore:
>
> Reviewed-by: Brian Norris <briannorris@chromium.org>

Ok, fair enough as long as this will not end up normal users using it. I
still would prefer to have extensive bitrate handling via nl80211 but
clearly it's not going anywhere.

But could the debugfs filename be more descriptive, for example
force_basic_rates or something like that?

-- 
https://patchwork.kernel.org/project/linux-wireless/list/

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

RE: [PATCH v2 2/2] rtw88: add debugfs to force lowest basic rate

[Pkshih]

> -----Original Message-----
> From: kvalo=codeaurora.org@mg.codeaurora.org <kvalo=codeaurora.org@mg.codeaurora.org> On Behalf Of Kalle
> Valo
> Sent: Monday, November 1, 2021 7:01 PM
> To: Brian Norris <briannorris@chromium.org>
> Cc: Pkshih <pkshih@realtek.com>; tony0620emma@gmail.com; linux-wireless@vger.kernel.org;
> steventing@realtek.com
> Subject: Re: [PATCH v2 2/2] rtw88: add debugfs to force lowest basic rate
> 
> (replying to an old thread:
> 
> https://patchwork.kernel.org/project/linux-wireless/patch/20210422030413.9738-2-pkshih@realtek.com/ )
> 
> 
> Brian Norris <briannorris@chromium.org> writes:
> 
> > On Thu, Apr 22, 2021 at 11:04:13AM +0800, Ping-Ke Shih wrote:
> >> From: Yu-Yen Ting <steventing@realtek.com>
> >>
> >> The management frame with high rate e.g. 24M may not be transmitted
> >> smoothly in long range environment.
> >> Add a debugfs to force to use the lowest basic rate
> >> in order to debug the reachability of transmitting management frame.
> >>
> >> obtain current setting
> >> cat /sys/kernel/debug/ieee80211/phyX/rtw88/basic_rates
> >>
> >> force lowest rate:
> >> echo 1 > /sys/kernel/debug/ieee80211/phyX/rtw88/basic_rates
> >>
> >> Signed-off-by: Yu-Yen Ting <steventing@realtek.com>
> >> Signed-off-by: Ping-Ke Shih <pkshih@realtek.com>
> >
> > I believe some initial objection to this was because it was unclear if
> > this is for "production" use (e.g., recommending distros to play with
> > this) or for debugging. I'll admit, I requested the feature for patch 1,
> > because I've seen that for those networks where people *do* configure
> > odd Basic Rates, they intend for stations to follow those, and not use
> > the lowest (and most airtime-hogging) rates.
> >
> > And I can say, I don't see why distributions should be turning that back
> > off. If the Basic Rates setting is wrong, then the that's up to the
> > network admin to fix.
> >
> > All that is to say: I agree that this patch is purely for debugging, as
> > stated, and that it belongs in debugfs. I also maintain a distribution,
> > and I don't plan on using this beyond debugging.
> >
> > Therefore:
> >
> > Reviewed-by: Brian Norris <briannorris@chromium.org>
> 
> Ok, fair enough as long as this will not end up normal users using it. I
> still would prefer to have extensive bitrate handling via nl80211 but
> clearly it's not going anywhere.
> 
> But could the debugfs filename be more descriptive, for example
> force_basic_rates or something like that?
> 

OK. Use 'force_lowest_basic_rate' by v3 [1].

[1] https://lore.kernel.org/linux-wireless/20211102022454.10944-1-pkshih@realtek.com/T/#t

--
Ping-Ke