[RFC/RFT] rtl8187: Improve wireless statistics for RTL8187B

[RFC/RFT] rtl8187: Improve wireless statistics for RTL8187B

[Larry Finger]

Wireless statistics produced by the RTL8187B driver are not particularly
informative about the strength of the received signal. From the data sheet
provided by Realtek, I discovered that certain parts of the RX header
should have the information necessary to calculate signal quality and
strength. With testing, it became clear that most of these quantities were
very jittery - only the AGC correlated with the signals expected from nearby
AP's. As a result, the quality and strength are derived from the agc value.
The scaling has been determined so that the numbers are close to those
obtained by b43 under the same conditions. The results are qualitatively
correct.

Statistics derived for the RTL8187 have not been changed.

The RX header variables have been renamed to match the quantites described
in the Realtek data sheet.

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

Pavel,

Please check that this patch does not break the RTL8187 statistics. Perhaps,
similar changes should be made there, but I don't have the hardware.

Larry


Index: wireless-testing/drivers/net/wireless/rtl8187.h
===================================================================
--- wireless-testing.orig/drivers/net/wireless/rtl8187.h
+++ wireless-testing/drivers/net/wireless/rtl8187.h
@@ -47,11 +47,13 @@ struct rtl8187_rx_hdr {
 struct rtl8187b_rx_hdr {
 	__le32 flags;
 	__le64 mac_time;
-	u8 noise;
-	u8 signal;
+	u8 sq;
+	u8 rssi;
 	u8 agc;
-	u8 reserved;
-	__le32 unused;
+	u8 flags2;
+	__le16 snr_long2end;
+	s8 pwdb_g12;
+	u8 fot;
 } __attribute__((packed));
 
 /* {rtl8187,rtl8187b}_tx_info is in skb */
Index: wireless-testing/drivers/net/wireless/rtl8187_dev.c
===================================================================
--- wireless-testing.orig/drivers/net/wireless/rtl8187_dev.c
+++ wireless-testing/drivers/net/wireless/rtl8187_dev.c
@@ -257,6 +257,7 @@ static void rtl8187_rx_cb(struct urb *ur
 	struct ieee80211_rx_status rx_status = { 0 };
 	int rate, signal;
 	u32 flags;
+	u32 quality;
 
 	spin_lock(&priv->rx_queue.lock);
 	if (skb->next)
@@ -280,44 +281,57 @@ static void rtl8187_rx_cb(struct urb *ur
 		flags = le32_to_cpu(hdr->flags);
 		signal = hdr->signal & 0x7f;
 		rx_status.antenna = (hdr->signal >> 7) & 1;
-		rx_status.signal = signal;
 		rx_status.noise = hdr->noise;
 		rx_status.mactime = le64_to_cpu(hdr->mac_time);
-		priv->signal = signal;
 		priv->quality = signal;
+		rx_status.qual = priv->quality;
 		priv->noise = hdr->noise;
+		rate = (flags >> 20) & 0xF;
+		if (rate > 3) {	/* OFDM rate */
+			if (signal > 90)
+				signal = 90;
+			else if (signal < 25)
+				signal = 25;
+			signal = 90 - signal;
+		} else {	/* CCK rate */
+			if (signal > 95)
+				signal = 95;
+			else if (signal < 30)
+				signal = 30;
+			signal = 95 - signal;
+		}
+		rx_status.signal = signal;
+		priv->signal = signal;
 	} else {
 		struct rtl8187b_rx_hdr *hdr =
 			(typeof(hdr))(skb_tail_pointer(skb) - sizeof(*hdr));
+		/* The Realtek datasheet for the RTL8187B shows that the RX
+		 * header contains the following quantities: signal quality,
+		 * RSSI, AGC, the received power in dB, and the measured SNR.
+		 * In testing, none of these quantities show qualitative
+		 * agreement with AP signal strength, except for the AGC,
+		 * which is inversely proportional to the strength of the
+		 * signal. In the following, the quality and signal strength
+		 * are derived from the AGC. The arbitrary scaling constants
+		 * are chosen to make the results close to the values obtained
+		 * for a BCM4312 using b43 as the driver. The noise is ignored
+		 * for now.
+		 */
 		flags = le32_to_cpu(hdr->flags);
-		signal = hdr->agc >> 1;
-		rx_status.antenna = (hdr->signal >> 7) & 1;
-		rx_status.signal = 64 - min(hdr->noise, (u8)64);
-		rx_status.noise = hdr->noise;
+		quality = 170 - hdr->agc;
+		if (quality > 100)
+			quality = 100;
+		signal = 14 - hdr->agc / 2;
+		rx_status.qual = quality;
+		priv->quality = quality;
+		rx_status.signal = signal;
+		priv->signal = signal;
+		rx_status.antenna = (hdr->rssi >> 7) & 1;
 		rx_status.mactime = le64_to_cpu(hdr->mac_time);
-		priv->signal = hdr->signal;
-		priv->quality = hdr->agc >> 1;
-		priv->noise = hdr->noise;
+		rate = (flags >> 20) & 0xF;
 	}
 
 	skb_trim(skb, flags & 0x0FFF);
-	rate = (flags >> 20) & 0xF;
-	if (rate > 3) {	/* OFDM rate */
-		if (signal > 90)
-			signal = 90;
-		else if (signal < 25)
-			signal = 25;
-		signal = 90 - signal;
-	} else {	/* CCK rate */
-		if (signal > 95)
-			signal = 95;
-		else if (signal < 30)
-			signal = 30;
-		signal = 95 - signal;
-	}
-
-	rx_status.qual = priv->quality;
-	rx_status.signal = signal;
 	rx_status.rate_idx = rate;
 	rx_status.freq = dev->conf.channel->center_freq;
 	rx_status.band = dev->conf.channel->band;
@@ -1015,9 +1029,7 @@ static int __devinit rtl8187_probe(struc
 
 	priv->mode = IEEE80211_IF_TYPE_MNTR;
 	dev->flags = IEEE80211_HW_HOST_BROADCAST_PS_BUFFERING |
-		     IEEE80211_HW_RX_INCLUDES_FCS |
-		     IEEE80211_HW_SIGNAL_UNSPEC;
-	dev->max_signal = 65;
+		     IEEE80211_HW_RX_INCLUDES_FCS;
 
 	eeprom.data = dev;
 	eeprom.register_read = rtl8187_eeprom_register_read;
@@ -1132,10 +1144,16 @@ static int __devinit rtl8187_probe(struc
 		(*channel++).hw_value = txpwr >> 8;
 	}
 
-	if (priv->is_rtl8187b)
+	if (priv->is_rtl8187b) {
 		printk(KERN_WARNING "rtl8187: 8187B chip detected. Support "
 			"is EXPERIMENTAL, and could damage your\n"
 			"         hardware, use at your own risk\n");
+		dev->flags |= IEEE80211_HW_SIGNAL_DBM;
+	} else {
+		dev->flags |= IEEE80211_HW_SIGNAL_UNSPEC;
+		dev->max_signal = 65;
+	}
+
 	if ((id->driver_info == DEVICE_RTL8187) && priv->is_rtl8187b)
 		printk(KERN_INFO "rtl8187: inconsistency between id with OEM"
 		       " info!\n");

Re: [RFC/RFT] rtl8187: Improve wireless statistics for RTL8187B

[Herton Ronaldo Krzesinski]

Em Thursday 17 July 2008 12:02:56 Larry Finger escreveu:
> Wireless statistics produced by the RTL8187B driver are not particularly
> informative about the strength of the received signal. From the data sheet
> provided by Realtek, I discovered that certain parts of the RX header
> should have the information necessary to calculate signal quality and
> strength. With testing, it became clear that most of these quantities were
> very jittery - only the AGC correlated with the signals expected from
> nearby AP's. As a result, the quality and strength are derived from the agc
> value. The scaling has been determined so that the numbers are close to
> those obtained by b43 under the same conditions. The results are
> qualitatively correct.
>
> Statistics derived for the RTL8187 have not been changed.
>
> The RX header variables have been renamed to match the quantites described
> in the Realtek data sheet.

Hi,

I did some tests here and statistics are better now. I only couldn't use 
latest wireless-testing because something of latest changes broke 
communication, I couldn't scan/associate anymore, but using wireless-testing 
head at d61f20889de4978ed87d2a37d6f27147faec22d1 plus later anaparam*off 
change, with your patch I can confirm the better statistics, later I'll try 
to find what in wireless-testing broke here, I didn't have time to look into 
this today.

>
> Signed-off-by: Larry Finger <Larry.Finger@lwfinger.net>
> ---
>
> Pavel,
>
> Please check that this patch does not break the RTL8187 statistics.
> Perhaps, similar changes should be made there, but I don't have the
> hardware.
>
> Larry

-- 
[]'s
Herton

Re: [RFC/RFT] rtl8187: Improve wireless statistics for RTL8187B

[Larry Finger]

Herton Ronaldo Krzesinski wrote:
> 
> I did some tests here and statistics are better now. I only couldn't use 
> latest wireless-testing because something of latest changes broke 
> communication, I couldn't scan/associate anymore, but using wireless-testing 
> head at d61f20889de4978ed87d2a37d6f27147faec22d1 plus later anaparam*off 
> change, with your patch I can confirm the better statistics, later I'll try 
> to find what in wireless-testing broke here, I didn't have time to look into 
> this today.

What platform are you using? I'm on x86_64 and the latest one works for me. At 
least you won't have that many commits to bisect.

Larry

Re: [RFC/RFT] rtl8187: Improve wireless statistics for RTL8187B

[Hin-Tak Leung]

On Sat, Jul 19, 2008 at 5:14 AM, Larry Finger <Larry.Finger@lwfinger.net> wrote:
> Herton Ronaldo Krzesinski wrote:
>>
>> I did some tests here and statistics are better now. I only couldn't use
>> latest wireless-testing because something of latest changes broke
>> communication, I couldn't scan/associate anymore, but using wireless-testing
>> head at d61f20889de4978ed87d2a37d6f27147faec22d1 plus later anaparam*off
>> change, with your patch I can confirm the better statistics, later I'll try
>> to find what in wireless-testing broke here, I didn't have time to look into
>> this today.
>
> What platform are you using? I'm on x86_64 and the latest one works for me.
> At least you won't have that many commits to bisect.

I am on x86_64 and I am on a similar situation - I am using the
rawhide kernels from:
http://koji.fedoraproject.org/koji/packageinfo?packageID=8
and the last one that worked for me is kernel-2.6.25.11-92.fc9 - something in
kernel-2.6.25.11-93.fc9 broke connectivity. Apparently the difference
is this set of changes:
http://marc.info/?l=linux-wireless&m=121606436000705&w=2

I just don't feel like mentioning it earlier because I had mistakenly
complained about Herton's
last patch and I don't want to make the same mistake - and I did test
it. Beside Herton's
 ANAPARAM*_OFF patch (which I don't think is the source of the
problem), there is
only one other "rtl8187: Fixed section mismatch in rtl8187_dev.c"
change - but that's is also
obviously right, and besides, its nature shouldn't affect association
anyway; but there is
a lot of mac80211 changes elsewhere, and I wonder.

My symptom is that I can authenticate and associate (apparently,
according to dmesg)
but cannot get an ip address via dhcp. I have tried and just alternate
between the two kernels,
identical steps, etc and one works and the other doesn't.

Re: [RFC/RFT] rtl8187: Improve wireless statistics for RTL8187B

[Larry Finger]

Hin-Tak Leung wrote:
> 
> I am on x86_64 and I am on a similar situation - I am using the
> rawhide kernels from:
> http://koji.fedoraproject.org/koji/packageinfo?packageID=8
> and the last one that worked for me is kernel-2.6.25.11-92.fc9 - something in
> kernel-2.6.25.11-93.fc9 broke connectivity. Apparently the difference
> is this set of changes:
> http://marc.info/?l=linux-wireless&m=121606436000705&w=2
> 
> I just don't feel like mentioning it earlier because I had mistakenly
> complained about Herton's
> last patch and I don't want to make the same mistake - and I did test
> it. Beside Herton's
>  ANAPARAM*_OFF patch (which I don't think is the source of the
> problem), there is
> only one other "rtl8187: Fixed section mismatch in rtl8187_dev.c"
> change - but that's is also
> obviously right, and besides, its nature shouldn't affect association
> anyway; but there is
> a lot of mac80211 changes elsewhere, and I wonder.
> 
> My symptom is that I can authenticate and associate (apparently,
> according to dmesg)
> but cannot get an ip address via dhcp. I have tried and just alternate
> between the two kernels,
> identical steps, etc and one works and the other doesn't.
> 

Curious. According to the git log, my copy of the wireless-testing tree has all 
those patches installed. There must be some really obscure configuration 
difference, or the rawhide kernel has some unexpected difference with the wl 
kernels.

Larry

Re: [RFC/RFT] rtl8187: Improve wireless statistics for RTL8187B

[Herton Ronaldo Krzesinski]

Em Saturday 19 July 2008 23:55:12 Larry Finger escreveu:
> Hin-Tak Leung wrote:
> > I am on x86_64 and I am on a similar situation - I am using the
> > rawhide kernels from:
> > http://koji.fedoraproject.org/koji/packageinfo?packageID=8
> > and the last one that worked for me is kernel-2.6.25.11-92.fc9 -
> > something in kernel-2.6.25.11-93.fc9 broke connectivity. Apparently the
> > difference is this set of changes:
> > http://marc.info/?l=linux-wireless&m=121606436000705&w=2
> >
> > I just don't feel like mentioning it earlier because I had mistakenly
> > complained about Herton's
> > last patch and I don't want to make the same mistake - and I did test
> > it. Beside Herton's
> >  ANAPARAM*_OFF patch (which I don't think is the source of the
> > problem), there is
> > only one other "rtl8187: Fixed section mismatch in rtl8187_dev.c"
> > change - but that's is also
> > obviously right, and besides, its nature shouldn't affect association
> > anyway; but there is
> > a lot of mac80211 changes elsewhere, and I wonder.
> >
> > My symptom is that I can authenticate and associate (apparently,
> > according to dmesg)
> > but cannot get an ip address via dhcp. I have tried and just alternate
> > between the two kernels,
> > identical steps, etc and one works and the other doesn't.
>
> Curious. According to the git log, my copy of the wireless-testing tree has
> all those patches installed. There must be some really obscure
> configuration difference, or the rawhide kernel has some unexpected
> difference with the wl kernels.

I got some time to take at a look at it again, I did the bisect and found 
commit 8f87dd7e540d455f8e7f11478133b85edc969c67 ("mac80211: power management 
wext hooks") to cause the issue here (non working interface). But with it, 
making "iwconfig wlan* power off" and making interface go up again, it starts 
to work. May be something related to the power management needs to be 
implemented in driver now? or keep it disabled with rtl8187...

>
> Larry
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-wireless"
> in the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

--
[]'s
Herton

Re: [RFC/RFT] rtl8187: Improve wireless statistics for RTL8187B

[Hin-Tak Leung]

On Tue, Jul 22, 2008 at 8:13 PM, Herton Ronaldo Krzesinski
<herton@mandriva.com.br> wrote:
<snipped>
> I got some time to take at a look at it again, I did the bisect and found
> commit 8f87dd7e540d455f8e7f11478133b85edc969c67 ("mac80211: power management
> wext hooks") to cause the issue here (non working interface). But with it,
> making "iwconfig wlan* power off" and making interface go up again, it starts
> to work. May be something related to the power management needs to be
> implemented in driver now? or keep it disabled with rtl8187...

Disabling power management doesn't improve the situation with latest
wireless testing
for me. I had a look at the change and it seems simple enough.

The latest vendor driver has a lot of suspend/resume code in it. I
have had a go at
trying to get it running against a current kernel. modprobe works but
ifconfig up results in a lot

rtl8187: EE:cannot submit RX command. URB_STATUS 0

Hin-Tak

Re: [RFC/RFT] rtl8187: Improve wireless statistics for RTL8187B

[Herton Ronaldo Krzesinski]

Em Tuesday 22 July 2008 19:25:20 Hin-Tak Leung escreveu:
> On Tue, Jul 22, 2008 at 8:13 PM, Herton Ronaldo Krzesinski
> <herton@mandriva.com.br> wrote:
> <snipped>
>
> > I got some time to take at a look at it again, I did the bisect and found
> > commit 8f87dd7e540d455f8e7f11478133b85edc969c67 ("mac80211: power
> > management wext hooks") to cause the issue here (non working interface).
> > But with it, making "iwconfig wlan* power off" and making interface go up
> > again, it starts to work. May be something related to the power
> > management needs to be implemented in driver now? or keep it disabled
> > with rtl8187...
>
> Disabling power management doesn't improve the situation with latest
> wireless testing
> for me. I had a look at the change and it seems simple enough.

Yes, I also looked at the commit, it's simple and I can't see how it's 
affecting transmission in my case. It's bizarre and I must be doing something 
wrong.

>
> The latest vendor driver has a lot of suspend/resume code in it. I
> have had a go at
> trying to get it running against a current kernel. modprobe works but
> ifconfig up results in a lot
>
> rtl8187: EE:cannot submit RX command. URB_STATUS 0
>
> Hin-Tak

-- 
[]'s
Herton

Re: [RFC/RFT] rtl8187: Improve wireless statistics for RTL8187B

[Larry Finger]

Herton Ronaldo Krzesinski wrote:
> Em Tuesday 22 July 2008 19:25:20 Hin-Tak Leung escreveu:
>> Disabling power management doesn't improve the situation with latest
>> wireless testing
>> for me. I had a look at the change and it seems simple enough.
> 
> Yes, I also looked at the commit, it's simple and I can't see how it's 
> affecting transmission in my case. It's bizarre and I must be doing something 
> wrong.

I just found that my RTL8187B doesn't work with the current wireless-testing 
either. I don't know why I thought otherwise earlier. If I do a 'git checkout 
d61f20889de4978ed8', which is my patch to change the detection of early 8187B, 
then it works fine. I'll do a bisection to see if my system agrees that the wext 
power patch is the problem.

I'll let you know what I find.

Larry

Commit 741b4fbc44 (mac80211: fix TX sequence numbers) breaks rtl8187

[Larry Finger]

Johannes,

In the current wireless-testing (v2.6.26-rc9-14207-ga520bdb), rtl8187 is broken. 
In most cases, it is unable to complete a connection. Occasionally, it will 
authenticate and even get an IP using DHCP; however, only a few pings get 
through to the AP before the connection fails completely.

Using bisection, this problem was traced to:

==================================================
commit 741b4fbc441dba885cc8f97a10e87f2acd04c5f2
Author: Johannes Berg <johannes@sipsolutions.net>
Date:   Thu Jul 10 11:21:26 2008 +0200

     mac80211: fix TX sequence numbers

     This patch makes mac80211 assign proper sequence numbers to
     QoS-data frames. It also removes the old sequence number code
     because we noticed that only the driver or hardware can assign
     sequence numbers to non-QoS-data and especially management
     frames in a race-free manner because beacons aren't passed
     through mac80211's TX path.
===============================================================

The validity of the bisection was confirmed by reverse application of the patch. 
The resulting driver worked.

The appears that rtl8187 needs some kind of modification such as b43, b43legacy, 
and rt2x00 got in the original patch. I'll be studying the changes to the other 
drivers to see if I can find the problem, but I will appreciate your help when 
you get a chance. Perhaps after your trip to Canada is over.

Thanks,

Larry

Re: Commit 741b4fbc44 (mac80211: fix TX sequence numbers) breaks rtl8187

[Stefanik Gábor]

On Wed, Jul 23, 2008 at 7:43 AM, Larry Finger <Larry.Finger@lwfinger.net> wrote:
> Johannes,
>
> In the current wireless-testing (v2.6.26-rc9-14207-ga520bdb), rtl8187 is
> broken. In most cases, it is unable to complete a connection. Occasionally,
> it will authenticate and even get an IP using DHCP; however, only a few
> pings get through to the AP before the connection fails completely.
>
> Using bisection, this problem was traced to:
>
> ==================================================
> commit 741b4fbc441dba885cc8f97a10e87f2acd04c5f2
> Author: Johannes Berg <johannes@sipsolutions.net>
> Date:   Thu Jul 10 11:21:26 2008 +0200
>
>    mac80211: fix TX sequence numbers
>
>    This patch makes mac80211 assign proper sequence numbers to
>    QoS-data frames. It also removes the old sequence number code
>    because we noticed that only the driver or hardware can assign
>    sequence numbers to non-QoS-data and especially management
>    frames in a race-free manner because beacons aren't passed
>    through mac80211's TX path.
> ===============================================================
>
> The validity of the bisection was confirmed by reverse application of the
> patch. The resulting driver worked.
>
> The appears that rtl8187 needs some kind of modification such as b43,
> b43legacy, and rt2x00 got in the original patch. I'll be studying the
> changes to the other drivers to see if I can find the problem, but I will
> appreciate your help when you get a chance. Perhaps after your trip to
> Canada is over.
>
> Thanks,
>
> Larry
> --
> To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Zd1211rw also appears to be affected, though not as seriously as with
rtl8187: it scans, associates and gets DHCP, but it consistently
disconnects after about 2~5 minutes, either with "No ProbeResp" in
dmesg, or sometimes with no error message, just "Removed STA" and
"Destroyed STA". I don't know if this is the same bug, as I haven't
bisected, but it appeared when I switched from a 2.6.26-rc8-wl kernel
to a 2.6.26-wl one that contained the above patch.

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

Re: Commit 741b4fbc44 (mac80211: fix TX sequence numbers) breaks rtl8187

[Johannes Berg]

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

On Wed, 2008-07-23 at 14:55 +0200, Stefanik Gábor wrote:

> Zd1211rw also appears to be affected, though not as seriously as with
> rtl8187: it scans, associates and gets DHCP, but it consistently
> disconnects after about 2~5 minutes, either with "No ProbeResp" in
> dmesg, or sometimes with no error message, just "Removed STA" and
> "Destroyed STA". I don't know if this is the same bug, as I haven't
> bisected, but it appeared when I switched from a 2.6.26-rc8-wl kernel
> to a 2.6.26-wl one that contained the above patch.

zd1211rw works fine here, been connected for about 15 minutes now.

johannes

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

Re: Commit 741b4fbc44 (mac80211: fix TX sequence numbers) breaks rtl8187

[Johannes Berg]

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

On Wed, 2008-07-23 at 00:43 -0500, Larry Finger wrote:
> Johannes,
> 
> In the current wireless-testing (v2.6.26-rc9-14207-ga520bdb), rtl8187 is broken. 
> In most cases, it is unable to complete a connection. Occasionally, it will 
> authenticate and even get an IP using DHCP; however, only a few pings get 
> through to the AP before the connection fails completely.
> 
> Using bisection, this problem was traced to:
> 
> ==================================================
> commit 741b4fbc441dba885cc8f97a10e87f2acd04c5f2
> Author: Johannes Berg <johannes@sipsolutions.net>
> Date:   Thu Jul 10 11:21:26 2008 +0200
> 
>      mac80211: fix TX sequence numbers

> The validity of the bisection was confirmed by reverse application of the patch. 
> The resulting driver worked.
> 
> The appears that rtl8187 needs some kind of modification such as b43, b43legacy, 
> and rt2x00 got in the original patch. I'll be studying the changes to the other 
> drivers to see if I can find the problem, but I will appreciate your help when 
> you get a chance. Perhaps after your trip to Canada is over.

Oh, crap. If you look at the patch, it removes the sequence numbers for
non-TID frames, and puts them back into rt2x00 because I thought rt2x00
was the only driver that needed software sequence numbering. It looks
like rtl8187 and zd1211 might need that too, so we need to copy the
rt2x00 code that I put into that particular patch to those drivers.

johannes

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

Re: Commit 741b4fbc44 (mac80211: fix TX sequence numbers) breaks rtl8187

[Larry Finger]

Johannes Berg wrote:
> On Wed, 2008-07-23 at 00:43 -0500, Larry Finger wrote:
>> Johannes,
>>
>> In the current wireless-testing (v2.6.26-rc9-14207-ga520bdb), rtl8187 is broken. 
>> In most cases, it is unable to complete a connection. Occasionally, it will 
>> authenticate and even get an IP using DHCP; however, only a few pings get 
>> through to the AP before the connection fails completely.
>>
>> Using bisection, this problem was traced to:
>>
>> ==================================================
>> commit 741b4fbc441dba885cc8f97a10e87f2acd04c5f2
>> Author: Johannes Berg <johannes@sipsolutions.net>
>> Date:   Thu Jul 10 11:21:26 2008 +0200
>>
>>      mac80211: fix TX sequence numbers
> 
>> The validity of the bisection was confirmed by reverse application of the patch. 
>> The resulting driver worked.
>>
>> The appears that rtl8187 needs some kind of modification such as b43, b43legacy, 
>> and rt2x00 got in the original patch. I'll be studying the changes to the other 
>> drivers to see if I can find the problem, but I will appreciate your help when 
>> you get a chance. Perhaps after your trip to Canada is over.
> 
> Oh, crap. If you look at the patch, it removes the sequence numbers for
> non-TID frames, and puts them back into rt2x00 because I thought rt2x00
> was the only driver that needed software sequence numbering. It looks
> like rtl8187 and zd1211 might need that too, so we need to copy the
> rt2x00 code that I put into that particular patch to those drivers.

I tried something like that, but it didn't work. Maybe you can see what is wrong 
here:


Index: wireless-testing/drivers/net/wireless/rtl8187.h
===================================================================
--- wireless-testing.orig/drivers/net/wireless/rtl8187.h
+++ wireless-testing/drivers/net/wireless/rtl8187.h
@@ -100,6 +100,7 @@ struct rtl8187_priv {
  	struct usb_device *udev;
  	u32 rx_conf;
  	u16 txpwr_base;
+	u16 seqno;
  	u8 asic_rev;
  	u8 is_rtl8187b;
  	enum {
Index: wireless-testing/drivers/net/wireless/rtl8187_dev.c
===================================================================
--- wireless-testing.orig/drivers/net/wireless/rtl8187_dev.c
+++ wireless-testing/drivers/net/wireless/rtl8187_dev.c
@@ -169,6 +169,7 @@ static int rtl8187_tx(struct ieee80211_h
  {
  	struct rtl8187_priv *priv = dev->priv;
  	struct ieee80211_tx_info *info = IEEE80211_SKB_CB(skb);
+	struct ieee80211_hdr *ieee80211hdr = (struct ieee80211_hdr *)skb->data;
  	unsigned int ep;
  	void *buf;
  	struct urb *urb;
@@ -198,6 +199,14 @@ static int rtl8187_tx(struct ieee80211_h
  		flags |= ieee80211_get_rts_cts_rate(dev, info)->hw_value << 19;
  	}

+	/* XXX: This sequence is an attempt to match what happens in r2x00. */
+	if (info->flags & IEEE80211_TX_CTL_ASSIGN_SEQ) {
+		if (info->flags & IEEE80211_TX_CTL_FIRST_FRAGMENT)
+			priv->seqno += 0x10;
+		ieee80211hdr->seq_ctrl &= cpu_to_le16(IEEE80211_SCTL_FRAG);
+		ieee80211hdr->seq_ctrl |= cpu_to_le16(priv->seqno);
+	}
+
  	if (!priv->is_rtl8187b) {
  		struct rtl8187_tx_hdr *hdr =
  			(struct rtl8187_tx_hdr *)skb_push(skb, sizeof(*hdr));

Re: Commit 741b4fbc44 (mac80211: fix TX sequence numbers) breaks rtl8187

[Johannes Berg]

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

On Wed, 2008-07-23 at 10:32 -0500, Larry Finger wrote:

> I tried something like that, but it didn't work. Maybe you can see what is wrong 
> here:
> 
> 
> Index: wireless-testing/drivers/net/wireless/rtl8187.h
> ===================================================================
> --- wireless-testing.orig/drivers/net/wireless/rtl8187.h
> +++ wireless-testing/drivers/net/wireless/rtl8187.h
> @@ -100,6 +100,7 @@ struct rtl8187_priv {
>   	struct usb_device *udev;
>   	u32 rx_conf;
>   	u16 txpwr_base;
> +	u16 seqno;
>   	u8 asic_rev;
>   	u8 is_rtl8187b;
>   	enum {
> Index: wireless-testing/drivers/net/wireless/rtl8187_dev.c
> ===================================================================
> --- wireless-testing.orig/drivers/net/wireless/rtl8187_dev.c
> +++ wireless-testing/drivers/net/wireless/rtl8187_dev.c
> @@ -169,6 +169,7 @@ static int rtl8187_tx(struct ieee80211_h
>   {
>   	struct rtl8187_priv *priv = dev->priv;
>   	struct ieee80211_tx_info *info = IEEE80211_SKB_CB(skb);
> +	struct ieee80211_hdr *ieee80211hdr = (struct ieee80211_hdr *)skb->data;
>   	unsigned int ep;
>   	void *buf;
>   	struct urb *urb;
> @@ -198,6 +199,14 @@ static int rtl8187_tx(struct ieee80211_h
>   		flags |= ieee80211_get_rts_cts_rate(dev, info)->hw_value << 19;
>   	}
> 
> +	/* XXX: This sequence is an attempt to match what happens in r2x00. */
> +	if (info->flags & IEEE80211_TX_CTL_ASSIGN_SEQ) {
> +		if (info->flags & IEEE80211_TX_CTL_FIRST_FRAGMENT)
> +			priv->seqno += 0x10;
> +		ieee80211hdr->seq_ctrl &= cpu_to_le16(IEEE80211_SCTL_FRAG);
> +		ieee80211hdr->seq_ctrl |= cpu_to_le16(priv->seqno);
> +	}
> +
>   	if (!priv->is_rtl8187b) {
>   		struct rtl8187_tx_hdr *hdr =
>   			(struct rtl8187_tx_hdr *)skb_push(skb, sizeof(*hdr));

Odd, that looks like it should work. Can you try printing the seqno and
see if it does increase? Or maybe I messed up something with the
flags...

johannes

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

Re: Commit 741b4fbc44 (mac80211: fix TX sequence numbers) breaks rtl8187

[Larry Finger]

Johannes Berg wrote:
> 
> Odd, that looks like it should work. Can you try printing the seqno and
> see if it does increase? Or maybe I messed up something with the
> flags...

It is increasing just as expected, but no connection.

Larry

Re: Commit 741b4fbc44 (mac80211: fix TX sequence numbers) breaks rtl8187

[Johannes Berg]

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

On Wed, 2008-07-23 at 13:03 -0500, Larry Finger wrote:
> Johannes Berg wrote:
> > 
> > Odd, that looks like it should work. Can you try printing the seqno and
> > see if it does increase? Or maybe I messed up something with the
> > flags...
> 
> It is increasing just as expected, but no connection.

Strange, don't understand that. Can you monitor with another card? I'll
see if John has an rtl8187 here.

johannes

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

Re: Commit 741b4fbc44 (mac80211: fix TX sequence numbers) breaks rtl8187

[Hin-Tak Leung]

On Wed, Jul 23, 2008 at 4:32 PM, Larry Finger <Larry.Finger@lwfinger.net> wrote:
<snipped>
>
> I tried something like that, but it didn't work. Maybe you can see what is
> wrong here:

Your patch actually works here! :-). Maybe you should retest?

It doesn't patch cleanly against wireless-testing - just indentation
oddness, I think.
Had to apply the patch by hand. but I am using it right now, so it
seems to work.

If somebody else retest okay, can we have this in wireless-testing
prompto, please?

Hin-Tak

Re: Commit 741b4fbc44 (mac80211: fix TX sequence numbers) breaks rtl8187

[Larry Finger]

Hin-Tak Leung wrote:
> 
> Your patch actually works here! :-). Maybe you should retest?
> 
> It doesn't patch cleanly against wireless-testing - just indentation
> oddness, I think.
> Had to apply the patch by hand. but I am using it right now, so it
> seems to work.

As I wasn't planning on it to be an official patch, I did not handle it with the
usual care. My mailer is Thunderbird, and it does mess with the white space in
patches.

> If somebody else retest okay, can we have this in wireless-testing
> prompto, please?

To retest, I did a 'git checkout 741b4fbc44' and then applied my patch just in
case there was a second breakage of the driver in the tree. It worked for a
while, then went offline, but then so did the case with 741b4fbc44 reverted. I 
think some of my problems may have been due to overheating. I thought we had 
that problem licked, but maybe not.

I will be submitting the proper patch tomorrow, particularly if it works for others.

Larry

Re: Commit 741b4fbc44 (mac80211: fix TX sequence numbers) breaks rtl8187

[Hin-Tak Leung]

On Thu, Jul 24, 2008 at 6:40 AM, Larry Finger <Larry.Finger@lwfinger.net> wrote:
<snipped>
> I will be submitting the proper patch tomorrow, particularly if it works for
> others.

I was being over-optimistic... it worked for me for a good while but
doesn't after a reboot/what-not.
So it probably needs to be done but there may be more missing.
Do we need to start the seq number from some where?

Hin-Tak

Re: Commit 741b4fbc44 (mac80211: fix TX sequence numbers) breaks rtl8187

[Johannes Berg]

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

On Thu, 2008-07-24 at 09:05 +0100, Hin-Tak Leung wrote:
> On Thu, Jul 24, 2008 at 6:40 AM, Larry Finger <Larry.Finger@lwfinger.net> wrote:
> <snipped>
> > I will be submitting the proper patch tomorrow, particularly if it works for
> > others.
> 
> I was being over-optimistic... it worked for me for a good while but
> doesn't after a reboot/what-not.
> So it probably needs to be done but there may be more missing.
> Do we need to start the seq number from some where?

You shouldn't have to, but if you ever want to support multiple virtual
interfaces you should put it into the vif state instead of the hw state.
Not that it should matter when you have just a single virtual interface.

johannes

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

Re: Commit 741b4fbc44 (mac80211: fix TX sequence numbers) breaks rtl8187

[Hin-Tak Leung]

On Thu, Jul 24, 2008 at 9:05 AM, Hin-Tak Leung <hintak.leung@gmail.com> wrote:
> On Thu, Jul 24, 2008 at 6:40 AM, Larry Finger <Larry.Finger@lwfinger.net> wrote:
> <snipped>
>> I will be submitting the proper patch tomorrow, particularly if it works for
>> others.
>
> I was being over-optimistic... it worked for me for a good while but
> doesn't after a reboot/what-not.
> So it probably needs to be done but there may be more missing.
> Do we need to start the seq number from some where?

I put the patch on top of
a520bdbe7d344296482f9355e29b0018ea58760f
(current wireless-testing master head), and did
make -C /lib/modules/`uname -r`/build M=`pwd` rtl8187.ko and replaced
the one shipped in kernel-2.6.25.12-100.fc9 (that should be essentially also
wireless testing head), depmod -a , and it works. Wierd. The as
shipped module has the problem
Larry described earlier - I could get an ip address, and leaving a ping running
it fails after about the 7th ping packets, quite soon.

So my problem seems to be largely due to the tx seq no change.
(maybe with some other issues thrown in occasionally).

Is adding the couple of lines likely to cause problems? it
is an improvement in my case although so far it seems to work say, maybe
2/3 or 3/4 of the time (i.e. I patch a few new kernel rpms, it either
work or it
doesn't and if it doesn't I go back to the trust-worthy one before
July 14), and the code seems to be
a sensible (for the time being). Maybe Larry is affected by more than one
problem, like possibly the power management issue Herton mentioned.

Hin-Tak

Re: Commit 741b4fbc44 (mac80211: fix TX sequence numbers) breaks rtl8187

[Herton Ronaldo Krzesinski]

Em Thursday 24 July 2008 02:40:46 Larry Finger escreveu:
> Hin-Tak Leung wrote:
> > Your patch actually works here! :-). Maybe you should retest?
> >
> > It doesn't patch cleanly against wireless-testing - just indentation
> > oddness, I think.
> > Had to apply the patch by hand. but I am using it right now, so it
> > seems to work.
>
> As I wasn't planning on it to be an official patch, I did not handle it
> with the usual care. My mailer is Thunderbird, and it does mess with the
> white space in patches.
>
> > If somebody else retest okay, can we have this in wireless-testing
> > prompto, please?
>
> To retest, I did a 'git checkout 741b4fbc44' and then applied my patch just
> in case there was a second breakage of the driver in the tree. It worked
> for a while, then went offline, but then so did the case with 741b4fbc44
> reverted. I think some of my problems may have been due to overheating. I
> thought we had that problem licked, but maybe not.
>
> I will be submitting the proper patch tomorrow, particularly if it works
> for others.

For me it works with or without the patch, may be there is some condition that 
triggers the bug that I'm not hitting. But I still have the same problem that 
I previously reported about power management, but the bug isn't on that 
commit and doesn't have to do with power management, later I found that an 
ifconfig <interface> down + ifconfig <interface> up works too, it's something 
on userspace that triggers the bug (shortly after I modprobe the module, udev 
calls automatically scripts to configure the interface, if I disable this in 
udev and run commands manually interface works here, I'll see if I catch this 
other problem, may be a race in initialization code somewhere).

>
> Larry
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-wireless"
> in the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
[]'s
Herton

Re: Commit 741b4fbc44 (mac80211: fix TX sequence numbers) breaks rtl8187

[Hin-Tak Leung]

On Thu, Jul 24, 2008 at 5:37 PM, Herton Ronaldo Krzesinski
<herton@mandriva.com.br> wrote:
<sniped>
> For me it works with or without the patch, may be there is some condition that
> triggers the bug that I'm not hitting. But I still have the same problem that
> I previously reported about power management, but the bug isn't on that
> commit and doesn't have to do with power management, later I found that an
> ifconfig <interface> down + ifconfig <interface> up works too, it's something
> on userspace that triggers the bug (shortly after I modprobe the module, udev
> calls automatically scripts to configure the interface, if I disable this in
> udev and run commands manually interface works here, I'll see if I catch this
> other problem, may be a race in initialization code somewhere).

Some kind of race condition is quite likely. I think I have seen two
instances of dhclient
being launched by udevd quite routinely; and I seems to notice that I have to
do ifconfig up, iwconfig etc in fairly quick succession and in some
specific order to get
it to work, and sometimes killing off udevd launched dhclient and do my own.

I notice that dhclient seems to want to ifconfig the interface down if
it can't get an ip address
(this seems extra to me - there are occasions when I want to have a
bare interface without address up?).

So what's the verdict so far? I am tracking fedora 9 koji test kernels
mostly, and something between
kernel-2.6.25.11-92.fc9 and kernel-2.6.25.11-93.fc9 broke rtl8187. I
just checked out the source rpm
itself, and the difference is just in the wireless area, and supposedly one of
 http://marc.info/?l=linux-wireless&m=121606436000705&w=2 . I think
this is what most agreed - i.e.
nobody is getting much success with latest wireless with rtl8187?

Both the power management hook and the tx seq no removal are among those.
the iwconfig power off doesn't do it for me (it seems to default to
off anyway?),
but I had one success with the tx seq fix having quite substantial
traffic - 50MB? - going through
before I reboot or otherwise lose the connectivity in some non-problematic way
 and didn't get it back ever.

The journey continues...

Hin-Tak

Re: Commit 741b4fbc44 (mac80211: fix TX sequence numbers) breaks rtl8187

[Hin-Tak Leung]

Hi Larry, please tidy the tx seq no patch up and submit to wireless-testing..
I think it is more appropriate to copy the actual comments in rt2000
from Johannes?
instead of or in addtion to: "XXX: This sequence is an attempt to
match what happens in r2x00"

... and feel free to add ack-by/tested-by me.

I am wondering about Herton's problem with power saving (which I had
reproduced).
I think one way to move forward is to enable MAC80211_PS_DEBUG(?) and
see what happens
to the tx queues, now that we have a reliable way of reproducing the problem.

I also wonder how Herton encountered the problem :-). Older
wireless-testing simply
say "unimplemented" or something if one tries to set power-saving.
Maybe Herton has
a few devices which he routinely tries out every part of iwconfig?

Hin-Tak


On Wed, Jul 23, 2008 at 4:32 PM, Larry Finger <Larry.Finger@lwfinger.net> wrote:
<snipped>
>
> I tried something like that, but it didn't work. Maybe you can see what is
> wrong here:
>
>
> Index: wireless-testing/drivers/net/wireless/rtl8187.h
> ===================================================================
> --- wireless-testing.orig/drivers/net/wireless/rtl8187.h
> +++ wireless-testing/drivers/net/wireless/rtl8187.h
> @@ -100,6 +100,7 @@ struct rtl8187_priv {
>        struct usb_device *udev;
>        u32 rx_conf;
>        u16 txpwr_base;
> +       u16 seqno;
>        u8 asic_rev;
>        u8 is_rtl8187b;
>        enum {
> Index: wireless-testing/drivers/net/wireless/rtl8187_dev.c
> ===================================================================
> --- wireless-testing.orig/drivers/net/wireless/rtl8187_dev.c
> +++ wireless-testing/drivers/net/wireless/rtl8187_dev.c
> @@ -169,6 +169,7 @@ static int rtl8187_tx(struct ieee80211_h
>  {
>        struct rtl8187_priv *priv = dev->priv;
>        struct ieee80211_tx_info *info = IEEE80211_SKB_CB(skb);
> +       struct ieee80211_hdr *ieee80211hdr = (struct ieee80211_hdr
> *)skb->data;
>        unsigned int ep;
>        void *buf;
>        struct urb *urb;
> @@ -198,6 +199,14 @@ static int rtl8187_tx(struct ieee80211_h
>                flags |= ieee80211_get_rts_cts_rate(dev, info)->hw_value <<
> 19;
>        }
>
> +       /* XXX: This sequence is an attempt to match what happens in r2x00.
> */
> +       if (info->flags & IEEE80211_TX_CTL_ASSIGN_SEQ) {
> +               if (info->flags & IEEE80211_TX_CTL_FIRST_FRAGMENT)
> +                       priv->seqno += 0x10;
> +               ieee80211hdr->seq_ctrl &= cpu_to_le16(IEEE80211_SCTL_FRAG);
> +               ieee80211hdr->seq_ctrl |= cpu_to_le16(priv->seqno);
> +       }
> +
>        if (!priv->is_rtl8187b) {
>                struct rtl8187_tx_hdr *hdr =
>                        (struct rtl8187_tx_hdr *)skb_push(skb, sizeof(*hdr));
>

Re: Commit 741b4fbc44 (mac80211: fix TX sequence numbers) breaks rtl8187

[Larry Finger]

Hin-Tak Leung wrote:
> Hi Larry, please tidy the tx seq no patch up and submit to wireless-testing..
> I think it is more appropriate to copy the actual comments in rt2000
> from Johannes?
> instead of or in addtion to: "XXX: This sequence is an attempt to
> match what happens in r2x00"
> 
> ... and feel free to add ack-by/tested-by me.
> 
> I am wondering about Herton's problem with power saving (which I had
> reproduced).
> I think one way to move forward is to enable MAC80211_PS_DEBUG(?) and
> see what happens
> to the tx queues, now that we have a reliable way of reproducing the problem.
> 
> I also wonder how Herton encountered the problem :-). Older
> wireless-testing simply
> say "unimplemented" or something if one tries to set power-saving.
> Maybe Herton has
> a few devices which he routinely tries out every part of iwconfig?

I sent that patch on July 24, or at least I thought I did. I was quite surprised 
to not find it in the wireless archives. In any case, I have sent it again tonight.

I tried Herton's sequence of steps for cennecting and it failed no matter what I 
did. In my case, only NetworkManager can get a connection.

I'm not sure how to proceed; however, if you can produce the problem reliably, 
then you have an edge.

Larry

Re: Commit 741b4fbc44 (mac80211: fix TX sequence numbers) breaks rtl8187

[Herton Ronaldo Krzesinski]

Em Tuesday 29 July 2008 00:15:52 Larry Finger escreveu:
> Hin-Tak Leung wrote:
> > Hi Larry, please tidy the tx seq no patch up and submit to
> > wireless-testing.. I think it is more appropriate to copy the actual
> > comments in rt2000 from Johannes?
> > instead of or in addtion to: "XXX: This sequence is an attempt to
> > match what happens in r2x00"
> >
> > ... and feel free to add ack-by/tested-by me.
> >
> > I am wondering about Herton's problem with power saving (which I had
> > reproduced).
> > I think one way to move forward is to enable MAC80211_PS_DEBUG(?) and
> > see what happens
> > to the tx queues, now that we have a reliable way of reproducing the
> > problem.
> >
> > I also wonder how Herton encountered the problem :-). Older
> > wireless-testing simply
> > say "unimplemented" or something if one tries to set power-saving.
> > Maybe Herton has
> > a few devices which he routinely tries out every part of iwconfig?
>
> I sent that patch on July 24, or at least I thought I did. I was quite
> surprised to not find it in the wireless archives. In any case, I have sent
> it again tonight.
>
> I tried Herton's sequence of steps for cennecting and it failed no matter
> what I did. In my case, only NetworkManager can get a connection.
>
> I'm not sure how to proceed; however, if you can produce the problem
> reliably, then you have an edge.

You mean that you couldn't reproduce the bug, ie. with the steps I gave you 
still could connect and scan for wireless networks? I ask because the failure 
is the actual bug, for example if you place let's say a sleep for 5 seconds 
between '... essid any' and '... power on' it doesn't fail.

The bug is that with that sequence, any attemps to associate or scan fail, 
where it shouldn't, at least it's what is expected.

>
> Larry

-- 
[]'s
Herton

Re: Commit 741b4fbc44 (mac80211: fix TX sequence numbers) breaks rtl8187

[Larry Finger]

Herton Ronaldo Krzesinski wrote:
> 
> You mean that you couldn't reproduce the bug, ie. with the steps I gave you 
> still could connect and scan for wireless networks? I ask because the failure 
> is the actual bug, for example if you place let's say a sleep for 5 seconds 
> between '... essid any' and '... power on' it doesn't fail.
> 
> The bug is that with that sequence, any attemps to associate or scan fail, 
> where it shouldn't, at least it's what is expected.

I'll try again, but I couldn't make a connection at all using anything other 
than NetworkManager.

Larry

Re: Commit 741b4fbc44 (mac80211: fix TX sequence numbers) breaks rtl8187

[Herton Ronaldo Krzesinski]

Em Monday 28 July 2008 23:39:26 Hin-Tak Leung escreveu:
> Hi Larry, please tidy the tx seq no patch up and submit to
> wireless-testing.. I think it is more appropriate to copy the actual
> comments in rt2000 from Johannes?
> instead of or in addtion to: "XXX: This sequence is an attempt to
> match what happens in r2x00"
>
> ... and feel free to add ack-by/tested-by me.
>
> I am wondering about Herton's problem with power saving (which I had
> reproduced).
> I think one way to move forward is to enable MAC80211_PS_DEBUG(?) and
> see what happens
> to the tx queues, now that we have a reliable way of reproducing the
> problem.

I enabled here CONFIG_MAC80211_VERBOSE_PS_DEBUG but didn't got anything 
useful. I looked at the code for some time and until now no idea on what is 
happening.

>
> I also wonder how Herton encountered the problem :-). Older
> wireless-testing simply
> say "unimplemented" or something if one tries to set power-saving.
> Maybe Herton has
> a few devices which he routinely tries out every part of iwconfig?

The original sequence of commands:
[1] /sbin/ip link set dev wlan0 down
[2] /sbin/ip link set dev wlan0 up
[3] /sbin/iwconfig wlan0 power on
[4] /sbin/iwconfig wlan0 essid any
[5] /sbin/iwconfig wlan0 power on
[6] /sbin/iwconfig wlan0 essid <my AP>
[7] /sbin/iwconfig wlan0 key open <my key>
That show the issue on rtl8187b I have to test just is near on what Mandriva 
2008.0 ifup-wireless script does, so that's what first made me catch the bug. 
the '... power on' after '... essid any' is what causes later the association 
or any scan attempt to fail, if I place a sleep between [4] and [5] I don't 
get the problem too. The problem is, I couldn't see yet how a power on after 
essid any could cause this.

>
> Hin-Tak
>

-- 
[]'s
Herton

Re: Commit 741b4fbc44 (mac80211: fix TX sequence numbers) breaks rtl8187

[Johannes Berg]

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

On Tue, 2008-07-29 at 00:21 -0300, Herton Ronaldo Krzesinski wrote:

> [3] /sbin/iwconfig wlan0 power on

I don't think turning on power management (yes, this command is
completely misnamed, it turns on power _management_ not the power, you
may want "txpower auto") does anything on this chip, does it?

johannes

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

Re: Commit 741b4fbc44 (mac80211: fix TX sequence numbers) breaks rtl8187

[Tomas Winkler]

On Tue, Jul 29, 2008 at 9:53 AM, Johannes Berg
<johannes@sipsolutions.net> wrote:
> On Tue, 2008-07-29 at 00:21 -0300, Herton Ronaldo Krzesinski wrote:
>
>> [3] /sbin/iwconfig wlan0 power on
>
> I don't think turning on power management (yes, this command is
> completely misnamed, it turns on power _management_ not the power, you
> may want "txpower auto") does anything on this chip, does it?
>
iwconfig power on is not implemented by mac80211 yet. There was one
patch posted that implements it.
I wanted to expand it but  wireless_tools 29 has a bug in parsing this
command. It is fixed only in 30.pre6 which is usually not in distros.

Tomas

Re: Commit 741b4fbc44 (mac80211: fix TX sequence numbers) breaks rtl8187

[Johannes Berg]

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

On Tue, 2008-07-29 at 11:23 +0300, Tomas Winkler wrote:
> On Tue, Jul 29, 2008 at 9:53 AM, Johannes Berg
> <johannes@sipsolutions.net> wrote:
> > On Tue, 2008-07-29 at 00:21 -0300, Herton Ronaldo Krzesinski wrote:
> >
> >> [3] /sbin/iwconfig wlan0 power on
> >
> > I don't think turning on power management (yes, this command is
> > completely misnamed, it turns on power _management_ not the power, you
> > may want "txpower auto") does anything on this chip, does it?
> >
> iwconfig power on is not implemented by mac80211 yet. There was one
> patch posted that implements it.
> I wanted to expand it but  wireless_tools 29 has a bug in parsing this
> command. It is fixed only in 30.pre6 which is usually not in distros.

Heh, fun. Time for putting that into cfg80211 instead if nobody can use
it with wext anyway?

johannes

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

Re: Commit 741b4fbc44 (mac80211: fix TX sequence numbers) breaks rtl8187

[Tomas Winkler]

On Tue, Jul 29, 2008 at 11:28 AM, Johannes Berg
<johannes@sipsolutions.net> wrote:
> On Tue, 2008-07-29 at 11:23 +0300, Tomas Winkler wrote:
>> On Tue, Jul 29, 2008 at 9:53 AM, Johannes Berg
>> <johannes@sipsolutions.net> wrote:
>> > On Tue, 2008-07-29 at 00:21 -0300, Herton Ronaldo Krzesinski wrote:
>> >
>> >> [3] /sbin/iwconfig wlan0 power on
>> >
>> > I don't think turning on power management (yes, this command is
>> > completely misnamed, it turns on power _management_ not the power, you
>> > may want "txpower auto") does anything on this chip, does it?
>> >
>> iwconfig power on is not implemented by mac80211 yet. There was one
>> patch posted that implements it.
>> I wanted to expand it but  wireless_tools 29 has a bug in parsing this
>> command. It is fixed only in 30.pre6 which is usually not in distros.
>
> Heh, fun. Time for putting that into cfg80211 instead if nobody can use
> it with wext anyway?

I've planned to but I waited for some API design suggestions  you guys
will come from OLS but I didn't get much input.
Tomas

Re: Commit 741b4fbc44 (mac80211: fix TX sequence numbers) breaks rtl8187

[Johannes Berg]

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

On Tue, 2008-07-29 at 11:38 +0300, Tomas Winkler wrote:

> >> iwconfig power on is not implemented by mac80211 yet. There was one
> >> patch posted that implements it.
> >> I wanted to expand it but  wireless_tools 29 has a bug in parsing this
> >> command. It is fixed only in 30.pre6 which is usually not in distros.
> >
> > Heh, fun. Time for putting that into cfg80211 instead if nobody can use
> > it with wext anyway?
> 
> I've planned to but I waited for some API design suggestions  you guys
> will come from OLS but I didn't get much input.

This particular thing I don't remember talking about except that we
want, when powersave is on, be able to select the powersaving level
based on information applications attach to their sockets, rather than
requiring the user to make a guess.

johannes

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

Re: Commit 741b4fbc44 (mac80211: fix TX sequence numbers) breaks rtl8187

[Tomas Winkler]

On Tue, Jul 29, 2008 at 12:16 PM, Johannes Berg
<johannes@sipsolutions.net> wrote:
> On Tue, 2008-07-29 at 11:38 +0300, Tomas Winkler wrote:
>
>> >> iwconfig power on is not implemented by mac80211 yet. There was one
>> >> patch posted that implements it.
>> >> I wanted to expand it but  wireless_tools 29 has a bug in parsing this
>> >> command. It is fixed only in 30.pre6 which is usually not in distros.
>> >
>> > Heh, fun. Time for putting that into cfg80211 instead if nobody can use
>> > it with wext anyway?
>>
>> I've planned to but I waited for some API design suggestions  you guys
>> will come from OLS but I didn't get much input.
>
> This particular thing I don't remember talking about except that we
> want, when powersave is on, be able to select the powersaving level
> based on information applications attach to their sockets, rather than
> requiring the user to make a guess.

PS setting is rather global NIC state and should be set as user
preference rather then per application ... socket. If I understand
correctly what you mean by application.
What user actually selects is performance/power consumption trade off.
Tomas.



> johannes
>

Re: Commit 741b4fbc44 (mac80211: fix TX sequence numbers) breaks rtl8187

[Johannes Berg]

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


> PS setting is rather global NIC state and should be set as user
> preference rather then per application ... socket. If I understand
> correctly what you mean by application.
> What user actually selects is performance/power consumption trade off.

yes, but what we also pointed out was that the user isn't able to make a
good trade-off, and PS isn't necessarily linear anyway. Therefore,
making the default be based on letting applications set what sort of
latency/quality guarantees they set makes sense for a good user
experience with maximum power saving that doesn't affect applications,
if the user wants to go higher maybe we can later allow that in some
way, but the default should be based on what performance apps expect.

johannes

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

Re: Commit 741b4fbc44 (mac80211: fix TX sequence numbers) breaks rtl8187

[Tomas Winkler]

On Tue, Jul 29, 2008 at 12:29 PM, Johannes Berg
<johannes@sipsolutions.net> wrote:
>
>> PS setting is rather global NIC state and should be set as user
>> preference rather then per application ... socket. If I understand
>> correctly what you mean by application.
>> What user actually selects is performance/power consumption trade off.
>
> yes, but what we also pointed out was that the user isn't able to make a
> good trade-off, and PS isn't necessarily linear anyway. Therefore,
> making the default be based on letting applications set what sort of
> latency/quality guarantees they set makes sense for a good user
> experience with maximum power saving that doesn't affect applications,
> if the user wants to go higher maybe we can later allow that in some
> way, but the default should be based on what performance apps expect.

I think the path of enabling user to control this through iw(nl) or
wext is closer to
come to usability as to relay on any application to implement it in
next half a year.
You need single API anyway since there have to be single point of
decision as there are many applications
running over single NIC e.g. VoIP and web browser have different requirements.

What I would actually start with is to get notification about dc or
battery supply. I don't think that class
implements notification chain though.

Tomas

Re: Commit 741b4fbc44 (mac80211: fix TX sequence numbers) breaks rtl8187

[Johannes Berg]

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

On Tue, 2008-07-29 at 14:16 +0300, Tomas Winkler wrote:

> I think the path of enabling user to control this through iw(nl) or
> wext is closer to
> come to usability as to relay on any application to implement it in
> next half a year.
> You need single API anyway since there have to be single point of
> decision as there are many applications
> running over single NIC e.g. VoIP and web browser have different requirements.

Umm, no, you just use min(what applications requested).

> What I would actually start with is to get notification about dc or
> battery supply. I don't think that class
> implements notification chain though.

That policy is probably just powersave on/off and definitely belongs
into userland.

johannes

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

Re: Commit 741b4fbc44 (mac80211: fix TX sequence numbers) breaks rtl8187

[Kalle Valo]

Johannes Berg <johannes@sipsolutions.net> writes:

>> What I would actually start with is to get notification about dc or
>> battery supply. I don't think that class
>> implements notification chain though.
>
> That policy is probably just powersave on/off and definitely belongs
> into userland.

I fully agree with Johannes, it belongs to user space. And besides, I
might want to use powersave even when connected to AC.

-- 
Kalle Valo

Re: Commit 741b4fbc44 (mac80211: fix TX sequence numbers) breaks rtl8187

[Kalle Valo]

Johannes Berg <johannes@sipsolutions.net> writes:

>> I've planned to but I waited for some API design suggestions  you guys
>> will come from OLS but I didn't get much input.
>
> This particular thing I don't remember talking about except that we
> want, when powersave is on, be able to select the powersaving level
> based on information applications attach to their sockets, rather than
> requiring the user to make a guess.

We also talked about first implementing a simple interface to enable
and disable powersave. And also about having a timer which enables
powersave whenever where is no traffic, ie. sort of dynamic powersave.
That's a good enough solution which is still simple to implement.

-- 
Kalle Valo

Re: Commit 741b4fbc44 (mac80211: fix TX sequence numbers) breaks rtl8187

[Kalle Valo]

Johannes Berg <johannes@sipsolutions.net> writes:

> On Tue, 2008-07-29 at 11:23 +0300, Tomas Winkler wrote:
>> On Tue, Jul 29, 2008 at 9:53 AM, Johannes Berg
>> <johannes@sipsolutions.net> wrote:
>> > On Tue, 2008-07-29 at 00:21 -0300, Herton Ronaldo Krzesinski wrote:
>> >
>> >> [3] /sbin/iwconfig wlan0 power on
>> >
>> > I don't think turning on power management (yes, this command is
>> > completely misnamed, it turns on power _management_ not the power, you
>> > may want "txpower auto") does anything on this chip, does it?
>> >
>> iwconfig power on is not implemented by mac80211 yet. There was one
>> patch posted that implements it.
>> I wanted to expand it but  wireless_tools 29 has a bug in parsing this
>> command. It is fixed only in 30.pre6 which is usually not in distros.
>
> Heh, fun. Time for putting that into cfg80211 instead if nobody can use
> it with wext anyway?

Wireless Extensions needs to be supported as well, we simply cannot
yet rely only on cfg80211 interface. The reason being that it's not
widely supported yet.

-- 
Kalle Valo

Re: Commit 741b4fbc44 (mac80211: fix TX sequence numbers) breaks rtl8187

[Kalle Valo]

Tomas Winkler <tomasw@gmail.com> writes:

> On Tue, Jul 29, 2008 at 9:53 AM, Johannes Berg
> <johannes@sipsolutions.net> wrote:
>> On Tue, 2008-07-29 at 00:21 -0300, Herton Ronaldo Krzesinski wrote:
>>
>>> [3] /sbin/iwconfig wlan0 power on
>>
>> I don't think turning on power management (yes, this command is
>> completely misnamed, it turns on power _management_ not the power, you
>> may want "txpower auto") does anything on this chip, does it?
>
> iwconfig power on is not implemented by mac80211 yet. There was one
> patch posted that implements it.

John has already applied Samuel's patch to wireless-testing:

http://git.kernel.org/?p=linux/kernel/git/linville/wireless-testing.git;a=commitdiff;h=8f87dd7e540d455f8e7f11478133b85edc969c67

> I wanted to expand it but wireless_tools 29 has a bug in parsing
> this command. It is fixed only in 30.pre6 which is usually not in
> distros.

Could you be more specific, please? We have been using it for few
years now.

-- 
Kalle Valo

Re: Commit 741b4fbc44 (mac80211: fix TX sequence numbers) breaks rtl8187

[Hin-Tak Leung]

On Tue, Jul 29, 2008 at 10:19 PM, Kalle Valo <kalle.valo@nokia.com> wrote:
> Tomas Winkler <tomasw@gmail.com> writes:
>
>> On Tue, Jul 29, 2008 at 9:53 AM, Johannes Berg
>> <johannes@sipsolutions.net> wrote:
>>> On Tue, 2008-07-29 at 00:21 -0300, Herton Ronaldo Krzesinski wrote:
>>>
>>>> [3] /sbin/iwconfig wlan0 power on
>>>
>>> I don't think turning on power management (yes, this command is
>>> completely misnamed, it turns on power _management_ not the power, you
>>> may want "txpower auto") does anything on this chip, does it?

I had look through the code - mac80211 itself, the rtl8187b driver,
and the client tool
(iwconfig 29). The thing is, there is absolutely nothing in rtl8187b
other than one reference
to IEEE80211_HW_HOST_BROADCAST_PS_BUFFERING, and in mac80211, as far
as I understand,
power saving should only has effect for driver in AP mode (as is
IEEE80211_HW_HOST_BROADCAST_PS_BUFFERING)?

In any case, I tried removing IEEE80211_HW_HOST_BROADCAST_PS_BUFFERING
in the driver code,
and also enabling CONFIG_MAC80211_VERBOSE_PS_DEBUG in net/mac80211/*.c
- the former does not improve
the situation, the latter seems to have no effect (i.e. none of the
code with PS_DEBUG is invoked when using
the driver in STA mode) - is that correct?


>>
>> iwconfig power on is not implemented by mac80211 yet. There was one
>> patch posted that implements it.
>
> John has already applied Samuel's patch to wireless-testing:
>
> http://git.kernel.org/?p=linux/kernel/git/linville/wireless-testing.git;a=commitdiff;h=8f87dd7e540d455f8e7f11478133b85edc969c67

This is one of the two patches we have been talking about which breaks
the rt8187b driver. (the other being the tx seq no one).

>
>> I wanted to expand it but wireless_tools 29 has a bug in parsing
>> this command. It is fixed only in 30.pre6 which is usually not in
>> distros.
>
> Could you be more specific, please? We have been using it for few
> years now.

Yes, I like to know what's the bug referring to also. I have looked at
iwconfig 29
and it seems to be doing the right thing ("off" sets disable, "on"
sets the value).

Hin-Tak

Re: Commit 741b4fbc44 (mac80211: fix TX sequence numbers) breaks rtl8187

[Kalle Valo]

Johannes Berg <johannes@sipsolutions.net> writes:

> On Tue, 2008-07-29 at 00:21 -0300, Herton Ronaldo Krzesinski wrote:
>
>> [3] /sbin/iwconfig wlan0 power on
>
> I don't think turning on power management (yes, this command is
> completely misnamed, it turns on power _management_ not the power, you
> may want "txpower auto") does anything on this chip, does it?

There isn't any driver yet in wireless-testing which supports
IEEE80211_CONF_PS:

$ git grep -l IEEE80211_CONF_PS
include/net/mac80211.h
net/mac80211/wext.c
$

So yes, currently 'iwconfig wlan0 power' shouldn't change any
behavious, expect an extra call to config() op.

-- 
Kalle Valo

Re: Commit 741b4fbc44 (mac80211: fix TX sequence numbers) breaks rtl8187

[Kalle Valo]

Herton Ronaldo Krzesinski <herton@mandriva.com.br> writes:

>> I also wonder how Herton encountered the problem :-). Older
>> wireless-testing simply say "unimplemented" or something if one
>> tries to set power-saving. Maybe Herton has a few devices which he
>> routinely tries out every part of iwconfig?
>
> The original sequence of commands:
> [1] /sbin/ip link set dev wlan0 down
> [2] /sbin/ip link set dev wlan0 up
> [3] /sbin/iwconfig wlan0 power on
> [4] /sbin/iwconfig wlan0 essid any
> [5] /sbin/iwconfig wlan0 power on
> [6] /sbin/iwconfig wlan0 essid <my AP>
> [7] /sbin/iwconfig wlan0 key open <my key>
>
> That show the issue on rtl8187b I have to test just is near on what Mandriva 
> 2008.0 ifup-wireless script does, so that's what first made me catch the bug. 
> the '... power on' after '... essid any' is what causes later the association 
> or any scan attempt to fail, if I place a sleep between [4] and [5] I don't 
> get the problem too.

Thanks for the clarification, this kind of information is important
when reporting a bug.

> The problem is, I couldn't see yet how a power on after essid any
> could cause this.

I'm guessing that it has something to do ieee80211_ioctl_siwpower()
calling ieee80211_hw_config(local) which ends up as an extra call to
rtl8187_config(). I do not know why this is a problem for rtl8187b.

It would help if you, or someone else seeing the same problem, could
provide more information by enabling MAC80211_VERBOSE_DEBUG and
MAC80211_LOWTX_FRAME_DUMP config options, and sending the kernel log
output here. I don't have a rtl8187b card myself so I cannot test
this.

-- 
Kalle Valo

Re: Commit 741b4fbc44 (mac80211: fix TX sequence numbers) breaks rtl8187

[Herton Ronaldo Krzesinski]

Em Tuesday 29 July 2008 18:12:58 Kalle Valo escreveu:
> Herton Ronaldo Krzesinski <herton@mandriva.com.br> writes:
> >> I also wonder how Herton encountered the problem :-). Older
> >> wireless-testing simply say "unimplemented" or something if one
> >> tries to set power-saving. Maybe Herton has a few devices which he
> >> routinely tries out every part of iwconfig?
> >
> > The original sequence of commands:
> > [1] /sbin/ip link set dev wlan0 down
> > [2] /sbin/ip link set dev wlan0 up
> > [3] /sbin/iwconfig wlan0 power on
> > [4] /sbin/iwconfig wlan0 essid any
> > [5] /sbin/iwconfig wlan0 power on
> > [6] /sbin/iwconfig wlan0 essid <my AP>
> > [7] /sbin/iwconfig wlan0 key open <my key>
> >
> > That show the issue on rtl8187b I have to test just is near on what
> > Mandriva 2008.0 ifup-wireless script does, so that's what first made me
> > catch the bug. the '... power on' after '... essid any' is what causes
> > later the association or any scan attempt to fail, if I place a sleep
> > between [4] and [5] I don't get the problem too.
>
> Thanks for the clarification, this kind of information is important
> when reporting a bug.
>
> > The problem is, I couldn't see yet how a power on after essid any
> > could cause this.
>
> I'm guessing that it has something to do ieee80211_ioctl_siwpower()
> calling ieee80211_hw_config(local) which ends up as an extra call to
> rtl8187_config(). I do not know why this is a problem for rtl8187b.

You are right, I was looking at wrong place, indeed the issue is on rtl8187. 
The problem here is that mac80211 can call rtl8187_config while another 
instance of it is still running, and on rtl8187_config this is a critical 
section:

...
        /* Enable TX loopback on MAC level to avoid TX during channel
         * changes, as this has be seen to causes problems and the
         * card will stop work until next reset
         */
        rtl818x_iowrite32(priv, &priv->map->TX_CONF,
                          reg | RTL818X_TX_CONF_LOOPBACK_MAC);
        msleep(10);
        priv->rf->set_chan(dev, conf);
        msleep(10);
        rtl818x_iowrite32(priv, &priv->map->TX_CONF, reg);
...

My sequence of commands trigger this, so there are cases where it disables TX 
loopback while there are another instance that can be still 
inside ->set_chan, that's explain also why card stops to work. This section 
should be lock protected or mac80211 ensure to not call local->ops->config 
while another call is still running.

>
> It would help if you, or someone else seeing the same problem, could
> provide more information by enabling MAC80211_VERBOSE_DEBUG and
> MAC80211_LOWTX_FRAME_DUMP config options, and sending the kernel log
> output here. I don't have a rtl8187b card myself so I cannot test
> this.

-- 
[]'s
Herton

Re: Commit 741b4fbc44 (mac80211: fix TX sequence numbers) breaks rtl8187

[Kalle Valo]

Herton Ronaldo Krzesinski <herton@mandriva.com.br> writes:

> You are right, I was looking at wrong place, indeed the issue is on rtl8187. 
> The problem here is that mac80211 can call rtl8187_config while another 
> instance of it is still running, and on rtl8187_config this is a critical 
> section:
>

[...]

> My sequence of commands trigger this, so there are cases where it disables TX 
> loopback while there are another instance that can be still 
> inside ->set_chan, that's explain also why card stops to work. This section 
> should be lock protected or mac80211 ensure to not call local->ops->config 
> while another call is still running.

To tell you the truth, I don't know if ops->config() calls should be
serialized or not, but I have always used locking on the driver as a
precaution.

Johannes, can you comment on this, please?

-- 
Kalle Valo

Re: Commit 741b4fbc44 (mac80211: fix TX sequence numbers) breaks rtl8187

[Johannes Berg]

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

On Wed, 2008-07-30 at 11:54 +0300, Kalle Valo wrote:

> To tell you the truth, I don't know if ops->config() calls should be
> serialized or not, but I have always used locking on the driver as a
> precaution.
> 
> Johannes, can you comment on this, please?

TBH, I couldn't care less. Most drivers need serialisation anyway to
guard against others accessing their registers, so I never saw this as a
big problem.

johannes

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