On 9/6/2022 16:35, Johannes Berg wrote:
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?
The policy for each varies. For AP power mode, it can vary from 0 to 2
(total 3 power modes currently), and for clients 0 to 1 (total 2 power
modes). So, if we have just 1 NL_ATTR, while parsing obviously we can do
based on iftype but in NL_ATTR policy validation, for clients it will
pass value 2 where actually it should not. Will that be fine?
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
Sure, will improvise in next version.
...
Btw, all of that probably also needs to be per *link* rather than per
*interface* now, with MLO?
Yes, it should be per *link*. Im working on the changes, will send next
version soon.
+ 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
Sure will do.
+ {
+ .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
Yes, got it.
+ if (wdev->reg_6ghz_pwr_configured)
+ wdev->reg_6ghz_pwr_configured = false;
No need for if
Got it, thanks. Will rectify.
johannes
Aditya
