Received-SPF: pass (google.com: domain of linux-kernel+bounces-7374-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Message-ID: <4c89b71e-8667-40fe-add0-205748de51ef@marcan.st>
Date: Thu, 21 Dec 2023 03:14:25 +0900
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH] wifi: brcmfmac: cfg80211: Use WSEC to set SAE password
Content-Language: en-US
To: Kalle Valo <kvalo@kernel.org>,
 Linus Torvalds <torvalds@linux-foundation.org>
Cc: Daniel Berlin <dberlin@dberlin.org>,
 Arend van Spriel <arend.vanspriel@broadcom.com>,
 Arend van Spriel <aspriel@gmail.com>, Franky Lin <franky.lin@broadcom.com>,
 Hante Meuleman <hante.meuleman@broadcom.com>,
 SHA-cyfmac-dev-list@infineon.com, asahi@lists.linux.dev,
 brcm80211-dev-list.pdl@broadcom.com, linux-kernel@vger.kernel.org,
 linux-wireless@vger.kernel.org, David Airlie <airlied@redhat.com>,
 Daniel Vetter <daniel@ffwll.ch>
References: <20231107-brcmfmac-wpa3-v1-1-4c7db8636680@marcan.st>
 <170281231651.2255653.7498073085103487666.kvalo@kernel.org>
 <18c80d15e30.279b.9b12b7fc0a3841636cfb5e919b41b954@broadcom.com>
 <1b51997f-2994-46e8-ac58-90106d1c486d@marcan.st>
 <c392f901-789a-42e2-8cf7-5e246365a1ca@broadcom.com>
 <CAF4BwTXNtu30DAgBXo4auDaDK0iWc9Ch8f=EH+facQ-_F-oMUQ@mail.gmail.com>
 <87r0jiqmnx.fsf@kernel.org> <01bd8c68-1b9c-49b2-8ace-1c7d1b5192ad@marcan.st>
 <CAHk-=whDLKZZEuxU_jEhZRdeWjXAkL8=J_JRk2Ar6wp9UK3h2w@mail.gmail.com>
 <871qbhqio8.fsf@kernel.org>
From: Hector Martin <marcan@marcan.st>
In-Reply-To: <871qbhqio8.fsf@kernel.org>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit


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.

Here's an example of the case where an abstraction was *not* needed: I
switched over WPA PSK configuration from hex-encoded to binary. That was
needed to make more recent Apple firmwares work. My evidence at the time
that that change *was* at least fairly backwards compatible was that the
OpenBSD driver had been doing it that way all along. Had we introduced
an abstraction/conditional for that case "just in case", it would have
generated superfluous technical debt for no reason.

- Hector