From: Christian Lamparter <[email protected]>
For p54pci and p54usb, skb_get_queue_mapping does not work correctly for
multiple queues.
Signed-off-by: Christian Lamparter <[email protected]>
Acked by: Larry Finger <[email protected]>
CC: [email protected]
---
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;
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
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 <[email protected]>
---
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));
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.
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.
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 <[email protected]>
>
> 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
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
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 <[email protected]>
>
These changes fix the problem on my system. If it is made into a proper patch,
I'll ACK it.
Larry
On Thu, 2008-08-07 at 08:21 -0500, Larry Finger wrote:
> From: Christian Lamparter <[email protected]>
>
> For p54pci and p54usb, skb_get_queue_mapping does not work correctly for
> multiple queues.
>
> Signed-off-by: Christian Lamparter <[email protected]>
> Acked by: Larry Finger <[email protected]>
> CC: [email protected]
> ---
>
> 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
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 <[email protected]>
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
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
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
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