| Message ID | 20201120021403.2646574-1-bryan.odonoghue@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | wcn36xx: Don't run scan_init multiple times | expand |
On Fri, 20 Nov 2020 at 03:13, Bryan O'Donoghue <bryan.odonoghue@linaro.org> wrote: > > Run scan_init only once. There's no need to run this command multiple times > if it has already been run once. > > The software scan algorithm can end up repeatedly calling scan_init on each > loop resulting in between four and eight milliseconds of lost time on each > callout. > > Subtract the overhead now. This command defines parameters like the BSSID we want to inform, etc... So this can change depending on the scan is done while connected or not. Moreover in the connected case, the scans are interleaved with normal data listening period, and AFAIU, init/stop scan allow to submit a null data packet with PS/non-PS bit when mac80211 leaves the operating channel to scanning another one (so that AP does no submit packet to it). So at first glance, this patch would break that, right? > > Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> > --- > drivers/net/wireless/ath/wcn36xx/smd.c | 6 ++++++ > drivers/net/wireless/ath/wcn36xx/wcn36xx.h | 1 + > 2 files changed, 7 insertions(+) > > diff --git a/drivers/net/wireless/ath/wcn36xx/smd.c b/drivers/net/wireless/ath/wcn36xx/smd.c > index acf533fae46a..ec082cf3ab09 100644 > --- a/drivers/net/wireless/ath/wcn36xx/smd.c > +++ b/drivers/net/wireless/ath/wcn36xx/smd.c > @@ -706,6 +706,10 @@ int wcn36xx_smd_init_scan(struct wcn36xx *wcn, enum wcn36xx_hal_sys_mode mode, > int ret; > > mutex_lock(&wcn->hal_mutex); > + if (wcn->scan_init) { > + ret = 0; > + goto out; > + } > INIT_HAL_MSG(msg_body, WCN36XX_HAL_INIT_SCAN_REQ); > > msg_body.mode = mode; > @@ -731,6 +735,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->scan_init = true; > out: > mutex_unlock(&wcn->hal_mutex); > return ret; > @@ -761,6 +766,7 @@ int wcn36xx_smd_start_scan(struct wcn36xx *wcn, u8 scan_channel) > wcn36xx_err("hal_start_scan response failed err=%d\n", ret); > goto out; > } > + wcn->scan_init = false; > out: > mutex_unlock(&wcn->hal_mutex); > return ret; > diff --git a/drivers/net/wireless/ath/wcn36xx/wcn36xx.h b/drivers/net/wireless/ath/wcn36xx/wcn36xx.h > index 71fa9992b118..156df6d184c8 100644 > --- a/drivers/net/wireless/ath/wcn36xx/wcn36xx.h > +++ b/drivers/net/wireless/ath/wcn36xx/wcn36xx.h > @@ -235,6 +235,7 @@ struct wcn36xx { > struct ieee80211_vif *sw_scan_vif; > struct mutex scan_lock; > bool scan_aborted; > + bool scan_init; > > /* DXE channels */ > struct wcn36xx_dxe_ch dxe_tx_l_ch; /* TX low */ > -- > 2.28.0 > _______________________________________________ wcn36xx mailing list wcn36xx@lists.infradead.org http://lists.infradead.org/mailman/listinfo/wcn36xx
On 20/11/2020 08:12, Loic Poulain wrote: > On Fri, 20 Nov 2020 at 03:13, Bryan O'Donoghue > <bryan.odonoghue@linaro.org> wrote: >> >> Run scan_init only once. There's no need to run this command multiple times >> if it has already been run once. >> >> The software scan algorithm can end up repeatedly calling scan_init on each >> loop resulting in between four and eight milliseconds of lost time on each >> callout. >> >> Subtract the overhead now. > > This command defines parameters like the BSSID we want to inform, > etc... So this can change depending on the scan is done while > connected or not. So you're saying a scan is started and our connection state toggles from non-connected to connected. Possible I guess. > Moreover in the connected case, the scans are > interleaved with normal data listening period, and AFAIU, init/stop > scan allow to submit a null data packet with PS/non-PS bit when > mac80211 leaves the operating channel to scanning another one (so that > AP does no submit packet to it). So at first glance, this patch would > break that, right? I agree with that logic, and actually looking at downstream - we see that downstream doesn't set the notification byte before starting a scan on a new channel connected: [ 63.475897] BOD WDI_SendMsg/23794 message = 0x04 version = 0x01 len 0x00000030 // WLAN_HAL_INIT_SCAN_REQ [ 63.475902] SMD <<< 00000000: 04 00 01 00 30 00 00 00 02 00 00 00 00 00 00 00 00 00 00 00 00 ff ff ff c8 9f 7a 21 c0 ff ff ff [ 63.475907] SMD <<< 00000020: d4 2e 7c 00 c0 ff ff ff 00 54 93 6d c0 22 22 00 [ 63.478242] SMD >>> 00000000: 05 00 00 00 0c 00 00 00 00 00 00 00 type=04 00 version=01 00 length=30 00 00 00 mode=02 00 00 00 bssid=00 000 00 00 00 00 notify=00 ... which I accept is actually a bug downstream, conceptually at any rate. I need to ensure a scan isn't in process when we go into suspend but, there's no reason to skip the scan_init() command for that. flagging != skipping Let's forget this one. --- bod _______________________________________________ wcn36xx mailing list wcn36xx@lists.infradead.org http://lists.infradead.org/mailman/listinfo/wcn36xx
diff --git a/drivers/net/wireless/ath/wcn36xx/smd.c b/drivers/net/wireless/ath/wcn36xx/smd.c index acf533fae46a..ec082cf3ab09 100644 --- a/drivers/net/wireless/ath/wcn36xx/smd.c +++ b/drivers/net/wireless/ath/wcn36xx/smd.c @@ -706,6 +706,10 @@ int wcn36xx_smd_init_scan(struct wcn36xx *wcn, enum wcn36xx_hal_sys_mode mode, int ret; mutex_lock(&wcn->hal_mutex); + if (wcn->scan_init) { + ret = 0; + goto out; + } INIT_HAL_MSG(msg_body, WCN36XX_HAL_INIT_SCAN_REQ); msg_body.mode = mode; @@ -731,6 +735,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->scan_init = true; out: mutex_unlock(&wcn->hal_mutex); return ret; @@ -761,6 +766,7 @@ int wcn36xx_smd_start_scan(struct wcn36xx *wcn, u8 scan_channel) wcn36xx_err("hal_start_scan response failed err=%d\n", ret); goto out; } + wcn->scan_init = false; out: mutex_unlock(&wcn->hal_mutex); return ret; diff --git a/drivers/net/wireless/ath/wcn36xx/wcn36xx.h b/drivers/net/wireless/ath/wcn36xx/wcn36xx.h index 71fa9992b118..156df6d184c8 100644 --- a/drivers/net/wireless/ath/wcn36xx/wcn36xx.h +++ b/drivers/net/wireless/ath/wcn36xx/wcn36xx.h @@ -235,6 +235,7 @@ struct wcn36xx { struct ieee80211_vif *sw_scan_vif; struct mutex scan_lock; bool scan_aborted; + bool scan_init; /* DXE channels */ struct wcn36xx_dxe_ch dxe_tx_l_ch; /* TX low */
Run scan_init only once. There's no need to run this command multiple times if it has already been run once. The software scan algorithm can end up repeatedly calling scan_init on each loop resulting in between four and eight milliseconds of lost time on each callout. Subtract the overhead now. Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> --- drivers/net/wireless/ath/wcn36xx/smd.c | 6 ++++++ drivers/net/wireless/ath/wcn36xx/wcn36xx.h | 1 + 2 files changed, 7 insertions(+) -- 2.28.0 _______________________________________________ wcn36xx mailing list wcn36xx@lists.infradead.org http://lists.infradead.org/mailman/listinfo/wcn36xx