Here's another series of 5 patches for wcn36xx that address some issues
with scanning. The first one is the most important one.
Daniel
Daniel Mack (5):
wcn36xx: abort scan request when 'dequeued' indicator is sent
wcn36xx: cancel pending scan request when interface goes down
wcn36xx: handle scan cancellation when firmware support is missing
wcn36xx: send bss_type in scan requests
wcn36xx: pass information elements in scan requests
drivers/net/wireless/ath/wcn36xx/hal.h | 8 +++++++-
drivers/net/wireless/ath/wcn36xx/main.c | 29 +++++++++++++++++++++++++----
drivers/net/wireless/ath/wcn36xx/smd.c | 20 +++++++++++++++++---
drivers/net/wireless/ath/wcn36xx/wcn36xx.h | 7 +------
4 files changed, 50 insertions(+), 14 deletions(-)
--
2.14.3
Daniel Mack <[email protected]> writes:
> On Monday, April 16, 2018 04:13 PM, Kalle Valo wrote:
>> Daniel Mack <[email protected]> writes:
>>> On Monday, April 16, 2018 04:03 PM, Kalle Valo wrote:
>>>> Daniel Mack <[email protected]> writes:
>
>>>>> them to the firmware message. The driver currently tells the core that
>>>>> it is capable of attaching up to WCN36XX_MAX_SCAN_IE_LEN octets, but
>>>>> doesn't actually pass them to the the hardware.
>>>>>
>>>>> Some defines were moved around to avoid cyclic include dependencies.
>>>>
>>>> Does this fix anything or change functionality somehow? You should
>>>> document that also in the commit log.
>>>
>>> I don't have a test case for this, no. But as the change was pretty much
>>> straight forward, I sent it anyway.
>>
>> Ok, but you should still explain that in the commit log. In other words,
>> you should always answer the question "Why?" in the commit log.
>>
>>> I can resend with some more information on this if you like.
>>
>> No need to resend because of this, I can edit the commit log.
>
> Hmm, given that I can't even be sure the firmware does the right thing
> when instructed this way, we should probably just drop this patch from
> the series. The others are more important anyway, as they address bugs
> that hit me on actual hardware.
IMHO it's useful and if you don't see anything breaking I would prefer
to take it anyway. I can't immeadiately say in what use cases this helps
or fixes, though. But maybe you (or someone else) could verify that it
works correctly by comparing before and after probe requests with a
sniffer?
--
Kalle Valo
For firmwares that don't have the SCAN_OFFLOAD feature bit set, do
not call into wcn36xx_smd_stop_hw_scan(). Instead, stop the asynchronous
work and call into ieee80211_scan_completed() immediately.
Signed-off-by: Daniel Mack <[email protected]>
---
drivers/net/wireless/ath/wcn36xx/main.c | 14 +++++++++++---
1 file changed, 11 insertions(+), 3 deletions(-)
diff --git a/drivers/net/wireless/ath/wcn36xx/main.c b/drivers/net/wireless/ath/wcn36xx/main.c
index e5ef2cbb622f..28c0286ae47b 100644
--- a/drivers/net/wireless/ath/wcn36xx/main.c
+++ b/drivers/net/wireless/ath/wcn36xx/main.c
@@ -685,10 +685,18 @@ static void wcn36xx_cancel_hw_scan(struct ieee80211_hw *hw,
wcn->scan_aborted = true;
mutex_unlock(&wcn->scan_lock);
- /* ieee80211_scan_completed will be called on FW scan indication */
- wcn36xx_smd_stop_hw_scan(wcn);
+ if (get_feat_caps(wcn->fw_feat_caps, SCAN_OFFLOAD)) {
+ /* ieee80211_scan_completed will be called on FW scan
+ * indication */
+ wcn36xx_smd_stop_hw_scan(wcn);
+ } else {
+ struct cfg80211_scan_info scan_info = {
+ .aborted = true,
+ };
- cancel_work_sync(&wcn->scan_work);
+ cancel_work_sync(&wcn->scan_work);
+ ieee80211_scan_completed(wcn->hw, &scan_info);
+ }
}
static void wcn36xx_update_allowed_rates(struct ieee80211_sta *sta,
--
2.14.3
Daniel Mack <[email protected]> writes:
> On Monday, April 16, 2018 04:41 PM, Kalle Valo wrote:
>> Daniel Mack <[email protected]> writes:
>>
>>> On Monday, April 16, 2018 04:13 PM, Kalle Valo wrote:
>>>> Daniel Mack <[email protected]> writes:
>>>>> On Monday, April 16, 2018 04:03 PM, Kalle Valo wrote:
>>>>>> Daniel Mack <[email protected]> writes:
>>>
>>>>>>> them to the firmware message. The driver currently tells the core that
>>>>>>> it is capable of attaching up to WCN36XX_MAX_SCAN_IE_LEN octets, but
>>>>>>> doesn't actually pass them to the the hardware.
>>>>>>>
>>>>>>> Some defines were moved around to avoid cyclic include dependencies.
>>>>>>
>>>>>> Does this fix anything or change functionality somehow? You should
>>>>>> document that also in the commit log.
>>>>>
>>>>> I don't have a test case for this, no. But as the change was pretty much
>>>>> straight forward, I sent it anyway.
>>>>
>>>> Ok, but you should still explain that in the commit log. In other words,
>>>> you should always answer the question "Why?" in the commit log.
>>>>
>>>>> I can resend with some more information on this if you like.
>>>>
>>>> No need to resend because of this, I can edit the commit log.
>>>
>>> Hmm, given that I can't even be sure the firmware does the right thing
>>> when instructed this way, we should probably just drop this patch from
>>> the series. The others are more important anyway, as they address bugs
>>> that hit me on actual hardware.
>>
>> IMHO it's useful and if you don't see anything breaking I would prefer
>> to take it anyway. I can't immeadiately say in what use cases this helps
>> or fixes, though. But maybe you (or someone else) could verify that it
>> works correctly by comparing before and after probe requests with a
>> sniffer?
>
> It's certainly not a regression, no. I'm running extensive stress-tests
> with this driver. Then I guess adding the following to the commit log
> should suffice?
>
> "Note that this patch doesn't fix a bug that was observed. The change is
> merely done for the sake of completeness as the hardware supports
> appending IEs in scans. Tests show that network scans work fine with
> this patch applied."
Looks good, thanks. I see that you submitted v2 already with this added.
--
Kalle Valo
Daniel Mack <[email protected]> writes:
> When the ieee8022 core passes IE elements in the scan request, append
You mean mac80211?
And yeah, the ieee80211_ prefix is confusing. Many many years I
started to change that to mac80211_ but gave up :(
> them to the firmware message. The driver currently tells the core that
> it is capable of attaching up to WCN36XX_MAX_SCAN_IE_LEN octets, but
> doesn't actually pass them to the the hardware.
>
> Some defines were moved around to avoid cyclic include dependencies.
Does this fix anything or change functionality somehow? You should
document that also in the commit log.
--
Kalle Valo
On Monday, April 16, 2018 04:41 PM, Kalle Valo wrote:
> Daniel Mack <[email protected]> writes:
>
>> On Monday, April 16, 2018 04:13 PM, Kalle Valo wrote:
>>> Daniel Mack <[email protected]> writes:
>>>> On Monday, April 16, 2018 04:03 PM, Kalle Valo wrote:
>>>>> Daniel Mack <[email protected]> writes:
>>
>>>>>> them to the firmware message. The driver currently tells the core that
>>>>>> it is capable of attaching up to WCN36XX_MAX_SCAN_IE_LEN octets, but
>>>>>> doesn't actually pass them to the the hardware.
>>>>>>
>>>>>> Some defines were moved around to avoid cyclic include dependencies.
>>>>>
>>>>> Does this fix anything or change functionality somehow? You should
>>>>> document that also in the commit log.
>>>>
>>>> I don't have a test case for this, no. But as the change was pretty much
>>>> straight forward, I sent it anyway.
>>>
>>> Ok, but you should still explain that in the commit log. In other words,
>>> you should always answer the question "Why?" in the commit log.
>>>
>>>> I can resend with some more information on this if you like.
>>>
>>> No need to resend because of this, I can edit the commit log.
>>
>> Hmm, given that I can't even be sure the firmware does the right thing
>> when instructed this way, we should probably just drop this patch from
>> the series. The others are more important anyway, as they address bugs
>> that hit me on actual hardware.
>
> IMHO it's useful and if you don't see anything breaking I would prefer
> to take it anyway. I can't immeadiately say in what use cases this helps
> or fixes, though. But maybe you (or someone else) could verify that it
> works correctly by comparing before and after probe requests with a
> sniffer?
It's certainly not a regression, no. I'm running extensive stress-tests
with this driver. Then I guess adding the following to the commit log
should suffice?
"Note that this patch doesn't fix a bug that was observed. The change is
merely done for the sake of completeness as the hardware supports
appending IEs in scans. Tests show that network scans work fine with
this patch applied."
Thanks,
Daniel
Daniel Mack <[email protected]> writes:
> On Monday, April 16, 2018 04:03 PM, Kalle Valo wrote:
>> Daniel Mack <[email protected]> writes:
>>
>>> When the ieee8022 core passes IE elements in the scan request, append
>>
>> You mean mac80211?
>>
>> And yeah, the ieee80211_ prefix is confusing. Many many years I
>> started to change that to mac80211_ but gave up :(
>
> Ah, yeah. I was just referring to the wifi core driver stack.
We call the components mac80211 and cfg80211, please don't invent new
names for them.
>>> them to the firmware message. The driver currently tells the core that
>>> it is capable of attaching up to WCN36XX_MAX_SCAN_IE_LEN octets, but
>>> doesn't actually pass them to the the hardware.
>>>
>>> Some defines were moved around to avoid cyclic include dependencies.
>>
>> Does this fix anything or change functionality somehow? You should
>> document that also in the commit log.
>
> I don't have a test case for this, no. But as the change was pretty much
> straight forward, I sent it anyway.
Ok, but you should still explain that in the commit log. In other words,
you should always answer the question "Why?" in the commit log.
> I can resend with some more information on this if you like.
No need to resend because of this, I can edit the commit log.
--
Kalle Valo
When the ieee8022 core passes IE elements in the scan request, append
them to the firmware message. The driver currently tells the core that
it is capable of attaching up to WCN36XX_MAX_SCAN_IE_LEN octets, but
doesn't actually pass them to the the hardware.
Some defines were moved around to avoid cyclic include dependencies.
Signed-off-by: Daniel Mack <[email protected]>
---
drivers/net/wireless/ath/wcn36xx/hal.h | 8 +++++++-
drivers/net/wireless/ath/wcn36xx/smd.c | 12 ++++++++++++
drivers/net/wireless/ath/wcn36xx/wcn36xx.h | 6 ------
3 files changed, 19 insertions(+), 7 deletions(-)
diff --git a/drivers/net/wireless/ath/wcn36xx/hal.h b/drivers/net/wireless/ath/wcn36xx/hal.h
index 182963522941..2aed6c233508 100644
--- a/drivers/net/wireless/ath/wcn36xx/hal.h
+++ b/drivers/net/wireless/ath/wcn36xx/hal.h
@@ -88,6 +88,12 @@
/* version string max length (including NULL) */
#define WCN36XX_HAL_VERSION_LENGTH 64
+/* How many frames until we start a-mpdu TX session */
+#define WCN36XX_AMPDU_START_THRESH 20
+
+#define WCN36XX_MAX_SCAN_SSIDS 9
+#define WCN36XX_MAX_SCAN_IE_LEN 500
+
/* message types for messages exchanged between WDI and HAL */
enum wcn36xx_hal_host_msg_type {
/* Init/De-Init */
@@ -1170,7 +1176,7 @@ struct wcn36xx_hal_start_scan_offload_req_msg {
/* IE field */
u16 ie_len;
- u8 ie[0];
+ u8 ie[WCN36XX_MAX_SCAN_IE_LEN];
} __packed;
struct wcn36xx_hal_start_scan_offload_rsp_msg {
diff --git a/drivers/net/wireless/ath/wcn36xx/smd.c b/drivers/net/wireless/ath/wcn36xx/smd.c
index 09320c3f6bd0..ea74f2b92df5 100644
--- a/drivers/net/wireless/ath/wcn36xx/smd.c
+++ b/drivers/net/wireless/ath/wcn36xx/smd.c
@@ -620,9 +620,13 @@ int wcn36xx_smd_finish_scan(struct wcn36xx *wcn,
int wcn36xx_smd_start_hw_scan(struct wcn36xx *wcn, struct ieee80211_vif *vif,
struct cfg80211_scan_request *req)
{
+ struct wcn36xx_vif *vif_priv = wcn36xx_vif_to_priv(vif);
struct wcn36xx_hal_start_scan_offload_req_msg msg_body;
int ret, i;
+ if (req->ie_len > WCN36XX_MAX_SCAN_IE_LEN)
+ return -EINVAL;
+
mutex_lock(&wcn->hal_mutex);
INIT_HAL_MSG(msg_body, WCN36XX_HAL_START_SCAN_OFFLOAD_REQ);
@@ -647,6 +651,14 @@ int wcn36xx_smd_start_hw_scan(struct wcn36xx *wcn, struct ieee80211_vif *vif,
for (i = 0; i < msg_body.num_channel; i++)
msg_body.channels[i] = req->channels[i]->hw_value;
+ msg_body.header.len -= WCN36XX_MAX_SCAN_IE_LEN;
+
+ if (req->ie_len > 0) {
+ msg_body.ie_len = req->ie_len;
+ msg_body.header.len += req->ie_len;
+ memcpy(msg_body.ie, req->ie, req->ie_len);
+ }
+
PREPARE_HAL_BUF(wcn->hal_buf, msg_body);
wcn36xx_dbg(WCN36XX_DBG_HAL,
diff --git a/drivers/net/wireless/ath/wcn36xx/wcn36xx.h b/drivers/net/wireless/ath/wcn36xx/wcn36xx.h
index 22e05cb4eb98..9343989d1169 100644
--- a/drivers/net/wireless/ath/wcn36xx/wcn36xx.h
+++ b/drivers/net/wireless/ath/wcn36xx/wcn36xx.h
@@ -32,12 +32,6 @@
#define WLAN_NV_FILE "wlan/prima/WCNSS_qcom_wlan_nv.bin"
#define WCN36XX_AGGR_BUFFER_SIZE 64
-/* How many frames until we start a-mpdu TX session */
-#define WCN36XX_AMPDU_START_THRESH 20
-
-#define WCN36XX_MAX_SCAN_SSIDS 9
-#define WCN36XX_MAX_SCAN_IE_LEN 500
-
extern unsigned int wcn36xx_dbg_mask;
enum wcn36xx_debug_mask {
--
2.14.3
On 16 April 2018 at 16:16, Daniel Mack <[email protected]> wrote:
> Here's another series of 5 patches for wcn36xx that address some issues
> with scanning. The first one is the most important one.
>
Nicely done. Thanks !
>
> Daniel
>
> Daniel Mack (5):
> wcn36xx: abort scan request when 'dequeued' indicator is sent
> wcn36xx: cancel pending scan request when interface goes down
> wcn36xx: handle scan cancellation when firmware support is missing
> wcn36xx: send bss_type in scan requests
> wcn36xx: pass information elements in scan requests
>
> drivers/net/wireless/ath/wcn36xx/hal.h | 8 +++++++-
> drivers/net/wireless/ath/wcn36xx/main.c | 29 +++++++++++++++++++++++++----
> drivers/net/wireless/ath/wcn36xx/smd.c | 20 +++++++++++++++++---
> drivers/net/wireless/ath/wcn36xx/wcn36xx.h | 7 +------
> 4 files changed, 50 insertions(+), 14 deletions(-)
>
> --
> 2.14.3
>
Pass the bss_type of the currently configured BSS in the message for the
scan request. Therefore, that setting needs to be kept in struct
wcn36xx_vif.
This seems to be only interesting when scanning for a specific SSID
and doesn't matter for regular wildcard scans.
Signed-off-by: Daniel Mack <[email protected]>
---
drivers/net/wireless/ath/wcn36xx/smd.c | 4 +++-
drivers/net/wireless/ath/wcn36xx/wcn36xx.h | 1 +
2 files changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/net/wireless/ath/wcn36xx/smd.c b/drivers/net/wireless/ath/wcn36xx/smd.c
index b7964f79d837..09320c3f6bd0 100644
--- a/drivers/net/wireless/ath/wcn36xx/smd.c
+++ b/drivers/net/wireless/ath/wcn36xx/smd.c
@@ -631,6 +631,7 @@ int wcn36xx_smd_start_hw_scan(struct wcn36xx *wcn, struct ieee80211_vif *vif,
msg_body.max_ch_time = 100;
msg_body.scan_hidden = 1;
memcpy(msg_body.mac, vif->addr, ETH_ALEN);
+ msg_body.bss_type = vif_priv->bss_type;
msg_body.p2p_search = vif->p2p;
msg_body.num_ssid = min_t(u8, req->n_ssids, ARRAY_SIZE(msg_body.ssids));
@@ -1399,9 +1400,10 @@ int wcn36xx_smd_config_bss(struct wcn36xx *wcn, struct ieee80211_vif *vif,
bss->spectrum_mgt_enable = 0;
bss->tx_mgmt_power = 0;
bss->max_tx_power = WCN36XX_MAX_POWER(wcn);
-
bss->action = update;
+ vif_priv->bss_type = bss->bss_type;
+
wcn36xx_dbg(WCN36XX_DBG_HAL,
"hal config bss bssid %pM self_mac_addr %pM bss_type %d oper_mode %d nw_type %d\n",
bss->bssid, bss->self_mac_addr, bss->bss_type,
diff --git a/drivers/net/wireless/ath/wcn36xx/wcn36xx.h b/drivers/net/wireless/ath/wcn36xx/wcn36xx.h
index 5854adf43f3a..22e05cb4eb98 100644
--- a/drivers/net/wireless/ath/wcn36xx/wcn36xx.h
+++ b/drivers/net/wireless/ath/wcn36xx/wcn36xx.h
@@ -123,6 +123,7 @@ struct wcn36xx_vif {
bool is_joining;
bool sta_assoc;
struct wcn36xx_hal_mac_ssid ssid;
+ enum wcn36xx_hal_bss_type bss_type;
/* Power management */
enum wcn36xx_power_state pw_state;
--
2.14.3
When the network interface goes down while a scan request is still
pending that can't be stopped due to firmware hickups, wcn->scan_req
remains set, even though the hardware is deinitialized. This results
in -EBUSY for all scan requests after the interface was brought up
again.
Fix this by explicitly completing pending scan requests in
wcn36xx_stop().
Signed-off-by: Daniel Mack <[email protected]>
---
drivers/net/wireless/ath/wcn36xx/main.c | 13 +++++++++++++
1 file changed, 13 insertions(+)
diff --git a/drivers/net/wireless/ath/wcn36xx/main.c b/drivers/net/wireless/ath/wcn36xx/main.c
index 1b17c35a7944..e5ef2cbb622f 100644
--- a/drivers/net/wireless/ath/wcn36xx/main.c
+++ b/drivers/net/wireless/ath/wcn36xx/main.c
@@ -353,6 +353,19 @@ static void wcn36xx_stop(struct ieee80211_hw *hw)
wcn36xx_dbg(WCN36XX_DBG_MAC, "mac stop\n");
+ cancel_work_sync(&wcn->scan_work);
+
+ mutex_lock(&wcn->scan_lock);
+ if (wcn->scan_req) {
+ struct cfg80211_scan_info scan_info = {
+ .aborted = true,
+ };
+
+ ieee80211_scan_completed(wcn->hw, &scan_info);
+ }
+ wcn->scan_req = NULL;
+ mutex_unlock(&wcn->scan_lock);
+
wcn36xx_debugfs_exit(wcn);
wcn36xx_smd_stop(wcn);
wcn36xx_dxe_deinit(wcn);
--
2.14.3
On Monday, April 16, 2018 04:03 PM, Kalle Valo wrote:
> Daniel Mack <[email protected]> writes:
>
>> When the ieee8022 core passes IE elements in the scan request, append
>
> You mean mac80211?
>
> And yeah, the ieee80211_ prefix is confusing. Many many years I
> started to change that to mac80211_ but gave up :(
Ah, yeah. I was just referring to the wifi core driver stack.
>> them to the firmware message. The driver currently tells the core that
>> it is capable of attaching up to WCN36XX_MAX_SCAN_IE_LEN octets, but
>> doesn't actually pass them to the the hardware.
>>
>> Some defines were moved around to avoid cyclic include dependencies.
>
> Does this fix anything or change functionality somehow? You should
> document that also in the commit log.
I don't have a test case for this, no. But as the change was pretty much
straight forward, I sent it anyway.
I can resend with some more information on this if you like.
Thanks,
Daniel
On Monday, April 16, 2018 04:13 PM, Kalle Valo wrote:
> Daniel Mack <[email protected]> writes:
>> On Monday, April 16, 2018 04:03 PM, Kalle Valo wrote:
>>> Daniel Mack <[email protected]> writes:
>>>> them to the firmware message. The driver currently tells the core that
>>>> it is capable of attaching up to WCN36XX_MAX_SCAN_IE_LEN octets, but
>>>> doesn't actually pass them to the the hardware.
>>>>
>>>> Some defines were moved around to avoid cyclic include dependencies.
>>>
>>> Does this fix anything or change functionality somehow? You should
>>> document that also in the commit log.
>>
>> I don't have a test case for this, no. But as the change was pretty much
>> straight forward, I sent it anyway.
>
> Ok, but you should still explain that in the commit log. In other words,
> you should always answer the question "Why?" in the commit log.
>
>> I can resend with some more information on this if you like.
>
> No need to resend because of this, I can edit the commit log.
>
Hmm, given that I can't even be sure the firmware does the right thing
when instructed this way, we should probably just drop this patch from
the series. The others are more important anyway, as they address bugs
that hit me on actual hardware.
Thanks,
Daniel
When the firmware sends a WCN36XX_HAL_SCAN_IND_DEQUEUED indication,
the request is apparently no longer valid. Attempts to stop the hardware
scan request subsequently will lead to the following error message, and
the hardware is no longer able to communicate with any AP:
[ 57.917186] wcn36xx: ERROR hal_stop_scan_offload response failed err=5
Interpreting this indicator message as scan abortion fixes this.
While at it, add a newline to a debug print.
Signed-off-by: Daniel Mack <[email protected]>
---
drivers/net/wireless/ath/wcn36xx/smd.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/net/wireless/ath/wcn36xx/smd.c b/drivers/net/wireless/ath/wcn36xx/smd.c
index 297a785d0785..b7964f79d837 100644
--- a/drivers/net/wireless/ath/wcn36xx/smd.c
+++ b/drivers/net/wireless/ath/wcn36xx/smd.c
@@ -2140,10 +2140,11 @@ static int wcn36xx_smd_hw_scan_ind(struct wcn36xx *wcn, void *buf, size_t len)
return -EIO;
}
- wcn36xx_dbg(WCN36XX_DBG_HAL, "scan indication (type %x)", rsp->type);
+ wcn36xx_dbg(WCN36XX_DBG_HAL, "scan indication (type %x)\n", rsp->type);
switch (rsp->type) {
case WCN36XX_HAL_SCAN_IND_FAILED:
+ case WCN36XX_HAL_SCAN_IND_DEQUEUED:
scan_info.aborted = true;
/* fall through */
case WCN36XX_HAL_SCAN_IND_COMPLETED:
@@ -2156,7 +2157,6 @@ static int wcn36xx_smd_hw_scan_ind(struct wcn36xx *wcn, void *buf, size_t len)
break;
case WCN36XX_HAL_SCAN_IND_STARTED:
case WCN36XX_HAL_SCAN_IND_FOREIGN_CHANNEL:
- case WCN36XX_HAL_SCAN_IND_DEQUEUED:
case WCN36XX_HAL_SCAN_IND_PREEMPTED:
case WCN36XX_HAL_SCAN_IND_RESTARTED:
break;
--
2.14.3