Re: [PATCH 1/7] d80211: add SIOCSIWTXPOW, SIOCGIWTXPOW, SIOCSIWPOWER and SIOCGIWPOWER

[mabbas <mabbas@linux.intel.com>]

Jiri Benc wrote:
> Hi,
>
> sorry for the long delay.
>
> On Mon, 28 Aug 2006 13:45:18 -0700, mabbas wrote:
>   
>> [...]
>> +static int ieee80211_ioctl_siwtxpow(struct net_device *dev,
>> +				    struct iw_request_info *info,
>> +				    union iwreq_data *wrqu, 
>> +				    char *extra)
>> +{
>> +	int rc = 0;
>> +	struct ieee80211_conf *conf = ieee80211_get_hw_conf(dev);
>> +
>> +	if (wrqu->txpower.flags != IW_TXPOW_DBM)
>> +		rc = -EINVAL;
>> +	else
>> +		conf->power_level = wrqu->txpower.value;
>> +
>> +
>> +	ieee80211_ioctl_set_radio_enabled(dev, !wrqu->txpower.disabled);
>>     
>
> Expecting implicit call of ieee80211_hw_config in
> ieee80211_ioctl_set_radio_enabled doesn't look like a good idea. If the
> latter function is changed (to call hw_config only if the radio_enabled
> field isn't the same as before), the SIOCSIWTXPOW ioctl won't have any
> effect.
>
> I'd prefer setting conf->radio_enabled directly and calling of
> ieee80211_hw_config explicitly.
>
> Also, always double check that you do error handling correctly (functions
> usually return something on purpose). (Hm, I already told you in this
> specific case. Please also double check that you haven't missed any comment
> before resending patches.)
>
>   
What about adding a new callback function fot txpower. 
ieee80211_hw_config callback still confusing if txpower was
changed for channel change or because the user setting txpower limit.
>> +	return rc;
>> +}
>> +
>> +static int ieee80211_ioctl_siwpower(struct net_device *dev,
>> +				    struct iw_request_info *info,
>> +				    union iwreq_data *wrqu, 
>> +				    char *extra)
>> +{
>> +	struct ieee80211_conf *conf = ieee80211_get_hw_conf(dev);
>> +
>> +	if (wrqu->power.disabled) {
>> +		conf->power_management_enable = 0;
>> +		if (ieee80211_hw_config(dev))
>> +			return -EINVAL;
>> +		return 0;
>>     
>
> This is wrong. Return the error code you've got from ieee80211_hw_config.
>   
Ok
>   
>> +	}
>> +
>> +	if (wrqu->power.flags & IW_POWER_TYPE)
>> +		return -EINVAL;
>> +
>> +	switch (wrqu->power.flags & IW_POWER_MODE) {
>> +	case IW_POWER_ON:       /* If not specified */
>> +	case IW_POWER_MODE:     /* If set all mask */
>> +	case IW_POWER_ALL_R:    /* If explicitely state all */
>> +		break;
>> +	default:                /* Otherwise we don't support it */
>> +		return -EINVAL;
>> +	}
>> +
>> +	conf->power_management_enable = 1;
>> +
>> +	if (ieee80211_hw_config(dev))
>> +		return -EINVAL;
>>     
>
> Ditto.
>
> Thanks,
>
>  Jiri
>
>