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

[Johannes Berg <johannes@xxxxxxxxxxxxxxxx>]

> + * @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