[PATCH] wifi: brcmfmac: drain bus_reset work on device removal

[PATCH] wifi: brcmfmac: drain bus_reset work on device removal

[Fan Wu]

brcmf_fw_crashed() and the debugfs "reset" entry both schedule
drvr->bus_reset, whose callback recovers drvr through container_of()
and dereferences it.  The teardown paths free drvr (brcmf_free ->
wiphy_free) without draining the work, so a bus_reset callback pending
or running during removal can outlive drvr.

Cancellation cannot live in brcmf_detach() or brcmf_free(): the work
callback reaches teardown through the bus .reset op (PCIe
brcmf_pcie_reset -> brcmf_detach; SDIO brcmf_sdio_bus_reset ->
brcmf_sdiod_remove -> brcmf_free), so cancelling there would wait for
the running work and deadlock.  Arming and the drain must also be
mutually exclusive: a debugfs writer can otherwise schedule bus_reset
after the drain and before the debugfs file is removed in
brcmf_cfg80211_detach(), re-opening the window.

Add a per-bus mutex and route all arming through
brcmf_bus_schedule_reset(), which under the lock skips when the bus is
marked removing.  Each bus remove entry calls
brcmf_bus_cancel_reset_work(), which under the same lock sets removing
and cancels the work.  Where applicable the remove entry first stops
the firmware-crash producer: on PCIe mask the mailbox and
synchronize_irq; on SDIO unregister the bus interrupt and cancel the
data worker, which also reports firmware halts through
brcmf_fw_crashed().  The mutex is initialized at bus allocation so it
is ready before any firmware-probe or removal path can reach it.  The
SDIO suspend power-off path frees drvr through the same
brcmf_sdiod_remove() and takes the same lock; resume re-allows the work
only on a successful re-probe.

This issue was found by an in-house static analysis tool.

Fixes: 4684997d9eea ("brcmfmac: reset PCIe bus on a firmware crash")
Cc: stable@vger.kernel.org
Signed-off-by: Fan Wu <fanwu01@zju.edu.cn>
Assisted-by: Codex:gpt-5.5
---
 .../broadcom/brcm80211/brcmfmac/bcmsdh.c      | 13 ++++++++
 .../broadcom/brcm80211/brcmfmac/bus.h         |  6 ++++
 .../broadcom/brcm80211/brcmfmac/core.c        | 33 +++++++++++++++++--
 .../broadcom/brcm80211/brcmfmac/pcie.c        |  6 ++++
 .../broadcom/brcm80211/brcmfmac/sdio.c        |  6 ++++
 .../broadcom/brcm80211/brcmfmac/sdio.h        |  1 +
 .../broadcom/brcm80211/brcmfmac/usb.c         |  3 ++
 7 files changed, 66 insertions(+), 2 deletions(-)

diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c
index ac02244a6..c4bb32aec 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c
@@ -1043,6 +1043,7 @@ static int brcmf_ops_sdio_probe(struct sdio_func *func,
 	bus_if = kzalloc(sizeof(struct brcmf_bus), GFP_KERNEL);
 	if (!bus_if)
 		return -ENOMEM;
+	mutex_init(&bus_if->bus_reset_lock);
 	sdiodev = kzalloc(sizeof(struct brcmf_sdio_dev), GFP_KERNEL);
 	if (!sdiodev) {
 		kfree(bus_if);
@@ -1102,6 +1103,14 @@ static void brcmf_ops_sdio_remove(struct sdio_func *func)
 		if (func->num != 1)
 			return;
 
+		/* Drain bus_reset before the shared brcmf_sdiod_remove()
+		 * teardown, which the SDIO reset callback also reaches.  The
+		 * data worker can arm bus_reset via brcmf_fw_crashed(); cancel
+		 * it first.
+		 */
+		brcmf_sdio_cancel_datawork(sdiodev->bus);
+		brcmf_bus_cancel_reset_work(bus_if);
+
 		/* only proceed with rest of cleanup if func 1 */
 		brcmf_sdiod_remove(sdiodev);
 
@@ -1163,6 +1172,8 @@ static int brcmf_ops_sdio_suspend(struct device *dev)
 	} else {
 		/* power will be cut so remove device, probe again in resume */
 		brcmf_sdiod_intr_unregister(sdiodev);
+		brcmf_sdio_cancel_datawork(sdiodev->bus);
+		brcmf_bus_cancel_reset_work(bus_if);
 		ret = brcmf_sdiod_remove(sdiodev);
 		if (ret)
 			brcmf_err("Failed to remove device on suspend\n");
@@ -1188,6 +1199,8 @@ static int brcmf_ops_sdio_resume(struct device *dev)
 		ret = brcmf_sdiod_probe(sdiodev);
 		if (ret)
 			brcmf_err("Failed to probe device on resume\n");
+		else
+			brcmf_bus_allow_reset_work(bus_if);
 	} else {
 		if (sdiodev->wowl_enabled &&
 		    sdiodev->settings->bus.sdio.oob_irq_supported)
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bus.h b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bus.h
index 3f5da3bb6..b606094af 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bus.h
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bus.h
@@ -6,6 +6,7 @@
 #ifndef BRCMFMAC_BUS_H
 #define BRCMFMAC_BUS_H
 
+#include <linux/mutex.h>
 #include "debug.h"
 
 /* IDs of the 6 default common rings of msgbuf protocol */
@@ -149,11 +150,16 @@ struct brcmf_bus {
 	u32 chiprev;
 	bool always_use_fws_queue;
 	bool wowl_supported;
+	bool removing;		/* device removal in progress; quiesce async work */
+	struct mutex bus_reset_lock;
 
 	const struct brcmf_bus_ops *ops;
 	struct brcmf_bus_msgbuf *msgbuf;
 };
 
+void brcmf_bus_cancel_reset_work(struct brcmf_bus *bus_if);
+void brcmf_bus_allow_reset_work(struct brcmf_bus *bus_if);
+
 /*
  * callback wrappers
  */
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
index fed9cd5f2..b934feb9b 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
@@ -1164,6 +1164,35 @@ static int brcmf_revinfo_read(struct seq_file *s, void *data)
 	return 0;
 }
 
+/* Serialize bus_reset arming (debugfs reset write, brcmf_fw_crashed) against the
+ * teardown drain: the remove path takes bus_reset_lock, sets ->removing and cancels
+ * the work under it, so a racing armer either schedules before the cancel (and is
+ * drained) or observes ->removing and desists.
+ */
+static void brcmf_bus_schedule_reset(struct brcmf_bus *bus_if)
+{
+	mutex_lock(&bus_if->bus_reset_lock);
+	if (bus_if->drvr && bus_if->drvr->bus_reset.func && !bus_if->removing)
+		schedule_work(&bus_if->drvr->bus_reset);
+	mutex_unlock(&bus_if->bus_reset_lock);
+}
+
+void brcmf_bus_cancel_reset_work(struct brcmf_bus *bus_if)
+{
+	mutex_lock(&bus_if->bus_reset_lock);
+	bus_if->removing = true;
+	if (bus_if->drvr)
+		cancel_work_sync(&bus_if->drvr->bus_reset);
+	mutex_unlock(&bus_if->bus_reset_lock);
+}
+
+void brcmf_bus_allow_reset_work(struct brcmf_bus *bus_if)
+{
+	mutex_lock(&bus_if->bus_reset_lock);
+	bus_if->removing = false;
+	mutex_unlock(&bus_if->bus_reset_lock);
+}
+
 static void brcmf_core_bus_reset(struct work_struct *work)
 {
 	struct brcmf_pub *drvr = container_of(work, struct brcmf_pub,
@@ -1184,7 +1213,7 @@ static ssize_t bus_reset_write(struct file *file, const char __user *user_buf,
 	if (value != 1)
 		return -EINVAL;
 
-	schedule_work(&drvr->bus_reset);
+	brcmf_bus_schedule_reset(drvr->bus_if);
 
 	return count;
 }
@@ -1408,7 +1437,7 @@ void brcmf_fw_crashed(struct device *dev)
 
 	brcmf_dev_coredump(dev);
 
-	schedule_work(&drvr->bus_reset);
+	brcmf_bus_schedule_reset(bus_if);
 }
 
 void brcmf_detach(struct device *dev)
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c
index 8b149996f..3c6775166 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c
@@ -1914,6 +1914,7 @@ brcmf_pcie_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 		ret = -ENOMEM;
 		goto fail;
 	}
+	mutex_init(&bus->bus_reset_lock);
 	bus->msgbuf = kzalloc(sizeof(*bus->msgbuf), GFP_KERNEL);
 	if (!bus->msgbuf) {
 		ret = -ENOMEM;
@@ -1985,6 +1986,11 @@ brcmf_pcie_remove(struct pci_dev *pdev)
 	if (devinfo->ci)
 		brcmf_pcie_intr_disable(devinfo);
 
+	if (devinfo->irq_allocated)
+		synchronize_irq(pdev->irq);
+
+	brcmf_bus_cancel_reset_work(bus);
+
 	brcmf_detach(&pdev->dev);
 	brcmf_free(&pdev->dev);
 
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
index 8effeb7a7..31e37b0d4 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
@@ -4541,6 +4541,12 @@ struct brcmf_sdio *brcmf_sdio_probe(struct brcmf_sdio_dev *sdiodev)
 	return NULL;
 }
 
+void brcmf_sdio_cancel_datawork(struct brcmf_sdio *bus)
+{
+	if (bus)
+		cancel_work_sync(&bus->datawork);
+}
+
 /* Detach and free everything */
 void brcmf_sdio_remove(struct brcmf_sdio *bus)
 {
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.h b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.h
index 15d2c02fa..3c68ebf8e 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.h
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.h
@@ -373,6 +373,7 @@ int brcmf_sdiod_remove(struct brcmf_sdio_dev *sdiodev);
 struct brcmf_sdio *brcmf_sdio_probe(struct brcmf_sdio_dev *sdiodev);
 void brcmf_sdio_remove(struct brcmf_sdio *bus);
 void brcmf_sdio_isr(struct brcmf_sdio *bus, bool in_isr);
+void brcmf_sdio_cancel_datawork(struct brcmf_sdio *bus);
 
 void brcmf_sdio_wd_timer(struct brcmf_sdio *bus, bool active);
 void brcmf_sdio_wowl_config(struct device *dev, bool enabled);
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/usb.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/usb.c
index 9fb68c2dc..97d65ba36 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/usb.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/usb.c
@@ -1271,6 +1271,7 @@ static int brcmf_usb_probe_cb(struct brcmf_usbdev_info *devinfo)
 		ret = -ENOMEM;
 		goto fail;
 	}
+	mutex_init(&bus->bus_reset_lock);
 
 	bus->dev = dev;
 	bus_pub->bus = bus;
@@ -1336,6 +1337,8 @@ brcmf_usb_disconnect_cb(struct brcmf_usbdev_info *devinfo)
 		return;
 	brcmf_dbg(USB, "Enter, bus_pub %p\n", devinfo);
 
+	brcmf_bus_cancel_reset_work(devinfo->bus_pub.bus);
+
 	brcmf_detach(devinfo->dev);
 	brcmf_free(devinfo->dev);
 	kfree(devinfo->bus_pub.bus);
-- 
2.34.1

Re: [PATCH] wifi: brcmfmac: drain bus_reset work on device removal

[Eddie Phillips]

On Thu,  9 Jul 2026 10:16:35 +0000 Fan Wu <fanwu01@zju.edu.cn> wrote:

> brcmf_fw_crashed() and the debugfs "reset" entry both schedule
> drvr->bus_reset, whose callback recovers drvr through container_of()
> and dereferences it.  The teardown paths free drvr (brcmf_free ->
> wiphy_free) without draining the work, so a bus_reset callback pending
> or running during removal can outlive drvr.
> 
> Cancellation cannot live in brcmf_detach() or brcmf_free(): the work
> callback reaches teardown through the bus .reset op (PCIe
> brcmf_pcie_reset -> brcmf_detach; SDIO brcmf_sdio_bus_reset ->
> brcmf_sdiod_remove -> brcmf_free), so cancelling there would wait for
> the running work and deadlock.  Arming and the drain must also be
> mutually exclusive: a debugfs writer can otherwise schedule bus_reset
> after the drain and before the debugfs file is removed in
> brcmf_cfg80211_detach(), re-opening the window.
> 
> Add a per-bus mutex and route all arming through
> brcmf_bus_schedule_reset(), which under the lock skips when the bus is
> marked removing.  Each bus remove entry calls
> brcmf_bus_cancel_reset_work(), which under the same lock sets removing
> and cancels the work.  Where applicable the remove entry first stops
> the firmware-crash producer: on PCIe mask the mailbox and
> synchronize_irq; on SDIO unregister the bus interrupt and cancel the
> data worker, which also reports firmware halts through
> brcmf_fw_crashed().  The mutex is initialized at bus allocation so it
> is ready before any firmware-probe or removal path can reach it.  The
> SDIO suspend power-off path frees drvr through the same
> brcmf_sdiod_remove() and takes the same lock; resume re-allows the work
> only on a successful re-probe.
> 
> This issue was found by an in-house static analysis tool.
> 
> Fixes: 4684997d9eea ("brcmfmac: reset PCIe bus on a firmware crash")
> Cc: stable@vger.kernel.org
> Signed-off-by: Fan Wu <fanwu01@zju.edu.cn>
> Assisted-by: Codex:gpt-5.5
> ---
>  .../broadcom/brcm80211/brcmfmac/bcmsdh.c      | 13 ++++++++
>  .../broadcom/brcm80211/brcmfmac/bus.h         |  6 ++++
>  .../broadcom/brcm80211/brcmfmac/core.c        | 33 +++++++++++++++++--
>  .../broadcom/brcm80211/brcmfmac/pcie.c        |  6 ++++
>  .../broadcom/brcm80211/brcmfmac/sdio.c        |  6 ++++
>  .../broadcom/brcm80211/brcmfmac/sdio.h        |  1 +
>  .../broadcom/brcm80211/brcmfmac/usb.c         |  3 ++
>  7 files changed, 66 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c
> index ac02244a6..c4bb32aec 100644
> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c
> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c
> @@ -1043,6 +1043,7 @@ static int brcmf_ops_sdio_probe(struct sdio_func *func,
>  	bus_if = kzalloc(sizeof(struct brcmf_bus), GFP_KERNEL);
>  	if (!bus_if)
>  		return -ENOMEM;
> +	mutex_init(&bus_if->bus_reset_lock);
>  	sdiodev = kzalloc(sizeof(struct brcmf_sdio_dev), GFP_KERNEL);
>  	if (!sdiodev) {
>  		kfree(bus_if);
> @@ -1102,6 +1103,14 @@ static void brcmf_ops_sdio_remove(struct sdio_func *func)
>  		if (func->num != 1)
>  			return;
>  
> +		/* Drain bus_reset before the shared brcmf_sdiod_remove()
> +		 * teardown, which the SDIO reset callback also reaches.  The
> +		 * data worker can arm bus_reset via brcmf_fw_crashed(); cancel
> +		 * it first.
> +		 */
> +		brcmf_sdio_cancel_datawork(sdiodev->bus);
> +		brcmf_bus_cancel_reset_work(bus_if);
> +
>  		/* only proceed with rest of cleanup if func 1 */
>  		brcmf_sdiod_remove(sdiodev);
>  
> @@ -1163,6 +1172,8 @@ static int brcmf_ops_sdio_suspend(struct device *dev)
>  	} else {
>  		/* power will be cut so remove device, probe again in resume */
>  		brcmf_sdiod_intr_unregister(sdiodev);
> +		brcmf_sdio_cancel_datawork(sdiodev->bus);
> +		brcmf_bus_cancel_reset_work(bus_if);
>  		ret = brcmf_sdiod_remove(sdiodev);
>  		if (ret)
>  			brcmf_err("Failed to remove device on suspend\n");
> @@ -1188,6 +1199,8 @@ static int brcmf_ops_sdio_resume(struct device *dev)
>  		ret = brcmf_sdiod_probe(sdiodev);
>  		if (ret)
>  			brcmf_err("Failed to probe device on resume\n");
> +		else
> +			brcmf_bus_allow_reset_work(bus_if);
>  	} else {
>  		if (sdiodev->wowl_enabled &&
>  		    sdiodev->settings->bus.sdio.oob_irq_supported)
> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bus.h b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bus.h
> index 3f5da3bb6..b606094af 100644
> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bus.h
> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bus.h
> @@ -6,6 +6,7 @@
>  #ifndef BRCMFMAC_BUS_H
>  #define BRCMFMAC_BUS_H
>  
> +#include <linux/mutex.h>
>  #include "debug.h"
>  
>  /* IDs of the 6 default common rings of msgbuf protocol */
> @@ -149,11 +150,16 @@ struct brcmf_bus {
>  	u32 chiprev;
>  	bool always_use_fws_queue;
>  	bool wowl_supported;
> +	bool removing;		/* device removal in progress; quiesce async work */
> +	struct mutex bus_reset_lock;
>  
>  	const struct brcmf_bus_ops *ops;
>  	struct brcmf_bus_msgbuf *msgbuf;
>  };
>  
> +void brcmf_bus_cancel_reset_work(struct brcmf_bus *bus_if);
> +void brcmf_bus_allow_reset_work(struct brcmf_bus *bus_if);
> +
>  /*
>   * callback wrappers
>   */
> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
> index fed9cd5f2..b934feb9b 100644
> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
> @@ -1164,6 +1164,35 @@ static int brcmf_revinfo_read(struct seq_file *s, void *data)
>  	return 0;
>  }
>  
> +/* Serialize bus_reset arming (debugfs reset write, brcmf_fw_crashed) against the
> + * teardown drain: the remove path takes bus_reset_lock, sets ->removing and cancels
> + * the work under it, so a racing armer either schedules before the cancel (and is
> + * drained) or observes ->removing and desists.
> + */
> +static void brcmf_bus_schedule_reset(struct brcmf_bus *bus_if)
> +{
> +	mutex_lock(&bus_if->bus_reset_lock);
> +	if (bus_if->drvr && bus_if->drvr->bus_reset.func && !bus_if->removing)
> +		schedule_work(&bus_if->drvr->bus_reset);
> +	mutex_unlock(&bus_if->bus_reset_lock);
> +}

Is this safe in a softIRQ context?                                                                                                        
mutex_lock() sleeps until it can get the lock.

> +
> +void brcmf_bus_cancel_reset_work(struct brcmf_bus *bus_if)
> +{
> +	mutex_lock(&bus_if->bus_reset_lock);
> +	bus_if->removing = true;
> +	if (bus_if->drvr)
> +		cancel_work_sync(&bus_if->drvr->bus_reset);
> +	mutex_unlock(&bus_if->bus_reset_lock);
> +}

How about if brcmf_pcie_remove() calls brcmf_bus_cancel_reset_work(),
takes the lock and calls cancel_work_sync(), sleeps. If debugfs
path is already running, it can invoke the worker thread. Is there
potential that both try to reset?

> +
> +void brcmf_bus_allow_reset_work(struct brcmf_bus *bus_if)
> +{
> +	mutex_lock(&bus_if->bus_reset_lock);
> +	bus_if->removing = false;
> +	mutex_unlock(&bus_if->bus_reset_lock);
> +}
> +
>  static void brcmf_core_bus_reset(struct work_struct *work)
>  {
>  	struct brcmf_pub *drvr = container_of(work, struct brcmf_pub,
> @@ -1184,7 +1213,7 @@ static ssize_t bus_reset_write(struct file *file, const char __user *user_buf,
>  	if (value != 1)
>  		return -EINVAL;
>  
> -	schedule_work(&drvr->bus_reset);
> +	brcmf_bus_schedule_reset(drvr->bus_if);
>  
>  	return count;
>  }
> @@ -1408,7 +1437,7 @@ void brcmf_fw_crashed(struct device *dev)
>  
>  	brcmf_dev_coredump(dev);
>  
> -	schedule_work(&drvr->bus_reset);
> +	brcmf_bus_schedule_reset(bus_if);
>  }
>  
>  void brcmf_detach(struct device *dev)
> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c
> index 8b149996f..3c6775166 100644
> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c
> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c
> @@ -1914,6 +1914,7 @@ brcmf_pcie_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>  		ret = -ENOMEM;
>  		goto fail;
>  	}
> +	mutex_init(&bus->bus_reset_lock);
>  	bus->msgbuf = kzalloc(sizeof(*bus->msgbuf), GFP_KERNEL);
>  	if (!bus->msgbuf) {
>  		ret = -ENOMEM;
> @@ -1985,6 +1986,11 @@ brcmf_pcie_remove(struct pci_dev *pdev)
>  	if (devinfo->ci)
>  		brcmf_pcie_intr_disable(devinfo);
>  
> +	if (devinfo->irq_allocated)
> +		synchronize_irq(pdev->irq);
> +
> +	brcmf_bus_cancel_reset_work(bus);
> +
>  	brcmf_detach(&pdev->dev);
>  	brcmf_free(&pdev->dev);
>  
> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
> index 8effeb7a7..31e37b0d4 100644
> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
> @@ -4541,6 +4541,12 @@ struct brcmf_sdio *brcmf_sdio_probe(struct brcmf_sdio_dev *sdiodev)
>  	return NULL;
>  }
>  
> +void brcmf_sdio_cancel_datawork(struct brcmf_sdio *bus)
> +{
> +	if (bus)
> +		cancel_work_sync(&bus->datawork);
> +}
> +
>  /* Detach and free everything */
>  void brcmf_sdio_remove(struct brcmf_sdio *bus)
>  {
> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.h b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.h
> index 15d2c02fa..3c68ebf8e 100644
> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.h
> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.h
> @@ -373,6 +373,7 @@ int brcmf_sdiod_remove(struct brcmf_sdio_dev *sdiodev);
>  struct brcmf_sdio *brcmf_sdio_probe(struct brcmf_sdio_dev *sdiodev);
>  void brcmf_sdio_remove(struct brcmf_sdio *bus);
>  void brcmf_sdio_isr(struct brcmf_sdio *bus, bool in_isr);
> +void brcmf_sdio_cancel_datawork(struct brcmf_sdio *bus);
>  
>  void brcmf_sdio_wd_timer(struct brcmf_sdio *bus, bool active);
>  void brcmf_sdio_wowl_config(struct device *dev, bool enabled);
> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/usb.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/usb.c
> index 9fb68c2dc..97d65ba36 100644
> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/usb.c
> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/usb.c
> @@ -1271,6 +1271,7 @@ static int brcmf_usb_probe_cb(struct brcmf_usbdev_info *devinfo)
>  		ret = -ENOMEM;
>  		goto fail;
>  	}
> +	mutex_init(&bus->bus_reset_lock);
>  
>  	bus->dev = dev;
>  	bus_pub->bus = bus;
> @@ -1336,6 +1337,8 @@ brcmf_usb_disconnect_cb(struct brcmf_usbdev_info *devinfo)
>  		return;
>  	brcmf_dbg(USB, "Enter, bus_pub %p\n", devinfo);
>  
> +	brcmf_bus_cancel_reset_work(devinfo->bus_pub.bus);
> +
>  	brcmf_detach(devinfo->dev);
>  	brcmf_free(devinfo->dev);
>  	kfree(devinfo->bus_pub.bus);
> -- 
> 2.34.1

Sent using hkml (https://github.com/sjp38/hackermail)

Re: [PATCH] wifi: brcmfmac: drain bus_reset work on device removal

[Fan Wu]

On Thu, Jul 09, 2026 at 05:23:37PM -0700, Eddie Phillips wrote:
> Is this safe in a softIRQ context?
> mutex_lock() sleeps until it can get the lock.

Thanks for taking a look. I agree this is not the right interface shape.

The current call sites I traced reach brcmf_fw_crashed() from the PCIe
threaded IRQ path or from SDIO workqueue context, but brcmf_fw_crashed()
is a core helper and the reset scheduling path should not grow a hidden
sleepable-context requirement. I will respin this part so the arming gate
does not use a mutex.

> How about if brcmf_pcie_remove() calls brcmf_bus_cancel_reset_work(),
> takes the lock and calls cancel_work_sync(), sleeps. If debugfs
> path is already running, it can invoke the worker thread. Is there
> potential that both try to reset?

Yes, that is a real issue with this version. The lock only serializes
scheduling against the cancel point. If the reset work has already started,
cancel_work_sync() waits for it, but the worker may already be executing the
bus reset teardown path; remove can then continue and run its own teardown
afterwards.

So the patch needs a stronger state/ownership model than just
schedule-vs-cancel serialization. I will rework it so reset and remove cannot
both own the same teardown sequence, and will send a v2 after auditing the
PCIe/SDIO/USB paths again.

Thanks,
Fan

Re: [PATCH] wifi: brcmfmac: drain bus_reset work on device removal

[Arend van Spriel]

On 10/07/2026 02:23, Eddie Phillips wrote:
> On Thu,  9 Jul 2026 10:16:35 +0000 Fan Wu <fanwu01@zju.edu.cn> wrote:
> 
>> brcmf_fw_crashed() and the debugfs "reset" entry both schedule
>> drvr->bus_reset, whose callback recovers drvr through container_of()
>> and dereferences it.  The teardown paths free drvr (brcmf_free ->
>> wiphy_free) without draining the work, so a bus_reset callback pending
>> or running during removal can outlive drvr.
>>

[...]

>>
>> This issue was found by an in-house static analysis tool.
>>
>> Fixes: 4684997d9eea ("brcmfmac: reset PCIe bus on a firmware crash")
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Fan Wu <fanwu01@zju.edu.cn>
>> Assisted-by: Codex:gpt-5.5
>> ---
>>   .../broadcom/brcm80211/brcmfmac/bcmsdh.c      | 13 ++++++++
>>   .../broadcom/brcm80211/brcmfmac/bus.h         |  6 ++++
>>   .../broadcom/brcm80211/brcmfmac/core.c        | 33 +++++++++++++++++--
>>   .../broadcom/brcm80211/brcmfmac/pcie.c        |  6 ++++
>>   .../broadcom/brcm80211/brcmfmac/sdio.c        |  6 ++++
>>   .../broadcom/brcm80211/brcmfmac/sdio.h        |  1 +
>>   .../broadcom/brcm80211/brcmfmac/usb.c         |  3 ++
>>   7 files changed, 66 insertions(+), 2 deletions(-)

[...]

>> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
>> index fed9cd5f2..b934feb9b 100644
>> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
>> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
>> @@ -1164,6 +1164,35 @@ static int brcmf_revinfo_read(struct seq_file *s, void *data)
>>   	return 0;
>>   }
>>   
>> +/* Serialize bus_reset arming (debugfs reset write, brcmf_fw_crashed) against the
>> + * teardown drain: the remove path takes bus_reset_lock, sets ->removing and cancels
>> + * the work under it, so a racing armer either schedules before the cancel (and is
>> + * drained) or observes ->removing and desists.
>> + */
>> +static void brcmf_bus_schedule_reset(struct brcmf_bus *bus_if)
>> +{
>> +	mutex_lock(&bus_if->bus_reset_lock);
>> +	if (bus_if->drvr && bus_if->drvr->bus_reset.func && !bus_if->removing)
>> +		schedule_work(&bus_if->drvr->bus_reset);
>> +	mutex_unlock(&bus_if->bus_reset_lock);
>> +}
> 
> Is this safe in a softIRQ context?
> mutex_lock() sleeps until it can get the lock.

What softIRQ context? brcmf_fw_crashed() is called by PCIe (thread) and 
SDIO (worker).
>> +
>> +void brcmf_bus_cancel_reset_work(struct brcmf_bus *bus_if)
>> +{
>> +	mutex_lock(&bus_if->bus_reset_lock);
>> +	bus_if->removing = true;
>> +	if (bus_if->drvr)
>> +		cancel_work_sync(&bus_if->drvr->bus_reset);
>> +	mutex_unlock(&bus_if->bus_reset_lock);
>> +}
> 
> How about if brcmf_pcie_remove() calls brcmf_bus_cancel_reset_work(),
> takes the lock and calls cancel_work_sync(), sleeps. If debugfs
> path is already running, it can invoke the worker thread. Is there
> potential that both try to reset?

What is "both" here?
Regards,
Arend

Re: [PATCH] wifi: brcmfmac: drain bus_reset work on device removal

[Eddie Phillips]

On Fri, Jul 10, 2026 at 12:18 PM Arend van Spriel
<arend.vanspriel@broadcom.com> wrote:
>
> On 10/07/2026 02:23, Eddie Phillips wrote:
> > On Thu,  9 Jul 2026 10:16:35 +0000 Fan Wu <fanwu01@zju.edu.cn> wrote:
> >
> >> brcmf_fw_crashed() and the debugfs "reset" entry both schedule
> >> drvr->bus_reset, whose callback recovers drvr through container_of()
> >> and dereferences it.  The teardown paths free drvr (brcmf_free ->
> >> wiphy_free) without draining the work, so a bus_reset callback pending
> >> or running during removal can outlive drvr.
> >>
>
> [...]
>
> >>
> >> This issue was found by an in-house static analysis tool.
> >>
> >> Fixes: 4684997d9eea ("brcmfmac: reset PCIe bus on a firmware crash")
> >> Cc: stable@vger.kernel.org
> >> Signed-off-by: Fan Wu <fanwu01@zju.edu.cn>
> >> Assisted-by: Codex:gpt-5.5
> >> ---
> >>   .../broadcom/brcm80211/brcmfmac/bcmsdh.c      | 13 ++++++++
> >>   .../broadcom/brcm80211/brcmfmac/bus.h         |  6 ++++
> >>   .../broadcom/brcm80211/brcmfmac/core.c        | 33 +++++++++++++++++--
> >>   .../broadcom/brcm80211/brcmfmac/pcie.c        |  6 ++++
> >>   .../broadcom/brcm80211/brcmfmac/sdio.c        |  6 ++++
> >>   .../broadcom/brcm80211/brcmfmac/sdio.h        |  1 +
> >>   .../broadcom/brcm80211/brcmfmac/usb.c         |  3 ++
> >>   7 files changed, 66 insertions(+), 2 deletions(-)
>
> [...]
>
> >> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
> >> index fed9cd5f2..b934feb9b 100644
> >> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
> >> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
> >> @@ -1164,6 +1164,35 @@ static int brcmf_revinfo_read(struct seq_file *s, void *data)
> >>      return 0;
> >>   }
> >>
> >> +/* Serialize bus_reset arming (debugfs reset write, brcmf_fw_crashed) against the
> >> + * teardown drain: the remove path takes bus_reset_lock, sets ->removing and cancels
> >> + * the work under it, so a racing armer either schedules before the cancel (and is
> >> + * drained) or observes ->removing and desists.
> >> + */
> >> +static void brcmf_bus_schedule_reset(struct brcmf_bus *bus_if)
> >> +{
> >> +    mutex_lock(&bus_if->bus_reset_lock);
> >> +    if (bus_if->drvr && bus_if->drvr->bus_reset.func && !bus_if->removing)
> >> +            schedule_work(&bus_if->drvr->bus_reset);
> >> +    mutex_unlock(&bus_if->bus_reset_lock);
> >> +}
> >
> > Is this safe in a softIRQ context?
> > mutex_lock() sleeps until it can get the lock.
>
> What softIRQ context? brcmf_fw_crashed() is called by PCIe (thread) and
> SDIO (worker).

Yes, you're right. Since it's thread/worker context, sleeping is fine here.

> >> +
> >> +void brcmf_bus_cancel_reset_work(struct brcmf_bus *bus_if)
> >> +{
> >> +    mutex_lock(&bus_if->bus_reset_lock);
> >> +    bus_if->removing = true;
> >> +    if (bus_if->drvr)
> >> +            cancel_work_sync(&bus_if->drvr->bus_reset);
> >> +    mutex_unlock(&bus_if->bus_reset_lock);
> >> +}
> >
> > How about if brcmf_pcie_remove() calls brcmf_bus_cancel_reset_work(),
> > takes the lock and calls cancel_work_sync(), sleeps. If debugfs
> > path is already running, it can invoke the worker thread. Is there
> > potential that both try to reset?
>
> What is "both" here?

It may be possible that the worker thread is running and then the device is
unplugged, causing a deadlock.

Another possibility here would be to just lock the state change, but both
implementations should are fine.

Best, Eddie

> Regards,
> Arend

Re: [PATCH] wifi: brcmfmac: drain bus_reset work on device removal

[Arend van Spriel]

On Thu,  9 Jul 2026 10:16:35 +0000, Fan Wu wrote:
> brcmf_fw_crashed() and the debugfs "reset" entry both schedule
> drvr->bus_reset, whose callback recovers drvr through container_of()
> and dereferences it.

[...]

The patch does not apply cleanly on wl-next/main -- a rebase is needed.

I looked more carefully at the cancel_work_sync-under-lock concern that was
raised in the thread. brcmf_core_bus_reset() never acquires bus_reset_lock,
so there is no deadlock. If the work has already started when
brcmf_bus_cancel_reset_work() is called, cancel_work_sync() waits for it to
finish; by then the reset op (brcmf_pcie_reset) has completed its own
teardown and reinitialized the device, so the subsequent remove teardown acts
on a clean device. The design is correct as submitted; only the rebase is
needed.

Regards,
Arend