@@ -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);
@@ -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);
@@ -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;
@@ -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);
@@ -1105,6 +1105,19 @@ static int wilc_spi_write(struct wilc *wilc, u32 addr, u8 *buf, u32 size)
* Bus interfaces
*
********************************************/
+static void wilc_spi_claim(struct wilc *wilc)
+{
+ struct spi_device *spi = to_spi_device(wilc->dev);
+
+ spi_bus_lock(spi->controller);
+}
+
+static void wilc_spi_release(struct wilc *wilc)
+{
+ struct spi_device *spi = to_spi_device(wilc->dev);
+
+ spi_bus_unlock(spi->controller);
+}
static int wilc_spi_reset(struct wilc *wilc)
{
@@ -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,
@@ -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
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() should not 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 spi_bus_lock() and spi_bus_unlock(), and get rid of hif_cs mutex. Make acquire_bus() and release_bus() call the .hif_claim/.hif_release() callbacks. 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. Infograpics. With current code, this can happen, which is not good, because transfers from multiple threads can be interleaved and interfere with each other: " thread 1 | thread2 do_some_higher_level_op() { | ... | read_register_0x3b0000() { | claim_bus | CMD52 0x00 | release bus | ksdioirqd() { | claim_bus | CMD52 0x0f, lets read SDIO_CCCR_INTx | release_bus claim bus | } CMD52 0x00 | release_bus | claim_bus | CMD52 0x3b | release_bus | claim_bus | CMD53 lets read data | release_bus | } | ... | } | " What should happen is either: " thread 1 | thread2 | ksdioirqd() { // option 1 | claim_bus | CMD52 0x0f, lets read SDIO_CCCR_INTx do_some_higher_level_op() { | release_bus claim_bus | } ... | read_register_0x3b0000 { | CMD52 0x00 | CMD52 0x00 | CMD52 0x3b | CMD53 lets read data | } | ... | read_another_register() | ... | release_bus | ksdioirqd() { // option 2 } | claim_bus | CMD52 0x0f, lets read SDIO_CCCR_INTx | release_bus | } " Signed-off-by: Marek Vasut <marex@denx.de> --- Cc: "David S. Miller" <davem@davemloft.net> Cc: Adham Abozaeid <adham.abozaeid@microchip.com> Cc: Ajay Singh <ajay.kathat@microchip.com> Cc: Alexis Lothoré <alexis.lothore@bootlin.com> Cc: Claudiu Beznea <claudiu.beznea@tuxon.dev> Cc: Conor Dooley <conor+dt@kernel.org> Cc: Eric Dumazet <edumazet@google.com> Cc: Jakub Kicinski <kuba@kernel.org> Cc: Kalle Valo <kvalo@kernel.org> Cc: Krzysztof Kozlowski <krzk+dt@kernel.org> Cc: Paolo Abeni <pabeni@redhat.com> Cc: Rob Herring <robh@kernel.org> Cc: devicetree@vger.kernel.org Cc: linux-wireless@vger.kernel.org Cc: netdev@vger.kernel.org --- V2: Use spi_bus_lock() and spi_bus_unlock() to get rid of hif_cs mutex --- 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 | 13 ++++++ .../net/wireless/microchip/wilc1000/wlan.c | 30 ++++++++++---- .../net/wireless/microchip/wilc1000/wlan.h | 2 + 7 files changed, 63 insertions(+), 29 deletions(-)