On Mon, 2022-07-04 at 15:53 +0530, Aditya Kumar Singh wrote:
>
> + * @NL80211_ATTR_6GHZ_REG_AP_POWER_MODE: Configure 6 GHz regulatory power mode
> + * for access points. Referenced from &enum ieee80211_ap_reg_power.
> + *
> + * @NL80211_ATTR_6GHZ_REG_CLIENT_POWER_MODE: Configure 6 GHz regulatory power
> + * mode for clients. Referenced from &enum ieee80211_client_reg_power.
I don't really see a good reason to have two attributes for this, rather
than validating their value based on the iftype?
> err = __cfg80211_stop_ap(rdev, dev, link_id, notify);
> +
> + if (wdev->reg_6ghz_pwr_configured)
> + wdev->reg_6ghz_pwr_configured = false;
no need to check first
> +static int nl80211_set_6ghz_power_mode(struct sk_buff *skb,
> + struct genl_info *info)
> +{
> + struct net_device *dev = info->user_ptr[1];
> + struct wireless_dev *wdev = NULL;
> + enum nl80211_iftype iftype = NL80211_IFTYPE_UNSPECIFIED;
> + int ret = -EINVAL;
> +
> + if (dev)
> + wdev = dev->ieee80211_ptr;
> +
> + if (wdev)
> + iftype = wdev->iftype;
that's all pretty useless
> + if (iftype != NL80211_IFTYPE_AP &&
> + iftype != NL80211_IFTYPE_STATION)
> + return -EOPNOTSUPP;
you're going to return here anyway ...
Better to just simplify that and return if there's no wdev, and actually
you can enforce that anyway through the internal flags?? Not sure why
you do all this.
Btw, all of that probably also needs to be per *link* rather than per
*interface* now, with MLO?
> + if (!info->attrs[NL80211_ATTR_6GHZ_REG_AP_POWER_MODE] &&
> + !info->attrs[NL80211_ATTR_6GHZ_REG_CLIENT_POWER_MODE])
> + return -EINVAL;
> +
> + wdev_lock(wdev);
> + if (wdev->reg_6ghz_pwr_configured) {
> + wdev_unlock(wdev);
> + return -EALREADY;
> + }
> +
> + if (iftype == NL80211_IFTYPE_AP &&
> + info->attrs[NL80211_ATTR_6GHZ_REG_AP_POWER_MODE]) {
> + wdev->ap_6ghz_power =
> + nla_get_u8(info->attrs[NL80211_ATTR_6GHZ_REG_AP_POWER_MODE]);
> + ret = 0;
> + }
> +
> + if (iftype == NL80211_IFTYPE_STATION &&
> + info->attrs[NL80211_ATTR_6GHZ_REG_CLIENT_POWER_MODE]) {
> + wdev->client_6ghz_power =
> + nla_get_u8(info->attrs[NL80211_ATTR_6GHZ_REG_CLIENT_POWER_MODE]);
> + ret = 0;
> + }
> +
> + if (!ret)
> + wdev->reg_6ghz_pwr_configured = true;
and honestly that logic here with the two attributes is pretty
strange...
I'd even say you should remove the union in the wdev struct since you
only can have one of them at a time anyway
> + {
> + .cmd = NL80211_CMD_SET_6GHZ_POWER_MODE,
> + .validate = GENL_DONT_VALIDATE_STRICT | GENL_DONT_VALIDATE_DUMP,
> + .doit = nl80211_set_6ghz_power_mode,
> + .flags = GENL_UNS_ADMIN_PERM,
> + .internal_flags = IFLAGS(NL80211_FLAG_NEED_NETDEV),
if you have netdev then you have wdev too, later
>
> + if (wdev->reg_6ghz_pwr_configured)
> + wdev->reg_6ghz_pwr_configured = false;
>
No need for if
johannes
