| Message ID | 309965e8ef4d220053ca7e6bd34393f892ea1bb8.1707486287.git.vinayak.yadawad@broadcom.com |
|---|---|
| State | New |
| Headers | show |
| Series | [1/1] wifi: nl80211: Add support for plumbing SAE groups to driver | expand |
On Fri, 2024-02-09 at 17:01 -0800, Jeff Johnson wrote: > > > + [NL80211_ATTR_SAE_DH_GROUPS] = { .type = NLA_NESTED }, > > can/should this have a policy? not sure it can have a policy, so see > below... Yeah not really. Should it even be nested though? There's not a huge difference to just making it a binary with a bunch of u32 values, and then the max length can even encode (and make visible to userspace) the now hidden value of CFG80211_MAX_NUM_SAE_DH_GROUPS. johannes
Vinayak Yadawad <vinayak.yadawad@broadcom.com> writes: > In case of SAE authentication offload, the driver would need > the info of SAE DH groups for both STA connection and soft AP. > In the current change we update the SAE DH group support info > to driver in the order/priority as provided by the supplicant/ > upper layer. > > Signed-off-by: Vinayak Yadawad <vinayak.yadawad@broadcom.com> [...] > @@ -1235,6 +1236,9 @@ struct survey_info { > * > * NL80211_SAE_PWE_BOTH > * Allow either hunting-and-pecking loop or hash-to-element > + * > + * @sae_dh_groups: SAE DH groups in preference order. > + * @n_sae_dhd_groups: No of SAE DH groups assigned. > */ > struct cfg80211_crypto_settings { > u32 wpa_versions; > @@ -1252,6 +1256,8 @@ struct cfg80211_crypto_settings { > const u8 *sae_pwd; > u8 sae_pwd_len; > enum nl80211_sae_pwe_mechanism sae_pwe; > + u32 sae_dh_groups[CFG80211_MAX_NUM_SAE_DH_GROUPS]; > + u8 n_sae_dh_groups; > }; What driver is going to use these new crypto settings? Or is this for an out-of-tree driver?
On Mon, 2024-02-12 at 09:25 +0200, Kalle Valo wrote: > What driver is going to use these new crypto settings? Or is this for an > out-of-tree driver? > I'm sure it's for an out-of-tree driver. This is the _entirety_ of "@broadcom"'s wireless contributions with "--since=2020" (somewhat arbitrarily chosen, though going a bit further back has some "real" work in brcmfmac), as far as I can tell: Arend Van Spriel (1): cfg80211: adapt to new channelization of the 6GHz band Arend van Spriel (23): cfg80211: add VHT rate entries for MCS-10 and MCS-11 brcmfmac: use different error value for invalid ram base address brcmfmac: increase core revision column aligning core list brcmfmac: add xtlv support to firmware interface layer brcmfmac: support chipsets with different core enumeration space wifi: cfg80211: fix memory leak in query_regdb_file() wifi: brcmfmac: add function to unbind device to bus layer api wifi: brcmfmac: add firmware vendor info in driver info wifi: brcmfmac: add support for vendor-specific firmware api wifi: brcmfmac: add support for Cypress firmware api wifi: brcmfmac: add support Broadcom BCA firmware api wifi: brcmfmac: add vendor name in revinfo debugfs file wifi: brcmfmac: introduce BRCMFMAC exported symbols namespace wifi: brcmfmac: avoid handling disabled channels for survey dump wifi: brcmfmac: avoid NULL-deref in survey dump for 2G only device wifi: brcmfmac: fix regression for Broadcom PCIe wifi devices wifi: brcmfmac: change cfg80211_set_channel() name and signature wifi: brcmfmac: export firmware interface functions wifi: brcmfmac: add per-vendor feature detection callback wifi: brcmfmac: move feature overrides before feature_disable wifi: brcmfmac: avoid invalid list operation when vendor attach fails wifi: brcmfmac: allow per-vendor event handling wifi: brcmfmac: add linefeed at end of file Vinayak Yadawad (4): wifi: cfg80211: Allow P2P client interface to indicate port authorization cfg80211: Update Transition Disable policy during port authorization wifi: cfg80211: Allow AP/P2PGO to indicate port authorization to peer STA/P2PClient wifi: nl80211: Extend del pmksa support for SAE and OWE security So looks to me like Broadcom doesn't want its (real) drivers to work in upstream, so I guess we really ought to just stop accommodating for them in the wireless stack... This only works if we collaborate, and I've said this before: I can't maintain something well that I cannot see (and possibly change) the user(s) of. I guess if Broadcom's plans change they can start by submitting drivers that actually use the relevant infrastructure. And note that I've said this to Qualcomm before: I don't really want to and can't (well) maintain a lot of stuff in the tree that exists there solely to make out-of-tree drivers happy. And @Broadcom: we really _want_ you to contribute upstream. But that shouldn't be dumping APIs over the wall when you need them and letting us sort out everything else ... johannes
On 2/12/2024 8:58 PM, Johannes Berg wrote: > On Mon, 2024-02-12 at 09:25 +0200, Kalle Valo wrote: > >> What driver is going to use these new crypto settings? Or is this for an >> out-of-tree driver? >> > > I'm sure it's for an out-of-tree driver. > > This is the _entirety_ of "@broadcom"'s wireless contributions with > "--since=2020" (somewhat arbitrarily chosen, though going a bit further > back has some "real" work in brcmfmac), as far as I can tell: > > Arend Van Spriel (1): > cfg80211: adapt to new channelization of the 6GHz band > > Arend van Spriel (23): > cfg80211: add VHT rate entries for MCS-10 and MCS-11 > brcmfmac: use different error value for invalid ram base address > brcmfmac: increase core revision column aligning core list > brcmfmac: add xtlv support to firmware interface layer > brcmfmac: support chipsets with different core enumeration space > wifi: cfg80211: fix memory leak in query_regdb_file() > wifi: brcmfmac: add function to unbind device to bus layer api > wifi: brcmfmac: add firmware vendor info in driver info > wifi: brcmfmac: add support for vendor-specific firmware api > wifi: brcmfmac: add support for Cypress firmware api > wifi: brcmfmac: add support Broadcom BCA firmware api > wifi: brcmfmac: add vendor name in revinfo debugfs file > wifi: brcmfmac: introduce BRCMFMAC exported symbols namespace > wifi: brcmfmac: avoid handling disabled channels for survey dump > wifi: brcmfmac: avoid NULL-deref in survey dump for 2G only device > wifi: brcmfmac: fix regression for Broadcom PCIe wifi devices > wifi: brcmfmac: change cfg80211_set_channel() name and signature > wifi: brcmfmac: export firmware interface functions > wifi: brcmfmac: add per-vendor feature detection callback > wifi: brcmfmac: move feature overrides before feature_disable > wifi: brcmfmac: avoid invalid list operation when vendor attach fails > wifi: brcmfmac: allow per-vendor event handling > wifi: brcmfmac: add linefeed at end of file > > Vinayak Yadawad (4): > wifi: cfg80211: Allow P2P client interface to indicate port authorization > cfg80211: Update Transition Disable policy during port authorization > wifi: cfg80211: Allow AP/P2PGO to indicate port authorization to peer STA/P2PClient > wifi: nl80211: Extend del pmksa support for SAE and OWE security > > > So looks to me like Broadcom doesn't want its (real) drivers to work in > upstream, so I guess we really ought to just stop accommodating for them > in the wireless stack... This only works if we collaborate, and I've > said this before: I can't maintain something well that I cannot see (and > possibly change) the user(s) of. Understood and you are right. The brcm80211 drivers, which are not less real ;-) , were a side-project to serve a certain group of customers. Unfortunately it never became the main driver for Broadcom. Cypress did invest in brcmfmac, but we know how that went since Infineon took over. Maybe they will upstream the ifxfmac driver [1] some day but I have no high hopes on that. Regards, Arend [1] https://github.com/Infineon/rpi-linux-kernel/releases/tag/6.1.21-hedorah-IFXFMAC-20231128
On Tue, 2024-02-13 at 10:42 +0100, Arend van Spriel wrote: > > So looks to me like Broadcom doesn't want its (real) drivers to work in > > upstream, so I guess we really ought to just stop accommodating for them > > in the wireless stack... This only works if we collaborate, and I've > > said this before: I can't maintain something well that I cannot see (and > > possibly change) the user(s) of. > > Understood and you are right. The brcm80211 drivers, which are not less > real ;-) , were a side-project to serve a certain group of customers. It's a real driver, fair enough. But yeah, you do get the sense that whatever it is, it really "was" and "isn't" any more. > Unfortunately it never became the main driver for Broadcom. Cypress did > invest in brcmfmac, but we know how that went since Infineon took over. > Maybe they will upstream the ifxfmac driver [1] some day but I have no > high hopes on that. That doesn't even look super awful, they could probably drop it into staging as is ... But that'd mean somebody actually cares, which really seems to be the problem. But since clearly there's no incentive for anyone here in this game to upstream anything to start with, I also don't see why I should give more incentive to _not_ upstream things by accommodating non-upstream drivers in the upstream wifi stack... So I'm dropping this patch, Broadcom can decide what they want first, but you can't have both upstream and non-upstream together. And for the record, I'm very happy that you have and still are maintaining this driver as far as it's come. johannes
On 2/13/2024 11:09 AM, Johannes Berg wrote: > On Tue, 2024-02-13 at 10:42 +0100, Arend van Spriel wrote: >>> So looks to me like Broadcom doesn't want its (real) drivers to work in >>> upstream, so I guess we really ought to just stop accommodating for them >>> in the wireless stack... This only works if we collaborate, and I've >>> said this before: I can't maintain something well that I cannot see (and >>> possibly change) the user(s) of. >> >> Understood and you are right. The brcm80211 drivers, which are not less >> real ;-) , were a side-project to serve a certain group of customers. > > It's a real driver, fair enough. But yeah, you do get the sense that > whatever it is, it really "was" and "isn't" any more. > >> Unfortunately it never became the main driver for Broadcom. Cypress did >> invest in brcmfmac, but we know how that went since Infineon took over. >> Maybe they will upstream the ifxfmac driver [1] some day but I have no >> high hopes on that. > > That doesn't even look super awful, they could probably drop it into > staging as is ... > > But that'd mean somebody actually cares, which really seems to be the > problem. > > But since clearly there's no incentive for anyone here in this game to > upstream anything to start with, I also don't see why I should give more > incentive to _not_ upstream things by accommodating non-upstream drivers > in the upstream wifi stack... > > So I'm dropping this patch, Broadcom can decide what they want first, > but you can't have both upstream and non-upstream together. > > And for the record, I'm very happy that you have and still are > maintaining this driver as far as it's come. Thanks for the record ;-) I recall the rule was that nl80211 API changes should also have at least one driver implementing it. Guess we let that slip a couple of times. I fully agree enforcing this. FWIW I am actually planning on submitting brcmfmac patches to support NL80211_CMD_EXTERNAL_AUTH. Gr. AvS
On Tue, 2024-02-13 at 12:13 +0100, Arend van Spriel wrote: > > I recall the rule was that nl80211 API changes > should also have at least one driver implementing it. Guess we let that > slip a couple of times. I fully agree enforcing this. Well, enforcing it strictly never really worked all that well in practice, since you don't necessarily want to have a complex driver implementation while hashing out the API, and the API fundamentally has to come first. So in a sense it comes down to trust, and that people will actually follow up with implementations. And yeah, plans can change and you end up not really supporting everything that was defined ... that's life, I guess. But the mode here seems to be that there's not even any _intent_ to do that? I guess we could hash out the API, review the patches, and then _not_ apply them until a driver is ready? So the first round of reviews would still come with API only, but once that settles we don't actually merge it immediately, unlike normally where we merge a patch we've reviewed? And then if whoever did it lost interest, we already have a reviewed version for anyone else who might need it? > FWIW I am actually > planning on submitting brcmfmac patches to support > NL80211_CMD_EXTERNAL_AUTH. Cool :) johannes
On 2/13/2024 12:45 PM, Johannes Berg wrote: > On Tue, 2024-02-13 at 12:13 +0100, Arend van Spriel wrote: >> >> I recall the rule was that nl80211 API changes >> should also have at least one driver implementing it. Guess we let that >> slip a couple of times. I fully agree enforcing this. > > Well, enforcing it strictly never really worked all that well in > practice, since you don't necessarily want to have a complex driver > implementation while hashing out the API, and the API fundamentally has > to come first. > > So in a sense it comes down to trust, and that people will actually > follow up with implementations. And yeah, plans can change and you end > up not really supporting everything that was defined ... that's life, I > guess. > > But the mode here seems to be that there's not even any _intent_ to do > that? > > I guess we could hash out the API, review the patches, and then _not_ > apply them until a driver is ready? So the first round of reviews would > still come with API only, but once that settles we don't actually merge > it immediately, unlike normally where we merge a patch we've reviewed? > And then if whoever did it lost interest, we already have a reviewed > version for anyone else who might need it? Sounds like a plan. Maybe they can get a separate state in patchwork and let them sit there for grabs. Gr. AvS
On Tue, 2024-02-13 at 13:19 +0100, Arend van Spriel wrote: > On 2/13/2024 12:45 PM, Johannes Berg wrote: > > On Tue, 2024-02-13 at 12:13 +0100, Arend van Spriel wrote: > > > > > > I recall the rule was that nl80211 API changes > > > should also have at least one driver implementing it. Guess we let that > > > slip a couple of times. I fully agree enforcing this. > > > > Well, enforcing it strictly never really worked all that well in > > practice, since you don't necessarily want to have a complex driver > > implementation while hashing out the API, and the API fundamentally has > > to come first. > > > > So in a sense it comes down to trust, and that people will actually > > follow up with implementations. And yeah, plans can change and you end > > up not really supporting everything that was defined ... that's life, I > > guess. > > > > But the mode here seems to be that there's not even any _intent_ to do > > that? > > > > I guess we could hash out the API, review the patches, and then _not_ > > apply them until a driver is ready? So the first round of reviews would > > still come with API only, but once that settles we don't actually merge > > it immediately, unlike normally where we merge a patch we've reviewed? > > And then if whoever did it lost interest, we already have a reviewed > > version for anyone else who might need it? > > Sounds like a plan. Maybe they can get a separate state in patchwork and > let them sit there for grabs. I guess I can leave them open as 'under review' or something? Not sure we can add other states. johannes
Johannes Berg <johannes@sipsolutions.net> writes: > So looks to me like Broadcom doesn't want its (real) drivers to work in > upstream, so I guess we really ought to just stop accommodating for them > in the wireless stack... This only works if we collaborate, and I've > said this before: I can't maintain something well that I cannot see (and > possibly change) the user(s) of. > > I guess if Broadcom's plans change they can start by submitting drivers > that actually use the relevant infrastructure. > > And note that I've said this to Qualcomm before: I don't really want to > and can't (well) maintain a lot of stuff in the tree that exists there > solely to make out-of-tree drivers happy. Yeah, we need to make a hard stance here and solve this "throw patches over the wall and run away as fast as you can" model which corporations use. If kernel.org wireless is important for you, or your company, then help us! Don't just expect that we do everything for you, that doesn't scale. > And @Broadcom: we really _want_ you to contribute upstream. But that > shouldn't be dumping APIs over the wall when you need them and letting > us sort out everything else ... A practical tip I can give to Broadcom is to remind that you have a great engineer with upstream knowledge: Arend. I recommend utilising him and asking for guidance anything upstream wireless related. Even better if all the code coming from Broadcom would go through him. And thenever you add a new feature to the stack, add support to brcm80211 driver at the same time. This help Broadcom to get the feature you need to upstream and the upstream community would have a better Broadcom driver. Arend, sorry for dumping more work for you :D
Johannes Berg <johannes@sipsolutions.net> writes: > On Tue, 2024-02-13 at 13:19 +0100, Arend van Spriel wrote: > >> On 2/13/2024 12:45 PM, Johannes Berg wrote: >> > On Tue, 2024-02-13 at 12:13 +0100, Arend van Spriel wrote: >> > > >> > > I recall the rule was that nl80211 API changes >> > > should also have at least one driver implementing it. Guess we let that >> > > slip a couple of times. I fully agree enforcing this. >> > >> > Well, enforcing it strictly never really worked all that well in >> > practice, since you don't necessarily want to have a complex driver >> > implementation while hashing out the API, and the API fundamentally has >> > to come first. >> > >> > So in a sense it comes down to trust, and that people will actually >> > follow up with implementations. And yeah, plans can change and you end >> > up not really supporting everything that was defined ... that's life, I >> > guess. >> > >> > But the mode here seems to be that there's not even any _intent_ to do >> > that? >> > >> > I guess we could hash out the API, review the patches, and then _not_ >> > apply them until a driver is ready? So the first round of reviews would >> > still come with API only, but once that settles we don't actually merge >> > it immediately, unlike normally where we merge a patch we've reviewed? >> > And then if whoever did it lost interest, we already have a reviewed >> > version for anyone else who might need it? >> >> Sounds like a plan. Maybe they can get a separate state in patchwork and >> let them sit there for grabs. > > I guess I can leave them open as 'under review' or something? Not sure > we can add other states. I belong to the church of 'Clean Inbox' so I use 'Deferred' state for stuff I can't work on right now. Though I know a lot of people don't like it because deferred patches are not shown in the default patchwok view.
Hi Johannes, Kalle, >I'm sure it's for an out-of-tree driver. Yes, it's for an out of tree driver. > I've said this before: I can't maintain something well that I cannot see (and possibly change) the user(s) of. >I recall the rule was that nl80211 API changes should also have at least one driver implementing it. Got your point. The recent efforts for patch submission was to align more with the nl80211/cfg80211 interface and remove vendor patches/interfaces. I wasn’t aware of this upstream requirement, but I do get your point that the requirement is to have at least one driver that exercises these interfaces for code maintainability/understanding the why interface changes are needed. >A practical tip I can give to Broadcom is to remind that you have a great engineer with upstream knowledge: Arend >And thenever you add a new feature to the stack, add support to brcm80211 driver at the same time. This help Broadcom to get the feature you need to upstream and the upstream community would have a better Broadcom driver. Thanks for the inputs. Let me sync up internally and get back. Thanks, On Tue, Feb 13, 2024 at 6:20 PM Kalle Valo <kvalo@kernel.org> wrote: > > Johannes Berg <johannes@sipsolutions.net> writes: > > > On Tue, 2024-02-13 at 13:19 +0100, Arend van Spriel wrote: > > > >> On 2/13/2024 12:45 PM, Johannes Berg wrote: > >> > On Tue, 2024-02-13 at 12:13 +0100, Arend van Spriel wrote: > >> > > > >> > > I recall the rule was that nl80211 API changes > >> > > should also have at least one driver implementing it. Guess we let that > >> > > slip a couple of times. I fully agree enforcing this. > >> > > >> > Well, enforcing it strictly never really worked all that well in > >> > practice, since you don't necessarily want to have a complex driver > >> > implementation while hashing out the API, and the API fundamentally has > >> > to come first. > >> > > >> > So in a sense it comes down to trust, and that people will actually > >> > follow up with implementations. And yeah, plans can change and you end > >> > up not really supporting everything that was defined ... that's life, I > >> > guess. > >> > > >> > But the mode here seems to be that there's not even any _intent_ to do > >> > that? > >> > > >> > I guess we could hash out the API, review the patches, and then _not_ > >> > apply them until a driver is ready? So the first round of reviews would > >> > still come with API only, but once that settles we don't actually merge > >> > it immediately, unlike normally where we merge a patch we've reviewed? > >> > And then if whoever did it lost interest, we already have a reviewed > >> > version for anyone else who might need it? > >> > >> Sounds like a plan. Maybe they can get a separate state in patchwork and > >> let them sit there for grabs. > > > > I guess I can leave them open as 'under review' or something? Not sure > > we can add other states. > > I belong to the church of 'Clean Inbox' so I use 'Deferred' state for > stuff I can't work on right now. Though I know a lot of people don't > like it because deferred patches are not shown in the default patchwok > view. > > -- > https://patchwork.kernel.org/project/linux-wireless/list/ > > https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
On Mon, 12 Feb 2024 20:58:51 +0100 Johannes Berg wrote: > And @Broadcom: we really _want_ you to contribute upstream. But that > shouldn't be dumping APIs over the wall when you need them and letting > us sort out everything else ... I may be missing the point but is there any official way we can designate level of vendor involvement? MAINTAINERS seems like the most basic, and we have BROADCOM BRCM80211 IEEE802.11 WIRELESS DRIVERS M: Arend van Spriel <arend.vanspriel@broadcom.com> L: linux-wireless@vger.kernel.org L: brcm80211@lists.linux.dev L: brcm80211-dev-list.pdl@broadcom.com S: Supported F: drivers/net/wireless/broadcom/brcm80211/ F: include/linux/platform_data/brcmfmac.h where (for the uninitiated): Supported: Someone is actually paid to look after this. It doesn't seem like Arend is afforded much paid time "to look after this". On the Ethernet side I have a nebulous hope to require vendors who want the "Supported" status to run our in-tree tests against their HW daily. As a way to trigger the loss of status. Otherwise it's hard to catch people drifting away.
On Tue, 2024-02-13 at 17:43 -0800, Jakub Kicinski wrote: > I may be missing the point but is there any official way we can > designate level of vendor involvement? MAINTAINERS seems like > the most basic, and we have > > BROADCOM BRCM80211 IEEE802.11 WIRELESS DRIVERS > M: Arend van Spriel <arend.vanspriel@broadcom.com> > L: linux-wireless@vger.kernel.org > L: brcm80211@lists.linux.dev > L: brcm80211-dev-list.pdl@broadcom.com > S: Supported > F: drivers/net/wireless/broadcom/brcm80211/ > F: include/linux/platform_data/brcmfmac.h > > where (for the uninitiated): > > Supported: Someone is actually paid to look after this. > > It doesn't seem like Arend is afforded much paid time "to look after > this". I don't know if that's even the core of my complaint. I don't know if it's true, but let's assume Arend _does_ get sufficient time to take care of the driver. The core of the complaint here is that regardless of that, Broadcom is treating the driver as a dead end, a fork in the road they're no longer travelling. So "supporting" that driver may all be well, in the sense that it's there for the existing hardware/firmware that it supports. But! It's not getting new features nor support for new devices, I don't even know if it still supports newer firmware images for the devices it already supports. Some new driver support is coming in by way of the Apple-support folks, but you saw how that's going ... Yet at the same time Broadcom _are_ sending patches to the core wifi stack, in order to support new features/offloads for their new firmware builds etc. on some/other/new devices. New features for the stack where we cannot actually see the driver implementation, maintain it, etc. Not that in many cases the driver implementation would be all that interesting, but it's still pushing code and work into upstream that it will never benefit from. So this disconnect really is the complaint: Broadcom want us to maintain the stack for them, do things for them like in this patch in support of their latest firmware builds, but they definitely do _not_ want to do anything upstream that would actually support these new things they have. At which point, yeah, I'm putting my foot down and saying this has to stop. I really don't (**) care about Broadcom doing their own vendor- specific APIs if there's zero chance the things they're needed for will ever land upstream anyway. (**) No longer. I used to think that being more open about this would encourage folks to start a journey of contributing more upstream, but clearly that hasn't worked out. Now this is why I used to be more open: I will also most definitely not accept all the vendor APIs upstream if someone later decides they do want an upstream driver, and then push all the vendor stuff on grounds that "it's used now and we have to support it" ... We don't, at least not upstream, what you sell to your customers really isn't our problem. (And to be honest, if customers cared, we'd not be in this position) > On the Ethernet side I have a nebulous hope to require vendors who want > the "Supported" status to run our in-tree tests against their HW daily. > As a way to trigger the loss of status. Otherwise it's hard to catch > people drifting away. Every day seems a bit excessive? OTOH, every day is a good way of saying "you really have to automate this", but then once you do that, maybe you don't need to pay anyone to really maintain it, beyond trying to keep the tests running? Also not sure what that status really implies, I think Broadcom would be quite happy to just mark the driver as orphaned... johannes
On Wed, 14 Feb 2024 11:27:42 +0100 Johannes Berg wrote: > > It doesn't seem like Arend is afforded much paid time "to look after > > this". > > I don't know if that's even the core of my complaint. I don't know if > it's true, but let's assume Arend _does_ get sufficient time to take > care of the driver. Right, right, I know that's not the core of your complaint. More of an adjacent question about somehow reflecting the "vendor engagement level" > The core of the complaint here is that regardless of that, Broadcom is > treating the driver as a dead end, a fork in the road they're no longer > travelling. So "supporting" that driver may all be well, in the sense > that it's there for the existing hardware/firmware that it supports. > > But! It's not getting new features nor support for new devices, I don't > even know if it still supports newer firmware images for the devices it > already supports. Some new driver support is coming in by way of the > Apple-support folks, but you saw how that's going ... To a large extent I think it's on us to define what "paid to look after a driver" means. Any line we draw, no matter how arbitrary, can be used by the developers to justify the time spent working upstream to their management. Or so I hope. Since Broadcom didn't abandon client WiFi chipsets, wouldn't it be reasonable to expect someone to work on the upstream driver at least half time? > Yet at the same time Broadcom _are_ sending patches to the core wifi > stack, in order to support new features/offloads for their new firmware > builds etc. on some/other/new devices. New features for the stack where > we cannot actually see the driver implementation, maintain it, etc. Not > that in many cases the driver implementation would be all that > interesting, but it's still pushing code and work into upstream that it > will never benefit from. > > So this disconnect really is the complaint: Broadcom want us to maintain > the stack for them, do things for them like in this patch in support of > their latest firmware builds, but they definitely do _not_ want to do > anything upstream that would actually support these new things they > have. > > At which point, yeah, I'm putting my foot down and saying this has to > stop. I really don't (**) care about Broadcom doing their own vendor- > specific APIs if there's zero chance the things they're needed for will > ever land upstream anyway. > > (**) No longer. I used to think that being more open about this would > encourage folks to start a journey of contributing more upstream, but > clearly that hasn't worked out. > > Now this is why I used to be more open: I will also most definitely not > accept all the vendor APIs upstream if someone later decides they do > want an upstream driver, and then push all the vendor stuff on grounds > that "it's used now and we have to support it" ... We don't, at least > not upstream, what you sell to your customers really isn't our problem. > > (And to be honest, if customers cared, we'd not be in this position) > > > On the Ethernet side I have a nebulous hope to require vendors who want > > the "Supported" status to run our in-tree tests against their HW daily. > > As a way to trigger the loss of status. Otherwise it's hard to catch > > people drifting away. > > Every day seems a bit excessive? OTOH, every day is a good way of saying > "you really have to automate this", but then once you do that, maybe you > don't need to pay anyone to really maintain it, beyond trying to keep > the tests running? Ack, I'm curious what would end up happening. It's not the primary reason for working on a shared test pool just a potential side benefit. > Also not sure what that status really implies, I think Broadcom would be > quite happy to just mark the driver as orphaned...
On 2/14/2024 2:27 AM, Johannes Berg wrote: > At which point, yeah, I'm putting my foot down and saying this has to > stop. I really don't (**) care about Broadcom doing their own vendor- > specific APIs if there's zero chance the things they're needed for will > ever land upstream anyway. > > (**) No longer. I used to think that being more open about this would > encourage folks to start a journey of contributing more upstream, but > clearly that hasn't worked out. > > Now this is why I used to be more open: I will also most definitely not > accept all the vendor APIs upstream if someone later decides they do > want an upstream driver, and then push all the vendor stuff on grounds > that "it's used now and we have to support it" ... We don't, at least > not upstream, what you sell to your customers really isn't our problem. > > (And to be honest, if customers cared, we'd not be in this position) I feel a need to respond since, as part of the thread, Qualcomm was also mentioned, and rightfully so. In addition to our in-tree ath drivers we have multiple out-of-tree wifi drivers, some for mobile-based products and some for infrastructure-based products. And we have also contributed patches in the past that were only in support of our out-of-tree drivers. There are good reasons these out-of-tree drivers exist, but there is also a movement, at least for the Qualcomm infrastructure products, to transition to an upstream driver, in part due to customer requests. So it is disconcerting that you are talking about inserting barriers to converting to an upstream driver. Converting from an out-of-tree driver to an upstream driver involves significance NRE cost with little to no revenue gain, but that is something Qualcomm is willing to do for the driver. But we need our userspace interfaces to survive since both Qualcomm and our customers have years of work invested in the existing userspace interfaces and applications. The customers who want an upstream driver do not want to be forced to rewrite their applications to support it. In the kernel we have a clear mantra to not break userspace. That should hopefully hold true when converting from an out-of-tree driver to an upstream one. /jeff
Hi, Sorry, I buried this thread because I thought I needed more time to respond than I had two weeks ago, and then forgot about it. My bad. On Wed, 2024-02-14 at 08:57 -0800, Jeff Johnson wrote: > There are good reasons these out-of-tree drivers exist, but there is > also a movement, at least for the Qualcomm infrastructure products, to > transition to an upstream driver, in part due to customer requests. So > it is disconcerting that you are talking about inserting barriers to > converting to an upstream driver. FWIW, I don't think of what I wrote as advocating for *inserting* barriers that didn't already exist today. > But we need our userspace interfaces to survive since both > Qualcomm and our customers have years of work invested in the existing > userspace interfaces and applications. The customers who want an > upstream driver do not want to be forced to rewrite their applications > to support it. Then maybe they don't _really_ want an upstream driver? What's their reasoning for wanting an upstream driver anyway - usually it ends up being something around upstream's checks & balances, etc. But not inventing gratuitous API differences is part of those? > In the kernel we have a clear mantra to not break userspace. That should > hopefully hold true when converting from an out-of-tree driver to an > upstream one. No, not at all? The kernel's policy of not breaking userspace unsurprisingly only extends to ... the kernel. Whatever happened out of tree isn't covered, and really shouldn't be. It doesn't even really quite extend to staging. And this policy is actually often a reason _not_ to include something in the kernel, until userspace interfaces stabilize. And since this was prompted by my mention of vendor APIs: Our upstream stance on vendor APIs has been debated and consequently documented here: https://wireless.wiki.kernel.org/en/developers/documentation/nl80211#vendor-specific_api As far as I'm concerned, there's no intention of deviating from that policy for the purpose of getting a currently non-upstream driver into the tree. johannes
diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h index 5b42bfc..0b2db0d 100644 --- a/include/net/cfg80211.h +++ b/include/net/cfg80211.h @@ -1194,6 +1194,7 @@ struct survey_info { }; #define CFG80211_MAX_NUM_AKM_SUITES 10 +#define CFG80211_MAX_NUM_SAE_DH_GROUPS 10 /** * struct cfg80211_crypto_settings - Crypto settings @@ -1235,6 +1236,9 @@ struct survey_info { * * NL80211_SAE_PWE_BOTH * Allow either hunting-and-pecking loop or hash-to-element + * + * @sae_dh_groups: SAE DH groups in preference order. + * @n_sae_dhd_groups: No of SAE DH groups assigned. */ struct cfg80211_crypto_settings { u32 wpa_versions; @@ -1252,6 +1256,8 @@ struct cfg80211_crypto_settings { const u8 *sae_pwd; u8 sae_pwd_len; enum nl80211_sae_pwe_mechanism sae_pwe; + u32 sae_dh_groups[CFG80211_MAX_NUM_SAE_DH_GROUPS]; + u8 n_sae_dh_groups; }; /** diff --git a/include/uapi/linux/nl80211.h b/include/uapi/linux/nl80211.h index 853ac53..7c1a7b4 100644 --- a/include/uapi/linux/nl80211.h +++ b/include/uapi/linux/nl80211.h @@ -2855,6 +2855,11 @@ enum nl80211_commands { * %NL80211_CMD_ASSOCIATE indicating the SPP A-MSDUs * are used on this connection * + * @NL80211_ATTR_SAE_DH_GROUPS: Attribute to indicate the supported SAE DH + * groups to driver. For STA role, the dh groups should be tried in the + * indicated order. For AP role, the order does not have any specific + * significance. + * * @NUM_NL80211_ATTR: total number of nl80211_attrs available * @NL80211_ATTR_MAX: highest attribute number currently defined * @__NL80211_ATTR_AFTER_LAST: internal use @@ -3400,6 +3405,8 @@ enum nl80211_attrs { NL80211_ATTR_ASSOC_SPP_AMSDU, + NL80211_ATTR_SAE_DH_GROUPS, + /* add attributes here, update the policy in nl80211.c */ __NL80211_ATTR_AFTER_LAST, diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c index 68c2040..555eb0f 100644 --- a/net/wireless/nl80211.c +++ b/net/wireless/nl80211.c @@ -826,6 +826,7 @@ static int validate_he_capa(const struct nlattr *attr, [NL80211_ATTR_MLO_TTLM_DLINK] = NLA_POLICY_EXACT_LEN(sizeof(u16) * 8), [NL80211_ATTR_MLO_TTLM_ULINK] = NLA_POLICY_EXACT_LEN(sizeof(u16) * 8), [NL80211_ATTR_ASSOC_SPP_AMSDU] = { .type = NLA_FLAG }, + [NL80211_ATTR_SAE_DH_GROUPS] = { .type = NLA_NESTED }, }; /* policy for the key attributes */ @@ -10883,6 +10884,27 @@ static int nl80211_crypto_settings(struct cfg80211_registered_device *rdev, else settings->sae_pwe = NL80211_SAE_PWE_UNSPECIFIED; + if (info->attrs[NL80211_ATTR_SAE_DH_GROUPS]) { + struct nlattr *dh_group; + int tmp, i = 0; + + if (!wiphy_ext_feature_isset(&rdev->wiphy, + NL80211_EXT_FEATURE_SAE_OFFLOAD) && + !wiphy_ext_feature_isset(&rdev->wiphy, + NL80211_EXT_FEATURE_SAE_OFFLOAD_AP)) + return -EINVAL; + + nla_for_each_nested(dh_group, info->attrs[NL80211_ATTR_SAE_DH_GROUPS], + tmp) { + settings->sae_dh_groups[i] = nla_get_u32(dh_group); + i++; + + if (i == CFG80211_MAX_NUM_SAE_DH_GROUPS) + break; + } + settings->n_sae_dh_groups = i; + } + return 0; }
In case of SAE authentication offload, the driver would need the info of SAE DH groups for both STA connection and soft AP. In the current change we update the SAE DH group support info to driver in the order/priority as provided by the supplicant/ upper layer. Signed-off-by: Vinayak Yadawad <vinayak.yadawad@broadcom.com> --- include/net/cfg80211.h | 6 ++++++ include/uapi/linux/nl80211.h | 7 +++++++ net/wireless/nl80211.c | 22 ++++++++++++++++++++++ 3 files changed, 35 insertions(+)