Re: [PATCH v3 1/2] cfg80211: Add support to set tx power for a station associated

[Johannes Berg]

> + * @txpwr: tx power (in mBm) to be used for sending data traffic. If
> tx power
> + *   is not provided, the default per-interface tx power
> setting will be
> + *   overriding. Driver should be picking up the lowest tx
> power, either tx
> + *   power per-interface or per-station.
>   */
>  struct station_parameters {
>       const u8 *supported_rates;
> @@ -876,6 +881,7 @@ struct station_parameters {
>       u8 opmode_notif;
>       bool opmode_notif_used;
>       int support_p2p_ps;
> +     u16 txpwr;
>  };

Wouldn't it be better to add the tx power type (limited/automatic) here
as well? That way, we don't have to play games with 0 meaning automatic
(which you didn't even document, but seems to be the case), when in
fact 0dBm could technically be a valid TX power as well.

> +     idx = NL80211_ATTR_STA_TX_POWER_SETTING;
> +     type = nla_get_u32(info->attrs[idx]);
> +
> +     if (!info->attrs[NL80211_ATTR_STA_TX_POWER] &&
> +         (type != NL80211_TX_POWER_AUTOMATIC))
> +             return -EINVAL;
> +
> +     if (type != NL80211_TX_POWER_AUTOMATIC) {
> +             if (type == NL80211_TX_POWER_LIMITED) {
> +                     idx = NL80211_ATTR_STA_TX_POWER;
> +                     params->txpwr = nla_get_u32(info-
> >attrs[idx]);
> +             } else {
> +                     return -EINVAL;
> +             }
> +     } else {
> +             params->txpwr = 0;
> +     }

This could be nicer using a switch on the type, perhaps?

> +     if (info->attrs[NL80211_ATTR_STA_TX_POWER_SETTING]) {
> +             err = nl80211_parse_sta_txpower_setting(info,
&params);
> +             if (err)
> +                     return err;
> +     }

Why not move the check that the attribute exists into the function?

johannes