Re: [PATCH] wifi: brcmfmac: cfg80211: Use WSEC to set SAE password

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

[-- Attachment #1: Type: text/plain, Size: 13313 bytes --]

On 12/22/2023 6:10 AM, Hector Martin wrote:
> 
> 
> On 2023/12/21 18:57, Arend van Spriel wrote:
>> - SHA-cyfmac-dev-list@infineon.com
>>
>> On 12/21/2023 1:49 AM, Hector Martin wrote:
>>>
>>>
>>> On 2023/12/21 4:36, Arend van Spriel wrote:
>>>> On 12/20/2023 7:14 PM, Hector Martin wrote:
>>>>>
>>>>>
>>>>> On 2023/12/20 19:20, Kalle Valo wrote:
>>>>>> Linus Torvalds <torvalds@linux-foundation.org> writes:
>>>>>>
>>>>>>>> Just recently a patch was posted to remove the Infineon list from
>>>>>>>> MAINTAINERS because that company cares so little they have literally
>>>>>>>> stopped accepting emails from us. Meanwhile they are telling their
>>>>>>>> customers that they do not recommend upstream brcmfmac and they should
>>>>>>>> use their downstream driver [1].
>>>>>>>
>>>>>>> Unquestionably broadcom is not helping maintain things, and I think it
>>>>>>> should matter.
>>>>>>>
>>>>>>> As Hector says, they point to their random driver dumps on their site
>>>>>>> that you can't even download unless you are a "Broadcom community
>>>>>>> member" or whatever, and hey - any company that works that way should
>>>>>>> be seen as pretty much hostile to any actual maintenance and proper
>>>>>>> development.
>>>>>>
>>>>>> Sadly this is the normal in the wireless world. All vendors focus on the
>>>>>> latest generation, currently it's Wi-Fi 7, and lose interest on older
>>>>>> generations. And vendors lose focus on the upstream drivers even faster,
>>>>>> usually after a customer project ends.
>>>>>>
>>>>>> So in practise what we try to do is keep the drivers working somehow on
>>>>>> our own, even after the vendors are long gone. If we would deliberately
>>>>>> allow breaking drivers because vendor/corporations don't support us, I
>>>>>> suspect we would have sevaral broken drivers in upstream.
>>>>>>
>>>>>>> If Daniel and Hector are responsive to actual problem reports for the
>>>>>>> changes they cause, I do think that should count a lot.
>>>>>>
>>>>>> Sure, but they could also respect to the review comments. I find Arend's
>>>>>> proposal is reasonable and that's what I would implement in v2. We
>>>>>> (linux-wireless) make abstractions to workaround firmware problems or
>>>>>> interface conflicts all the time, just look at ath10k for example. I
>>>>>> would not be surprised if we need to add even more abstractions to
>>>>>> brcmfmac in the future. And Arend is the expert here, he has best
>>>>>> knowledge of Broadcom devices and I trust him.
>>>>>>
>>>>>> Has anyone even investigated what it would need to implement Arend's
>>>>>> proposal? At least I don't see any indication of that.
>>>>>
>>>>> Of course we can implement it (and we will as we actually got a report
>>>>> of this patch breaking Cypress now, finally).
>>>>>
>>>>> The question was never whether it could be done, we're already doing a
>>>>> bunch of abstractions to deal with just the Broadcom-only side of things
>>>>> too. The point I was trying to make is that we need to *know* what
>>>>> firmware abstractions we need and *why* they are needed. We can't just
>>>>> say, for every change, "well, nobody knows if the existing code works or
>>>>> not, so let's just add an abstraction just in case the change breaks
>>>>> something". As far as anyone involved in the discussions until now could
>>>>> tell, this code was just something some Cypress person dumped upstream,
>>>>> and nobody involved was being responsive to any of our inquiries, so
>>>>> there was no way to be certain it worked at all, whether it was
>>>>> supported in public firmware, or anything else.
>>>>>
>>>>> *Now* that we know the existing code is actually functional and not just
>>>>> dead/broken, and that the WSEC approach is conversely not functional on
>>>>> the Cypress firmwares, it makes sense to introduce an abstraction.
>>>>
>>>> Just a quick look in the git history could have told you that it was not
>>>> just dumped upstream and at least one person was using it and extended
>>>> it for 802.11r support (fast-roaming):
>>>>
>>>>
>>>> author	Paweł Drewniak <czajernia@gmail.com>	2021-08-24 23:13:30 +0100
>>>> committer	Kalle Valo <kvalo@codeaurora.org>	2021-08-29 11:33:07 +0300
>>>> commit	4b51de063d5310f1fb297388b7955926e63e45c9 (patch)
>>>> tree	ba2ccb5cbd055d482a8daa263f5e53531c07667f
>>>> /drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
>>>> parent	81f9ebd43659320a88cae8ed5124c50b4d47ab66 (diff)
>>>> download	wireless-4b51de063d5310f1fb297388b7955926e63e45c9.tar.gz
>>>> brcmfmac: Add WPA3 Personal with FT to supported cipher suites
>>>> This allows the driver to connect to BSSIDs supporting SAE with 802.11r.
>>>> Tested on Raspberry Pi 4 Model B (STA) and UniFi 6LR/OpenWRT 21.02.0-rc2.
>>>> AP was set to 'sae-mixed' (WPA2/3 Personal).
>>>>
>>>> Signed-off-by: Paweł Drewniak <czajernia@gmail.com>
>>>> Signed-off-by: Kalle Valo <kvalo@codeaurora.org>
>>>> Link: https://lore.kernel.org/r/20210824221330.3847139-1-czajernia@gmail.com
>>>
>>> Sure, but we also had user reports of it *not* actually working (maybe
>>> it regressed?). We didn't know whether it was functional with the
>>> linux-firmware blobs in any way, I wanted confirmation of that. And we
>>> also didn't know that the patch *would* break it at all; perhaps the
>>> Cypress firmware had also grown support for the WSEC mechanism.
>>>
>>> That's why I wanted someone to actually confirm the code worked (in some
>>> subset of cases) and the patch didn't, before starting to introduce
>>> conditionals. There is, of course, also the element that the Cypress
>>> side has been long unmaintained, and if nobody is testing/giving
>>> feedback/complaining, perhaps it's better to err on the side of maybe
>>> breaking something and see if that gets someone to come out of the
>>> woodwork if it really breaks, rather than tiptoeing around the code
>>> without knowing what's going on and without anyone actually testing things.
>>
>> That is because you distrust the intent that Cypress was really
>> contributing. They were and I trusted them to not just throw in a
>> feature like WPA3. When Infineon took over they went mute. Upon
>> reviewing your patch (again) I also sent an email to them asking
>> specifically about the status of the sae_password interface. I did not
>> use the mailing list which indeed bounces these days (hence removed
>> them) but the last living soul that I had contact with about a year ago
>> whether they were still comitted to be involved. I guess out of
>> politeness or embarrassment I got confirmation they were and never heard
>> from him again. The query about the sae_password interface is still pending.
> 
> If only corporate acquisition politics didn't repeatedly throw a wrench
> into this one... :/
> 
> This is where we are though, Infineon clearly doesn't care, so it's time
> to move on.
> 
>>> It's not about this *specific* patch, it's about the general situation
>>> of not being able to touch firmware interfaces "just in case Cypress
>>> breaks" being unsustainable in the long term. I wasn't pushing back
>>> because I think this particular one will be hard, I was pushing back
>>> because I can read the tea leaves and see this is not going to end well
>>> if it's the approach we start taking for everything. We *need* someone
>>> to be testing patches on Cypress, we can't just "try not to touch it"
>>> and cross our fingers. That just ends in disaster, we are not going to
>>> succeed in not breaking it either way and it's going to make the driver
>>> worse.
>>
>> I admire you ability of reading tea leaves. You saw the Grim I reckon.
>> Admittedly your responses on every comment from my side (or Kalle for
>> that matter) was polarizing every discussion. That is common way people
>> treat each other nowadays especially online where a conversation is just
>> a pile of text going shit. It does not bring out the best in me either,
>> but it was draining every ounce of energy from me so better end it by
>> stepping out.
> 
> The hilariously outdated kernel development model surely doesn't help
> either (I've stated my opinion on this quite a few times if you've
> followed around) ;)

It is not a fair statement to call the kernel development process 
outdated. It is a vast code base that needs agreed upon steps to keep it 
rolling as it is. Attend a plumbers conference or collaboration summit 
or better become a speaker and vent all your opinions there and have a 
discussion with community members. They are held yearly and maybe over 
the past years things have been introduced that give more churn than 
value and that would be a great topic for discussion. However, it is 
better left outside of the development workflow.

> This stuff gets *really* frustrating when you're trying to improve what
> is, I hope we can all admit, an undermaintained driver (that is not to
> say it's anyone's fault personally), and end up getting held back due to
> everything from coding style nitpicks to people not having the time to
> be responsive. It's just not helpful. It's important to know when to
> step aside and let people actually get stuff done.
> 
> When Daniel started sending me brcmfmac patches downstream, I took a
> look at a few of them, decided he knew what he was doing, and just
> started pulling in his branches wholesale. Was it perfect? No, I had to
> debug at least one regression at one point. But it took me less time to
> do that than it would've to go through the commits with a fine toothed
> comb, so it was clearly the right decision.

With the patch that started it all I simply had another view based on 
trusting my peers. Infineon has been pulling away from brcmfmac off the 
bat, but Cypress was serious enough about the driver not to drop a heap 
of dung on it. Based on that I felt regressions would be around the 
corner if we took it as is.

> That is not to say that should be the standard upstream (we make a point
> of moving fast and breaking things more downstream, since it's a proving
> ground for what eventually will be upstreamed), but I think it does
> demonstrate the kind of delegation ability that is sorely lacking in
> many drivers and subsystems in the kernel these days. Maintainers become
> entrenched in their position, long beyond the point where they have the
> time/motivation/ability to drive the code forward, and end up in the way
> of new people who are trying to make a difference. I think Linus knows
> full well the kernel maintainer community is stagnating.
> 
> That doesn't mean people should step down entirely. But it does mean
> they need to recognize when this is happening and, at least, proactively
> try to bring new people in, instead of just continuing to play a
> gatekeeping role. The role of maintainers should not be that of a wall
> people have to climb over to get their changes in, it should be to guide
> new contributors and help onboard people who can contribute, as peers
> and eventually as future maintainers.
> 
> Kalle, in the other thread you said "this is not fun anymore, this is
> more like a business with requirements and demands coming from
> everywhere.". That's what it feels like to us when our changes get
> rejected because the local vars aren't in reverse Christmas tree order,
> or because our commit messages have "v2:" in them. It feels like some
> manager is trying to justify their position by creating busywork for
> everyone else. Nobody should actually care about any of those things,
> and if they do, they need to step back and really ask themselves how
> they ended up believing that. If the goal is to enforce a reasonable
> shared coding style so things don't spiral into chaos, FFS, let's just
> do what every other project does these days and adopt clang-format. Then
> *all* of us can stop wasting time on these trivialities and go back to
> getting stuff done. And really, nobody cares about commit messages as
> long as the tags are right, the subject line is succinct, and the
> important information is in there. Extra stuff never hurt anyone.

https://docs.kernel.org/process/clang-format.html#clangformat

Enjoy!!

>> I added the ground work for multi-vendor support, but have not decided
>> on the approach to take. Abstract per firmware interface primitive or
>> simply have a cfg80211.c and fwil_types.h per vendor OR implement a
>> vendor-specific cfg80211 callback and override the default callback
>> during the driver attach, ie. in brcmf_fwvid_wcc_attach(). The latter
>> duplicates things, but lean towards that as it may be easier on the
>> long-term. What do your tea leaves tell you ;-)
> 
> FWIW, I was hoping you'd stay on at least as a reviewer. Your
> contributions are valuable. You obviously know the driver and hardware
> much better than most people. I encourage you to, at least, post a v2 of
> the MAINTAINERS patch with yourself as an R: line.
> 
> As far as the actual driver abstraction architecture, I'm going to leave
> it to Daniel to decide what makes the most sense, since he's the one
> introducing new mechanisms for that already. There's always room for
> refactoring later though, depending on the direction things take with
> the vendor split. BTW, clang-format also makes refactoring a lot less
> painful ;)

Refactoring a single driver is not so painful, but rather a nice 
relaxing puzzle ;-)

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4219 bytes --]