Hi John,
Here are 3 patches for rt2x00, they are all intended for 2.6.26 since
they fix serious bugs. Patch 1 should prevent problems as reported by
Johannes Berg regarding skb->truesize by removing the only pskb_expand_head()
call in rt2x00 (and use a already existing buffer instead).
Patches 2 and 3 fix problems when DMA allocation fails, that usually doesn't happen,
hence the reason these problems have gone unnoticed for so long. :S
Ivo
---
The following changes since commit 0bacadffc86bcba2fdf2b989afc286a8184664a0:
Ivo van Doorn (1):
rt2x00: Fix quality/activity led handling
are available in the git repository at:
git://git.kernel.org/pub/scm/linux/kernel/git/ivd/rt2x00.git/ upstream
Gertjan van Wingerde (1):
rt2x00: Clean up error handling of PCI queue DMA allocation.
Ivo van Doorn (2):
rt2x00: Don't use pskb_expand_head()
rt2x00: Fix broken recover-on-error path
drivers/net/wireless/rt2x00/rt2x00dev.c | 11 ++++-------
drivers/net/wireless/rt2x00/rt2x00pci.c | 5 +++--
drivers/net/wireless/rt2x00/rt61pci.c | 31 +++++++++++--------------------
3 files changed, 18 insertions(+), 29 deletions(-)
On Friday 02 May 2008, Scott White wrote:
>
> >Was this patch series in time for the 2.6.26 merge window?
> >
> >If not, please consider merging at least the following patches since
> >they are real bugfixes:
> >
> >> rt2x00: Don't enable short preamble for 1MBs
> >> rt2x00: Fix quality/activity led handling
> >> rt2x00: Clarify supported chipsets in Kconfig
> >
> >The others are code cleanups which are "nice to have", but could otherwise
> >wait for 2.6.27.
>
> The merge window is still open at this time. I haven't seen a rc1 yet. Anyway I saw that only the 3 bugfixes are currently in the queue. As the owner of several (10 by the university and 2 personal) rt73 usb wireless cards, I would really like it if the latest version 2.1.5 made into 2.6.26. As someone who has used 2.6.24 and finished compiling 2.6.25 two days ago I find that the driver in both kernels is not good. Not good means I get a wireless signal and I get data transfers, but the speed fluctuates and have other issues. However with each release, the problems I have are slowing decreasing. I have been waiting patiently for 2.6.25-rc1 to have 2.1.5 since 2.1.5 was announced. I really think all of 2.1.5 should get in 2.6.25-rc1.
Have you tried forcing the rate to 54Mbit instead of letting the rate selection algorithm doing its work?
It seems to have helped with several other users.
Ivo
> Hmm, perhaps we need some way the driver can inform the rate selection
> module about statistics. I mean the hardware does measure the number
> of failed TX frames, and the number of retries, but it doesn't set them
> in the descriptor but in the hardware.
Yes we should have this.
johannes
> > > For rt73? IIRC the rate selection algo does not work at all for that
> > > device, because we cannot report failed frames. Or have you done
> > > anything about that I might have missed, Ivo? If not, the rate is
> > > probably fixed at 54Mbit anyway.
> >
> > Well I didn't do anything new, but this line will report rt2x00lib that the
> > frame was succesfully send or failed to send:
> > txdesc.status = !urb->status ? TX_SUCCESS : TX_FAIL_RETRY;
> > What is missing is the retry count, since that is something that
> > the USB drivers cannot report. (Unless I grab the max retry count
> > as configured by mac80211 as the number of retries on failure)
>
> Well, I have just put my rt73 into a metal box. Many frames do fail,
> only very few get through pinging my AP. The rate control still keeps
> the rate at 54Mbit/s :-) So it seems neither excessive_retries nor the
> retry_count in the tx_status struct is set at all.
Hmm, perhaps we need some way the driver can inform the rate selection
module about statistics. I mean the hardware does measure the number
of failed TX frames, and the number of retries, but it doesn't set them
in the descriptor but in the hardware.
Ivo
On Sat, 2008-05-03 at 11:58 +0200, Ivo van Doorn wrote:
> On Saturday 03 May 2008, Mattias Nissler wrote:
> > On Fri, 2008-05-02 at 23:25 +0200, Ivo van Doorn wrote:
> > > On Friday 02 May 2008, Scott White wrote:
> > > >
> > > > >Was this patch series in time for the 2.6.26 merge window?
> > > > >
> > > > >If not, please consider merging at least the following patches since
> > > > >they are real bugfixes:
> > > > >
> > > > >> rt2x00: Don't enable short preamble for 1MBs
> > > > >> rt2x00: Fix quality/activity led handling
> > > > >> rt2x00: Clarify supported chipsets in Kconfig
> > > > >
> > > > >The others are code cleanups which are "nice to have", but could otherwise
> > > > >wait for 2.6.27.
> > > >
> > > > The merge window is still open at this time. I haven't seen a rc1 yet. Anyway I saw that only the 3 bugfixes are currently in the queue. As the owner of several (10 by the university and 2 personal) rt73 usb wireless cards, I would really like it if the latest version 2.1.5 made into 2.6.26. As someone who has used 2.6.24 and finished compiling 2.6.25 two days ago I find that the driver in both kernels is not good. Not good means I get a wireless signal and I get data transfers, but the speed fluctuates and have other issues. However with each release, the problems I have are slowing decreasing. I have been waiting patiently for 2.6.25-rc1 to have 2.1.5 since 2.1.5 was announced. I really think all of 2.1.5 should get in 2.6.25-rc1.
> > >
> > > Have you tried forcing the rate to 54Mbit instead of letting the rate selection algorithm doing its work?
> > > It seems to have helped with several other users.
> >
> > For rt73? IIRC the rate selection algo does not work at all for that
> > device, because we cannot report failed frames. Or have you done
> > anything about that I might have missed, Ivo? If not, the rate is
> > probably fixed at 54Mbit anyway.
>
> Well I didn't do anything new, but this line will report rt2x00lib that the
> frame was succesfully send or failed to send:
> txdesc.status = !urb->status ? TX_SUCCESS : TX_FAIL_RETRY;
> What is missing is the retry count, since that is something that
> the USB drivers cannot report. (Unless I grab the max retry count
> as configured by mac80211 as the number of retries on failure)
Well, I have just put my rt73 into a metal box. Many frames do fail,
only very few get through pinging my AP. The rate control still keeps
the rate at 54Mbit/s :-) So it seems neither excessive_retries nor the
retry_count in the tx_status struct is set at all.
Mattias
During initialization the initialize() callback function
in rt2x00pci and rt2x00usb will cleanup the mess they made.
rt2x00lib shouldn't call uninitialize because the callback function already
cleaned up _and_ the DEVICE_INITIALIZED isn't set which causes the
rt2x00lib_uninitialize() to halt directly anyway. All that is required
to be cleaned up by rt2x00lib is the queue, and that can be done by
calling rt2x00queue_uninitialize() directly.
Signed-off-by: Ivo van Doorn <[email protected]>
---
drivers/net/wireless/rt2x00/rt2x00dev.c | 11 ++++-------
1 files changed, 4 insertions(+), 7 deletions(-)
diff --git a/drivers/net/wireless/rt2x00/rt2x00dev.c b/drivers/net/wireless/rt2x00/rt2x00dev.c
index 9929b15..61510c5 100644
--- a/drivers/net/wireless/rt2x00/rt2x00dev.c
+++ b/drivers/net/wireless/rt2x00/rt2x00dev.c
@@ -1028,8 +1028,10 @@ static int rt2x00lib_initialize(struct rt2x00_dev *rt2x00dev)
* Initialize the device.
*/
status = rt2x00dev->ops->lib->initialize(rt2x00dev);
- if (status)
- goto exit;
+ if (status) {
+ rt2x00queue_uninitialize(rt2x00dev);
+ return status;
+ }
__set_bit(DEVICE_INITIALIZED, &rt2x00dev->flags);
@@ -1039,11 +1041,6 @@ static int rt2x00lib_initialize(struct rt2x00_dev *rt2x00dev)
rt2x00rfkill_register(rt2x00dev);
return 0;
-
-exit:
- rt2x00lib_uninitialize(rt2x00dev);
-
- return status;
}
int rt2x00lib_start(struct rt2x00_dev *rt2x00dev)
--
1.5.5.1
On Saturday 03 May 2008, Mattias Nissler wrote:
> On Sat, 2008-05-03 at 17:27 +0200, Johannes Berg wrote:
> > > Hmm, perhaps we need some way the driver can inform the rate selection
> > > module about statistics. I mean the hardware does measure the number
> > > of failed TX frames, and the number of retries, but it doesn't set them
> > > in the descriptor but in the hardware.
> >
> > Yes we should have this.
>
> New rate control algorithm probably. Or even better: Split what is
> currently the rate control algorithm into an link quality estimation
> part (failed frames percentage and such) and the actual rate control
> that interprets that value and acts accordingly.
Hmm, splitting that up would be nice, especially when the driver
will have access to those failed frame percentages directly.
rt2x00 currently keeps track of these statistics to perform link tuning,
I wouldn't be surprised if other drivers also keep similar statistics for
the same reason.
Ivo
>Was this patch series in time for the 2.6.26 merge window?
>
>If not, please consider merging at least the following patches since
>they are real bugfixes:
>
>> rt2x00: Don't enable short preamble for 1MBs
>> rt2x00: Fix quality/activity led handling
>> rt2x00: Clarify supported chipsets in Kconfig
>
>The others are code cleanups which are "nice to have", but could otherwise
>wait for 2.6.27.
The merge window is still open at this time. I haven't seen a rc1 yet. Anyway I saw that only the 3 bugfixes are currently in the queue. As the owner of several (10 by the university and 2 personal) rt73 usb wireless cards, I would really like it if the latest version 2.1.5 made into 2.6.26. As someone who has used 2.6.24 and finished compiling 2.6.25 two days ago I find that the driver in both kernels is not good. Not good means I get a wireless signal and I get data transfers, but the speed fluctuates and have other issues. However with each release, the problems I have are slowing decreasing. I have been waiting patiently for 2.6.25-rc1 to have 2.1.5 since 2.1.5 was announced. I really think all of 2.1.5 should get in 2.6.25-rc1.
Thanks,
_________________________________________________________________
Windows Live SkyDrive lets you share files with faraway friends.
http://www.windowslive.com/skydrive/overview.html?ocid=TXT_TAGLM_WL_Refresh_skydrive_052008
> To: [email protected]
> Subject: Re: Please pull 'upstream' branch of rt2x00
> Date: Fri, 2 May 2008 23:25:38 +0200
> CC: [email protected]
> From: [email protected]
>
> On Friday 02 May 2008, Scott White wrote:
>>
>>>Was this patch series in time for the 2.6.26 merge window?
>>>
>>>If not, please consider merging at least the following patches since
>>>they are real bugfixes:
>>>
>>>> rt2x00: Don't enable short preamble for 1MBs
>>>> rt2x00: Fix quality/activity led handling
>>>> rt2x00: Clarify supported chipsets in Kconfig
>>>
>>>The others are code cleanups which are "nice to have", but could otherwise
>>>wait for 2.6.27.
>>
>> The merge window is still open at this time. I haven't seen a rc1 yet. Anyway I saw that only the 3 bugfixes are currently in the queue. As the owner of several (10 by the university and 2 personal) rt73 usb wireless cards, I would really like it if the latest version 2.1.5 made into 2.6.26. As someone who has used 2.6.24 and finished compiling 2.6.25 two days ago I find that the driver in both kernels is not good. Not good means I get a wireless signal and I get data transfers, but the speed fluctuates and have other issues. However with each release, the problems I have are slowing decreasing. I have been waiting patiently for 2.6.25-rc1 to have 2.1.5 since 2.1.5 was announced. I really think all of 2.1.5 should get in 2.6.25-rc1.
>
> Have you tried forcing the rate to 54Mbit instead of letting the rate selection algorithm doing its work?
> It seems to have helped with several other users.
>
> Ivo
No I haven't tried that. However I have 54Mbit (g) at home, but only 11Mbit (b only) at work. I guess I would have to change the rate when I change locations.
Thanks,
_________________________________________________________________
Make Windows Vista more reliable and secure with Windows Vista Service Pack 1.
http://www.windowsvista.com/SP1?WT.mc_id=hotmailvistasp1banner
On Fri, May 02, 2008 at 04:04:52PM -0500, Scott White wrote:
>
> >Was this patch series in time for the 2.6.26 merge window?
> >
> >If not, please consider merging at least the following patches since
> >they are real bugfixes:
> >
> >> rt2x00: Don't enable short preamble for 1MBs
> >> rt2x00: Fix quality/activity led handling
> >> rt2x00: Clarify supported chipsets in Kconfig
> >
> >The others are code cleanups which are "nice to have", but could otherwise
> >wait for 2.6.27.
>
> The merge window is still open at this time. I haven't seen
> a rc1 yet. Anyway I saw that only the 3 bugfixes are currently
> in the queue. As the owner of several (10 by the university and
> 2 personal) rt73 usb wireless cards, I would really like it if the
> latest version 2.1.5 made into 2.6.26. As someone who has used 2.6.24
> and finished compiling 2.6.25 two days ago I find that the driver in
> both kernels is not good. Not good means I get a wireless signal and
> I get data transfers, but the speed fluctuates and have other issues.
> However with each release, the problems I have are slowing decreasing.
> I have been waiting patiently for 2.6.25-rc1 to have 2.1.5 since 2.1.5
> was announced. I really think all of 2.1.5 should get in 2.6.25-rc1.
I'm sorry, but this isn't how the merge window is used. The merge
window is for merging code that is _already_ pending when the
window opens. No one should wait for the merge window to open before
sending code to the upstream maintainers. (I'm not suggesting that
Ivo delayed on purpose, I'm just stating the policy.)
Anything that fixes an actual bug can still go to 2.6.26 over the
next several weeks. But code that is simply new development will
have to wait for 2.6.27's merge window before it gets to Linus.
Thanks,
John
--
John W. Linville
[email protected]
On Sat, 2008-05-03 at 17:27 +0200, Johannes Berg wrote:
> > Hmm, perhaps we need some way the driver can inform the rate selection
> > module about statistics. I mean the hardware does measure the number
> > of failed TX frames, and the number of retries, but it doesn't set them
> > in the descriptor but in the hardware.
>
> Yes we should have this.
New rate control algorithm probably. Or even better: Split what is
currently the rate control algorithm into an link quality estimation
part (failed frames percentage and such) and the actual rate control
that interprets that value and acts accordingly.
Mattias
On Fri, 2008-05-02 at 23:25 +0200, Ivo van Doorn wrote:
> On Friday 02 May 2008, Scott White wrote:
> >
> > >Was this patch series in time for the 2.6.26 merge window?
> > >
> > >If not, please consider merging at least the following patches since
> > >they are real bugfixes:
> > >
> > >> rt2x00: Don't enable short preamble for 1MBs
> > >> rt2x00: Fix quality/activity led handling
> > >> rt2x00: Clarify supported chipsets in Kconfig
> > >
> > >The others are code cleanups which are "nice to have", but could otherwise
> > >wait for 2.6.27.
> >
> > The merge window is still open at this time. I haven't seen a rc1 yet. Anyway I saw that only the 3 bugfixes are currently in the queue. As the owner of several (10 by the university and 2 personal) rt73 usb wireless cards, I would really like it if the latest version 2.1.5 made into 2.6.26. As someone who has used 2.6.24 and finished compiling 2.6.25 two days ago I find that the driver in both kernels is not good. Not good means I get a wireless signal and I get data transfers, but the speed fluctuates and have other issues. However with each release, the problems I have are slowing decreasing. I have been waiting patiently for 2.6.25-rc1 to have 2.1.5 since 2.1.5 was announced. I really think all of 2.1.5 should get in 2.6.25-rc1.
>
> Have you tried forcing the rate to 54Mbit instead of letting the rate selection algorithm doing its work?
> It seems to have helped with several other users.
For rt73? IIRC the rate selection algo does not work at all for that
device, because we cannot report failed frames. Or have you done
anything about that I might have missed, Ivo? If not, the rate is
probably fixed at 54Mbit anyway.
Mattias
rt2x00pci allocates DMA for descriptor and data,
rt61pci doesn't use this for the beacon, but it can
use the descriptor part as temporary buffer instead
of using pskb_expand_head().
Using this temporary buffer is obviously much better
then reallocating the skb buffer...
At the same time we can set the data length for the
beacon queue at 0, to make sure no DMA is allocated for
data (but just for the descriptor).
Signed-off-by: Ivo van Doorn <[email protected]>
---
drivers/net/wireless/rt2x00/rt61pci.c | 31 +++++++++++--------------------
1 files changed, 11 insertions(+), 20 deletions(-)
diff --git a/drivers/net/wireless/rt2x00/rt61pci.c b/drivers/net/wireless/rt2x00/rt61pci.c
index 98af4d2..b64f2f5 100644
--- a/drivers/net/wireless/rt2x00/rt61pci.c
+++ b/drivers/net/wireless/rt2x00/rt61pci.c
@@ -2362,6 +2362,7 @@ static int rt61pci_beacon_update(struct ieee80211_hw *hw, struct sk_buff *skb,
{
struct rt2x00_dev *rt2x00dev = hw->priv;
struct rt2x00_intf *intf = vif_to_intf(control->vif);
+ struct queue_entry_priv_pci_tx *priv_tx;
struct skb_frame_desc *skbdesc;
unsigned int beacon_base;
u32 reg;
@@ -2369,21 +2370,8 @@ static int rt61pci_beacon_update(struct ieee80211_hw *hw, struct sk_buff *skb,
if (unlikely(!intf->beacon))
return -ENOBUFS;
- /*
- * We need to append the descriptor in front of the
- * beacon frame.
- */
- if (skb_headroom(skb) < intf->beacon->queue->desc_size) {
- if (pskb_expand_head(skb, intf->beacon->queue->desc_size,
- 0, GFP_ATOMIC))
- return -ENOMEM;
- }
-
- /*
- * Add the descriptor in front of the skb.
- */
- skb_push(skb, intf->beacon->queue->desc_size);
- memset(skb->data, 0, intf->beacon->queue->desc_size);
+ priv_tx = intf->beacon->priv_data;
+ memset(priv_tx->desc, 0, intf->beacon->queue->desc_size);
/*
* Fill in skb descriptor
@@ -2391,9 +2379,9 @@ static int rt61pci_beacon_update(struct ieee80211_hw *hw, struct sk_buff *skb,
skbdesc = get_skb_frame_desc(skb);
memset(skbdesc, 0, sizeof(*skbdesc));
skbdesc->flags |= FRAME_DESC_DRIVER_GENERATED;
- skbdesc->data = skb->data + intf->beacon->queue->desc_size;
- skbdesc->data_len = skb->len - intf->beacon->queue->desc_size;
- skbdesc->desc = skb->data;
+ skbdesc->data = skb->data;
+ skbdesc->data_len = skb->len;
+ skbdesc->desc = priv_tx->desc;
skbdesc->desc_len = intf->beacon->queue->desc_size;
skbdesc->entry = intf->beacon;
@@ -2414,7 +2402,10 @@ static int rt61pci_beacon_update(struct ieee80211_hw *hw, struct sk_buff *skb,
rt2x00lib_write_tx_desc(rt2x00dev, skb, control);
beacon_base = HW_BEACON_OFFSET(intf->beacon->entry_idx);
rt2x00pci_register_multiwrite(rt2x00dev, beacon_base,
- skb->data, skb->len);
+ skbdesc->desc, skbdesc->desc_len);
+ rt2x00pci_register_multiwrite(rt2x00dev,
+ beacon_base + skbdesc->desc_len,
+ skbdesc->data, skbdesc->data_len);
rt61pci_kick_tx_queue(rt2x00dev, QID_BEACON);
return 0;
@@ -2479,7 +2470,7 @@ static const struct data_queue_desc rt61pci_queue_tx = {
static const struct data_queue_desc rt61pci_queue_bcn = {
.entry_num = 4 * BEACON_ENTRIES,
- .data_size = MGMT_FRAME_SIZE,
+ .data_size = 0, /* No DMA required for beacons */
.desc_size = TXINFO_SIZE,
.priv_size = sizeof(struct queue_entry_priv_pci_tx),
};
--
1.5.5.1
On Saturday 03 May 2008, Mattias Nissler wrote:
> On Fri, 2008-05-02 at 23:25 +0200, Ivo van Doorn wrote:
> > On Friday 02 May 2008, Scott White wrote:
> > >
> > > >Was this patch series in time for the 2.6.26 merge window?
> > > >
> > > >If not, please consider merging at least the following patches since
> > > >they are real bugfixes:
> > > >
> > > >> rt2x00: Don't enable short preamble for 1MBs
> > > >> rt2x00: Fix quality/activity led handling
> > > >> rt2x00: Clarify supported chipsets in Kconfig
> > > >
> > > >The others are code cleanups which are "nice to have", but could otherwise
> > > >wait for 2.6.27.
> > >
> > > The merge window is still open at this time. I haven't seen a rc1 yet. Anyway I saw that only the 3 bugfixes are currently in the queue. As the owner of several (10 by the university and 2 personal) rt73 usb wireless cards, I would really like it if the latest version 2.1.5 made into 2.6.26. As someone who has used 2.6.24 and finished compiling 2.6.25 two days ago I find that the driver in both kernels is not good. Not good means I get a wireless signal and I get data transfers, but the speed fluctuates and have other issues. However with each release, the problems I have are slowing decreasing. I have been waiting patiently for 2.6.25-rc1 to have 2.1.5 since 2.1.5 was announced. I really think all of 2.1.5 should get in 2.6.25-rc1.
> >
> > Have you tried forcing the rate to 54Mbit instead of letting the rate selection algorithm doing its work?
> > It seems to have helped with several other users.
>
> For rt73? IIRC the rate selection algo does not work at all for that
> device, because we cannot report failed frames. Or have you done
> anything about that I might have missed, Ivo? If not, the rate is
> probably fixed at 54Mbit anyway.
Well I didn't do anything new, but this line will report rt2x00lib that the
frame was succesfully send or failed to send:
txdesc.status = !urb->status ? TX_SUCCESS : TX_FAIL_RETRY;
What is missing is the retry count, since that is something that
the USB drivers cannot report. (Unless I grab the max retry count
as configured by mac80211 as the number of retries on failure)
Ivo
When, for some reason, the rt2x00pci module fails to allocate DMA memory for
the queues, it tries to undo the complete initialization of the PCI device,
including freeing of the irq. This results in the following error in dmesg, as
the irq hadn't been requested yet:
[ 78.123456] Trying to free already-free IRQ 17
Fix this by implementing proper error handling code, instead of just using the
full uninitialization function.
Signed-off-by: Gertjan van Wingerde <[email protected]>
Signed-off-by: Ivo van Doorn <[email protected]>
---
drivers/net/wireless/rt2x00/rt2x00pci.c | 5 +++--
1 files changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/net/wireless/rt2x00/rt2x00pci.c b/drivers/net/wireless/rt2x00/rt2x00pci.c
index 9e5d94e..c17078e 100644
--- a/drivers/net/wireless/rt2x00/rt2x00pci.c
+++ b/drivers/net/wireless/rt2x00/rt2x00pci.c
@@ -314,13 +314,14 @@ int rt2x00pci_initialize(struct rt2x00_dev *rt2x00dev)
if (status) {
ERROR(rt2x00dev, "IRQ %d allocation failed (error %d).\n",
pci_dev->irq, status);
- return status;
+ goto exit;
}
return 0;
exit:
- rt2x00pci_uninitialize(rt2x00dev);
+ queue_for_each(rt2x00dev, queue)
+ rt2x00pci_free_queue_dma(rt2x00dev, queue);
return status;
}
--
1.5.5.1