Re: [PATCH AUTOSEL 5.4 39/52] brcmfmac: properly check for bus register errors

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

Resending without disclaimer

On 5/25/2021 9:04 AM, Arend Van Spriel wrote:
> 
> 
> On 5/25/2021 8:43 AM, Greg Kroah-Hartman wrote:
>> On Tue, May 25, 2021 at 08:38:34AM +0200, Arend van Spriel wrote:
>>> On 5/24/2021 4:48 PM, Sasha Levin wrote:
>>>> From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>>>>
>>>> [ Upstream commit 419b4a142a7ece36cebcd434f8ce2af59ef94b85 ]
>>>>
>>>> The brcmfmac driver ignores any errors on initialization with the
>>>> different busses by deferring the initialization to a workqueue and
>>>> ignoring all possible errors that might happen.  Fix up all of this by
>>>> only allowing the module to load if all bus registering worked 
>>>> properly.
>>>
>>> Hi Greg,
>>>
>>> Saw this one flying by for stable kernel. Actually the first time I 
>>> saw this
>>> patch, because I don't follow LKML as much as linux-wireless. The 
>>> patch is
>>> fine, but wanted to give some context on the workqueue approach. It was
>>> there for historic reasons. Back then we had the UMH to provide firmware
>>> loading and because we request firmware during driver probe we could 
>>> cause
>>> kernel boot to show significant delay when driver was built-in. Hence 
>>> the
>>> workqueue which allowed kernel boot to proceed and driver probe was 
>>> running
>>> in another thread context. These days we have direct firmware loading 
>>> from
>>> the kernel and brcmfmac uses the asynchronous firmware loading API so 
>>> there
>>> is indeed no longer a need for the workqueue.
>>>
>>> Just for my understanding could you explain the motivation behind this
>>> change. In the preceding revert patch I saw this remark:
>>>
>>> """
>>> The original commit here did nothing to actually help if usb_register()
>>> failed, so it gives a "false sense of security" when there is none.  The
>>> correct solution is to correctly unwind from this error.
>>> """
>>>
>>> Does this mean the patch is addressing some security issue. Before your
>>> patch the module would remain loaded despite a bus register failure. 
>>> I guess
>>> there is a story behind this that I am curious about.
>>
>> The module would remain loaded, yes, but nothing would work, and so no
>> one would have any idea that something went wrong.  The original commit
>> was wrong, it did not actually solve anything.
> 
> Agree.
> 
>> This commit properly propagates any error that happens back to the user,
>> like any other module being loaded.
> 
> I understand, but this might cause a regression for the user. For 
> instance if the usb_register() fails, but the other driver registrations 
> succeed and the user has a wireless PCIe device. Before this change the 
> user would have a functioning wifi device, but with this change it does 
> not?
> 
> Regards,
> Arend

Sorry for that. Corporate email can be a drag.

Regards,
Arend