[RFC V2] bcm43xx-mac80211: Provide information to allow transmission rate decreases

[RFC V2] bcm43xx-mac80211: Provide information to allow transmission rate decreases

[Larry Finger]

Michael,

I think I took care of your comments in the previous version.

Larry
---

In bcm43xx-mac80211, the mechanism for decreasing the transmit rate cannot
be triggered. This may be shown by walking away from the AP with a laptop.
At some distance, communications will be lost and never recovered because
the rate decreasing mechanism of rc80211_simple needs to see excessive_retries
set in the ieee80211_tx_status struct. With this patch, the transmit rate
will decrease until communications restart.

Signed-off-by: Larry Finger <Larry.Finger@lwfinger.net>
---

Index: wireless-dev/drivers/net/wireless/bcm43xx-mac80211/bcm43xx_dma.c
===================================================================
--- wireless-dev.orig/drivers/net/wireless/bcm43xx-mac80211/bcm43xx_dma.c
+++ wireless-dev/drivers/net/wireless/bcm43xx-mac80211/bcm43xx_dma.c
@@ -1311,6 +1311,16 @@ void bcm43xx_dma_handle_txstatus(struct 
 			 */
 			if (status->acked)
 				meta->txstat.flags |= IEEE80211_TX_STATUS_ACK;
+			else
+				if (dev->short_preamble) {
+					if (status->frame_count >=
+					    dev->short_retry_limit)
+						meta->txstat.excessive_retries = 1;
+				} else {
+					if (status->frame_count >=
+					    dev->long_retry_limit)
+						meta->txstat.excessive_retries = 1;
+				}
 			meta->txstat.retry_count = status->frame_count - 1;
 			ieee80211_tx_status_irqsafe(dev->wl->hw, meta->skb, &(meta->txstat));
 			/* skb is freed by ieee80211_tx_status_irqsafe() */
Index: wireless-dev/drivers/net/wireless/bcm43xx-mac80211/bcm43xx.h
===================================================================
--- wireless-dev.orig/drivers/net/wireless/bcm43xx-mac80211/bcm43xx.h
+++ wireless-dev/drivers/net/wireless/bcm43xx-mac80211/bcm43xx.h
@@ -707,6 +707,8 @@ struct bcm43xx_wldev {
 	bool short_preamble;		/* TRUE, if short preamble is enabled. */
 	bool short_slot;		/* TRUE, if short slot timing is enabled. */
 	bool radio_hw_enable;		/* saved state of radio hardware enabled state */
+	u8 short_retry_limit;
+	u8 long_retry_limit;
 
 	/* PHY/Radio device. */
 	struct bcm43xx_phy phy;
Index: wireless-dev/drivers/net/wireless/bcm43xx-mac80211/bcm43xx_main.c
===================================================================
--- wireless-dev.orig/drivers/net/wireless/bcm43xx-mac80211/bcm43xx_main.c
+++ wireless-dev/drivers/net/wireless/bcm43xx-mac80211/bcm43xx_main.c
@@ -3333,10 +3333,12 @@ static int bcm43xx_wireless_core_init(st
 	tmp = limit_value(modparam_short_retry, 0, 0xF);
 	bcm43xx_shm_write16(dev, BCM43xx_SHM_SCRATCH,
 			    BCM43xx_SHM_SC_SRLIMIT, tmp);
+	dev->short_retry_limit = tmp;
 	tmp = limit_value(modparam_long_retry, 0, 0xF);
 	bcm43xx_shm_write16(dev, BCM43xx_SHM_SCRATCH,
 			    BCM43xx_SHM_SC_LRLIMIT, tmp);
 
+	dev->long_retry_limit = tmp;
 	bcm43xx_shm_write16(dev, BCM43xx_SHM_SHARED,
 			    BCM43xx_SHM_SH_SFFBLIM, 3);
 	bcm43xx_shm_write16(dev, BCM43xx_SHM_SHARED,

Re: [RFC V2] bcm43xx-mac80211: Provide information to allow transmission rate decreases

[Michael Buesch]

On Wednesday 01 August 2007 22:56:07 Larry Finger wrote:
> Michael,
> 
> I think I took care of your comments in the previous version.
> 
> Larry
> ---
> 
> In bcm43xx-mac80211, the mechanism for decreasing the transmit rate cannot
> be triggered. This may be shown by walking away from the AP with a laptop.
> At some distance, communications will be lost and never recovered because
> the rate decreasing mechanism of rc80211_simple needs to see excessive_retries
> set in the ieee80211_tx_status struct. With this patch, the transmit rate
> will decrease until communications restart.
> 
> Signed-off-by: Larry Finger <Larry.Finger@lwfinger.net>
> ---
> 
> Index: wireless-dev/drivers/net/wireless/bcm43xx-mac80211/bcm43xx_dma.c
> ===================================================================
> --- wireless-dev.orig/drivers/net/wireless/bcm43xx-mac80211/bcm43xx_dma.c
> +++ wireless-dev/drivers/net/wireless/bcm43xx-mac80211/bcm43xx_dma.c
> @@ -1311,6 +1311,16 @@ void bcm43xx_dma_handle_txstatus(struct 
>  			 */
>  			if (status->acked)
>  				meta->txstat.flags |= IEEE80211_TX_STATUS_ACK;
> +			else
> +				if (dev->short_preamble) {

Short preamble is something different.
The thing I was talking about is
mac_ctl |= BCM43xx_TX4_MAC_LONGFRAME;
which we do in xmit.c:338.
We currently do it for frames sent with CTS or RTS.

Though, I have no idea how we can easily track the information for each
packet if it was sent with LONGFRAME bit or without.
It needs to be a per-frame-attribute.
I think the txstatus doesn't tell us (but I'm not sure).

> +					if (status->frame_count >=
> +					    dev->short_retry_limit)
> +						meta->txstat.excessive_retries = 1;
> +				} else {
> +					if (status->frame_count >=
> +					    dev->long_retry_limit)
> +						meta->txstat.excessive_retries = 1;
> +				}
>  			meta->txstat.retry_count = status->frame_count - 1;
>  			ieee80211_tx_status_irqsafe(dev->wl->hw, meta->skb, &(meta->txstat));
>  			/* skb is freed by ieee80211_tx_status_irqsafe() */
> Index: wireless-dev/drivers/net/wireless/bcm43xx-mac80211/bcm43xx.h
> ===================================================================
> --- wireless-dev.orig/drivers/net/wireless/bcm43xx-mac80211/bcm43xx.h
> +++ wireless-dev/drivers/net/wireless/bcm43xx-mac80211/bcm43xx.h
> @@ -707,6 +707,8 @@ struct bcm43xx_wldev {
>  	bool short_preamble;		/* TRUE, if short preamble is enabled. */
>  	bool short_slot;		/* TRUE, if short slot timing is enabled. */
>  	bool radio_hw_enable;		/* saved state of radio hardware enabled state */
> +	u8 short_retry_limit;
> +	u8 long_retry_limit;
>  
>  	/* PHY/Radio device. */
>  	struct bcm43xx_phy phy;
> Index: wireless-dev/drivers/net/wireless/bcm43xx-mac80211/bcm43xx_main.c
> ===================================================================
> --- wireless-dev.orig/drivers/net/wireless/bcm43xx-mac80211/bcm43xx_main.c
> +++ wireless-dev/drivers/net/wireless/bcm43xx-mac80211/bcm43xx_main.c
> @@ -3333,10 +3333,12 @@ static int bcm43xx_wireless_core_init(st
>  	tmp = limit_value(modparam_short_retry, 0, 0xF);
>  	bcm43xx_shm_write16(dev, BCM43xx_SHM_SCRATCH,
>  			    BCM43xx_SHM_SC_SRLIMIT, tmp);
> +	dev->short_retry_limit = tmp;
>  	tmp = limit_value(modparam_long_retry, 0, 0xF);
>  	bcm43xx_shm_write16(dev, BCM43xx_SHM_SCRATCH,
>  			    BCM43xx_SHM_SC_LRLIMIT, tmp);
>  
> +	dev->long_retry_limit = tmp;
>  	bcm43xx_shm_write16(dev, BCM43xx_SHM_SHARED,
>  			    BCM43xx_SHM_SH_SFFBLIM, 3);
>  	bcm43xx_shm_write16(dev, BCM43xx_SHM_SHARED,
> 
> 



-- 
Greetings Michael.

Re: [RFC V2] bcm43xx-mac80211: Provide information to allow transmission rate decreases

[rob m]

#1, 2, 3, and 4:

THANK YOU!!!  for all you are doing to make this card work. 

I wish I could help somehow.  I've been lurking on this mailing list for 
a while now, and most of the emails are way beyond my talents.

I am, at this very moment, using a broadcom "4306" as reported by 
iwconfig, on Debian Etch, installed in early July 2007.  My question:

        What software do you use to determine the quality of the 
connection to the AP? 

gkrellm says that I have a -253 "level".  I know enough about radio 
transmission to know that -253 db is useless, but this interface is 
working fairly well as long as I'm relatively close (up one floor, over 
one room).  I can't connect at the opposite end of my home, <100', which 
is where I want to hang out.

Thanks,

Rob

Larry Finger wrote:
> Michael,
>
>
>   

Re: [RFC V2] bcm43xx-mac80211: Provide information to allow transmission rate decreases

[Johannes Berg]

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

On Thu, 2007-08-02 at 00:14 +0200, Michael Buesch wrote:

> Though, I have no idea how we can easily track the information for each
> packet if it was sent with LONGFRAME bit or without.
> It needs to be a per-frame-attribute.
> I think the txstatus doesn't tell us (but I'm not sure).

Why do you need to know when you get a tx status? Oh I see. Hm. Well,
mac80211 tells you how much to retry now (Daniel made a patch); you can
select long/short based on that and on tx status simply compare with
that number from the tx control struct.

johannes

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

Re: [RFC V2] bcm43xx-mac80211: Provide information to allow transmission rate decreases

[Michael Buesch]

On Thursday 02 August 2007 11:09:47 Johannes Berg wrote:
> On Thu, 2007-08-02 at 00:14 +0200, Michael Buesch wrote:
> 
> > Though, I have no idea how we can easily track the information for each
> > packet if it was sent with LONGFRAME bit or without.
> > It needs to be a per-frame-attribute.
> > I think the txstatus doesn't tell us (but I'm not sure).
> 
> Why do you need to know when you get a tx status? Oh I see. Hm. Well,
> mac80211 tells you how much to retry now (Daniel made a patch); you can
> select long/short based on that and on tx status simply compare with
> that number from the tx control struct.

We don't get a "I failed to transmit this" bit from the firmware,
so we must work around this my comparing the retries count to
the maximum possible retry count.
Of course, that's not an ideal solution. Especially because I think
When it succeed on the exact last retry, we take it as failure.
Though, that's very unlikely it fails 7 times give or take and then
suddenly succeeds.

-- 
Greetings Michael.

Re: [RFC V2] bcm43xx-mac80211: Provide information to allow transmission rate decreases

[Johannes Berg]

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

On Thu, 2007-08-02 at 13:11 +0200, Michael Buesch wrote:

> We don't get a "I failed to transmit this" bit from the firmware,

Well, if you expect the frame to be acked then you do get a bit 'this
frame was ever acked'

> so we must work around this my comparing the retries count to
> the maximum possible retry count.
> Of course, that's not an ideal solution. Especially because I think
> When it succeed on the exact last retry, we take it as failure.
> Though, that's very unlikely it fails 7 times give or take and then
> suddenly succeeds.

johannes

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

Re: [RFC V2] bcm43xx-mac80211: Provide information to allow transmission rate decreases

[Michael Buesch]

On Thursday 02 August 2007 13:18:17 Johannes Berg wrote:
> On Thu, 2007-08-02 at 13:11 +0200, Michael Buesch wrote:
> 
> > We don't get a "I failed to transmit this" bit from the firmware,
> 
> Well, if you expect the frame to be acked then you do get a bit 'this
> frame was ever acked'

Yep, the problem is for frames that don't get acked.
If we just check the ack bit, rate control would throttle, just because
we sent unacked frames.
So if we didn't get an ack, we need to check if we failed, or if...
Oh, acutally. Why not simply check the noack bit in the tx_control... :)

-- 
Greetings Michael.

Re: [RFC V2] bcm43xx-mac80211: Provide information to allow transmission rate decreases

[Johannes Berg]

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

On Thu, 2007-08-02 at 13:37 +0200, Michael Buesch wrote:

> If we just check the ack bit, rate control would throttle, just because
> we sent unacked frames.

Ah but rate control shouldn't actually care about frames that we never
expected an ACK for since there's no way to know for those anyway. So
IMHO the rate control algorithm shouldn't even be called for those
frames.

> So if we didn't get an ack, we need to check if we failed, or if...
> Oh, acutally. Why not simply check the noack bit in the tx_control... :)

Works too, but it seems mac80211 should do that.

johannes

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

Re: [RFC V2] bcm43xx-mac80211: Provide information to allow transmission rate decreases

[Michael Buesch]

On Thursday 02 August 2007 13:48:39 Johannes Berg wrote:
> On Thu, 2007-08-02 at 13:37 +0200, Michael Buesch wrote:
> 
> > If we just check the ack bit, rate control would throttle, just because
> > we sent unacked frames.
> 
> Ah but rate control shouldn't actually care about frames that we never
> expected an ACK for since there's no way to know for those anyway. So
> IMHO the rate control algorithm shouldn't even be called for those
> frames.
> 
> > So if we didn't get an ack, we need to check if we failed, or if...
> > Oh, acutally. Why not simply check the noack bit in the tx_control... :)
> 
> Works too, but it seems mac80211 should do that.

So, what's the point of this "excessive retries" field anyway?
We already have an "acked" bit. So if it's not set, but we expected an
ack, what's the point of setting excessive retries in the driver?
the rc algo sould know _anyway_, as it has the "acked" and the
"we wanted to have an ack" bits.

confused..

-- 
Greetings Michael.

Re: [RFC V2] bcm43xx-mac80211: Provide information to allow transmission rate decreases

[Johannes Berg]

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

On Thu, 2007-08-02 at 15:03 +0200, Michael Buesch wrote:

> So, what's the point of this "excessive retries" field anyway?
> We already have an "acked" bit. So if it's not set, but we expected an
> ack, what's the point of setting excessive retries in the driver?
> the rc algo sould know _anyway_, as it has the "acked" and the
> "we wanted to have an ack" bits.

No idea. I guess you get to dig through the code and remove it ;)

johannes

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

Re: [RFC V2] bcm43xx-mac80211: Provide information to allow transmission rate decreases

[Larry Finger]

Johannes Berg wrote:
> On Thu, 2007-08-02 at 15:03 +0200, Michael Buesch wrote:
> 
>> So, what's the point of this "excessive retries" field anyway?
>> We already have an "acked" bit. So if it's not set, but we expected an
>> ack, what's the point of setting excessive retries in the driver?
>> the rc algo sould know _anyway_, as it has the "acked" and the
>> "we wanted to have an ack" bits.
> 
> No idea. I guess you get to dig through the code and remove it ;)

When I first started investigating the problem of mac80211 not reducing the rate as I moved away 
from the AP, it seemed to me that the decision regarding excessive retries should be made in 
mac80211, not in the driver; however, I have had extreme difficulty in getting any changes into 
mac80211 on several occasions. Linville assures me that he has had private discussions about this 
problem; however, I needed a quick fix and couldn't stand any protracted discussion and/or review 
delays. I knew Michael would be tough, but that his comments would not be delayed.

At the moment, I have more pressing matters to resolve than fixing this problem in mac80211; 
however, I feel really good that the port of bcm43xx-softmac to mac80211 has this issue.

Larry

Re: [RFC V2] bcm43xx-mac80211: Provide information to allow transmission rate decreases

[Johannes Berg]

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

On Thu, 2007-08-02 at 10:02 -0500, Larry Finger wrote:

> When I first started investigating the problem of mac80211 not reducing the rate as I moved away 
> from the AP, it seemed to me that the decision regarding excessive retries should be made in 
> mac80211, not in the driver; however, I have had extreme difficulty in getting any changes into 
> mac80211 on several occasions. Linville assures me that he has had private discussions about this 
> problem; however, I needed a quick fix and couldn't stand any protracted discussion and/or review 
> delays. I knew Michael would be tough, but that his comments would not be delayed.

Agreed. There are definitely problems with getting things into mac80211
and I've complained about that in the past.

> At the moment, I have more pressing matters to resolve than fixing this problem in mac80211; 
> however, I feel really good that the port of bcm43xx-softmac to mac80211 has this issue.

I think that a great many other drivers will have this issue too.

johannes

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