| Message ID | TYAP286MB0315339CF8B38FC1286CF39DBCA3A@TYAP286MB0315.JPNP286.PROD.OUTLOOK.COM (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Kalle Valo |
| Headers | show |
| Series | [1/3] wifi: rt2x00: introduce DMA busy check watchdog for rt2800 | expand |
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 >
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
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
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>
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
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; /*
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(-)