Received-SPF: pass (google.com: domain of linux-wireless-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <b98e76416ce4edba699f2bb4d456489e69ceeff1.camel@sipsolutions.net>
Subject: Re: [RFC 1/4] wifi: nl80211: advertise RU puncturing support to
 userspace
From:   Johannes Berg <johannes@sipsolutions.net>
To:     Muna Sinada <quic_msinada@quicinc.com>
Cc:     linux-wireless@vger.kernel.org,
        Aloka Dixit <quic_alokad@quicinc.com>
Date:   Thu, 19 Jan 2023 16:31:56 +0100
In-Reply-To: <1670006154-6092-2-git-send-email-quic_msinada@quicinc.com>
References: <1670006154-6092-1-git-send-email-quic_msinada@quicinc.com>
         <1670006154-6092-2-git-send-email-quic_msinada@quicinc.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
User-Agent: Evolution 3.46.3 (3.46.3-1.fc37) 
MIME-Version: 1.0
Precedence: bulk

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

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 punctu=
ring
> + *
> + * @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 o=
f 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 =3D=3D not supported in cfg80211.

johannes