Message-ID: <31292508-f881-4457-a4bf-2ca0b8e8f435@broadcom.com>
Date: Thu, 21 Dec 2023 10:57:37 +0100
From: Arend van Spriel <arend.vanspriel@...adcom.com>
To: Hector Martin <marcan@...can.st>, Kalle Valo <kvalo@...nel.org>,
 Linus Torvalds <torvalds@...ux-foundation.org>
Cc: 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

- 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.

> 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.

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

Regards,
Arend


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

Powered by blists - more mailing lists