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

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

[mabbas]

[-- Attachment #1: Type: text/plain, Size: 1 bytes --]



[-- Attachment #2: d80211-wpower.patch --]
[-- Type: text/x-patch, Size: 4539 bytes --]


This patch modify d80211 to add SIOCSIWTXPOW, SIOCGIWTXPOW, SIOCSIWPOWER
and SIOCGIWPOWER. This is not a complete soluation but add the hook for
them.

Signed-off-by: Mohamed Abbas <mabbas@linux.intel.com>

diff --git a/include/net/d80211.h b/include/net/d80211.h
index 42fdbf7..bc5eb87 100644
--- a/include/net/d80211.h
+++ b/include/net/d80211.h
@@ -259,6 +259,7 @@ struct ieee80211_conf {
         u8 antenna_max;			/* maximum antenna gain */
 	short tx_power_reduction; /* in 0.1 dBm */
 
+	u8 power_management_enable;     /* flag to enable/disable power management*/
 	int antenna_sel;		/* default antenna conf:
 					 *	0 = default/diversity,
 			  		 *	1 = Ant0,
diff --git a/net/d80211/ieee80211.c b/net/d80211/ieee80211.c
index e72721f..05419d5 100644
--- a/net/d80211/ieee80211.c
+++ b/net/d80211/ieee80211.c
@@ -4357,6 +4357,7 @@ struct net_device *ieee80211_alloc_hw(si
 	local->long_retry_limit = 4;
 	local->conf.calib_int = 60;
 	local->conf.radio_enabled = 1;
+	local->conf.power_management_enable = 0;
 	local->rate_ctrl_num_up = RATE_CONTROL_NUM_UP;
 	local->rate_ctrl_num_down = RATE_CONTROL_NUM_DOWN;
 
diff --git a/net/d80211/ieee80211_ioctl.c b/net/d80211/ieee80211_ioctl.c
index 89a58e3..d05e159 100644
--- a/net/d80211/ieee80211_ioctl.c
+++ b/net/d80211/ieee80211_ioctl.c
@@ -2086,6 +2086,87 @@ static int ieee80211_ioctl_giwfrag(struc
 	return 0;
 }
 
+static int ieee80211_ioctl_giwtxpow(struct net_device *dev,
+				    struct iw_request_info *info,
+				    union iwreq_data *wrqu, 
+				    char *extra)
+{
+	struct ieee80211_conf *conf = ieee80211_get_hw_conf(dev);
+
+	wrqu->txpower.flags = IW_TXPOW_DBM;
+	wrqu->txpower.fixed = 1;
+	wrqu->txpower.disabled = (conf->radio_enabled) ? 0 : 1;
+	wrqu->txpower.value = conf->power_level;
+	return 0;
+}
+
+
+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);
+	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;
+	}
+
+	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;
+
+	return 0;
+}
+
+static int ieee80211_ioctl_giwpower(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 (!conf->power_management_enable)
+		wrqu->power.disabled = 1;
+	else
+		wrqu->power.disabled = 0;
+	return 0;
+}
+
 
 static int ieee80211_ioctl_siwretry(struct net_device *dev,
 				    struct iw_request_info *info,
@@ -3148,14 +3229,14 @@ static const iw_handler ieee80211_handle
 	(iw_handler) ieee80211_ioctl_giwrts,		/* SIOCGIWRTS */
 	(iw_handler) ieee80211_ioctl_siwfrag,		/* SIOCSIWFRAG */
 	(iw_handler) ieee80211_ioctl_giwfrag,		/* SIOCGIWFRAG */
-	(iw_handler) NULL,				/* SIOCSIWTXPOW */
-	(iw_handler) NULL,				/* SIOCGIWTXPOW */
+	(iw_handler) ieee80211_ioctl_siwtxpow,		/* SIOCSIWTXPOW */
+	(iw_handler) ieee80211_ioctl_giwtxpow,		/* SIOCGIWTXPOW */
 	(iw_handler) ieee80211_ioctl_siwretry,		/* SIOCSIWRETRY */
 	(iw_handler) ieee80211_ioctl_giwretry,		/* SIOCGIWRETRY */
 	(iw_handler) ieee80211_ioctl_siwencode,		/* SIOCSIWENCODE */
 	(iw_handler) ieee80211_ioctl_giwencode,		/* SIOCGIWENCODE */
-	(iw_handler) NULL,				/* SIOCSIWPOWER */
-	(iw_handler) NULL,				/* SIOCGIWPOWER */
+	(iw_handler) ieee80211_ioctl_siwpower,		/* SIOCSIWPOWER */
+	(iw_handler) ieee80211_ioctl_giwpower,		/* SIOCGIWPOWER */
 	(iw_handler) NULL,				/* -- hole -- */
 	(iw_handler) NULL,				/* -- hole -- */
 	(iw_handler) ieee80211_ioctl_siwgenie,		/* SIOCSIWGENIE */

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

[Jiri Benc]

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.)

> +	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.

> +	}
> +
> +	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

-- 
Jiri Benc
SUSE Labs

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

[mabbas]

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
>
>   

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

[Jiri Benc]

Thu, 21 Sep 2006 09:43:31 -0700, mabbas pise:
> 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.

Why should the driver care?

 Jiri

-- 
Jiri Benc
SUSE Labs

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

[mabbas]

Jiri Benc wrote:
> Thu, 21 Sep 2006 09:43:31 -0700, mabbas pise:
>   
>> 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.
>>     
>
> Why should the driver care?
>
>  Jiri
>
>   
if the user set txpoer using iwconfig command, the driver will clap all 
txpower to this
value for all available channels if channels original txpower is higher 
than txpower value.
Mohamed

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

[Jiri Benc]

On Thu, 21 Sep 2006 12:57:01 -0700, mabbas wrote:
> if the user set txpoer using iwconfig command, the driver will clap all 
> txpower to this
> value for all available channels if channels original txpower is higher 
> than txpower value.

Looks like a job for the stack.

If the stack takes care about that and always pass the correct value to a
driver, is there still a reason for the driver to know where the request
originated from?

 Jiri

-- 
Jiri Benc
SUSE Labs

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

[mabbas]

Jiri Benc wrote:
> On Thu, 21 Sep 2006 12:57:01 -0700, mabbas wrote:
>   
>> if the user set txpoer using iwconfig command, the driver will clap all 
>> txpower to this
>> value for all available channels if channels original txpower is higher 
>> than txpower value.
>>     
>
> Looks like a job for the stack.
>
> If the stack takes care about that and always pass the correct value to a
> driver, is there still a reason for the driver to know where the request
> originated from?
>
>  Jiri
>
>   
if the stack handle it then no need to know. When I call 
ieee80211_register_hw with driver's channel, rate and txpower info. the 
txpower values are overwritten by ieee80211_init_client. I get all the 
txpower info from EEPROMin the NIC and they are different that what 
d80211 default to. Is there is away to keep the value the driver passed 
in ieee80211_register_hw?
Mohamed

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

[Jiri Benc]

Thu, 21 Sep 2006 15:39:40 -0700, mabbas pise:
> if the stack handle it then no need to know. When I call 
> ieee80211_register_hw with driver's channel, rate and txpower info. the 
> txpower values are overwritten by ieee80211_init_client. I get all the 
> txpower info from EEPROMin the NIC and they are different that what 
> d80211 default to. Is there is away to keep the value the driver passed 
> in ieee80211_register_hw?

Well, there is still a work to do here. See Larry Finger's proposal,
http://www.spinics.net/lists/netdev/msg05623.html - it's not ideal, there
are some problems with its design, but it's probably the way to go.

Thanks,

 Jiri

-- 
Jiri Benc
SUSE Labs

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

[Larry Finger]

Jiri Benc wrote:
> Thu, 21 Sep 2006 15:39:40 -0700, mabbas pise:
>> if the stack handle it then no need to know. When I call 
>> ieee80211_register_hw with driver's channel, rate and txpower info. the 
>> txpower values are overwritten by ieee80211_init_client. I get all the 
>> txpower info from EEPROMin the NIC and they are different that what 
>> d80211 default to. Is there is away to keep the value the driver passed 
>> in ieee80211_register_hw?
> 
> Well, there is still a work to do here. See Larry Finger's proposal,
> http://www.spinics.net/lists/netdev/msg05623.html - it's not ideal, there
> are some problems with its design, but it's probably the way to go.

That project is on hold now, but I'm willing to share the information I have and to participate in 
further developments.

Larry

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

[Johannes Berg]

On Fri, 2006-09-22 at 08:24 -0500, Larry Finger wrote:

> That project is on hold now, but I'm willing to share the information I have and to participate in 
> further developments.

The database seems useful, maybe nl80211 could subsume the userspace
interface?

johannes

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

[Larry Finger]

Johannes Berg wrote:
> On Fri, 2006-09-22 at 08:24 -0500, Larry Finger wrote:
> 
>> That project is on hold now, but I'm willing to share the information I have and to participate in 
>> further developments.
> 
> The database seems useful, maybe nl80211 could subsume the userspace
> interface?

That sounds perfect as nl80211 seems to be the coming userspace client. Let me know how to help.

Larry

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

[Johannes Berg]

On Fri, 2006-09-22 at 08:54 -0500, Larry Finger wrote:

> That sounds perfect as nl80211 seems to be the coming userspace client. Let me know how to help.

I'll have to read up on what it actually needs to provide first :)

johannes