From: Eddie Phillips <eddiephillips@google.com>
To: eddiephillips@google.com, Fan Wu <fanwu01@zju.edu.cn>
Cc: Arend van Spriel <aspriel@gmail.com>,
Kalle Valo <kvalo@kernel.org>,
Franky Lin <franky.lin@broadcom.com>,
Hante Meuleman <hante.meuleman@broadcom.com>,
Chi-Hsien Lin <chi-hsien.lin@infineon.com>,
Wright Feng <wright.feng@infineon.com>,
Chung-Hsien Hsu <chung-hsien.hsu@infineon.com>,
"David S . Miller" <davem@davemloft.net>,
Jakub Kicinski <kuba@kernel.org>,
linux-wireless@vger.kernel.org,
brcm80211-dev-list.pdl@broadcom.com,
SHA-cyfmac-dev-list@infineon.com, netdev@vger.kernel.org,
linux-kernel@vger.kernel.org, stable@vger.kernel.org
Subject: Re: [PATCH] wifi: brcmfmac: drain bus_reset work on device removal
Date: Thu, 9 Jul 2026 17:23:37 -0700 [thread overview]
Message-ID: <20260710002451.500112-1-eddiephillips@google.com> (raw)
In-Reply-To: <20260709101635.103005-1-fanwu01@zju.edu.cn>
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)
next prev parent reply other threads:[~2026-07-10 0:24 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2026-07-09 10:16 [PATCH] wifi: brcmfmac: drain bus_reset work on device removal Fan Wu
2026-07-10 0:23 ` Eddie Phillips [this message]
2026-07-10 2:21 ` Fan Wu
2026-07-10 19:18 ` Arend van Spriel
2026-07-11 5:23 ` Eddie Phillips
2026-07-12 21:57 ` Arend van Spriel
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20260710002451.500112-1-eddiephillips@google.com \
--to=eddiephillips@google.com \
--cc=SHA-cyfmac-dev-list@infineon.com \
--cc=aspriel@gmail.com \
--cc=brcm80211-dev-list.pdl@broadcom.com \
--cc=chi-hsien.lin@infineon.com \
--cc=chung-hsien.hsu@infineon.com \
--cc=davem@davemloft.net \
--cc=fanwu01@zju.edu.cn \
--cc=franky.lin@broadcom.com \
--cc=hante.meuleman@broadcom.com \
--cc=kuba@kernel.org \
--cc=kvalo@kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-wireless@vger.kernel.org \
--cc=netdev@vger.kernel.org \
--cc=stable@vger.kernel.org \
--cc=wright.feng@infineon.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox