[RFC 0/4] Static RU Puncturing

[RFC 0/4] Static RU Puncturing

[Muna Sinada]

Add Static RU Puncturing feature for AP mode. This patchset is dependent on
the following RFC:
        https://patchwork.kernel.org/project/linux-wireless/patch/20220325140859.e48bf244f157.I3547481d49f958389f59dfeba3fcc75e72b0aa6e@changeid/

This feature has been tested

Aloka Dixit (3):
  wifi: nl80211: advertise RU puncturing support to userspace
  wifi: cfg80211: RU puncturing bitmap
  wifi: nl80211: validate RU puncturing bitmap

Muna Sinada (1):
  wifi: mac80211: Handle RU Puncturing information

 include/net/cfg80211.h       | 22 +++++++++++++++++
 include/uapi/linux/nl80211.h | 35 ++++++++++++++++++++++++++
 net/mac80211/cfg.c           |  8 ++++++
 net/mac80211/chan.c          | 14 +++++++++--
 net/wireless/core.c          |  4 +++
 net/wireless/nl80211.c       | 59 ++++++++++++++++++++++++++++++++++++++++++++
 6 files changed, 140 insertions(+), 2 deletions(-)

-- 
2.7.4

[RFC 1/4] wifi: nl80211: advertise RU puncturing support to userspace

[Muna Sinada]

From: Aloka Dixit <quic_alokad@quicinc.com>

RU preamble puncturing is allowed for bandwidths more than 80 MHz
except 80+80. Drivers may not support puncturing at all or have
restrictions for a minimum bandwidth.
Add new attribute NL80211_ATTR_RU_PUNCT_SUPP_BW to advertise the
driver support to the userspace. Default value (0) will indicate that
RU puncturing is not supported.

Signed-off-by: Aloka Dixit <quic_alokad@quicinc.com>
---
 include/net/cfg80211.h       |  6 ++++++
 include/uapi/linux/nl80211.h | 25 +++++++++++++++++++++++++
 net/wireless/core.c          |  4 ++++
 net/wireless/nl80211.c       | 19 +++++++++++++++++++
 4 files changed, 54 insertions(+)

diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h
index 11a370e64143..cb856b06a3ac 100644
--- a/include/net/cfg80211.h
+++ b/include/net/cfg80211.h
@@ -5299,6 +5299,10 @@ struct wiphy_iftype_akm_suites {
  *	NL80211_MAX_NR_AKM_SUITES in order to avoid compatibility issues with
  *	legacy userspace and maximum allowed value is
  *	CFG80211_MAX_NUM_AKM_SUITES.
+ *
+ * @ru_punct_supp_bw: Whether the driver supports RU puncturing, and if so,
+ *     for which bandwidths. See &enum nl80211_ru_punct_supp_bw for the
+ *     possible values for this field.
  */
 struct wiphy {
 	struct mutex mtx;
@@ -5447,6 +5451,8 @@ struct wiphy {
 	u8 ema_max_profile_periodicity;
 	u16 max_num_akm_suites;
 
+	enum nl80211_ru_punct_supp_bw ru_punct_supp_bw;
+
 	char priv[] __aligned(NETDEV_ALIGN);
 };
 
diff --git a/include/uapi/linux/nl80211.h b/include/uapi/linux/nl80211.h
index c14a91bbca7c..e5218fd7b37b 100644
--- a/include/uapi/linux/nl80211.h
+++ b/include/uapi/linux/nl80211.h
@@ -2751,7 +2751,13 @@ enum nl80211_commands {
  *	the incoming frame RX timestamp.
  * @NL80211_ATTR_TD_BITMAP: Transition Disable bitmap, for subsequent
  *	(re)associations.
+ *
+ * @NL80211_ATTR_RU_PUNCT_SUPP_BW: (u8) Minimum bandwidth for which
+ *	the driver supports preamble puncturing, value should be of type
+ *	&enum nl80211_ru_punct_supp_bw
+ *
  * @NUM_NL80211_ATTR: total number of nl80211_attrs available
+ *
  * @NL80211_ATTR_MAX: highest attribute number currently defined
  * @__NL80211_ATTR_AFTER_LAST: internal use
  */
@@ -3280,6 +3286,8 @@ enum nl80211_attrs {
 	NL80211_ATTR_RX_HW_TIMESTAMP,
 	NL80211_ATTR_TD_BITMAP,
 
+	NL80211_ATTR_RU_PUNCT_SUPP_BW,
+
 	/* add attributes here, update the policy in nl80211.c */
 
 	__NL80211_ATTR_AFTER_LAST,
@@ -7723,4 +7731,21 @@ enum nl80211_ap_settings_flags {
 	NL80211_AP_SETTINGS_SA_QUERY_OFFLOAD_SUPPORT	= 1 << 1,
 };
 
+/**
+ * enum nl80211_ru_punct_supp_bw - Bandwidths supporting preamble puncturing
+ *
+ * @NL80211_RU_PUNCT_NOT_SUPP: preamble puncturing is not supported
+ * @NL80211_RU_PUNCT_SUPP_BW_80: puncturing supported within channels of at
+ *	least 80 MHz bandwidth
+ * @NL80211_RU_PUNCT_SUPP_BW_160: puncturing supported within channels of at
+ *	least 160 MHz bandwidth
+ * @NL80211_RU_PUNCT_SUPP_BW_320: puncturing supported within 320 MHz.
+ */
+enum nl80211_ru_punct_supp_bw {
+	NL80211_RU_PUNCT_NOT_SUPP,
+	NL80211_RU_PUNCT_SUPP_BW_80,
+	NL80211_RU_PUNCT_SUPP_BW_160,
+	NL80211_RU_PUNCT_SUPP_BW_320,
+};
+
 #endif /* __LINUX_NL80211_H */
diff --git a/net/wireless/core.c b/net/wireless/core.c
index 5b0c4d5b80cf..aa2a49a8ccbe 100644
--- a/net/wireless/core.c
+++ b/net/wireless/core.c
@@ -908,6 +908,10 @@ int wiphy_register(struct wiphy *wiphy)
 		return -EINVAL;
 	}
 
+	if (WARN_ON(rdev->wiphy.ru_punct_supp_bw >
+		    NL80211_RU_PUNCT_SUPP_BW_320))
+		return -EINVAL;
+
 	for (i = 0; i < rdev->wiphy.n_vendor_commands; i++) {
 		/*
 		 * Validate we have a policy (can be explicitly set to
diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
index 148f66edb015..4b4cb3c64f62 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -805,6 +805,8 @@ static const struct nla_policy nl80211_policy[NUM_NL80211_ATTR] = {
 	[NL80211_ATTR_MLD_ADDR] = NLA_POLICY_EXACT_LEN(ETH_ALEN),
 	[NL80211_ATTR_MLO_SUPPORT] = { .type = NLA_FLAG },
 	[NL80211_ATTR_MAX_NUM_AKM_SUITES] = { .type = NLA_REJECT },
+	[NL80211_ATTR_RU_PUNCT_SUPP_BW] =
+		NLA_POLICY_MAX(NLA_U8, NL80211_RU_PUNCT_SUPP_BW_320),
 };
 
 /* policy for the key attributes */
@@ -2355,6 +2357,20 @@ static int nl80211_put_mbssid_support(struct wiphy *wiphy, struct sk_buff *msg)
 	return -ENOBUFS;
 }
 
+static int
+nl80211_put_ru_punct_supp_bw(struct cfg80211_registered_device *rdev,
+			     struct sk_buff *msg)
+{
+	if (rdev->wiphy.ru_punct_supp_bw == NL80211_RU_PUNCT_NOT_SUPP)
+		return 0;
+
+	if (nla_put_u8(msg, NL80211_ATTR_RU_PUNCT_SUPP_BW,
+		       rdev->wiphy.ru_punct_supp_bw))
+		return -ENOBUFS;
+
+	return 0;
+}
+
 struct nl80211_dump_wiphy_state {
 	s64 filter_wiphy;
 	long start;
@@ -2959,6 +2975,9 @@ static int nl80211_send_wiphy(struct cfg80211_registered_device *rdev,
 		if (rdev->wiphy.flags & WIPHY_FLAG_SUPPORTS_MLO)
 			nla_put_flag(msg, NL80211_ATTR_MLO_SUPPORT);
 
+		if (nl80211_put_ru_punct_supp_bw(rdev, msg))
+			goto nla_put_failure;
+
 		/* done */
 		state->split_start = 0;
 		break;
-- 
2.7.4

Re: [RFC 1/4] wifi: nl80211: advertise RU puncturing support to userspace

[Kalle Valo]

Muna Sinada <quic_msinada@quicinc.com> writes:

> From: Aloka Dixit <quic_alokad@quicinc.com>
>
> RU preamble puncturing is allowed for bandwidths more than 80 MHz
> except 80+80. Drivers may not support puncturing at all or have
> restrictions for a minimum bandwidth.
> Add new attribute NL80211_ATTR_RU_PUNCT_SUPP_BW to advertise the
> driver support to the userspace. Default value (0) will indicate that
> RU puncturing is not supported.
>
> Signed-off-by: Aloka Dixit <quic_alokad@quicinc.com>

Missing submitter's s-o-b.

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

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

Re: [RFC 1/4] wifi: nl80211: advertise RU puncturing support to userspace

[Johannes Berg]

On Fri, 2022-12-02 at 10:35 -0800, Muna Sinada wrote:
> From: Aloka Dixit <quic_alokad@quicinc.com>
> [...]
> Signed-off-by: Aloka Dixit <quic_alokad@quicinc.com>

You should probably add your S-o-b since you handled the patch.

> + * @ru_punct_supp_bw: Whether the driver supports RU puncturing, and if so,
> + *     for which bandwidths.
> 

I find the definition in nl80211 to be clearer which talks about minimum
bandwidth, to be honest.

>   * @NUM_NL80211_ATTR: total number of nl80211_attrs available
> + *

unneeded blank line?

> +/**
> + * enum nl80211_ru_punct_supp_bw - Bandwidths supporting preamble puncturing
> + *
> + * @NL80211_RU_PUNCT_NOT_SUPP: preamble puncturing is not supported

I see why you did this (cfg80211) but can't say I like it since in
netlink terms we'd just not include the attribute in that case ...

> + * @NL80211_RU_PUNCT_SUPP_BW_80: puncturing supported within channels of at
> + *	least 80 MHz bandwidth
> + * @NL80211_RU_PUNCT_SUPP_BW_160: puncturing supported within channels of at
> + *	least 160 MHz bandwidth
> + * @NL80211_RU_PUNCT_SUPP_BW_320: puncturing supported within 320 MHz.
> + */
> +enum nl80211_ru_punct_supp_bw {
> +	NL80211_RU_PUNCT_NOT_SUPP,
> +	NL80211_RU_PUNCT_SUPP_BW_80,
> +	NL80211_RU_PUNCT_SUPP_BW_160,
> +	NL80211_RU_PUNCT_SUPP_BW_320,
> +};

Btw why is this a minimum bandwidth - couldn't this be a bitmap of
bandwidths for example? I mean, is there a technical requirement that if
you do it in 80, you can do it in 160? It's probably true in general,
but maybe we can just completely avoid defining the new enum by saying

 u32 puncturing_widths;

and put there BIT(NL80211_CHAN_WIDTH_80), BIT(NL80211_CHAN_WIDTH_160)
and BIT(NL80211_CHAN_WIDTH_320) - and maybe even
BIT(NL80211_CHAN_WIDTH_80P80) which could conceivably be harder/less
supported, and you haven't covered (is it part of your 80+, 160+ or not
covered at all?)

That way also 0 == not supported in cfg80211.

johannes

[RFC 2/4] wifi: cfg80211: RU puncturing bitmap

[Muna Sinada]

From: Aloka Dixit <quic_alokad@quicinc.com>

RU puncturing bitmap consists of 16 bits, each bit corresponding to
a 20 MHz channel in the operating bandwidth. Lowest bit corresponds to
the lowest frequency. Bit set to 1 indicates that the channel is
punctured otherwise it is active.

Signed-off-by: Aloka Dixit <quic_alokad@quicinc.com>
Co-developed-by: Muna Sinada <quic_msinada@quicinc.com>
Signed-off-by: Muna Sinada <quic_msinada@quicinc.com>
---
 include/net/cfg80211.h | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h
index cb856b06a3ac..b4ca8c2118ff 100644
--- a/include/net/cfg80211.h
+++ b/include/net/cfg80211.h
@@ -698,6 +698,11 @@ static inline void wiphy_read_of_freq_limits(struct wiphy *wiphy)
  *	belonging to that MU-MIMO groupID; %NULL if not changed
  * @vht_mumimo_follow_addr: MU-MIMO follow address, used for monitoring
  *	MU-MIMO packets going to the specified station; %NULL if not changed
+ * @ru_punct_bitmap: RU puncturing bitmap. Each bit represents a 20 MHz channel
+ *	with lowest bit corresponding to the lowest frequency. Bit set to 1
+ *	indicates that the channel is punctured, otherwise the channel is active
+ * @ru_punct_bitmap_supp_he: Indicates whether RU puncturing bitmap validation
+ *	should include OFDMA bitmaps.
  */
 struct vif_params {
 	u32 flags;
@@ -705,6 +710,8 @@ struct vif_params {
 	u8 macaddr[ETH_ALEN];
 	const u8 *vht_mumimo_groups;
 	const u8 *vht_mumimo_follow_addr;
+	u16 ru_punct_bitmap;
+	bool ru_punct_bitmap_supp_he;
 };
 
 /**
@@ -5732,6 +5739,12 @@ static inline void wiphy_unlock(struct wiphy *wiphy)
  * @links: array of %IEEE80211_MLD_MAX_NUM_LINKS elements containing @addr
  *	@ap and @client for each link
  * @valid_links: bitmap describing what elements of @links are valid
+ * @ru_punct_bitmap: (private) RU puncturing bitmap. Each bit represents a
+ *	20 MHz channel with lowest bit corresponding to the lowest frequency.
+ *	Bit set to 1 indicates that the channel is punctured, otherwise the
+ *	channel is active
+ * @ru_punct_bitmap_supp_he: (private) Indicates whether RU puncturing bitmap
+ *	validation should include OFDMA bitmaps.
  */
 struct wireless_dev {
 	struct wiphy *wiphy;
@@ -5846,6 +5859,9 @@ struct wireless_dev {
 		};
 	} links[IEEE80211_MLD_MAX_NUM_LINKS];
 	u16 valid_links;
+
+	u16 ru_punct_bitmap;
+	bool ru_punct_bitmap_supp_he;
 };
 
 static inline const u8 *wdev_address(struct wireless_dev *wdev)
-- 
2.7.4

Re: [RFC 2/4] wifi: cfg80211: RU puncturing bitmap

[Johannes Berg]

On Fri, 2022-12-02 at 10:35 -0800, Muna Sinada wrote:
> 
> + * @ru_punct_bitmap: RU puncturing bitmap. Each bit represents a 20 MHz channel
> + *	with lowest bit corresponding to the lowest frequency. Bit set to 1
> + *	indicates that the channel is punctured, otherwise the channel is active
> + * @ru_punct_bitmap_supp_he: Indicates whether RU puncturing bitmap validation
> + *	should include OFDMA bitmaps.

Why _supp_he and not _supp_ofdma? Is it restricted to HE? What about
EHT?

I'm also not really sure what that kernel-doc means for the _supp_he
part?


Also this patch is basically empty - please squash appropriately.

johannes

[RFC 3/4] wifi: nl80211: validate RU puncturing bitmap

[Muna Sinada]

From: Aloka Dixit <quic_alokad@quicinc.com>

Add new attributes NL80211_ATTR_RU_PUNCT_BITMAP and
NL80211_ATTR_RU_PUNCT_SUPP_HE to receive RU puncturing information
from the userspace.
- Bitmap consists of 16 bits, each bit corresponding to a 20 MHz
channel in the operating bandwidth. Lowest bit corresponds to
the lowest frequency. Validate the bitmap against the minimum
bandwidth support advertised by the driver.
- HE support flag indicates whether OFDMA puncturing patterns
should be considered during validation.

Signed-off-by: Aloka Dixit <quic_alokad@quicinc.com>
Co-developed-by: Muna Sinada <quic_msinada@quicinc.com>
Signed-off-by: Muna Sinada <quic_msinada@quicinc.com>
---
 include/uapi/linux/nl80211.h | 10 ++++++++++
 net/wireless/nl80211.c       | 40 ++++++++++++++++++++++++++++++++++++++++
 2 files changed, 50 insertions(+)

diff --git a/include/uapi/linux/nl80211.h b/include/uapi/linux/nl80211.h
index e5218fd7b37b..286579d56809 100644
--- a/include/uapi/linux/nl80211.h
+++ b/include/uapi/linux/nl80211.h
@@ -2756,6 +2756,14 @@ enum nl80211_commands {
  *	the driver supports preamble puncturing, value should be of type
  *	&enum nl80211_ru_punct_supp_bw
  *
+ * @NL80211_ATTR_RU_PUNCT_SUPP_HE: flag attribute, used to indicate that RU
+ *	puncturing bitmap validation should include OFDMA bitmaps.
+ *
+ * @NL80211_ATTR_RU_PUNCT_BITMAP: (u16) RU puncturing bitmap where the lowest
+ *	bit corresponds to the lowest 20 MHz channel. Each bit set to 1
+ *	indicates that the sub-channel is punctured, set 0 indicates that the
+ *	channel is active.
+ *
  * @NUM_NL80211_ATTR: total number of nl80211_attrs available
  *
  * @NL80211_ATTR_MAX: highest attribute number currently defined
@@ -3287,6 +3295,8 @@ enum nl80211_attrs {
 	NL80211_ATTR_TD_BITMAP,
 
 	NL80211_ATTR_RU_PUNCT_SUPP_BW,
+	NL80211_ATTR_RU_PUNCT_SUPP_HE,
+	NL80211_ATTR_RU_PUNCT_BITMAP,
 
 	/* add attributes here, update the policy in nl80211.c */
 
diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
index 4b4cb3c64f62..fd7d83c533a8 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -807,6 +807,8 @@ static const struct nla_policy nl80211_policy[NUM_NL80211_ATTR] = {
 	[NL80211_ATTR_MAX_NUM_AKM_SUITES] = { .type = NLA_REJECT },
 	[NL80211_ATTR_RU_PUNCT_SUPP_BW] =
 		NLA_POLICY_MAX(NLA_U8, NL80211_RU_PUNCT_SUPP_BW_320),
+	[NL80211_ATTR_RU_PUNCT_SUPP_HE] = { .type = NLA_FLAG },
+	[NL80211_ATTR_RU_PUNCT_BITMAP] = { .type = NLA_U16 },
 };
 
 /* policy for the key attributes */
@@ -3192,6 +3194,38 @@ static bool nl80211_can_set_dev_channel(struct wireless_dev *wdev)
 		wdev->iftype == NL80211_IFTYPE_P2P_GO;
 }
 
+static int nl80211_parse_ru_punct_bitmap(struct cfg80211_registered_device *rdev,
+					 struct net_device *dev,
+					 struct genl_info *info,
+					 struct vif_params *params)
+{
+	struct wireless_dev *wdev = dev->ieee80211_ptr;
+	bool change = false;
+
+	if (info->attrs[NL80211_ATTR_RU_PUNCT_BITMAP]) {
+		params->ru_punct_bitmap =
+			 nla_get_u16(info->attrs[NL80211_ATTR_RU_PUNCT_BITMAP]);
+
+	if (!params->ru_punct_bitmap)
+		return change;
+
+	params->ru_punct_bitmap_supp_he =
+		nla_get_flag(info->attrs[NL80211_ATTR_RU_PUNCT_SUPP_HE]);
+
+	if (!rdev->wiphy.ru_punct_supp_bw &&
+	    (params->ru_punct_bitmap || params->ru_punct_bitmap_supp_he))
+		return -EOPNOTSUPP;
+
+	changed = true;
+
+	wdev_lock(wdev);
+	wdev->ru_punct_bitmap_supp_he = params->ru_punct_bitmap_supp_he;
+	wdev->ru_punct_bitmap = params->ru_punct_bitmap;
+	wdev_unlock(wdev);
+
+	return change ? 1 : 0;
+}
+
 int nl80211_parse_chandef(struct cfg80211_registered_device *rdev,
 			  struct genl_info *info,
 			  struct cfg80211_chan_def *chandef)
@@ -4175,6 +4209,12 @@ static int nl80211_set_interface(struct sk_buff *skb, struct genl_info *info)
 		params.use_4addr = -1;
 	}
 
+	err = nl80211_parse_ru_punct_bitmap(rdev, dev, info, &params);
+	if (err < 0)
+		return err;
+	else if (err > 0)
+		change = true;
+
 	err = nl80211_parse_mon_options(rdev, ntype, info, &params);
 	if (err < 0)
 		return err;
-- 
2.7.4

Re: [RFC 3/4] wifi: nl80211: validate RU puncturing bitmap

[Johannes Berg]

On Fri, 2022-12-02 at 10:35 -0800, Muna Sinada wrote:
> 
> +	[NL80211_ATTR_RU_PUNCT_SUPP_HE] = { .type = NLA_FLAG },
> +	[NL80211_ATTR_RU_PUNCT_BITMAP] = { .type = NLA_U16 },


I feel like at least in the API we should make that bigger already,
maybe U32 or even U64, at some point they're going to come up with wider
channels... should use NLA_RANGE or something to restrict it for now
though.


> +static int nl80211_parse_ru_punct_bitmap(struct cfg80211_registered_device *rdev,
> +					 struct net_device *dev,
> +					 struct genl_info *info,
> +					 struct vif_params *params)
> +{
> +	struct wireless_dev *wdev = dev->ieee80211_ptr;
> +	bool change = false;
> +
> +	if (info->attrs[NL80211_ATTR_RU_PUNCT_BITMAP]) {
> +		params->ru_punct_bitmap =
> +			 nla_get_u16(info->attrs[NL80211_ATTR_RU_PUNCT_BITMAP]);
> +
> +	if (!params->ru_punct_bitmap)
> +		return change;
> +
> +	params->ru_punct_bitmap_supp_he =
> +		nla_get_flag(info->attrs[NL80211_ATTR_RU_PUNCT_SUPP_HE]);
> +
> +	if (!rdev->wiphy.ru_punct_supp_bw &&
> +	    (params->ru_punct_bitmap || params->ru_punct_bitmap_supp_he))
> +		return -EOPNOTSUPP;
> +
> +	changed = true;

That doesn't even compile, right? :)

But you can get rid of the 'change' variable entirely.

> @@ -4175,6 +4209,12 @@ static int nl80211_set_interface(struct sk_buff *skb, struct genl_info *info)
>  		params.use_4addr = -1;
>  	}
>  
> +	err = nl80211_parse_ru_punct_bitmap(rdev, dev, info, &params);
> +	if (err < 0)
> +		return err;
> +	else if (err > 0)
> +		change = true;
> 

Frankly I'm not happy for this to be stuck into set_interface() like
that - can we add it to set_channel() only or something? At least there
should be too? And read it only if the channel is actually touched? This
feels very ad-hoc.

johannes

Re: [RFC 3/4] wifi: nl80211: validate RU puncturing bitmap

[Aloka Dixit]

On 1/19/2023 7:37 AM, Johannes Berg wrote:
> Frankly I'm not happy for this to be stuck into set_interface() like
> that - can we add it to set_channel() only or something? At least there
> should be too? And read it only if the channel is actually touched? This
> feels very ad-hoc.
> 
> johannes

Hi Johannes,

I will work on these comments.

Secondly, this RFC uses ieee80211_valid_disable_subchannel_bitmap() 
defined in following RFC you sent:
https://patchwork.kernel.org/project/linux-wireless/patch/20220325140859.e48bf244f157.I3547481d49f958389f59dfeba3fcc75e72b0aa6e@changeid/

Is there is any plan for the next version?
If not, I can move the validation function defined in cfg80211 to 
mac80211 from the following patch-set:
https://patchwork.kernel.org/project/linux-wireless/patch/20220214223051.3610-3-quic_alokad@quicinc.com/

You had asked me about exporting this during the review as well.

Please let me know so that I can incorporate accordingly.
Thanks.

Re: [RFC 3/4] wifi: nl80211: validate RU puncturing bitmap

[Johannes Berg]

Hi Aloka,

> Secondly, this RFC uses ieee80211_valid_disable_subchannel_bitmap() 
> defined in following RFC you sent:
> https://patchwork.kernel.org/project/linux-wireless/patch/20220325140859.e48bf244f157.I3547481d49f958389f59dfeba3fcc75e72b0aa6e@changeid/

Yes, I saw that.

> Is there is any plan for the next version?

Am I correct that by basing your work here on top of that, you're OK
with that being included, and we've concluded the discussion about where
and how the puncturing bitmap should be stored? This patchset was an
RFC, I'm personally happy with the design, but really also want to hear
your opinion and perspective on it.


Anyway if that's the case then I'll go and resubmit the patch - we also
made some more fixes to it since, I think. I'm not sure I'll get to it
today, but I'll do it soon.

johannes

Re: [RFC 3/4] wifi: nl80211: validate RU puncturing bitmap

[Aloka Dixit]

On 1/20/2023 1:20 AM, Johannes Berg wrote:
> Hi Aloka,
> 
>> Secondly, this RFC uses ieee80211_valid_disable_subchannel_bitmap()
>> defined in following RFC you sent:
>> https://patchwork.kernel.org/project/linux-wireless/patch/20220325140859.e48bf244f157.I3547481d49f958389f59dfeba3fcc75e72b0aa6e@changeid/
> 
> Yes, I saw that.
> 
>> Is there is any plan for the next version?
> 
> Am I correct that by basing your work here on top of that, you're OK
> with that being included, and we've concluded the discussion about where
> and how the puncturing bitmap should be stored? This patchset was an
> RFC, I'm personally happy with the design, but really also want to hear
> your opinion and perspective on it.
> 
> 
> Anyway if that's the case then I'll go and resubmit the patch - we also
> made some more fixes to it since, I think. I'm not sure I'll get to it
> today, but I'll do it soon.
> 
> johannes

Yes, we are okay with the bitmap being part of the bss_conf/link_conf.
Please send the new version with fixes as soon as you can, I'm still 
looking into your comments regarding the design Muna sent.

[RFC 4/4] wifi: mac80211: Handle RU Puncturing information

[Muna Sinada]

Handle RU Puncturing information received from user space.
RU Puncturing bitmap is initially received during
ieee80211_change_iface() and stored. During AP chanwidth setting,
the bitmap is validated. In addition driver is notified of new bitmap
value.

Signed-off-by: Muna Sinada <quic_msinada@quicinc.com>
---
 net/mac80211/cfg.c  |  8 ++++++++
 net/mac80211/chan.c | 14 ++++++++++++--
 2 files changed, 20 insertions(+), 2 deletions(-)

diff --git a/net/mac80211/cfg.c b/net/mac80211/cfg.c
index c848fe04dd44..d82060d8fd4e 100644
--- a/net/mac80211/cfg.c
+++ b/net/mac80211/cfg.c
@@ -213,6 +213,12 @@ static int ieee80211_change_iface(struct wiphy *wiphy,
 	struct sta_info *sta;
 	int ret;
 
+	sdata->vif.bss_conf.ru_punct_bitmap = params->ru_punct_bitmap;
+	sdata->vif.bss_conf.ru_punct_bitmap_supp_he = params->ru_punct_bitmap_supp_he;
+
+	if (!sdata->vif.bss_conf.ru_punct_bitmap)
+		sdata_dbg(sdata, "RU Puncturing Bitmap was not set by user\n");
+
 	ret = ieee80211_if_change_type(sdata, type);
 	if (ret)
 		return ret;
@@ -1251,6 +1257,8 @@ static int ieee80211_start_ap(struct wiphy *wiphy, struct net_device *dev,
 	prev_beacon_int = link_conf->beacon_int;
 	link_conf->beacon_int = params->beacon_interval;
 
+	sdata->vif.bss_conf.ru_punct_bitmap = dev->ieee80211_ptr->ru_punct_bitmap;
+
 	if (params->he_cap && params->he_oper) {
 		link_conf->he_support = true;
 		link_conf->htc_trig_based_pkt_ext =
diff --git a/net/mac80211/chan.c b/net/mac80211/chan.c
index e72cf0749d49..55a1f8bb309c 100644
--- a/net/mac80211/chan.c
+++ b/net/mac80211/chan.c
@@ -838,6 +838,7 @@ static int ieee80211_assign_link_chanctx(struct ieee80211_link_data *link,
 	struct ieee80211_local *local = sdata->local;
 	struct ieee80211_chanctx_conf *conf;
 	struct ieee80211_chanctx *curr_ctx = NULL;
+	u64 changed;
 	int ret = 0;
 
 	if (WARN_ON(sdata->vif.type == NL80211_IFTYPE_NAN))
@@ -882,8 +883,10 @@ static int ieee80211_assign_link_chanctx(struct ieee80211_link_data *link,
 	}
 
 	if (sdata->vif.type != NL80211_IFTYPE_P2P_DEVICE &&
-	    sdata->vif.type != NL80211_IFTYPE_MONITOR)
-		ieee80211_vif_cfg_change_notify(sdata, BSS_CHANGED_IDLE);
+	    sdata->vif.type != NL80211_IFTYPE_MONITOR) {
+		changed |= BSS_CHANGED_IDLE | BSS_CHANGED_EHT_PUNCTURING;
+		ieee80211_vif_cfg_change_notify(sdata, changed);
+	}
 
 	ieee80211_check_fast_xmit_iface(sdata);
 
@@ -1942,6 +1945,13 @@ int ieee80211_link_change_bandwidth(struct ieee80211_link_data *link,
 		goto out;
 	}
 
+	if (!ieee80211_valid_disable_subchannel_bitmap(
+					   &sdata->vif.bss_conf.ru_punct_bitmap,
+					   chandef->width)) {
+		ret = -EINVAL;
+		goto out;
+	}
+
 	conf = rcu_dereference_protected(link_conf->chanctx_conf,
 					 lockdep_is_held(&local->chanctx_mtx));
 	if (!conf) {
-- 
2.7.4

Re: [RFC 4/4] wifi: mac80211: Handle RU Puncturing information

[Johannes Berg]

On Fri, 2022-12-02 at 10:35 -0800, Muna Sinada wrote:
> Handle RU Puncturing information received from user space.
> RU Puncturing bitmap is initially received during
> ieee80211_change_iface() and stored. During AP chanwidth setting,
> the bitmap is validated. In addition driver is notified of new bitmap
> value.


"initially" is a bit of a problem, right?


> @@ -213,6 +213,12 @@ static int ieee80211_change_iface(struct wiphy *wiphy,
>  	struct sta_info *sta;
>  	int ret;
>  
> +	sdata->vif.bss_conf.ru_punct_bitmap = params->ru_punct_bitmap;
> +	sdata->vif.bss_conf.ru_punct_bitmap_supp_he = params->ru_punct_bitmap_supp_he;

I mean this can happen at any point in time due to the way you've wired
it up in nl80211 (which I'm not happy about), and then ... it just gets
ignored.

> +	if (!sdata->vif.bss_conf.ru_punct_bitmap)
> +		sdata_dbg(sdata, "RU Puncturing Bitmap was not set by user\n");
> +

?

> @@ -1251,6 +1257,8 @@ static int ieee80211_start_ap(struct wiphy *wiphy, struct net_device *dev,
>  	prev_beacon_int = link_conf->beacon_int;
>  	link_conf->beacon_int = params->beacon_interval;
>  
> +	sdata->vif.bss_conf.ru_punct_bitmap = dev->ieee80211_ptr->ru_punct_bitmap;

Why grab only one of them here? In fact why grab them at all?

I think you really need to work on the API here.

Also this completely ignores links ... so it's wrong for that too. Must
use link_conf->...

johannes