[PATCH 2/3] wifi: rt2x00: disable RTS threshold for rt2800 by default

[PATCH 2/3] wifi: rt2x00: disable RTS threshold for rt2800 by default

[Shiji Yang]

Disable the RTS threshold for OFDM and CCK rates by default as the
initial RTS threshold is 'IEEE80211_MAX_RTS_THRESHOLD'. And RTS
thresholds for all other rates have already been disabled when init.

Signed-off-by: Shiji Yang <yangshiji66@outlook.com>
---
 drivers/net/wireless/ralink/rt2x00/rt2800lib.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/wireless/ralink/rt2x00/rt2800lib.c b/drivers/net/wireless/ralink/rt2x00/rt2800lib.c
index 6ca2f2c23..bcc63f1cb 100644
--- a/drivers/net/wireless/ralink/rt2x00/rt2800lib.c
+++ b/drivers/net/wireless/ralink/rt2x00/rt2800lib.c
@@ -6104,7 +6104,7 @@ static int rt2800_init_registers(struct rt2x00_dev *rt2x00dev)
 	rt2x00_set_field32(&reg, CCK_PROT_CFG_TX_OP_ALLOW_MM40, 0);
 	rt2x00_set_field32(&reg, CCK_PROT_CFG_TX_OP_ALLOW_GF20, 1);
 	rt2x00_set_field32(&reg, CCK_PROT_CFG_TX_OP_ALLOW_GF40, 0);
-	rt2x00_set_field32(&reg, CCK_PROT_CFG_RTS_TH_EN, 1);
+	rt2x00_set_field32(&reg, CCK_PROT_CFG_RTS_TH_EN, 0);
 	rt2800_register_write(rt2x00dev, CCK_PROT_CFG, reg);
 
 	reg = rt2800_register_read(rt2x00dev, OFDM_PROT_CFG);
@@ -6117,7 +6117,7 @@ static int rt2800_init_registers(struct rt2x00_dev *rt2x00dev)
 	rt2x00_set_field32(&reg, OFDM_PROT_CFG_TX_OP_ALLOW_MM40, 0);
 	rt2x00_set_field32(&reg, OFDM_PROT_CFG_TX_OP_ALLOW_GF20, 1);
 	rt2x00_set_field32(&reg, OFDM_PROT_CFG_TX_OP_ALLOW_GF40, 0);
-	rt2x00_set_field32(&reg, OFDM_PROT_CFG_RTS_TH_EN, 1);
+	rt2x00_set_field32(&reg, OFDM_PROT_CFG_RTS_TH_EN, 0);
 	rt2800_register_write(rt2x00dev, OFDM_PROT_CFG, reg);
 
 	reg = rt2800_register_read(rt2x00dev, MM20_PROT_CFG);
-- 
2.39.2

[PATCH 3/3] wifi: rt2x00: restart beacon queue when hardware reset

[Shiji Yang]

When a hardware reset is triggered, all registers are reset, so all
queues are forced to stop in hardware interface. However, mac80211
will not automatically stop the queue. If we don't manually stop the
beacon queue, the queue will be deadlocked and unable to start again.
This patch fixes the issue where Apple devices cannot connect to the
AP after calling ieee80211_restart_hw().

Signed-off-by: Shiji Yang <yangshiji66@outlook.com>
---
 drivers/net/wireless/ralink/rt2x00/rt2x00dev.c | 3 +++
 drivers/net/wireless/ralink/rt2x00/rt2x00mac.c | 4 +++-
 2 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/net/wireless/ralink/rt2x00/rt2x00dev.c b/drivers/net/wireless/ralink/rt2x00/rt2x00dev.c
index 9a9cfd0ce..ac58a56c3 100644
--- a/drivers/net/wireless/ralink/rt2x00/rt2x00dev.c
+++ b/drivers/net/wireless/ralink/rt2x00/rt2x00dev.c
@@ -101,6 +101,8 @@ void rt2x00lib_disable_radio(struct rt2x00_dev *rt2x00dev)
 	rt2x00link_stop_tuner(rt2x00dev);
 	rt2x00queue_stop_queues(rt2x00dev);
 	rt2x00queue_flush_queues(rt2x00dev, true);
+	if (test_bit(DEVICE_STATE_RESET, &rt2x00dev->flags))
+		rt2x00queue_stop_queue(rt2x00dev->bcn);
 
 	/*
 	 * Disable radio.
@@ -1286,6 +1288,7 @@ int rt2x00lib_start(struct rt2x00_dev *rt2x00dev)
 	rt2x00dev->intf_ap_count = 0;
 	rt2x00dev->intf_sta_count = 0;
 	rt2x00dev->intf_associated = 0;
+	rt2x00dev->intf_beaconing = 0;
 
 	/* Enable the radio */
 	retval = rt2x00lib_enable_radio(rt2x00dev);
diff --git a/drivers/net/wireless/ralink/rt2x00/rt2x00mac.c b/drivers/net/wireless/ralink/rt2x00/rt2x00mac.c
index 4202c6517..6fcbf534a 100644
--- a/drivers/net/wireless/ralink/rt2x00/rt2x00mac.c
+++ b/drivers/net/wireless/ralink/rt2x00/rt2x00mac.c
@@ -615,7 +615,9 @@ void rt2x00mac_bss_info_changed(struct ieee80211_hw *hw,
 			 * and keep it running on other interfaces.
 			 */
 			rt2x00queue_clear_beacon(rt2x00dev, vif);
-		} else if (bss_conf->enable_beacon && !intf->enable_beacon) {
+		} else if (bss_conf->enable_beacon &&
+			   (!intf->enable_beacon ||
+			    test_bit(DEVICE_STATE_RESET, &rt2x00dev->flags))) {
 			rt2x00dev->intf_beaconing++;
 			intf->enable_beacon = true;
 			/*
-- 
2.39.2

Re: [PATCH 2/3] wifi: rt2x00: disable RTS threshold for rt2800 by default

[Kalle Valo]

Shiji Yang <yangshiji66@outlook.com> writes:

> Disable the RTS threshold for OFDM and CCK rates by default as the
> initial RTS threshold is 'IEEE80211_MAX_RTS_THRESHOLD'. And RTS
> thresholds for all other rates have already been disabled when init.
>
> Signed-off-by: Shiji Yang <yangshiji66@outlook.com>

The commit log should always answer to the question "Why?". What problem
does this patch fix?

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

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

Re: [PATCH 2/3] wifi: rt2x00: disable RTS threshold for rt2800 by default

[Shiji Yang]

On Sat, 28 Oct 2023 17:54:27 +0300, Kalle Valo wrote:

>Shiji Yang <yangshiji66@outlook.com> writes:
>
>> Disable the RTS threshold for OFDM and CCK rates by default as the
>> initial RTS threshold is 'IEEE80211_MAX_RTS_THRESHOLD'. And RTS
>> thresholds for all other rates have already been disabled when init.
>>
>> Signed-off-by: Shiji Yang <yangshiji66@outlook.com>
>
>The commit log should always answer to the question "Why?". What problem
>does this patch fix?

Hi! Thanks for your review.

rt2800 has a lot of registers to control the RTS enable/disable
status for different rates. And the driver control them via
rt2800_set_rts_threshold(). I found that when RTS was disabled
in user interface, this function won't be called at all. This
means that the RTS is still 'on' for CCK and OFDM rates. So we'd
better to disable them by default in case they did some bad
things. The RTS for HT20 and HT40 is already default off so we
don't need to touch them. If we toggle the RTS status, these
register bits will be enabled/disabled again by
rt2800_set_rts_threshold().

If this patch is acceptable, I will add more explanations in the
v2 patch. Anyway, I don't know if it really solves some existing
problems, but I think it should be like this.

Regards,
Shiji Yang

Re: [PATCH 2/3] wifi: rt2x00: disable RTS threshold for rt2800 by default

[Stanislaw Gruszka]

On Sat, Oct 28, 2023 at 08:15:31PM +0800, Shiji Yang wrote:
> Disable the RTS threshold for OFDM and CCK rates by default as the
> initial RTS threshold is 'IEEE80211_MAX_RTS_THRESHOLD'. And RTS
> thresholds for all other rates have already been disabled when init.
> Signed-off-by: Shiji Yang <yangshiji66@outlook.com>
Acked-by: Stanislaw Gruszka <stf_xl@wp.pl>

Re: [PATCH 3/3] wifi: rt2x00: restart beacon queue when hardware reset

[Stanislaw Gruszka]

On Sat, Oct 28, 2023 at 08:15:32PM +0800, Shiji Yang wrote:
> When a hardware reset is triggered, all registers are reset, so all
> queues are forced to stop in hardware interface. However, mac80211
> will not automatically stop the queue. If we don't manually stop the
> beacon queue, the queue will be deadlocked and unable to start again.
> This patch fixes the issue where Apple devices cannot connect to the
> AP after calling ieee80211_restart_hw().

Should not this be solved in mac80211 then? ieee80211_restart_work
does a lot o diffrent things, why beconing is not also
stoped/started there ? 

Regards
Stanislaw

> Signed-off-by: Shiji Yang <yangshiji66@outlook.com>
> ---
>  drivers/net/wireless/ralink/rt2x00/rt2x00dev.c | 3 +++
>  drivers/net/wireless/ralink/rt2x00/rt2x00mac.c | 4 +++-
>  2 files changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/wireless/ralink/rt2x00/rt2x00dev.c b/drivers/net/wireless/ralink/rt2x00/rt2x00dev.c
> index 9a9cfd0ce..ac58a56c3 100644
> --- a/drivers/net/wireless/ralink/rt2x00/rt2x00dev.c
> +++ b/drivers/net/wireless/ralink/rt2x00/rt2x00dev.c
> @@ -101,6 +101,8 @@ void rt2x00lib_disable_radio(struct rt2x00_dev *rt2x00dev)
>  	rt2x00link_stop_tuner(rt2x00dev);
>  	rt2x00queue_stop_queues(rt2x00dev);
>  	rt2x00queue_flush_queues(rt2x00dev, true);
> +	if (test_bit(DEVICE_STATE_RESET, &rt2x00dev->flags))
> +		rt2x00queue_stop_queue(rt2x00dev->bcn);
>  
>  	/*
>  	 * Disable radio.
> @@ -1286,6 +1288,7 @@ int rt2x00lib_start(struct rt2x00_dev *rt2x00dev)
>  	rt2x00dev->intf_ap_count = 0;
>  	rt2x00dev->intf_sta_count = 0;
>  	rt2x00dev->intf_associated = 0;
> +	rt2x00dev->intf_beaconing = 0;
>  
>  	/* Enable the radio */
>  	retval = rt2x00lib_enable_radio(rt2x00dev);
> diff --git a/drivers/net/wireless/ralink/rt2x00/rt2x00mac.c b/drivers/net/wireless/ralink/rt2x00/rt2x00mac.c
> index 4202c6517..6fcbf534a 100644
> --- a/drivers/net/wireless/ralink/rt2x00/rt2x00mac.c
> +++ b/drivers/net/wireless/ralink/rt2x00/rt2x00mac.c
> @@ -615,7 +615,9 @@ void rt2x00mac_bss_info_changed(struct ieee80211_hw *hw,
>  			 * and keep it running on other interfaces.
>  			 */
>  			rt2x00queue_clear_beacon(rt2x00dev, vif);
> -		} else if (bss_conf->enable_beacon && !intf->enable_beacon) {
> +		} else if (bss_conf->enable_beacon &&
> +			   (!intf->enable_beacon ||
> +			    test_bit(DEVICE_STATE_RESET, &rt2x00dev->flags))) {

>  			rt2x00dev->intf_beaconing++;
>  			intf->enable_beacon = true;
>  			/*
> -- 
> 2.39.2
> 

Re: [PATCH 3/3] wifi: rt2x00: restart beacon queue when hardware reset

[Shiji Yang]

On Wed, 1 Nov 2023 10:07:10 +0100, Stanislaw Gruszka wrote:

>On Sat, Oct 28, 2023 at 08:15:32PM +0800, Shiji Yang wrote:
>> When a hardware reset is triggered, all registers are reset, so all
>> queues are forced to stop in hardware interface. However, mac80211
>> will not automatically stop the queue. If we don't manually stop the
>> beacon queue, the queue will be deadlocked and unable to start again.
>> This patch fixes the issue where Apple devices cannot connect to the
>> AP after calling ieee80211_restart_hw().
>
>Should not this be solved in mac80211 then? ieee80211_restart_work
>does a lot o diffrent things, why beconing is not also
>stoped/started there ? 
>
>Regards
>Stanislaw
>

Hi! Thanks for your review.

I think this issue is a bug of the rt2x00. When restart is called,
mac80211 didn't call rt2x00mac_bss_info_changed() to update the
flag (This may be expected? I'm not sure. But all other Tx/Rx queues
are also manually disabled). And after resetting,
'bss_conf->enable_beacon' and 'intf->enable_beacon' are still true.
Though mac80211 will call this function and try to enable the beacon
queue again. However, both 'if' and 'else if' blocks will never be
entered anymore because all conditions are false. This patch just
fixes this dead lock.

Maybe Kalle Valo knows if it's a mac80211 bug. This issue has been
here for several years.

Looking forward to your reply.

By the way, it seems that 'intf_beaconing' variable is useless. Does
it really can be increased to '2'? Maybe in multi ssid mode?

```
void rt2x00mac_bss_info_changed(struct ieee80211_hw *hw,
				struct ieee80211_vif *vif,
				struct ieee80211_bss_conf *bss_conf,
				u64 changes)
{
......
		if (!bss_conf->enable_beacon && intf->enable_beacon) {
			rt2x00dev->intf_beaconing--;
			intf->enable_beacon = false;

			if (rt2x00dev->intf_beaconing == 0) {
				/*
				 * Last beaconing interface disabled
				 * -> stop beacon queue.
				 */
				rt2x00queue_stop_queue(rt2x00dev->bcn);
			}
			/*
			 * Clear beacon in the H/W for this vif. This is needed
			 * to disable beaconing on this particular interface
			 * and keep it running on other interfaces.
			 */
			rt2x00queue_clear_beacon(rt2x00dev, vif);
		} else if (bss_conf->enable_beacon && !intf->enable_beacon) {
			rt2x00dev->intf_beaconing++;
			intf->enable_beacon = true;
			/*
			 * Upload beacon to the H/W. This is only required on
			 * USB devices. PCI devices fetch beacons periodically.
			 */
			if (rt2x00_is_usb(rt2x00dev))
				rt2x00queue_update_beacon(rt2x00dev, vif);

			if (rt2x00dev->intf_beaconing == 1) {
				/*
				 * First beaconing interface enabled
				 * -> start beacon queue.
				 */
				rt2x00queue_start_queue(rt2x00dev->bcn);
			}
		}
```

Regards,
Shiji Yang

Re: [PATCH 3/3] wifi: rt2x00: restart beacon queue when hardware reset

[Stanislaw Gruszka]

On Thu, Nov 02, 2023 at 08:36:04PM +0800, Shiji Yang wrote:
> On Wed, 1 Nov 2023 10:07:10 +0100, Stanislaw Gruszka wrote:
> 
> >On Sat, Oct 28, 2023 at 08:15:32PM +0800, Shiji Yang wrote:
> >> When a hardware reset is triggered, all registers are reset, so all
> >> queues are forced to stop in hardware interface. However, mac80211
> >> will not automatically stop the queue. If we don't manually stop the
> >> beacon queue, the queue will be deadlocked and unable to start again.
> >> This patch fixes the issue where Apple devices cannot connect to the
> >> AP after calling ieee80211_restart_hw().
> >
> >Should not this be solved in mac80211 then? ieee80211_restart_work
> >does a lot o diffrent things, why beconing is not also
> >stoped/started there ? 
> >
> >Regards
> >Stanislaw
> >
> 
> Hi! Thanks for your review.
> 
> I think this issue is a bug of the rt2x00. When restart is called,
Yes, I think you have right, this is rt2x00 issue. 

> mac80211 didn't call rt2x00mac_bss_info_changed() to update the
> flag (This may be expected? I'm not sure. But all other Tx/Rx queues
> are also manually disabled). And after resetting,
> 'bss_conf->enable_beacon' and 'intf->enable_beacon' are still true.
> Though mac80211 will call this function and try to enable the beacon
> queue again. However, both 'if' and 'else if' blocks will never be
> entered anymore because all conditions are false. This patch just
> fixes this dead lock.
Ok, I see. 

I don't remember how this supposed to work. I see we do 

        for (i = 0; i < queue->limit; i++) {
                entry = &queue->entries[i];
                clear_bit(ENTRY_BCN_ASSIGNED, &entry->flags);
        }

in rt2800_pre_reset_hw() But I think what should be done there is
clear intf->enable_beacon for each interface. 

Now I don't remember how I tested this, probably only in STA mode.

> Maybe Kalle Valo knows if it's a mac80211 bug. This issue has been
> here for several years.
> 
> Looking forward to your reply.

:-)  

> By the way, it seems that 'intf_beaconing' variable is useless. Does
> it really can be increased to '2'? Maybe in multi ssid mode?

Yes. When you can have multiple vif interfaces this variable 
can be bigger than 1. We advertise support for that for AP
and mesh interfaces in rt2x00lib_set_if_combinations().

> 		} else if (bss_conf->enable_beacon && !intf->enable_beacon) {
> 			rt2x00dev->intf_beaconing++;
> 			intf->enable_beacon = true;
> 			/*
> 			 * Upload beacon to the H/W. This is only required on
> 			 * USB devices. PCI devices fetch beacons periodically.
> 			 */
> 			if (rt2x00_is_usb(rt2x00dev))
> 				rt2x00queue_update_beacon(rt2x00dev, vif);
Hmm, maybe I also tested on AP USB, but don't remember.

Thanks for explanations! Patch is ok for me.

Regards
Stanislaw

Re: [PATCH 3/3] wifi: rt2x00: restart beacon queue when hardware reset

[Stanislaw Gruszka]

On Sat, Oct 28, 2023 at 08:15:32PM +0800, Shiji Yang wrote:
> When a hardware reset is triggered, all registers are reset, so all
> queues are forced to stop in hardware interface. However, mac80211
> will not automatically stop the queue. If we don't manually stop the
> beacon queue, the queue will be deadlocked and unable to start again.
> This patch fixes the issue where Apple devices cannot connect to the
> AP after calling ieee80211_restart_hw().
> 
> Signed-off-by: Shiji Yang <yangshiji66@outlook.com>
Acked-by: Stanislaw Gruszka <stf_xl@wp.pl>

Re: [PATCH 3/3] wifi: rt2x00: restart beacon queue when hardware reset

[Shiji Yang]

On Fri, 3 Nov 2023 06:55:47 +0100, Stanislaw Gruszka wrote:

>On Thu, Nov 02, 2023 at 08:36:04PM +0800, Shiji Yang wrote:
>> On Wed, 1 Nov 2023 10:07:10 +0100, Stanislaw Gruszka wrote:
>> 
>> >On Sat, Oct 28, 2023 at 08:15:32PM +0800, Shiji Yang wrote:
>> >> When a hardware reset is triggered, all registers are reset, so all
>> >> queues are forced to stop in hardware interface. However, mac80211
>> >> will not automatically stop the queue. If we don't manually stop the
>> >> beacon queue, the queue will be deadlocked and unable to start again.
>> >> This patch fixes the issue where Apple devices cannot connect to the
>> >> AP after calling ieee80211_restart_hw().
>> >
>> >Should not this be solved in mac80211 then? ieee80211_restart_work
>> >does a lot o diffrent things, why beconing is not also
>> >stoped/started there ? 
>> >
>> >Regards
>> >Stanislaw
>> >
>> 
>> Hi! Thanks for your review.
>> 
>> I think this issue is a bug of the rt2x00. When restart is called,
>Yes, I think you have right, this is rt2x00 issue. 
>
>> mac80211 didn't call rt2x00mac_bss_info_changed() to update the
>> flag (This may be expected? I'm not sure. But all other Tx/Rx queues
>> are also manually disabled). And after resetting,
>> 'bss_conf->enable_beacon' and 'intf->enable_beacon' are still true.
>> Though mac80211 will call this function and try to enable the beacon
>> queue again. However, both 'if' and 'else if' blocks will never be
>> entered anymore because all conditions are false. This patch just
>> fixes this dead lock.
>Ok, I see. 
>
>I don't remember how this supposed to work. I see we do 
>
>        for (i = 0; i < queue->limit; i++) {
>                entry = &queue->entries[i];
>                clear_bit(ENTRY_BCN_ASSIGNED, &entry->flags);
>        }
>
>in rt2800_pre_reset_hw() But I think what should be done there is
>clear intf->enable_beacon for each interface. 

Yes, idealy we should do that. But 'intf->enable_beacon' variable is
owned by mac80211, we can not access it here. So I can only say that
the current solution just a reasonable workaround. I made some little
changes in v2 patch will help developers to understand what happened
in rt2x00mac_bss_info_changed() after reset.

>
>Now I don't remember how I tested this, probably only in STA mode.
>
>> Maybe Kalle Valo knows if it's a mac80211 bug. This issue has been
>> here for several years.
>> 
>> Looking forward to your reply.
>
>:-)  
>
>> By the way, it seems that 'intf_beaconing' variable is useless. Does
>> it really can be increased to '2'? Maybe in multi ssid mode?
>
>Yes. When you can have multiple vif interfaces this variable 
>can be bigger than 1. We advertise support for that for AP
>and mesh interfaces in rt2x00lib_set_if_combinations().
>
>> 		} else if (bss_conf->enable_beacon && !intf->enable_beacon) {
>> 			rt2x00dev->intf_beaconing++;
>> 			intf->enable_beacon = true;
>> 			/*
>> 			 * Upload beacon to the H/W. This is only required on
>> 			 * USB devices. PCI devices fetch beacons periodically.
>> 			 */
>> 			if (rt2x00_is_usb(rt2x00dev))
>> 				rt2x00queue_update_beacon(rt2x00dev, vif);
>Hmm, maybe I also tested on AP USB, but don't remember.
>
>Thanks for explanations! Patch is ok for me.
>
>Regards
>Stanislaw
>

Regards,
Shiji Yang