On 2/9/2024 5:50 AM, Vinayak Yadawad wrote:
> 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
who is we?
<https://www.kernel.org/doc/html/latest/process/submitting-patches.html#describe-your-changes>
Describe your changes in imperative mood, e.g. "make xyzzy do frotz"
instead of "[This patch] makes xyzzy do frotz" or "[I] changed xyzzy to
do frotz", as if you are giving orders to the codebase to change its
behaviour.
> to driver in the order/priority as provided by the supplicant/
> upper layer.
>
> Signed-off-by: Vinayak Yadawad <[email protected]>
> ---
> include/net/cfg80211.h | 6 ++++++
> include/uapi/linux/nl80211.h | 7 +++++++
> net/wireless/nl80211.c | 22 ++++++++++++++++++++++
> 3 files changed, 35 insertions(+)
>
> 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
suggest you be more specific and say this is a nest attribute containing
zero or more u32 attributes containing a DH Group number
> + * groups to driver. For STA role, the dh groups should be tried in the
s/dh/DH/
> + * 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 },
can/should this have a policy? not sure it can have a policy, so see
below...
> };
>
> /* 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);
...note that since you don't have a policy to ensure that each attribute
actually has 4 bytes of data, this can result in a buffer overread if
userspace provides less than 4 bytes of payload.
perhaps add logic like the following (borrowed from validate_scan_freqs():
if (nla_len(dh_group) != sizeof(u32))
return -EINVAL;
also if it passes the u32 size test, don't you also want to check each
value to make sure it is a valid DH Group #?
of course, this begs the question of whether cfg80211 or the individual
drivers should validate the DH Group values. if we don't validate in
cfg80211 then perhaps add to the documentation of @sae_dh_groups that
the values have not been validated
> + i++;
> +
> + if (i == CFG80211_MAX_NUM_SAE_DH_GROUPS)
> + break;
> + }
> + settings->n_sae_dh_groups = i;
> + }
> +
> return 0;
> }
>
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 <[email protected]> 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 <[email protected]>
[...]
> @@ -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?
--
https://patchwork.kernel.org/project/linux-wireless/list/
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
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 <[email protected]> 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
--
https://patchwork.kernel.org/project/linux-wireless/list/
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
Johannes Berg <[email protected]> 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
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 <[email protected]> wrote:
>
> Johannes Berg <[email protected]> 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 <[email protected]>
L: [email protected]
L: [email protected]
L: [email protected]
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 <[email protected]>
> L: [email protected]
> L: [email protected]
> L: [email protected]
> 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