2017-10-25 13:14:12

by Maya Erez

[permalink] [raw]
Subject: [PATCH 00/12] wil6210 patches

The following patches include:
- Bug fixes
- run-time PM when interface down

Dedy Lansky (1):
wil6210: print human readable names of WMI commands and events

Hamad Kadmany (1):
wil6210: abort properly in cfg suspend

Lazar Alexei (5):
wil6210: run-time PM when interface down
wil6210: get suspend reject reason and resume triggers from FW
wil6210: fix PCIe bus mastering in case of interface down
wil6210: remove suspend time statistics
wil6210: update statistics for suspend

Lior David (5):
wil6210: refresh FW capabilities during interface up
wil6210: fix length check in __wmi_send
wil6210: add block size checks during FW load
wil6210: missing length check in wmi_set_ie
wil6210: missing length check in wil_cfg80211_mgmt_tx

drivers/net/wireless/ath/wil6210/cfg80211.c | 17 +-
drivers/net/wireless/ath/wil6210/debugfs.c | 134 +++++++++---
drivers/net/wireless/ath/wil6210/ethtool.c | 15 ++
drivers/net/wireless/ath/wil6210/fw_inc.c | 79 ++++---
drivers/net/wireless/ath/wil6210/interrupt.c | 22 +-
drivers/net/wireless/ath/wil6210/main.c | 26 ++-
drivers/net/wireless/ath/wil6210/netdev.c | 18 +-
drivers/net/wireless/ath/wil6210/pcie_bus.c | 87 +++++---
drivers/net/wireless/ath/wil6210/pm.c | 100 ++++++---
drivers/net/wireless/ath/wil6210/wil6210.h | 38 +++-
drivers/net/wireless/ath/wil6210/wmi.c | 304 ++++++++++++++++++++++++++-
drivers/net/wireless/ath/wil6210/wmi.h | 17 +-
12 files changed, 706 insertions(+), 151 deletions(-)

--
1.9.1


2017-10-29 19:58:20

by Maya Haim

[permalink] [raw]
Subject: RE: [01/12] wil6210: run-time PM when interface down

S2FsbGUgVmFsbyA8a3ZhbG9AY29kZWF1cm9yYS5vcmc+IHdyb3RlOg0KDQo+IE1heWEgRXJleiA8
cWNhX21lcmV6QHFjYS5xdWFsY29tbS5jb20+IHdyb3RlOg0KDQo+PiBBbGxvdyBydW4tdGltZSBz
dXNwZW5kIHdoZW4gaW50ZXJmYWNlIGlzIGRvd24sIGtlZXAgY2FyZCBhbGl2ZSB3aGVuIA0KPj4g
aW50ZXJmYWNlIGlzIHVwLg0KPj4gSWYgZHJpdmVyIGlzIGluIHdtaSBvbmx5IG9yIGRlYnVnX2Z3
IG1vZGUgcnVuLXRpbWUgUE0gd29uJ3Qgc3VzcGVuZC4NCj4+DQo+PiBTaWduZWQtb2ZmLWJ5OiBM
YXphciBBbGV4ZWkgPHFjYV9haWxpemFyb0BxY2EucXVhbGNvbW0uY29tPg0KPj4gCXBjaWVfYnVz
LmMNCj4+IFNpZ25lZC1vZmYtYnk6IE1heWEgRXJleiA8cWNhX21lcmV6QHFjYS5xdWFsY29tbS5j
b20+DQo+PiBTaWduZWQtb2ZmLWJ5OiBLYWxsZSBWYWxvIDxrdmFsb0BxY2EucXVhbGNvbW0uY29t
Pg0KDQo+IEtidWlsZCBib3QgZm91bmQgcHJvYmxlbXM6DQoNClRoYW5rcywgS2FsbGUuIFdlIHdp
bGwgZml4IHRoZSBlcnJvcnMuDQo=

2017-10-25 13:14:24

by Maya Erez

[permalink] [raw]
Subject: [PATCH 06/12] wil6210: add block size checks during FW load

From: Lior David <[email protected]>

When loading FW from file add block size checks to ensure a
corrupted FW file will not cause the driver to write outside
the device memory.

Signed-off-by: Lior David <[email protected]>
Signed-off-by: Maya Erez <[email protected]>
---
drivers/net/wireless/ath/wil6210/fw_inc.c | 58 +++++++++++++++++++-----------
drivers/net/wireless/ath/wil6210/wil6210.h | 1 +
drivers/net/wireless/ath/wil6210/wmi.c | 11 +++++-
3 files changed, 49 insertions(+), 21 deletions(-)

diff --git a/drivers/net/wireless/ath/wil6210/fw_inc.c b/drivers/net/wireless/ath/wil6210/fw_inc.c
index 7a33792..77d1902 100644
--- a/drivers/net/wireless/ath/wil6210/fw_inc.c
+++ b/drivers/net/wireless/ath/wil6210/fw_inc.c
@@ -26,14 +26,17 @@
prefix_type, rowsize, \
groupsize, buf, len, ascii)

-#define FW_ADDR_CHECK(ioaddr, val, msg) do { \
- ioaddr = wmi_buffer(wil, val); \
- if (!ioaddr) { \
- wil_err_fw(wil, "bad " msg ": 0x%08x\n", \
- le32_to_cpu(val)); \
- return -EINVAL; \
- } \
- } while (0)
+static bool wil_fw_addr_check(struct wil6210_priv *wil,
+ void __iomem **ioaddr, __le32 val,
+ u32 size, const char *msg)
+{
+ *ioaddr = wmi_buffer_block(wil, val, size);
+ if (!(*ioaddr)) {
+ wil_err_fw(wil, "bad %s: 0x%08x\n", msg, le32_to_cpu(val));
+ return false;
+ }
+ return true;
+}

/**
* wil_fw_verify - verify firmware file validity
@@ -160,7 +163,8 @@ static int fw_handle_data(struct wil6210_priv *wil, const void *data,
return -EINVAL;
}

- FW_ADDR_CHECK(dst, d->addr, "address");
+ if (!wil_fw_addr_check(wil, &dst, d->addr, s, "address"))
+ return -EINVAL;
wil_dbg_fw(wil, "write [0x%08x] <== %zu bytes\n", le32_to_cpu(d->addr),
s);
wil_memcpy_toio_32(dst, d->data, s);
@@ -192,7 +196,8 @@ static int fw_handle_fill(struct wil6210_priv *wil, const void *data,
return -EINVAL;
}

- FW_ADDR_CHECK(dst, d->addr, "address");
+ if (!wil_fw_addr_check(wil, &dst, d->addr, s, "address"))
+ return -EINVAL;

v = le32_to_cpu(d->value);
wil_dbg_fw(wil, "fill [0x%08x] <== 0x%08x, %zu bytes\n",
@@ -248,7 +253,8 @@ static int fw_handle_direct_write(struct wil6210_priv *wil, const void *data,
u32 v = le32_to_cpu(block[i].value);
u32 x, y;

- FW_ADDR_CHECK(dst, block[i].addr, "address");
+ if (!wil_fw_addr_check(wil, &dst, block[i].addr, 0, "address"))
+ return -EINVAL;

x = readl(dst);
y = (x & m) | (v & ~m);
@@ -314,10 +320,15 @@ static int fw_handle_gateway_data(struct wil6210_priv *wil, const void *data,
wil_dbg_fw(wil, "gw write record [%3d] blocks, cmd 0x%08x\n",
n, gw_cmd);

- FW_ADDR_CHECK(gwa_addr, d->gateway_addr_addr, "gateway_addr_addr");
- FW_ADDR_CHECK(gwa_val, d->gateway_value_addr, "gateway_value_addr");
- FW_ADDR_CHECK(gwa_cmd, d->gateway_cmd_addr, "gateway_cmd_addr");
- FW_ADDR_CHECK(gwa_ctl, d->gateway_ctrl_address, "gateway_ctrl_address");
+ if (!wil_fw_addr_check(wil, &gwa_addr, d->gateway_addr_addr, 0,
+ "gateway_addr_addr") ||
+ !wil_fw_addr_check(wil, &gwa_val, d->gateway_value_addr, 0,
+ "gateway_value_addr") ||
+ !wil_fw_addr_check(wil, &gwa_cmd, d->gateway_cmd_addr, 0,
+ "gateway_cmd_addr") ||
+ !wil_fw_addr_check(wil, &gwa_ctl, d->gateway_ctrl_address, 0,
+ "gateway_ctrl_address"))
+ return -EINVAL;

wil_dbg_fw(wil, "gw addresses: addr 0x%08x val 0x%08x"
" cmd 0x%08x ctl 0x%08x\n",
@@ -373,12 +384,19 @@ static int fw_handle_gateway_data4(struct wil6210_priv *wil, const void *data,
wil_dbg_fw(wil, "gw4 write record [%3d] blocks, cmd 0x%08x\n",
n, gw_cmd);

- FW_ADDR_CHECK(gwa_addr, d->gateway_addr_addr, "gateway_addr_addr");
+ if (!wil_fw_addr_check(wil, &gwa_addr, d->gateway_addr_addr, 0,
+ "gateway_addr_addr"))
+ return -EINVAL;
for (k = 0; k < ARRAY_SIZE(block->value); k++)
- FW_ADDR_CHECK(gwa_val[k], d->gateway_value_addr[k],
- "gateway_value_addr");
- FW_ADDR_CHECK(gwa_cmd, d->gateway_cmd_addr, "gateway_cmd_addr");
- FW_ADDR_CHECK(gwa_ctl, d->gateway_ctrl_address, "gateway_ctrl_address");
+ if (!wil_fw_addr_check(wil, &gwa_val[k],
+ d->gateway_value_addr[k],
+ 0, "gateway_value_addr"))
+ return -EINVAL;
+ if (!wil_fw_addr_check(wil, &gwa_cmd, d->gateway_cmd_addr, 0,
+ "gateway_cmd_addr") ||
+ !wil_fw_addr_check(wil, &gwa_ctl, d->gateway_ctrl_address, 0,
+ "gateway_ctrl_address"))
+ return -EINVAL;

wil_dbg_fw(wil, "gw4 addresses: addr 0x%08x cmd 0x%08x ctl 0x%08x\n",
le32_to_cpu(d->gateway_addr_addr),
diff --git a/drivers/net/wireless/ath/wil6210/wil6210.h b/drivers/net/wireless/ath/wil6210/wil6210.h
index 8dfb4a7..6b09998 100644
--- a/drivers/net/wireless/ath/wil6210/wil6210.h
+++ b/drivers/net/wireless/ath/wil6210/wil6210.h
@@ -877,6 +877,7 @@ int wil_ps_update(struct wil6210_priv *wil,
int wil_find_cid(struct wil6210_priv *wil, const u8 *mac);
void wil_set_ethtoolops(struct net_device *ndev);

+void __iomem *wmi_buffer_block(struct wil6210_priv *wil, __le32 ptr, u32 size);
void __iomem *wmi_buffer(struct wil6210_priv *wil, __le32 ptr);
void __iomem *wmi_addr(struct wil6210_priv *wil, u32 ptr);
int wmi_read_hdr(struct wil6210_priv *wil, __le32 ptr,
diff --git a/drivers/net/wireless/ath/wil6210/wmi.c b/drivers/net/wireless/ath/wil6210/wmi.c
index dbdf71d..16aa624 100644
--- a/drivers/net/wireless/ath/wil6210/wmi.c
+++ b/drivers/net/wireless/ath/wil6210/wmi.c
@@ -140,13 +140,15 @@ static u32 wmi_addr_remap(u32 x)
/**
* Check address validity for WMI buffer; remap if needed
* @ptr - internal (linker) fw/ucode address
+ * @size - if non zero, validate the block does not
+ * exceed the device memory (bar)
*
* Valid buffer should be DWORD aligned
*
* return address for accessing buffer from the host;
* if buffer is not valid, return NULL.
*/
-void __iomem *wmi_buffer(struct wil6210_priv *wil, __le32 ptr_)
+void __iomem *wmi_buffer_block(struct wil6210_priv *wil, __le32 ptr_, u32 size)
{
u32 off;
u32 ptr = le32_to_cpu(ptr_);
@@ -161,10 +163,17 @@ void __iomem *wmi_buffer(struct wil6210_priv *wil, __le32 ptr_)
off = HOSTADDR(ptr);
if (off > wil->bar_size - 4)
return NULL;
+ if (size && ((off + size > wil->bar_size) || (off + size < off)))
+ return NULL;

return wil->csr + off;
}

+void __iomem *wmi_buffer(struct wil6210_priv *wil, __le32 ptr_)
+{
+ return wmi_buffer_block(wil, ptr_, 0);
+}
+
/**
* Check address validity
*/
--
1.9.1

2017-10-25 13:14:16

by Maya Erez

[permalink] [raw]
Subject: [PATCH 02/12] wil6210: print human readable names of WMI commands and events

From: Dedy Lansky <[email protected]>

Upon sending/receiving WMI commands/events, print human readable
names in addition to id for easier debugging.

Signed-off-by: Dedy Lansky <[email protected]>
Signed-off-by: Maya Erez <[email protected]>
---
drivers/net/wireless/ath/wil6210/wmi.c | 237 ++++++++++++++++++++++++++++++++-
1 file changed, 232 insertions(+), 5 deletions(-)

diff --git a/drivers/net/wireless/ath/wil6210/wmi.c b/drivers/net/wireless/ath/wil6210/wmi.c
index ffdd2fa..8a780f2 100644
--- a/drivers/net/wireless/ath/wil6210/wmi.c
+++ b/drivers/net/wireless/ath/wil6210/wmi.c
@@ -198,6 +198,232 @@ int wmi_read_hdr(struct wil6210_priv *wil, __le32 ptr,
return 0;
}

+static const char *cmdid2name(u16 cmdid)
+{
+ switch (cmdid) {
+ case WMI_NOTIFY_REQ_CMDID:
+ return "WMI_NOTIFY_REQ_CMD";
+ case WMI_START_SCAN_CMDID:
+ return "WMI_START_SCAN_CMD";
+ case WMI_CONNECT_CMDID:
+ return "WMI_CONNECT_CMD";
+ case WMI_DISCONNECT_CMDID:
+ return "WMI_DISCONNECT_CMD";
+ case WMI_SW_TX_REQ_CMDID:
+ return "WMI_SW_TX_REQ_CMD";
+ case WMI_GET_RF_SECTOR_PARAMS_CMDID:
+ return "WMI_GET_RF_SECTOR_PARAMS_CMD";
+ case WMI_SET_RF_SECTOR_PARAMS_CMDID:
+ return "WMI_SET_RF_SECTOR_PARAMS_CMD";
+ case WMI_GET_SELECTED_RF_SECTOR_INDEX_CMDID:
+ return "WMI_GET_SELECTED_RF_SECTOR_INDEX_CMD";
+ case WMI_SET_SELECTED_RF_SECTOR_INDEX_CMDID:
+ return "WMI_SET_SELECTED_RF_SECTOR_INDEX_CMD";
+ case WMI_BRP_SET_ANT_LIMIT_CMDID:
+ return "WMI_BRP_SET_ANT_LIMIT_CMD";
+ case WMI_TOF_SESSION_START_CMDID:
+ return "WMI_TOF_SESSION_START_CMD";
+ case WMI_AOA_MEAS_CMDID:
+ return "WMI_AOA_MEAS_CMD";
+ case WMI_PMC_CMDID:
+ return "WMI_PMC_CMD";
+ case WMI_TOF_GET_TX_RX_OFFSET_CMDID:
+ return "WMI_TOF_GET_TX_RX_OFFSET_CMD";
+ case WMI_TOF_SET_TX_RX_OFFSET_CMDID:
+ return "WMI_TOF_SET_TX_RX_OFFSET_CMD";
+ case WMI_VRING_CFG_CMDID:
+ return "WMI_VRING_CFG_CMD";
+ case WMI_BCAST_VRING_CFG_CMDID:
+ return "WMI_BCAST_VRING_CFG_CMD";
+ case WMI_TRAFFIC_SUSPEND_CMDID:
+ return "WMI_TRAFFIC_SUSPEND_CMD";
+ case WMI_TRAFFIC_RESUME_CMDID:
+ return "WMI_TRAFFIC_RESUME_CMD";
+ case WMI_ECHO_CMDID:
+ return "WMI_ECHO_CMD";
+ case WMI_SET_MAC_ADDRESS_CMDID:
+ return "WMI_SET_MAC_ADDRESS_CMD";
+ case WMI_LED_CFG_CMDID:
+ return "WMI_LED_CFG_CMD";
+ case WMI_PCP_START_CMDID:
+ return "WMI_PCP_START_CMD";
+ case WMI_PCP_STOP_CMDID:
+ return "WMI_PCP_STOP_CMD";
+ case WMI_SET_SSID_CMDID:
+ return "WMI_SET_SSID_CMD";
+ case WMI_GET_SSID_CMDID:
+ return "WMI_GET_SSID_CMD";
+ case WMI_SET_PCP_CHANNEL_CMDID:
+ return "WMI_SET_PCP_CHANNEL_CMD";
+ case WMI_GET_PCP_CHANNEL_CMDID:
+ return "WMI_GET_PCP_CHANNEL_CMD";
+ case WMI_P2P_CFG_CMDID:
+ return "WMI_P2P_CFG_CMD";
+ case WMI_START_LISTEN_CMDID:
+ return "WMI_START_LISTEN_CMD";
+ case WMI_START_SEARCH_CMDID:
+ return "WMI_START_SEARCH_CMD";
+ case WMI_DISCOVERY_STOP_CMDID:
+ return "WMI_DISCOVERY_STOP_CMD";
+ case WMI_DELETE_CIPHER_KEY_CMDID:
+ return "WMI_DELETE_CIPHER_KEY_CMD";
+ case WMI_ADD_CIPHER_KEY_CMDID:
+ return "WMI_ADD_CIPHER_KEY_CMD";
+ case WMI_SET_APPIE_CMDID:
+ return "WMI_SET_APPIE_CMD";
+ case WMI_CFG_RX_CHAIN_CMDID:
+ return "WMI_CFG_RX_CHAIN_CMD";
+ case WMI_TEMP_SENSE_CMDID:
+ return "WMI_TEMP_SENSE_CMD";
+ case WMI_DEL_STA_CMDID:
+ return "WMI_DEL_STA_CMD";
+ case WMI_DISCONNECT_STA_CMDID:
+ return "WMI_DISCONNECT_STA_CMD";
+ case WMI_VRING_BA_EN_CMDID:
+ return "WMI_VRING_BA_EN_CMD";
+ case WMI_VRING_BA_DIS_CMDID:
+ return "WMI_VRING_BA_DIS_CMD";
+ case WMI_RCP_DELBA_CMDID:
+ return "WMI_RCP_DELBA_CMD";
+ case WMI_RCP_ADDBA_RESP_CMDID:
+ return "WMI_RCP_ADDBA_RESP_CMD";
+ case WMI_PS_DEV_PROFILE_CFG_CMDID:
+ return "WMI_PS_DEV_PROFILE_CFG_CMD";
+ case WMI_SET_MGMT_RETRY_LIMIT_CMDID:
+ return "WMI_SET_MGMT_RETRY_LIMIT_CMD";
+ case WMI_GET_MGMT_RETRY_LIMIT_CMDID:
+ return "WMI_GET_MGMT_RETRY_LIMIT_CMD";
+ case WMI_ABORT_SCAN_CMDID:
+ return "WMI_ABORT_SCAN_CMD";
+ case WMI_NEW_STA_CMDID:
+ return "WMI_NEW_STA_CMD";
+ case WMI_SET_THERMAL_THROTTLING_CFG_CMDID:
+ return "WMI_SET_THERMAL_THROTTLING_CFG_CMD";
+ case WMI_GET_THERMAL_THROTTLING_CFG_CMDID:
+ return "WMI_GET_THERMAL_THROTTLING_CFG_CMD";
+ case WMI_LINK_MAINTAIN_CFG_WRITE_CMDID:
+ return "WMI_LINK_MAINTAIN_CFG_WRITE_CMD";
+ case WMI_LO_POWER_CALIB_FROM_OTP_CMDID:
+ return "WMI_LO_POWER_CALIB_FROM_OTP_CMD";
+ default:
+ return "Untracked CMD";
+ }
+}
+
+static const char *eventid2name(u16 eventid)
+{
+ switch (eventid) {
+ case WMI_NOTIFY_REQ_DONE_EVENTID:
+ return "WMI_NOTIFY_REQ_DONE_EVENT";
+ case WMI_DISCONNECT_EVENTID:
+ return "WMI_DISCONNECT_EVENT";
+ case WMI_SW_TX_COMPLETE_EVENTID:
+ return "WMI_SW_TX_COMPLETE_EVENT";
+ case WMI_GET_RF_SECTOR_PARAMS_DONE_EVENTID:
+ return "WMI_GET_RF_SECTOR_PARAMS_DONE_EVENT";
+ case WMI_SET_RF_SECTOR_PARAMS_DONE_EVENTID:
+ return "WMI_SET_RF_SECTOR_PARAMS_DONE_EVENT";
+ case WMI_GET_SELECTED_RF_SECTOR_INDEX_DONE_EVENTID:
+ return "WMI_GET_SELECTED_RF_SECTOR_INDEX_DONE_EVENT";
+ case WMI_SET_SELECTED_RF_SECTOR_INDEX_DONE_EVENTID:
+ return "WMI_SET_SELECTED_RF_SECTOR_INDEX_DONE_EVENT";
+ case WMI_BRP_SET_ANT_LIMIT_EVENTID:
+ return "WMI_BRP_SET_ANT_LIMIT_EVENT";
+ case WMI_FW_READY_EVENTID:
+ return "WMI_FW_READY_EVENT";
+ case WMI_TRAFFIC_RESUME_EVENTID:
+ return "WMI_TRAFFIC_RESUME_EVENT";
+ case WMI_TOF_GET_TX_RX_OFFSET_EVENTID:
+ return "WMI_TOF_GET_TX_RX_OFFSET_EVENT";
+ case WMI_TOF_SET_TX_RX_OFFSET_EVENTID:
+ return "WMI_TOF_SET_TX_RX_OFFSET_EVENT";
+ case WMI_VRING_CFG_DONE_EVENTID:
+ return "WMI_VRING_CFG_DONE_EVENT";
+ case WMI_READY_EVENTID:
+ return "WMI_READY_EVENT";
+ case WMI_RX_MGMT_PACKET_EVENTID:
+ return "WMI_RX_MGMT_PACKET_EVENT";
+ case WMI_TX_MGMT_PACKET_EVENTID:
+ return "WMI_TX_MGMT_PACKET_EVENT";
+ case WMI_SCAN_COMPLETE_EVENTID:
+ return "WMI_SCAN_COMPLETE_EVENT";
+ case WMI_ACS_PASSIVE_SCAN_COMPLETE_EVENTID:
+ return "WMI_ACS_PASSIVE_SCAN_COMPLETE_EVENT";
+ case WMI_CONNECT_EVENTID:
+ return "WMI_CONNECT_EVENT";
+ case WMI_EAPOL_RX_EVENTID:
+ return "WMI_EAPOL_RX_EVENT";
+ case WMI_BA_STATUS_EVENTID:
+ return "WMI_BA_STATUS_EVENT";
+ case WMI_RCP_ADDBA_REQ_EVENTID:
+ return "WMI_RCP_ADDBA_REQ_EVENT";
+ case WMI_DELBA_EVENTID:
+ return "WMI_DELBA_EVENT";
+ case WMI_VRING_EN_EVENTID:
+ return "WMI_VRING_EN_EVENT";
+ case WMI_DATA_PORT_OPEN_EVENTID:
+ return "WMI_DATA_PORT_OPEN_EVENT";
+ case WMI_AOA_MEAS_EVENTID:
+ return "WMI_AOA_MEAS_EVENT";
+ case WMI_TOF_SESSION_END_EVENTID:
+ return "WMI_TOF_SESSION_END_EVENT";
+ case WMI_TOF_GET_CAPABILITIES_EVENTID:
+ return "WMI_TOF_GET_CAPABILITIES_EVENT";
+ case WMI_TOF_SET_LCR_EVENTID:
+ return "WMI_TOF_SET_LCR_EVENT";
+ case WMI_TOF_SET_LCI_EVENTID:
+ return "WMI_TOF_SET_LCI_EVENT";
+ case WMI_TOF_FTM_PER_DEST_RES_EVENTID:
+ return "WMI_TOF_FTM_PER_DEST_RES_EVENT";
+ case WMI_TOF_CHANNEL_INFO_EVENTID:
+ return "WMI_TOF_CHANNEL_INFO_EVENT";
+ case WMI_TRAFFIC_SUSPEND_EVENTID:
+ return "WMI_TRAFFIC_SUSPEND_EVENT";
+ case WMI_ECHO_RSP_EVENTID:
+ return "WMI_ECHO_RSP_EVENT";
+ case WMI_LED_CFG_DONE_EVENTID:
+ return "WMI_LED_CFG_DONE_EVENT";
+ case WMI_PCP_STARTED_EVENTID:
+ return "WMI_PCP_STARTED_EVENT";
+ case WMI_PCP_STOPPED_EVENTID:
+ return "WMI_PCP_STOPPED_EVENT";
+ case WMI_GET_SSID_EVENTID:
+ return "WMI_GET_SSID_EVENT";
+ case WMI_GET_PCP_CHANNEL_EVENTID:
+ return "WMI_GET_PCP_CHANNEL_EVENT";
+ case WMI_P2P_CFG_DONE_EVENTID:
+ return "WMI_P2P_CFG_DONE_EVENT";
+ case WMI_LISTEN_STARTED_EVENTID:
+ return "WMI_LISTEN_STARTED_EVENT";
+ case WMI_SEARCH_STARTED_EVENTID:
+ return "WMI_SEARCH_STARTED_EVENT";
+ case WMI_DISCOVERY_STOPPED_EVENTID:
+ return "WMI_DISCOVERY_STOPPED_EVENT";
+ case WMI_CFG_RX_CHAIN_DONE_EVENTID:
+ return "WMI_CFG_RX_CHAIN_DONE_EVENT";
+ case WMI_TEMP_SENSE_DONE_EVENTID:
+ return "WMI_TEMP_SENSE_DONE_EVENT";
+ case WMI_RCP_ADDBA_RESP_SENT_EVENTID:
+ return "WMI_RCP_ADDBA_RESP_SENT_EVENT";
+ case WMI_PS_DEV_PROFILE_CFG_EVENTID:
+ return "WMI_PS_DEV_PROFILE_CFG_EVENT";
+ case WMI_SET_MGMT_RETRY_LIMIT_EVENTID:
+ return "WMI_SET_MGMT_RETRY_LIMIT_EVENT";
+ case WMI_GET_MGMT_RETRY_LIMIT_EVENTID:
+ return "WMI_GET_MGMT_RETRY_LIMIT_EVENT";
+ case WMI_SET_THERMAL_THROTTLING_CFG_EVENTID:
+ return "WMI_SET_THERMAL_THROTTLING_CFG_EVENT";
+ case WMI_GET_THERMAL_THROTTLING_CFG_EVENTID:
+ return "WMI_GET_THERMAL_THROTTLING_CFG_EVENT";
+ case WMI_LINK_MAINTAIN_CFG_WRITE_DONE_EVENTID:
+ return "WMI_LINK_MAINTAIN_CFG_WRITE_DONE_EVENT";
+ case WMI_LO_POWER_CALIB_FROM_OTP_EVENTID:
+ return "WMI_LO_POWER_CALIB_FROM_OTP_EVENT";
+ default:
+ return "Untracked EVENT";
+ }
+}
+
static int __wmi_send(struct wil6210_priv *wil, u16 cmdid, void *buf, u16 len)
{
struct {
@@ -294,7 +520,8 @@ static int __wmi_send(struct wil6210_priv *wil, u16 cmdid, void *buf, u16 len)
}
cmd.hdr.seq = cpu_to_le16(++wil->wmi_seq);
/* set command */
- wil_dbg_wmi(wil, "WMI command 0x%04x [%d]\n", cmdid, len);
+ wil_dbg_wmi(wil, "sending %s (0x%04x) [%d]\n",
+ cmdid2name(cmdid), cmdid, len);
wil_hex_dump_wmi("Cmd ", DUMP_PREFIX_OFFSET, 16, 1, &cmd,
sizeof(cmd), true);
wil_hex_dump_wmi("cmd ", DUMP_PREFIX_OFFSET, 16, 1, buf,
@@ -963,8 +1190,8 @@ void wmi_recv_cmd(struct wil6210_priv *wil)
}
spin_unlock_irqrestore(&wil->wmi_ev_lock, flags);

- wil_dbg_wmi(wil, "WMI event 0x%04x MID %d @%d msec\n",
- id, wmi->mid, tstamp);
+ wil_dbg_wmi(wil, "recv %s (0x%04x) MID %d @%d msec\n",
+ eventid2name(id), id, wmi->mid, tstamp);
trace_wil6210_wmi_event(wmi, &wmi[1],
len - sizeof(*wmi));
}
@@ -1906,8 +2133,8 @@ static void wmi_event_handle(struct wil6210_priv *wil,
void *evt_data = (void *)(&wmi[1]);
u16 id = le16_to_cpu(wmi->command_id);

- wil_dbg_wmi(wil, "Handle WMI 0x%04x (reply_id 0x%04x)\n",
- id, wil->reply_id);
+ wil_dbg_wmi(wil, "Handle %s (0x%04x) (reply_id 0x%04x)\n",
+ eventid2name(id), id, wil->reply_id);
/* check if someone waits for this event */
if (wil->reply_id && wil->reply_id == id) {
WARN_ON(wil->reply_buf);
--
1.9.1

2017-10-28 15:18:58

by Kalle Valo

[permalink] [raw]
Subject: Re: [PATCH 12/12] wil6210: update statistics for suspend

Maya Erez <[email protected]> writes:

> From: Lazar Alexei <[email protected]>
>
> Currently the statistics show how many successful/failed
> suspend/resume operations the system had.
> Update the statistics by splitting each successful/failed
> suspend/resume operations to radio on/off.
>
> Signed-off-by: Lazar Alexei <[email protected]>
> Signed-off-by: Maya Erez <[email protected]>
> ---
> drivers/net/wireless/ath/wil6210/debugfs.c | 27 ++++++++++++++++++---------
> drivers/net/wireless/ath/wil6210/pcie_bus.c | 20 ++++++++++++++------
> drivers/net/wireless/ath/wil6210/pm.c | 8 +++++---
> drivers/net/wireless/ath/wil6210/wil6210.h | 11 ++++++++---
> 4 files changed, 45 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/net/wireless/ath/wil6210/debugfs.c b/drivers/net/wireless/ath/wil6210/debugfs.c
> index 03d17ea..9164855 100644
> --- a/drivers/net/wireless/ath/wil6210/debugfs.c
> +++ b/drivers/net/wireless/ath/wil6210/debugfs.c
> @@ -1685,20 +1685,29 @@ static ssize_t wil_read_suspend_stats(struct file *file,
> size_t count, loff_t *ppos)
> {
> struct wil6210_priv *wil = file->private_data;
> - static char text[400];
> + static char text[500];

I was first about to mention about excessive stack usage but only then I
noticed the static keyword. That again is problematic when you have two
(or more) devices as then they would be accessing the same buffer,
right?

As this from debugfs interface I admit that it's a very theoretical
issue but still something which should be fixed. I can still take this
patch, but please fix the static usage in a followup patch.

--
Kalle Valo

2017-10-25 13:14:34

by Maya Erez

[permalink] [raw]
Subject: [PATCH 11/12] wil6210: remove suspend time statistics

From: Lazar Alexei <[email protected]>

Currently suspend time statistics are showed through debugfs.
Remove time statistics in suspend state since the timing may
not be accurate in that state.

Signed-off-by: Lazar Alexei <[email protected]>
Signed-off-by: Maya Erez <[email protected]>
---
drivers/net/wireless/ath/wil6210/debugfs.c | 17 ++---------------
drivers/net/wireless/ath/wil6210/main.c | 1 -
drivers/net/wireless/ath/wil6210/pm.c | 20 ++------------------
drivers/net/wireless/ath/wil6210/wil6210.h | 5 -----
4 files changed, 4 insertions(+), 39 deletions(-)

diff --git a/drivers/net/wireless/ath/wil6210/debugfs.c b/drivers/net/wireless/ath/wil6210/debugfs.c
index 2634650..03d17ea 100644
--- a/drivers/net/wireless/ath/wil6210/debugfs.c
+++ b/drivers/net/wireless/ath/wil6210/debugfs.c
@@ -1676,8 +1676,6 @@ static ssize_t wil_write_suspend_stats(struct file *file,
struct wil6210_priv *wil = file->private_data;

memset(&wil->suspend_stats, 0, sizeof(wil->suspend_stats));
- wil->suspend_stats.min_suspend_time = ULONG_MAX;
- wil->suspend_stats.collection_start = ktime_get();

return len;
}
@@ -1689,27 +1687,18 @@ static ssize_t wil_read_suspend_stats(struct file *file,
struct wil6210_priv *wil = file->private_data;
static char text[400];
int n;
- unsigned long long stats_collection_time =
- ktime_to_us(ktime_sub(ktime_get(),
- wil->suspend_stats.collection_start));

n = snprintf(text, sizeof(text),
"Suspend statistics:\n"
"successful suspends:%ld failed suspends:%ld\n"
"successful resumes:%ld failed resumes:%ld\n"
- "rejected by host:%ld rejected by device:%ld\n"
- "total suspend time:%lld min suspend time:%lld\n"
- "max suspend time:%lld stats collection time: %lld\n",
+ "rejected by host:%ld rejected by device:%ld\n",
wil->suspend_stats.successful_suspends,
wil->suspend_stats.failed_suspends,
wil->suspend_stats.successful_resumes,
wil->suspend_stats.failed_resumes,
wil->suspend_stats.rejected_by_host,
- wil->suspend_stats.rejected_by_device,
- wil->suspend_stats.total_suspend_time,
- wil->suspend_stats.min_suspend_time,
- wil->suspend_stats.max_suspend_time,
- stats_collection_time);
+ wil->suspend_stats.rejected_by_device);

n = min_t(int, n, sizeof(text));

@@ -1881,8 +1870,6 @@ int wil6210_debugfs_init(struct wil6210_priv *wil)

wil6210_debugfs_create_ITR_CNT(wil, dbg);

- wil->suspend_stats.collection_start = ktime_get();
-
return 0;
}

diff --git a/drivers/net/wireless/ath/wil6210/main.c b/drivers/net/wireless/ath/wil6210/main.c
index ad1f9f8..9b3726d 100644
--- a/drivers/net/wireless/ath/wil6210/main.c
+++ b/drivers/net/wireless/ath/wil6210/main.c
@@ -580,7 +580,6 @@ int wil_priv_init(struct wil6210_priv *wil)
wil->wakeup_trigger = WMI_WAKEUP_TRIGGER_UCAST |
WMI_WAKEUP_TRIGGER_BCAST;
memset(&wil->suspend_stats, 0, sizeof(wil->suspend_stats));
- wil->suspend_stats.min_suspend_time = ULONG_MAX;
wil->vring_idle_trsh = 16;

return 0;
diff --git a/drivers/net/wireless/ath/wil6210/pm.c b/drivers/net/wireless/ath/wil6210/pm.c
index ce4f546..48ec186 100644
--- a/drivers/net/wireless/ath/wil6210/pm.c
+++ b/drivers/net/wireless/ath/wil6210/pm.c
@@ -319,16 +319,12 @@ int wil_suspend(struct wil6210_priv *wil, bool is_runtime, bool keep_radio_on)
wil_dbg_pm(wil, "suspend: %s => %d\n",
is_runtime ? "runtime" : "system", rc);

- if (!rc)
- wil->suspend_stats.suspend_start_time = ktime_get();
-
return rc;
}

int wil_resume(struct wil6210_priv *wil, bool is_runtime, bool keep_radio_on)
{
int rc = 0;
- unsigned long long suspend_time_usec = 0;

wil_dbg_pm(wil, "resume: %s\n", is_runtime ? "runtime" : "system");

@@ -346,21 +342,9 @@ int wil_resume(struct wil6210_priv *wil, bool is_runtime, bool keep_radio_on)
else
rc = wil_resume_radio_off(wil);

- if (rc)
- goto out;
-
- suspend_time_usec =
- ktime_to_us(ktime_sub(ktime_get(),
- wil->suspend_stats.suspend_start_time));
- wil->suspend_stats.total_suspend_time += suspend_time_usec;
- if (suspend_time_usec < wil->suspend_stats.min_suspend_time)
- wil->suspend_stats.min_suspend_time = suspend_time_usec;
- if (suspend_time_usec > wil->suspend_stats.max_suspend_time)
- wil->suspend_stats.max_suspend_time = suspend_time_usec;
-
out:
- wil_dbg_pm(wil, "resume: %s => %d, suspend time %lld usec\n",
- is_runtime ? "runtime" : "system", rc, suspend_time_usec);
+ wil_dbg_pm(wil, "resume: %s => %d\n", is_runtime ? "runtime" : "system",
+ rc);
return rc;
}

diff --git a/drivers/net/wireless/ath/wil6210/wil6210.h b/drivers/net/wireless/ath/wil6210/wil6210.h
index b7509f3..0d8457e 100644
--- a/drivers/net/wireless/ath/wil6210/wil6210.h
+++ b/drivers/net/wireless/ath/wil6210/wil6210.h
@@ -89,11 +89,6 @@ struct wil_suspend_stats {
unsigned long failed_resumes;
unsigned long rejected_by_device;
unsigned long rejected_by_host;
- unsigned long long total_suspend_time;
- unsigned long long min_suspend_time;
- unsigned long long max_suspend_time;
- ktime_t collection_start;
- ktime_t suspend_start_time;
};

/* Calculate MAC buffer size for the firmware. It includes all overhead,
--
1.9.1

2017-10-28 15:01:16

by Kalle Valo

[permalink] [raw]
Subject: Re: [PATCH 03/12] wil6210: refresh FW capabilities during interface up

Maya Erez <[email protected]> writes:

> From: Lior David <[email protected]>
>
> FW capabilities are currently retrieved only during module
> initialization, but userspace can replace the firmware while
> interface is down, so refresh the FW capabilities when
> interface is up (after FW is loaded) to ensure driver
> functionality matches the loaded FW.

I think usually the firmware is loaded only once during probe() and I
think it's quite special that you retrieve it during interface up. Being
able to change the firmware version runtime like that can lead to
problems eventually, for example cfg80211 might not allow changing
already registered configuration etc.

--
Kalle Valo

2017-10-25 13:14:18

by Maya Erez

[permalink] [raw]
Subject: [PATCH 03/12] wil6210: refresh FW capabilities during interface up

From: Lior David <[email protected]>

FW capabilities are currently retrieved only during module
initialization, but userspace can replace the firmware while
interface is down, so refresh the FW capabilities when
interface is up (after FW is loaded) to ensure driver
functionality matches the loaded FW.

Signed-off-by: Lior David <[email protected]>
Signed-off-by: Maya Erez <[email protected]>
---
drivers/net/wireless/ath/wil6210/fw_inc.c | 21 ++++++++-------------
drivers/net/wireless/ath/wil6210/main.c | 25 +++++++++++++++++++++++--
drivers/net/wireless/ath/wil6210/pcie_bus.c | 13 +------------
drivers/net/wireless/ath/wil6210/wil6210.h | 1 +
4 files changed, 33 insertions(+), 27 deletions(-)

diff --git a/drivers/net/wireless/ath/wil6210/fw_inc.c b/drivers/net/wireless/ath/wil6210/fw_inc.c
index e01acac..7a33792 100644
--- a/drivers/net/wireless/ath/wil6210/fw_inc.c
+++ b/drivers/net/wireless/ath/wil6210/fw_inc.c
@@ -124,24 +124,19 @@ static int fw_ignore_section(struct wil6210_priv *wil, const void *data,
return 0;
}

-static int fw_handle_comment(struct wil6210_priv *wil, const void *data,
- size_t size)
-{
- wil_hex_dump_fw("", DUMP_PREFIX_OFFSET, 16, 1, data, size, true);
-
- return 0;
-}
-
static int
-fw_handle_capabilities(struct wil6210_priv *wil, const void *data,
- size_t size)
+fw_handle_comment(struct wil6210_priv *wil, const void *data,
+ size_t size)
{
const struct wil_fw_record_capabilities *rec = data;
size_t capa_size;

if (size < sizeof(*rec) ||
- le32_to_cpu(rec->magic) != WIL_FW_CAPABILITIES_MAGIC)
+ le32_to_cpu(rec->magic) != WIL_FW_CAPABILITIES_MAGIC) {
+ wil_hex_dump_fw("", DUMP_PREFIX_OFFSET, 16, 1,
+ data, size, true);
return 0;
+ }

capa_size = size - offsetof(struct wil_fw_record_capabilities,
capabilities);
@@ -422,7 +417,7 @@ static int fw_handle_gateway_data4(struct wil6210_priv *wil, const void *data,
int (*parse_handler)(struct wil6210_priv *wil, const void *data,
size_t size);
} wil_fw_handlers[] = {
- {wil_fw_type_comment, fw_handle_comment, fw_handle_capabilities},
+ {wil_fw_type_comment, fw_handle_comment, fw_handle_comment},
{wil_fw_type_data, fw_handle_data, fw_ignore_section},
{wil_fw_type_fill, fw_handle_fill, fw_ignore_section},
/* wil_fw_type_action */
@@ -517,7 +512,7 @@ int wil_request_firmware(struct wil6210_priv *wil, const char *name,

rc = request_firmware(&fw, name, wil_to_dev(wil));
if (rc) {
- wil_err_fw(wil, "Failed to load firmware %s\n", name);
+ wil_err_fw(wil, "Failed to load firmware %s rc %d\n", name, rc);
return rc;
}
wil_dbg_fw(wil, "Loading <%s>, %zu bytes\n", name, fw->size);
diff --git a/drivers/net/wireless/ath/wil6210/main.c b/drivers/net/wireless/ath/wil6210/main.c
index bac829a..ad1f9f8 100644
--- a/drivers/net/wireless/ath/wil6210/main.c
+++ b/drivers/net/wireless/ath/wil6210/main.c
@@ -761,6 +761,8 @@ static void wil_collect_fw_info(struct wil6210_priv *wil)
u8 retry_short;
int rc;

+ wil_refresh_fw_capabilities(wil);
+
rc = wmi_get_mgmt_retry(wil, &retry_short);
if (!rc) {
wiphy->retry_short = retry_short;
@@ -768,6 +770,25 @@ static void wil_collect_fw_info(struct wil6210_priv *wil)
}
}

+void wil_refresh_fw_capabilities(struct wil6210_priv *wil)
+{
+ struct wiphy *wiphy = wil_to_wiphy(wil);
+
+ wil->keep_radio_on_during_sleep =
+ wil->platform_ops.keep_radio_on_during_sleep &&
+ wil->platform_ops.keep_radio_on_during_sleep(
+ wil->platform_handle) &&
+ test_bit(WMI_FW_CAPABILITY_D3_SUSPEND, wil->fw_capabilities);
+
+ wil_info(wil, "keep_radio_on_during_sleep (%d)\n",
+ wil->keep_radio_on_during_sleep);
+
+ if (test_bit(WMI_FW_CAPABILITY_RSSI_REPORTING, wil->fw_capabilities))
+ wiphy->signal_type = CFG80211_SIGNAL_TYPE_MBM;
+ else
+ wiphy->signal_type = CFG80211_SIGNAL_TYPE_UNSPEC;
+}
+
void wil_mbox_ring_le2cpus(struct wil6210_mbox_ring *r)
{
le32_to_cpus(&r->base);
@@ -1072,11 +1093,11 @@ int wil_reset(struct wil6210_priv *wil, bool load_fw)
return rc;
}

+ wil_collect_fw_info(wil);
+
if (wil->ps_profile != WMI_PS_PROFILE_TYPE_DEFAULT)
wil_ps_update(wil, wil->ps_profile);

- wil_collect_fw_info(wil);
-
if (wil->platform_ops.notify) {
rc = wil->platform_ops.notify(wil->platform_handle,
WIL_PLATFORM_EVT_FW_RDY);
diff --git a/drivers/net/wireless/ath/wil6210/pcie_bus.c b/drivers/net/wireless/ath/wil6210/pcie_bus.c
index 9cc97da..659c32c 100644
--- a/drivers/net/wireless/ath/wil6210/pcie_bus.c
+++ b/drivers/net/wireless/ath/wil6210/pcie_bus.c
@@ -85,9 +85,7 @@ void wil_set_capabilities(struct wil6210_priv *wil)

/* extract FW capabilities from file without loading the FW */
wil_request_firmware(wil, wil->wil_fw_name, false);
-
- if (test_bit(WMI_FW_CAPABILITY_RSSI_REPORTING, wil->fw_capabilities))
- wil_to_wiphy(wil)->signal_type = CFG80211_SIGNAL_TYPE_MBM;
+ wil_refresh_fw_capabilities(wil);
}

void wil_disable_irq(struct wil6210_priv *wil)
@@ -297,15 +295,6 @@ static int wil_pcie_probe(struct pci_dev *pdev, const struct pci_device_id *id)
wil_set_capabilities(wil);
wil6210_clear_irq(wil);

- wil->keep_radio_on_during_sleep =
- wil->platform_ops.keep_radio_on_during_sleep &&
- wil->platform_ops.keep_radio_on_during_sleep(
- wil->platform_handle) &&
- test_bit(WMI_FW_CAPABILITY_D3_SUSPEND, wil->fw_capabilities);
-
- wil_info(wil, "keep_radio_on_during_sleep (%d)\n",
- wil->keep_radio_on_during_sleep);
-
/* FW should raise IRQ when ready */
rc = wil_if_pcie_enable(wil);
if (rc) {
diff --git a/drivers/net/wireless/ath/wil6210/wil6210.h b/drivers/net/wireless/ath/wil6210/wil6210.h
index ad40a5a..8dfb4a7 100644
--- a/drivers/net/wireless/ath/wil6210/wil6210.h
+++ b/drivers/net/wireless/ath/wil6210/wil6210.h
@@ -872,6 +872,7 @@ int wil_ps_update(struct wil6210_priv *wil,
int __wil_up(struct wil6210_priv *wil);
int wil_down(struct wil6210_priv *wil);
int __wil_down(struct wil6210_priv *wil);
+void wil_refresh_fw_capabilities(struct wil6210_priv *wil);
void wil_mbox_ring_le2cpus(struct wil6210_mbox_ring *r);
int wil_find_cid(struct wil6210_priv *wil, const u8 *mac);
void wil_set_ethtoolops(struct net_device *ndev);
--
1.9.1

2017-10-25 13:14:36

by Maya Erez

[permalink] [raw]
Subject: [PATCH 12/12] wil6210: update statistics for suspend

From: Lazar Alexei <[email protected]>

Currently the statistics show how many successful/failed
suspend/resume operations the system had.
Update the statistics by splitting each successful/failed
suspend/resume operations to radio on/off.

Signed-off-by: Lazar Alexei <[email protected]>
Signed-off-by: Maya Erez <[email protected]>
---
drivers/net/wireless/ath/wil6210/debugfs.c | 27 ++++++++++++++++++---------
drivers/net/wireless/ath/wil6210/pcie_bus.c | 20 ++++++++++++++------
drivers/net/wireless/ath/wil6210/pm.c | 8 +++++---
drivers/net/wireless/ath/wil6210/wil6210.h | 11 ++++++++---
4 files changed, 45 insertions(+), 21 deletions(-)

diff --git a/drivers/net/wireless/ath/wil6210/debugfs.c b/drivers/net/wireless/ath/wil6210/debugfs.c
index 03d17ea..9164855 100644
--- a/drivers/net/wireless/ath/wil6210/debugfs.c
+++ b/drivers/net/wireless/ath/wil6210/debugfs.c
@@ -1685,20 +1685,29 @@ static ssize_t wil_read_suspend_stats(struct file *file,
size_t count, loff_t *ppos)
{
struct wil6210_priv *wil = file->private_data;
- static char text[400];
+ static char text[500];
int n;

n = snprintf(text, sizeof(text),
- "Suspend statistics:\n"
+ "Radio on suspend statistics:\n"
+ "successful suspends:%ld failed suspends:%ld\n"
+ "successful resumes:%ld failed resumes:%ld\n"
+ "rejected by device:%ld\n"
+ "Radio off suspend statistics:\n"
"successful suspends:%ld failed suspends:%ld\n"
"successful resumes:%ld failed resumes:%ld\n"
- "rejected by host:%ld rejected by device:%ld\n",
- wil->suspend_stats.successful_suspends,
- wil->suspend_stats.failed_suspends,
- wil->suspend_stats.successful_resumes,
- wil->suspend_stats.failed_resumes,
- wil->suspend_stats.rejected_by_host,
- wil->suspend_stats.rejected_by_device);
+ "General statistics:\n"
+ "rejected by host:%ld\n",
+ wil->suspend_stats.r_on.successful_suspends,
+ wil->suspend_stats.r_on.failed_suspends,
+ wil->suspend_stats.r_on.successful_resumes,
+ wil->suspend_stats.r_on.failed_resumes,
+ wil->suspend_stats.rejected_by_device,
+ wil->suspend_stats.r_off.successful_suspends,
+ wil->suspend_stats.r_off.failed_suspends,
+ wil->suspend_stats.r_off.successful_resumes,
+ wil->suspend_stats.r_off.failed_resumes,
+ wil->suspend_stats.rejected_by_host);

n = min_t(int, n, sizeof(text));

diff --git a/drivers/net/wireless/ath/wil6210/pcie_bus.c b/drivers/net/wireless/ath/wil6210/pcie_bus.c
index 07a44d3..9f11fdc 100644
--- a/drivers/net/wireless/ath/wil6210/pcie_bus.c
+++ b/drivers/net/wireless/ath/wil6210/pcie_bus.c
@@ -398,14 +398,16 @@ static int wil6210_suspend(struct device *dev, bool is_runtime)

rc = wil_suspend(wil, is_runtime, keep_radio_on);
if (!rc) {
- wil->suspend_stats.successful_suspends++;
-
/* In case radio stays on, platform device will control
* PCIe master
*/
- if (!keep_radio_on)
+ if (!keep_radio_on) {
/* disable bus mastering */
pci_clear_master(pdev);
+ wil->suspend_stats.r_off.successful_suspends++;
+ } else {
+ wil->suspend_stats.r_on.successful_suspends++;
+ }
}
out:
return rc;
@@ -431,11 +433,17 @@ static int wil6210_resume(struct device *dev, bool is_runtime)
rc = wil_resume(wil, is_runtime, keep_radio_on);
if (rc) {
wil_err(wil, "device failed to resume (%d)\n", rc);
- wil->suspend_stats.failed_resumes++;
- if (!keep_radio_on)
+ if (!keep_radio_on) {
pci_clear_master(pdev);
+ wil->suspend_stats.r_off.failed_resumes++;
+ } else {
+ wil->suspend_stats.r_on.failed_resumes++;
+ }
} else {
- wil->suspend_stats.successful_resumes++;
+ if (keep_radio_on)
+ wil->suspend_stats.r_on.successful_resumes++;
+ else
+ wil->suspend_stats.r_off.successful_resumes++;
}

return rc;
diff --git a/drivers/net/wireless/ath/wil6210/pm.c b/drivers/net/wireless/ath/wil6210/pm.c
index 48ec186..ec39ac7 100644
--- a/drivers/net/wireless/ath/wil6210/pm.c
+++ b/drivers/net/wireless/ath/wil6210/pm.c
@@ -179,7 +179,7 @@ static int wil_suspend_keep_radio_on(struct wil6210_priv *wil)
break;
wil_err(wil,
"TO waiting for idle RX, suspend failed\n");
- wil->suspend_stats.failed_suspends++;
+ wil->suspend_stats.r_on.failed_suspends++;
goto resume_after_fail;
}
wil_dbg_ratelimited(wil, "rx vring is not empty -> NAPI\n");
@@ -195,7 +195,7 @@ static int wil_suspend_keep_radio_on(struct wil6210_priv *wil)
*/
if (!wil_is_wmi_idle(wil)) {
wil_err(wil, "suspend failed due to pending WMI events\n");
- wil->suspend_stats.failed_suspends++;
+ wil->suspend_stats.r_on.failed_suspends++;
goto resume_after_fail;
}

@@ -209,7 +209,7 @@ static int wil_suspend_keep_radio_on(struct wil6210_priv *wil)
if (rc) {
wil_err(wil, "platform device failed to suspend (%d)\n",
rc);
- wil->suspend_stats.failed_suspends++;
+ wil->suspend_stats.r_on.failed_suspends++;
wil_c(wil, RGF_USER_CLKS_CTL_0, BIT_USER_CLKS_RST_PWGD);
wil_unmask_irq(wil);
goto resume_after_fail;
@@ -256,6 +256,7 @@ static int wil_suspend_radio_off(struct wil6210_priv *wil)
rc = wil_down(wil);
if (rc) {
wil_err(wil, "wil_down : %d\n", rc);
+ wil->suspend_stats.r_off.failed_suspends++;
goto out;
}
}
@@ -268,6 +269,7 @@ static int wil_suspend_radio_off(struct wil6210_priv *wil)
rc = wil->platform_ops.suspend(wil->platform_handle, false);
if (rc) {
wil_enable_irq(wil);
+ wil->suspend_stats.r_off.failed_suspends++;
goto out;
}
}
diff --git a/drivers/net/wireless/ath/wil6210/wil6210.h b/drivers/net/wireless/ath/wil6210/wil6210.h
index 0d8457e..52c477f 100644
--- a/drivers/net/wireless/ath/wil6210/wil6210.h
+++ b/drivers/net/wireless/ath/wil6210/wil6210.h
@@ -82,12 +82,17 @@ static inline u32 WIL_GET_BITS(u32 x, int b0, int b1)
*/
#define WIL_MAX_MPDU_OVERHEAD (62)

-struct wil_suspend_stats {
+struct wil_suspend_count_stats {
unsigned long successful_suspends;
- unsigned long failed_suspends;
unsigned long successful_resumes;
+ unsigned long failed_suspends;
unsigned long failed_resumes;
- unsigned long rejected_by_device;
+};
+
+struct wil_suspend_stats {
+ struct wil_suspend_count_stats r_off;
+ struct wil_suspend_count_stats r_on;
+ unsigned long rejected_by_device; /* only radio on */
unsigned long rejected_by_host;
};

--
1.9.1

2017-10-25 13:14:20

by Maya Erez

[permalink] [raw]
Subject: [PATCH 04/12] wil6210: abort properly in cfg suspend

From: Hamad Kadmany <[email protected]>

On-going operations were not aborted properly
and required locks were not taken.

Signed-off-by: Hamad Kadmany <[email protected]>
Signed-off-by: Maya Erez <[email protected]>
---
drivers/net/wireless/ath/wil6210/cfg80211.c | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/net/wireless/ath/wil6210/cfg80211.c b/drivers/net/wireless/ath/wil6210/cfg80211.c
index 85d5c04..c3d3c0c 100644
--- a/drivers/net/wireless/ath/wil6210/cfg80211.c
+++ b/drivers/net/wireless/ath/wil6210/cfg80211.c
@@ -1727,9 +1727,12 @@ static int wil_cfg80211_suspend(struct wiphy *wiphy,

wil_dbg_pm(wil, "suspending\n");

- wil_p2p_stop_discovery(wil);
-
+ mutex_lock(&wil->mutex);
+ mutex_lock(&wil->p2p_wdev_mutex);
+ wil_p2p_stop_radio_operations(wil);
wil_abort_scan(wil, true);
+ mutex_unlock(&wil->p2p_wdev_mutex);
+ mutex_unlock(&wil->mutex);

out:
return rc;
--
1.9.1

2017-10-25 13:14:26

by Maya Erez

[permalink] [raw]
Subject: [PATCH 07/12] wil6210: missing length check in wmi_set_ie

From: Lior David <[email protected]>

Add a length check in wmi_set_ie to detect unsigned integer
overflow.

Signed-off-by: Lior David <[email protected]>
Signed-off-by: Maya Erez <[email protected]>
---
drivers/net/wireless/ath/wil6210/wmi.c | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/net/wireless/ath/wil6210/wmi.c b/drivers/net/wireless/ath/wil6210/wmi.c
index 16aa624..dd25f63 100644
--- a/drivers/net/wireless/ath/wil6210/wmi.c
+++ b/drivers/net/wireless/ath/wil6210/wmi.c
@@ -1616,8 +1616,14 @@ int wmi_set_ie(struct wil6210_priv *wil, u8 type, u16 ie_len, const void *ie)
};
int rc;
u16 len = sizeof(struct wmi_set_appie_cmd) + ie_len;
- struct wmi_set_appie_cmd *cmd = kzalloc(len, GFP_KERNEL);
+ struct wmi_set_appie_cmd *cmd;

+ if (len < ie_len) {
+ rc = -EINVAL;
+ goto out;
+ }
+
+ cmd = kzalloc(len, GFP_KERNEL);
if (!cmd) {
rc = -ENOMEM;
goto out;
--
1.9.1

2017-10-25 13:14:28

by Maya Erez

[permalink] [raw]
Subject: [PATCH 08/12] wil6210: missing length check in wil_cfg80211_mgmt_tx

From: Lior David <[email protected]>

Add a length check in wil_cfg80211_mgmt_tx to detect unsigned integer
overflow.

Signed-off-by: Lior David <[email protected]>
Signed-off-by: Maya Erez <[email protected]>
---
drivers/net/wireless/ath/wil6210/cfg80211.c | 10 +++++++---
1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/net/wireless/ath/wil6210/cfg80211.c b/drivers/net/wireless/ath/wil6210/cfg80211.c
index c3d3c0c..771a534 100644
--- a/drivers/net/wireless/ath/wil6210/cfg80211.c
+++ b/drivers/net/wireless/ath/wil6210/cfg80211.c
@@ -901,7 +901,7 @@ int wil_cfg80211_mgmt_tx(struct wiphy *wiphy, struct wireless_dev *wdev,
u64 *cookie)
{
const u8 *buf = params->buf;
- size_t len = params->len;
+ size_t len = params->len, total;
struct wil6210_priv *wil = wiphy_to_wil(wiphy);
int rc;
bool tx_status = false;
@@ -926,7 +926,11 @@ int wil_cfg80211_mgmt_tx(struct wiphy *wiphy, struct wireless_dev *wdev,
if (len < sizeof(struct ieee80211_hdr_3addr))
return -EINVAL;

- cmd = kmalloc(sizeof(*cmd) + len, GFP_KERNEL);
+ total = sizeof(*cmd) + len;
+ if (total < len)
+ return -EINVAL;
+
+ cmd = kmalloc(total, GFP_KERNEL);
if (!cmd) {
rc = -ENOMEM;
goto out;
@@ -936,7 +940,7 @@ int wil_cfg80211_mgmt_tx(struct wiphy *wiphy, struct wireless_dev *wdev,
cmd->len = cpu_to_le16(len);
memcpy(cmd->payload, buf, len);

- rc = wmi_call(wil, WMI_SW_TX_REQ_CMDID, cmd, sizeof(*cmd) + len,
+ rc = wmi_call(wil, WMI_SW_TX_REQ_CMDID, cmd, total,
WMI_SW_TX_COMPLETE_EVENTID, &evt, sizeof(evt), 2000);
if (rc == 0)
tx_status = !evt.evt.status;
--
1.9.1

2017-10-30 06:54:40

by Kalle Valo

[permalink] [raw]
Subject: Re: [PATCH 03/12] wil6210: refresh FW capabilities during interface up

Lior David <[email protected]> writes:

> On 10/28/2017 6:01 PM, Kalle Valo wrote:
>> Maya Erez <[email protected]> writes:
>>=20
>>> From: Lior David <[email protected]>
>>>
>>> FW capabilities are currently retrieved only during module
>>> initialization, but userspace can replace the firmware while
>>> interface is down, so refresh the FW capabilities when
>>> interface is up (after FW is loaded) to ensure driver
>>> functionality matches the loaded FW.
>>=20
>> I think usually the firmware is loaded only once during probe() and I
>> think it's quite special that you retrieve it during interface up. Being
>> able to change the firmware version runtime like that can lead to
>> problems eventually, for example cfg80211 might not allow changing
>> already registered configuration etc.
>>=20
> Yes you are right, it is not perfect.
> We shutdown our chip in interface down so the FW is lost, we have to load=
it
> every interface up. We could request_firmware only once at insmod and sto=
re it
> so the same FW will be used every time but this is a big waste of memory =
(FW
> size is ~400KB and may be larger with future chips). We only touch one or=
two
> very simple fields in struct wiphy that are not validated in wiphy_regist=
er. The
> behavior is not 100% proof but good enough, and we recommend to our users=
to
> always rmmod/insmod when replacing FW (replacing FW at runtime is usually=
not
> done in production systems, this is only for debugging).
> However, if we do not refresh the capabilities we will have larger proble=
ms -
> the driver can try to access features not supported by FW and cause FW/ho=
st crashes.

Yes, I agree that this patch does improve things and should be applied.
But I think in the long run you should consider changing this and do the
same as other drivers do (load the firmware only on probe()).

--=20
Kalle Valo=

2017-10-25 13:14:30

by Maya Erez

[permalink] [raw]
Subject: [PATCH 09/12] wil6210: get suspend reject reason and resume triggers from FW

From: Lazar Alexei <[email protected]>

Upon receiving suspend reject, print reject reason.
Upon receiving resume event, print resume triggers.

Signed-off-by: Lazar Alexei <[email protected]>
Signed-off-by: Maya Erez <[email protected]>
---
drivers/net/wireless/ath/wil6210/wmi.c | 46 +++++++++++++++++++++++++++++++---
drivers/net/wireless/ath/wil6210/wmi.h | 17 ++++++++++---
2 files changed, 57 insertions(+), 6 deletions(-)

diff --git a/drivers/net/wireless/ath/wil6210/wmi.c b/drivers/net/wireless/ath/wil6210/wmi.c
index dd25f63..8ace618 100644
--- a/drivers/net/wireless/ath/wil6210/wmi.c
+++ b/drivers/net/wireless/ath/wil6210/wmi.c
@@ -2043,6 +2043,16 @@ void wmi_event_flush(struct wil6210_priv *wil)
spin_unlock_irqrestore(&wil->wmi_ev_lock, flags);
}

+static const char *suspend_status2name(u8 status)
+{
+ switch (status) {
+ case WMI_TRAFFIC_SUSPEND_REJECTED_LINK_NOT_IDLE:
+ return "LINK_NOT_IDLE";
+ default:
+ return "Untracked status";
+ }
+}
+
int wmi_suspend(struct wil6210_priv *wil)
{
int rc;
@@ -2058,7 +2068,7 @@ int wmi_suspend(struct wil6210_priv *wil)
wil->suspend_resp_rcvd = false;
wil->suspend_resp_comp = false;

- reply.evt.status = WMI_TRAFFIC_SUSPEND_REJECTED;
+ reply.evt.status = WMI_TRAFFIC_SUSPEND_REJECTED_LINK_NOT_IDLE;

rc = wmi_call(wil, WMI_TRAFFIC_SUSPEND_CMDID, &cmd, sizeof(cmd),
WMI_TRAFFIC_SUSPEND_EVENTID, &reply, sizeof(reply),
@@ -2090,8 +2100,9 @@ int wmi_suspend(struct wil6210_priv *wil)
}

wil_dbg_wmi(wil, "suspend_response_completed rcvd\n");
- if (reply.evt.status == WMI_TRAFFIC_SUSPEND_REJECTED) {
- wil_dbg_pm(wil, "device rejected the suspend\n");
+ if (reply.evt.status != WMI_TRAFFIC_SUSPEND_APPROVED) {
+ wil_dbg_pm(wil, "device rejected the suspend, %s\n",
+ suspend_status2name(reply.evt.status));
wil->suspend_stats.rejected_by_device++;
}
rc = reply.evt.status;
@@ -2103,21 +2114,50 @@ int wmi_suspend(struct wil6210_priv *wil)
return rc;
}

+static void resume_triggers2string(u32 triggers, char *string, int str_size)
+{
+ string[0] = '\0';
+
+ if (!triggers) {
+ strlcat(string, " UNKNOWN", str_size);
+ return;
+ }
+
+ if (triggers & WMI_RESUME_TRIGGER_HOST)
+ strlcat(string, " HOST", str_size);
+
+ if (triggers & WMI_RESUME_TRIGGER_UCAST_RX)
+ strlcat(string, " UCAST_RX", str_size);
+
+ if (triggers & WMI_RESUME_TRIGGER_BCAST_RX)
+ strlcat(string, " BCAST_RX", str_size);
+
+ if (triggers & WMI_RESUME_TRIGGER_WMI_EVT)
+ strlcat(string, " WMI_EVT", str_size);
+}
+
int wmi_resume(struct wil6210_priv *wil)
{
int rc;
+ char string[100];
struct {
struct wmi_cmd_hdr wmi;
struct wmi_traffic_resume_event evt;
} __packed reply;

reply.evt.status = WMI_TRAFFIC_RESUME_FAILED;
+ reply.evt.resume_triggers = WMI_RESUME_TRIGGER_UNKNOWN;

rc = wmi_call(wil, WMI_TRAFFIC_RESUME_CMDID, NULL, 0,
WMI_TRAFFIC_RESUME_EVENTID, &reply, sizeof(reply),
WIL_WAIT_FOR_SUSPEND_RESUME_COMP);
if (rc)
return rc;
+ resume_triggers2string(le32_to_cpu(reply.evt.resume_triggers), string,
+ sizeof(string));
+ wil_dbg_pm(wil, "device resume %s, resume triggers:%s (0x%x)\n",
+ reply.evt.status ? "failed" : "passed", string,
+ le32_to_cpu(reply.evt.resume_triggers));

return reply.evt.status;
}
diff --git a/drivers/net/wireless/ath/wil6210/wmi.h b/drivers/net/wireless/ath/wil6210/wmi.h
index 5263ee7..d9e220a 100644
--- a/drivers/net/wireless/ath/wil6210/wmi.h
+++ b/drivers/net/wireless/ath/wil6210/wmi.h
@@ -2267,8 +2267,8 @@ struct wmi_link_maintain_cfg_read_done_event {
} __packed;

enum wmi_traffic_suspend_status {
- WMI_TRAFFIC_SUSPEND_APPROVED = 0x0,
- WMI_TRAFFIC_SUSPEND_REJECTED = 0x1,
+ WMI_TRAFFIC_SUSPEND_APPROVED = 0x0,
+ WMI_TRAFFIC_SUSPEND_REJECTED_LINK_NOT_IDLE = 0x1,
};

/* WMI_TRAFFIC_SUSPEND_EVENTID */
@@ -2282,10 +2282,21 @@ enum wmi_traffic_resume_status {
WMI_TRAFFIC_RESUME_FAILED = 0x1,
};

+enum wmi_resume_trigger {
+ WMI_RESUME_TRIGGER_UNKNOWN = 0x0,
+ WMI_RESUME_TRIGGER_HOST = 0x1,
+ WMI_RESUME_TRIGGER_UCAST_RX = 0x2,
+ WMI_RESUME_TRIGGER_BCAST_RX = 0x4,
+ WMI_RESUME_TRIGGER_WMI_EVT = 0x8,
+};
+
/* WMI_TRAFFIC_RESUME_EVENTID */
struct wmi_traffic_resume_event {
- /* enum wmi_traffic_resume_status_e */
+ /* enum wmi_traffic_resume_status */
u8 status;
+ u8 reserved[3];
+ /* enum wmi_resume_trigger bitmap */
+ __le32 resume_triggers;
} __packed;

/* Power Save command completion status codes */
--
1.9.1

2017-10-25 13:14:32

by Maya Erez

[permalink] [raw]
Subject: [PATCH 10/12] wil6210: fix PCIe bus mastering in case of interface down

From: Lazar Alexei <[email protected]>

In case of interface down, radio is turned off but PCIe mastering is
not cleared.
This can cause unexpected PCIe access to the shutdown device.
Fix this by clearing PCIe mastering also in case interface is down

Signed-off-by: Lazar Alexei <[email protected]>
Signed-off-by: Maya Erez <[email protected]>
---
drivers/net/wireless/ath/wil6210/pcie_bus.c | 24 +++++++++++++++---------
drivers/net/wireless/ath/wil6210/pm.c | 10 ++--------
drivers/net/wireless/ath/wil6210/wil6210.h | 4 ++--
3 files changed, 19 insertions(+), 19 deletions(-)

diff --git a/drivers/net/wireless/ath/wil6210/pcie_bus.c b/drivers/net/wireless/ath/wil6210/pcie_bus.c
index 659c32c..07a44d3 100644
--- a/drivers/net/wireless/ath/wil6210/pcie_bus.c
+++ b/drivers/net/wireless/ath/wil6210/pcie_bus.c
@@ -386,6 +386,9 @@ static int wil6210_suspend(struct device *dev, bool is_runtime)
int rc = 0;
struct pci_dev *pdev = to_pci_dev(dev);
struct wil6210_priv *wil = pci_get_drvdata(pdev);
+ struct net_device *ndev = wil_to_ndev(wil);
+ bool keep_radio_on = ndev->flags & IFF_UP &&
+ wil->keep_radio_on_during_sleep;

wil_dbg_pm(wil, "suspend: %s\n", is_runtime ? "runtime" : "system");

@@ -393,14 +396,14 @@ static int wil6210_suspend(struct device *dev, bool is_runtime)
if (rc)
goto out;

- rc = wil_suspend(wil, is_runtime);
+ rc = wil_suspend(wil, is_runtime, keep_radio_on);
if (!rc) {
wil->suspend_stats.successful_suspends++;

- /* If platform device supports keep_radio_on_during_sleep
- * it will control PCIe master
+ /* In case radio stays on, platform device will control
+ * PCIe master
*/
- if (!wil->keep_radio_on_during_sleep)
+ if (!keep_radio_on)
/* disable bus mastering */
pci_clear_master(pdev);
}
@@ -413,20 +416,23 @@ static int wil6210_resume(struct device *dev, bool is_runtime)
int rc = 0;
struct pci_dev *pdev = to_pci_dev(dev);
struct wil6210_priv *wil = pci_get_drvdata(pdev);
+ struct net_device *ndev = wil_to_ndev(wil);
+ bool keep_radio_on = ndev->flags & IFF_UP &&
+ wil->keep_radio_on_during_sleep;

wil_dbg_pm(wil, "resume: %s\n", is_runtime ? "runtime" : "system");

- /* If platform device supports keep_radio_on_during_sleep it will
- * control PCIe master
+ /* In case radio stays on, platform device will control
+ * PCIe master
*/
- if (!wil->keep_radio_on_during_sleep)
+ if (!keep_radio_on)
/* allow master */
pci_set_master(pdev);
- rc = wil_resume(wil, is_runtime);
+ rc = wil_resume(wil, is_runtime, keep_radio_on);
if (rc) {
wil_err(wil, "device failed to resume (%d)\n", rc);
wil->suspend_stats.failed_resumes++;
- if (!wil->keep_radio_on_during_sleep)
+ if (!keep_radio_on)
pci_clear_master(pdev);
} else {
wil->suspend_stats.successful_resumes++;
diff --git a/drivers/net/wireless/ath/wil6210/pm.c b/drivers/net/wireless/ath/wil6210/pm.c
index 2ef2f34..ce4f546 100644
--- a/drivers/net/wireless/ath/wil6210/pm.c
+++ b/drivers/net/wireless/ath/wil6210/pm.c
@@ -300,12 +300,9 @@ static int wil_resume_radio_off(struct wil6210_priv *wil)
return rc;
}

-int wil_suspend(struct wil6210_priv *wil, bool is_runtime)
+int wil_suspend(struct wil6210_priv *wil, bool is_runtime, bool keep_radio_on)
{
int rc = 0;
- struct net_device *ndev = wil_to_ndev(wil);
- bool keep_radio_on = ndev->flags & IFF_UP &&
- wil->keep_radio_on_during_sleep;

wil_dbg_pm(wil, "suspend: %s\n", is_runtime ? "runtime" : "system");

@@ -328,12 +325,9 @@ int wil_suspend(struct wil6210_priv *wil, bool is_runtime)
return rc;
}

-int wil_resume(struct wil6210_priv *wil, bool is_runtime)
+int wil_resume(struct wil6210_priv *wil, bool is_runtime, bool keep_radio_on)
{
int rc = 0;
- struct net_device *ndev = wil_to_ndev(wil);
- bool keep_radio_on = ndev->flags & IFF_UP &&
- wil->keep_radio_on_during_sleep;
unsigned long long suspend_time_usec = 0;

wil_dbg_pm(wil, "resume: %s\n", is_runtime ? "runtime" : "system");
diff --git a/drivers/net/wireless/ath/wil6210/wil6210.h b/drivers/net/wireless/ath/wil6210/wil6210.h
index 6b09998..b7509f3 100644
--- a/drivers/net/wireless/ath/wil6210/wil6210.h
+++ b/drivers/net/wireless/ath/wil6210/wil6210.h
@@ -1018,8 +1018,8 @@ int wil_request_firmware(struct wil6210_priv *wil, const char *name,
void wil_pm_runtime_put(struct wil6210_priv *wil);

int wil_can_suspend(struct wil6210_priv *wil, bool is_runtime);
-int wil_suspend(struct wil6210_priv *wil, bool is_runtime);
-int wil_resume(struct wil6210_priv *wil, bool is_runtime);
+int wil_suspend(struct wil6210_priv *wil, bool is_runtime, bool keep_radio_on);
+int wil_resume(struct wil6210_priv *wil, bool is_runtime, bool keep_radio_on);
bool wil_is_wmi_idle(struct wil6210_priv *wil);
int wmi_resume(struct wil6210_priv *wil);
int wmi_suspend(struct wil6210_priv *wil);
--
1.9.1

2017-10-27 13:33:18

by Kalle Valo

[permalink] [raw]
Subject: Re: [01/12] wil6210: run-time PM when interface down

Maya Erez <[email protected]> wrote:

> Allow run-time suspend when interface is down, keep card alive when
> interface is up.
> If driver is in wmi only or debug_fw mode run-time PM won't suspend.
>
> Signed-off-by: Lazar Alexei <[email protected]>
> pcie_bus.c
> Signed-off-by: Maya Erez <[email protected]>
> Signed-off-by: Kalle Valo <[email protected]>

Kbuild bot found problems:

drivers/net//wireless/ath/wil6210/pcie_bus.c: In function 'wil6210_pm_runtime_resume':
>> drivers/net//wireless/ath/wil6210/pcie_bus.c:511:2: error: implicit
>> declaration of function 'wil6210_resume'
>> [-Werror=implicit-function-declaration]
return wil6210_resume(dev, true);
^
drivers/net//wireless/ath/wil6210/pcie_bus.c: In function
'wil6210_pm_runtime_suspend':
>> drivers/net//wireless/ath/wil6210/pcie_bus.c:524:2: error: implicit
>> declaration of function 'wil6210_suspend'
>> [-Werror=implicit-function-declaration]
return wil6210_suspend(dev, true);
^

--
https://patchwork.kernel.org/patch/10026675/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

2017-10-25 13:14:14

by Maya Erez

[permalink] [raw]
Subject: [PATCH 01/12] wil6210: run-time PM when interface down

From: Lazar Alexei <[email protected]>

Allow run-time suspend when interface is down, keep card alive when
interface is up.
If driver is in wmi only or debug_fw mode run-time PM won't suspend.

Signed-off-by: Lazar Alexei <[email protected]>
pcie_bus.c
Signed-off-by: Maya Erez <[email protected]>
---
drivers/net/wireless/ath/wil6210/debugfs.c | 96 ++++++++++++++++++++++++++---
drivers/net/wireless/ath/wil6210/ethtool.c | 15 +++++
drivers/net/wireless/ath/wil6210/netdev.c | 18 +++++-
drivers/net/wireless/ath/wil6210/pcie_bus.c | 34 ++++++++++
drivers/net/wireless/ath/wil6210/pm.c | 62 +++++++++++++++++++
drivers/net/wireless/ath/wil6210/wil6210.h | 16 +++++
6 files changed, 231 insertions(+), 10 deletions(-)

diff --git a/drivers/net/wireless/ath/wil6210/debugfs.c b/drivers/net/wireless/ath/wil6210/debugfs.c
index 6db00c1..2634650 100644
--- a/drivers/net/wireless/ath/wil6210/debugfs.c
+++ b/drivers/net/wireless/ath/wil6210/debugfs.c
@@ -242,12 +242,19 @@ static void wil_print_ring(struct seq_file *s, const char *prefix,
static int wil_mbox_debugfs_show(struct seq_file *s, void *data)
{
struct wil6210_priv *wil = s->private;
+ int ret;
+
+ ret = wil_pm_runtime_get(wil);
+ if (ret < 0)
+ return ret;

wil_print_ring(s, "tx", wil->csr + HOST_MBOX +
offsetof(struct wil6210_mbox_ctl, tx));
wil_print_ring(s, "rx", wil->csr + HOST_MBOX +
offsetof(struct wil6210_mbox_ctl, rx));

+ wil_pm_runtime_put(wil);
+
return 0;
}

@@ -265,15 +272,37 @@ static int wil_mbox_seq_open(struct inode *inode, struct file *file)

static int wil_debugfs_iomem_x32_set(void *data, u64 val)
{
- writel(val, (void __iomem *)data);
+ struct wil_debugfs_iomem_data *d = (struct
+ wil_debugfs_iomem_data *)data;
+ struct wil6210_priv *wil = d->wil;
+ int ret;
+
+ ret = wil_pm_runtime_get(wil);
+ if (ret < 0)
+ return ret;
+
+ writel(val, (void __iomem *)d->offset);
wmb(); /* make sure write propagated to HW */

+ wil_pm_runtime_put(wil);
+
return 0;
}

static int wil_debugfs_iomem_x32_get(void *data, u64 *val)
{
- *val = readl((void __iomem *)data);
+ struct wil_debugfs_iomem_data *d = (struct
+ wil_debugfs_iomem_data *)data;
+ struct wil6210_priv *wil = d->wil;
+ int ret;
+
+ ret = wil_pm_runtime_get(wil);
+ if (ret < 0)
+ return ret;
+
+ *val = readl((void __iomem *)d->offset);
+
+ wil_pm_runtime_put(wil);

return 0;
}
@@ -284,10 +313,21 @@ static int wil_debugfs_iomem_x32_get(void *data, u64 *val)
static struct dentry *wil_debugfs_create_iomem_x32(const char *name,
umode_t mode,
struct dentry *parent,
- void *value)
+ void *value,
+ struct wil6210_priv *wil)
{
- return debugfs_create_file(name, mode, parent, value,
- &fops_iomem_x32);
+ struct dentry *file;
+ struct wil_debugfs_iomem_data *data = &wil->dbg_data.data_arr[
+ wil->dbg_data.iomem_data_count];
+
+ data->wil = wil;
+ data->offset = value;
+
+ file = debugfs_create_file(name, mode, parent, data, &fops_iomem_x32);
+ if (!IS_ERR_OR_NULL(file))
+ wil->dbg_data.iomem_data_count++;
+
+ return file;
}

static int wil_debugfs_ulong_set(void *data, u64 val)
@@ -346,7 +386,8 @@ static void wil6210_debugfs_init_offset(struct wil6210_priv *wil,
case doff_io32:
f = wil_debugfs_create_iomem_x32(tbl[i].name,
tbl[i].mode, dbg,
- base + tbl[i].off);
+ base + tbl[i].off,
+ wil);
break;
case doff_u8:
f = debugfs_create_u8(tbl[i].name, tbl[i].mode, dbg,
@@ -475,13 +516,22 @@ static int wil6210_debugfs_create_ITR_CNT(struct wil6210_priv *wil,
static int wil_memread_debugfs_show(struct seq_file *s, void *data)
{
struct wil6210_priv *wil = s->private;
- void __iomem *a = wmi_buffer(wil, cpu_to_le32(mem_addr));
+ void __iomem *a;
+ int ret;
+
+ ret = wil_pm_runtime_get(wil);
+ if (ret < 0)
+ return ret;
+
+ a = wmi_buffer(wil, cpu_to_le32(mem_addr));

if (a)
seq_printf(s, "[0x%08x] = 0x%08x\n", mem_addr, readl(a));
else
seq_printf(s, "[0x%08x] = INVALID\n", mem_addr);

+ wil_pm_runtime_put(wil);
+
return 0;
}

@@ -502,10 +552,12 @@ static ssize_t wil_read_file_ioblob(struct file *file, char __user *user_buf,
{
enum { max_count = 4096 };
struct wil_blob_wrapper *wil_blob = file->private_data;
+ struct wil6210_priv *wil = wil_blob->wil;
loff_t pos = *ppos;
size_t available = wil_blob->blob.size;
void *buf;
size_t ret;
+ int rc;

if (test_bit(wil_status_suspending, wil_blob->wil->status) ||
test_bit(wil_status_suspended, wil_blob->wil->status))
@@ -526,10 +578,19 @@ static ssize_t wil_read_file_ioblob(struct file *file, char __user *user_buf,
if (!buf)
return -ENOMEM;

+ rc = wil_pm_runtime_get(wil);
+ if (rc < 0) {
+ kfree(buf);
+ return rc;
+ }
+
wil_memcpy_fromio_32(buf, (const void __iomem *)
wil_blob->blob.data + pos, count);

ret = copy_to_user(user_buf, buf, count);
+
+ wil_pm_runtime_put(wil);
+
kfree(buf);
if (ret == count)
return -EFAULT;
@@ -1781,14 +1842,31 @@ static void wil6210_debugfs_init_isr(struct wil6210_priv *wil,
{},
};

+static const int dbg_off_count = 4 * (ARRAY_SIZE(isr_off) - 1) +
+ ARRAY_SIZE(dbg_wil_regs) - 1 +
+ ARRAY_SIZE(pseudo_isr_off) - 1 +
+ ARRAY_SIZE(lgc_itr_cnt_off) - 1 +
+ ARRAY_SIZE(tx_itr_cnt_off) - 1 +
+ ARRAY_SIZE(rx_itr_cnt_off) - 1;
+
int wil6210_debugfs_init(struct wil6210_priv *wil)
{
struct dentry *dbg = wil->debug = debugfs_create_dir(WIL_NAME,
wil_to_wiphy(wil)->debugfsdir);
-
if (IS_ERR_OR_NULL(dbg))
return -ENODEV;

+ wil->dbg_data.data_arr = kcalloc(dbg_off_count,
+ sizeof(struct wil_debugfs_iomem_data),
+ GFP_KERNEL);
+ if (!wil->dbg_data.data_arr) {
+ debugfs_remove_recursive(dbg);
+ wil->debug = NULL;
+ return -ENOMEM;
+ }
+
+ wil->dbg_data.iomem_data_count = 0;
+
wil_pmc_init(wil);

wil6210_debugfs_init_files(wil, dbg);
@@ -1813,6 +1891,8 @@ void wil6210_debugfs_remove(struct wil6210_priv *wil)
debugfs_remove_recursive(wil->debug);
wil->debug = NULL;

+ kfree(wil->dbg_data.data_arr);
+
/* free pmc memory without sending command to fw, as it will
* be reset on the way down anyway
*/
diff --git a/drivers/net/wireless/ath/wil6210/ethtool.c b/drivers/net/wireless/ath/wil6210/ethtool.c
index adcfef4..66200f6 100644
--- a/drivers/net/wireless/ath/wil6210/ethtool.c
+++ b/drivers/net/wireless/ath/wil6210/ethtool.c
@@ -47,9 +47,14 @@ static int wil_ethtoolops_get_coalesce(struct net_device *ndev,
struct wil6210_priv *wil = ndev_to_wil(ndev);
u32 tx_itr_en, tx_itr_val = 0;
u32 rx_itr_en, rx_itr_val = 0;
+ int ret;

wil_dbg_misc(wil, "ethtoolops_get_coalesce\n");

+ ret = wil_pm_runtime_get(wil);
+ if (ret < 0)
+ return ret;
+
tx_itr_en = wil_r(wil, RGF_DMA_ITR_TX_CNT_CTL);
if (tx_itr_en & BIT_DMA_ITR_TX_CNT_CTL_EN)
tx_itr_val = wil_r(wil, RGF_DMA_ITR_TX_CNT_TRSH);
@@ -58,6 +63,8 @@ static int wil_ethtoolops_get_coalesce(struct net_device *ndev,
if (rx_itr_en & BIT_DMA_ITR_RX_CNT_CTL_EN)
rx_itr_val = wil_r(wil, RGF_DMA_ITR_RX_CNT_TRSH);

+ wil_pm_runtime_put(wil);
+
cp->tx_coalesce_usecs = tx_itr_val;
cp->rx_coalesce_usecs = rx_itr_val;
return 0;
@@ -67,6 +74,7 @@ static int wil_ethtoolops_set_coalesce(struct net_device *ndev,
struct ethtool_coalesce *cp)
{
struct wil6210_priv *wil = ndev_to_wil(ndev);
+ int ret;

wil_dbg_misc(wil, "ethtoolops_set_coalesce: rx %d usec, tx %d usec\n",
cp->rx_coalesce_usecs, cp->tx_coalesce_usecs);
@@ -86,8 +94,15 @@ static int wil_ethtoolops_set_coalesce(struct net_device *ndev,

wil->tx_max_burst_duration = cp->tx_coalesce_usecs;
wil->rx_max_burst_duration = cp->rx_coalesce_usecs;
+
+ ret = wil_pm_runtime_get(wil);
+ if (ret < 0)
+ return ret;
+
wil_configure_interrupt_moderation(wil);

+ wil_pm_runtime_put(wil);
+
return 0;

out_bad:
diff --git a/drivers/net/wireless/ath/wil6210/netdev.c b/drivers/net/wireless/ath/wil6210/netdev.c
index 4a6ab2d..b641ac1 100644
--- a/drivers/net/wireless/ath/wil6210/netdev.c
+++ b/drivers/net/wireless/ath/wil6210/netdev.c
@@ -21,6 +21,7 @@
static int wil_open(struct net_device *ndev)
{
struct wil6210_priv *wil = ndev_to_wil(ndev);
+ int rc;

wil_dbg_misc(wil, "open\n");

@@ -30,16 +31,29 @@ static int wil_open(struct net_device *ndev)
return -EINVAL;
}

- return wil_up(wil);
+ rc = wil_pm_runtime_get(wil);
+ if (rc < 0)
+ return rc;
+
+ rc = wil_up(wil);
+ if (rc)
+ wil_pm_runtime_put(wil);
+
+ return rc;
}

static int wil_stop(struct net_device *ndev)
{
struct wil6210_priv *wil = ndev_to_wil(ndev);
+ int rc;

wil_dbg_misc(wil, "stop\n");

- return wil_down(wil);
+ rc = wil_down(wil);
+ if (!rc)
+ wil_pm_runtime_put(wil);
+
+ return rc;
}

static const struct net_device_ops wil_netdev_ops = {
diff --git a/drivers/net/wireless/ath/wil6210/pcie_bus.c b/drivers/net/wireless/ath/wil6210/pcie_bus.c
index 6a3ab4b..9cc97da 100644
--- a/drivers/net/wireless/ath/wil6210/pcie_bus.c
+++ b/drivers/net/wireless/ath/wil6210/pcie_bus.c
@@ -21,6 +21,7 @@
#include <linux/suspend.h>
#include "wil6210.h"
#include <linux/rtnetlink.h>
+#include <linux/pm_runtime.h>

static bool use_msi = true;
module_param(use_msi, bool, 0444);
@@ -333,6 +334,7 @@ static int wil_pcie_probe(struct pci_dev *pdev, const struct pci_device_id *id)

wil6210_debugfs_init(wil);

+ wil_pm_runtime_allow(wil);

return 0;

@@ -365,6 +367,8 @@ static void wil_pcie_remove(struct pci_dev *pdev)
#endif /* CONFIG_PM_SLEEP */
#endif /* CONFIG_PM */

+ wil_pm_runtime_forbid(wil);
+
wil6210_debugfs_remove(wil);
rtnl_lock();
wil_p2p_wdev_free(wil);
@@ -492,10 +496,40 @@ static int wil6210_pm_resume(struct device *dev)
}
#endif /* CONFIG_PM_SLEEP */

+static int wil6210_pm_runtime_idle(struct device *dev)
+{
+ struct pci_dev *pdev = to_pci_dev(dev);
+ struct wil6210_priv *wil = pci_get_drvdata(pdev);
+
+ wil_dbg_pm(wil, "Runtime idle\n");
+
+ return wil_can_suspend(wil, true);
+}
+
+static int wil6210_pm_runtime_resume(struct device *dev)
+{
+ return wil6210_resume(dev, true);
+}
+
+static int wil6210_pm_runtime_suspend(struct device *dev)
+{
+ struct pci_dev *pdev = to_pci_dev(dev);
+ struct wil6210_priv *wil = pci_get_drvdata(pdev);
+
+ if (test_bit(wil_status_suspended, wil->status)) {
+ wil_dbg_pm(wil, "trying to suspend while suspended\n");
+ return 1;
+ }
+
+ return wil6210_suspend(dev, true);
+}
#endif /* CONFIG_PM */

static const struct dev_pm_ops wil6210_pm_ops = {
SET_SYSTEM_SLEEP_PM_OPS(wil6210_pm_suspend, wil6210_pm_resume)
+ SET_RUNTIME_PM_OPS(wil6210_pm_runtime_suspend,
+ wil6210_pm_runtime_resume,
+ wil6210_pm_runtime_idle)
};

static struct pci_driver wil6210_driver = {
diff --git a/drivers/net/wireless/ath/wil6210/pm.c b/drivers/net/wireless/ath/wil6210/pm.c
index 8f5d1b44..2ef2f34 100644
--- a/drivers/net/wireless/ath/wil6210/pm.c
+++ b/drivers/net/wireless/ath/wil6210/pm.c
@@ -16,15 +16,26 @@

#include "wil6210.h"
#include <linux/jiffies.h>
+#include <linux/pm_runtime.h>
+
+#define WIL6210_AUTOSUSPEND_DELAY_MS (1000)

int wil_can_suspend(struct wil6210_priv *wil, bool is_runtime)
{
int rc = 0;
struct wireless_dev *wdev = wil->wdev;
struct net_device *ndev = wil_to_ndev(wil);
+ bool wmi_only = test_bit(WMI_FW_CAPABILITY_WMI_ONLY,
+ wil->fw_capabilities);

wil_dbg_pm(wil, "can_suspend: %s\n", is_runtime ? "runtime" : "system");

+ if (wmi_only || debug_fw) {
+ wil_dbg_pm(wil, "Deny any suspend - %s mode\n",
+ wmi_only ? "wmi_only" : "debug_fw");
+ rc = -EPERM;
+ goto out;
+ }
if (!(ndev->flags & IFF_UP)) {
/* can always sleep when down */
wil_dbg_pm(wil, "Interface is down\n");
@@ -44,6 +55,10 @@ int wil_can_suspend(struct wil6210_priv *wil, bool is_runtime)
/* interface is running */
switch (wdev->iftype) {
case NL80211_IFTYPE_MONITOR:
+ wil_dbg_pm(wil, "Sniffer\n");
+ rc = -EBUSY;
+ goto out;
+ /* for STA-like interface, don't runtime suspend */
case NL80211_IFTYPE_STATION:
case NL80211_IFTYPE_P2P_CLIENT:
if (test_bit(wil_status_fwconnecting, wil->status)) {
@@ -51,6 +66,12 @@ int wil_can_suspend(struct wil6210_priv *wil, bool is_runtime)
rc = -EBUSY;
goto out;
}
+ /* Runtime pm not supported in case the interface is up */
+ if (is_runtime) {
+ wil_dbg_pm(wil, "STA-like interface\n");
+ rc = -EBUSY;
+ goto out;
+ }
break;
/* AP-like interface - can't suspend */
default:
@@ -348,3 +369,44 @@ int wil_resume(struct wil6210_priv *wil, bool is_runtime)
is_runtime ? "runtime" : "system", rc, suspend_time_usec);
return rc;
}
+
+void wil_pm_runtime_allow(struct wil6210_priv *wil)
+{
+ struct device *dev = wil_to_dev(wil);
+
+ pm_runtime_put_noidle(dev);
+ pm_runtime_set_autosuspend_delay(dev, WIL6210_AUTOSUSPEND_DELAY_MS);
+ pm_runtime_use_autosuspend(dev);
+ pm_runtime_allow(dev);
+}
+
+void wil_pm_runtime_forbid(struct wil6210_priv *wil)
+{
+ struct device *dev = wil_to_dev(wil);
+
+ pm_runtime_forbid(dev);
+ pm_runtime_get_noresume(dev);
+}
+
+int wil_pm_runtime_get(struct wil6210_priv *wil)
+{
+ int rc;
+ struct device *dev = wil_to_dev(wil);
+
+ rc = pm_runtime_get_sync(dev);
+ if (rc < 0) {
+ wil_err(wil, "pm_runtime_get_sync() failed, rc = %d\n", rc);
+ pm_runtime_put_noidle(dev);
+ return rc;
+ }
+
+ return 0;
+}
+
+void wil_pm_runtime_put(struct wil6210_priv *wil)
+{
+ struct device *dev = wil_to_dev(wil);
+
+ pm_runtime_mark_last_busy(dev);
+ pm_runtime_put_autosuspend(dev);
+}
diff --git a/drivers/net/wireless/ath/wil6210/wil6210.h b/drivers/net/wireless/ath/wil6210/wil6210.h
index 315ec8b..ad40a5a 100644
--- a/drivers/net/wireless/ath/wil6210/wil6210.h
+++ b/drivers/net/wireless/ath/wil6210/wil6210.h
@@ -616,6 +616,16 @@ struct blink_on_off_time {
u32 off_ms;
};

+struct wil_debugfs_iomem_data {
+ void *offset;
+ struct wil6210_priv *wil;
+};
+
+struct wil_debugfs_data {
+ struct wil_debugfs_iomem_data *data_arr;
+ int iomem_data_count;
+};
+
extern struct blink_on_off_time led_blink_time[WIL_LED_TIME_LAST];
extern u8 led_id;
extern u8 led_polarity;
@@ -708,6 +718,7 @@ struct wil6210_priv {
u8 abft_len;
u8 wakeup_trigger;
struct wil_suspend_stats suspend_stats;
+ struct wil_debugfs_data dbg_data;

void *platform_handle;
struct wil_platform_ops platform_ops;
@@ -999,6 +1010,11 @@ int wil_request_firmware(struct wil6210_priv *wil, const char *name,
bool load);
bool wil_fw_verify_file_exists(struct wil6210_priv *wil, const char *name);

+void wil_pm_runtime_allow(struct wil6210_priv *wil);
+void wil_pm_runtime_forbid(struct wil6210_priv *wil);
+int wil_pm_runtime_get(struct wil6210_priv *wil);
+void wil_pm_runtime_put(struct wil6210_priv *wil);
+
int wil_can_suspend(struct wil6210_priv *wil, bool is_runtime);
int wil_suspend(struct wil6210_priv *wil, bool is_runtime);
int wil_resume(struct wil6210_priv *wil, bool is_runtime);
--
1.9.1

2017-10-25 13:14:22

by Maya Erez

[permalink] [raw]
Subject: [PATCH 05/12] wil6210: fix length check in __wmi_send

From: Lior David <[email protected]>

The current length check:
sizeof(cmd) + len > r->entry_size
will allow very large values of len (> U16_MAX - sizeof(cmd))
and can cause a buffer overflow. Fix the check to cover this case.
In addition, ensure the mailbox entry_size is not too small,
since this can also bypass the above check.

Signed-off-by: Lior David <[email protected]>
Signed-off-by: Maya Erez <[email protected]>
---
drivers/net/wireless/ath/wil6210/interrupt.c | 22 +++++++++++++++++++++-
drivers/net/wireless/ath/wil6210/wmi.c | 2 +-
2 files changed, 22 insertions(+), 2 deletions(-)

diff --git a/drivers/net/wireless/ath/wil6210/interrupt.c b/drivers/net/wireless/ath/wil6210/interrupt.c
index 59def4f..5cf3417 100644
--- a/drivers/net/wireless/ath/wil6210/interrupt.c
+++ b/drivers/net/wireless/ath/wil6210/interrupt.c
@@ -358,6 +358,25 @@ static void wil_cache_mbox_regs(struct wil6210_priv *wil)
wil_mbox_ring_le2cpus(&wil->mbox_ctl.tx);
}

+static bool wil_validate_mbox_regs(struct wil6210_priv *wil)
+{
+ size_t min_size = sizeof(struct wil6210_mbox_hdr) +
+ sizeof(struct wmi_cmd_hdr);
+
+ if (wil->mbox_ctl.rx.entry_size < min_size) {
+ wil_err(wil, "rx mbox entry too small (%d)\n",
+ wil->mbox_ctl.rx.entry_size);
+ return false;
+ }
+ if (wil->mbox_ctl.tx.entry_size < min_size) {
+ wil_err(wil, "tx mbox entry too small (%d)\n",
+ wil->mbox_ctl.tx.entry_size);
+ return false;
+ }
+
+ return true;
+}
+
static irqreturn_t wil6210_irq_misc(int irq, void *cookie)
{
struct wil6210_priv *wil = cookie;
@@ -393,7 +412,8 @@ static irqreturn_t wil6210_irq_misc(int irq, void *cookie)
if (isr & ISR_MISC_FW_READY) {
wil_dbg_irq(wil, "IRQ: FW ready\n");
wil_cache_mbox_regs(wil);
- set_bit(wil_status_mbox_ready, wil->status);
+ if (wil_validate_mbox_regs(wil))
+ set_bit(wil_status_mbox_ready, wil->status);
/**
* Actual FW ready indicated by the
* WMI_FW_READY_EVENTID
diff --git a/drivers/net/wireless/ath/wil6210/wmi.c b/drivers/net/wireless/ath/wil6210/wmi.c
index 8a780f2..dbdf71d 100644
--- a/drivers/net/wireless/ath/wil6210/wmi.c
+++ b/drivers/net/wireless/ath/wil6210/wmi.c
@@ -448,7 +448,7 @@ static int __wmi_send(struct wil6210_priv *wil, u16 cmdid, void *buf, u16 len)
uint retry;
int rc = 0;

- if (sizeof(cmd) + len > r->entry_size) {
+ if (len > r->entry_size - sizeof(cmd)) {
wil_err(wil, "WMI size too large: %d bytes, max is %d\n",
(int)(sizeof(cmd) + len), r->entry_size);
return -ERANGE;
--
1.9.1

2017-10-28 15:26:04

by Lior David

[permalink] [raw]
Subject: Re: [PATCH 03/12] wil6210: refresh FW capabilities during interface up



On 10/28/2017 6:01 PM, Kalle Valo wrote:
> Maya Erez <[email protected]> writes:
>
>> From: Lior David <[email protected]>
>>
>> FW capabilities are currently retrieved only during module
>> initialization, but userspace can replace the firmware while
>> interface is down, so refresh the FW capabilities when
>> interface is up (after FW is loaded) to ensure driver
>> functionality matches the loaded FW.
>
> I think usually the firmware is loaded only once during probe() and I
> think it's quite special that you retrieve it during interface up. Being
> able to change the firmware version runtime like that can lead to
> problems eventually, for example cfg80211 might not allow changing
> already registered configuration etc.
>
Yes you are right, it is not perfect.
We shutdown our chip in interface down so the FW is lost, we have to load it
every interface up. We could request_firmware only once at insmod and store it
so the same FW will be used every time but this is a big waste of memory (FW
size is ~400KB and may be larger with future chips). We only touch one or two
very simple fields in struct wiphy that are not validated in wiphy_register. The
behavior is not 100% proof but good enough, and we recommend to our users to
always rmmod/insmod when replacing FW (replacing FW at runtime is usually not
done in production systems, this is only for debugging).
However, if we do not refresh the capabilities we will have larger problems -
the driver can try to access features not supported by FW and cause FW/host crashes.