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: <3df39d4f2e2e64bd3899cfbe7bade547e54330f7.camel@sipsolutions.net>
Subject: Re: [RFC 3/4] wifi: nl80211: validate RU puncturing bitmap
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:37:47 +0100
In-Reply-To: <1670006154-6092-4-git-send-email-quic_msinada@quicinc.com>
References: <1670006154-6092-1-git-send-email-quic_msinada@quicinc.com>
         <1670006154-6092-4-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:
>=20
> +	[NL80211_ATTR_RU_PUNCT_SUPP_HE] =3D { .type =3D NLA_FLAG },
> +	[NL80211_ATTR_RU_PUNCT_BITMAP] =3D { .type =3D 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_devi=
ce *rdev,
> +					 struct net_device *dev,
> +					 struct genl_info *info,
> +					 struct vif_params *params)
> +{
> +	struct wireless_dev *wdev =3D dev->ieee80211_ptr;
> +	bool change =3D false;
> +
> +	if (info->attrs[NL80211_ATTR_RU_PUNCT_BITMAP]) {
> +		params->ru_punct_bitmap =3D
> +			 nla_get_u16(info->attrs[NL80211_ATTR_RU_PUNCT_BITMAP]);
> +
> +	if (!params->ru_punct_bitmap)
> +		return change;
> +
> +	params->ru_punct_bitmap_supp_he =3D
> +		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 =3D 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 *s=
kb, struct genl_info *info)
>  		params.use_4addr =3D -1;
>  	}
> =20
> +	err =3D nl80211_parse_ru_punct_bitmap(rdev, dev, info, &params);
> +	if (err < 0)
> +		return err;
> +	else if (err > 0)
> +		change =3D true;
>=20

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