Message-ID: <20241022013855.284783-1-marex@denx.de>
Date: Tue, 22 Oct 2024 03:38:31 +0200
From: Marek Vasut <marex@...x.de>
To: linux-wireless@...r.kernel.org
Cc: Marek Vasut <marex@...x.de>,
	"David S. Miller" <davem@...emloft.net>,
	Adham Abozaeid <adham.abozaeid@...rochip.com>,
	Ajay Singh <ajay.kathat@...rochip.com>,
	Alexis Lothoré <alexis.lothore@...tlin.com>,
	Claudiu Beznea <claudiu.beznea@...on.dev>,
	Conor Dooley <conor+dt@...nel.org>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>,
	Kalle Valo <kvalo@...nel.org>,
	Krzysztof Kozlowski <krzk+dt@...nel.org>,
	Paolo Abeni <pabeni@...hat.com>,
	Rob Herring <robh@...nel.org>,
	devicetree@...r.kernel.org,
	netdev@...r.kernel.org
Subject: [PATCH] wifi: wilc1000: Rework bus locking

The bus locking in this driver is broken and produces subtle race
condition with ksdioirqd and its mmc_claim_host()/mmc_release_host()
usage in case of SDIO bus. Rework the locking to avoid this race
condition.

The problem is the hif_cs mutex used in acquire_bus()/release_bus(),
which makes it look like calling acquire_bus() results in exclusive
access to the bus, but that is not true for SDIO bus. For SDIO bus,
to obtain exclusive access (any access, really), it is necessary to
call sdio_claim_host(), which is a wrapper around mmc_claim_host(),
which does its own locking. The acquire_bus() does not do that, but
the SDIO interface implementation does call sdio_claim_host() and
sdio_release_host() every single command, which is problematic. To
make things worse, wilc_sdio_interrupt() implementation called from
ksdioirqd first calls sdio_release_host(), then interrupt handling
and finally sdio_claim_host().

The core problem is that sdio_claim_host() cannot be done per command,
but has to be done per register/data IO which consists of multiple
commands. Usually the WILC register read/write consists of 3x CMD52
to push in CSA pointer address and 1x CMD53 to read/write data to that
address. Most other accesses are also composed of multiple commands.

Currently, if ksdioirqd wakes up and attempts to read SDIO_CCCR_INTx
to get pending SDIO IRQs in sdio_get_pending_irqs(), it can easily
perform that transfer between two consecutive CMD52 which are pushing
in the CSA pointer address and possibly disrupt the WILC operation.
This is undesired behavior.

Rework the locking.

Introduce new .hif_claim/.hif_release callbacks which implement bus
specific locking. Lock/unlock SDIO bus access using sdio_claim_host()
and sdio_release_host(), lock/unlock SPI bus access using the current
hif_cs mutex moved purely into the spi.c interface. Make acquire_bus()
and release_bus() call the .hif_claim/.hif_release() callbacks and do
not access the hif_cs mutex from there at all.

Remove any SDIO bus locking used directly in commands and the broken
SDIO bus unlocking in wilc_sdio_interrupt(), this is no longer needed.
Fix up SDIO initialization code which newly needs sdio_claim_host()
and sdio_release_host(), since it cannot depend on the locking being
done per-command anymore.

Signed-off-by: Marek Vasut <marex@...x.de>
---
Cc: "David S. Miller" <davem@...emloft.net>
Cc: Adham Abozaeid <adham.abozaeid@...rochip.com>
Cc: Ajay Singh <ajay.kathat@...rochip.com>
Cc: Alexis Lothoré <alexis.lothore@...tlin.com>
Cc: Claudiu Beznea <claudiu.beznea@...on.dev>
Cc: Conor Dooley <conor+dt@...nel.org>
Cc: Eric Dumazet <edumazet@...gle.com>
Cc: Jakub Kicinski <kuba@...nel.org>
Cc: Kalle Valo <kvalo@...nel.org>
Cc: Krzysztof Kozlowski <krzk+dt@...nel.org>
Cc: Paolo Abeni <pabeni@...hat.com>
Cc: Rob Herring <robh@...nel.org>
Cc: devicetree@...r.kernel.org
Cc: linux-wireless@...r.kernel.org
Cc: netdev@...r.kernel.org
---
NOTE: I only tested the SDIO part
---
 .../wireless/microchip/wilc1000/cfg80211.c    |  2 -
 .../net/wireless/microchip/wilc1000/netdev.c  |  2 -
 .../net/wireless/microchip/wilc1000/netdev.h  |  3 --
 .../net/wireless/microchip/wilc1000/sdio.c    | 40 ++++++++++++-------
 drivers/net/wireless/microchip/wilc1000/spi.c | 15 +++++++
 .../net/wireless/microchip/wilc1000/wlan.c    | 30 ++++++++++----
 .../net/wireless/microchip/wilc1000/wlan.h    |  2 +
 7 files changed, 65 insertions(+), 29 deletions(-)

diff --git a/drivers/net/wireless/microchip/wilc1000/cfg80211.c b/drivers/net/wireless/microchip/wilc1000/cfg80211.c
index b0dae6f7c633b..9a9fc8e8c8354 100644
--- a/drivers/net/wireless/microchip/wilc1000/cfg80211.c
+++ b/drivers/net/wireless/microchip/wilc1000/cfg80211.c
@@ -1730,7 +1730,6 @@ static const struct cfg80211_ops wilc_cfg80211_ops = {
 
 static void wlan_init_locks(struct wilc *wl)
 {
-	mutex_init(&wl->hif_cs);
 	mutex_init(&wl->rxq_cs);
 	mutex_init(&wl->cfg_cmd_lock);
 	mutex_init(&wl->vif_mutex);
@@ -1748,7 +1747,6 @@ static void wlan_init_locks(struct wilc *wl)
 
 void wlan_deinit_locks(struct wilc *wilc)
 {
-	mutex_destroy(&wilc->hif_cs);
 	mutex_destroy(&wilc->rxq_cs);
 	mutex_destroy(&wilc->cfg_cmd_lock);
 	mutex_destroy(&wilc->txq_add_to_head_cs);
diff --git a/drivers/net/wireless/microchip/wilc1000/netdev.c b/drivers/net/wireless/microchip/wilc1000/netdev.c
index 7e84fc0fd9118..22c91a0b9b648 100644
--- a/drivers/net/wireless/microchip/wilc1000/netdev.c
+++ b/drivers/net/wireless/microchip/wilc1000/netdev.c
@@ -468,9 +468,7 @@ static void wilc_wlan_deinitialize(struct net_device *dev)
 
 		if (!wl->dev_irq_num &&
 		    wl->hif_func->disable_interrupt) {
-			mutex_lock(&wl->hif_cs);
 			wl->hif_func->disable_interrupt(wl);
-			mutex_unlock(&wl->hif_cs);
 		}
 		complete(&wl->txq_event);
 
diff --git a/drivers/net/wireless/microchip/wilc1000/netdev.h b/drivers/net/wireless/microchip/wilc1000/netdev.h
index 95bc8b8fe65a5..8bdc27edf00af 100644
--- a/drivers/net/wireless/microchip/wilc1000/netdev.h
+++ b/drivers/net/wireless/microchip/wilc1000/netdev.h
@@ -240,9 +240,6 @@ struct wilc {
 	/* protect rxq_entry_t receiver queue */
 	struct mutex rxq_cs;
 
-	/* lock to protect hif access */
-	struct mutex hif_cs;
-
 	struct completion cfg_event;
 	struct completion sync_event;
 	struct completion txq_event;
diff --git a/drivers/net/wireless/microchip/wilc1000/sdio.c b/drivers/net/wireless/microchip/wilc1000/sdio.c
index 170470d1c2092..0796e2ccfe32e 100644
--- a/drivers/net/wireless/microchip/wilc1000/sdio.c
+++ b/drivers/net/wireless/microchip/wilc1000/sdio.c
@@ -56,11 +56,23 @@ struct sdio_cmd53 {
 
 static const struct wilc_hif_func wilc_hif_sdio;
 
-static void wilc_sdio_interrupt(struct sdio_func *func)
+static void wilc_sdio_claim(struct wilc *wilc)
+{
+	struct sdio_func *func = container_of(wilc->dev, struct sdio_func, dev);
+
+	sdio_claim_host(func);
+}
+
+static void wilc_sdio_release(struct wilc *wilc)
 {
+	struct sdio_func *func = container_of(wilc->dev, struct sdio_func, dev);
+
 	sdio_release_host(func);
+}
+
+static void wilc_sdio_interrupt(struct sdio_func *func)
+{
 	wilc_handle_isr(sdio_get_drvdata(func));
-	sdio_claim_host(func);
 }
 
 static int wilc_sdio_cmd52(struct wilc *wilc, struct sdio_cmd52 *cmd)
@@ -69,8 +81,6 @@ static int wilc_sdio_cmd52(struct wilc *wilc, struct sdio_cmd52 *cmd)
 	int ret;
 	u8 data;
 
-	sdio_claim_host(func);
-
 	func->num = cmd->function;
 	if (cmd->read_write) {  /* write */
 		if (cmd->raw) {
@@ -85,8 +95,6 @@ static int wilc_sdio_cmd52(struct wilc *wilc, struct sdio_cmd52 *cmd)
 		cmd->data = data;
 	}
 
-	sdio_release_host(func);
-
 	if (ret)
 		dev_err(&func->dev, "%s..failed, err(%d)\n", __func__, ret);
 	return ret;
@@ -99,8 +107,6 @@ static int wilc_sdio_cmd53(struct wilc *wilc, struct sdio_cmd53 *cmd)
 	struct wilc_sdio *sdio_priv = wilc->bus_data;
 	u8 *buf = cmd->buffer;
 
-	sdio_claim_host(func);
-
 	func->num = cmd->function;
 	func->cur_blksize = cmd->block_size;
 	if (cmd->block_mode)
@@ -128,8 +134,6 @@ static int wilc_sdio_cmd53(struct wilc *wilc, struct sdio_cmd53 *cmd)
 			memcpy(cmd->buffer, buf, size);
 	}
 out:
-	sdio_release_host(func);
-
 	if (ret)
 		dev_err(&func->dev, "%s..failed, err(%d)\n", __func__,  ret);
 
@@ -180,9 +184,11 @@ static int wilc_sdio_probe(struct sdio_func *func,
 		goto dispose_irq;
 	}
 
+	wilc_sdio_claim(wilc);
 	wilc_sdio_init(wilc, false);
 
 	ret = wilc_get_chipid(wilc);
+	wilc_sdio_release(wilc);
 	if (ret)
 		goto dispose_irq;
 
@@ -196,7 +202,9 @@ static int wilc_sdio_probe(struct sdio_func *func,
 		goto dispose_irq;
 	}
 
+	wilc_sdio_claim(wilc);
 	wilc_sdio_deinit(wilc);
+	wilc_sdio_release(wilc);
 
 	vif = wilc_netdev_ifc_init(wilc, "wlan%d", WILC_STATION_MODE,
 				   NL80211_IFTYPE_STATION, false);
@@ -258,9 +266,9 @@ static int wilc_sdio_enable_interrupt(struct wilc *dev)
 	struct sdio_func *func = container_of(dev->dev, struct sdio_func, dev);
 	int ret = 0;
 
-	sdio_claim_host(func);
+	wilc_sdio_claim(dev);
 	ret = sdio_claim_irq(func, wilc_sdio_interrupt);
-	sdio_release_host(func);
+	wilc_sdio_release(dev);
 
 	if (ret < 0) {
 		dev_err(&func->dev, "can't claim sdio_irq, err(%d)\n", ret);
@@ -274,11 +282,11 @@ static void wilc_sdio_disable_interrupt(struct wilc *dev)
 	struct sdio_func *func = container_of(dev->dev, struct sdio_func, dev);
 	int ret;
 
-	sdio_claim_host(func);
+	wilc_sdio_claim(dev);
 	ret = sdio_release_irq(func);
 	if (ret < 0)
 		dev_err(&func->dev, "can't release sdio_irq, err(%d)\n", ret);
-	sdio_release_host(func);
+	wilc_sdio_release(dev);
 }
 
 /********************************************
@@ -1013,6 +1021,8 @@ static const struct wilc_hif_func wilc_hif_sdio = {
 	.disable_interrupt = wilc_sdio_disable_interrupt,
 	.hif_reset = wilc_sdio_reset,
 	.hif_is_init = wilc_sdio_is_init,
+	.hif_claim = wilc_sdio_claim,
+	.hif_release = wilc_sdio_release,
 };
 
 static int wilc_sdio_suspend(struct device *dev)
@@ -1053,7 +1063,9 @@ static int wilc_sdio_resume(struct device *dev)
 	if (!IS_ERR(wilc->rtc_clk))
 		clk_prepare_enable(wilc->rtc_clk);
 
+	wilc_sdio_claim(wilc);
 	wilc_sdio_init(wilc, true);
+	wilc_sdio_release(wilc);
 	wilc_sdio_enable_interrupt(wilc);
 
 	return host_wakeup_notify(wilc);
diff --git a/drivers/net/wireless/microchip/wilc1000/spi.c b/drivers/net/wireless/microchip/wilc1000/spi.c
index ce2a9cdd6aa78..7af25ea6068f4 100644
--- a/drivers/net/wireless/microchip/wilc1000/spi.c
+++ b/drivers/net/wireless/microchip/wilc1000/spi.c
@@ -50,6 +50,9 @@ struct wilc_spi {
 		struct gpio_desc *enable;	/* ENABLE GPIO or NULL */
 		struct gpio_desc *reset;	/* RESET GPIO or NULL */
 	} gpios;
+
+	/* lock to protect hif access */
+	struct mutex hif_cs;
 };
 
 static const struct wilc_hif_func wilc_hif_spi;
@@ -1105,6 +1108,15 @@ static int wilc_spi_write(struct wilc *wilc, u32 addr, u8 *buf, u32 size)
  *      Bus interfaces
  *
  ********************************************/
+static void wilc_spi_claim(struct wilc *wilc)
+{
+	mutex_lock(&wilc->hif_cs);
+}
+
+static void wilc_spi_release(struct wilc *wilc)
+{
+	mutex_unlock(&wilc->hif_cs);
+}
 
 static int wilc_spi_reset(struct wilc *wilc)
 {
@@ -1132,6 +1144,7 @@ static int wilc_spi_deinit(struct wilc *wilc)
 
 	spi_priv->isinit = false;
 	wilc_wlan_power(wilc, false);
+	mutex_destroy(&wilc->hif_cs);
 	return 0;
 }
 
@@ -1140,6 +1153,8 @@ static int wilc_spi_init(struct wilc *wilc, bool resume)
 	struct wilc_spi *spi_priv = wilc->bus_data;
 	int ret;
 
+	mutex_init(&spi->hif_cs);
+
 	if (spi_priv->isinit) {
 		/* Confirm we can read chipid register without error: */
 		if (wilc_validate_chipid(wilc) == 0)
diff --git a/drivers/net/wireless/microchip/wilc1000/wlan.c b/drivers/net/wireless/microchip/wilc1000/wlan.c
index 9d80adc45d6be..b149734f19a05 100644
--- a/drivers/net/wireless/microchip/wilc1000/wlan.c
+++ b/drivers/net/wireless/microchip/wilc1000/wlan.c
@@ -767,25 +767,37 @@ static int chip_wakeup(struct wilc *wilc)
 
 static inline int acquire_bus(struct wilc *wilc, enum bus_acquire acquire)
 {
-	int ret = 0;
+	const struct wilc_hif_func *hif_func = wilc->hif_func;
+	int ret;
 
-	mutex_lock(&wilc->hif_cs);
-	if (acquire == WILC_BUS_ACQUIRE_AND_WAKEUP && wilc->power_save_mode) {
-		ret = chip_wakeup(wilc);
-		if (ret)
-			mutex_unlock(&wilc->hif_cs);
-	}
+	hif_func->hif_claim(wilc);
+
+	if (!wilc->power_save_mode)
+		return 0;
 
+	if (acquire != WILC_BUS_ACQUIRE_AND_WAKEUP)
+		return 0;
+
+	ret = chip_wakeup(wilc);
+	if (ret)
+		goto err;
+
+	return 0;
+
+err:
+	hif_func->hif_release(wilc);
 	return ret;
 }
 
 static inline int release_bus(struct wilc *wilc, enum bus_release release)
 {
+	const struct wilc_hif_func *hif_func = wilc->hif_func;
 	int ret = 0;
 
 	if (release == WILC_BUS_RELEASE_ALLOW_SLEEP && wilc->power_save_mode)
 		ret = chip_allow_sleep(wilc);
-	mutex_unlock(&wilc->hif_cs);
+
+	hif_func->hif_release(wilc);
 
 	return ret;
 }
@@ -1447,7 +1459,9 @@ void wilc_wlan_cleanup(struct net_device *dev)
 	wilc->rx_buffer = NULL;
 	kfree(wilc->tx_buffer);
 	wilc->tx_buffer = NULL;
+	acquire_bus(wilc, WILC_BUS_ACQUIRE_AND_WAKEUP);
 	wilc->hif_func->hif_deinit(wilc);
+	release_bus(wilc, WILC_BUS_RELEASE_ALLOW_SLEEP);
 }
 
 static int wilc_wlan_cfg_commit(struct wilc_vif *vif, int type,
diff --git a/drivers/net/wireless/microchip/wilc1000/wlan.h b/drivers/net/wireless/microchip/wilc1000/wlan.h
index b9e7f9222eadd..ade2db95e8a0f 100644
--- a/drivers/net/wireless/microchip/wilc1000/wlan.h
+++ b/drivers/net/wireless/microchip/wilc1000/wlan.h
@@ -403,6 +403,8 @@ struct wilc_hif_func {
 	void (*disable_interrupt)(struct wilc *nic);
 	int (*hif_reset)(struct wilc *wilc);
 	bool (*hif_is_init)(struct wilc *wilc);
+	void (*hif_claim)(struct wilc *wilc);
+	void (*hif_release)(struct wilc *wilc);
 };
 
 #define WILC_MAX_CFG_FRAME_SIZE		1468
-- 
2.45.2

Powered by blists - more mailing lists