Re: [PATCH] wifi: brcmfmac: drain bus_reset work on device removal

[Arend van Spriel <arend.vanspriel@broadcom.com>]

On 10/07/2026 02:23, Eddie Phillips wrote:
> 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.
>>

[...]

>>
>> 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/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