Re: [PATCH] brcmfmac: change the order of things in brcmf_detach()

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

On July 20, 2019 6:26:30 PM Rafał Miłecki <rafal@milecki.pl> wrote:

> On 30.04.2019 12:10, Arend Van Spriel wrote:
>> On 4/30/2019 10:11 AM, Piotr Figiel wrote:
>>> Hi Arend,
>>>
>>> On Mon, Apr 29, 2019 at 12:09:21PM +0200, Arend van Spriel wrote:
>>>> When brcmf_detach() from the bus layer upon rmmod we can no longer
>>>> communicate. Hence we will set the bus state to DOWN and cleanup
>>>> the event and protocol layer. The network interfaces need to be
>>>> deleted before brcmf_cfg80211_detach() because the latter does the
>>>> wiphy_unregister() which issues a warning if there are still network
>>>> devices linked to the wiphy instance.
>>>
>>> This seems to already happen - brcmf_cfg80211_detach() is called after the
>>> interfaces are removed.
>>
>> Right. This was just to remind me why brcmf_cfg80211_detach() must be 
>> called after removing the interfaces.
>>
>>>> This change solves a null pointer dereference issue which happened
>>>> upon issueing rmmod while there are packets queued in bus protocol
>>>> layer.
>>>>
>>>>
>>>> Reported-by: Rafał Miłecki <rafal@milecki.pl>
>>>> Reviewed-by: Hante Meuleman <hante.meuleman@broadcom.com>
>>>> Reviewed-by: Pieter-Paul Giesberts <pieter-paul.giesberts@broadcom.com>
>>>> Reviewed-by: Franky Lin <franky.lin@broadcom.com>
>>>> Signed-off-by: Arend van Spriel <arend.vanspriel@broadcom.com>
>>>> ---
>>>> Hi Piotr,
>>>>
>>>>
>>>> While working on an issue with msgbuf protocol (used for PCIe devices)
>>>> your change 5cdb0ef6144f ("brcmfmac: fix NULL pointer derefence during
>>>> USB disconnect") conflicted. I suspect my reordering stuff in
>>>> brcmf_detach() also fixes your issue so could you retest this patch,
>>>> which basically reverts your change and applies my reordering, and see
>>>> whether my suspicion can be confirmed.
>>>
>>> Does the issue reported by Rafał you are trying to solve with this patch occur
>>> on current linux-next?
>>
>> Looking at you patch I suspect it does, because brcmf_proto_msgbuf_detach() 
>> is invoked in brcmf_proto_detach_post_delif(). However, I could not 
>> reproduce it with or without the patch.
>>
>> Rafał,
>>
>> Do you know whether your reported issue, ie. calling brcmf_tx_finalize() 
>> after interfaces were removed, still exists in wireless-testing (or 
>> linux-next).
>
> Sorry for a terribly late reply. It took me many attempts to crash a
> firmware in a fully reproducible way.
>
> I can say for sure this patch fixes crashes in brcmf_txfinalize() I saw
> when unloading brcmfmac after a crash.

Thanks, Rafał

There was no deadline so no need to feel sorry.

Regards,
Arend