| Message ID | 1670006154-6092-4-git-send-email-quic_msinada@quicinc.com (mailing list archive) |
|---|---|
| State | RFC |
| Delegated to: | Johannes Berg |
| Headers | show |
| Series | Static RU Puncturing | expand |
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, ¶ms); > + 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
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.
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
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.
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, ¶ms); + if (err < 0) + return err; + else if (err > 0) + change = true; + err = nl80211_parse_mon_options(rdev, ntype, info, ¶ms); if (err < 0) return err;