Re: [PATCH] wifi: brcmfmac: Fix potential kernel oops when probe fails

Op 2 februari 2026 19:31:17 schreef Arend van Spriel 
<[email protected]>:

> Op 15 januari 2026 00:16:31 schreef Marek Szyprowski 
> <[email protected]>:
>
>> On 14.01.2026 14:45, Arend van Spriel wrote:
>>> On 12/31/2025 3:35 PM, Marek Szyprowski wrote:
>>>> When probe of the sdio brcmfmac device fails for some reasons (i.e.
>>>> missing firmware), the sdiodev->bus is set to error instead of NULL,
>>>> thus
>>>> the cleanup later in brcmf_sdio_remove() tries to free resources via
>>>> invalid bus pointer. Fix this.
>>>
>>> Hi Marek,
>>>
>>> Thanks for the fix. Please consider my suggestion below...
>>>
>>>> Fixes: 0ff0843310b7 ("wifi: brcmfmac: Add optional lpo clock enable
>>>> support")
>>>> Signed-off-by: Marek Szyprowski <[email protected]>
>>>> ---
>>>> drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c | 1 +
>>>> 1 file changed, 1 insertion(+)
>>>>
>>>> diff --git
>>>> a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c
>>>> b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c
>>>> index 6a3f187320fc..6615748fa5bb 100644
>>>> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c
>>>> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c
>>>> @@ -954,6 +954,7 @@ int brcmf_sdiod_probe(struct brcmf_sdio_dev
>>>> *sdiodev)
>>>
>>> Maybe use a local variable bus and assign it only on success:
>>>
>>>> bus = brcmf_sdio_probe(sdiodev);
>>>> if (IS_ERR(bus)) {
>>>>   ret = PTR_ERR(bus);
>>>>   goto out;
>>>> }
>>>> +    sdiodev->bus = bus;
>>>> brcmf_sdiod_host_fixup(sdiodev->func2->card->host);
>> Well, that would look much better, but sdiodev->bus is also assigned
>> inside brcmf_sdio_probe() and I didn't check if this is required by the
>> functions called there or not. Maybe brcmf_sdio_probe() should simply
>> return error code to make things easier to track?

Correcting some typos here...

>>
> Sorry for the late response. Indeed the bus instance is allocated and 
> assigned to sdiodev

...in brcmf_sdio_probe.

> So that is

*NOT*

> something the caller needs to repeat. So changing the return type of 
> brcmf_sdio_probe() makes sense. The only thing that should be taken care of 
> in the failure path of brcmf_sdio_probe() is to set sdiodev->bus to NULL 
> after calling brcmf_sdio_remove().

Regards,
Arend