diff mbox

[RFC,V2] cfg80211: Add feature flag for 4-way handshake offload

Message ID 1418935344-22159-1-git-send-email-arend@broadcom.com (mailing list archive)
State RFC
Headers show

Commit Message

Arend van Spriel Dec. 18, 2014, 8:42 p.m. UTC 
From: Gautam Kumar Shukla <gautams@broadcom.com>

The new feature flag allows the driver to indicate that it can
offload the 4-way handshake for WPA/RSN-PSK. With the
wiphy::features flag being used up this patch adds a new
field wiphy::ext_features. Considering extensibility this
new field is declared as a byte array.

Signed-off-by: Gautam (Gautam Kumar) Shukla <gautams@broadcom.com>
Signed-off-by: Arend van Spriel <arend@broadcom.com>
---
V2:
  - rename helper functions.
  - make helper function static inline.
  - derive ext_feature array size from extended feature count.
---
 include/net/cfg80211.h       | 39 +++++++++++++++++++++++++++++++++++++++
 include/uapi/linux/nl80211.h | 22 ++++++++++++++++++++++
 net/wireless/nl80211.c       |  5 +++++
 3 files changed, 66 insertions(+)

Comments

Johannes Berg Dec. 19, 2014, 9:18 a.m. UTC | #1 
On Thu, 2014-12-18 at 21:42 +0100, Arend van Spriel wrote:

> +	u8 ext_features[(NUM_NL80211_EXT_FEATURES / 8) + 1];

Should probably use DIV_ROUND_UP() here.

> +static inline bool
> +wiphy_ext_feature_isset(struct wiphy *wiphy,
> +			enum nl80211_ext_feature_index ftidx)
> +{
> +	u8 ft_byte;
> +
> +	ft_byte = wiphy->ext_features[ftidx / 8];
> +	return (ft_byte & BIT(ftidx % 8)) != 0;
> +}

Don't really need != 0 (but doesn't hurt either) since the type
promotion is to bool in the return value.

>  /**
> + * enum nl80211_ext_feature_index - bit index of extended features.
> + *
> + * @NL80211_EXT_FEATURE_4WAY_HANDSHAKE: the device supports 4way handshake
> + */
> +enum nl80211_ext_feature_index {
> +	NL80211_EXT_FEATURE_4WAY_HANDSHAKE,
> +
> +	/* add new features before the definition below */
> +	NUM_NL80211_EXT_FEATURES,
> +	MAX_NL80211_EXT_FEATURES = NUM_NL80211_EXT_FEATURES - 1
> +};

Don't think you need MAX_? Also you should add NUM_ to the documentation
(and MAX_ also if you want to keep it)

> +++ b/net/wireless/nl80211.c
> @@ -1603,6 +1603,11 @@ static int nl80211_send_wiphy(struct cfg80211_registered_device *rdev,
>  		if (nla_put_u32(msg, NL80211_ATTR_FEATURE_FLAGS, features))
>  			goto nla_put_failure;
>  
> +		if (nla_put(msg, NL80211_ATTR_EXT_FEATURES,
> +			    sizeof(rdev->wiphy.ext_features),
> +			    rdev->wiphy.ext_features))
> +			goto nla_put_failure;

I don't think this should go here - I can see how it's tempting to put
it with the feature flags but there's a size limit issue with the wiphy
data on old userspace, so this should go towards the end to deal with
the split data.

johannes

--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Arend van Spriel Dec. 19, 2014, 9:43 a.m. UTC | #2 
On 12/19/14 10:18, Johannes Berg wrote:
> On Thu, 2014-12-18 at 21:42 +0100, Arend van Spriel wrote:
>
>> +	u8 ext_features[(NUM_NL80211_EXT_FEATURES / 8) + 1];
>
> Should probably use DIV_ROUND_UP() here.

Will do.

>> +static inline bool
>> +wiphy_ext_feature_isset(struct wiphy *wiphy,
>> +			enum nl80211_ext_feature_index ftidx)
>> +{
>> +	u8 ft_byte;
>> +
>> +	ft_byte = wiphy->ext_features[ftidx / 8];
>> +	return (ft_byte&  BIT(ftidx % 8)) != 0;
>> +}
>
> Don't really need != 0 (but doesn't hurt either) since the type
> promotion is to bool in the return value.

It is a personal preference to use logical expression for a bool return.

>>   /**
>> + * enum nl80211_ext_feature_index - bit index of extended features.
>> + *
>> + * @NL80211_EXT_FEATURE_4WAY_HANDSHAKE: the device supports 4way handshake
>> + */
>> +enum nl80211_ext_feature_index {
>> +	NL80211_EXT_FEATURE_4WAY_HANDSHAKE,
>> +
>> +	/* add new features before the definition below */
>> +	NUM_NL80211_EXT_FEATURES,
>> +	MAX_NL80211_EXT_FEATURES = NUM_NL80211_EXT_FEATURES - 1
>> +};
>
> Don't think you need MAX_? Also you should add NUM_ to the documentation
> (and MAX_ also if you want to keep it)

True. Just added it for consistency with other enumerations. Maybe the 
API users can find good use for it ;-) I will add the kerneldoc.

>> +++ b/net/wireless/nl80211.c
>> @@ -1603,6 +1603,11 @@ static int nl80211_send_wiphy(struct cfg80211_registered_device *rdev,
>>   		if (nla_put_u32(msg, NL80211_ATTR_FEATURE_FLAGS, features))
>>   			goto nla_put_failure;
>>
>> +		if (nla_put(msg, NL80211_ATTR_EXT_FEATURES,
>> +			    sizeof(rdev->wiphy.ext_features),
>> +			    rdev->wiphy.ext_features))
>> +			goto nla_put_failure;
>
> I don't think this should go here - I can see how it's tempting to put
> it with the feature flags but there's a size limit issue with the wiphy
> data on old userspace, so this should go towards the end to deal with
> the split data.

I suspected that. Will change it.

Regards,
Arend
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Arend van Spriel Dec. 19, 2014, 9:59 a.m. UTC | #3 
On 12/19/14 10:43, Arend van Spriel wrote:
> On 12/19/14 10:18, Johannes Berg wrote:
>> On Thu, 2014-12-18 at 21:42 +0100, Arend van Spriel wrote:
>>
>>> +    u8 ext_features[(NUM_NL80211_EXT_FEATURES / 8) + 1];
>>
>> Should probably use DIV_ROUND_UP() here.
>
> Will do.

Looking at this again I wonder if it would be of value to have this as 
NL80211_EXT_FEATURES_ATTR_LEN in nl80211.h so user-space can check the 
nla_len() against it and detect API in-compatibility. Just can not use 
DIV_ROUND_UP() in that case. What's your opinion?

Regards,
Arend
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Johannes Berg Dec. 19, 2014, 10:05 a.m. UTC | #4 
On Fri, 2014-12-19 at 10:59 +0100, Arend van Spriel wrote:

> Looking at this again I wonder if it would be of value to have this as 
> NL80211_EXT_FEATURES_ATTR_LEN in nl80211.h so user-space can check the 
> nla_len() against it and detect API in-compatibility. Just can not use 
> DIV_ROUND_UP() in that case. What's your opinion?

Well, userspace must not check it since it should be compatible with new
kernels ... That was kinda my original point about not liking it in the
header to start with :)

johannes

--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Arend van Spriel Dec. 19, 2014, 10:15 a.m. UTC | #5 
On 12/19/14 11:05, Johannes Berg wrote:
> On Fri, 2014-12-19 at 10:59 +0100, Arend van Spriel wrote:
>
>> Looking at this again I wonder if it would be of value to have this as
>> NL80211_EXT_FEATURES_ATTR_LEN in nl80211.h so user-space can check the
>> nla_len() against it and detect API in-compatibility. Just can not use
>> DIV_ROUND_UP() in that case. What's your opinion?
>
> Well, userspace must not check it since it should be compatible with new
> kernels ... That was kinda my original point about not liking it in the
> header to start with :)

Understood.

Thanks,
Arend

--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h
index 670b9ed..38f30e5 100644
--- a/include/net/cfg80211.h
+++ b/include/net/cfg80211.h
@@ -3013,6 +3013,8 @@  struct wiphy_vendor_command {
  * @regulatory_flags: wiphy regulatory flags, see
  *	&enum ieee80211_regulatory_flags
  * @features: features advertised to nl80211, see &enum nl80211_feature_flags.
+ * @ext_features: extended features advertised to nl80211, see
+ *	&enum nl80211_ext_feature_index.
  * @bss_priv_size: each BSS struct has private data allocated with it,
  *	this variable determines its size
  * @max_scan_ssids: maximum number of SSIDs the device can scan for in
@@ -3122,6 +3124,7 @@  struct wiphy {
 	u16 max_acl_mac_addrs;
 
 	u32 flags, regulatory_flags, features;
+	u8 ext_features[(NUM_NL80211_EXT_FEATURES / 8) + 1];
 
 	u32 ap_sme_capa;
 
@@ -5049,6 +5052,42 @@  void cfg80211_stop_iface(struct wiphy *wiphy, struct wireless_dev *wdev,
  */
 void cfg80211_shutdown_all_interfaces(struct wiphy *wiphy);
 
+/**
+ * wiphy_ext_feature_set - set the extended feature flag
+ *
+ * @wiphy: the wiphy to modify.
+ * @ftidx: extended feature bit index.
+ *
+ * The extended features are flagged in multiple bytes (see
+ * &struct wiphy.@ext_features)
+ */
+static inline void wiphy_ext_feature_set(struct wiphy *wiphy,
+					 enum nl80211_ext_feature_index ftidx)
+{
+	u8 *ft_byte;
+
+	ft_byte = &wiphy->ext_features[ftidx / 8];
+	*ft_byte |= BIT(ftidx % 8);
+}
+
+/**
+ * wiphy_ext_feature_isset - check the extended feature flag
+ *
+ * @wiphy: the wiphy to modify.
+ * @ftidx: extended feature bit index.
+ *
+ * The extended features are flagged in multiple bytes (see
+ * &struct wiphy.@ext_features)
+ */
+static inline bool
+wiphy_ext_feature_isset(struct wiphy *wiphy,
+			enum nl80211_ext_feature_index ftidx)
+{
+	u8 ft_byte;
+
+	ft_byte = wiphy->ext_features[ftidx / 8];
+	return (ft_byte & BIT(ftidx % 8)) != 0;
+}
 
 /* ethtool helper */
 void cfg80211_get_drvinfo(struct net_device *dev, struct ethtool_drvinfo *info);
diff --git a/include/uapi/linux/nl80211.h b/include/uapi/linux/nl80211.h
index c0383e9..13f201b 100644
--- a/include/uapi/linux/nl80211.h
+++ b/include/uapi/linux/nl80211.h
@@ -1713,6 +1713,13 @@  enum nl80211_commands {
  *	obtained from it is coming from the device's wiphy and not the global
  *	cfg80211 regdomain.
  *
+ * @NL80211_ATTR_EXT_FEATURES: extended feature flags contained in a byte
+ *	array. The feature flags are identified by their bit index (see &enum
+ *	nl80211_ext_feature_index). The bit index is ordered starting at the
+ *	least-significant bit of the first byte in the array, ie. bit index 0
+ *	is located at bit 0 of byte 0. bit index 25 would be located at bit 1
+ *	of byte 3 (u8 array).
+ *
  * @NUM_NL80211_ATTR: total number of nl80211_attrs available
  * @NL80211_ATTR_MAX: highest attribute number currently defined
  * @__NL80211_ATTR_AFTER_LAST: internal use
@@ -2072,6 +2079,8 @@  enum nl80211_attrs {
 
 	NL80211_ATTR_WIPHY_SELF_MANAGED_REG,
 
+	NL80211_ATTR_EXT_FEATURES,
+
 	/* add attributes here, update the policy in nl80211.c */
 
 	__NL80211_ATTR_AFTER_LAST,
@@ -4221,6 +4230,19 @@  enum nl80211_feature_flags {
 };
 
 /**
+ * enum nl80211_ext_feature_index - bit index of extended features.
+ *
+ * @NL80211_EXT_FEATURE_4WAY_HANDSHAKE: the device supports 4way handshake
+ */
+enum nl80211_ext_feature_index {
+	NL80211_EXT_FEATURE_4WAY_HANDSHAKE,
+
+	/* add new features before the definition below */
+	NUM_NL80211_EXT_FEATURES,
+	MAX_NL80211_EXT_FEATURES = NUM_NL80211_EXT_FEATURES - 1
+};
+
+/**
  * enum nl80211_probe_resp_offload_support_attr - optional supported
  *	protocols for probe-response offloading by the driver/FW.
  *	To be used with the %NL80211_ATTR_PROBE_RESP_OFFLOAD attribute.
diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
index 7029201..7c00577 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -1603,6 +1603,11 @@  static int nl80211_send_wiphy(struct cfg80211_registered_device *rdev,
 		if (nla_put_u32(msg, NL80211_ATTR_FEATURE_FLAGS, features))
 			goto nla_put_failure;
 
+		if (nla_put(msg, NL80211_ATTR_EXT_FEATURES,
+			    sizeof(rdev->wiphy.ext_features),
+			    rdev->wiphy.ext_features))
+			goto nla_put_failure;
+
 		if (rdev->wiphy.ht_capa_mod_mask &&
 		    nla_put(msg, NL80211_ATTR_HT_CAPABILITY_MASK,
 			    sizeof(*rdev->wiphy.ht_capa_mod_mask),