Re: [PATCH v3 1/6] wifi: nl80211: configure puncturing bitmap in NL80211_CMD_START_AP

[Johannes Berg <johannes@sipsolutions.net>]

On Sun, 2023-01-29 at 23:22 -0800, Aloka Dixit wrote:
> 
> v3: Validation and storing the bitmap moved to MAC80211.

I think I'd prefer we move the validation function to cfg80211 so both
can use it, this way all potential non-mac80211 drivers have to do it as
well, and then they'll move the function _anyway_ to do the validation
in a single place, I'd hope?

> + * @punct_bitmap: Preamble puncturing bitmap. Each bit represents a 20 MHz
> + *	channel, lowest bit corresponding to the lowest frequency. Bit set
> + *	to 1 indicates that the channel is punctured. Higher 16 bits are
> + *	reserved.
>   */
>  struct cfg80211_ap_settings {
>  	struct cfg80211_chan_def chandef;
> @@ -1350,6 +1354,7 @@ struct cfg80211_ap_settings {
>  	struct cfg80211_fils_discovery fils_discovery;
>  	struct cfg80211_unsol_bcast_probe_resp unsol_bcast_probe_resp;
>  	struct cfg80211_mbssid_config mbssid_config;
> +	u32 punct_bitmap;

Internally I think we can continue to use u16, that's trivial to change
later.

> + * @NL80211_EXT_FEATURE_EHT_PUNCTURING: Driver supports preamble puncturing in
> + *	EHT.

That should probably make some mention of AP mode? It's not optional in
any way for client, after all, and also not relevant to the API how
client does it.

> +static int nl80211_parse_punct_bitmap(struct cfg80211_registered_device *rdev,
> +				      struct genl_info *info,
> +				      u32 *bitmap)
> +{
> +	if (!bitmap ||
> +	    !wiphy_ext_feature_isset(&rdev->wiphy,
> +				     NL80211_EXT_FEATURE_EHT_PUNCTURING))
> +		return -EINVAL;
> +
> +	*bitmap = nla_get_u32(info->attrs[NL80211_ATTR_PUNCT_BITMAP]) & 0xFFFF;

As the top bits are *reserved* then you should check that they're indeed
zero - now they're ignored, which is generally bad. They might not
always be.

johannes