2008-10-07 19:47:40

by John W. Linville

[permalink] [raw]
Subject: [RFC PATCH] rtl8187: do not report ACKs if USB Tx status is non-zero

The vendor-supplied driver treats a USB Tx failure as an un-ACKed frame.
I don't see why we shouldn't do the same thing -- hopefully this makes
rate-scaling algorithms behave sanely with the rtl8187 driver.

Thanks to Felix Fietkau <[email protected]> for suggesting this as an
option.

Signed-off-by: John W. Linville <[email protected]>
---
This is currently untested -- anyone with rtl8187 bored enough to try
it? :-)

drivers/net/wireless/rtl8187_dev.c | 7 ++++++-
1 files changed, 6 insertions(+), 1 deletions(-)

diff --git a/drivers/net/wireless/rtl8187_dev.c b/drivers/net/wireless/rtl8187_dev.c
index ca5deb6..ae21191 100644
--- a/drivers/net/wireless/rtl8187_dev.c
+++ b/drivers/net/wireless/rtl8187_dev.c
@@ -164,7 +164,12 @@ static void rtl8187_tx_cb(struct urb *urb)
skb_pull(skb, priv->is_rtl8187b ? sizeof(struct rtl8187b_tx_hdr) :
sizeof(struct rtl8187_tx_hdr));
memset(&info->status, 0, sizeof(info->status));
- info->flags |= IEEE80211_TX_STAT_ACK;
+ if (!(info->flags & IEEE80211_TX_CTL_NO_ACK)) {
+ if (!urb->status)
+ info->flags |= IEEE80211_TX_STAT_ACK;
+ else /* assume ACK not received */
+ info->status.excessive_retries = 1;
+ }
ieee80211_tx_status_irqsafe(hw, skb);
}

--
1.5.4.3



2008-10-08 20:03:31

by John W. Linville

[permalink] [raw]
Subject: Re: [RFC PATCH] rtl8187: do not report ACKs if USB Tx status is non-zero

On Wed, Oct 08, 2008 at 09:38:49PM +0200, Stefanik G=E1bor wrote:

> Another weird thing: the code above essentially attempts to do this:
> Wait for an ACK
> If not TX_CTL_NO_ACK:
> If acked:
> Report the packet as acked.
> Endif
> If no ACK until timeout:
> Report packet as unacked
> Endif
> Endif
>=20
> This would be better, as it doesn't waste time waiting for an ACK for
> unacked frames:
>=20
> If not TX_CTL_NO_ACK:
> Wait for an ACK
> If acked:
> Report the packet as acked.
> Endif
> If no ACK until timeout:
> Report packet as unacked
> Endif
> Endif
>=20
> Of course, this only works if "Wait for an ACK" actually works.

Well as I understand it, the only waiting is related to the URB
submission (which is asynchronous anyway). I don't really see how
we could avoid it.

John
--=20
John W. Linville Linux should be at the core
[email protected] of your literate lifestyle.

2008-10-08 19:38:50

by Gábor Stefanik

[permalink] [raw]
Subject: Re: [RFC PATCH] rtl8187: do not report ACKs if USB Tx status is non-zero

On Wed, Oct 8, 2008 at 8:42 PM, John W. Linville <[email protected]> wrote:
> On Tue, Oct 07, 2008 at 03:18:18PM -0400, John W. Linville wrote:
>> The vendor-supplied driver treats a USB Tx failure as an un-ACKed frame.
>> I don't see why we shouldn't do the same thing -- hopefully this makes
>> rate-scaling algorithms behave sanely with the rtl8187 driver.
>>
>> Thanks to Felix Fietkau <[email protected]> for suggesting this as an
>> option.
>>
>> Signed-off-by: John W. Linville <[email protected]>
>> ---
>> This is currently untested -- anyone with rtl8187 bored enough to try
>> it? :-)
>
> AFAICT, this doesn't actually trigger -- at least, the device can
> go into its typical failure mode (no frames ACKed until you force a
> lower rate) without ever hitting the "assume ACK not received" clause.
>
> I'll keep looking at it -- there are a couple of 'secrets' still
> buried in the vendor driver (if I can stomach to keep looking at it)...
>
> John
> --
> John W. Linville Linux should be at the core
> [email protected] of your literate lifestyle.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>

Another weird thing: the code above essentially attempts to do this:
Wait for an ACK
If not TX_CTL_NO_ACK:
If acked:
Report the packet as acked.
Endif
If no ACK until timeout:
Report packet as unacked
Endif
Endif

This would be better, as it doesn't waste time waiting for an ACK for
unacked frames:

If not TX_CTL_NO_ACK:
Wait for an ACK
If acked:
Report the packet as acked.
Endif
If no ACK until timeout:
Report packet as unacked
Endif
Endif

Of course, this only works if "Wait for an ACK" actually works.

--
Vista: [V]iruses, [I]ntruders, [S]pyware, [T]rojans and [A]dware. :-)

Subject: Re: [RFC PATCH] rtl8187: do not report ACKs if USB Tx status is non-zero

On Wednesday 08 October 2008 15:42:05 John W. Linville wrote:
> On Tue, Oct 07, 2008 at 03:18:18PM -0400, John W. Linville wrote:
> > The vendor-supplied driver treats a USB Tx failure as an un-ACKed frame.
> > I don't see why we shouldn't do the same thing -- hopefully this makes
> > rate-scaling algorithms behave sanely with the rtl8187 driver.
> >
> > Thanks to Felix Fietkau <[email protected]> for suggesting this as an
> > option.
> >
> > Signed-off-by: John W. Linville <[email protected]>
> > ---
> > This is currently untested -- anyone with rtl8187 bored enough to try
> > it? :-)
>
> AFAICT, this doesn't actually trigger -- at least, the device can
> go into its typical failure mode (no frames ACKed until you force a
> lower rate) without ever hitting the "assume ACK not received" clause.

I tested here and it didn't trigger indeed, here at least urb->status == 0
always no matter what I tried, which seems normal, only would trigger at some
not typical usb failure? But there is one thing that I like about the patch,
seems correct that we shouldn't set IEEE80211_TX_STAT_ACK if
IEEE80211_TX_CTL_NO_ACK, that is, looks like a bug always setting ack in the
flags when no_ack is present, not sure if it has some effect in practice
though (have to look more at the code that cares about info->flags). I would
say that the patch 'as is' is correct and will not hurt.

>
> I'll keep looking at it -- there are a couple of 'secrets' still
> buried in the vendor driver (if I can stomach to keep looking at it)...

yeah, not very straightforward to grasp the vendor driver unfortunately...
good would be to have some times docs/explanation about some of the values
used, when you're luck there is some comments and references at some places.

>
> John

--
[]'s
Herton

2008-10-08 18:42:45

by John W. Linville

[permalink] [raw]
Subject: Re: [RFC PATCH] rtl8187: do not report ACKs if USB Tx status is non-zero

On Tue, Oct 07, 2008 at 03:18:18PM -0400, John W. Linville wrote:
> The vendor-supplied driver treats a USB Tx failure as an un-ACKed frame.
> I don't see why we shouldn't do the same thing -- hopefully this makes
> rate-scaling algorithms behave sanely with the rtl8187 driver.
>
> Thanks to Felix Fietkau <[email protected]> for suggesting this as an
> option.
>
> Signed-off-by: John W. Linville <[email protected]>
> ---
> This is currently untested -- anyone with rtl8187 bored enough to try
> it? :-)

AFAICT, this doesn't actually trigger -- at least, the device can
go into its typical failure mode (no frames ACKed until you force a
lower rate) without ever hitting the "assume ACK not received" clause.

I'll keep looking at it -- there are a couple of 'secrets' still
buried in the vendor driver (if I can stomach to keep looking at it)...

John
--
John W. Linville Linux should be at the core
[email protected] of your literate lifestyle.