mac80211: Export short-preamble bit to drivers

PATCH In review Michael Buesch 4 participants 15 messages Jun 25, 2007 – Jun 30, 2007 mac80211

Share & tools

Michael Buesch

author

2007-06-25 14:29 UTC #0 lore raw

PATCH 1/1 mac80211: Export short-preamble bit to drivers

Export the short-preamble-enabled-bit so drivers can use it. Drivers like bcm43xx make decisions in the firmware on the short-preamble state, so the driver needs to tell firmware about it.
There's no functional change to mac80211 in this patch. A patch for bcm43xx will follow once this got merged.

Signed-off-by Michael Buesch <[email protected]>

include/net/mac80211.h +2 -0

Index: mac80211/include/net/mac80211.h
===================================================================
@@ -283,6 +283,8 @@ struct ieee80211_conf {
 #define IEEE80211_CONF_SSID_HIDDEN	(1<<1) /* do not broadcast the ssid */
 #define IEEE80211_CONF_RADIOTAP		(1<<2) /* use radiotap if supported
 						  check this bit at RX time */
+#define IEEE80211_CONF_SHORT_PREAMBLE	(1<<3) /* use short preamble
+						* with IEEE 802.11b */
 	u32 flags;			/* configuration flags defined above */
 
 	u8 power_level;			/* transmit power limit for current

net/mac80211/ieee80211_i.h +0 -1

Index: mac80211/net/mac80211/ieee80211_i.h
===================================================================
@@ -449,7 +449,6 @@ struct ieee80211_local {
 	int fragmentation_threshold;
 	int short_retry_limit; /* dot11ShortRetryLimit */
 	int long_retry_limit; /* dot11LongRetryLimit */
-	int short_preamble; /* use short preamble with IEEE 802.11b */
 
 	struct crypto_blkcipher *wep_tx_tfm;
 	struct crypto_blkcipher *wep_rx_tfm;

net/mac80211/ieee80211.c +17 ~~-10~~

Index: mac80211/net/mac80211/ieee80211.c
===================================================================
@@ -409,8 +409,10 @@ static int ieee80211_is_eapol(const stru
 static ieee80211_txrx_result
 ieee80211_tx_h_rate_ctrl(struct ieee80211_txrx_data *tx)
 {
+	struct ieee80211_hw *hw;
 	struct rate_control_extra extra;
 
+	hw = local_to_hw(tx->local);
 	memset(&extra, 0, sizeof(extra));
 	extra.mode = tx->u.tx.mode;
 	extra.mgmt_data = tx->sdata &&

@@ -444,7 +446,7 @@ ieee80211_tx_h_rate_ctrl(struct ieee8021
 	}
 	tx->u.tx.control->tx_rate = tx->u.tx.rate->val;
 	if ((tx->u.tx.rate->flags & IEEE80211_RATE_PREAMBLE2) &&
-	    tx->local->short_preamble &&
+	    (hw->conf.flags & IEEE80211_CONF_SHORT_PREAMBLE) &&
 	    (!tx->sta || (tx->sta->flags & WLAN_STA_SHORT_PREAMBLE))) {
 		tx->u.tx.short_preamble = 1;
 		tx->u.tx.control->tx_rate = tx->u.tx.rate->val2;

@@ -707,8 +709,8 @@ __le16 ieee80211_generic_frame_duration(
 	int erp;
 
 	erp = ieee80211_is_erp_rate(hw->conf.phymode, rate);
-	dur = ieee80211_frame_duration(local, frame_len, rate,
-				       erp, local->short_preamble);
+	dur = ieee80211_frame_duration(local, frame_len, rate, erp,
+				       (hw->conf.flags & IEEE80211_CONF_SHORT_PREAMBLE));
 
 	return cpu_to_le16(dur);
 }

@@ -721,9 +723,12 @@ static u16 ieee80211_duration(struct iee
 	int rate, mrate, erp, dur, i;
 	struct ieee80211_rate *txrate = tx->u.tx.rate;
 	struct ieee80211_local *local = tx->local;
+	struct ieee80211_hw *hw = local_to_hw(local);
 	struct ieee80211_hw_mode *mode = tx->u.tx.mode;
+	int short_preamble;
 
 	erp = txrate->flags & IEEE80211_RATE_ERP;
+	short_preamble = (hw->conf.flags & IEEE80211_CONF_SHORT_PREAMBLE);
 
 	/*
 	 * data and mgmt (except PS Poll):

@@ -805,7 +810,7 @@ static u16 ieee80211_duration(struct iee
 	 * to closest integer */
 
 	dur = ieee80211_frame_duration(local, 10, rate, erp,
-				       local->short_preamble);
+				       short_preamble);
 
 	if (next_frag_len) {
 		/* Frame is fragmented: duration increases with time needed to

@@ -814,7 +819,7 @@ static u16 ieee80211_duration(struct iee
 		/* next fragment */
 		dur += ieee80211_frame_duration(local, next_frag_len,
 						txrate->rate, erp,
-						local->short_preamble);
+						short_preamble);
 	}
 
 	return dur;

@@ -1877,9 +1882,11 @@ struct sk_buff * ieee80211_beacon_get(st
 			return NULL;
 		}
 
-		control->tx_rate = (local->short_preamble &&
-				    (rate->flags & IEEE80211_RATE_PREAMBLE2)) ?
-			rate->val2 : rate->val;
+		if ((hw->conf.flags & IEEE80211_CONF_SHORT_PREAMBLE) &&
+		    (rate->flags & IEEE80211_RATE_PREAMBLE2))
+			control->tx_rate = rate->val2;
+		else
+			control->tx_rate = rate->val;
 		control->antenna_sel_tx = local->hw.conf.antenna_sel_tx;
 		control->power_level = local->hw.conf.power_level;
 		control->flags |= IEEE80211_TXCTL_NO_ACK;

@@ -1898,7 +1905,7 @@ __le16 ieee80211_rts_duration(struct iee
 {
 	struct ieee80211_local *local = hw_to_local(hw);
 	struct ieee80211_rate *rate;
-	int short_preamble = local->short_preamble;
+	int short_preamble = (hw->conf.flags & IEEE80211_CONF_SHORT_PREAMBLE);
 	int erp;
 	u16 dur;

@@ -1926,7 +1933,7 @@ __le16 ieee80211_ctstoself_duration(stru
 {
 	struct ieee80211_local *local = hw_to_local(hw);
 	struct ieee80211_rate *rate;
-	int short_preamble = local->short_preamble;
+	int short_preamble = (hw->conf.flags & IEEE80211_CONF_SHORT_PREAMBLE);
 	int erp;
 	u16 dur;

net/mac80211/ieee80211_ioctl.c +7 -2

Index: mac80211/net/mac80211/ieee80211_ioctl.c
===================================================================
@@ -2434,7 +2434,12 @@ static int ieee80211_ioctl_prism2_param(
 		break;
 
 	case PRISM2_PARAM_PREAMBLE:
-		local->short_preamble = value;
+		if (value)
+			local_to_hw(local)->conf.flags |= IEEE80211_CONF_SHORT_PREAMBLE;
+		else
+			local_to_hw(local)->conf.flags &= ~IEEE80211_CONF_SHORT_PREAMBLE;
+		if (ieee80211_hw_config(local))
+			ret = -EINVAL;
 		break;
 
 	case PRISM2_PARAM_STAT_TIME:

@@ -2664,7 +2669,7 @@ static int ieee80211_ioctl_get_prism2_pa
 		break;
 
 	case PRISM2_PARAM_PREAMBLE:
-		*param = local->short_preamble;
+		*param = !!(local_to_hw(local)->conf.flags & IEEE80211_CONF_SHORT_PREAMBLE);
 		break;
 
 	case PRISM2_PARAM_STAT_TIME:

net/mac80211/ieee80211_sta.c +6 -3

Index: mac80211/net/mac80211/ieee80211_sta.c
===================================================================
@@ -2298,6 +2298,7 @@ static int ieee80211_sta_join_ibss(struc
 				   struct ieee80211_sta_bss *bss)
 {
 	struct ieee80211_local *local = wdev_priv(dev->ieee80211_ptr);
+	struct ieee80211_hw *hw = local_to_hw(local);
 	int res, rates, i, j;
 	struct sk_buff *skb;
 	struct ieee80211_mgmt *mgmt;

@@ -2397,9 +2398,11 @@ static int ieee80211_sta_join_ibss(struc
 			       "for IBSS beacon\n", dev->name);
 			break;
 		}
-		control.tx_rate = (local->short_preamble &&
-				   (rate->flags & IEEE80211_RATE_PREAMBLE2)) ?
-			rate->val2 : rate->val;
+		if ((hw->conf.flags & IEEE80211_CONF_SHORT_PREAMBLE) &&
+		    (rate->flags & IEEE80211_RATE_PREAMBLE2))
+			control.tx_rate = rate->val2;
+		else
+			control.tx_rate = rate->val;
 		control.antenna_sel_tx = local->hw.conf.antenna_sel_tx;
 		control.power_level = local->hw.conf.power_level;
 		control.flags |= IEEE80211_TXCTL_NO_ACK;

Signature · 1 line

Greetings Michael.

Johannes Berg

maintainer

2007-06-27 09:39 UTC #1 lore raw

net/mac80211/ieee80211_ioctl.c static int ieee80211_ioctl_prism2_param( · line 2442

Quoted 8 lines · Michael Buesch

net/mac80211/ieee80211_ioctl.c +6 -1

 	case PRISM2_PARAM_PREAMBLE:
-		local->short_preamble = value;
+		if (value)
+			local_to_hw(local)->conf.flags |= IEEE80211_CONF_SHORT_PREAMBLE;
+		else
+			local_to_hw(local)->conf.flags &= ~IEEE80211_CONF_SHORT_PREAMBLE;
+		if (ieee80211_hw_config(local))
+			ret = -EINVAL;

any reason not to return the actual error code from hw_config here? Not that it'll be any different from -EINVAL I expect...

johannes

Michael Buesch

author

2007-06-27 13:09 UTC #2 lore raw

Quoted 13 lines · On Wednesday 27 June 2007 10:55:00 Johannes Berg wrote:

net/mac80211/ieee80211_ioctl.c static int ieee80211_ioctl_prism2_param( · line 2442
Quoted 8 lines · Michael Buesch
net/mac80211/ieee80211_ioctl.c +6 -1
 	case PRISM2_PARAM_PREAMBLE:
-		local->short_preamble = value;
+		if (value)
+			local_to_hw(local)->conf.flags |= IEEE80211_CONF_SHORT_PREAMBLE;
+		else
+			local_to_hw(local)->conf.flags &= ~IEEE80211_CONF_SHORT_PREAMBLE;
+		if (ieee80211_hw_config(local))
+			ret = -EINVAL;
any reason not to return the actual error code from hw_config here? Not that it'll be any different from -EINVAL I expect...

The reason is copy&paste from other PRISM2 IOCTLs ;)
I'm OK with the real error code, too.
Though, the error code of hw_config is usually not useful _at all_. As it might not be the SHORT_PRMBL flag that produced it, but some of the other config settings.
I would probably vote for any code in mac80211 to ignore that return value, except the places where a failure of hw_config would be fatal (init or places like that).

Signature · 1 line

Greetings Michael.

Johannes Berg

maintainer

2007-06-27 14:30 UTC #3 lore raw

Quoted 15 lines · Michael Buesch

Quoted 13 lines · On Wednesday 27 June 2007 10:55:00 Johannes Berg wrote:
net/mac80211/ieee80211_ioctl.c static int ieee80211_ioctl_prism2_param( · line 2442
Quoted 8 lines · Michael Buesch
net/mac80211/ieee80211_ioctl.c +6 -1
 	case PRISM2_PARAM_PREAMBLE:
-		local->short_preamble = value;
+		if (value)
+			local_to_hw(local)->conf.flags |= IEEE80211_CONF_SHORT_PREAMBLE;
+		else
+			local_to_hw(local)->conf.flags &= ~IEEE80211_CONF_SHORT_PREAMBLE;
+		if (ieee80211_hw_config(local))
+			ret = -EINVAL;
any reason not to return the actual error code from hw_config here? Not that it'll be any different from -EINVAL I expect...

Quoted 2 lines

The reason is copy&paste from other PRISM2 IOCTLs ;) I'm OK with the real error code, too.

I don't really care either way.

Quoted 6 lines

Though, the error code of hw_config is usually not useful _at all_. As it might not be the SHORT_PRMBL flag that produced it, but some of the other config settings.
I would probably vote for any code in mac80211 to ignore that return value, except the places where a failure of hw_config would be fatal (init or places like that).

Yeah, there's a todo item saying that we need a way to just commit the changed settings.

johannes

Michael Buesch

author

2007-06-27 14:38 UTC #4 lore raw

Quoted 28 lines · On Wednesday 27 June 2007 16:30:27 Johannes Berg wrote:

Quoted 15 lines · Michael Buesch
Quoted 13 lines · On Wednesday 27 June 2007 10:55:00 Johannes Berg wrote:
net/mac80211/ieee80211_ioctl.c static int ieee80211_ioctl_prism2_param( · line 2442
Quoted 8 lines · Michael Buesch
net/mac80211/ieee80211_ioctl.c +6 -1
 	case PRISM2_PARAM_PREAMBLE:
-		local->short_preamble = value;
+		if (value)
+			local_to_hw(local)->conf.flags |= IEEE80211_CONF_SHORT_PREAMBLE;
+		else
+			local_to_hw(local)->conf.flags &= ~IEEE80211_CONF_SHORT_PREAMBLE;
+		if (ieee80211_hw_config(local))
+			ret = -EINVAL;
any reason not to return the actual error code from hw_config here? Not that it'll be any different from -EINVAL I expect...
Quoted 2 lines
The reason is copy&paste from other PRISM2 IOCTLs ;) I'm OK with the real error code, too.
I don't really care either way.
Quoted 6 lines
Though, the error code of hw_config is usually not useful _at all_. As it might not be the SHORT_PRMBL flag that produced it, but some of the other config settings.
I would probably vote for any code in mac80211 to ignore that return value, except the places where a failure of hw_config would be fatal (init or places like that).

Quoted 2 lines

Yeah, there's a todo item saying that we need a way to just commit the changed settings.

Well, sane drivers should basically already do that, as most information is saved redundantly in the driver's structs. Especially for operations that can fail.
So it's basically a TODO for drivers, rather than mac80211.

Signature · 1 line

Greetings Michael.

Michael Buesch

author

2007-06-27 13:35 UTC #5 lore raw

In reply to Johannes Berg

Quoted 14 lines · On Wednesday 27 June 2007 10:55:00 Johannes Berg wrote:

net/mac80211/ieee80211_ioctl.c static int ieee80211_ioctl_prism2_param( · line 2442
Quoted 8 lines · Michael Buesch
net/mac80211/ieee80211_ioctl.c +6 -1
 	case PRISM2_PARAM_PREAMBLE:
-		local->short_preamble = value;
+		if (value)
+			local_to_hw(local)->conf.flags |= IEEE80211_CONF_SHORT_PREAMBLE;
+		else
+			local_to_hw(local)->conf.flags &= ~IEEE80211_CONF_SHORT_PREAMBLE;
+		if (ieee80211_hw_config(local))
+			ret = -EINVAL;
any reason not to return the actual error code from hw_config here? Not that it'll be any different from -EINVAL I expect...

Quoted 2 lines

johannes

Another thing, while we are at it.
How is locking for the conf structure actually done? How is it made impossible to read the value of "flags" in another thread, while we update it in a non-atomic way, here?

Signature · 1 line

Greetings Michael.

Johannes Berg

maintainer

2007-06-27 13:58 UTC #6 lore raw

Quoted 15 lines · Michael Buesch

Quoted 14 lines · On Wednesday 27 June 2007 10:55:00 Johannes Berg wrote:
net/mac80211/ieee80211_ioctl.c static int ieee80211_ioctl_prism2_param( · line 2442
Quoted 8 lines · Michael Buesch
net/mac80211/ieee80211_ioctl.c +6 -1
 	case PRISM2_PARAM_PREAMBLE:
-		local->short_preamble = value;
+		if (value)
+			local_to_hw(local)->conf.flags |= IEEE80211_CONF_SHORT_PREAMBLE;
+		else
+			local_to_hw(local)->conf.flags &= ~IEEE80211_CONF_SHORT_PREAMBLE;
+		if (ieee80211_hw_config(local))
+			ret = -EINVAL;
any reason not to return the actual error code from hw_config here? Not that it'll be any different from -EINVAL I expect...

Quoted 7 lines

Quoted 2 lines
johannes
Another thing, while we are at it.
How is locking for the conf structure actually done? How is it made impossible to read the value of "flags" in another thread, while we update it in a non-atomic way, here?

I don't see any locking, though we do have this code running under rtnl so you might need to check the other accesses.

johannes

Michael Wu

2007-06-28 01:40 UTC #7 lore raw

In reply to Michael Buesch

include/net/mac80211.h struct ieee80211_conf { · line 287

Quoted 2 lines · Michael Buesch

include/net/mac80211.h +2 -0

+#define IEEE80211_CONF_SHORT_PREAMBLE	(1<<3) /* use short preamble
+						* with IEEE 802.11b */

Shouldn't this be more of a "short preamble allowed" bit rather than "use short preamble"? After all, short preamble is usually configured per frame.

-Michael Wu

Michael Buesch

author

2007-06-28 08:36 UTC #8 lore raw

Quoted 8 lines · On Thursday 28 June 2007 03:40:05 Michael Wu wrote:

include/net/mac80211.h struct ieee80211_conf { · line 287
Quoted 2 lines · Michael Buesch
include/net/mac80211.h +2 -0
+#define IEEE80211_CONF_SHORT_PREAMBLE	(1<<3) /* use short preamble
+						* with IEEE 802.11b */
Shouldn't this be more of a "short preamble allowed" bit rather than "use short preamble"? After all, short preamble is usually configured per frame.
-Michael Wu

Well, I copied the comment from existing mac80211 code. Sure, we can change it.

Signature · 1 line

Greetings Michael.

Johannes Berg

maintainer

2007-06-28 09:56 UTC #9 lore raw

In reply to Michael Wu

Quoted 5 lines · Michael Wu

include/net/mac80211.h struct ieee80211_conf { · line 287
Quoted 2 lines · Michael Buesch
include/net/mac80211.h +2 -0
+#define IEEE80211_CONF_SHORT_PREAMBLE	(1<<3) /* use short preamble
+						* with IEEE 802.11b */
Shouldn't this be more of a "short preamble allowed" bit rather than "use short preamble"? After all, short preamble is usually configured per frame.

I don't think we ever care about configuring it per-frame and currently can't afaik, so I don't think we can rightfully say "short preamble allowed" unless we want to confuse driver authors about who is responsible for then determining whether to use it or not for each frame.

johannes

Michael Wu

2007-06-29 06:00 UTC #10 lore raw

Quoted 6 lines · Johannes Berg

I don't think we ever care about configuring it per-frame and currently can't afaik, so I don't think we can rightfully say "short preamble allowed" unless we want to confuse driver authors about who is responsible for then determining whether to use it or not for each frame.

mac80211 does this per-frame. It does matter. Short preamble should not be configured by a global bit. What does the bcm43xx firmware do with this bit?

-Michael Wu

Johannes Berg

maintainer

2007-06-29 06:07 UTC #11 lore raw

Quoted 2 lines · Michael Wu

mac80211 does this per-frame. It does matter. Short preamble should not be configured by a global bit. What does the bcm43xx firmware do with this bit?

Hah ok. I'd have to look at the corresponding bcm43xx patch but that does indeed not make too much sense then.

johannes

Michael Buesch

author

2007-06-29 08:13 UTC #12 lore raw

In reply to Michael Wu

Quoted 9 lines · On Friday 29 June 2007 08:01:11 Michael Wu wrote:

Quoted 6 lines · Johannes Berg
I don't think we ever care about configuring it per-frame and currently can't afaik, so I don't think we can rightfully say "short preamble allowed" unless we want to confuse driver authors about who is responsible for then determining whether to use it or not for each frame.
mac80211 does this per-frame. It does matter. Short preamble should not be configured by a global bit. What does the bcm43xx firmware do with this bit?

Configure it for management/ctl frames that are sent in hw. We send them with long preamble atm.

Signature · 1 line

Greetings Michael.

Daniel Drake

2007-06-29 22:41 UTC #13 lore raw

Quoted 2 lines · Michael Buesch

Configure it for management/ctl frames that are sent in hw. We send them with long preamble atm.

I am working on patches which will help you here. Currently mac80211's handling of short preambles is correct for hostap/master mode but not for a STA.

The interface I'm working on adding will be somewhat similar to the softmac interface. That is, preambles are available on a per-frame basis and the driver also gets a notification when the availability of short preambles changes.

zd1211rw seems to have similar requirements for bcm43xx. The correct behaviour (IMO) is to send frames with long preambles until mac80211 uses the above notification to tell you that short preambles are now allowable. This is how zd1211rw-softmac does it.

I have written the code for the above but have not had time to test it yet. Hopefully within the next few days...

Daniel

Michael Buesch

author

2007-06-30 12:48 UTC #14 lore raw

Quoted 13 lines · On Saturday 30 June 2007 00:39:45 Daniel Drake wrote:

Quoted 2 lines · Michael Buesch
Configure it for management/ctl frames that are sent in hw. We send them with long preamble atm.
I am working on patches which will help you here. Currently mac80211's handling of short preambles is correct for hostap/master mode but not for a STA.
The interface I'm working on adding will be somewhat similar to the softmac interface. That is, preambles are available on a per-frame basis and the driver also gets a notification when the availability of short preambles changes.

Quoted 7 lines

zd1211rw seems to have similar requirements for bcm43xx. The correct behaviour (IMO) is to send frames with long preambles until mac80211 uses the above notification to tell you that short preambles are now allowable. This is how zd1211rw-softmac does it.
I have written the code for the above but have not had time to test it yet. Hopefully within the next few days...

Great!
So Jiri please drop my patch infavour of Daniel's.

Signature · 1 line

Greetings Michael.