[PATCH] p54: Fix regression due to commit b19fa1f

[PATCH] p54: Fix regression due to commit b19fa1f

[Larry Finger]

In commit b19fa1fa91845234961c64dbd564671aa7c0fd27, the configuration
parameter NETDEVICES_MULTIQUEUE was eliminated making multiple TX queues
the normal behavior. For p54usb, enabling multiple queues broke the driver.

The real failure is not known, but a temporary hack that forces only one
queue is presented here.

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

John,

This is clearly not the best fix, but there has been no action so far, and
it does fix the b19fa1f regression. Let's push this up to 2.6.27.

Larry


Index: linux-2.6/drivers/net/wireless/p54/p54common.c
===================================================================
--- linux-2.6.orig/drivers/net/wireless/p54/p54common.c
+++ linux-2.6/drivers/net/wireless/p54/p54common.c
@@ -150,7 +150,7 @@ void p54_parse_firmware(struct ieee80211
 		priv->tx_stats[1].limit = 4;
 		priv->tx_stats[2].limit = 3;
 		priv->tx_stats[3].limit = 1;
-		dev->queues = 4;
+		dev->queues = 1; /* temp. hack, set to 1 as 4 breaks p54usb */
 	}
 }
 EXPORT_SYMBOL_GPL(p54_parse_firmware);

Re: [PATCH] p54: Fix regression due to commit b19fa1f

[Chr]

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

On Monday 04 August 2008 23:14:51 Larry Finger wrote:
> In commit b19fa1fa91845234961c64dbd564671aa7c0fd27, the configuration
> parameter NETDEVICES_MULTIQUEUE was eliminated making multiple TX queues
> the normal behavior. For p54usb, enabling multiple queues broke the driver.
>
> The real failure is not known, but a temporary hack that forces only one
> queue is presented here.
>

The real problem seems to be that skb_get_queue_mapping doesn't
work the way it should when we process the firmwares callback. It's
always "0" and unfortunately also when it should be something else like
queue 1, 2 or 3..... problem solved? 

However someone should really take a closer look at the multiqueue thing,
especially why it has to BLOCK/SPIN (uninterruptible?) when a queue is stopped 
and tx returns therefore NETDEV_BUSY. 
The is assumption that "a queue is in any case going to become free again" is
well-intentioned, but as my devices are crashing left & right on a daily basis 
it's even dangerous for my RAID ;-).
 
(BTW: patch is diffed against 2.6.27-rc2)

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

[-- Attachment #2: p54-multiscrew.diff --]
[-- Type: text/x-patch, Size: 825 bytes --]

--- a/drivers/net/wireless/p54/p54common.c.orig	2008-08-07 00:27:35.000000000 +0200
+++ b/drivers/net/wireless/p54/p54common.c	2008-08-07 00:30:38.000000000 +0200
@@ -413,12 +413,12 @@ static void p54_rx_frame_sent(struct iee
 			last_addr = range->end_addr;
 			__skb_unlink(entry, &priv->tx_queue);
 			memset(&info->status, 0, sizeof(info->status));
-			priv->tx_stats[skb_get_queue_mapping(skb)].len--;
 			entry_hdr = (struct p54_control_hdr *) entry->data;
 			entry_data = (struct p54_tx_control_allocdata *) entry_hdr->data;
 			if ((entry_hdr->magic1 & cpu_to_le16(0x4000)) != 0)
 				pad = entry_data->align[0];
 
+			priv->tx_stats[le32_to_cpu(entry_data->frame_type - 4)].len--;		
 			if (!(info->flags & IEEE80211_TX_CTL_NO_ACK)) {
 				if (!(payload->status & 0x01))
 					info->flags |= IEEE80211_TX_STAT_ACK;

Re: [PATCH] p54: Fix regression due to commit b19fa1f

[Larry Finger]

Chr wrote:
> On Monday 04 August 2008 23:14:51 Larry Finger wrote:
>> In commit b19fa1fa91845234961c64dbd564671aa7c0fd27, the configuration
>> parameter NETDEVICES_MULTIQUEUE was eliminated making multiple TX queues
>> the normal behavior. For p54usb, enabling multiple queues broke the driver.
>>
>> The real failure is not known, but a temporary hack that forces only one
>> queue is presented here.
>>
> 
> The real problem seems to be that skb_get_queue_mapping doesn't
> work the way it should when we process the firmwares callback. It's
> always "0" and unfortunately also when it should be something else like
> queue 1, 2 or 3..... problem solved? 
> 
> However someone should really take a closer look at the multiqueue thing,
> especially why it has to BLOCK/SPIN (uninterruptible?) when a queue is stopped 
> and tx returns therefore NETDEV_BUSY. 
> The is assumption that "a queue is in any case going to become free again" is
> well-intentioned, but as my devices are crashing left & right on a daily basis 
> it's even dangerous for my RAID ;-).
>  
> (BTW: patch is diffed against 2.6.27-rc2)
> 
> Signed-off-by: Christian Lamparter <chunkeey@web.de>
> 

These changes fix the problem on my system. If it is made into a proper patch, 
I'll ACK it.

Larry

Re: [PATCH] p54: Fix regression due to commit b19fa1f

[Chr]

On Thursday 07 August 2008 05:33:51 Larry Finger wrote:
>
> If it is made into a proper patch, I'll ACK it.
>
No problemo! but can you please tell me what you would like to see?
I don't spot anything, except some extra tab damage at the EOL.
That said, I didn't make much patches lately, so I probably need an bigger 
update about the latest "how-to-send-a-proper-patch" etiquette than I thought. 
;-)

Regards,
	Chr 

Re: [PATCH] p54: Fix regression due to commit b19fa1f

[Johannes Berg]

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

On Thu, 2008-08-07 at 00:57 +0200, Chr wrote:

> The real problem seems to be that skb_get_queue_mapping doesn't
> work the way it should when we process the firmwares callback. It's
> always "0" and unfortunately also when it should be something else like
> queue 1, 2 or 3..... problem solved? 

Oh, eh, sorry about that, of course it cannot return the right thing on
a _received_ frame. Thanks for finding.

johannes

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

[PATCH] p54: Fix regression due to commit b19fa1f

[Larry Finger]

From: Christian Lamparter <chunkeey@web.de>

For p54pci and p54usb, skb_get_queue_mapping does not work correctly for
multiple queues.

Signed-off-by: Christian Lamparter <chunkeey@web.de>
Acked by: Larry Finger <Larry.Finger@lwfinger.net>
CC: stable@kernel.org
---

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
@@ -413,12 +413,12 @@ static void p54_rx_frame_sent(struct iee
 			last_addr = range->end_addr;
 			__skb_unlink(entry, &priv->tx_queue);
 			memset(&info->status, 0, sizeof(info->status));
-			priv->tx_stats[skb_get_queue_mapping(skb)].len--;
 			entry_hdr = (struct p54_control_hdr *) entry->data;
 			entry_data = (struct p54_tx_control_allocdata *) entry_hdr->data;
 			if ((entry_hdr->magic1 & cpu_to_le16(0x4000)) != 0)
 				pad = entry_data->align[0];
 
+			priv->tx_stats[le32_to_cpu(entry_data->frame_type - 4)].len--;
 			if (!(info->flags & IEEE80211_TX_CTL_NO_ACK)) {
 				if (!(payload->status & 0x01))
 					info->flags |= IEEE80211_TX_STAT_ACK;

Re: [PATCH] p54: Fix regression due to commit b19fa1f

[Johannes Berg]

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

On Thu, 2008-08-07 at 08:21 -0500, Larry Finger wrote:
> From: Christian Lamparter <chunkeey@web.de>
> 
> For p54pci and p54usb, skb_get_queue_mapping does not work correctly for
> multiple queues.
> 
> Signed-off-by: Christian Lamparter <chunkeey@web.de>
> Acked by: Larry Finger <Larry.Finger@lwfinger.net>
> CC: stable@kernel.org
> ---
> 
> 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
> @@ -413,12 +413,12 @@ static void p54_rx_frame_sent(struct iee
>  			last_addr = range->end_addr;
>  			__skb_unlink(entry, &priv->tx_queue);
>  			memset(&info->status, 0, sizeof(info->status));
> -			priv->tx_stats[skb_get_queue_mapping(skb)].len--;
>  			entry_hdr = (struct p54_control_hdr *) entry->data;
>  			entry_data = (struct p54_tx_control_allocdata *) entry_hdr->data;
>  			if ((entry_hdr->magic1 & cpu_to_le16(0x4000)) != 0)
>  				pad = entry_data->align[0];
>  
> +			priv->tx_stats[le32_to_cpu(entry_data->frame_type - 4)].len--;

Sorry for not catching this earlier, but this looks wrong, the -4 surely
should be outside the endian conversion?

johannes

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

Re: [PATCH] p54: Fix regression due to commit b19fa1f

[Larry Finger]

Johannes Berg wrote:
>> +			priv->tx_stats[le32_to_cpu(entry_data->frame_type - 4)].len--;
> 
> Sorry for not catching this earlier, but this looks wrong, the -4 surely
> should be outside the endian conversion?

I thought mixed-endian arithmetic was the latest rage ;). I'll sent V2 right away.

Have a good vacation.

Larry

Re: [PATCH] p54: Fix regression due to commit b19fa1f

[Chr]

On Thursday 07 August 2008 15:27:51 Johannes Berg wrote:
> > +			priv->tx_stats[le32_to_cpu(entry_data->frame_type - 4)].len--;
>
> Sorry for not catching this earlier, but this looks wrong, the -4 surely
> should be outside the endian conversion?

likely... let's do the job properly for once and for all.
--

This patch fixes not only the regression which has sneaked in
with commit b19fa1f, but also provides some extra code for
cts rates.

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

---
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-07 16:11:43.000000000 +0200
+++ b/drivers/net/wireless/p54/p54common.c	2008-08-07 17:24:49.000000000 +0200
@@ -413,12 +413,12 @@ static void p54_rx_frame_sent(struct iee
 			last_addr = range->end_addr;
 			__skb_unlink(entry, &priv->tx_queue);
 			memset(&info->status, 0, sizeof(info->status));
-			priv->tx_stats[skb_get_queue_mapping(skb)].len--;
 			entry_hdr = (struct p54_control_hdr *) entry->data;
 			entry_data = (struct p54_tx_control_allocdata *) entry_hdr->data;
 			if ((entry_hdr->magic1 & cpu_to_le16(0x4000)) != 0)
 				pad = entry_data->align[0];
 
+			priv->tx_stats[entry_data->hw_queue - 4].len--;
 			if (!(info->flags & IEEE80211_TX_CTL_NO_ACK)) {
 				if (!(payload->status & 0x01))
 					info->flags |= IEEE80211_TX_STAT_ACK;
@@ -555,7 +555,7 @@ static int p54_tx(struct ieee80211_hw *d
 	struct p54_control_hdr *hdr;
 	struct p54_tx_control_allocdata *txhdr;
 	size_t padding, len;
-	u8 rate;
+	u8 rate, cts_rate = 0x20;
 
 	current_queue = &priv->tx_stats[skb_get_queue_mapping(skb)];
 	if (unlikely(current_queue->len > current_queue->limit))
@@ -580,28 +580,26 @@ static int p54_tx(struct ieee80211_hw *d
 	hdr->type = (info->flags & IEEE80211_TX_CTL_NO_ACK) ? 0 : cpu_to_le16(1);
 	hdr->retry1 = hdr->retry2 = info->control.retry_limit;
 
-	memset(txhdr->wep_key, 0x0, 16);
-	txhdr->padding = 0;
-	txhdr->padding2 = 0;
-
 	/* TODO: add support for alternate retry TX rates */
 	rate = ieee80211_get_tx_rate(dev, info)->hw_value;
-	if (info->flags & IEEE80211_TX_CTL_SHORT_PREAMBLE)
+	if (info->flags & IEEE80211_TX_CTL_SHORT_PREAMBLE) {
 		rate |= 0x10;
-	if (info->flags & IEEE80211_TX_CTL_USE_RTS_CTS)
+		cts_rate |= 0x10;
+	} if (info->flags & IEEE80211_TX_CTL_USE_RTS_CTS) {
 		rate |= 0x40;
-	else if (info->flags & IEEE80211_TX_CTL_USE_CTS_PROTECT)
+		cts_rate |= ieee80211_get_rts_cts_rate(dev, info)->hw_value;
+	} else if (info->flags & IEEE80211_TX_CTL_USE_CTS_PROTECT) {
 		rate |= 0x20;
+		cts_rate |= ieee80211_get_rts_cts_rate(dev, info)->hw_value;
+	}
 	memset(txhdr->rateset, rate, 8);
-	txhdr->wep_key_present = 0;
-	txhdr->wep_key_len = 0;
-	txhdr->frame_type = cpu_to_le32(skb_get_queue_mapping(skb) + 4);
-	txhdr->magic4 = 0;
-	txhdr->antenna = (info->antenna_sel_tx == 0) ?
+	txhdr->key_type = 0;
+	txhdr->key_len = 0;
+	txhdr->hw_queue = skb_get_queue_mapping(skb) + 4;
+	txhdr->tx_antenna = (info->antenna_sel_tx == 0) ?
 		2 : info->antenna_sel_tx - 1;
 	txhdr->output_power = 0x7f; // HW Maximum
-	txhdr->magic5 = (info->flags & IEEE80211_TX_CTL_NO_ACK) ?
-		0 : ((rate > 0x3) ? cpu_to_le32(0x33) : cpu_to_le32(0x23));
+	txhdr->cts_rate = (info->flags & IEEE80211_TX_CTL_NO_ACK) ? 0 : cts_rate;
 	if (padding)
 		txhdr->align[0] = padding;
 
diff -Nurp a/drivers/net/wireless/p54/p54common.h b/drivers/net/wireless/p54/p54common.h
--- a/drivers/net/wireless/p54/p54common.h	2008-08-07 16:10:20.000000000 +0200
+++ b/drivers/net/wireless/p54/p54common.h	2008-08-07 16:24:10.000000000 +0200
@@ -183,16 +183,18 @@ struct p54_frame_sent_hdr {
 
 struct p54_tx_control_allocdata {
 	u8 rateset[8];
-	u16 padding;
-	u8 wep_key_present;
-	u8 wep_key_len;
-	u8 wep_key[16];
-	__le32 frame_type;
-	u32 padding2;
-	__le16 magic4;
-	u8 antenna;
+	u16:16;
+	u8 key_type;
+	u8 key_len;
+	u8 key[16];
+	u8 hw_queue;
+	u32:24;
+	u32:32;
+	u16:16;
+	u8 tx_antenna;
 	u8 output_power;
-	__le32 magic5;
+	u8 cts_rate;
+	u32:24;
 	u8 align[0];
 } __attribute__ ((packed));
 

Re: [PATCH] p54: Fix regression due to commit b19fa1f

[Johannes Berg]

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

On Thu, 2008-08-07 at 17:27 +0200, Chr wrote:
> On Thursday 07 August 2008 15:27:51 Johannes Berg wrote:
> > > +			priv->tx_stats[le32_to_cpu(entry_data->frame_type - 4)].len--;
> >
> > Sorry for not catching this earlier, but this looks wrong, the -4 surely
> > should be outside the endian conversion?
> 
> likely... let's do the job properly for once and for all.
> --

> +			priv->tx_stats[entry_data->hw_queue - 4].len--;

> +	u8 hw_queue;

Ah, heh, so it's not a u32 field after all?

johannes

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

Re: [PATCH] p54: Fix regression due to commit b19fa1f

[Johannes Berg]

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

On Thu, 2008-08-07 at 17:27 +0200, Chr wrote:
> 
> +       u32:24;
> +       u32:32;
> +       u16:16;

That piece looks just wrong though. Bitfields, and in a hw struct?

johannes

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

Re: [PATCH] p54: Fix regression due to commit b19fa1f

[Chr]

On Thursday 07 August 2008 17:27:14 Johannes Berg wrote:
> On Thu, 2008-08-07 at 17:27 +0200, Chr wrote:
> > +       u32:24;
> > +       u32:32;
> > +       u16:16;
>
> That piece looks just wrong though. Bitfields, and in a hw struct?
well, this is a crude way to silence gcc... without the ":32"
it complains "warning: declaration does not declare anything".

Of course we/I can make lot's of u8 paddingZ[X] arrays,
but we'll run out of Z and X in the long run ;-).

Regards,
	Chr.

Re: [PATCH] p54: Fix regression due to commit b19fa1f

[Michael Buesch]

On Thursday 07 August 2008 18:00:57 Chr wrote:
> On Thursday 07 August 2008 17:27:14 Johannes Berg wrote:
> > On Thu, 2008-08-07 at 17:27 +0200, Chr wrote:
> > > +       u32:24;
> > > +       u32:32;
> > > +       u16:16;
> >
> > That piece looks just wrong though. Bitfields, and in a hw struct?
> well, this is a crude way to silence gcc... without the ":32"
> it complains "warning: declaration does not declare anything".
> 
> Of course we/I can make lot's of u8 paddingZ[X] arrays,
> but we'll run out of Z and X in the long run ;-).


You can use this magic macro:

#define P4D_BYT3S(magic, nr_bytes)	u8 __p4dding##magic[nr_bytes]
#define P4D_BYTES(line, nr_bytes)	P4D_BYT3S(line, nr_bytes)
/* Magic helper macro to pad structures. Ignore those above. It's magic. */
#define PAD_BYTES(nr_bytes)		P4D_BYTES( __LINE__ , (nr_bytes))


struct xyz {
	//foo
	PAD_BYTES(4); /* Pad 4 bytes */
	//bar
};

-- 
Greetings Michael.

Re: [PATCH] p54: Fix regression due to commit b19fa1f

[Larry Finger]

Chr wrote:
> On Thursday 07 August 2008 15:27:51 Johannes Berg wrote:
>>> +			priv->tx_stats[le32_to_cpu(entry_data->frame_type - 4)].len--;
>> Sorry for not catching this earlier, but this looks wrong, the -4 surely
>> should be outside the endian conversion?
> 
> likely... let's do the job properly for once and for all.
> --
> 
> This patch fixes not only the regression which has sneaked in
> with commit b19fa1f, but also provides some extra code for
> cts rates.
> 
> Signed-off-by: Christian Lamparter <chunkeey@web.de>

Is the extra code for cts rates a bug fix or a feature enhancement? If the 
latter, then this patch should be in two parts. The main reason is that fixes 
should go to stable as well as wireless-testing.

Larry

Re: [PATCH] p54: Fix regression due to commit b19fa1f

[Chr]

On Thursday 07 August 2008 17:59:26 Larry Finger wrote:
> Chr wrote:
> > On Thursday 07 August 2008 15:27:51 Johannes Berg wrote:
> >>> +			priv->tx_stats[le32_to_cpu(entry_data->frame_type - 4)].len--;
> >>
> >> Sorry for not catching this earlier, but this looks wrong, the -4 surely
> >> should be outside the endian conversion?
> >
> > likely... let's do the job properly for once and for all.
> > --
> >
> > This patch fixes not only the regression which has sneaked in
> > with commit b19fa1f, but also provides some extra code for
> > cts rates.
> >
> > Signed-off-by: Christian Lamparter <chunkeey@web.de>
>
> Is the extra code for cts rates a bug fix or a feature enhancement? If the
> latter, then this patch should be in two parts. The main reason is that
> fixes should go to stable as well as wireless-testing.
>
The old logic always tx'ed cts frames (if enabled) with a
short preamble when [rate > 3]. (read: with any 802.11g rate).
Of course this isn't that bad, but it's still wrong!

Regards,
	Chr