| Message ID | 20201228162839.369156-12-bryan.odonoghue@linaro.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Kalle Valo |
| Headers | show |
| Series | wcn36xx: Enable downstream consistent Wake on Lan | expand |
On 12/28/20 8:28 AM, Bryan O'Donoghue wrote: > If a scan is in progress do not attempt to enter into suspend. Allow the > scan process to quiesce before proceeding. > > Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> > --- > drivers/net/wireless/ath/wcn36xx/main.c | 5 +++++ > drivers/net/wireless/ath/wcn36xx/smd.c | 13 +++++++++++++ > drivers/net/wireless/ath/wcn36xx/smd.h | 2 ++ > drivers/net/wireless/ath/wcn36xx/wcn36xx.h | 1 + > 4 files changed, 21 insertions(+) > > diff --git a/drivers/net/wireless/ath/wcn36xx/main.c b/drivers/net/wireless/ath/wcn36xx/main.c > index b48a3f0dcc0b..feb909192c8e 100644 > --- a/drivers/net/wireless/ath/wcn36xx/main.c > +++ b/drivers/net/wireless/ath/wcn36xx/main.c > @@ -1113,6 +1113,11 @@ static int wcn36xx_suspend(struct ieee80211_hw *hw, struct cfg80211_wowlan *wow) > > wcn36xx_dbg(WCN36XX_DBG_MAC, "mac suspend\n"); > > + if (wcn36xx_smd_is_scanning(wcn)) { > + ret = -EBUSY; > + goto out; > + } > + Should just be a return, since we haven't locked conf_mutex yet? > mutex_lock(&wcn->conf_mutex); > vif = wcn36xx_get_first_vif(wcn); > if (vif) { > diff --git a/drivers/net/wireless/ath/wcn36xx/smd.c b/drivers/net/wireless/ath/wcn36xx/smd.c > index dd12575f33c3..378282a93aa0 100644 > --- a/drivers/net/wireless/ath/wcn36xx/smd.c > +++ b/drivers/net/wireless/ath/wcn36xx/smd.c > @@ -731,6 +731,7 @@ int wcn36xx_smd_init_scan(struct wcn36xx *wcn, enum wcn36xx_hal_sys_mode mode, > wcn36xx_err("hal_init_scan response failed err=%d\n", ret); > goto out; > } > + wcn->scanning = true; > out: > mutex_unlock(&wcn->hal_mutex); > return ret; > @@ -807,6 +808,7 @@ int wcn36xx_smd_finish_scan(struct wcn36xx *wcn, > mutex_lock(&wcn->hal_mutex); > INIT_HAL_MSG(msg_body, WCN36XX_HAL_FINISH_SCAN_REQ); > > + wcn->scanning = false; > msg_body.mode = mode; > msg_body.oper_channel = WCN36XX_HW_CHANNEL(wcn); > if (vif_priv->bss_index != WCN36XX_HAL_BSS_INVALID_IDX) { > @@ -938,6 +940,17 @@ int wcn36xx_smd_stop_hw_scan(struct wcn36xx *wcn) > return ret; > } > > +bool wcn36xx_smd_is_scanning(struct wcn36xx *wcn) > +{ > + bool scanning; > + > + mutex_lock(&wcn->hal_mutex); > + scanning = wcn->scanning; > + mutex_unlock(&wcn->hal_mutex); > + > + return scanning; > +} > + > static int wcn36xx_smd_switch_channel_rsp(void *buf, size_t len) > { > struct wcn36xx_hal_switch_channel_rsp_msg *rsp; > diff --git a/drivers/net/wireless/ath/wcn36xx/smd.h b/drivers/net/wireless/ath/wcn36xx/smd.h > index b225c805107c..3488abb201d0 100644 > --- a/drivers/net/wireless/ath/wcn36xx/smd.h > +++ b/drivers/net/wireless/ath/wcn36xx/smd.h > @@ -159,4 +159,6 @@ int wcn36xx_smd_gtk_offload(struct wcn36xx *wcn, struct ieee80211_vif *vif, > int wcn36xx_smd_gtk_offload_get_info(struct wcn36xx *wcn, > struct ieee80211_vif *vif); > > +bool wcn36xx_smd_is_scanning(struct wcn36xx *wcn); > + > #endif /* _SMD_H_ */ > diff --git a/drivers/net/wireless/ath/wcn36xx/wcn36xx.h b/drivers/net/wireless/ath/wcn36xx/wcn36xx.h > index 6121d8a5641a..36ea768a5203 100644 > --- a/drivers/net/wireless/ath/wcn36xx/wcn36xx.h > +++ b/drivers/net/wireless/ath/wcn36xx/wcn36xx.h > @@ -249,6 +249,7 @@ struct wcn36xx { > struct ieee80211_vif *sw_scan_vif; > struct mutex scan_lock; > bool scan_aborted; > + bool scanning; > > /* DXE channels */ > struct wcn36xx_dxe_ch dxe_tx_l_ch; /* TX low */ >
Bryan O'Donoghue <bryan.odonoghue@linaro.org> writes: > If a scan is in progress do not attempt to enter into suspend. Allow the > scan process to quiesce before proceeding. > > Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> Why? I would have considered the opposite and if we go to suspend we cancel the scan. No strong feelings, just don't see the need for scan results during suspend. But of course I might be missing something...
Bryan O'Donoghue <bryan.odonoghue@linaro.org> writes: > If a scan is in progress do not attempt to enter into suspend. Allow the > scan process to quiesce before proceeding. > > Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> [...] > +bool wcn36xx_smd_is_scanning(struct wcn36xx *wcn) > +{ > + bool scanning; > + > + mutex_lock(&wcn->hal_mutex); > + scanning = wcn->scanning; > + mutex_unlock(&wcn->hal_mutex); > + > + return scanning; > +} Instead of having a bool you could use SET_BIT() & co, that way you don't need this extra function. For example see dev_flags in ath10k.
On 11/01/2021 11:31, Kalle Valo wrote: > Bryan O'Donoghue <bryan.odonoghue@linaro.org> writes: > >> If a scan is in progress do not attempt to enter into suspend. Allow the >> scan process to quiesce before proceeding. >> >> Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> > > Why? I would have considered the opposite and if we go to suspend we > cancel the scan. No strong feelings, just don't see the need for scan > results during suspend. But of course I might be missing something... We need to be switched to the AP's channel when calling the suspend routine. During a s/w scan we switch off channel to scan for 100s of milliseconds. If the suspend() routine is called while that is true, we suspend on the wrong channel. So we would need to switch to the right channel explicitly in suspend but, at the moment wcn36xx_config() for switching channels and I thought it best to leave the channel switching logic in the one place. I'm not opposed in principle to - Entering suspend - Switching to the last known active channel - Suspending --- bod
Bryan O'Donoghue <bryan.odonoghue@linaro.org> writes: > On 11/01/2021 11:31, Kalle Valo wrote: >> Bryan O'Donoghue <bryan.odonoghue@linaro.org> writes: >> >>> If a scan is in progress do not attempt to enter into suspend. Allow the >>> scan process to quiesce before proceeding. >>> >>> Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> >> >> Why? I would have considered the opposite and if we go to suspend we >> cancel the scan. No strong feelings, just don't see the need for scan >> results during suspend. But of course I might be missing something... > > We need to be switched to the AP's channel when calling the suspend > routine. During a s/w scan we switch off channel to scan for 100s of > milliseconds. > > If the suspend() routine is called while that is true, we suspend on > the wrong channel. > > So we would need to switch to the right channel explicitly in suspend > but, at the moment wcn36xx_config() for switching channels and I > thought it best to leave the channel switching logic in the one place. > > I'm not opposed in principle to > > - Entering suspend > - Switching to the last known active channel > - Suspending Should this be fixed in mac80211? Otherwise every driver using software scan needs to have a workaround for this, right?
On 11/01/2021 12:26, Kalle Valo wrote: > Bryan O'Donoghue <bryan.odonoghue@linaro.org> writes: > >> On 11/01/2021 11:31, Kalle Valo wrote: >>> Bryan O'Donoghue <bryan.odonoghue@linaro.org> writes: >>> >>>> If a scan is in progress do not attempt to enter into suspend. Allow the >>>> scan process to quiesce before proceeding. >>>> >>>> Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> >>> >>> Why? I would have considered the opposite and if we go to suspend we >>> cancel the scan. No strong feelings, just don't see the need for scan >>> results during suspend. But of course I might be missing something... >> >> We need to be switched to the AP's channel when calling the suspend >> routine. During a s/w scan we switch off channel to scan for 100s of >> milliseconds. >> >> If the suspend() routine is called while that is true, we suspend on >> the wrong channel. >> >> So we would need to switch to the right channel explicitly in suspend >> but, at the moment wcn36xx_config() for switching channels and I >> thought it best to leave the channel switching logic in the one place. >> >> I'm not opposed in principle to >> >> - Entering suspend >> - Switching to the last known active channel >> - Suspending > > Should this be fixed in mac80211? Otherwise every driver using software > scan needs to have a workaround for this, right? > I'll check. I thought this problem was likely specific to wcn36xx but, conceptually I can't argue with you there. Given I only see this behavior on Android and not on Debian - I test both - where Android tends to scan constantly - it is possible I'm the only one really triggering the bug given most drivers don't do s/w scan and probably wcn36xx is the only s/w scan being used on an Android system close to what we have upstream. So yeah fair point, I'll see if the fix fits better at a higher level. --- bod
diff --git a/drivers/net/wireless/ath/wcn36xx/main.c b/drivers/net/wireless/ath/wcn36xx/main.c index b48a3f0dcc0b..feb909192c8e 100644 --- a/drivers/net/wireless/ath/wcn36xx/main.c +++ b/drivers/net/wireless/ath/wcn36xx/main.c @@ -1113,6 +1113,11 @@ static int wcn36xx_suspend(struct ieee80211_hw *hw, struct cfg80211_wowlan *wow) wcn36xx_dbg(WCN36XX_DBG_MAC, "mac suspend\n"); + if (wcn36xx_smd_is_scanning(wcn)) { + ret = -EBUSY; + goto out; + } + mutex_lock(&wcn->conf_mutex); vif = wcn36xx_get_first_vif(wcn); if (vif) { diff --git a/drivers/net/wireless/ath/wcn36xx/smd.c b/drivers/net/wireless/ath/wcn36xx/smd.c index dd12575f33c3..378282a93aa0 100644 --- a/drivers/net/wireless/ath/wcn36xx/smd.c +++ b/drivers/net/wireless/ath/wcn36xx/smd.c @@ -731,6 +731,7 @@ int wcn36xx_smd_init_scan(struct wcn36xx *wcn, enum wcn36xx_hal_sys_mode mode, wcn36xx_err("hal_init_scan response failed err=%d\n", ret); goto out; } + wcn->scanning = true; out: mutex_unlock(&wcn->hal_mutex); return ret; @@ -807,6 +808,7 @@ int wcn36xx_smd_finish_scan(struct wcn36xx *wcn, mutex_lock(&wcn->hal_mutex); INIT_HAL_MSG(msg_body, WCN36XX_HAL_FINISH_SCAN_REQ); + wcn->scanning = false; msg_body.mode = mode; msg_body.oper_channel = WCN36XX_HW_CHANNEL(wcn); if (vif_priv->bss_index != WCN36XX_HAL_BSS_INVALID_IDX) { @@ -938,6 +940,17 @@ int wcn36xx_smd_stop_hw_scan(struct wcn36xx *wcn) return ret; } +bool wcn36xx_smd_is_scanning(struct wcn36xx *wcn) +{ + bool scanning; + + mutex_lock(&wcn->hal_mutex); + scanning = wcn->scanning; + mutex_unlock(&wcn->hal_mutex); + + return scanning; +} + static int wcn36xx_smd_switch_channel_rsp(void *buf, size_t len) { struct wcn36xx_hal_switch_channel_rsp_msg *rsp; diff --git a/drivers/net/wireless/ath/wcn36xx/smd.h b/drivers/net/wireless/ath/wcn36xx/smd.h index b225c805107c..3488abb201d0 100644 --- a/drivers/net/wireless/ath/wcn36xx/smd.h +++ b/drivers/net/wireless/ath/wcn36xx/smd.h @@ -159,4 +159,6 @@ int wcn36xx_smd_gtk_offload(struct wcn36xx *wcn, struct ieee80211_vif *vif, int wcn36xx_smd_gtk_offload_get_info(struct wcn36xx *wcn, struct ieee80211_vif *vif); +bool wcn36xx_smd_is_scanning(struct wcn36xx *wcn); + #endif /* _SMD_H_ */ diff --git a/drivers/net/wireless/ath/wcn36xx/wcn36xx.h b/drivers/net/wireless/ath/wcn36xx/wcn36xx.h index 6121d8a5641a..36ea768a5203 100644 --- a/drivers/net/wireless/ath/wcn36xx/wcn36xx.h +++ b/drivers/net/wireless/ath/wcn36xx/wcn36xx.h @@ -249,6 +249,7 @@ struct wcn36xx { struct ieee80211_vif *sw_scan_vif; struct mutex scan_lock; bool scan_aborted; + bool scanning; /* DXE channels */ struct wcn36xx_dxe_ch dxe_tx_l_ch; /* TX low */
If a scan is in progress do not attempt to enter into suspend. Allow the scan process to quiesce before proceeding. Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> --- drivers/net/wireless/ath/wcn36xx/main.c | 5 +++++ drivers/net/wireless/ath/wcn36xx/smd.c | 13 +++++++++++++ drivers/net/wireless/ath/wcn36xx/smd.h | 2 ++ drivers/net/wireless/ath/wcn36xx/wcn36xx.h | 1 + 4 files changed, 21 insertions(+)