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@xxxxxxxxxxxx> wrote:
>
> On 10/07/2026 02:23, Eddie Phillips wrote:
> > On Thu,  9 Jul 2026 10:16:35 +0000 Fan Wu <fanwu01@xxxxxxxxxx> 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@xxxxxxxxxxxxxxx
> >> Signed-off-by: Fan Wu <fanwu01@xxxxxxxxxx>
> >> 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