Message-ID: <CAOgh=FxsMMbp1Pyg7GSNmwNvfeX+7JDibQbMbx-hdLsCQz5qYA@mail.gmail.com>
Date: Fri, 22 Dec 2023 12:25:27 +0000
From: Eric Curtin <ecurtin@...hat.com>
To: Hector Martin <marcan@...can.st>
Cc: Arend van Spriel <arend.vanspriel@...adcom.com>, Kalle Valo <kvalo@...nel.org>, 
	Linus Torvalds <torvalds@...ux-foundation.org>, Daniel Berlin <dberlin@...rlin.org>, 
	Arend van Spriel <aspriel@...il.com>, Franky Lin <franky.lin@...adcom.com>, 
	Hante Meuleman <hante.meuleman@...adcom.com>, asahi@...ts.linux.dev, 
	brcm80211-dev-list.pdl@...adcom.com, linux-kernel@...r.kernel.org, 
	linux-wireless@...r.kernel.org, David Airlie <airlied@...hat.com>, 
	Daniel Vetter <daniel@...ll.ch>
Subject: Re: [PATCH] wifi: brcmfmac: cfg80211: Use WSEC to set SAE password

On Fri, 22 Dec 2023 at 05:19, Hector Martin <marcan@...can.st> wrote:
>
>
>
> On 2023/12/21 18:57, Arend van Spriel wrote:
> > - SHA-cyfmac-dev-list@...ineon.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@...ux-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@...il.com>    2021-08-24 23:13:30 +0100
> >>> committer   Kalle Valo <kvalo@...eaurora.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@...il.com>
> >>> Signed-off-by: Kalle Valo <kvalo@...eaurora.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) ;)
>
> 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.
>
> 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.
>
> > 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.

I generally agree with this email, especially that Arend should stay
on as a reviewer/maintainer.

We need more people as either maintainers/contributors/reviewers/code
writers/testers, not less, to delegate, co-maintain, test, merge, make
the code more portable to many wifi devices, etc.

What really matters at the end of the day I guess is writing the code
that works across all the devices and testing it.

Which is why I spread awareness about this area, got 100s of
responses, especially on Linkedin, there's at least a portion of these
that want to help, in good spirits.

Is mise le meas/Regards,

Eric Curtin

>
> 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 ;)
>
> - Hector
>

Powered by blists - more mailing lists