2007-09-08 21:43:47

by Michael Büsch

[permalink] [raw]
Subject: zd-mac80211: Fix TX status reports.

Automatic rate scaling does not work on the zd-mac80211 driver.
The driver does not properly report succeed and failed frames
to mac80211.

We need to indicate failed frames with the excessive_retries field
(Should we also fake report some high retry count here?)
Otherwise the rc algo is not able to scale down.

Remove the conditional in REQ_TX_STATUS, as mac80211 handles that internally.

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

---


Johannes, to me it seems that there's also a bug in mac80211.
I never get a frame with the REQ_TX_STATUS bit set, so frames
will always end up on the "unreliable" tx status queue.


Index: wireless-dev/drivers/net/wireless/zd1211rw-mac80211/zd_mac.c
===================================================================
--- wireless-dev.orig/drivers/net/wireless/zd1211rw-mac80211/zd_mac.c 2007-09-07 16:13:10.000000000 +0200
+++ wireless-dev/drivers/net/wireless/zd1211rw-mac80211/zd_mac.c 2007-09-08 23:33:47.000000000 +0200
@@ -348,6 +348,7 @@ static int init_tx_skb_control_block(str
* @hw - a &struct ieee80211_hw pointer
* @skb - a sk-buffer
* @status - the tx status of the packet without control information
+ * @success - True for successfull transmission of the frame
*
* This information calls ieee80211_tx_status_irqsafe() if required by the
* control information. It copies the control information into the status
@@ -356,19 +357,18 @@ static int init_tx_skb_control_block(str
* If no status information has been requested, the skb is freed.
*/
static void tx_status(struct ieee80211_hw *hw, struct sk_buff *skb,
- struct ieee80211_tx_status *status)
+ struct ieee80211_tx_status *status,
+ bool success)
{
struct zd_tx_skb_control_block *cb = (struct zd_tx_skb_control_block *)
skb->cb;

ZD_ASSERT(cb->control != NULL);
- if ((cb->control->flags & IEEE80211_TXCTL_REQ_TX_STATUS)) {
- memcpy(&status->control, cb->control, sizeof(status->control));
- clear_tx_skb_control_block(skb);
- ieee80211_tx_status_irqsafe(hw, skb, status);
- } else {
- kfree_tx_skb(skb);
- }
+ memcpy(&status->control, cb->control, sizeof(status->control));
+ if (!success)
+ status->excessive_retries = 1;
+ clear_tx_skb_control_block(skb);
+ ieee80211_tx_status_irqsafe(hw, skb, status);
}

/**
@@ -388,7 +388,7 @@ void zd_mac_tx_failed(struct ieee80211_h
skb = skb_dequeue(q);
if (skb == NULL)
return;
- tx_status(hw, skb, &status);
+ tx_status(hw, skb, &status, 0);
}

/**
@@ -413,7 +413,7 @@ void zd_mac_tx_to_dev(struct sk_buff *sk
(cb->control->flags & IEEE80211_TXCTL_NO_ACK)))
{
struct ieee80211_tx_status status = {{0}};
- tx_status(hw, skb, &status);
+ tx_status(hw, skb, &status, !error);
} else {
struct sk_buff_head *q =
&zd_hw_mac(hw)->ack_wait_queue;
@@ -664,7 +664,7 @@ static int filter_ack(struct ieee80211_h
status.flags = IEEE80211_TX_STATUS_ACK;
status.ack_signal = stats->ssi;
__skb_unlink(skb, q);
- tx_status(hw, skb, &status);
+ tx_status(hw, skb, &status, 1);
goto out;
}
}


2007-09-11 10:28:30

by Johannes Berg

[permalink] [raw]
Subject: Re: zd-mac80211: Fix TX status reports.

On Tue, 2007-09-11 at 12:20 +0200, Michael Buesch wrote:

> What about the following:
> We have a "the packet failed" IRQ. so we know that if that didn't
> raise for a packet, it must have succeed.
> So currently we already maintain a queue of TX packets. What about
> changing the handling of this queue? Instead of dropping (and
> telling mac80211 success) on an ACK RX, simply do a timeout.
> We can calculate the time (plus some additional msecs to be sure)
> by when an ACK must have arrived, no?

That's tricky though, because multiple retry rates mean that it can
possibly take quite a while for the packet to go through. And ath5k
wants to support up to 7 different rates for each packet.

> So, if that times out,
> signal a success. Wouldn't that be reliable? Given that the "tx failed"
> IRQ actually _is_ reliable.

Yeah, and in-order would have to be guaranteed as well so we can match
which packet failed and assume all previous ones were OK.

johannes


Attachments:
signature.asc (190.00 B)
This is a digitally signed message part

2007-09-12 08:40:56

by Tomas Winkler

[permalink] [raw]
Subject: Re: zd-mac80211: Fix TX status reports.

On 9/12/07, Ulrich Kunitz <[email protected]> wrote:
> Johannes Berg wrote:
>
> > On Tue, 2007-09-11 at 12:52 +0200, Michael Buesch wrote:
> > > On Tuesday 11 September 2007 12:29:56 Johannes Berg wrote:
> > > > On Tue, 2007-09-11 at 12:20 +0200, Michael Buesch wrote:
> > > >
> > > > > What about the following:
> > > > > We have a "the packet failed" IRQ. so we know that if that didn't
> > > > > raise for a packet, it must have succeed.
> > > > > So currently we already maintain a queue of TX packets. What about
> > > > > changing the handling of this queue? Instead of dropping (and
> > > > > telling mac80211 success) on an ACK RX, simply do a timeout.
> > > > > We can calculate the time (plus some additional msecs to be sure)
> > > > > by when an ACK must have arrived, no?
> > > >
> > > > That's tricky though, because multiple retry rates mean that it can
> > > > possibly take quite a while for the packet to go through. And ath5k
> > > > wants to support up to 7 different rates for each packet.
> > >
> > > I'm only talking about zd, though.
> >
> > Yeah, but that means the driver should implement it because it has much
> > less problems getting the heuristics right.
>
> Notify that the TX-failed interrupt has only the destination MAC
> address of the transmitted packet. So if the driver has sent two packets
> to the device and both are still in their timeout time, it will
> not be known, whether the first or the second failed. (The
> destination address for STAs will always be the same.)
>
> Sending only one packet until the timeout period is over is
> certainly doable and could be used for critical activity like
> association and authentication, but for normal mode the driver
> should only be required to provide statistics.
>
That was my suggestion as well.
> --
> Uli Kunitz
>

2007-09-11 10:03:26

by Tomas Winkler

[permalink] [raw]
Subject: Re: zd-mac80211: Fix TX status reports.

On 9/11/07, Daniel Drake <[email protected]> wrote:
> Tomas Winkler wrote:
> > Receiving all ACKs packets will degraded performance greatly for sure,
> > even on PCI interface.
> > Receving ACK can maybe allowed selectively only for management packets
> > including EAPOLS in some synchronous way, yet this is still error
> > prone.
>
> I don't think we have such control. It's either all ACKs or no ACKs.
>
For MLME completeness
Set timer for direct management frames and EAPOLs cancel the timer on
failure TX in interrupt arrival. Send TX status success from the
timer handler.
For data frames just report success otherwise it will effect performance.
The drawback is that you need to watch each TX frame in the driver.
..
You probably have to implement your own rate scale statistic
collection like iwlwifi does

> > It looks strange to me that there is no other HW mechanism that does that?
> > Is this driver reverse engineered?
>
> No. There is another mechanism for detecting failed transmissions: we
> get an interrupt for every failed TX. I feel that this should be enough
> to implement rate control, however mac80211 at the moment requires
> reports of both success and failure for rate control to work. Detecting
> success is the tricky part.
>
> Daniel
>
>

2007-09-11 10:23:41

by Michael Büsch

[permalink] [raw]
Subject: Re: zd-mac80211: Fix TX status reports.

On Tuesday 11 September 2007 12:17:01 Johannes Berg wrote:
> > However I liked the idea about
> >
> > "An alternative option could be that the mac80211 stack would call
> > a non-atomic function of the driver to report the total count of
> > transmitted packets and the number of successful transmissions."
> >
> > This mechanism will help us to implement aggregation rate scaling
> > I would actually prefer if such statistics will be pushed by driver
> > rather then pulled by mac though.
>
> It doesn't help knowing whether the relevant packets were acked though.

What about the following:
We have a "the packet failed" IRQ. so we know that if that didn't
raise for a packet, it must have succeed.
So currently we already maintain a queue of TX packets. What about
changing the handling of this queue? Instead of dropping (and
telling mac80211 success) on an ACK RX, simply do a timeout.
We can calculate the time (plus some additional msecs to be sure)
by when an ACK must have arrived, no? So, if that times out,
signal a success. Wouldn't that be reliable? Given that the "tx failed"
IRQ actually _is_ reliable.

--
Greetings Michael.

2007-09-11 22:32:59

by Ulrich Kunitz

[permalink] [raw]
Subject: Re: zd-mac80211: Fix TX status reports.

Johannes Berg wrote:

> On Tue, 2007-09-11 at 12:52 +0200, Michael Buesch wrote:
> > On Tuesday 11 September 2007 12:29:56 Johannes Berg wrote:
> > > On Tue, 2007-09-11 at 12:20 +0200, Michael Buesch wrote:
> > >
> > > > What about the following:
> > > > We have a "the packet failed" IRQ. so we know that if that didn't
> > > > raise for a packet, it must have succeed.
> > > > So currently we already maintain a queue of TX packets. What about
> > > > changing the handling of this queue? Instead of dropping (and
> > > > telling mac80211 success) on an ACK RX, simply do a timeout.
> > > > We can calculate the time (plus some additional msecs to be sure)
> > > > by when an ACK must have arrived, no?
> > >
> > > That's tricky though, because multiple retry rates mean that it can
> > > possibly take quite a while for the packet to go through. And ath5k
> > > wants to support up to 7 different rates for each packet.
> >
> > I'm only talking about zd, though.
>
> Yeah, but that means the driver should implement it because it has much
> less problems getting the heuristics right.

Notify that the TX-failed interrupt has only the destination MAC
address of the transmitted packet. So if the driver has sent two packets
to the device and both are still in their timeout time, it will
not be known, whether the first or the second failed. (The
destination address for STAs will always be the same.)

Sending only one packet until the timeout period is over is
certainly doable and could be used for critical activity like
association and authentication, but for normal mode the driver
should only be required to provide statistics.

--
Uli Kunitz

2007-09-11 10:55:49

by Johannes Berg

[permalink] [raw]
Subject: Re: zd-mac80211: Fix TX status reports.

On Tue, 2007-09-11 at 12:52 +0200, Michael Buesch wrote:
> On Tuesday 11 September 2007 12:29:56 Johannes Berg wrote:
> > On Tue, 2007-09-11 at 12:20 +0200, Michael Buesch wrote:
> >
> > > What about the following:
> > > We have a "the packet failed" IRQ. so we know that if that didn't
> > > raise for a packet, it must have succeed.
> > > So currently we already maintain a queue of TX packets. What about
> > > changing the handling of this queue? Instead of dropping (and
> > > telling mac80211 success) on an ACK RX, simply do a timeout.
> > > We can calculate the time (plus some additional msecs to be sure)
> > > by when an ACK must have arrived, no?
> >
> > That's tricky though, because multiple retry rates mean that it can
> > possibly take quite a while for the packet to go through. And ath5k
> > wants to support up to 7 different rates for each packet.
>
> I'm only talking about zd, though.

Yeah, but that means the driver should implement it because it has much
less problems getting the heuristics right.

johannes


Attachments:
signature.asc (190.00 B)
This is a digitally signed message part

2007-09-11 03:50:59

by Tomas Winkler

[permalink] [raw]
Subject: Re: zd-mac80211: Fix TX status reports.

On 9/10/07, Johannes Berg <[email protected]> wrote:
> On Mon, 2007-09-10 at 12:58 +0200, Michael Buesch wrote:
>
> > I think the ack callback is pretty reliable, so that should be OK.
> > It looks for the ACK in software.
>
> Right. But Uli mentioned that it can become out of sync. Is there no
> indication about how many times a packet was transmitted before it was
> acked?
>
Receiving all ACKs packets will degraded performance greatly for sure,
even on PCI interface.
Receving ACK can maybe allowed selectively only for management packets
including EAPOLS in some synchronous way, yet this is still error
prone.
It looks strange to me that there is no other HW mechanism that does that?
Is this driver reverse engineered?

However I liked the idea about

"An alternative option could be that the mac80211 stack would call
a non-atomic function of the driver to report the total count of
transmitted packets and the number of successful transmissions."

This mechanism will help us to implement aggregation rate scaling
I would actually prefer if such statistics will be pushed by driver
rather then pulled by mac though.

Thanks
Tomas

> johannes
>
>

2007-09-10 10:50:55

by Johannes Berg

[permalink] [raw]
Subject: Re: zd-mac80211: Fix TX status reports.

On Sun, 2007-09-09 at 12:58 +0200, Michael Buesch wrote:

> Basically, the TX status report is only useful for the rate control
> algorithm. So it's basically "only" statistics. But we have to
> get these statistics approximately right to get the RC algo working
> correctly. That is what this patch does. With it, the RC algo properly
> scales up and down if the signal gets better or worse.

Michael, this is unfortunately not true. At least hostapd relies on the
ack callback because it requires stations to acknowledge the association
response before advancing the state machine, and we have also made
monitoring outgoing packets depend on the ack callback.

> > > Johannes, to me it seems that there's also a bug in mac80211.
> > > I never get a frame with the REQ_TX_STATUS bit set, so frames
> > > will always end up on the "unreliable" tx status queue.

Yes, this is by design, you will only get the bit set on any frame if
hostapd requested it via setting one of the version bits when sending a
frame on the management interface.

> Well, I think the driver should ignore that flag in any case, as
> mac80211 does handle it internally.
> Especially on devices like the zd, which don't support TX status
> reporting in hw, we should gather as much information as possible
> and pass it to mac80211 to get the RC algorithm working.
> mac80211 will handle the unrequested status requests with more care,
> so that's OK.

This is a really fine line to walk. For "good" rate scaling like
minstrel or such you really need multiple retry rates per packet and
such; for "simple" rate scaling just statistics are sufficient. However,
in the case of hostapd, having the tx status callback be precise is
actually required for *correctness*.

johannes


Attachments:
signature.asc (190.00 B)
This is a digitally signed message part

2007-09-09 11:01:36

by Michael Büsch

[permalink] [raw]
Subject: Re: zd-mac80211: Fix TX status reports.

On Sunday 09 September 2007 12:05:30 Ulrich Kunitz wrote:
> Just a general remark: The ZD1211 device doesn't support TX status
> reporting directly. The driver emulates this by getting ACK
> packets and transmission failures reported. The driver simply
> reports them in sequence to transmitted packets without having a
> reliable way to verify that the ACK packet or the failure report
> actually belongs to the packet for which the status is reported.
> This is error-prone. A typical problem are several correct ACKs
> received for the same packet. In such a situation the driver loses
> the synchronization of status and transmitted packets. If the
> mac80211 stack uses the returned status for more than statistics,
> this may cause bugs.
[snip]
> An alternative option could be that the mac80211 stack would call
> a non-atomic function of the driver to report the total count of
> transmitted packets and the number of successful transmissions.
> The driver could even use counters maintained by the device to
> support this. In specific cases where a status is required and
> performance is not an issue (association) a reliable status report
> mode could be supported, but this would add complexity to the
> driver. I understand that this would require invasive changes to
> the mac80211 stack, but I believe this change would be the right
> thing.

Basically, the TX status report is only useful for the rate control
algorithm. So it's basically "only" statistics. But we have to
get these statistics approximately right to get the RC algo working
correctly. That is what this patch does. With it, the RC algo properly
scales up and down if the signal gets better or worse.

> > Johannes, to me it seems that there's also a bug in mac80211.
> > I never get a frame with the REQ_TX_STATUS bit set, so frames
> > will always end up on the "unreliable" tx status queue.
>
> It appears that the patch tries to fix this by ignoring
> REQ_TX_STATUS. See below.

Well, I think the driver should ignore that flag in any case, as
mac80211 does handle it internally.
Especially on devices like the zd, which don't support TX status
reporting in hw, we should gather as much information as possible
and pass it to mac80211 to get the RC algorithm working.
mac80211 will handle the unrequested status requests with more care,
so that's OK.

> > @@ -356,19 +357,18 @@ static int init_tx_skb_control_block(str
> > * If no status information has been requested, the skb is freed.
> > */
> > static void tx_status(struct ieee80211_hw *hw, struct sk_buff *skb,
> > - struct ieee80211_tx_status *status)
> > + struct ieee80211_tx_status *status,
> > + bool success)
> > {
> > struct zd_tx_skb_control_block *cb = (struct zd_tx_skb_control_block *)
> > skb->cb;
> >
> > ZD_ASSERT(cb->control != NULL);
> > - if ((cb->control->flags & IEEE80211_TXCTL_REQ_TX_STATUS)) {
> > - memcpy(&status->control, cb->control, sizeof(status->control));
> > - clear_tx_skb_control_block(skb);
> > - ieee80211_tx_status_irqsafe(hw, skb, status);
> > - } else {
> > - kfree_tx_skb(skb);
> > - }
> > + memcpy(&status->control, cb->control, sizeof(status->control));
> > + if (!success)
> > + status->excessive_retries = 1;
> > + clear_tx_skb_control_block(skb);
> > + ieee80211_tx_status_irqsafe(hw, skb, status);
> > }
>
>



--
Greetings Michael.

2007-09-12 08:53:13

by Johannes Berg

[permalink] [raw]
Subject: Re: zd-mac80211: Fix TX status reports.

On Wed, 2007-09-12 at 11:40 +0300, Tomas Winkler wrote:

> > Sending only one packet until the timeout period is over is
> > certainly doable and could be used for critical activity like
> > association and authentication, but for normal mode the driver
> > should only be required to provide statistics.

> That was my suggestion as well.

The only question is how we know which packets we need to be careful
with. Right now, it seems that is only packets from hostapd?

johannes


Attachments:
signature.asc (190.00 B)
This is a digitally signed message part

2007-09-11 10:54:37

by Michael Büsch

[permalink] [raw]
Subject: Re: zd-mac80211: Fix TX status reports.

On Tuesday 11 September 2007 12:29:56 Johannes Berg wrote:
> On Tue, 2007-09-11 at 12:20 +0200, Michael Buesch wrote:
>
> > What about the following:
> > We have a "the packet failed" IRQ. so we know that if that didn't
> > raise for a packet, it must have succeed.
> > So currently we already maintain a queue of TX packets. What about
> > changing the handling of this queue? Instead of dropping (and
> > telling mac80211 success) on an ACK RX, simply do a timeout.
> > We can calculate the time (plus some additional msecs to be sure)
> > by when an ACK must have arrived, no?
>
> That's tricky though, because multiple retry rates mean that it can
> possibly take quite a while for the packet to go through. And ath5k
> wants to support up to 7 different rates for each packet.

I'm only talking about zd, though.

> > So, if that times out,
> > signal a success. Wouldn't that be reliable? Given that the "tx failed"
> > IRQ actually _is_ reliable.
>
> Yeah, and in-order would have to be guaranteed as well so we can match
> which packet failed and assume all previous ones were OK.

--
Greetings Michael.

2007-09-13 05:56:10

by Ulrich Kunitz

[permalink] [raw]
Subject: Re: zd-mac80211: Fix TX status reports.

Johannes Berg wrote:

> On Wed, 2007-09-12 at 10:54 +0200, Johannes Berg wrote:
> > On Wed, 2007-09-12 at 11:40 +0300, Tomas Winkler wrote:
> >
> > > > Sending only one packet until the timeout period is over is
> > > > certainly doable and could be used for critical activity like
> > > > association and authentication, but for normal mode the driver
> > > > should only be required to provide statistics.
> >
> > > That was my suggestion as well.
> >
> > The only question is how we know which packets we need to be careful
> > with. Right now, it seems that is only packets from hostapd?
>
> Also, do we really want to basically stop the whole AP when a new
> station is trying to associate? Maybe zd1211 is just not suitable for
> running an AP.

This is a valid point. However stopping the whole AP would not be
necessary, because the stop is only required for a particular
destination address. The destination address is available from the
TX retry failed interrupt.

I will try to experiment with a TX implementatation without
screening for ACKs for zd1211rw-mac80211 using Michael's timeout
idea by ensuring that only one packet per destination address is
transferred at a time to the device and positively acknowledge the
packet after a timeout if no failure interrupt has been received.

I would however still recommend to verify, whether rate-selection
algorithms could work with statistics only. The mac80211 stack
should require only ACKs, when they are in fact needed to support
ieee80211 protocols.

--
Uli Kunitz

2007-09-10 11:01:17

by Michael Büsch

[permalink] [raw]
Subject: Re: zd-mac80211: Fix TX status reports.

On Monday 10 September 2007 12:52:21 Johannes Berg wrote:
> On Sun, 2007-09-09 at 12:58 +0200, Michael Buesch wrote:
>
> > Basically, the TX status report is only useful for the rate control
> > algorithm. So it's basically "only" statistics. But we have to
> > get these statistics approximately right to get the RC algo working
> > correctly. That is what this patch does. With it, the RC algo properly
> > scales up and down if the signal gets better or worse.
>
> Michael, this is unfortunately not true. At least hostapd relies on the
> ack callback because it requires stations to acknowledge the association
> response before advancing the state machine, and we have also made
> monitoring outgoing packets depend on the ack callback.

I think the ack callback is pretty reliable, so that should be OK.
It looks for the ACK in software.

--
Greetings Michael.

2007-09-13 07:14:37

by Johannes Berg

[permalink] [raw]
Subject: Re: zd-mac80211: Fix TX status reports.

On Thu, 2007-09-13 at 07:56 +0200, Ulrich Kunitz wrote:

> This is a valid point. However stopping the whole AP would not be
> necessary, because the stop is only required for a particular
> destination address. The destination address is available from the
> TX retry failed interrupt.

True, but there's currently no way to ensure that unless you want to
silently drop the second packet, you can only stop the whole queue or
accept all packets, not stop a single destination mac address from
getting frames.

> I would however still recommend to verify, whether rate-selection
> algorithms could work with statistics only.

That very much depends on the algorithm. "simple" can probably work with
statistics only, but algorithms such as "minstrel" or "SampleRate" will
need more detailed information. But we need a framework for indicating
hw capabilities to the rate control algorithms anyway, so this can be
part of that framework and the more advanced algorithms just won't work
with zd1211 then. "minstrel" also requires multiple retry rates per
packet.

johannes


Attachments:
signature.asc (190.00 B)
This is a digitally signed message part

2007-09-11 07:54:37

by Daniel Drake

[permalink] [raw]
Subject: Re: zd-mac80211: Fix TX status reports.

Tomas Winkler wrote:
> Receiving all ACKs packets will degraded performance greatly for sure,
> even on PCI interface.
> Receving ACK can maybe allowed selectively only for management packets
> including EAPOLS in some synchronous way, yet this is still error
> prone.

I don't think we have such control. It's either all ACKs or no ACKs.

> It looks strange to me that there is no other HW mechanism that does that?
> Is this driver reverse engineered?

No. There is another mechanism for detecting failed transmissions: we
get an interrupt for every failed TX. I feel that this should be enough
to implement rate control, however mac80211 at the moment requires
reports of both success and failure for rate control to work. Detecting
success is the tricky part.

Daniel


2007-09-10 11:08:42

by Johannes Berg

[permalink] [raw]
Subject: Re: zd-mac80211: Fix TX status reports.

On Mon, 2007-09-10 at 12:58 +0200, Michael Buesch wrote:

> I think the ack callback is pretty reliable, so that should be OK.
> It looks for the ACK in software.

Right. But Uli mentioned that it can become out of sync. Is there no
indication about how many times a packet was transmitted before it was
acked?

johannes


Attachments:
signature.asc (190.00 B)
This is a digitally signed message part

2007-09-09 10:05:31

by Ulrich Kunitz

[permalink] [raw]
Subject: Re: zd-mac80211: Fix TX status reports.

Michael Buesch wrote:

> Automatic rate scaling does not work on the zd-mac80211 driver.
> The driver does not properly report succeed and failed frames
> to mac80211.
>
> We need to indicate failed frames with the excessive_retries field
> (Should we also fake report some high retry count here?)
> Otherwise the rc algo is not able to scale down.
>
> Remove the conditional in REQ_TX_STATUS, as mac80211 handles that internally.
>
> Signed-off-by: Michael Buesch <[email protected]>

Just a general remark: The ZD1211 device doesn't support TX status
reporting directly. The driver emulates this by getting ACK
packets and transmission failures reported. The driver simply
reports them in sequence to transmitted packets without having a
reliable way to verify that the ACK packet or the failure report
actually belongs to the packet for which the status is reported.
This is error-prone. A typical problem are several correct ACKs
received for the same packet. In such a situation the driver loses
the synchronization of status and transmitted packets. If the
mac80211 stack uses the returned status for more than statistics,
this may cause bugs.

The mac80211 stack currently requires the driver to support
semantics it has no way to support correctly without harming
performance. (There is the option to transmit one packet and wait
a fixed time to report the success of the packet.)

It should also be noted that requiring the device to report ACK
packets to the host takes USB bandwidth away and increases also
the interrupt load on the host. There is a measurable impact
depending on the type of USB interface and the speed of the host
processor.

An alternative option could be that the mac80211 stack would call
a non-atomic function of the driver to report the total count of
transmitted packets and the number of successful transmissions.
The driver could even use counters maintained by the device to
support this. In specific cases where a status is required and
performance is not an issue (association) a reliable status report
mode could be supported, but this would add complexity to the
driver. I understand that this would require invasive changes to
the mac80211 stack, but I believe this change would be the right
thing.

> Johannes, to me it seems that there's also a bug in mac80211.
> I never get a frame with the REQ_TX_STATUS bit set, so frames
> will always end up on the "unreliable" tx status queue.

It appears that the patch tries to fix this by ignoring
REQ_TX_STATUS. See below.

> @@ -356,19 +357,18 @@ static int init_tx_skb_control_block(str
> * If no status information has been requested, the skb is freed.
> */
> static void tx_status(struct ieee80211_hw *hw, struct sk_buff *skb,
> - struct ieee80211_tx_status *status)
> + struct ieee80211_tx_status *status,
> + bool success)
> {
> struct zd_tx_skb_control_block *cb = (struct zd_tx_skb_control_block *)
> skb->cb;
>
> ZD_ASSERT(cb->control != NULL);
> - if ((cb->control->flags & IEEE80211_TXCTL_REQ_TX_STATUS)) {
> - memcpy(&status->control, cb->control, sizeof(status->control));
> - clear_tx_skb_control_block(skb);
> - ieee80211_tx_status_irqsafe(hw, skb, status);
> - } else {
> - kfree_tx_skb(skb);
> - }
> + memcpy(&status->control, cb->control, sizeof(status->control));
> + if (!success)
> + status->excessive_retries = 1;
> + clear_tx_skb_control_block(skb);
> + ieee80211_tx_status_irqsafe(hw, skb, status);
> }


--
Uli Kunitz

2007-09-11 10:16:14

by Johannes Berg

[permalink] [raw]
Subject: Re: zd-mac80211: Fix TX status reports.

On Tue, 2007-09-11 at 06:50 +0300, Tomas Winkler wrote:

> Receiving all ACKs packets will degraded performance greatly for sure,
> even on PCI interface.

Well, b43 gives you another way to get transmit status indication, you
don't have to receive ACKs

> Receving ACK can maybe allowed selectively only for management packets
> including EAPOLS in some synchronous way, yet this is still error
> prone.

Yeah that's the thing...

> It looks strange to me that there is no other HW mechanism that does that?
> Is this driver reverse engineered?

Sort of, from the driver they had published.

> However I liked the idea about
>
> "An alternative option could be that the mac80211 stack would call
> a non-atomic function of the driver to report the total count of
> transmitted packets and the number of successful transmissions."
>
> This mechanism will help us to implement aggregation rate scaling
> I would actually prefer if such statistics will be pushed by driver
> rather then pulled by mac though.

It doesn't help knowing whether the relevant packets were acked though.

johannes


Attachments:
signature.asc (190.00 B)
This is a digitally signed message part

2007-09-12 08:55:05

by Johannes Berg

[permalink] [raw]
Subject: Re: zd-mac80211: Fix TX status reports.

On Wed, 2007-09-12 at 10:54 +0200, Johannes Berg wrote:
> On Wed, 2007-09-12 at 11:40 +0300, Tomas Winkler wrote:
>
> > > Sending only one packet until the timeout period is over is
> > > certainly doable and could be used for critical activity like
> > > association and authentication, but for normal mode the driver
> > > should only be required to provide statistics.
>
> > That was my suggestion as well.
>
> The only question is how we know which packets we need to be careful
> with. Right now, it seems that is only packets from hostapd?

Also, do we really want to basically stop the whole AP when a new
station is trying to associate? Maybe zd1211 is just not suitable for
running an AP.

johannes


Attachments:
signature.asc (190.00 B)
This is a digitally signed message part