[PATCH] p54: Fix for TX sequence number problem that resulted from commit 741b4fbc44

[PATCH] p54: Fix for TX sequence number problem that resulted from commit 741b4fbc44

[Larry Finger]

Following commit 741b4fbc441dba885cc8f97a10e87f2acd04c5f2, if a packet
has the IEEE80211_TX_CTL_ASSIGN_SEQ assigned, a sequence number must be
supplied, either by hardware or software. AFAIK, no such hardware exists
for the p54, thus it must be done in software. With this patch, a connection
qith p54usb is stable, whereas the interface went off-line in 2-3 hours
without this change. Note that this code will have to be reworked for proper
sequence numbers on beacons. In addition, the sequence number has been placed
in the hardware state, not the vif state.

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

John,

This is a bug fix for 2.6.27.

Larry
---

 p54.h       |    1 +
 p54common.c |   14 ++++++++++++++
 2 files changed, 15 insertions(+)

Index: wireless-testing/drivers/net/wireless/p54/p54.h
===================================================================
--- wireless-testing.orig/drivers/net/wireless/p54/p54.h
+++ wireless-testing/drivers/net/wireless/p54/p54.h
@@ -52,6 +52,7 @@ struct p54_common {
 	int (*open)(struct ieee80211_hw *dev);
 	void (*stop)(struct ieee80211_hw *dev);
 	int mode;
+	u16 seqno;
 	struct mutex conf_mutex;
 	u8 mac_addr[ETH_ALEN];
 	u8 bssid[ETH_ALEN];
Index: wireless-testing/drivers/net/wireless/p54/p54common.c
===================================================================
--- wireless-testing.orig/drivers/net/wireless/p54/p54common.c
+++ wireless-testing/drivers/net/wireless/p54/p54common.c
@@ -553,6 +553,7 @@ static int p54_tx(struct ieee80211_hw *d
 	struct ieee80211_tx_queue_stats *current_queue;
 	struct p54_common *priv = dev->priv;
 	struct p54_control_hdr *hdr;
+	struct ieee80211_hdr *ieee80211hdr = (struct ieee80211_hdr *)skb->data;
 	struct p54_tx_control_allocdata *txhdr;
 	size_t padding, len;
 	u8 rate;
@@ -605,6 +606,19 @@ static int p54_tx(struct ieee80211_hw *d
 	if (padding)
 		txhdr->align[0] = padding;
 
+	/* FIXME: The sequence that follows is needed for this driver to
+	 * work with mac80211 since "mac80211: fix TX sequence numbers".
+	 * As with the temporary code in rt2x00, changes will be needed
+	 * to get proper sequence numbers on beacons. In addition, this
+	 * patch places the sequence number in the hardware state, which
+	 * limits us to a single virtual state.
+	 */
+	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);
+	}
 	/* modifies skb->cb and with it info, so must be last! */
 	p54_assign_address(dev, skb, hdr, skb->len);
 

Re: [PATCH] p54: Fix for TX sequence number problem that resulted from commit 741b4fbc44

[Chr]

On Tuesday 05 August 2008 18:23:16 Larry Finger wrote:
>
> +	/* FIXME: The sequence that follows is needed for this driver to
> +	 * work with mac80211 since "mac80211: fix TX sequence numbers".
> +	 * As with the temporary code in rt2x00, changes will be needed
> +	 * to get proper sequence numbers on beacons. In addition, this
> +	 * patch places the sequence number in the hardware state, which
> +	 * limits us to a single virtual state.
> +	 */
> +	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);
> +	}
??? 
What is this for? The firmware does the sequence number counting
and will simply override this field without looking at it... Are you sure this
really fixes anything?
 
That said: the firmware will always report the sequence number of every
tx'ed frame as a part of p54_frame_sent_hdr in the _free_ tx callback...

Regards,
	Chr.

Re: [PATCH] p54: Fix for TX sequence number problem that resulted from commit 741b4fbc44

[Larry Finger]

Chr wrote:
> ??? 
> What is this for? The firmware does the sequence number counting
> and will simply override this field without looking at it... Are you sure this
> really fixes anything?
>  
> That said: the firmware will always report the sequence number of every
> tx'ed frame as a part of p54_frame_sent_hdr in the _free_ tx callback...

Without this patch, my p54usb would go offline every 2-3 hours. To recover, the 
driver had to be unloaded and reloaded. With it, the device has never gone 
offline for as long as I have tested. I have no idea what the firmware does, but 
it has helped my system.

Larry

Re: [PATCH] p54: Fix for TX sequence number problem that resulted from commit 741b4fbc44

[Chr]

On Friday 08 August 2008 16:29:07 Larry Finger wrote:
> Chr wrote:
> > ???
> > What is this for? The firmware does the sequence number counting
> > and will simply override this field without looking at it... Are you sure
> > this really fixes anything?
> >
> > That said: the firmware will always report the sequence number of every
> > tx'ed frame as a part of p54_frame_sent_hdr in the _free_ tx callback...
>
> Without this patch, my p54usb would go offline every 2-3 hours. To recover,
> the driver had to be unloaded and reloaded. With it, the device has never
> gone offline for as long as I have tested. I have no idea what the firmware
> does, but it has helped my system.

well, since I got the usb device I had nothing but problems with it.
e.g: one single ping -s 1400 $AP can bring it down and a _replug_ is necessary.
The minipci card on the other hand is very stable.

I guess p54usb needs some mutexes & udelays here & there.

on-topic: I digged a bit in the logs:

http://jbnote.free.fr/prism54usb/data/usbsnoop-logs/
and there "usbsnoop_old_drv_ver1.log.bz2"

you can easily find tx' data frames by searching for
"00000010: 10 40" string.

to cut the case here is a extract (only ieee 80211 header @line 26218)
08 01 <= frame control
00 00 <= duration ZERO
00 06 25 bb 9d 4b <= mac
00 0f 66 17 46 6b <= mac
ff ff ff ff ff ff <= mac
00 00 <= sequence ZERO
aa aa 03 <= 802.2 LLC header

the next one is at line 27188... and so down to end of packages at line 329241.
there isn't one single package where the sequence number is not zero!

So the problem must be somewhere else! what happens when:
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);
	ieee80211hdr->seq_ctrl &= cpu_to_le16(IEEE80211_SCTL_FRAG);
}
(yeah a new definition for idiotic! but it's about the same amount of of clock cycles).

Regards,
	Chr

Re: [PATCH] p54: Fix for TX sequence number problem that resulted from commit 741b4fbc44

[Larry Finger]

Chr wrote:
> On Friday 08 August 2008 16:29:07 Larry Finger wrote:
> 
> well, since I got the usb device I had nothing but problems with it.
> e.g: one single ping -s 1400 $AP can bring it down and a _replug_ is necessary.
> The minipci card on the other hand is very stable.
> 
> I guess p54usb needs some mutexes & udelays here & there.

Mine also crashes with a ping of length 1400, but 1390 and 1410 work 
just fine. I also tested with tcpperf and I am pretty sure that this 
is a transmit problem. When I set the MTU to 1300, then a ping of 
length 1400 works fine. Is this a firmware bug? Which firmware are you 
using? Mine is isl3887usb_bare.

> on-topic: I digged a bit in the logs:
> 
> http://jbnote.free.fr/prism54usb/data/usbsnoop-logs/
> and there "usbsnoop_old_drv_ver1.log.bz2"
> 
> you can easily find tx' data frames by searching for
> "00000010: 10 40" string.
> 
> to cut the case here is a extract (only ieee 80211 header @line 26218)
> 08 01 <= frame control
> 00 00 <= duration ZERO
> 00 06 25 bb 9d 4b <= mac
> 00 0f 66 17 46 6b <= mac
> ff ff ff ff ff ff <= mac
> 00 00 <= sequence ZERO
> aa aa 03 <= 802.2 LLC header
> 
> the next one is at line 27188... and so down to end of packages at line 329241.
> there isn't one single package where the sequence number is not zero!
> 
> So the problem must be somewhere else! what happens when:
> 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);
> 	ieee80211hdr->seq_ctrl &= cpu_to_le16(IEEE80211_SCTL_FRAG);
> }
> (yeah a new definition for idiotic! but it's about the same amount of of clock cycles).

This didn't help the "ping -s 1400" problem.

Larry

Re: [PATCH] p54: Fix for TX sequence number problem that resulted from commit 741b4fbc44

[Chr]

On Saturday 09 August 2008 18:02:49 Larry Finger wrote:
>
> Mine also crashes with a ping of length 1400, but 1390 and 1410 work
> just fine. I also tested with tcpperf and I am pretty sure that this
> is a transmit problem. When I set the MTU to 1300, then a ping of
> length 1400 works fine. Is this a firmware bug? Which firmware are you
> using? Mine is isl3887usb_bare.
hmm? there are lots of new & old firmwares. 
what chip do you have? a ISL3886 + net2280 or a ISL3887?
you can get the firmware version from dmesg, just load the
driver and grep for "p54: FW rev"

my devices: 
isl3880 minipci: 2.7.0.0 (best supported) & 2.13.1.0 (unstable)
isl3887 usb: 2.5.8.0(bugs), 2.6.16.0(DELL bugs) and 2.13.2.0 (WIP) 

>
> > on-topic: I digged a bit in the logs:
> >
> > http://jbnote.free.fr/prism54usb/data/usbsnoop-logs/
> > and there "usbsnoop_old_drv_ver1.log.bz2"
> > [...]
> > So the problem must be somewhere else! what happens when:
> > 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);
> > 	ieee80211hdr->seq_ctrl &= cpu_to_le16(IEEE80211_SCTL_FRAG);
> > }
> > (yeah a new definition for idiotic! but it's about the same amount of of
> > clock cycles).
>
> This didn't help the "ping -s 1400" problem.
Of course not, but maybe it could help with this 2-3 hour disconnect problem.

Regards, 
	Chr.

Re: [PATCH] p54: Fix for TX sequence number problem that resulted from commit 741b4fbc44

[Larry Finger]

Chr wrote:
> hmm? there are lots of new & old firmwares. 
> what chip do you have? a ISL3886 + net2280 or a ISL3887?
> you can get the firmware version from dmesg, just load the
> driver and grep for "p54: FW rev"
> 
> my devices: 
> isl3880 minipci: 2.7.0.0 (best supported) & 2.13.1.0 (unstable)
> isl3887 usb: 2.5.8.0(bugs), 2.6.16.0(DELL bugs) and 2.13.2.0 (WIP) 
>

 From dmesg:

firmware: requesting isl3887usb_bare
p54: LM86 firmware
p54: FW rev 2.5.8.0 - Softmac protocol 3.0

I think this shows that I have an ISL3887. As further confirmation, it 
uses routine p54u_tx_3887() to transmit.  Are the bugs in 2.5.8.0 and 
2.6.16.0 described on the Web? My device is a Dell DW1450.

The 2-3 hour disconnect is still present with the sequence number 
patch reverted and the MTU at 1300. Still testing.

Larry

Re: [PATCH] p54: Fix for TX sequence number problem that resulted from commit 741b4fbc44

[Larry Finger]

Chr wrote:
> the next one is at line 27188... and so down to end of packages at line 329241.
> there isn't one single package where the sequence number is not zero!
> 
> So the problem must be somewhere else! what happens when:
> 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);
> 	ieee80211hdr->seq_ctrl &= cpu_to_le16(IEEE80211_SCTL_FRAG);
> }
> (yeah a new definition for idiotic! but it's about the same amount of of clock cycles).

There must be some situation where the firmware fails to assign a 
proper sequence number to some packets. With this patch in place, the 
device failed after 2 hours. Restoring my original patch, it has been 
up for 15 hours and still going.

Larry

Re: [PATCH] p54: Fix for TX sequence number problem that resulted from commit 741b4fbc44

[Chr]

On Sunday 10 August 2008 16:19:10 Larry Finger wrote:
> Chr wrote:
> > the next one is at line 27188... and so down to end of packages at line
> > 329241. there isn't one single package where the sequence number is not
> > zero!
> >
> > So the problem must be somewhere else! what happens when:
> > 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);
> > 	ieee80211hdr->seq_ctrl &= cpu_to_le16(IEEE80211_SCTL_FRAG);
> > }
> > (yeah a new definition for idiotic! but it's about the same amount of of
> > clock cycles).
>
> There must be some situation where the firmware fails to assign a
> proper sequence number to some packets. With this patch in place, the
> device failed after 2 hours. Restoring my original patch, it has been
> up for 15 hours and still going.
well, if you don't ping around with -s 1400 of course ;-).

On the other hand, it doesn't break anything (that wasn't broken before) and 
if real helps, why not?

Acked-by: Christian Lamparter <chunkeey@web.de>

Re: [PATCH] p54: Fix for TX sequence number problem that resulted from commit 741b4fbc44

[Larry Finger]

Chr wrote:
> well, if you don't ping around with -s 1400 of course ;-).
> 
> On the other hand, it doesn't break anything (that wasn't broken before) and 
> if real helps, why not?
> 
> Acked-by: Christian Lamparter <chunkeey@web.de>

The packet size of 1408 problem is fixed here with an 'ifconfig wlan2 
mtu 1300' command. With it I can ping any length.

This patch is already in mainline as commit eda0c003d1ff14c9.

Larry

Re: [PATCH] p54: Fix for TX sequence number problem that resulted from commit 741b4fbc44

[Chr]

On Monday 11 August 2008 01:04:48 Larry Finger wrote:
> Chr wrote:
>
> The packet size of 1408 problem is fixed here with an 'ifconfig wlan2
> mtu 1300' command. With it I can ping any length.
>
well, seems to be a firmware problem.
2.13.3.0 has no problems with all kinds of big frames on my 3887...

So, is there any knob to tell mac80211 about fragmentation from
the driver side? I'm a bit lost here what to do.

Regards,
	Chr

Re: [PATCH] p54: Fix for TX sequence number problem that resulted from commit 741b4fbc44

[Chr]

On Sunday 10 August 2008 16:19:10 Larry Finger wrote:
> Chr wrote:
> > the next one is at line 27188... and so down to end of packages at line
> > 329241. there isn't one single package where the sequence number is not
> > zero!
> >
> > So the problem must be somewhere else! what happens when:
> > 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);
> > 	ieee80211hdr->seq_ctrl &= cpu_to_le16(IEEE80211_SCTL_FRAG);
> > }
> > (yeah a new definition for idiotic! but it's about the same amount of of
> > clock cycles).
>
> There must be some situation where the firmware fails to assign a
> proper sequence number to some packets. With this patch in place, the
> device failed after 2 hours. Restoring my original patch, it has been
> up for 15 hours and still going.
Hmm, or there's something else can't count?!??

Anyway, the firmware will always assign (as in overwrite) the
sequence number for every transmitted frame... unfortunatly
there isn't a uniform way to disable this behaviour. 

So, rather than doing the sequence number (ac-)couting in the
driver, we can stick to the one the firmware has already generated...

Larry, does this patch fixes the "every two hour disconnect" problem as well, or not at all?
---
diff -Nurp a/drivers/net/wireless/p54/p54common.c b/drivers/net/wireless/p54/p54common.c
--- a/drivers/net/wireless/p54/p54common.c	2008-08-13 20:50:03.000000000 +0200
+++ b/drivers/net/wireless/p54/p54common.c	2008-08-15 20:06:12.000000000 +0200
@@ -428,6 +428,12 @@ static void p54_rx_frame_sent(struct iee
 			info->status.retry_count = payload->retries - 1;
 			info->status.ack_signal = le16_to_cpu(payload->ack_rssi);
 			skb_pull(entry, sizeof(*hdr) + pad + sizeof(*entry_data));
+			if (info->flags & IEEE80211_TX_CTL_ASSIGN_SEQ) {
+				struct ieee80211_hdr *ieee80211hdr =
+					(struct ieee80211_hdr *) entry->data;
+
+				ieee80211hdr->seq_ctrl |= payload->seq;
+			}
 			ieee80211_tx_status_irqsafe(dev, entry);
 			break;
 		} else
@@ -553,7 +559,6 @@ static int p54_tx(struct ieee80211_hw *d
 	struct ieee80211_tx_queue_stats *current_queue;
 	struct p54_common *priv = dev->priv;
 	struct p54_control_hdr *hdr;
-	struct ieee80211_hdr *ieee80211hdr = (struct ieee80211_hdr *)skb->data;
 	struct p54_tx_control_allocdata *txhdr;
 	size_t padding, len;
 	u8 rate, cts_rate = 0x20;
@@ -604,19 +609,6 @@ static int p54_tx(struct ieee80211_hw *d
 	if (padding)
 		txhdr->align[0] = padding;
 
-	/* FIXME: The sequence that follows is needed for this driver to
-	 * work with mac80211 since "mac80211: fix TX sequence numbers".
-	 * As with the temporary code in rt2x00, changes will be needed
-	 * to get proper sequence numbers on beacons. In addition, this
-	 * patch places the sequence number in the hardware state, which
-	 * limits us to a single virtual state.
-	 */
-	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);
-	}
 	/* modifies skb->cb and with it info, so must be last! */
 	p54_assign_address(dev, skb, hdr, skb->len);
 
diff -Nurp a/drivers/net/wireless/p54/p54.h b/drivers/net/wireless/p54/p54.h
--- a/drivers/net/wireless/p54/p54.h	2008-08-13 20:50:30.000000000 +0200
+++ b/drivers/net/wireless/p54/p54.h	2008-08-15 19:38:10.000000000 +0200
@@ -52,7 +52,6 @@ struct p54_common {
 	int (*open)(struct ieee80211_hw *dev);
 	void (*stop)(struct ieee80211_hw *dev);
 	int mode;
-	u16 seqno;
 	struct mutex conf_mutex;
 	u8 mac_addr[ETH_ALEN];
 	u8 bssid[ETH_ALEN];

Re: [PATCH] p54: Fix for TX sequence number problem that resulted from commit 741b4fbc44

[Johannes Berg]

On Fri, 2008-08-15 at 20:34 +0200, Chr wrote:
> Hmm, or there's something else can't count?!??
> 
> Anyway, the firmware will always assign (as in overwrite) the
> sequence number for every transmitted frame... unfortunatly
> there isn't a uniform way to disable this behaviour. 

If it always does that, then it cannot be implementing QoS properly.

johannes

Re: [PATCH] p54: Fix for TX sequence number problem that resulted from commit 741b4fbc44

[Chr]

On Friday 15 August 2008 21:26:15 Johannes Berg wrote:
> On Fri, 2008-08-15 at 20:34 +0200, Chr wrote:
> > Hmm, or there's something else can't count?!??
> >
> > Anyway, the firmware will always assign (as in overwrite) the
> > sequence number for every transmitted frame... unfortunatly
> > there isn't a uniform way to disable this behaviour.
>
> If it always does that, then it cannot be implementing QoS properly.

hmm? has this something to do with the per STA TID in 
7.1.3.4.1 (802.11-2007) ?

Well the firmware (aka: LowerMAC) has some sort station
database so there could be some room for that... maybe.

(But yes, when I was playing with the AP/IBSS, 
all beacon frames had a running sequence counter...

maybe you can see this with a cx3110x in IBSS mode too?!)

Regards,
	Chr

Re: [PATCH] p54: Fix for TX sequence number problem that resulted from commit 741b4fbc44

[Johannes Berg]

On Fri, 2008-08-15 at 22:15 +0200, Chr wrote:

> hmm? has this something to do with the per STA TID in 
> 7.1.3.4.1 (802.11-2007) ?

yes

> Well the firmware (aka: LowerMAC) has some sort station
> database so there could be some room for that... maybe.

makes little sense since we don't tell it about stations, well, I dunno.

> (But yes, when I was playing with the AP/IBSS, 
> all beacon frames had a running sequence counter...

Maybe only non-QoS frames?

> maybe you can see this with a cx3110x in IBSS mode too?!)

we're supposed to be getting documentation on the lmac in there...

johannes

Re: [PATCH] p54: Fix for TX sequence number problem that resulted from commit 741b4fbc44

[Chr]

On Friday 15 August 2008 22:17:16 Johannes Berg wrote:
> On Fri, 2008-08-15 at 22:15 +0200, Chr wrote:
> > hmm? has this something to do with the per STA TID in
> > 7.1.3.4.1 (802.11-2007) ?
>
> yes
>
> > Well the firmware (aka: LowerMAC) has some sort station
> > database so there could be some room for that... maybe.
>
> makes little sense since we don't tell it about stations, well, I dunno.
well, the firmware parses some parts of the ieee80211 header.
And we are actually supposed to tell the firmware about the station aid,
on every tx (and that's why there's a AID field in the tx_info struct now. ;-) 
It's not used yet, but it will when Linville returns...

>
> > (But yes, when I was playing with the AP/IBSS,
> > all beacon frames had a running sequence counter...
>
> Maybe only non-QoS frames?
well, if you take a look at the cx3110x log you can see
that the duration & sequence field is always zero...

the firmware is actually ~30kb of ARM-code, but unfortunatly
they decided to pack it...

>
> > maybe you can see this with a cx3110x in IBSS mode too?!)
>
> we're supposed to be getting documentation on the lmac in there...
Really?! Well, from my POV it would be better to kickstart FreeMAC,
rather than writing a driver for these LMAC Firmwares... seriously!

Regards,
	Chr

Re: [PATCH] p54: Fix for TX sequence number problem that resulted from commit 741b4fbc44

[Johannes Berg]

> > we're supposed to be getting documentation on the lmac in there...
> Really?! Well, from my POV it would be better to kickstart FreeMAC,
> rather than writing a driver for these LMAC Firmwares... seriously!

Dunno. We have the header already: 
http://cx3110x.sipsolutions.net/Commands?action=AttachFile&do=view&target=lmac_longbow.h

johannes

Re: [PATCH] p54: Fix for TX sequence number problem that resulted from commit 741b4fbc44

[Larry Finger]

Chr wrote:
> Hmm, or there's something else can't count?!??
> 
> Anyway, the firmware will always assign (as in overwrite) the
> sequence number for every transmitted frame... unfortunatly
> there isn't a uniform way to disable this behaviour. 
> 
> So, rather than doing the sequence number (ac-)couting in the
> driver, we can stick to the one the firmware has already generated...
> 
> Larry, does this patch fixes the "every two hour disconnect" problem as well, or not at all?
> ---
> diff -Nurp a/drivers/net/wireless/p54/p54common.c b/drivers/net/wireless/p54/p54common.c
> --- a/drivers/net/wireless/p54/p54common.c	2008-08-13 20:50:03.000000000 +0200
> +++ b/drivers/net/wireless/p54/p54common.c	2008-08-15 20:06:12.000000000 +0200
> @@ -428,6 +428,12 @@ static void p54_rx_frame_sent(struct iee
>  			info->status.retry_count = payload->retries - 1;
>  			info->status.ack_signal = le16_to_cpu(payload->ack_rssi);
>  			skb_pull(entry, sizeof(*hdr) + pad + sizeof(*entry_data));
> +			if (info->flags & IEEE80211_TX_CTL_ASSIGN_SEQ) {
> +				struct ieee80211_hdr *ieee80211hdr =
> +					(struct ieee80211_hdr *) entry->data;
> +
> +				ieee80211hdr->seq_ctrl |= payload->seq;
> +			}
>  			ieee80211_tx_status_irqsafe(dev, entry);
>  			break;
>  		} else
> @@ -553,7 +559,6 @@ static int p54_tx(struct ieee80211_hw *d
>  	struct ieee80211_tx_queue_stats *current_queue;
>  	struct p54_common *priv = dev->priv;
>  	struct p54_control_hdr *hdr;
> -	struct ieee80211_hdr *ieee80211hdr = (struct ieee80211_hdr *)skb->data;
>  	struct p54_tx_control_allocdata *txhdr;
>  	size_t padding, len;
>  	u8 rate, cts_rate = 0x20;
> @@ -604,19 +609,6 @@ static int p54_tx(struct ieee80211_hw *d
>  	if (padding)
>  		txhdr->align[0] = padding;
>  
> -	/* FIXME: The sequence that follows is needed for this driver to
> -	 * work with mac80211 since "mac80211: fix TX sequence numbers".
> -	 * As with the temporary code in rt2x00, changes will be needed
> -	 * to get proper sequence numbers on beacons. In addition, this
> -	 * patch places the sequence number in the hardware state, which
> -	 * limits us to a single virtual state.
> -	 */
> -	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);
> -	}
>  	/* modifies skb->cb and with it info, so must be last! */
>  	p54_assign_address(dev, skb, hdr, skb->len);
>  
> diff -Nurp a/drivers/net/wireless/p54/p54.h b/drivers/net/wireless/p54/p54.h
> --- a/drivers/net/wireless/p54/p54.h	2008-08-13 20:50:30.000000000 +0200
> +++ b/drivers/net/wireless/p54/p54.h	2008-08-15 19:38:10.000000000 +0200
> @@ -52,7 +52,6 @@ struct p54_common {
>  	int (*open)(struct ieee80211_hw *dev);
>  	void (*stop)(struct ieee80211_hw *dev);
>  	int mode;
> -	u16 seqno;
>  	struct mutex conf_mutex;
>  	u8 mac_addr[ETH_ALEN];
>  	u8 bssid[ETH_ALEN];
> 
> 

This one lasted for 4 hours. What was worse is that it crashed my 
computer with a 1 Hz blink of the Caps Lock light when it failed. I 
hadn't seen that before.

Larry

Re: [PATCH] p54: Fix for TX sequence number problem that resulted from commit 741b4fbc44

[Chr]

On Saturday 16 August 2008 02:34:38 Larry Finger wrote:
> This one lasted for 4 hours. What was worse is that it crashed my
> computer with a 1 Hz blink of the Caps Lock light when it failed. I
> hadn't seen that before.

hmm,.... sounds like a bit of a old problem with the minipci card...
p54_rx_frame_sent walks in the tx_queue without holding the tx_queue.lock.

However, this problem is next to impossible to trigger in the current code.
So, a crash log from a serial console would be great?
---
diff -Nurp a/drivers/net/wireless/p54/p54common.c b/drivers/net/wireless/p54/p54common.c
--- a/drivers/net/wireless/p54/p54common.c	2008-08-13 20:50:03.000000000 +0200
+++ b/drivers/net/wireless/p54/p54common.c	2008-08-16 03:44:37.000000000 +0200
@@ -391,7 +391,9 @@ static void p54_rx_frame_sent(struct iee
 	struct memrecord *range = NULL;
 	u32 freed = 0;
 	u32 last_addr = priv->rx_start;
+	unsigned long flags;
 
+	spin_lock_irqsave(&priv->tx_queue.lock, flags);
 	while (entry != (struct sk_buff *)&priv->tx_queue) {
 		struct ieee80211_tx_info *info = IEEE80211_SKB_CB(entry);
 		range = (void *)info->driver_data;
@@ -412,6 +414,7 @@ static void p54_rx_frame_sent(struct iee
 
 			last_addr = range->end_addr;
 			__skb_unlink(entry, &priv->tx_queue);
+			spin_unlock_irqrestore(&priv->tx_queue.lock, flags);
 			memset(&info->status, 0, sizeof(info->status));
 			entry_hdr = (struct p54_control_hdr *) entry->data;
 			entry_data = (struct p54_tx_control_allocdata *) entry_hdr->data;
@@ -428,13 +431,24 @@ static void p54_rx_frame_sent(struct iee
 			info->status.retry_count = payload->retries - 1;
 			info->status.ack_signal = le16_to_cpu(payload->ack_rssi);
 			skb_pull(entry, sizeof(*hdr) + pad + sizeof(*entry_data));
+			if (info->flags & IEEE80211_TX_CTL_ASSIGN_SEQ) {
+				struct ieee80211_hdr *ieee80211hdr =
+					(struct ieee80211_hdr *) entry->data;
+
+				if (skb->len >= sizeof(*hdr))
+					ieee80211hdr->seq_ctrl |= payload->seq;
+				else
+					WARN_ON(1);
+			}
 			ieee80211_tx_status_irqsafe(dev, entry);
-			break;
+			goto out;
 		} else
 			last_addr = range->end_addr;
 		entry = entry->next;
 	}
+	spin_unlock_irqrestore(&priv->tx_queue.lock, flags);
 
+out:
 	if (freed >= IEEE80211_MAX_RTS_THRESHOLD + 0x170 +
 	    sizeof(struct p54_control_hdr))
 		p54_wake_free_queues(dev);
@@ -553,7 +567,6 @@ static int p54_tx(struct ieee80211_hw *d
 	struct ieee80211_tx_queue_stats *current_queue;
 	struct p54_common *priv = dev->priv;
 	struct p54_control_hdr *hdr;
-	struct ieee80211_hdr *ieee80211hdr = (struct ieee80211_hdr *)skb->data;
 	struct p54_tx_control_allocdata *txhdr;
 	size_t padding, len;
 	u8 rate, cts_rate = 0x20;
@@ -604,19 +617,6 @@ static int p54_tx(struct ieee80211_hw *d
 	if (padding)
 		txhdr->align[0] = padding;
 
-	/* FIXME: The sequence that follows is needed for this driver to
-	 * work with mac80211 since "mac80211: fix TX sequence numbers".
-	 * As with the temporary code in rt2x00, changes will be needed
-	 * to get proper sequence numbers on beacons. In addition, this
-	 * patch places the sequence number in the hardware state, which
-	 * limits us to a single virtual state.
-	 */
-	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);
-	}
 	/* modifies skb->cb and with it info, so must be last! */
 	p54_assign_address(dev, skb, hdr, skb->len);
 
diff -Nurp a/drivers/net/wireless/p54/p54.h b/drivers/net/wireless/p54/p54.h
--- a/drivers/net/wireless/p54/p54.h	2008-08-13 20:50:30.000000000 +0200
+++ b/drivers/net/wireless/p54/p54.h	2008-08-15 19:38:10.000000000 +0200
@@ -52,7 +52,6 @@ struct p54_common {
 	int (*open)(struct ieee80211_hw *dev);
 	void (*stop)(struct ieee80211_hw *dev);
 	int mode;
-	u16 seqno;
 	struct mutex conf_mutex;
 	u8 mac_addr[ETH_ALEN];
 	u8 bssid[ETH_ALEN];

Re: [PATCH] p54: Fix for TX sequence number problem that resulted from commit 741b4fbc44

[Larry Finger]

Chr wrote:
> On Saturday 16 August 2008 02:34:38 Larry Finger wrote:
>> This one lasted for 4 hours. What was worse is that it crashed my
>> computer with a 1 Hz blink of the Caps Lock light when it failed. I
>> hadn't seen that before.
> 
> hmm,.... sounds like a bit of a old problem with the minipci card...
> p54_rx_frame_sent walks in the tx_queue without holding the tx_queue.lock.
> 
> However, this problem is next to impossible to trigger in the current code.
> So, a crash log from a serial console would be great?

This one locked up the machine. I don't know how long it lasted as it 
was overnight. This time, the caps lock was not blinking, but the only 
recovery was to power it off.

A serial console is not possible - no hardware. In this case, 
netconsole is no help either as it would need to use the hardware 
whose driver has already failed.

Larry

Re: [PATCH] p54: Fix for TX sequence number problem that resulted from commit 741b4fbc44

[Chr]

On Saturday 16 August 2008 16:00:27 Larry Finger wrote:
> Chr wrote:
> > On Saturday 16 August 2008 02:34:38 Larry Finger wrote:
> >> This one lasted for 4 hours. What was worse is that it crashed my
> >> computer with a 1 Hz blink of the Caps Lock light when it failed. I
> >> hadn't seen that before.
> >
> > hmm,.... sounds like a bit of a old problem with the minipci card...
> > p54_rx_frame_sent walks in the tx_queue without holding the
> > tx_queue.lock.
> >
> > However, this problem is next to impossible to trigger in the current
> > code. So, a crash log from a serial console would be great?
>
> This one locked up the machine. I don't know how long it lasted as it
> was overnight. This time, the caps lock was not blinking, but the only
> recovery was to power it off.
hmm, so the system doesn't respond to sysrq-p.. bad?
one last thing, replace

-	return NETDEV_TX_BUSY;
with
+	return 0;

in p54_tx right at the top... it seems that ieee80211_stop_queue
doesn't work properly.

On the other hand. we might get a new driver, or atleast some
some help with the softmac part from a "company that sells this devices"
in the foreseeable future.

Regards,
	Chr