[PATCH 1/1] wifi: nl80211: Add support for plumbing SAE groups to driver

[PATCH 1/1] wifi: nl80211: Add support for plumbing SAE groups to driver

[Vinayak Yadawad]

[-- Attachment #1: Type: text/plain, Size: 3591 bytes --]

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(+)

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;
 }
 
-- 
1.8.3.1


[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4218 bytes --]

Re: [PATCH 1/1] wifi: nl80211: Add support for plumbing SAE groups to driver

[Jeff Johnson]

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 <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(+)
> 
> 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;
>  }
>  

Re: [PATCH 1/1] wifi: nl80211: Add support for plumbing SAE groups to driver

[Johannes Berg]

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

Re: [PATCH 1/1] wifi: nl80211: Add support for plumbing SAE groups to driver

[Kalle Valo]

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?

-- 
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

Re: [PATCH 1/1] wifi: nl80211: Add support for plumbing SAE groups to driver

[Johannes Berg]

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

Re: [PATCH 1/1] wifi: nl80211: Add support for plumbing SAE groups to driver

[Arend van Spriel]

[-- Attachment #1: Type: text/plain, Size: 3322 bytes --]



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

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4219 bytes --]

Re: [PATCH 1/1] wifi: nl80211: Add support for plumbing SAE groups to driver

[Johannes Berg]

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

Re: [PATCH 1/1] wifi: nl80211: Add support for plumbing SAE groups to driver

[Arend van Spriel]

[-- Attachment #1: Type: text/plain, Size: 1976 bytes --]

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

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4219 bytes --]

Re: [PATCH 1/1] wifi: nl80211: Add support for plumbing SAE groups to driver

[Johannes Berg]

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

Re: [PATCH 1/1] wifi: nl80211: Add support for plumbing SAE groups to driver

[Arend van Spriel]

[-- Attachment #1: Type: text/plain, Size: 1361 bytes --]

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


[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4219 bytes --]

Re: [PATCH 1/1] wifi: nl80211: Add support for plumbing SAE groups to driver

[Johannes Berg]

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

Re: [PATCH 1/1] wifi: nl80211: Add support for plumbing SAE groups to driver

[Kalle Valo]

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

Re: [PATCH 1/1] wifi: nl80211: Add support for plumbing SAE groups to driver

[Jithu Jance]

[-- Attachment #1: Type: text/plain, Size: 3351 bytes --]

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

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4206 bytes --]

Re: [PATCH 1/1] wifi: nl80211: Add support for plumbing SAE groups to driver

[Kalle Valo]

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

-- 
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

Re: [PATCH 1/1] wifi: nl80211: Add support for plumbing SAE groups to driver

[Jakub Kicinski]

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.

Re: [PATCH 1/1] wifi: nl80211: Add support for plumbing SAE groups to driver

[Johannes Berg]

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

Re: [PATCH 1/1] wifi: nl80211: Add support for plumbing SAE groups to driver

[Jakub Kicinski]

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

Re: [PATCH 1/1] wifi: nl80211: Add support for plumbing SAE groups to driver

[Jeff Johnson]

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

Re: [PATCH 1/1] wifi: nl80211: Add support for plumbing SAE groups to driver

[Johannes Berg]

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