All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/5] wcn36xx: scan related patches
@ 2018-04-16 13:16 Daniel Mack
  2018-04-16 13:16 ` [PATCH 1/5] wcn36xx: abort scan request when 'dequeued' indicator is sent Daniel Mack
                   ` (5 more replies)
  0 siblings, 6 replies; 14+ messages in thread
From: Daniel Mack @ 2018-04-16 13:16 UTC (permalink / raw)
  To: linux-wireless
  Cc: wcn36xx, kvalo, loic.poulain, rfried, bjorn.andersson, Daniel Mack

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* [PATCH 1/5] wcn36xx: abort scan request when 'dequeued' indicator is sent
  2018-04-16 13:16 [PATCH 0/5] wcn36xx: scan related patches Daniel Mack
@ 2018-04-16 13:16 ` Daniel Mack
  2018-04-16 13:16 ` [PATCH 2/5] wcn36xx: cancel pending scan request when interface goes down Daniel Mack
                   ` (4 subsequent siblings)
  5 siblings, 0 replies; 14+ messages in thread
From: Daniel Mack @ 2018-04-16 13:16 UTC (permalink / raw)
  To: linux-wireless
  Cc: wcn36xx, kvalo, loic.poulain, rfried, bjorn.andersson, Daniel Mack

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 <daniel@zonque.org>
---
 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

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH 2/5] wcn36xx: cancel pending scan request when interface goes down
  2018-04-16 13:16 [PATCH 0/5] wcn36xx: scan related patches Daniel Mack
  2018-04-16 13:16 ` [PATCH 1/5] wcn36xx: abort scan request when 'dequeued' indicator is sent Daniel Mack
@ 2018-04-16 13:16 ` Daniel Mack
  2018-04-16 13:16 ` [PATCH 3/5] wcn36xx: handle scan cancellation when firmware support is missing Daniel Mack
                   ` (3 subsequent siblings)
  5 siblings, 0 replies; 14+ messages in thread
From: Daniel Mack @ 2018-04-16 13:16 UTC (permalink / raw)
  To: linux-wireless
  Cc: wcn36xx, kvalo, loic.poulain, rfried, bjorn.andersson, Daniel Mack

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 <daniel@zonque.org>
---
 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

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH 3/5] wcn36xx: handle scan cancellation when firmware support is missing
  2018-04-16 13:16 [PATCH 0/5] wcn36xx: scan related patches Daniel Mack
  2018-04-16 13:16 ` [PATCH 1/5] wcn36xx: abort scan request when 'dequeued' indicator is sent Daniel Mack
  2018-04-16 13:16 ` [PATCH 2/5] wcn36xx: cancel pending scan request when interface goes down Daniel Mack
@ 2018-04-16 13:16 ` Daniel Mack
  2018-04-16 13:16 ` [PATCH 4/5] wcn36xx: send bss_type in scan requests Daniel Mack
                   ` (2 subsequent siblings)
  5 siblings, 0 replies; 14+ messages in thread
From: Daniel Mack @ 2018-04-16 13:16 UTC (permalink / raw)
  To: linux-wireless
  Cc: wcn36xx, kvalo, loic.poulain, rfried, bjorn.andersson, Daniel Mack

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 <daniel@zonque.org>
---
 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

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH 4/5] wcn36xx: send bss_type in scan requests
  2018-04-16 13:16 [PATCH 0/5] wcn36xx: scan related patches Daniel Mack
                   ` (2 preceding siblings ...)
  2018-04-16 13:16 ` [PATCH 3/5] wcn36xx: handle scan cancellation when firmware support is missing Daniel Mack
@ 2018-04-16 13:16 ` Daniel Mack
  2018-04-16 13:16 ` [PATCH 5/5] wcn36xx: pass information elements " Daniel Mack
  2018-04-16 22:24 ` [PATCH 0/5] wcn36xx: scan related patches Ramon Fried
  5 siblings, 0 replies; 14+ messages in thread
From: Daniel Mack @ 2018-04-16 13:16 UTC (permalink / raw)
  To: linux-wireless
  Cc: wcn36xx, kvalo, loic.poulain, rfried, bjorn.andersson, Daniel Mack

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 <daniel@zonque.org>
---
 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

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH 5/5] wcn36xx: pass information elements in scan requests
  2018-04-16 13:16 [PATCH 0/5] wcn36xx: scan related patches Daniel Mack
                   ` (3 preceding siblings ...)
  2018-04-16 13:16 ` [PATCH 4/5] wcn36xx: send bss_type in scan requests Daniel Mack
@ 2018-04-16 13:16 ` Daniel Mack
  2018-04-16 14:03   ` Kalle Valo
  2018-04-16 22:24 ` [PATCH 0/5] wcn36xx: scan related patches Ramon Fried
  5 siblings, 1 reply; 14+ messages in thread
From: Daniel Mack @ 2018-04-16 13:16 UTC (permalink / raw)
  To: linux-wireless
  Cc: wcn36xx, kvalo, loic.poulain, rfried, bjorn.andersson, Daniel Mack

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 <daniel@zonque.org>
---
 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

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* Re: [PATCH 5/5] wcn36xx: pass information elements in scan requests
  2018-04-16 13:16 ` [PATCH 5/5] wcn36xx: pass information elements " Daniel Mack
@ 2018-04-16 14:03   ` Kalle Valo
  2018-04-16 14:06     ` Daniel Mack
  0 siblings, 1 reply; 14+ messages in thread
From: Kalle Valo @ 2018-04-16 14:03 UTC (permalink / raw)
  To: Daniel Mack
  Cc: linux-wireless, wcn36xx, loic.poulain, rfried, bjorn.andersson

Daniel Mack <daniel@zonque.org> 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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 5/5] wcn36xx: pass information elements in scan requests
  2018-04-16 14:03   ` Kalle Valo
@ 2018-04-16 14:06     ` Daniel Mack
  2018-04-16 14:13       ` Kalle Valo
  0 siblings, 1 reply; 14+ messages in thread
From: Daniel Mack @ 2018-04-16 14:06 UTC (permalink / raw)
  To: Kalle Valo; +Cc: linux-wireless, wcn36xx, loic.poulain, rfried, bjorn.andersson

On Monday, April 16, 2018 04:03 PM, Kalle Valo wrote:
> Daniel Mack <daniel@zonque.org> 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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 5/5] wcn36xx: pass information elements in scan requests
  2018-04-16 14:06     ` Daniel Mack
@ 2018-04-16 14:13       ` Kalle Valo
  2018-04-16 14:29         ` Daniel Mack
  0 siblings, 1 reply; 14+ messages in thread
From: Kalle Valo @ 2018-04-16 14:13 UTC (permalink / raw)
  To: Daniel Mack
  Cc: linux-wireless, wcn36xx, loic.poulain, rfried, bjorn.andersson

Daniel Mack <daniel@zonque.org> writes:

> On Monday, April 16, 2018 04:03 PM, Kalle Valo wrote:
>> Daniel Mack <daniel@zonque.org> 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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 5/5] wcn36xx: pass information elements in scan requests
  2018-04-16 14:13       ` Kalle Valo
@ 2018-04-16 14:29         ` Daniel Mack
  2018-04-16 14:41           ` Kalle Valo
  0 siblings, 1 reply; 14+ messages in thread
From: Daniel Mack @ 2018-04-16 14:29 UTC (permalink / raw)
  To: Kalle Valo; +Cc: linux-wireless, wcn36xx, loic.poulain, rfried, bjorn.andersson

On Monday, April 16, 2018 04:13 PM, Kalle Valo wrote:
> Daniel Mack <daniel@zonque.org> writes:
>> On Monday, April 16, 2018 04:03 PM, Kalle Valo wrote:
>>> Daniel Mack <daniel@zonque.org> 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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 5/5] wcn36xx: pass information elements in scan requests
  2018-04-16 14:29         ` Daniel Mack
@ 2018-04-16 14:41           ` Kalle Valo
  2018-04-16 15:03             ` Daniel Mack
  0 siblings, 1 reply; 14+ messages in thread
From: Kalle Valo @ 2018-04-16 14:41 UTC (permalink / raw)
  To: Daniel Mack
  Cc: linux-wireless, wcn36xx, loic.poulain, rfried, bjorn.andersson

Daniel Mack <daniel@zonque.org> writes:

> On Monday, April 16, 2018 04:13 PM, Kalle Valo wrote:
>> Daniel Mack <daniel@zonque.org> writes:
>>> On Monday, April 16, 2018 04:03 PM, Kalle Valo wrote:
>>>> Daniel Mack <daniel@zonque.org> 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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 5/5] wcn36xx: pass information elements in scan requests
  2018-04-16 14:41           ` Kalle Valo
@ 2018-04-16 15:03             ` Daniel Mack
  2018-04-23 14:14               ` Kalle Valo
  0 siblings, 1 reply; 14+ messages in thread
From: Daniel Mack @ 2018-04-16 15:03 UTC (permalink / raw)
  To: Kalle Valo; +Cc: linux-wireless, wcn36xx, loic.poulain, rfried, bjorn.andersson

On Monday, April 16, 2018 04:41 PM, Kalle Valo wrote:
> Daniel Mack <daniel@zonque.org> writes:
> 
>> On Monday, April 16, 2018 04:13 PM, Kalle Valo wrote:
>>> Daniel Mack <daniel@zonque.org> writes:
>>>> On Monday, April 16, 2018 04:03 PM, Kalle Valo wrote:
>>>>> Daniel Mack <daniel@zonque.org> 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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 0/5] wcn36xx: scan related patches
  2018-04-16 13:16 [PATCH 0/5] wcn36xx: scan related patches Daniel Mack
                   ` (4 preceding siblings ...)
  2018-04-16 13:16 ` [PATCH 5/5] wcn36xx: pass information elements " Daniel Mack
@ 2018-04-16 22:24 ` Ramon Fried
  5 siblings, 0 replies; 14+ messages in thread
From: Ramon Fried @ 2018-04-16 22:24 UTC (permalink / raw)
  To: Daniel Mack
  Cc: linux-wireless, wcn36xx, kvalo, Loic Poulain, Ramon Fried,
	Bjorn Andersson

On 16 April 2018 at 16:16, Daniel Mack <daniel@zonque.org> 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
>

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 5/5] wcn36xx: pass information elements in scan requests
  2018-04-16 15:03             ` Daniel Mack
@ 2018-04-23 14:14               ` Kalle Valo
  0 siblings, 0 replies; 14+ messages in thread
From: Kalle Valo @ 2018-04-23 14:14 UTC (permalink / raw)
  To: Daniel Mack
  Cc: linux-wireless, wcn36xx, loic.poulain, rfried, bjorn.andersson

Daniel Mack <daniel@zonque.org> writes:

> On Monday, April 16, 2018 04:41 PM, Kalle Valo wrote:
>> Daniel Mack <daniel@zonque.org> writes:
>> 
>>> On Monday, April 16, 2018 04:13 PM, Kalle Valo wrote:
>>>> Daniel Mack <daniel@zonque.org> writes:
>>>>> On Monday, April 16, 2018 04:03 PM, Kalle Valo wrote:
>>>>>> Daniel Mack <daniel@zonque.org> 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

^ permalink raw reply	[flat|nested] 14+ messages in thread

end of thread, other threads:[~2018-04-23 14:14 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-04-16 13:16 [PATCH 0/5] wcn36xx: scan related patches Daniel Mack
2018-04-16 13:16 ` [PATCH 1/5] wcn36xx: abort scan request when 'dequeued' indicator is sent Daniel Mack
2018-04-16 13:16 ` [PATCH 2/5] wcn36xx: cancel pending scan request when interface goes down Daniel Mack
2018-04-16 13:16 ` [PATCH 3/5] wcn36xx: handle scan cancellation when firmware support is missing Daniel Mack
2018-04-16 13:16 ` [PATCH 4/5] wcn36xx: send bss_type in scan requests Daniel Mack
2018-04-16 13:16 ` [PATCH 5/5] wcn36xx: pass information elements " Daniel Mack
2018-04-16 14:03   ` Kalle Valo
2018-04-16 14:06     ` Daniel Mack
2018-04-16 14:13       ` Kalle Valo
2018-04-16 14:29         ` Daniel Mack
2018-04-16 14:41           ` Kalle Valo
2018-04-16 15:03             ` Daniel Mack
2018-04-23 14:14               ` Kalle Valo
2018-04-16 22:24 ` [PATCH 0/5] wcn36xx: scan related patches Ramon Fried

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.