(Patch also available as an attachment in case Gmail decides to linewrap it.)
Currently we always wait for an ACK even if IEEE80211_TX_CTL_NO_ACK is set.
This is just the opposite of what other drivers do.
Change both RTL8187L and RTL8187B to skip waiting for an ACK when no
ACK was requested.
Signed-off-by: G?bor Stefanik <[email protected]>
---
drivers/net/wireless/rtl818x/rtl8187_dev.c | 10 ++++++++--
1 files changed, 8 insertions(+), 2 deletions(-)
diff --git a/drivers/net/wireless/rtl818x/rtl8187_dev.c
b/drivers/net/wireless/rtl818x/rtl8187_dev.c
index 9fd23b5..7f12d9f 100644
--- a/drivers/net/wireless/rtl818x/rtl8187_dev.c
+++ b/drivers/net/wireless/rtl818x/rtl8187_dev.c
@@ -230,7 +230,10 @@ static int rtl8187_tx(struct ieee80211_hw *dev,
struct sk_buff *skb)
hdr->flags = cpu_to_le32(flags);
hdr->len = 0;
hdr->rts_duration = rts_dur;
- hdr->retry = cpu_to_le32(info->control.rates[0].count << 8);
+ if (!(info->flags & IEEE80211_TX_CTL_NO_ACK))
+ hdr->retry = cpu_to_le32(info->control.rates[0].count << 8);
+ else
+ hdr->retry = 0;
buf = hdr;
ep = 2;
@@ -248,7 +251,10 @@ static int rtl8187_tx(struct ieee80211_hw *dev,
struct sk_buff *skb)
memset(hdr, 0, sizeof(*hdr));
hdr->flags = cpu_to_le32(flags);
hdr->rts_duration = rts_dur;
- hdr->retry = cpu_to_le32(info->control.rates[0].count << 8);
+ if (!(info->flags & IEEE80211_TX_CTL_NO_ACK))
+ hdr->retry = cpu_to_le32(info->control.rates[0].count << 8);
+ else
+ hdr->retry = 0;
hdr->tx_duration =
ieee80211_generic_frame_duration(dev, priv->vif,
skb->len, txrate);
--
1.5.4.5
On Thu, Nov 27, 2008 at 10:59 PM, Johannes Berg
<[email protected]> wrote:
> On Thu, 2008-11-27 at 19:52 -0200, Herton Ronaldo Krzesinski wrote:
>
>> In this case then shouldn't mac80211/rate control alg give a right rates[0].count?
>
> It probably should put 1 into that value, yes. It doesn't seem to
> enforce that now though, I'll take a look at doing that.
>
>> And other drivers have same bug?
>
> possibly.
>
> Another thing: the ".count" is the number of tries you should do, so if
> the hardware expects retries then you need to subtract 1.
>
> johannes
>
I actually tested that 0 is the right setting - I initially tested it
with 1, and there was evidence that it was still retrying. Setting it
to 0 truly behaved like a NO_ACK bit would on other HW. (I used
aireplay-ng to get a view of transmission speed - the "chopchop" and
"interactive replay" are good indicators. I used a patch to make all
injected frames NO_ACK, then tested the highest rate that results in
no multiple tries for the same number in chopchop. With the original
code, I could transmit 25 frames per second - anything higher resulted
in chopchop requiring more than 256 packets for each byte. Most
drivers start to behave this way over about 600 to 700 packets per
second. Setting the retry count to 1 resulted in a max of 220/s before
producing more-than-256-attempts errors, still lower than what other
drivers do with NO_ACK, suggestive of the driver still retrying.
Setting to 0 resulted in a 625/s max, on par with other drivers.)
This means, hdr->retry is the number of retries, unlike ".count",
which is the number of tries, including the initial transmit.
--
Vista: [V]iruses, [I]ntruders, [S]pyware, [T]rojans and [A]dware. :-)
On Thu, Nov 27, 2008 at 6:43 PM, Herton Ronaldo Krzesinski
<[email protected]> wrote:
>
> nack.
>
> I've seen other drivers set just a flag when hardware has a bit field to
> signalize that there isn't need to ack the tx packet to be sent, but
> don't saw anyone changing retry in tx hw header. Also setting to 0 in
> no_ack case looks wrong, for example see ath5k where it even checks for
> 0 and returns an error if you try to do this.
>
> --
> []'s
> Herton
>
IMO the current implementation is even worse - completely disregard
NO_ACK, and when a packet that should not be ACKed according to IEEE
802.11:2007 is sent (such as a broadcast), retry repeatedly until
timeout.
Also, v26.1010 of the vendor driver does exactly what my patch does.
--
Vista: [V]iruses, [I]ntruders, [S]pyware, [T]rojans and [A]dware. :-)
On Wed, Nov 26, 2008 at 10:31 PM, Stefanik G=E1bor
<[email protected]> wrote:
> (Patch also available as an attachment in case Gmail decides to linew=
rap it.)
>
> Currently we always wait for an ACK even if IEEE80211_TX_CTL_NO_ACK i=
s set.
> This is just the opposite of what other drivers do.
> Change both RTL8187L and RTL8187B to skip waiting for an ACK when no
> ACK was requested.
>
> Signed-off-by: G=E1bor Stefanik <[email protected]>
Tested-by: Hin-Tak Leung <[email protected]>
I have it running and it doesn't seem to do anything bad, but there is
a curious issue - normally when I swap over
a new version of of the kernel module, wpa_supplicant/NetworkManager
kicks in soon afterwards; but this
(or latest wireless-testing - I haven't tried unpatched - I was
swapping from 2.6.27.7) seems to get ignored by wpa_supplicant
and requires a manual selection. Is there any possibility that this
might affect wpa_supplicant/NetworkManager for scanning for available
AP and/or associate/re-associate?
Hin-Tak
On Thu, Nov 27, 2008 at 9:52 PM, Herton Ronaldo Krzesinski
<[email protected]> wrote:
<snipped>
>> Also, v26.1010 of the vendor driver does exactly what my patch does.
>
> Is this newer than 26.1036.0708.2008 I have here? The version it never sets 0,
> in code there is this:
<snipped>
> I didn't found this version 26.1010 to download, do you know where I can get?
26.1010 is the non-B driver - I happened to have it on my hard disc -
the url is
ftp://152.104.238.19/cn/wlan/rtl8187_linux_26.1010.zip .
it is quite old - it unpacks to rtl8187_linux_26.1010.0622.2006 -
which as far as my understanding of the naming goes,
is for 2.6 kernel, release 1010, on 2006 June 22nd, so it is a lot
older than every B vendor driver we have.
Hin-Tak
Em Quinta 27 Novembro 2008, =E0s 18:02:22, Stefanik G=E1bor escreveu:
> On Thu, Nov 27, 2008 at 6:43 PM, Herton Ronaldo Krzesinski
>
> <[email protected]> wrote:
> > nack.
> >
> > I've seen other drivers set just a flag when hardware has a bit fie=
ld to
> > signalize that there isn't need to ack the tx packet to be sent, bu=
t
> > don't saw anyone changing retry in tx hw header. Also setting to 0 =
in
> > no_ack case looks wrong, for example see ath5k where it even checks=
for
> > 0 and returns an error if you try to do this.
> >
> > --
> > []'s
> > Herton
>
> IMO the current implementation is even worse - completely disregard
> NO_ACK, and when a packet that should not be ACKed according to IEEE
> 802.11:2007 is sent (such as a broadcast), retry repeatedly until
> timeout.
In this case then shouldn't mac80211/rate control alg give a right rate=
s[0].count?
And other drivers have same bug?
But I agree that we should set to 0 if to the hardware it means that 0 =
is the
same meaning to the no ack bitfield on others; only there is no referen=
ce/docs
mentioning this.
> Also, v26.1010 of the vendor driver does exactly what my patch does.
Is this newer than 26.1036.0708.2008 I have here? The version it never =
sets 0,
in code there is this:
if( WLAN_FC_GET_STYPE(le16_to_cpu(frag_hdr->frame_ctl))=
=3D=3D IEEE80211_STYPE_PROBE_RESP )
tx[5] |=3D (1<<8);//(priv->retry_data<<8); //re=
try lim ;
else
tx[5] |=3D (11<<8);//(priv->retry_data<<8); //r=
etry lim ;
I didn't found this version 26.1010 to download, do you know where I ca=
n get?
--
[]'s
Herton
On Thursday 27 November 2008 20:39:34 Stefanik G=E1bor wrote:
> On Thu, Nov 27, 2008 at 10:59 PM, Johannes Berg
>
> <[email protected]> wrote:
> > On Thu, 2008-11-27 at 19:52 -0200, Herton Ronaldo Krzesinski wrote:
> >> In this case then shouldn't mac80211/rate control alg give a right
> >> rates[0].count?
> >
> > It probably should put 1 into that value, yes. It doesn't seem to
> > enforce that now though, I'll take a look at doing that.
> >
> >> And other drivers have same bug?
> >
> > possibly.
> >
> > Another thing: the ".count" is the number of tries you should do, s=
o if
> > the hardware expects retries then you need to subtract 1.
Ops sorry, then I did a mistake in a previous patch reverting a subtrac=
t in=20
rtl8187_tx. This will fix the code, will submit a patch with proper cha=
ngelog:
diff --git a/drivers/net/wireless/rtl818x/rtl8187_dev.c b/drivers/net/w=
ireless/rtl818x/rtl8187_dev.c
index ceebeb7..41444e2 100644
--- a/drivers/net/wireless/rtl818x/rtl8187_dev.c
+++ b/drivers/net/wireless/rtl818x/rtl8187_dev.c
@@ -238,7 +238,7 @@ static int rtl8187_tx(struct ieee80211_hw *dev, str=
uct sk_buff *skb)
hdr->flags =3D cpu_to_le32(flags);
hdr->len =3D 0;
hdr->rts_duration =3D rts_dur;
- hdr->retry =3D cpu_to_le32(info->control.rates[0].count << 8);
+ hdr->retry =3D cpu_to_le32((info->control.rates[0].count - 1) << 8);
buf =3D hdr;
=20
ep =3D 2;
@@ -256,7 +256,7 @@ static int rtl8187_tx(struct ieee80211_hw *dev, str=
uct sk_buff *skb)
memset(hdr, 0, sizeof(*hdr));
hdr->flags =3D cpu_to_le32(flags);
hdr->rts_duration =3D rts_dur;
- hdr->retry =3D cpu_to_le32(info->control.rates[0].count << 8);
+ hdr->retry =3D cpu_to_le32((info->control.rates[0].count - 1) << 8);
hdr->tx_duration =3D
ieee80211_generic_frame_duration(dev, priv->vif,
skb->len, txrate);
> >
> > johannes
>
> I actually tested that 0 is the right setting - I initially tested it
> with 1, and there was evidence that it was still retrying. Setting it
> to 0 truly behaved like a NO_ACK bit would on other HW. (I used
> aireplay-ng to get a view of transmission speed - the "chopchop" and
> "interactive replay" are good indicators. I used a patch to make all
> injected frames NO_ACK, then tested the highest rate that results in
> no multiple tries for the same number in chopchop. With the original
> code, I could transmit 25 frames per second - anything higher resulte=
d
> in chopchop requiring more than 256 packets for each byte. Most
> drivers start to behave this way over about 600 to 700 packets per
> second. Setting the retry count to 1 resulted in a max of 220/s befor=
e
> producing more-than-256-attempts errors, still lower than what other
> drivers do with NO_ACK, suggestive of the driver still retrying.
> Setting to 0 resulted in a 625/s max, on par with other drivers.)
> This means, hdr->retry is the number of retries, unlike ".count",
> which is the number of tries, including the initial transmit.
yep, you're right, I wrongly considered the retry field, that also made=
me
introduce a bug that diff above fixes. Your patch was good, could be ap=
plied
while count is not enforced by mac80211.
--
[]'s
Herton
On Thu, 2008-11-27 at 19:52 -0200, Herton Ronaldo Krzesinski wrote:
> In this case then shouldn't mac80211/rate control alg give a right rates[0].count?
It probably should put 1 into that value, yes. It doesn't seem to
enforce that now though, I'll take a look at doing that.
> And other drivers have same bug?
possibly.
Another thing: the ".count" is the number of tries you should do, so if
the hardware expects retries then you need to subtract 1.
johannes
Em Quarta 26 Novembro 2008, =C3=A0s 20:31:03, Stefanik G=C3=A1bor escre=
veu:
> (Patch also available as an attachment in case Gmail decides to linew=
rap
> it.)
>
> Currently we always wait for an ACK even if IEEE80211_TX_CTL_NO_ACK i=
s set.
> This is just the opposite of what other drivers do.
nack.
I've seen other drivers set just a flag when hardware has a bit field t=
o
signalize that there isn't need to ack the tx packet to be sent, but
don't saw anyone changing retry in tx hw header. Also setting to 0 in
no_ack case looks wrong, for example see ath5k where it even checks for
0 and returns an error if you try to do this.
> Change both RTL8187L and RTL8187B to skip waiting for an ACK when no
> ACK was requested.
>
> Signed-off-by: G=C3=A1bor Stefanik <[email protected]>
> ---
> drivers/net/wireless/rtl818x/rtl8187_dev.c | 10 ++++++++--
> 1 files changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/wireless/rtl818x/rtl8187_dev.c
> b/drivers/net/wireless/rtl818x/rtl8187_dev.c
> index 9fd23b5..7f12d9f 100644
> --- a/drivers/net/wireless/rtl818x/rtl8187_dev.c
> +++ b/drivers/net/wireless/rtl818x/rtl8187_dev.c
> @@ -230,7 +230,10 @@ static int rtl8187_tx(struct ieee80211_hw *dev,
> struct sk_buff *skb)
> hdr->flags =3D cpu_to_le32(flags);
> hdr->len =3D 0;
> hdr->rts_duration =3D rts_dur;
> - hdr->retry =3D cpu_to_le32(info->control.rates[0].count << 8);
> + if (!(info->flags & IEEE80211_TX_CTL_NO_ACK))
> + hdr->retry =3D cpu_to_le32(info->control.rates[0].count << 8);
> + else
> + hdr->retry =3D 0;
> buf =3D hdr;
>
> ep =3D 2;
> @@ -248,7 +251,10 @@ static int rtl8187_tx(struct ieee80211_hw *dev,
> struct sk_buff *skb)
> memset(hdr, 0, sizeof(*hdr));
> hdr->flags =3D cpu_to_le32(flags);
> hdr->rts_duration =3D rts_dur;
> - hdr->retry =3D cpu_to_le32(info->control.rates[0].count << 8);
> + if (!(info->flags & IEEE80211_TX_CTL_NO_ACK))
> + hdr->retry =3D cpu_to_le32(info->control.rates[0].count << 8);
> + else
> + hdr->retry =3D 0;
> hdr->tx_duration =3D
> ieee80211_generic_frame_duration(dev, priv->vif,
> skb->len, txrate);
--
[]'s
Herton