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 <[email protected]>
---
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;
+ }
+
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 */
--
2.29.2
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 <[email protected]>
> ---
> 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 <[email protected]> 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 <[email protected]>
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...
--
https://patchwork.kernel.org/project/linux-wireless/list/
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
Bryan O'Donoghue <[email protected]> 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 <[email protected]>
[...]
> +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.
--
https://patchwork.kernel.org/project/linux-wireless/list/
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
On 11/01/2021 11:31, Kalle Valo wrote:
> Bryan O'Donoghue <[email protected]> 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 <[email protected]>
>
> 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 <[email protected]> writes:
> On 11/01/2021 11:31, Kalle Valo wrote:
>> Bryan O'Donoghue <[email protected]> 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 <[email protected]>
>>
>> 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?
--
https://patchwork.kernel.org/project/linux-wireless/list/
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
On 11/01/2021 12:26, Kalle Valo wrote:
> Bryan O'Donoghue <[email protected]> writes:
>
>> On 11/01/2021 11:31, Kalle Valo wrote:
>>> Bryan O'Donoghue <[email protected]> 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 <[email protected]>
>>>
>>> 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