Re: [PATCH 4/4] wifi: brcmfmac: Fix firmware requests racing against SDIO removal
From: Sean Anderson
Date: Tue Sep 22 2026 - 10:04:52 EST
On 9/21/26 5:18 PM, Sean Anderson wrote:
brcmf_sdio_firmware_callback can race with device removal. If this
happens it can re-register IRQs, dereference NULL pointers, and cause
all sorts of havoc. Prevent this by canceling any outstanding firmware
request as the first step of the removal process.
When canceling the firmware request, we primarily need to ensure fwctx
remains valid for all our calls to request_firmware_nowait_cancel. If we
let it get free'd early then it could get re-used for some unrelated
firmware request. To avoid this, we follow the same pattern that
firmware_loader does. But while firmware_loader needs a spinlock, we can
get away with a single pointer:
- When ctxp is NULL, then we can't be canceled
- When *ctxp == fwctx, we're still alive
- When *ctxp == NULL, someone else has canceled the request (or we have
run our natural course).
- Whoever clears ctxp is responsible for freeing fwctx.
There can be up to NR_CPUS requests in-flight at any given time, as each
(alt) firmware request can create a new firmware request. Eventually,
one of them will see that ctxp is cleared and stop spawning additional
requests.
If the firmware load fails while we are removing the device, we can no
longer attempt to call device_release_driver. This will deadlock. We
can't do this asynchronously either since we run the risk of releasing a
totally different driver/device combo. We could techincally do this by
dropping device_lock before waiting for the firmware to cancel, but that
seems like a major headache (we would need to make ctxp a separate
reference-counted allocation).
I also implemented this fix for PCIe but I have only build-tested it.
I didn't touch USB as it already uses a completion-based system to
determine when it's OK to remove the driver. I didn't go with this
approach because we could wait indefinitely for the firmware request to
complete (such as if the firmware is on a slow device or loaded by
userspace).
Fixes: bd0e1b1d380e ("brcmfmac: use asynchronous firmware request in SDIO")
Signed-off-by: Sean Anderson <sanderson@xxxxxxxxx>
---
.../broadcom/brcm80211/brcmfmac/bcmsdh.c | 7 ++++-
.../broadcom/brcm80211/brcmfmac/bus.h | 2 ++
.../broadcom/brcm80211/brcmfmac/firmware.c | 26 ++++++++++++++++---
.../broadcom/brcm80211/brcmfmac/firmware.h | 16 +++++++++++-
.../broadcom/brcm80211/brcmfmac/pcie.c | 11 +++++---
.../broadcom/brcm80211/brcmfmac/sdio.c | 8 +++---
.../broadcom/brcm80211/brcmfmac/usb.c | 6 +++--
7 files changed, 63 insertions(+), 13 deletions(-)
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c
index 71c2f99cdb711..39916f5a699dd 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c
@@ -1126,7 +1126,12 @@ static void brcmf_ops_sdio_remove(struct sdio_func *func)
if (bus_if) {
sdiodev = bus_if->bus_priv.sdio;
- /* start by unregistering irqs */
+ /* Cancel any outstanding firmware request, as it may try to
+ * call brcmf_sdiod_intr_register.
+ */
+ brcmf_fw_cancel(sdiodev->dev, &bus_if->fwctx);
+
+ /* Now we can unregister irqs */
brcmf_sdiod_intr_unregister(sdiodev);
if (func->num != 1)
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bus.h b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bus.h
index 9371c1489948c..81c17a8c235cc 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bus.h
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bus.h
@@ -157,6 +157,7 @@ struct brcmf_bus_stats {
* @chip: device identifier of the dongle chip.
* @chiprev: revision of the dongle chip.
* @fwvid: firmware vendor-support identifier of the device.
+ * @fwctx: firmware request cancellation context
* @always_use_fws_queue: bus wants use queue also when fwsignal is inactive.
* @wowl_supported: is wowl supported by bus driver.
* @ops: callbacks for this bus instance.
@@ -178,6 +179,7 @@ struct brcmf_bus {
u32 chip;
u32 chiprev;
enum brcmf_fwvendor fwvid;
+ void *fwctx;
bool always_use_fws_queue;
bool wowl_supported;
bool removing; /* device removal in progress; quiesce async work */
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c
index 22ff326f1924a..d1da65155e544 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c
@@ -459,6 +459,7 @@ struct brcmf_fw {
u32 curpos;
unsigned int board_index;
void (*done)(struct device *dev, int err, struct brcmf_fw_request *req);
+ void **ctxp;
};
#ifdef CONFIG_EFI
@@ -693,7 +694,7 @@ static void brcmf_fw_request_done(const struct firmware *fw, void *ctx)
fwctx->req = NULL;
}
fwctx->done(fwctx->dev, ret, fwctx->req);
- kfree(fwctx);
+ kfree(xchg(fwctx->ctxp, NULL));
}
static void brcmf_fw_request_done_alt_path(const struct firmware *fw, void *ctx)
@@ -703,7 +704,7 @@ static void brcmf_fw_request_done_alt_path(const struct firmware *fw, void *ctx)
const char *board_type, *alt_path;
int ret = 0;
- if (fw) {
+ if (fw || !READ_ONCE(*fwctx->ctxp)) {
brcmf_fw_request_done(fw, ctx);
return;
}
@@ -755,7 +756,8 @@ static bool brcmf_fw_request_is_valid(struct brcmf_fw_request *req)
int brcmf_fw_get_firmwares(struct device *dev, struct brcmf_fw_request *req,
void (*fw_cb)(struct device *dev, int err,
- struct brcmf_fw_request *req))
+ struct brcmf_fw_request *req),
+ void **ctxp)
{
struct brcmf_fw_item *first = &req->items[0];
struct brcmf_fw *fwctx;
@@ -776,6 +778,8 @@ int brcmf_fw_get_firmwares(struct device *dev, struct brcmf_fw_request *req,
fwctx->dev = dev;
fwctx->req = req;
fwctx->done = fw_cb;
+ fwctx->ctxp = ctxp;
+ WRITE_ONCE(*ctxp, fwctx);
/* First try alternative board-specific path if any */
if (fwctx->req->board_types[0])
@@ -799,6 +803,22 @@ int brcmf_fw_get_firmwares(struct device *dev, struct brcmf_fw_request *req,
return 0;
}
+void brcmf_fw_cancel(struct device *dev, void **ctxp)
+{
+ struct brcmf_fw *fwctx;
+
+ fwctx = xchg(ctxp, NULL);
+ if (!fwctx)
+ return;
+
+ /* Keep canceling requests until they see that we cleared ctxp */
+ while (request_firmware_nowait_cancel(dev, fwctx,
+ brcmf_fw_request_done_alt_path))
+ ;
+ request_firmware_nowait_cancel(dev, fwctx, brcmf_fw_request_done);
+ kfree(fwctx);
+}
+
struct brcmf_fw_request *
brcmf_fw_alloc_request(u32 chip, u32 chiprev,
const struct brcmf_firmware_mapping mapping_table[],
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.h b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.h
index 4002d326fd21b..932899d4086b3 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.h
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.h
@@ -87,9 +87,23 @@ brcmf_fw_alloc_request(u32 chip, u32 chiprev,
* Request firmware(s) asynchronously. When the asynchronous request
* fails it will not use the callback, but call device_release_driver()
* instead which will call the driver .remove() callback.
+ *
+ * ctxp is a (pointer to an) opaque pointer that may be passed to
+ * brcmf_fw_cancel().
*/
int brcmf_fw_get_firmwares(struct device *dev, struct brcmf_fw_request *req,
void (*fw_cb)(struct device *dev, int err,
- struct brcmf_fw_request *req));
+ struct brcmf_fw_request *req),
+ void **ctxp);
+
+/**
+ * brcmf_fw_cancel() - Cancel an outstanding firmware request
+ * @dev: Device requesting the firmware
+ * @ctxp: Opaque context pointer filled in by brcmf_fw_get_firmwares()
+ *
+ * Cancel an outstanding firmware request identified by @dev and @ctxp, which
+ * should be the same as passed to brcmf_fw_get_firmwares().
+ */
+void brcmf_fw_cancel(struct device *dev, void **ctxp);
#endif /* BRCMFMAC_FIRMWARE_H */
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c
index 55f4d7b970f28..9eae712b0d98c 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c
@@ -1555,6 +1555,8 @@ static int brcmf_pcie_reset(struct device *dev)
struct brcmf_fw_request *fwreq;
int err;
+ brcmf_fw_cancel(dev, &bus_if->fwctx);
+
brcmf_pcie_intr_disable(devinfo);
brcmf_pcie_bus_console_read(devinfo, true);
@@ -1572,7 +1574,8 @@ static int brcmf_pcie_reset(struct device *dev)
return -ENOMEM;
}
- err = brcmf_fw_get_firmwares(dev, fwreq, brcmf_pcie_setup);
+ err = brcmf_fw_get_firmwares(dev, fwreq, brcmf_pcie_setup,
+ &bus_if->fwctx);
if (err) {
dev_err(dev, "Failed to prepare FW request\n");
kfree(fwreq);
@@ -2231,7 +2234,6 @@ static void brcmf_pcie_setup(struct device *dev, int ret,
brcmf_err(bus, "Dongle setup failed\n");
brcmf_pcie_bus_console_read(devinfo, true);
brcmf_fw_crashed(dev);
- device_release_driver(dev);
}
static struct brcmf_fw_request *
@@ -2554,7 +2556,8 @@ brcmf_pcie_probe(struct pci_dev *pdev, const struct pci_device_id *id)
goto fail_brcmf;
}
- ret = brcmf_fw_get_firmwares(bus->dev, fwreq, brcmf_pcie_setup);
+ ret = brcmf_fw_get_firmwares(bus->dev, fwreq, brcmf_pcie_setup,
+ &bus->fwctx);
if (ret < 0) {
kfree(fwreq);
goto fail_brcmf;
@@ -2595,6 +2598,8 @@ brcmf_pcie_remove(struct pci_dev *pdev)
brcmf_pcie_bus_console_read(devinfo, false);
brcmf_pcie_fwcon_timer(devinfo, false);
+ brcmf_fw_cancel(bus->dev, &bus->fwctx);
+
devinfo->state = BRCMFMAC_PCIE_STATE_DOWN;
if (devinfo->ci)
brcmf_pcie_intr_disable(devinfo);
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
index 381801af3ac98..1c32fe5828a2f 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
@@ -4417,8 +4417,6 @@ static void brcmf_sdio_firmware_callback(struct device *dev, int err,
sdio_release_host(sdiod->func1);
fail:
brcmf_dbg(TRACE, "failed: dev=%s, err=%d\n", dev_name(dev), err);
- device_release_driver(&sdiod->func2->dev);
- device_release_driver(dev);
}
static struct brcmf_fw_request *
@@ -4546,7 +4544,8 @@ int brcmf_sdio_probe(struct brcmf_sdio_dev *sdiodev)
}
ret = brcmf_fw_get_firmwares(sdiodev->dev, fwreq,
- brcmf_sdio_firmware_callback);
+ brcmf_sdio_firmware_callback,
+ &sdiodev->bus_if->fwctx);
if (ret != 0) {
brcmf_err("async firmware request failed: %d\n", ret);
kfree(fwreq);
@@ -4581,6 +4580,9 @@ void brcmf_sdio_remove(struct brcmf_sdio *bus)
bus->watchdog_tsk = NULL;
}
+ brcmf_fw_cancel(bus->sdiodev->dev,
+ &bus->sdiodev->bus_if->fwctx);
+
/* De-register interrupt handler */
brcmf_sdiod_intr_unregister(bus->sdiodev);
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/usb.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/usb.c
index b41949a9bdc8e..e8c0db0c82689 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/usb.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/usb.c
@@ -1306,7 +1306,8 @@ static int brcmf_usb_probe_cb(struct brcmf_usbdev_info *devinfo,
}
/* request firmware here */
- ret = brcmf_fw_get_firmwares(dev, fwreq, brcmf_usb_probe_phase2);
+ ret = brcmf_fw_get_firmwares(dev, fwreq, brcmf_usb_probe_phase2,
+ &bus->fwctx);
if (ret) {
brcmf_err("firmware request failed: %d\n", ret);
kfree(fwreq);
@@ -1524,7 +1525,8 @@ static int brcmf_usb_reset_resume(struct usb_interface *intf)
if (!fwreq)
return -ENOMEM;
- ret = brcmf_fw_get_firmwares(&usb->dev, fwreq, brcmf_usb_probe_phase2);
+ ret = brcmf_fw_get_firmwares(&usb->dev, fwreq, brcmf_usb_probe_phase2,
+ &devinfo->bus_pub->bus->fwctx);
This should be devinfo->bus_pub.bus->fwctx. Forgot to add the hunk before sending.
--Sean
if (ret < 0)
kfree(fwreq);