[PATCH] mac80211: Export short-preamble bit to drivers

[PATCH] mac80211: Export short-preamble bit to drivers

[Michael Buesch]

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 <mb@bu3sch.de>

Index: mac80211/include/net/mac80211.h
===================================================================
--- mac80211.orig/include/net/mac80211.h	2007-06-25 16:18:58.000000000 +0200
+++ mac80211/include/net/mac80211.h	2007-06-25 16:19:00.000000000 +0200
@@ -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
Index: mac80211/net/mac80211/ieee80211_i.h
===================================================================
--- mac80211.orig/net/mac80211/ieee80211_i.h	2007-06-25 16:18:58.000000000 +0200
+++ mac80211/net/mac80211/ieee80211_i.h	2007-06-25 16:19:00.000000000 +0200
@@ -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;
Index: mac80211/net/mac80211/ieee80211.c
===================================================================
--- mac80211.orig/net/mac80211/ieee80211.c	2007-06-25 16:18:58.000000000 +0200
+++ mac80211/net/mac80211/ieee80211.c	2007-06-25 16:19:00.000000000 +0200
@@ -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;
 
Index: mac80211/net/mac80211/ieee80211_ioctl.c
===================================================================
--- mac80211.orig/net/mac80211/ieee80211_ioctl.c	2007-06-25 16:18:58.000000000 +0200
+++ mac80211/net/mac80211/ieee80211_ioctl.c	2007-06-25 16:26:56.000000000 +0200
@@ -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:
Index: mac80211/net/mac80211/ieee80211_sta.c
===================================================================
--- mac80211.orig/net/mac80211/ieee80211_sta.c	2007-06-25 16:18:58.000000000 +0200
+++ mac80211/net/mac80211/ieee80211_sta.c	2007-06-25 16:19:00.000000000 +0200
@@ -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;


-- 
Greetings Michael.

Re: [PATCH] mac80211: Export short-preamble bit to drivers

[Johannes Berg]

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

On Mon, 2007-06-25 at 16:28 +0200, Michael Buesch wrote:

>  	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

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 190 bytes --]

Re: [PATCH] mac80211: Export short-preamble bit to drivers

[Michael Buesch]

On Wednesday 27 June 2007 10:55:00 Johannes Berg wrote:
> On Mon, 2007-06-25 at 16:28 +0200, Michael Buesch wrote:
> 
> >  	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).

-- 
Greetings Michael.

Re: [PATCH] mac80211: Export short-preamble bit to drivers

[Michael Buesch]

On Wednesday 27 June 2007 10:55:00 Johannes Berg wrote:
> On Mon, 2007-06-25 at 16:28 +0200, Michael Buesch wrote:
> 
> >  	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
> 

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?

-- 
Greetings Michael.

Re: [PATCH] mac80211: Export short-preamble bit to drivers

[Johannes Berg]

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

On Wed, 2007-06-27 at 15:33 +0200, Michael Buesch wrote:
> On Wednesday 27 June 2007 10:55:00 Johannes Berg wrote:
> > On Mon, 2007-06-25 at 16:28 +0200, Michael Buesch wrote:
> > 
> > >  	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
> > 
> 
> 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

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 190 bytes --]

Re: [PATCH] mac80211: Export short-preamble bit to drivers

[Johannes Berg]

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

On Wed, 2007-06-27 at 15:08 +0200, Michael Buesch wrote:
> On Wednesday 27 June 2007 10:55:00 Johannes Berg wrote:
> > On Mon, 2007-06-25 at 16:28 +0200, Michael Buesch wrote:
> > 
> > >  	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.

I don't really care either way.

> 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

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 190 bytes --]

Re: [PATCH] mac80211: Export short-preamble bit to drivers

[Michael Buesch]

On Wednesday 27 June 2007 16:30:27 Johannes Berg wrote:
> On Wed, 2007-06-27 at 15:08 +0200, Michael Buesch wrote:
> > On Wednesday 27 June 2007 10:55:00 Johannes Berg wrote:
> > > On Mon, 2007-06-25 at 16:28 +0200, Michael Buesch wrote:
> > > 
> > > >  	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.
> 
> I don't really care either way.
> 
> > 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.

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.

-- 
Greetings Michael.

Re: [PATCH] mac80211: Export short-preamble bit to drivers

[Michael Wu]

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

On Monday 25 June 2007 07:28, Michael Buesch wrote:
> +#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

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH] mac80211: Export short-preamble bit to drivers

[Michael Buesch]

On Thursday 28 June 2007 03:40:05 Michael Wu wrote:
> On Monday 25 June 2007 07:28, Michael Buesch wrote:
> > +#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.

-- 
Greetings Michael.

Re: [PATCH] mac80211: Export short-preamble bit to drivers

[Johannes Berg]

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

On Wed, 2007-06-27 at 18:40 -0700, Michael Wu wrote:
> On Monday 25 June 2007 07:28, Michael Buesch wrote:
> > +#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

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 190 bytes --]

Re: [PATCH] mac80211: Export short-preamble bit to drivers

[Michael Wu]

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

On Thursday 28 June 2007 02:57, Johannes Berg wrote:
> 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

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH] mac80211: Export short-preamble bit to drivers

[Johannes Berg]

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

On Thu, 2007-06-28 at 23:01 -0700, Michael Wu wrote:

> 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

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 190 bytes --]

Re: [PATCH] mac80211: Export short-preamble bit to drivers

[Michael Buesch]

On Friday 29 June 2007 08:01:11 Michael Wu wrote:
> On Thursday 28 June 2007 02:57, Johannes Berg wrote:
> > 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.

-- 
Greetings Michael.

Re: [PATCH] mac80211: Export short-preamble bit to drivers

[Daniel Drake]

Michael Buesch wrote:
> 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

Re: [PATCH] mac80211: Export short-preamble bit to drivers

[Michael Buesch]

On Saturday 30 June 2007 00:39:45 Daniel Drake wrote:
> Michael Buesch wrote:
> > 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...

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

-- 
Greetings Michael.