[PATCH 0/5] wil6210 patches

[PATCH 0/5] wil6210 patches

[Vladimir Kondratiev]

Small bits here and there.
Cosmetic but visible is firmware name change

Vladimir Kondratiev (5):
  wil6210: fw debug mode
  wil6210: debug [add|del]_key operations
  wil6210: trace disconnect source
  wil6210: stop_ap to leave interface closed
  wil6210: update FW file name

 drivers/net/wireless/ath/wil6210/cfg80211.c | 15 +++++++++++----
 drivers/net/wireless/ath/wil6210/main.c     | 15 +++++++++++++++
 drivers/net/wireless/ath/wil6210/netdev.c   |  5 +++++
 drivers/net/wireless/ath/wil6210/pcie_bus.c |  6 ------
 drivers/net/wireless/ath/wil6210/wil6210.h  |  3 ++-
 5 files changed, 33 insertions(+), 11 deletions(-)

-- 
2.1.0

[PATCH 1/5] wil6210: fw debug mode

[Vladimir Kondratiev]

refactor module parameter debug_fw to act as "fw debug mode",
where driver do nothing but allow card memory access.

Signed-off-by: Vladimir Kondratiev <qca_vkondrat@qca.qualcomm.com>
---
 drivers/net/wireless/ath/wil6210/main.c     | 15 +++++++++++++++
 drivers/net/wireless/ath/wil6210/netdev.c   |  5 +++++
 drivers/net/wireless/ath/wil6210/pcie_bus.c |  6 ------
 drivers/net/wireless/ath/wil6210/wil6210.h  |  1 +
 4 files changed, 21 insertions(+), 6 deletions(-)

diff --git a/drivers/net/wireless/ath/wil6210/main.c b/drivers/net/wireless/ath/wil6210/main.c
index c2a2384..0623d8c 100644
--- a/drivers/net/wireless/ath/wil6210/main.c
+++ b/drivers/net/wireless/ath/wil6210/main.c
@@ -25,6 +25,10 @@
 #define WAIT_FOR_DISCONNECT_TIMEOUT_MS 2000
 #define WAIT_FOR_DISCONNECT_INTERVAL_MS 10
 
+bool debug_fw; /* = false; */
+module_param(debug_fw, bool, S_IRUGO);
+MODULE_PARM_DESC(debug_fw, " do not perform card reset. For FW debug");
+
 bool no_fw_recovery;
 module_param(no_fw_recovery, bool, S_IRUGO | S_IWUSR);
 MODULE_PARM_DESC(no_fw_recovery, " disable automatic FW error recovery");
@@ -686,6 +690,17 @@ int wil_reset(struct wil6210_priv *wil, bool load_fw)
 	WARN_ON(!mutex_is_locked(&wil->mutex));
 	WARN_ON(test_bit(wil_status_napi_en, wil->status));
 
+	if (debug_fw) {
+		static const u8 mac[ETH_ALEN] = {
+			0x00, 0xde, 0xad, 0x12, 0x34, 0x56,
+		};
+		struct net_device *ndev = wil_to_ndev(wil);
+
+		ether_addr_copy(ndev->perm_addr, mac);
+		ether_addr_copy(ndev->dev_addr, ndev->perm_addr);
+		return 0;
+	}
+
 	cancel_work_sync(&wil->disconnect_worker);
 	wil6210_disconnect(wil, NULL, WLAN_REASON_DEAUTH_LEAVING, false);
 	wil_bcast_fini(wil);
diff --git a/drivers/net/wireless/ath/wil6210/netdev.c b/drivers/net/wireless/ath/wil6210/netdev.c
index f2f7ea2..6042f61 100644
--- a/drivers/net/wireless/ath/wil6210/netdev.c
+++ b/drivers/net/wireless/ath/wil6210/netdev.c
@@ -24,6 +24,11 @@ static int wil_open(struct net_device *ndev)
 
 	wil_dbg_misc(wil, "%s()\n", __func__);
 
+	if (debug_fw) {
+		wil_err(wil, "%s() while in debug_fw mode\n", __func__);
+		return -EINVAL;
+	}
+
 	return wil_up(wil);
 }
 
diff --git a/drivers/net/wireless/ath/wil6210/pcie_bus.c b/drivers/net/wireless/ath/wil6210/pcie_bus.c
index 1099861..58c7916 100644
--- a/drivers/net/wireless/ath/wil6210/pcie_bus.c
+++ b/drivers/net/wireless/ath/wil6210/pcie_bus.c
@@ -27,10 +27,6 @@ MODULE_PARM_DESC(use_msi,
 		 " Use MSI interrupt: "
 		 "0 - don't, 1 - (default) - single, or 3");
 
-static bool debug_fw; /* = false; */
-module_param(debug_fw, bool, S_IRUGO);
-MODULE_PARM_DESC(debug_fw, " load driver if FW not ready. For FW debug");
-
 static
 void wil_set_capabilities(struct wil6210_priv *wil)
 {
@@ -133,8 +129,6 @@ static int wil_if_pcie_enable(struct wil6210_priv *wil)
 	mutex_lock(&wil->mutex);
 	rc = wil_reset(wil, false);
 	mutex_unlock(&wil->mutex);
-	if (debug_fw)
-		rc = 0;
 	if (rc)
 		goto release_irq;
 
diff --git a/drivers/net/wireless/ath/wil6210/wil6210.h b/drivers/net/wireless/ath/wil6210/wil6210.h
index 4310972..f4681e3 100644
--- a/drivers/net/wireless/ath/wil6210/wil6210.h
+++ b/drivers/net/wireless/ath/wil6210/wil6210.h
@@ -29,6 +29,7 @@ extern unsigned short rx_ring_overflow_thrsh;
 extern int agg_wsize;
 extern u32 vring_idle_trsh;
 extern bool rx_align_2;
+extern bool debug_fw;
 
 #define WIL_NAME "wil6210"
 #define WIL_FW_NAME "wil6210.fw" /* code */
-- 
2.1.0

[PATCH 2/5] wil6210: debug [add|del]_key operations

[Vladimir Kondratiev]

Provide info for [add|del]_key.

Signed-off-by: Vladimir Kondratiev <qca_vkondrat@qca.qualcomm.com>
---
 drivers/net/wireless/ath/wil6210/cfg80211.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/net/wireless/ath/wil6210/cfg80211.c b/drivers/net/wireless/ath/wil6210/cfg80211.c
index 8f7596f..d027a7f 100644
--- a/drivers/net/wireless/ath/wil6210/cfg80211.c
+++ b/drivers/net/wireless/ath/wil6210/cfg80211.c
@@ -569,6 +569,9 @@ static int wil_cfg80211_add_key(struct wiphy *wiphy,
 {
 	struct wil6210_priv *wil = wiphy_to_wil(wiphy);
 
+	wil_dbg_misc(wil, "%s(%pM[%d] %s)\n", __func__, mac_addr, key_index,
+		     pairwise ? "PTK" : "GTK");
+
 	/* group key is not used */
 	if (!pairwise)
 		return 0;
@@ -584,6 +587,9 @@ static int wil_cfg80211_del_key(struct wiphy *wiphy,
 {
 	struct wil6210_priv *wil = wiphy_to_wil(wiphy);
 
+	wil_dbg_misc(wil, "%s(%pM[%d] %s)\n", __func__, mac_addr, key_index,
+		     pairwise ? "PTK" : "GTK");
+
 	/* group key is not used */
 	if (!pairwise)
 		return 0;
-- 
2.1.0

[PATCH 3/5] wil6210: trace disconnect source

[Vladimir Kondratiev]

Trace where wil6210_disconnect() is called from.

Signed-off-by: Vladimir Kondratiev <qca_vkondrat@qca.qualcomm.com>
---
 drivers/net/wireless/ath/wil6210/cfg80211.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/net/wireless/ath/wil6210/cfg80211.c b/drivers/net/wireless/ath/wil6210/cfg80211.c
index d027a7f..ec46a94 100644
--- a/drivers/net/wireless/ath/wil6210/cfg80211.c
+++ b/drivers/net/wireless/ath/wil6210/cfg80211.c
@@ -507,6 +507,8 @@ static int wil_cfg80211_disconnect(struct wiphy *wiphy,
 	int rc;
 	struct wil6210_priv *wil = wiphy_to_wil(wiphy);
 
+	wil_dbg_misc(wil, "%s(reason=%d)\n", __func__, reason_code);
+
 	rc = wmi_send(wil, WMI_DISCONNECT_CMDID, NULL, 0);
 
 	return rc;
@@ -836,6 +838,9 @@ static int wil_cfg80211_del_station(struct wiphy *wiphy,
 {
 	struct wil6210_priv *wil = wiphy_to_wil(wiphy);
 
+	wil_dbg_misc(wil, "%s(%pM, reason=%d)\n", __func__, params->mac,
+		     params->reason_code);
+
 	mutex_lock(&wil->mutex);
 	wil6210_disconnect(wil, params->mac, params->reason_code, false);
 	mutex_unlock(&wil->mutex);
-- 
2.1.0

[PATCH 4/5] wil6210: stop_ap to leave interface closed

[Vladimir Kondratiev]

cfg80211_ops.stop_ap supposed to have interface closed
as post condition. Fulfill this requirement.

Signed-off-by: Vladimir Kondratiev <qca_vkondrat@qca.qualcomm.com>
---
 drivers/net/wireless/ath/wil6210/cfg80211.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/drivers/net/wireless/ath/wil6210/cfg80211.c b/drivers/net/wireless/ath/wil6210/cfg80211.c
index ec46a94..b51ac10 100644
--- a/drivers/net/wireless/ath/wil6210/cfg80211.c
+++ b/drivers/net/wireless/ath/wil6210/cfg80211.c
@@ -822,13 +822,9 @@ static int wil_cfg80211_stop_ap(struct wiphy *wiphy,
 	wmi_pcp_stop(wil);
 
 	__wil_down(wil);
-	__wil_up(wil);
 
 	mutex_unlock(&wil->mutex);
 
-	/* some functions above might fail (e.g. __wil_up). Nevertheless, we
-	 * return success because AP has stopped
-	 */
 	return 0;
 }
 
-- 
2.1.0

[PATCH 5/5] wil6210: update FW file name

[Vladimir Kondratiev]

Firmware "board" file name has changed from wil6210.board
to wil6210.brd by the FW generation tools.

Reflect this in the driver.

Signed-off-by: Vladimir Kondratiev <qca_vkondrat@qca.qualcomm.com>
---
 drivers/net/wireless/ath/wil6210/wil6210.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/wireless/ath/wil6210/wil6210.h b/drivers/net/wireless/ath/wil6210/wil6210.h
index f4681e3..abb2080 100644
--- a/drivers/net/wireless/ath/wil6210/wil6210.h
+++ b/drivers/net/wireless/ath/wil6210/wil6210.h
@@ -33,7 +33,7 @@ extern bool debug_fw;
 
 #define WIL_NAME "wil6210"
 #define WIL_FW_NAME "wil6210.fw" /* code */
-#define WIL_FW2_NAME "wil6210.board" /* board & radio parameters */
+#define WIL_FW2_NAME "wil6210.brd" /* board & radio parameters */
 
 #define WIL_MAX_BUS_REQUEST_KBPS 800000 /* ~6.1Gbps */
 
-- 
2.1.0

Re: [PATCH 4/5] wil6210: stop_ap to leave interface closed

[Johannes Berg]

On Mon, 2015-03-30 at 11:28 +0300, Vladimir Kondratiev wrote:
> cfg80211_ops.stop_ap supposed to have interface closed
> as post condition. Fulfill this requirement.

"closed" is rather misleading, since you most certainly should *not* do
dev_close() [and don't, but how should I know reading the commit log?]

johannes

Re: [PATCH 5/5] wil6210: update FW file name

[Kalle Valo]

Vladimir Kondratiev <QCA_vkondrat@QCA.qualcomm.com> writes:

> Firmware "board" file name has changed from wil6210.board
> to wil6210.brd by the FW generation tools.
>
> Reflect this in the driver.
>
> Signed-off-by: Vladimir Kondratiev <qca_vkondrat@qca.qualcomm.com>
> ---
>  drivers/net/wireless/ath/wil6210/wil6210.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/net/wireless/ath/wil6210/wil6210.h b/drivers/net/wireless/ath/wil6210/wil6210.h
> index f4681e3..abb2080 100644
> --- a/drivers/net/wireless/ath/wil6210/wil6210.h
> +++ b/drivers/net/wireless/ath/wil6210/wil6210.h
> @@ -33,7 +33,7 @@ extern bool debug_fw;
>  
>  #define WIL_NAME "wil6210"
>  #define WIL_FW_NAME "wil6210.fw" /* code */
> -#define WIL_FW2_NAME "wil6210.board" /* board & radio parameters */
> +#define WIL_FW2_NAME "wil6210.brd" /* board & radio parameters */

I consider the firmware file names as part of stable user space
interface (older user space need to work with newer kernel etc) and this
change breaks that.

-- 
Kalle Valo

Re: [PATCH 4/5] wil6210: stop_ap to leave interface closed

[Vladimir Kondratiev]

On Monday, March 30, 2015 10:45:56 AM Johannes Berg wrote:
> On Mon, 2015-03-30 at 11:28 +0300, Vladimir Kondratiev wrote:
> > cfg80211_ops.stop_ap supposed to have interface closed
> > as post condition. Fulfill this requirement.
> 
> "closed" is rather misleading, since you most certainly should *not* do
> dev_close() [and don't, but how should I know reading the commit log?]
> 
> johannes
> 

Yes, I see.
I meant that actually stop_ap leaves device in state similar to ndo_stop,
where it is non operational.

Would you suggest better wording? "Inactive", "down", "non operational"
or something else?

Thanks, Vladimir

Re: [PATCH 5/5] wil6210: update FW file name

[Vladimir Kondratiev]

On Monday, March 30, 2015 12:38:08 PM Kalle Valo wrote:
> Vladimir Kondratiev <QCA_vkondrat@QCA.qualcomm.com> writes:
> 
> > Firmware "board" file name has changed from wil6210.board
> > to wil6210.brd by the FW generation tools.
> >
> > Reflect this in the driver.
> >

> > -#define WIL_FW2_NAME "wil6210.board" /* board & radio parameters */
> > +#define WIL_FW2_NAME "wil6210.brd" /* board & radio parameters */
> 
> I consider the firmware file names as part of stable user space
> interface (older user space need to work with newer kernel etc) and this
> change breaks that.
> 

Hardware was not released to the public yet; no one besides developers
in the lab should be affected by this.

Thanks, Vladimir

Re: [PATCH 5/5] wil6210: update FW file name

[Kalle Valo]

Vladimir Kondratiev <QCA_vkondrat@QCA.qualcomm.com> writes:

> On Monday, March 30, 2015 12:38:08 PM Kalle Valo wrote:
>> Vladimir Kondratiev <QCA_vkondrat@QCA.qualcomm.com> writes:
>> 
>> > Firmware "board" file name has changed from wil6210.board
>> > to wil6210.brd by the FW generation tools.
>> >
>> > Reflect this in the driver.
>> >
>
>> > -#define WIL_FW2_NAME "wil6210.board" /* board & radio parameters */
>> > +#define WIL_FW2_NAME "wil6210.brd" /* board & radio parameters */
>> 
>> I consider the firmware file names as part of stable user space
>> interface (older user space need to work with newer kernel etc) and this
>> change breaks that.
>
> Hardware was not released to the public yet; no one besides developers
> in the lab should be affected by this.

Actually that doesn't matter as it's common to run few years old
kernels, especially in the embedded world. So no matter what, this
breaks compatibility with user space and without any real benefit
whatsoever. Now will anyone hit this in practise I don't know.

I guess this change is ok for now as wil6210 hw is not that common, but
in the future please be careful with this.

-- 
Kalle Valo

Re: [PATCH 4/5] wil6210: stop_ap to leave interface closed

[Kalle Valo]

Vladimir Kondratiev <QCA_vkondrat@QCA.qualcomm.com> writes:

> On Monday, March 30, 2015 10:45:56 AM Johannes Berg wrote:
>> On Mon, 2015-03-30 at 11:28 +0300, Vladimir Kondratiev wrote:
>> > cfg80211_ops.stop_ap supposed to have interface closed
>> > as post condition. Fulfill this requirement.
>> 
>> "closed" is rather misleading, since you most certainly should *not* do
>> dev_close() [and don't, but how should I know reading the commit log?]
>> 
>> johannes
>> 
>
> Yes, I see.
> I meant that actually stop_ap leaves device in state similar to ndo_stop,
> where it is non operational.
>
> Would you suggest better wording? "Inactive", "down", "non operational"
> or something else?

Isn't "down" good enough here? So can I change it to:

  cfg80211_ops.stop_ap supposed to have interface down as post
  condition. Fulfill this requirement.

-- 
Kalle Valo

Re: [PATCH 4/5] wil6210: stop_ap to leave interface closed

[Johannes Berg]

>>> "closed" is rather misleading, since you most certainly should *not*
>do
>>> dev_close() [and don't, but how should I know reading the commit
>log?]
>>> 
>>> johannes
>>> 
>>
>> Yes, I see.
>> I meant that actually stop_ap leaves device in state similar to
>ndo_stop,
>> where it is non operational.
>>
>> Would you suggest better wording? "Inactive", "down", "non
>operational"
>> or something else?
>
>Isn't "down" good enough here? So can I change it to:

Well that kinda proves my point - down is wrong.

Carrier turned off is probably the right thing to do here.

johannes

Re: [PATCH 4/5] wil6210: stop_ap to leave interface closed

[Kalle Valo]

Johannes Berg <johannes@sipsolutions.net> writes:

>>>> "closed" is rather misleading, since you most certainly should *not*
>>do
>>>> dev_close() [and don't, but how should I know reading the commit
>>log?]
>>>> 
>>>> johannes
>>>> 
>>>
>>> Yes, I see.
>>> I meant that actually stop_ap leaves device in state similar to
>>ndo_stop,
>>> where it is non operational.
>>>
>>> Would you suggest better wording? "Inactive", "down", "non
>>operational"
>>> or something else?
>>
>>Isn't "down" good enough here? So can I change it to:
>
> Well that kinda proves my point - down is wrong.
>
> Carrier turned off is probably the right thing to do here.

Ok, I change the commit log to:

  cfg80211_ops.stop_ap supposed to have interface carried turned off as
  post condition. Fulfill this requirement.

-- 
Kalle Valo