Message-ID: <14ce82c5-f807-59bd-30ec-7eff5066e4f0@broadcom.com>
Date:   Thu, 12 Oct 2023 10:41:33 +0200
From:   Arend van Spriel <arend.vanspriel@...adcom.com>
To:     Hector Martin <marcan@...can.st>, Kalle Valo <kvalo@...nel.org>,
        Neal Gompa <neal@...pa.dev>
Cc:     Dmitry Antipov <dmantipov@...dex.ru>,
        "linux-wireless@...r.kernel.org" <linux-wireless@...r.kernel.org>,
        "lvc-project@...uxtesting.org" <lvc-project@...uxtesting.org>,
        Arend van Spriel <aspriel@...il.com>,
        Franky Lin <franky.lin@...adcom.com>,
        Hante Meuleman <hante.meuleman@...adcom.com>,
        SHA-cyfmac-dev-list@...ineon.com,
        brcm80211-dev-list.pdl@...adcom.com,
        Asahi Linux <asahi@...ts.linux.dev>,
        LKML <linux-kernel@...r.kernel.org>,
        Julian Calaby <julian.calaby@...il.com>,
        Linus Torvalds <torvalds@...ux-foundation.org>,
        Phil Elwell <phil@...pberrypi.org>
Subject: Re: On brcm80211 maintenance and support

On 10/11/2023 1:28 PM, 'Hector Martin' via BRCM80211-DEV-LIST,PDL wrote:
> 
> 
> On 2023/10/11 19:23, Kalle Valo wrote:
>> Neal Gompa <neal@...pa.dev> writes:
>>
>>> On Sat, Oct 7, 2023 at 8:51 AM Hector Martin <marcan@...can.st> wrote:
>>>
>>>>
>>>> On 07/10/2023 00.48, Dmitry Antipov wrote:
>>>>> On 10/6/23 18:34, Hector Martin wrote:
>>>>>
>>>>>> For better or worse, if nobody else does, I'm willing to sign up to
>>>>>> maintain the chips shipping on Apple ARM64 machines (i.e. BCM4378,
>>>>>> BCM4387, BCM4388 - that last one I have bringup for downstream, just got
>>>>>> it done this week) and partially BCM4377 as a bonus (since I have access
>>>>>> to an older Intel Mac with that one, and already did bringup for it,
>>>>>> though my access is sporadic). I'm already playing part time maintainer
>>>>>> anyway (other folks have already sent us patches I'll have to upstream),
>>>>>> and we need this driver to keep working and continue to support new chips.
>>>>>
>>>>> Good news. Would you capable to consider some generic (not hooked to any
>>>>> particular hardware) things like [1] ?
>>>>>
>>>>> [1]
>>>>> https://lore.kernel.org/linux-wireless/20230703162458.155942-1-dmantipov@yandex.ru/
>>>>>
>>>>
>>>> Sure, I've done cleanup type stuff myself too.
>>>>
>>>
>>> Can we please get this done so that the pile of Broadcom patches can
>>> actually start landing again? It's been frustrating watching patch
>>> submissions be ignored for over a year now. At least add Hector as a
>>> co-maintainer and allow him to land stuff people have been using
>>> outside to get Broadcom Wi-Fi to *work*.
>>>
>>> Having stuff sit on the pile and be *ignored* is frustrating for
>>> contributors and users, and massively disincentivizes people from
>>> working in upstream Linux.
>>
>> Your email reminds me of this comic:
>>
>> https://xkcd.com/2347/
>>
>> In the last few years we seem to be getting more of these "Work faster!"
>> emails and honestly it's getting frustrating for us maintainers. If
>> Linux wireless is important for you then help us! You can review
>> patches, run tests on real hardware, write hwsim test cases[1], fix
>> compiler warnings[2] etc. to help us maintainers and speed up
>> development. There's so much to do and while you gain experience on the
>> wireless development you can help even more.
>>
>> Also take it into account that it's not just simple to "take patches"
>> and be done with it. There are high quality requirements, the code needs
>> to have no compiler warnings and must not cause any regressions in
>> existing setups. That's not easy at all, especially as our hardware
>> testing is basically limited to few the most active drivers. And let
>> alone there are very exotic hardware out there and it's impossible to
>> test all of them.
> 
> I think we need to qualify this "no regressions" "rule". People regress
> our machines in mainline all the time. We catch it and get it fixed,
> sometimes in RCs, sometimes it goes all the way to stable and needs to
> get fixed there. We've had patches break everything from Bluetooth LE
> pairing to core memory management to the point we needed earlycon to
> diagnose it. That last one was a blatantly wrong patch to core Linux MM,
> it wasn't even something specific to our platform, but even that got
> past review (it just happened to break us specifically due to a
> coincidence).
> 
> The review process doesn't magically avoid regressing things. "No
> regressions" is impossible without someone actually testing things.
> Claiming otherwise is dishonest. So if I end up as maintainer here I
> certainly am not going to promise "no regressions" for chips I can't
> test, without someone interested in those chips testing them. Of course
> I'll take regression fixes when they do happen and someone notices, but
> I can't know in advance until someone does.

I do not think Kalle was claiming that. Regression will happen as it is 
simply not possible to verify each patch on all devices supported by the 
driver.

> Consider a patch that changes some codepath in the driver. I can't know
> whether the original logic was always broken, or whether it worked on
> some chips, and whether the new logic works on those chips or will
> regress them, without testing. This is a regular occurrence with
> brcmfmac, due to the complexity and variability of the firmware
> interface. Often multiple versions of stuff are supported, or some
> structures can be extended, but sometimes they can't. It's a mess, and
> without firmware source code nor any official specs, there is no way to
> know exactly what is intended and what the backwards compatibility
> requirements are.
> 
> The only way to avoid that is to gratuitously introduce version/chip
> gates for *every single change affecting behavior from the firmware
> POV*, which is a complete non-starter and would quickly yield a giant
> mess of spaghetti code. It's bad enough having to support explicit
> ABI-breaking changes in firmware, and having to deal with multiple
> versions of huge structures and convert between them. Trying to outright
> keep existing logic identical for "other chips" is just not going to
> happen, not without first having confirmation of what the requirements
> are from someone who has the required docs/source.

> I have a patch to enable WPA3 in Broadcom chipsets (yes, the driver is
> in such a sorry state it doesn't even support that yet). The current
> support attempt was added by a Cypress engineer and uses a completely
> different firmware mechanism. Is that supposed to actually work? Does it
> work currently? Is that the case for all Cypress firmwares? Or only
> some? Does the alternate mechanism we have for Broadcom chips work too?
> Only Cypress can answer those questions ahead of time, and they aren't
> (they ignored me last time I brought this up). So my current patch just
> replaces the mechanism with the known-working one for Broadcom chips.

This is mainly why I introduced the vendor-split concept so we can keep 
the Cypress mechanism and allow a different mechanism for Broadcom 
chips. The Cypress mechanism did not work for the Broadcom chips I have 
so I wanted to test it on the Cypress chips I got shipped long ago and 
they simply do not come up. Have not tried with RPi as it is not running 
vanilla kernel. Could try with a backports driver.

> Next time I send a bunch of our downstream patches, I'm going to resend
> that WPA3 patch. And if it regresses Cypress WPA3 support, tough luck.
> If someone catches it (Phil?) and it turns out the existing support is
> the only way to do things on Cypress, I'll rework the patch to
> conditionally support both styles. But if nobody does, and nobody cares,
> then I'm sorry if I regress things, but there is no way to avoid it. We
> can't be gratuitously gating every single change just to hope to avoid
> regressions, without any confirmation of what is required. That just
> doesn't scale.

The "no regressions" rule is an important one throughout the kernel. 
Trying to avoid it makes sense, but I have to agree that for brcmfmac 
there is not enough coverage to give any guarantee. As long you are 
prepared to fix things when a regression report comes in that should be 
fine depending on how often there is a need to revert/fix things as it 
can disrupt the normal flow of patches.

Regards,
Arend

Download attachment "smime.p7s" of type "application/pkcs7-signature" (4219 bytes)

Powered by blists - more mailing lists