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@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).
> > +
> > +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