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

[Johannes Berg <johannes@sipsolutions.net>]

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