On 5/25/2021 9:26 AM, Greg Kroah-Hartman wrote:
On Tue, May 25, 2021 at 09:23:41AM +0200, Arend van Spriel wrote:
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@xxxxxxxxxxxxxxxxxxx>
[ 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?
If registering one of those other busses fails, you have major system
problems that need to be resolved and lots of other things will also
break.
Right.
You shouldn't just "eat error messages" and ignore them, as that's what
is what was happening here, you could have had errors and never knew it.
As said earlier I agree with the patch. I just thought I might learn
something today, because there was more to it than I could find in the
commit message. :-p
Regards,
Arend
