Re: [PATCH 4/4] wifi: brcmfmac: Fix firmware requests racing against SDIO removal

[Sean Anderson <sanderson@brivo.com>]

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@brivo.com>
> ---
> 
>   .../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);
>