[PATCH 00/10] staging: wfx: usual maintenance

[PATCH 00/10] staging: wfx: usual maintenance

[Jerome Pouiller]

From: Jérôme Pouiller <jerome.pouiller@silabs.com>

This PR contains fixes for various small defects I have seen these last
weeks.

Jérôme Pouiller (10):
  staging: wfx: sta.o was linked twice
  staging: wfx: fix struct alignment
  staging: wfx: format comments on 100 columns
  staging: wfx: format code on 100 columns
  staging: wfx: remove useless variable
  staging: wfx: drop useless include
  staging: wfx: remove duplicated code in wfx_cmd_send()
  staging: wfx: prefer to wait for an event instead to sleep
  staging: wfx: ensure HIF request has been sent before polling
  staging: wfx: flags for SPI IRQ were ignored

 drivers/staging/wfx/Makefile      |  1 -
 drivers/staging/wfx/bh.c          |  1 +
 drivers/staging/wfx/bus_spi.c     | 30 +++++++++++++++---------------
 drivers/staging/wfx/data_tx.c     |  8 ++------
 drivers/staging/wfx/fwio.c        |  3 +--
 drivers/staging/wfx/hif_api_cmd.h |  2 --
 drivers/staging/wfx/hif_rx.c      |  3 +--
 drivers/staging/wfx/hif_tx.c      | 13 ++++++-------
 drivers/staging/wfx/hif_tx_mib.c  |  3 +--
 drivers/staging/wfx/hwio.c        |  9 +++------
 drivers/staging/wfx/queue.c       |  9 ++++-----
 drivers/staging/wfx/sta.c         | 11 +++++------
 drivers/staging/wfx/sta.h         |  3 +--
 13 files changed, 40 insertions(+), 56 deletions(-)

-- 
2.34.1

[PATCH 01/10] staging: wfx: sta.o was linked twice

[Jerome Pouiller]

From: Jérôme Pouiller <jerome.pouiller@silabs.com>

sta.o was listed twice in the Makefile.

Signed-off-by: Jérôme Pouiller <jerome.pouiller@silabs.com>
---
 drivers/staging/wfx/Makefile | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/staging/wfx/Makefile b/drivers/staging/wfx/Makefile
index ae94c6552d77..c8b356f71c99 100644
--- a/drivers/staging/wfx/Makefile
+++ b/drivers/staging/wfx/Makefile
@@ -17,7 +17,6 @@ wfx-y := \
 	sta.o \
 	key.o \
 	main.o \
-	sta.o \
 	debug.o
 wfx-$(CONFIG_SPI) += bus_spi.o
 # When CONFIG_MMC == m, append to 'wfx-y' (and not to 'wfx-m')
-- 
2.34.1

[PATCH 02/10] staging: wfx: fix struct alignment

[Jerome Pouiller]

From: Jérôme Pouiller <jerome.pouiller@silabs.com>

There is no reason to add multiple spaces between a variable name and
its type.

Signed-off-by: Jérôme Pouiller <jerome.pouiller@silabs.com>
---
 drivers/staging/wfx/bus_spi.c | 28 ++++++++++++++--------------
 1 file changed, 14 insertions(+), 14 deletions(-)

diff --git a/drivers/staging/wfx/bus_spi.c b/drivers/staging/wfx/bus_spi.c
index 062826aa7e6c..a0a98c074cb5 100644
--- a/drivers/staging/wfx/bus_spi.c
+++ b/drivers/staging/wfx/bus_spi.c
@@ -65,14 +65,14 @@ static int wfx_spi_copy_from_io(void *priv, unsigned int addr, void *dst, size_t
 {
 	struct wfx_spi_priv *bus = priv;
 	u16 regaddr = (addr << 12) | (count / 2) | SET_READ;
-	struct spi_message      m;
-	struct spi_transfer     t_addr = {
-		.tx_buf         = &regaddr,
-		.len            = sizeof(regaddr),
+	struct spi_message m;
+	struct spi_transfer t_addr = {
+		.tx_buf = &regaddr,
+		.len = sizeof(regaddr),
 	};
-	struct spi_transfer     t_msg = {
-		.rx_buf         = dst,
-		.len            = count,
+	struct spi_transfer t_msg = {
+		.rx_buf = dst,
+		.len = count,
 	};
 	u16 *dst16 = dst;
 	int ret, i;
@@ -101,14 +101,14 @@ static int wfx_spi_copy_to_io(void *priv, unsigned int addr, const void *src, si
 	/* FIXME: use a bounce buffer */
 	u16 *src16 = (void *)src;
 	int ret, i;
-	struct spi_message      m;
-	struct spi_transfer     t_addr = {
-		.tx_buf         = &regaddr,
-		.len            = sizeof(regaddr),
+	struct spi_message m;
+	struct spi_transfer t_addr = {
+		.tx_buf = &regaddr,
+		.len = sizeof(regaddr),
 	};
-	struct spi_transfer     t_msg = {
-		.tx_buf         = src,
-		.len            = count,
+	struct spi_transfer t_msg = {
+		.tx_buf = src,
+		.len = count,
 	};
 
 	WARN(count % 2, "buffer size must be a multiple of 2");
-- 
2.34.1

[PATCH 03/10] staging: wfx: format comments on 100 columns

[Jerome Pouiller]

From: Jérôme Pouiller <jerome.pouiller@silabs.com>

A few comments were not yet formatted on 100 columns.

Signed-off-by: Jérôme Pouiller <jerome.pouiller@silabs.com>
---
 drivers/staging/wfx/data_tx.c | 8 ++------
 drivers/staging/wfx/queue.c   | 9 ++++-----
 2 files changed, 6 insertions(+), 11 deletions(-)

diff --git a/drivers/staging/wfx/data_tx.c b/drivers/staging/wfx/data_tx.c
index d7bcf3bae08a..e07381b2ff4d 100644
--- a/drivers/staging/wfx/data_tx.c
+++ b/drivers/staging/wfx/data_tx.c
@@ -117,9 +117,7 @@ static int wfx_tx_policy_get(struct wfx_vif *wvif, struct ieee80211_tx_rate *rat
 	if (idx >= 0) {
 		*renew = false;
 	} else {
-		/* If policy is not found create a new one using the oldest
-		 * entry in "free" list
-		 */
+		/* If policy is not found create a new one using the oldest entry in "free" list */
 		*renew = true;
 		entry = list_entry(cache->free.prev, struct wfx_tx_policy, link);
 		memcpy(entry->rates, wanted.rates, sizeof(entry->rates));
@@ -494,9 +492,7 @@ void wfx_tx_confirm_cb(struct wfx_dev *wdev, const struct wfx_hif_cnf_tx *arg)
 	wfx_tx_fill_rates(wdev, tx_info, arg);
 	skb_trim(skb, skb->len - tx_priv->icv_size);
 
-	/* From now, you can touch to tx_info->status, but do not touch to
-	 * tx_priv anymore
-	 */
+	/* From now, you can touch to tx_info->status, but do not touch to tx_priv anymore */
 	/* FIXME: use ieee80211_tx_info_clear_status() */
 	memset(tx_info->rate_driver_data, 0, sizeof(tx_info->rate_driver_data));
 	memset(tx_info->pad, 0, sizeof(tx_info->pad));
diff --git a/drivers/staging/wfx/queue.c b/drivers/staging/wfx/queue.c
index 9186726ff07f..729825230db2 100644
--- a/drivers/staging/wfx/queue.c
+++ b/drivers/staging/wfx/queue.c
@@ -210,8 +210,8 @@ bool wfx_tx_queues_has_cab(struct wfx_vif *wvif)
 	if (wvif->vif->type != NL80211_IFTYPE_AP)
 		return false;
 	for (i = 0; i < IEEE80211_NUM_ACS; ++i)
-		/* Note: since only AP can have mcast frames in queue and only
-		 * one vif can be AP, all queued frames has same interface id
+		/* Note: since only AP can have mcast frames in queue and only one vif can be AP,
+		 * all queued frames has same interface id
 		 */
 		if (!skb_queue_empty_lockless(&wvif->tx_queue[i].cab))
 			return true;
@@ -253,9 +253,8 @@ static struct sk_buff *wfx_tx_queues_get_skb(struct wfx_dev *wdev)
 			skb = skb_dequeue(&queues[i]->cab);
 			if (!skb)
 				continue;
-			/* Note: since only AP can have mcast frames in queue
-			 * and only one vif can be AP, all queued frames has
-			 * same interface id
+			/* Note: since only AP can have mcast frames in queue and only one vif can
+			 * be AP, all queued frames has same interface id
 			 */
 			hif = (struct wfx_hif_msg *)skb->data;
 			WARN_ON(hif->interface != wvif->id);
-- 
2.34.1

[PATCH 04/10] staging: wfx: format code on 100 columns

[Jerome Pouiller]

From: Jérôme Pouiller <jerome.pouiller@silabs.com>

A few lines were not yet formatted on 100 columns.

Signed-off-by: Jérôme Pouiller <jerome.pouiller@silabs.com>
---
 drivers/staging/wfx/fwio.c       | 3 +--
 drivers/staging/wfx/hif_rx.c     | 3 +--
 drivers/staging/wfx/hif_tx.c     | 3 +--
 drivers/staging/wfx/hif_tx_mib.c | 3 +--
 drivers/staging/wfx/hwio.c       | 9 +++------
 drivers/staging/wfx/sta.c        | 7 +++----
 drivers/staging/wfx/sta.h        | 3 +--
 7 files changed, 11 insertions(+), 20 deletions(-)

diff --git a/drivers/staging/wfx/fwio.c b/drivers/staging/wfx/fwio.c
index 9005a6fe48c8..3d1b8a135dc0 100644
--- a/drivers/staging/wfx/fwio.c
+++ b/drivers/staging/wfx/fwio.c
@@ -192,8 +192,7 @@ static int upload_firmware(struct wfx_dev *wdev, const u8 *data, size_t len)
 				return ret;
 		}
 		if (ktime_compare(now, start))
-			dev_dbg(wdev->dev, "answer after %lldus\n",
-				ktime_us_delta(now, start));
+			dev_dbg(wdev->dev, "answer after %lldus\n", ktime_us_delta(now, start));
 
 		ret = wfx_sram_write_dma_safe(wdev, WFX_DNLD_FIFO + (offs % DNLD_FIFO_SIZE),
 					      data + offs, DNLD_BLOCK_SIZE);
diff --git a/drivers/staging/wfx/hif_rx.c b/drivers/staging/wfx/hif_rx.c
index 302bdb2bf036..64ca8acb8e4f 100644
--- a/drivers/staging/wfx/hif_rx.c
+++ b/drivers/staging/wfx/hif_rx.c
@@ -244,8 +244,7 @@ static int wfx_hif_generic_indication(struct wfx_dev *wdev,
 		mutex_unlock(&wdev->tx_power_loop_info_lock);
 		return 0;
 	default:
-		dev_err(wdev->dev, "generic_indication: unknown indication type: %#.8x\n",
-			type);
+		dev_err(wdev->dev, "generic_indication: unknown indication type: %#.8x\n", type);
 		return -EIO;
 	}
 }
diff --git a/drivers/staging/wfx/hif_tx.c b/drivers/staging/wfx/hif_tx.c
index ea2582714bb9..236f9d62e3a9 100644
--- a/drivers/staging/wfx/hif_tx.c
+++ b/drivers/staging/wfx/hif_tx.c
@@ -169,8 +169,7 @@ int wfx_hif_reset(struct wfx_vif *wvif, bool reset_stat)
 	return ret;
 }
 
-int wfx_hif_read_mib(struct wfx_dev *wdev, int vif_id, u16 mib_id,
-		     void *val, size_t val_len)
+int wfx_hif_read_mib(struct wfx_dev *wdev, int vif_id, u16 mib_id, void *val, size_t val_len)
 {
 	int ret;
 	struct wfx_hif_msg *hif;
diff --git a/drivers/staging/wfx/hif_tx_mib.c b/drivers/staging/wfx/hif_tx_mib.c
index 1c57dd2b697c..df1bcb1e2c02 100644
--- a/drivers/staging/wfx/hif_tx_mib.c
+++ b/drivers/staging/wfx/hif_tx_mib.c
@@ -84,8 +84,7 @@ int wfx_hif_set_macaddr(struct wfx_vif *wvif, u8 *mac)
 				 &arg, sizeof(arg));
 }
 
-int wfx_hif_set_rx_filter(struct wfx_vif *wvif,
-			  bool filter_bssid, bool filter_prbreq)
+int wfx_hif_set_rx_filter(struct wfx_vif *wvif, bool filter_bssid, bool filter_prbreq)
 {
 	struct wfx_hif_mib_rx_filter arg = { };
 
diff --git a/drivers/staging/wfx/hwio.c b/drivers/staging/wfx/hwio.c
index c15810bdaecb..3f9750b470be 100644
--- a/drivers/staging/wfx/hwio.c
+++ b/drivers/staging/wfx/hwio.c
@@ -71,8 +71,7 @@ static int wfx_write32_locked(struct wfx_dev *wdev, int reg, u32 val)
 	return ret;
 }
 
-static int wfx_write32_bits_locked(struct wfx_dev *wdev,
-				   int reg, u32 mask, u32 val)
+static int wfx_write32_bits_locked(struct wfx_dev *wdev, int reg, u32 mask, u32 val)
 {
 	int ret;
 	u32 val_r, val_w;
@@ -94,8 +93,7 @@ static int wfx_write32_bits_locked(struct wfx_dev *wdev,
 	return ret;
 }
 
-static int wfx_indirect_read(struct wfx_dev *wdev, int reg, u32 addr,
-			     void *buf, size_t len)
+static int wfx_indirect_read(struct wfx_dev *wdev, int reg, u32 addr, void *buf, size_t len)
 {
 	int ret;
 	int i;
@@ -199,8 +197,7 @@ static int wfx_indirect_read32_locked(struct wfx_dev *wdev, int reg, u32 addr, u
 	return ret;
 }
 
-static int wfx_indirect_write32_locked(struct wfx_dev *wdev, int reg,
-				       u32 addr, u32 val)
+static int wfx_indirect_write32_locked(struct wfx_dev *wdev, int reg, u32 addr, u32 val)
 {
 	int ret;
 	__le32 *tmp = kmalloc(sizeof(u32), GFP_KERNEL);
diff --git a/drivers/staging/wfx/sta.c b/drivers/staging/wfx/sta.c
index ba53e4d70c4f..28474614a8e5 100644
--- a/drivers/staging/wfx/sta.c
+++ b/drivers/staging/wfx/sta.c
@@ -432,8 +432,7 @@ static void wfx_join(struct wfx_vif *wvif)
 	wfx_tx_unlock(wvif->wdev);
 }
 
-static void wfx_join_finalize(struct wfx_vif *wvif,
-			      struct ieee80211_bss_conf *info)
+static void wfx_join_finalize(struct wfx_vif *wvif, struct ieee80211_bss_conf *info)
 {
 	struct ieee80211_sta *sta = NULL;
 	int ampdu_density = 0;
@@ -539,8 +538,8 @@ void wfx_bss_info_changed(struct ieee80211_hw *hw, struct ieee80211_vif *vif,
 		wfx_enable_beacon(wvif, info->enable_beacon);
 
 	if (changed & BSS_CHANGED_KEEP_ALIVE)
-		wfx_hif_keep_alive_period(wvif, info->max_idle_period *
-						USEC_PER_TU / USEC_PER_MSEC);
+		wfx_hif_keep_alive_period(wvif,
+					  info->max_idle_period * USEC_PER_TU / USEC_PER_MSEC);
 
 	if (changed & BSS_CHANGED_ERP_CTS_PROT)
 		wfx_hif_erp_use_protection(wvif, info->use_cts_prot);
diff --git a/drivers/staging/wfx/sta.h b/drivers/staging/wfx/sta.h
index 082329d7bbcd..c69b2227e9ac 100644
--- a/drivers/staging/wfx/sta.h
+++ b/drivers/staging/wfx/sta.h
@@ -46,8 +46,7 @@ int wfx_ampdu_action(struct ieee80211_hw *hw, struct ieee80211_vif *vif,
 		     struct ieee80211_ampdu_params *params);
 int wfx_add_chanctx(struct ieee80211_hw *hw, struct ieee80211_chanctx_conf *conf);
 void wfx_remove_chanctx(struct ieee80211_hw *hw, struct ieee80211_chanctx_conf *conf);
-void wfx_change_chanctx(struct ieee80211_hw *hw, struct ieee80211_chanctx_conf *conf,
-			u32 changed);
+void wfx_change_chanctx(struct ieee80211_hw *hw, struct ieee80211_chanctx_conf *conf, u32 changed);
 int wfx_assign_vif_chanctx(struct ieee80211_hw *hw, struct ieee80211_vif *vif,
 			   struct ieee80211_chanctx_conf *conf);
 void wfx_unassign_vif_chanctx(struct ieee80211_hw *hw, struct ieee80211_vif *vif,
-- 
2.34.1

[PATCH 05/10] staging: wfx: remove useless variable

[Jerome Pouiller]

From: Jérôme Pouiller <jerome.pouiller@silabs.com>

Obviously, the variable "ret" was useless.

Signed-off-by: Jérôme Pouiller <jerome.pouiller@silabs.com>
---
 drivers/staging/wfx/sta.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/wfx/sta.c b/drivers/staging/wfx/sta.c
index 28474614a8e5..b1e9fb14d2b4 100644
--- a/drivers/staging/wfx/sta.c
+++ b/drivers/staging/wfx/sta.c
@@ -687,7 +687,7 @@ int wfx_config(struct ieee80211_hw *hw, u32 changed)
 
 int wfx_add_interface(struct ieee80211_hw *hw, struct ieee80211_vif *vif)
 {
-	int i, ret = 0;
+	int i;
 	struct wfx_dev *wdev = hw->priv;
 	struct wfx_vif *wvif = (struct wfx_vif *)vif->drv_priv;
 
@@ -747,7 +747,7 @@ int wfx_add_interface(struct ieee80211_hw *hw, struct ieee80211_vif *vif)
 		else
 			wfx_hif_set_block_ack_policy(wvif, 0x00, 0x00);
 	}
-	return ret;
+	return 0;
 }
 
 void wfx_remove_interface(struct ieee80211_hw *hw, struct ieee80211_vif *vif)
-- 
2.34.1

[PATCH 06/10] staging: wfx: drop useless include

[Jerome Pouiller]

From: Jérôme Pouiller <jerome.pouiller@silabs.com>

ieee80211.h is useless since commit 5e911c3d9dbc9 ("staging: wfx: avoid
defining array of flexible struct")

Signed-off-by: Jérôme Pouiller <jerome.pouiller@silabs.com>
---
 drivers/staging/wfx/hif_api_cmd.h | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/staging/wfx/hif_api_cmd.h b/drivers/staging/wfx/hif_api_cmd.h
index 444ff7ed882d..8b91b1d4a46b 100644
--- a/drivers/staging/wfx/hif_api_cmd.h
+++ b/drivers/staging/wfx/hif_api_cmd.h
@@ -8,8 +8,6 @@
 #ifndef WFX_HIF_API_CMD_H
 #define WFX_HIF_API_CMD_H
 
-#include <linux/ieee80211.h>
-
 #include "hif_api_general.h"
 
 enum wfx_hif_requests_ids {
-- 
2.34.1

[PATCH 07/10] staging: wfx: remove duplicated code in wfx_cmd_send()

[Jerome Pouiller]

From: Jérôme Pouiller <jerome.pouiller@silabs.com>

The code to execute on end of the function is the same whatever the
command replies or not.

Signed-off-by: Jérôme Pouiller <jerome.pouiller@silabs.com>
---
 drivers/staging/wfx/hif_tx.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/wfx/hif_tx.c b/drivers/staging/wfx/hif_tx.c
index 236f9d62e3a9..0b1ed12c0e83 100644
--- a/drivers/staging/wfx/hif_tx.c
+++ b/drivers/staging/wfx/hif_tx.c
@@ -74,9 +74,8 @@ int wfx_cmd_send(struct wfx_dev *wdev, struct wfx_hif_msg *request,
 	if (no_reply) {
 		/* Chip won't reply. Give enough time to the wq to send the buffer. */
 		msleep(100);
-		wdev->hif_cmd.buf_send = NULL;
-		mutex_unlock(&wdev->hif_cmd.lock);
-		return 0;
+		ret = 0;
+		goto end;
 	}
 
 	if (wdev->poll_irq)
@@ -98,6 +97,7 @@ int wfx_cmd_send(struct wfx_dev *wdev, struct wfx_hif_msg *request,
 		ret = wdev->hif_cmd.ret;
 	}
 
+end:
 	wdev->hif_cmd.buf_send = NULL;
 	mutex_unlock(&wdev->hif_cmd.lock);
 
-- 
2.34.1

[PATCH 08/10] staging: wfx: prefer to wait for an event instead to sleep

[Jerome Pouiller]

From: Jérôme Pouiller <jerome.pouiller@silabs.com>

When possible it is better to wait for an explicit event instead of wait
an arbitrary amount of time.

Signed-off-by: Jérôme Pouiller <jerome.pouiller@silabs.com>
---
 drivers/staging/wfx/hif_tx.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/wfx/hif_tx.c b/drivers/staging/wfx/hif_tx.c
index 0b1ed12c0e83..ae3cc5919dcd 100644
--- a/drivers/staging/wfx/hif_tx.c
+++ b/drivers/staging/wfx/hif_tx.c
@@ -72,8 +72,8 @@ int wfx_cmd_send(struct wfx_dev *wdev, struct wfx_hif_msg *request,
 	wfx_bh_request_tx(wdev);
 
 	if (no_reply) {
-		/* Chip won't reply. Give enough time to the wq to send the buffer. */
-		msleep(100);
+		/* Chip won't reply. Ensure the wq has send the buffer before to continue. */
+		flush_workqueue(system_highpri_wq);
 		ret = 0;
 		goto end;
 	}
-- 
2.34.1

[PATCH 09/10] staging: wfx: ensure HIF request has been sent before polling

[Jerome Pouiller]

From: Jérôme Pouiller <jerome.pouiller@silabs.com>

wfx_bh_request_tx() send HIF request asynchronously through bh_work().
Then the caller will run wfx_bh_poll_irq() to poll the answer.

However it useless to burn CPU cycles for the polling while the request
has yet been sent. Worse, wfx_bh_poll_irq() may get the CPU and prevent
wfx_bh_request_tx() to run. This problem has been observed on mono core
architecture.

This first exchange is correct:
    kworker/u2:1-24    [000] ....    : io_read32: CONTROL: 00003000
    kworker/u2:1-24    [000] ....    : io_read32: CONTROL: 00003000
    kworker/u2:1-24    [000] ....    : io_read32: CONTROL: 00003004
    kworker/u2:1-24    [000] ....    : io_read32: CONTROL: 00003004
    kworker/0:1H-40    [000] ....    : io_read: QUEUE: 08 00 09 0c 00 00 00 00 3a 7b 00 30 (12 bytes)
    kworker/0:1H-40    [000] ....    : piggyback: CONTROL: 00003000
    kworker/0:1H-40    [000] ....    : hif_recv: 0:2:CNF_CONFIGURATION: 00 00 00 00 (8 bytes)
    kworker/0:1H-40    [000] ....    : io_read32: CONFIG: 03010200
    kworker/0:1H-40    [000] ....    : bh_stats: IND/REQ/CNF:  0/  0/  1, REQ in progress:  0, WUP: release

... while the following is not:
    kworker/u2:1-24    [000] ....    : io_read32: CONTROL: 00003000
    kworker/u2:1-24    [000] ....    : io_read32: CONTROL: 00003000
    kworker/u2:1-24    [000] ....    : io_read32: CONTROL: 00003000
    [...loop until timeout...]
    wfx-sdio mmc0:0001:1: time out while polling control register

Signed-off-by: Jérôme Pouiller <jerome.pouiller@silabs.com>
---
 drivers/staging/wfx/bh.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/staging/wfx/bh.c b/drivers/staging/wfx/bh.c
index 4c6ba9c342a6..bcea9d5b119c 100644
--- a/drivers/staging/wfx/bh.c
+++ b/drivers/staging/wfx/bh.c
@@ -295,6 +295,7 @@ void wfx_bh_poll_irq(struct wfx_dev *wdev)
 	u32 reg;
 
 	WARN(!wdev->poll_irq, "unexpected IRQ polling can mask IRQ");
+	flush_workqueue(system_highpri_wq);
 	start = ktime_get();
 	for (;;) {
 		wfx_control_reg_read(wdev, &reg);
-- 
2.34.1

[PATCH 10/10] staging: wfx: flags for SPI IRQ were ignored

[Jerome Pouiller]

From: Jérôme Pouiller <jerome.pouiller@silabs.com>

The flags declared in the DT were not forwarded to request_irq().

Fixes: a7efb62509d8 ("staging: wfx: use threaded IRQ with SPI")
Signed-off-by: Jérôme Pouiller <jerome.pouiller@silabs.com>
---
 drivers/staging/wfx/bus_spi.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/wfx/bus_spi.c b/drivers/staging/wfx/bus_spi.c
index a0a98c074cb5..bb31f8a005bf 100644
--- a/drivers/staging/wfx/bus_spi.c
+++ b/drivers/staging/wfx/bus_spi.c
@@ -162,7 +162,7 @@ static int wfx_spi_irq_subscribe(void *priv)
 		flags = IRQF_TRIGGER_HIGH;
 	flags |= IRQF_ONESHOT;
 	return devm_request_threaded_irq(&bus->func->dev, bus->func->irq, NULL,
-					 wfx_spi_irq_handler, IRQF_ONESHOT, "wfx", bus);
+					 wfx_spi_irq_handler, flags, "wfx", bus);
 }
 
 static int wfx_spi_irq_unsubscribe(void *priv)
-- 
2.34.1

Re: [PATCH 03/10] staging: wfx: format comments on 100 columns

[Joe Perches]

On Fri, 2022-02-25 at 12:23 +0100, Jerome Pouiller wrote:
> From: Jérôme Pouiller <jerome.pouiller@silabs.com>
> 
> A few comments were not yet formatted on 100 columns.

IMO, none of these changes are necessary or good changes.

80 columns is preferred.

Really comments should most always use 80 columns, and
only occasionally should code be more than 80 columns
and almost never should code be more than 100 columns.

> diff --git a/drivers/staging/wfx/data_tx.c b/drivers/staging/wfx/data_tx.c
[]
> @@ -117,9 +117,7 @@ static int wfx_tx_policy_get(struct wfx_vif *wvif, struct ieee80211_tx_rate *rat
>  	if (idx >= 0) {
>  		*renew = false;
>  	} else {
> -		/* If policy is not found create a new one using the oldest
> -		 * entry in "free" list
> -		 */
> +		/* If policy is not found create a new one using the oldest entry in "free" list */
>  		*renew = true;
>  		entry = list_entry(cache->free.prev, struct wfx_tx_policy, link);
>  		memcpy(entry->rates, wanted.rates, sizeof(entry->rates));
> @@ -494,9 +492,7 @@ void wfx_tx_confirm_cb(struct wfx_dev *wdev, const struct wfx_hif_cnf_tx *arg)
>  	wfx_tx_fill_rates(wdev, tx_info, arg);
>  	skb_trim(skb, skb->len - tx_priv->icv_size);
>  
> -	/* From now, you can touch to tx_info->status, but do not touch to
> -	 * tx_priv anymore
> -	 */
> +	/* From now, you can touch to tx_info->status, but do not touch to tx_priv anymore */
>  	/* FIXME: use ieee80211_tx_info_clear_status() */
>  	memset(tx_info->rate_driver_data, 0, sizeof(tx_info->rate_driver_data));
>  	memset(tx_info->pad, 0, sizeof(tx_info->pad));
> diff --git a/drivers/staging/wfx/queue.c b/drivers/staging/wfx/queue.c
[]
> @@ -210,8 +210,8 @@ bool wfx_tx_queues_has_cab(struct wfx_vif *wvif)
>  	if (wvif->vif->type != NL80211_IFTYPE_AP)
>  		return false;
>  	for (i = 0; i < IEEE80211_NUM_ACS; ++i)
> -		/* Note: since only AP can have mcast frames in queue and only
> -		 * one vif can be AP, all queued frames has same interface id
> +		/* Note: since only AP can have mcast frames in queue and only one vif can be AP,
> +		 * all queued frames has same interface id
>  		 */
>  		if (!skb_queue_empty_lockless(&wvif->tx_queue[i].cab))
>  			return true;
> @@ -253,9 +253,8 @@ static struct sk_buff *wfx_tx_queues_get_skb(struct wfx_dev *wdev)
>  			skb = skb_dequeue(&queues[i]->cab);
>  			if (!skb)
>  				continue;
> -			/* Note: since only AP can have mcast frames in queue
> -			 * and only one vif can be AP, all queued frames has
> -			 * same interface id
> +			/* Note: since only AP can have mcast frames in queue and only one vif can
> +			 * be AP, all queued frames has same interface id
>  			 */
>  			hif = (struct wfx_hif_msg *)skb->data;
>  			WARN_ON(hif->interface != wvif->id);

Re: [PATCH 03/10] staging: wfx: format comments on 100 columns

[Jeff Johnson]

On 2/28/2022 5:12 PM, Joe Perches wrote:
> On Fri, 2022-02-25 at 12:23 +0100, Jerome Pouiller wrote:
>> From: Jérôme Pouiller <jerome.pouiller@silabs.com>
>>
>> A few comments were not yet formatted on 100 columns.
> 
> IMO, none of these changes are necessary or good changes.
> 
> 80 columns is preferred.
> 
> Really comments should most always use 80 columns, and
> only occasionally should code be more than 80 columns
> and almost never should code be more than 100 columns.

That was my reaction as well. Just because we've relaxed rules so that 
we *can* exceed 80 columns, it doesn't mean we *should*, and definitely 
doesn't mean we should *strive* to do so.

Re: [PATCH 09/10] staging: wfx: ensure HIF request has been sent before polling

[Tetsuo Handa]

Hello.

I found that commit c86176d51340d5ee ("staging: wfx: ensure HIF request has
been sent before polling") started using flush_workqueue(system_highpri_wq).

But please avoid flushing system-wide workqueues, for
flushing system-wide workqueues is dangerous and will be forbidden.

Link: https://lkml.kernel.org/r/49925af7-78a8-a3dd-bce6-cfc02e1a9236@I-love.SAKURA.ne.jp

Re: [PATCH 09/10] staging: wfx: ensure HIF request has been sent before polling

[Jérôme Pouiller]

On Tuesday 12 April 2022 15:11:53 CEST Tetsuo Handa wrote:
> Hello.
> 
> I found that commit c86176d51340d5ee ("staging: wfx: ensure HIF request has
> been sent before polling") started using flush_workqueue(system_highpri_wq).
> 
> But please avoid flushing system-wide workqueues, for
> flushing system-wide workqueues is dangerous and will be forbidden.
> 
> Link: https://lkml.kernel.org/r/49925af7-78a8-a3dd-bce6-cfc02e1a9236@I-love.SAKURA.ne.jp
> 
> 

There is a workqueue provided by mac80211. But I am not sure it can 
replace the system-wide workqueue.

I guess I need to declare a new workqueue. Give me a couple of weeks to
prepare a patch.


-- 
Jérôme Pouiller

[PATCH] wfx: avoid flush_workqueue(system_highpri_wq) usage

[Tetsuo Handa]

Flushing system-wide workqueues is dangerous and will be forbidden.
Replace system_highpri_wq with local wfx_wq.

While we are at it, add missing spi_unregister_driver() call when
sdio_register_driver() failed.

Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
---
Note: This patch is only compile tested.

 drivers/net/wireless/silabs/wfx/bh.c     |  6 +++---
 drivers/net/wireless/silabs/wfx/hif_tx.c |  2 +-
 drivers/net/wireless/silabs/wfx/main.c   | 11 +++++++++++
 drivers/net/wireless/silabs/wfx/wfx.h    |  2 ++
 4 files changed, 17 insertions(+), 4 deletions(-)

diff --git a/drivers/net/wireless/silabs/wfx/bh.c b/drivers/net/wireless/silabs/wfx/bh.c
index bcea9d5b119c..377dd8b010e2 100644
--- a/drivers/net/wireless/silabs/wfx/bh.c
+++ b/drivers/net/wireless/silabs/wfx/bh.c
@@ -267,7 +267,7 @@ void wfx_bh_request_rx(struct wfx_dev *wdev)
 	wfx_control_reg_read(wdev, &cur);
 	prev = atomic_xchg(&wdev->hif.ctrl_reg, cur);
 	complete(&wdev->hif.ctrl_ready);
-	queue_work(system_highpri_wq, &wdev->hif.bh);
+	queue_work(wfx_wq, &wdev->hif.bh);
 
 	if (!(cur & CTRL_NEXT_LEN_MASK))
 		dev_err(wdev->dev, "unexpected control register value: length field is 0: %04x\n",
@@ -280,7 +280,7 @@ void wfx_bh_request_rx(struct wfx_dev *wdev)
 /* Driver want to send data */
 void wfx_bh_request_tx(struct wfx_dev *wdev)
 {
-	queue_work(system_highpri_wq, &wdev->hif.bh);
+	queue_work(wfx_wq, &wdev->hif.bh);
 }
 
 /* If IRQ is not available, this function allow to manually poll the control register and simulate
@@ -295,7 +295,7 @@ void wfx_bh_poll_irq(struct wfx_dev *wdev)
 	u32 reg;
 
 	WARN(!wdev->poll_irq, "unexpected IRQ polling can mask IRQ");
-	flush_workqueue(system_highpri_wq);
+	flush_workqueue(wfx_wq);
 	start = ktime_get();
 	for (;;) {
 		wfx_control_reg_read(wdev, &reg);
diff --git a/drivers/net/wireless/silabs/wfx/hif_tx.c b/drivers/net/wireless/silabs/wfx/hif_tx.c
index 9c653d0e9034..85c68b1b11c4 100644
--- a/drivers/net/wireless/silabs/wfx/hif_tx.c
+++ b/drivers/net/wireless/silabs/wfx/hif_tx.c
@@ -73,7 +73,7 @@ int wfx_cmd_send(struct wfx_dev *wdev, struct wfx_hif_msg *request,
 
 	if (no_reply) {
 		/* Chip won't reply. Ensure the wq has send the buffer before to continue. */
-		flush_workqueue(system_highpri_wq);
+		flush_workqueue(wfx_wq);
 		ret = 0;
 		goto end;
 	}
diff --git a/drivers/net/wireless/silabs/wfx/main.c b/drivers/net/wireless/silabs/wfx/main.c
index e575a81ca2ca..d3d18c498dd0 100644
--- a/drivers/net/wireless/silabs/wfx/main.c
+++ b/drivers/net/wireless/silabs/wfx/main.c
@@ -40,6 +40,8 @@ MODULE_DESCRIPTION("Silicon Labs 802.11 Wireless LAN driver for WF200");
 MODULE_AUTHOR("J辿r担me Pouiller <jerome.pouiller@silabs.com>");
 MODULE_LICENSE("GPL");
 
+struct workqueue_struct *wfx_wq;
+
 #define RATETAB_ENT(_rate, _rateid, _flags) { \
 	.bitrate  = (_rate),   \
 	.hw_value = (_rateid), \
@@ -473,10 +475,18 @@ static int __init wfx_core_init(void)
 {
 	int ret = 0;
 
+	wfx_wq = alloc_workqueue("wfx_wq", WQ_HIGHPRI, 0);
+	if (!wfx_wq)
+		return -ENOMEM;
 	if (IS_ENABLED(CONFIG_SPI))
 		ret = spi_register_driver(&wfx_spi_driver);
 	if (IS_ENABLED(CONFIG_MMC) && !ret)
 		ret = sdio_register_driver(&wfx_sdio_driver);
+	if (ret) {
+		if (IS_ENABLED(CONFIG_SPI))
+			spi_unregister_driver(&wfx_spi_driver);
+		destroy_workqueue(wfx_wq);
+	}
 	return ret;
 }
 module_init(wfx_core_init);
@@ -487,5 +497,6 @@ static void __exit wfx_core_exit(void)
 		sdio_unregister_driver(&wfx_sdio_driver);
 	if (IS_ENABLED(CONFIG_SPI))
 		spi_unregister_driver(&wfx_spi_driver);
+	destroy_workqueue(wfx_wq);
 }
 module_exit(wfx_core_exit);
diff --git a/drivers/net/wireless/silabs/wfx/wfx.h b/drivers/net/wireless/silabs/wfx/wfx.h
index 6594cc647c2f..ea0ba002eb1f 100644
--- a/drivers/net/wireless/silabs/wfx/wfx.h
+++ b/drivers/net/wireless/silabs/wfx/wfx.h
@@ -159,4 +159,6 @@ static inline int memzcmp(void *src, unsigned int size)
 	return memcmp(buf, buf + 1, size - 1);
 }
 
+extern struct workqueue_struct *wfx_wq;
+
 #endif
-- 
2.34.1

Re: [PATCH] wfx: avoid flush_workqueue(system_highpri_wq) usage

[Kalle Valo]

Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> writes:

> Flushing system-wide workqueues is dangerous and will be forbidden.
> Replace system_highpri_wq with local wfx_wq.
>
> While we are at it, add missing spi_unregister_driver() call when
> sdio_register_driver() failed.
>
> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>

[...]

> @@ -473,10 +475,18 @@ static int __init wfx_core_init(void)
>  {
>  	int ret = 0;
>  
> +	wfx_wq = alloc_workqueue("wfx_wq", WQ_HIGHPRI, 0);
> +	if (!wfx_wq)
> +		return -ENOMEM;
>  	if (IS_ENABLED(CONFIG_SPI))
>  		ret = spi_register_driver(&wfx_spi_driver);
>  	if (IS_ENABLED(CONFIG_MMC) && !ret)
>  		ret = sdio_register_driver(&wfx_sdio_driver);
> +	if (ret) {
> +		if (IS_ENABLED(CONFIG_SPI))
> +			spi_unregister_driver(&wfx_spi_driver);
> +		destroy_workqueue(wfx_wq);
> +	}
>  	return ret;
>  }
>  module_init(wfx_core_init);

So now the thread is created every time the module loaded, even if
there's no device available. Also I'm not really a fan of global
variables (wfx_wq). I would rather create a workqueue per device in
wfx_probe() or use the workqueue provided by mac80211.

/**
 * ieee80211_queue_work - add work onto the mac80211 workqueue
 *
 * Drivers and mac80211 use this to add work onto the mac80211 workqueue.
 * This helper ensures drivers are not queueing work when they should not be.
 *
 * @hw: the hardware struct for the interface we are adding work for
 * @work: the work we want to add onto the mac80211 workqueue
 */
void ieee80211_queue_work(struct ieee80211_hw *hw, struct work_struct *work);

-- 
https://patchwork.kernel.org/project/linux-wireless/list/

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

Re: [PATCH] wfx: avoid flush_workqueue(system_highpri_wq) usage

[Tetsuo Handa]

On 2022/05/01 17:53, Kalle Valo wrote:
> So now the thread is created every time the module loaded, even if
> there's no device available.

Excuse me, but what thread? alloc_workqueue() without WQ_MEM_RECLAIM flag
does not create a thread, and therefore consumes little resource where
there's no device available does not matter.

>                              Also I'm not really a fan of global
> variables (wfx_wq). I would rather create a workqueue per device in
> wfx_probe() or use the workqueue provided by mac80211.

Whatever is fine. wfx is the last user of flush_workqueue(system_*_wq) and
I want to know whether I can include system_highpri_wq into below patch.

Subject: [PATCH] workqueue: wrap flush_workqueue() using a macro

While a conversion to stop flushing of kernel-global workqueues is in
progress, we can warn users who are not aware of this conversion, by
wrapping flush_workqueue() as a macro and injecting BUILD_BUG_ON() tests.

---
 include/linux/workqueue.h | 27 +++++++++++++++++++++++++++
 kernel/workqueue.c        |  1 +
 2 files changed, 28 insertions(+)

diff --git a/include/linux/workqueue.h b/include/linux/workqueue.h
index 7b13fae377e2..3eaf19262d19 100644
--- a/include/linux/workqueue.h
+++ b/include/linux/workqueue.h
@@ -654,4 +654,31 @@ int workqueue_offline_cpu(unsigned int cpu);
 void __init workqueue_init_early(void);
 void __init workqueue_init(void);
 
+/*
+ * Detect attempt to flush system-wide workqueues at compile time when possible.
+ * See https://lkml.kernel.org/r/49925af7-78a8-a3dd-bce6-cfc02e1a9236@I-love.SAKURA.ne.jp for
+ * reasons and steps for converting system-wide workqueues into local workqueues.
+ * Checks for system_wq and system_highpri_wq will be added after
+ * all in-tree users stopped flushing these workqueues.
+ */
+#define flush_workqueue(wq)						\
+({									\
+	BUILD_BUG_ON_MSG(__builtin_constant_p(&(wq) == &system_long_wq) && \
+			 &(wq) == &system_long_wq,			\
+			 "Please avoid flushing system_long_wq.");	\
+	BUILD_BUG_ON_MSG(__builtin_constant_p(&(wq) == &system_unbound_wq) && \
+			 &(wq) == &system_unbound_wq,			\
+			 "Please avoid flushing system_unbound_wq.");	\
+	BUILD_BUG_ON_MSG(__builtin_constant_p(&(wq) == &system_freezable_wq) &&	\
+			 &(wq) == &system_freezable_wq,			\
+			 "Please avoid flushing system_freezable_wq."); \
+	BUILD_BUG_ON_MSG(__builtin_constant_p(&(wq) == &system_power_efficient_wq) && \
+			 &(wq) == &system_power_efficient_wq,		\
+			 "Please avoid flushing system_power_efficient_wq."); \
+	BUILD_BUG_ON_MSG(__builtin_constant_p(&(wq) == &system_freezable_power_efficient_wq) &&	\
+			 &(wq) == &system_freezable_power_efficient_wq, \
+			 "Please avoid flushing system_freezable_power_efficient_wq."); \
+	flush_workqueue(wq);						\
+})
+
 #endif
diff --git a/kernel/workqueue.c b/kernel/workqueue.c
index b447012df177..de53813a92d2 100644
--- a/kernel/workqueue.c
+++ b/kernel/workqueue.c
@@ -2813,6 +2813,7 @@ static void warn_flush_attempt(struct workqueue_struct *wq)
  * This function sleeps until all work items which were queued on entry
  * have finished execution, but it is not livelocked by new incoming ones.
  */
+#undef flush_workqueue
 void flush_workqueue(struct workqueue_struct *wq)
 {
 	struct wq_flusher this_flusher = {
-- 
2.34.1

Re: [PATCH] wfx: avoid flush_workqueue(system_highpri_wq) usage

[Kalle Valo]

Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> writes:

> On 2022/05/01 17:53, Kalle Valo wrote:
>> So now the thread is created every time the module loaded, even if
>> there's no device available.
>
> Excuse me, but what thread?

Sorry, s/thread/workqueue/.

> alloc_workqueue() without WQ_MEM_RECLAIM flag does not create a
> thread, and therefore consumes little resource where there's no device
> available does not matter.

It still allocating memory which is not needed. To me allocating
resources during module_init is wrong.

-- 
https://patchwork.kernel.org/project/linux-wireless/list/

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

[PATCH v2] wfx: avoid flush_workqueue(system_highpri_wq) usage

[Tetsuo Handa]

Flushing system-wide workqueues is dangerous and will be forbidden.
Replace system_highpri_wq with per "struct wfx_dev" bh_wq.

Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
---
Note: This patch is only compile tested.

Changes in v2:
  Use per "struct wfx_dev" workqueue.

 drivers/net/wireless/silabs/wfx/bh.c     | 6 +++---
 drivers/net/wireless/silabs/wfx/hif_tx.c | 2 +-
 drivers/net/wireless/silabs/wfx/main.c   | 6 ++++++
 drivers/net/wireless/silabs/wfx/wfx.h    | 1 +
 4 files changed, 11 insertions(+), 4 deletions(-)

diff --git a/drivers/net/wireless/silabs/wfx/bh.c b/drivers/net/wireless/silabs/wfx/bh.c
index bcea9d5b119c..21dfdcf9cc27 100644
--- a/drivers/net/wireless/silabs/wfx/bh.c
+++ b/drivers/net/wireless/silabs/wfx/bh.c
@@ -267,7 +267,7 @@ void wfx_bh_request_rx(struct wfx_dev *wdev)
 	wfx_control_reg_read(wdev, &cur);
 	prev = atomic_xchg(&wdev->hif.ctrl_reg, cur);
 	complete(&wdev->hif.ctrl_ready);
-	queue_work(system_highpri_wq, &wdev->hif.bh);
+	queue_work(wdev->bh_wq, &wdev->hif.bh);
 
 	if (!(cur & CTRL_NEXT_LEN_MASK))
 		dev_err(wdev->dev, "unexpected control register value: length field is 0: %04x\n",
@@ -280,7 +280,7 @@ void wfx_bh_request_rx(struct wfx_dev *wdev)
 /* Driver want to send data */
 void wfx_bh_request_tx(struct wfx_dev *wdev)
 {
-	queue_work(system_highpri_wq, &wdev->hif.bh);
+	queue_work(wdev->bh_wq, &wdev->hif.bh);
 }
 
 /* If IRQ is not available, this function allow to manually poll the control register and simulate
@@ -295,7 +295,7 @@ void wfx_bh_poll_irq(struct wfx_dev *wdev)
 	u32 reg;
 
 	WARN(!wdev->poll_irq, "unexpected IRQ polling can mask IRQ");
-	flush_workqueue(system_highpri_wq);
+	flush_workqueue(wdev->bh_wq);
 	start = ktime_get();
 	for (;;) {
 		wfx_control_reg_read(wdev, &reg);
diff --git a/drivers/net/wireless/silabs/wfx/hif_tx.c b/drivers/net/wireless/silabs/wfx/hif_tx.c
index 9c653d0e9034..d35dd940d968 100644
--- a/drivers/net/wireless/silabs/wfx/hif_tx.c
+++ b/drivers/net/wireless/silabs/wfx/hif_tx.c
@@ -73,7 +73,7 @@ int wfx_cmd_send(struct wfx_dev *wdev, struct wfx_hif_msg *request,
 
 	if (no_reply) {
 		/* Chip won't reply. Ensure the wq has send the buffer before to continue. */
-		flush_workqueue(system_highpri_wq);
+		flush_workqueue(wdev->bh_wq);
 		ret = 0;
 		goto end;
 	}
diff --git a/drivers/net/wireless/silabs/wfx/main.c b/drivers/net/wireless/silabs/wfx/main.c
index e575a81ca2ca..e015bfb8d221 100644
--- a/drivers/net/wireless/silabs/wfx/main.c
+++ b/drivers/net/wireless/silabs/wfx/main.c
@@ -345,6 +345,10 @@ int wfx_probe(struct wfx_dev *wdev)
 	wdev->pdata.gpio_wakeup = NULL;
 	wdev->poll_irq = true;
 
+	wdev->bh_wq = alloc_workqueue("wfx_bh_wq", WQ_HIGHPRI, 0);
+	if (!wdev->bh_wq)
+		return -ENOMEM;
+
 	wfx_bh_register(wdev);
 
 	err = wfx_init_device(wdev);
@@ -458,6 +462,7 @@ int wfx_probe(struct wfx_dev *wdev)
 	wdev->hwbus_ops->irq_unsubscribe(wdev->hwbus_priv);
 bh_unregister:
 	wfx_bh_unregister(wdev);
+	destroy_workqueue(wdev->bh_wq);
 	return err;
 }
 
@@ -467,6 +472,7 @@ void wfx_release(struct wfx_dev *wdev)
 	wfx_hif_shutdown(wdev);
 	wdev->hwbus_ops->irq_unsubscribe(wdev->hwbus_priv);
 	wfx_bh_unregister(wdev);
+	destroy_workqueue(wdev->bh_wq);
 }
 
 static int __init wfx_core_init(void)
diff --git a/drivers/net/wireless/silabs/wfx/wfx.h b/drivers/net/wireless/silabs/wfx/wfx.h
index 6594cc647c2f..6f5e95dae21f 100644
--- a/drivers/net/wireless/silabs/wfx/wfx.h
+++ b/drivers/net/wireless/silabs/wfx/wfx.h
@@ -57,6 +57,7 @@ struct wfx_dev {
 	struct mutex               rx_stats_lock;
 	struct wfx_hif_tx_power_loop_info tx_power_loop_info;
 	struct mutex               tx_power_loop_info_lock;
+	struct workqueue_struct    *bh_wq;
 };
 
 struct wfx_vif {
-- 
2.34.1

Re: [PATCH v2] wfx: avoid flush_workqueue(system_highpri_wq) usage

[Jérôme Pouiller]

On Monday 2 May 2022 10:16:06 CEST Tetsuo Handa wrote:
> CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you recognize the sender and know the content is safe.
> 
> 
> Flushing system-wide workqueues is dangerous and will be forbidden.
> Replace system_highpri_wq with per "struct wfx_dev" bh_wq.
> 
> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> ---
> Note: This patch is only compile tested.
> 
> Changes in v2:
>   Use per "struct wfx_dev" workqueue.
> 
>  drivers/net/wireless/silabs/wfx/bh.c     | 6 +++---
>  drivers/net/wireless/silabs/wfx/hif_tx.c | 2 +-
>  drivers/net/wireless/silabs/wfx/main.c   | 6 ++++++
>  drivers/net/wireless/silabs/wfx/wfx.h    | 1 +
>  4 files changed, 11 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/wireless/silabs/wfx/bh.c b/drivers/net/wireless/silabs/wfx/bh.c
> index bcea9d5b119c..21dfdcf9cc27 100644
> --- a/drivers/net/wireless/silabs/wfx/bh.c
> +++ b/drivers/net/wireless/silabs/wfx/bh.c
> @@ -267,7 +267,7 @@ void wfx_bh_request_rx(struct wfx_dev *wdev)
>         wfx_control_reg_read(wdev, &cur);
>         prev = atomic_xchg(&wdev->hif.ctrl_reg, cur);
>         complete(&wdev->hif.ctrl_ready);
> -       queue_work(system_highpri_wq, &wdev->hif.bh);
> +       queue_work(wdev->bh_wq, &wdev->hif.bh);
> 
>         if (!(cur & CTRL_NEXT_LEN_MASK))
>                 dev_err(wdev->dev, "unexpected control register value: length field is 0: %04x\n",
> @@ -280,7 +280,7 @@ void wfx_bh_request_rx(struct wfx_dev *wdev)
>  /* Driver want to send data */
>  void wfx_bh_request_tx(struct wfx_dev *wdev)
>  {
> -       queue_work(system_highpri_wq, &wdev->hif.bh);
> +       queue_work(wdev->bh_wq, &wdev->hif.bh);
>  }
> 
>  /* If IRQ is not available, this function allow to manually poll the control register and simulate
> @@ -295,7 +295,7 @@ void wfx_bh_poll_irq(struct wfx_dev *wdev)
>         u32 reg;
> 
>         WARN(!wdev->poll_irq, "unexpected IRQ polling can mask IRQ");
> -       flush_workqueue(system_highpri_wq);
> +       flush_workqueue(wdev->bh_wq);
>         start = ktime_get();
>         for (;;) {
>                 wfx_control_reg_read(wdev, &reg);
> diff --git a/drivers/net/wireless/silabs/wfx/hif_tx.c b/drivers/net/wireless/silabs/wfx/hif_tx.c
> index 9c653d0e9034..d35dd940d968 100644
> --- a/drivers/net/wireless/silabs/wfx/hif_tx.c
> +++ b/drivers/net/wireless/silabs/wfx/hif_tx.c
> @@ -73,7 +73,7 @@ int wfx_cmd_send(struct wfx_dev *wdev, struct wfx_hif_msg *request,
> 
>         if (no_reply) {
>                 /* Chip won't reply. Ensure the wq has send the buffer before to continue. */
> -               flush_workqueue(system_highpri_wq);
> +               flush_workqueue(wdev->bh_wq);
>                 ret = 0;
>                 goto end;
>         }
> diff --git a/drivers/net/wireless/silabs/wfx/main.c b/drivers/net/wireless/silabs/wfx/main.c
> index e575a81ca2ca..e015bfb8d221 100644
> --- a/drivers/net/wireless/silabs/wfx/main.c
> +++ b/drivers/net/wireless/silabs/wfx/main.c
> @@ -345,6 +345,10 @@ int wfx_probe(struct wfx_dev *wdev)
>         wdev->pdata.gpio_wakeup = NULL;
>         wdev->poll_irq = true;
> 
> +       wdev->bh_wq = alloc_workqueue("wfx_bh_wq", WQ_HIGHPRI, 0);
> +       if (!wdev->bh_wq)
> +               return -ENOMEM;
> +
>         wfx_bh_register(wdev);
> 
>         err = wfx_init_device(wdev);
> @@ -458,6 +462,7 @@ int wfx_probe(struct wfx_dev *wdev)
>         wdev->hwbus_ops->irq_unsubscribe(wdev->hwbus_priv);
>  bh_unregister:
>         wfx_bh_unregister(wdev);
> +       destroy_workqueue(wdev->bh_wq);
>         return err;
>  }
> 
> @@ -467,6 +472,7 @@ void wfx_release(struct wfx_dev *wdev)
>         wfx_hif_shutdown(wdev);
>         wdev->hwbus_ops->irq_unsubscribe(wdev->hwbus_priv);
>         wfx_bh_unregister(wdev);
> +       destroy_workqueue(wdev->bh_wq);
>  }
> 
>  static int __init wfx_core_init(void)
> diff --git a/drivers/net/wireless/silabs/wfx/wfx.h b/drivers/net/wireless/silabs/wfx/wfx.h
> index 6594cc647c2f..6f5e95dae21f 100644
> --- a/drivers/net/wireless/silabs/wfx/wfx.h
> +++ b/drivers/net/wireless/silabs/wfx/wfx.h
> @@ -57,6 +57,7 @@ struct wfx_dev {
>         struct mutex               rx_stats_lock;
>         struct wfx_hif_tx_power_loop_info tx_power_loop_info;
>         struct mutex               tx_power_loop_info_lock;
> +       struct workqueue_struct    *bh_wq;
>  };
> 
>  struct wfx_vif {
> --
> 2.34.1
> 
> 
> 

It's right for me.

Signed-off-by: Jérôme Pouiller <jerome.pouiller@silabs.com>

-- 
Jérôme Pouiller

Re: [PATCH] wfx: avoid flush_workqueue(system_highpri_wq) usage

[Jérôme Pouiller]

On Sunday 1 May 2022 10:53:57 CEST Kalle Valo wrote:
> Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> writes:
> 
> > Flushing system-wide workqueues is dangerous and will be forbidden.
> > Replace system_highpri_wq with local wfx_wq.
> >
> > While we are at it, add missing spi_unregister_driver() call when
> > sdio_register_driver() failed.
> >
> > Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> 
> [...]
> 
> > @@ -473,10 +475,18 @@ static int __init wfx_core_init(void)
> >  {
> >       int ret = 0;
> >
> > +     wfx_wq = alloc_workqueue("wfx_wq", WQ_HIGHPRI, 0);
> > +     if (!wfx_wq)
> > +             return -ENOMEM;
> >       if (IS_ENABLED(CONFIG_SPI))
> >               ret = spi_register_driver(&wfx_spi_driver);
> >       if (IS_ENABLED(CONFIG_MMC) && !ret)
> >               ret = sdio_register_driver(&wfx_sdio_driver);
> > +     if (ret) {
> > +             if (IS_ENABLED(CONFIG_SPI))
> > +                     spi_unregister_driver(&wfx_spi_driver);
> > +             destroy_workqueue(wfx_wq);
> > +     }
> >       return ret;
> >  }
> >  module_init(wfx_core_init);
> 
> So now the thread is created every time the module loaded, even if
> there's no device available. Also I'm not really a fan of global
> variables (wfx_wq). I would rather create a workqueue per device in
> wfx_probe() or use the workqueue provided by mac80211.
> 
> /**
>  * ieee80211_queue_work - add work onto the mac80211 workqueue
>  *
>  * Drivers and mac80211 use this to add work onto the mac80211 workqueue.
>  * This helper ensures drivers are not queueing work when they should not be.
>  *
>  * @hw: the hardware struct for the interface we are adding work for
>  * @work: the work we want to add onto the mac80211 workqueue
>  */
> void ieee80211_queue_work(struct ieee80211_hw *hw, struct work_struct *work);
> 

The last time I have checked if I could use this workqueue, I remember
it was not well suited for wfx (I don't remember why exactly). So I
believe we have to allocate a new workqueue.


-- 
Jérôme Pouiller

Re: [PATCH v2] wfx: avoid flush_workqueue(system_highpri_wq) usage

[Kalle Valo]

Jérôme Pouiller <jerome.pouiller@silabs.com> writes:

> On Monday 2 May 2022 10:16:06 CEST Tetsuo Handa wrote:
>> CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you recognize the sender and know the content is safe.
>> 
>> 
>> Flushing system-wide workqueues is dangerous and will be forbidden.
>> Replace system_highpri_wq with per "struct wfx_dev" bh_wq.
>> 
>> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>

[...]

> It's right for me.
>
> Signed-off-by: Jérôme Pouiller <jerome.pouiller@silabs.com>

Thanks, but I'll change this to Acked-by:

https://www.kernel.org/doc/html/latest/process/submitting-patches.html#when-to-use-acked-by-cc-and-co-developed-by

Also please edit your quotes, otherwise patchwork is hard to use.

-- 
https://patchwork.kernel.org/project/linux-wireless/list/

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

Re: [v2] wfx: avoid flush_workqueue(system_highpri_wq) usage

[Kalle Valo]

Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> wrote:

> Flushing system-wide workqueues is dangerous and will be forbidden.
> Replace system_highpri_wq with per "struct wfx_dev" bh_wq.
> 
> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> Acked-by: Jérôme Pouiller <jerome.pouiller@silabs.com>

Patch applied to wireless-next.git, thanks.

eeff214dbfcb wfx: avoid flush_workqueue(system_highpri_wq) usage

-- 
https://patchwork.kernel.org/project/linux-wireless/patch/f15574a6-aba4-72bc-73af-26fdcdf9fb63@I-love.SAKURA.ne.jp/

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