Skip to contentSkip to conversation

d80211: add SIOCSIWTXPOW, SIOCGIWTXPOW, SIOCSIWPOWER and SIOCGIWPOWER

PATCH Applied mabbas 4 participants 12 messages Aug 28, 2006 – Sep 25, 2006

Subsystems (2)
Networking Network drivers
lore
Share & tools
lore

mabbas

author
#0 lore raw

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

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 <[email protected]>
include/net/d80211.h +1 -0
index 42fdbf7..bc5eb87 100644
@@ -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,
net/d80211/ieee80211.c +1 -0
index e72721f..05419d5 100644
@@ -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;
net/d80211/ieee80211_ioctl.c +85 -4

105 lines not on this page · raw message

#1 lore raw

Hi,

sorry for the long delay.

Quoted 6 lines · mabbas

[...]

net/d80211/ieee80211_ioctl.c +5 -0
+static int ieee80211_ioctl_siwtxpow(struct net_device *dev,
+				    struct iw_request_info *info,
+				    union iwreq_data *wrqu, 
+				    char *extra)
+{

net/d80211/ieee80211_ioctl.c static int ieee80211_ioctl_giwfrag(struc · line 2118

Quoted 10 lines
net/d80211/ieee80211_ioctl.c +10 -0
+	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.)

Quoted 5 lines
net/d80211/ieee80211_ioctl.c +5 -0
+	return rc;
+}
+
+static int ieee80211_ioctl_siwpower(struct net_device *dev,
+				    struct iw_request_info *info,

net/d80211/ieee80211_ioctl.c static int ieee80211_ioctl_giwfrag(struc · line 2133

Quoted 10 lines
net/d80211/ieee80211_ioctl.c +10 -0
+				    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.

Quoted 8 lines
net/d80211/ieee80211_ioctl.c +8 -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 */

net/d80211/ieee80211_ioctl.c static int ieee80211_ioctl_giwfrag(struc · line 2151

Quoted 10 lines
net/d80211/ieee80211_ioctl.c +10 -0
+	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

Signature · 2 lines
Jiri Benc
SUSE Labs
author
#2 lore raw
Quoted 30 lines · Jiri Benc

Hi,

sorry for the long delay.

Quoted 7 lines · mabbas

[...]

net/d80211/ieee80211_ioctl.c +5 -0
+static int ieee80211_ioctl_siwtxpow(struct net_device *dev,
+				    struct iw_request_info *info,
+				    union iwreq_data *wrqu, 
+				    char *extra)
+{

net/d80211/ieee80211_ioctl.c static int ieee80211_ioctl_giwfrag(struc · line 2118

Quoted 10 lines
net/d80211/ieee80211_ioctl.c +10 -0
+	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.

Quoted 9 lines

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.

Quoted 10 lines
Quoted 10 lines
net/d80211/ieee80211_ioctl.c +10 -0
+	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);
+
Quoted 9 lines

net/d80211/ieee80211_ioctl.c static int ieee80211_ioctl_giwfrag(struc · line 2133

Quoted 6 lines
net/d80211/ieee80211_ioctl.c +5 -0
+	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

Quoted 28 lines
Quoted 10 lines
net/d80211/ieee80211_ioctl.c +8 -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 */

net/d80211/ieee80211_ioctl.c static int ieee80211_ioctl_giwfrag(struc · line 2151

Quoted 10 lines
net/d80211/ieee80211_ioctl.c +10 -0
+	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

#3 lore raw

Thu, 21 Sep 2006 09:43:31 -0700, mabbas pise:

Quoted 3 lines

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

Signature · 2 lines
Jiri Benc
SUSE Labs
author
#4 lore raw
Quoted 11 lines · Jiri Benc

Thu, 21 Sep 2006 09:43:31 -0700, mabbas pise:

Quoted 3 lines

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

#5 lore raw
Quoted 4 lines · mabbas

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

Signature · 2 lines
Jiri Benc
SUSE Labs
author
#6 lore raw
Quoted 6 lines · Jiri Benc
Quoted 3 lines · mabbas

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.

Quoted 9 lines

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

#7 lore raw

Thu, 21 Sep 2006 15:39:40 -0700, mabbas pise:

Quoted 6 lines

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

Signature · 2 lines
Jiri Benc
SUSE Labs
#8 lore raw
Quoted 6 lines · Jiri Benc

Thu, 21 Sep 2006 15:39:40 -0700, mabbas pise:

Quoted 1 line

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

#9 lore raw
Quoted 2 lines · Larry Finger

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

#10 lore raw
Quoted 6 lines · Johannes Berg
Quoted 1 line · Larry Finger

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

#11 lore raw
Quoted 1 line · Larry Finger

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