LinuxLists.cc - Warning emited by 2.6.24-rc6-git5

[permalink] [raw]

Subject: Re: Warning emited by 2.6.24-rc6-git5

On Saturday 29 December 2007, Johannes Berg wrote:
>
> > but since the payload should be aligned to a 4 byte boundrary anyway
>
> That's not true. The "payload" in terms of "802.11 frame" shouldn't be
> aligned on a four-byte boundary for QoS or 4-addr frames.
>
> > can't predict what header length the frame will contain. :(
>
> Well, good hardware inserts padding, cf.
> http://bcm-v4.sipsolutions.net/802.11/RX
>
> > Although it would safe the initial allocation of the DMA buffer...
> > I'll look into it to see if it would be useful.
>
> More importantly, it'd save you the copy if your hardware is decent and
> inserts two bytes padding for QoS/WDS frames.

Well Ralink doesn't seem to add this padding since this bug appeared,
remember all bytes from the DMA was copied to the skb buffer so if there
was any padding included it would have been copied as well. ;)

Anyway, I have worked on a fix for the padding and I'll commit it to
rt2x00.git first to see if anybody reports any problems with it before
sending it to wireless-dev.

Same goes for letting the device copy the data into the skb buffer directly. :)

Ivo

2007-12-30 11:50:01

[permalink] [raw]

Subject: Re: Warning emited by 2.6.24-rc6-git5

On Saturday 29 December 2007, Ivo van Doorn wrote:
> On Saturday 29 December 2007, Johannes Berg wrote:
> > > Well Ralink doesn't seem to add this padding since this bug appeared,
> > > remember all bytes from the DMA was copied to the skb buffer so if
> > > there was any padding included it would have been copied as well. ;)
> >
> > Not necessarily, Broadcom hardware adds the padding in front of the
> > 802.11 header so if you'd start copying with the 802.11 header you'd run
> > into the same thing. A quick look at the rt2x00pci.c file doesn't
> > suggest that there's anything variable about the RX header though so I
> > guess that indeed this may be a problem.
> >
> > > Anyway, I have worked on a fix for the padding and I'll commit it to
> > > rt2x00.git first to see if anybody reports any problems with it before
> > > sending it to wireless-dev.
> >

If it would help, I'd be happy to test your fix, but I'm not a git user, so
you would need to post a patch that I could use...

> > Great. I just posted a similar fix in the other thread for zd1211, does
> > that look similar to yours as well? Should we have a static inline with
> > this code "ieee80211_needs_padding()" or something?
>
> My code looks more like:
>
> header_size = ieee80211_get_hdrlen_from_skb(entry->skb);
> if (header_size % 4 == 2) {
> /*
> * Move entire frame 2 bytes to the front.
> */
> skb_push(entry->skb, 2);
> memmove(entry->skb->data, entry->skb->data + 2,
> entry->skb->len - 2);
> }
>
> Ivo

Chris

--
Beer is proof that God loves us and wants us to be happy - Benjamin Franklin

2007-12-30 22:01:44

[permalink] [raw]

Subject: Re: Warning emited by 2.6.24-rc6-git5

Hi,

> > Here is the patch against the latest vanilla kernel,
> > the previous piece of code was bugged since the header size check was
> > incorrect, but that is fixed in this one.
> >
>
> Thanks. You'll be pleased to here that with this patch, the warning has gone
> away and laptop's connection to my wireless network is still fine. I'll be
> using it over the New Year holiday. If anything negative happens I'll let you
> know.

excellent news, I have a few other patches currently under testing,
and if no big problems are reported with them rt2x00 2.6.14 will be
released at the end of this week.

Ivo

2007-12-29 09:54:30

[permalink] [raw]

Subject: Re: Warning emited by 2.6.24-rc6-git5

I didn't think that many drivers had this problem... The warning is
harmless on machines that are ok with unaligned memory accesses.

> WARNING: at net/mac80211/rx.c:1486 __ieee80211_rx()
> Pid: 0, comm: swapper Not tainted 2.6.24-rc6-git5 #11

comes from
/*
* Drivers are required to align the payload data to a four-byte
* boundary, so the last two bits of the address where it starts
* may not be set. The header is required to be directly before
* the payload data, padding like atheros hardware adds which is
* inbetween the 802.11 header and the payload is not supported,
* the driver is required to move the 802.11 header further back
* in that case.
*/
hdrlen = ieee80211_get_hdrlen(rx.fc);
WARN_ON_ONCE(((unsigned long)(skb->data + hdrlen)) & 3);

Although I'm starting to doubt my own sanity here with this sanity check
if so many drivers can trigger it.

johannes

Attachments:

signature.asc (828.00 B)
This is a digitally signed message part

2007-12-29 10:26:14

[permalink] [raw]

Subject: Re: Warning emited by 2.6.24-rc6-git5

> If it helps as reference, rt2x00 allocates and initializes the data as follows:
>
> skb_reserve(skb, NET_IP_ALIGN);
> skb_put(skb, desc.size); /* Always a multiple of 4 bytes */
> memcpy(skb->data, entry->data_addr, desc.size);
>
> Should there have done more for the aligning of the data?

Well, typically, an RX buffer is comprised of the following elements:

* device-specific RX header [typically fixed length]
(* sometimes padding)
* 802.11 header
(* sometimes padding)
* 802.11 payload

The problem is that the 802.11 header has variable length. I explained
that previously, please see
http://article.gmane.org/gmane.linux.network/78792

Maybe the reporter here has received WDS or QoS (but not combined)
frames for some reason in which case the hardware has to insert padding
somewhere to make it work.

johannes

Attachments:

signature.asc (828.00 B)
This is a digitally signed message part

2007-12-29 12:01:19

[permalink] [raw]

Subject: Re: Warning emited by 2.6.24-rc6-git5

On Sat, 2007-12-29 at 12:59 +0100, Johannes Berg wrote:
> > but since the payload should be aligned to a 4 byte boundrary anyway
>
> That's not true. The "payload" in terms of "802.11 frame" shouldn't be
> aligned on a four-byte boundary for QoS or 4-addr frames.

Eh, sorry. The start of the 802.11 header must be aligned on a four-byte
boundary for non-QoS/non-WDS/QoS+WDS frames and on +2 mod 4 for QoS/WDS
frames.

johannes

Attachments:

signature.asc (828.00 B)
This is a digitally signed message part

2007-12-29 11:59:44

[permalink] [raw]

Subject: Re: Warning emited by 2.6.24-rc6-git5

> but since the payload should be aligned to a 4 byte boundrary anyway

That's not true. The "payload" in terms of "802.11 frame" shouldn't be
aligned on a four-byte boundary for QoS or 4-addr frames.

> can't predict what header length the frame will contain. :(

Well, good hardware inserts padding, cf.
http://bcm-v4.sipsolutions.net/802.11/RX

> Although it would safe the initial allocation of the DMA buffer...
> I'll look into it to see if it would be useful.

More importantly, it'd save you the copy if your hardware is decent and
inserts two bytes padding for QoS/WDS frames.

johannes

Attachments:

signature.asc (828.00 B)
This is a digitally signed message part

2007-12-29 10:36:57

[permalink] [raw]

Subject: Re: Warning emited by 2.6.24-rc6-git5

> If it helps as reference, rt2x00 allocates and initializes the data as follows:
>
> skb_reserve(skb, NET_IP_ALIGN);
> skb_put(skb, desc.size); /* Always a multiple of 4 bytes */
> memcpy(skb->data, entry->data_addr, desc.size);

So why do you copy all the data anyway? You have DMA queues and no need
to not have the hardware dump the data into the frame to start with, no?

In any case, with wireless NET_IP_ALIGN is pretty useless unless you
want to add two to it and then align the result. Maybe the mac80211
warning should be relaxed to take NET_IP_ALIGN into account.

johannes

Attachments:

signature.asc (828.00 B)
This is a digitally signed message part

2007-12-29 11:47:39

[permalink] [raw]

Subject: Re: Warning emited by 2.6.24-rc6-git5

On Saturday 29 December 2007, Johannes Berg wrote:
>
> > If it helps as reference, rt2x00 allocates and initializes the data as follows:
> >
> > skb_reserve(skb, NET_IP_ALIGN);
> > skb_put(skb, desc.size); /* Always a multiple of 4 bytes */
> > memcpy(skb->data, entry->data_addr, desc.size);
>
> So why do you copy all the data anyway? You have DMA queues and no need
> to not have the hardware dump the data into the frame to start with, no?

True, if no further aligning would be required that would be true,
but since the payload should be aligned to a 4 byte boundrary anyway
this would mean we must use memcpy() in all cases anyway since we
can't predict what header length the frame will contain. :(

Although it would safe the initial allocation of the DMA buffer...
I'll look into it to see if it would be useful.

> In any case, with wireless NET_IP_ALIGN is pretty useless unless you
> want to add two to it and then align the result. Maybe the mac80211
> warning should be relaxed to take NET_IP_ALIGN into account.

Ok, I'll look into better aligning of the skb buffer then.

Ivo

2007-12-30 19:27:06

[permalink] [raw]

Subject: Re: Warning emited by 2.6.24-rc6-git5

On Sunday 30 December 2007, Ivo van Doorn wrote:
> On Sunday 30 December 2007, Chris Clayton wrote:
> > On Saturday 29 December 2007, Ivo van Doorn wrote:
> > > On Saturday 29 December 2007, Johannes Berg wrote:
> > > > > Well Ralink doesn't seem to add this padding since this bug
> > > > > appeared, remember all bytes from the DMA was copied to the skb
> > > > > buffer so if there was any padding included it would have been
> > > > > copied as well. ;)
> > > >
> > > > Not necessarily, Broadcom hardware adds the padding in front of the
> > > > 802.11 header so if you'd start copying with the 802.11 header you'd
> > > > run into the same thing. A quick look at the rt2x00pci.c file doesn't
> > > > suggest that there's anything variable about the RX header though so
> > > > I guess that indeed this may be a problem.
> > > >
> > > > > Anyway, I have worked on a fix for the padding and I'll commit it
> > > > > to rt2x00.git first to see if anybody reports any problems with it
> > > > > before sending it to wireless-dev.
> >
> > If it would help, I'd be happy to test your fix, but I'm not a git user,
> > so you would need to post a patch that I could use...
>
> Here is the patch against the latest vanilla kernel,
> the previous piece of code was bugged since the header size check was
> incorrect, but that is fixed in this one.
>

Thanks. You'll be pleased to here that with this patch, the warning has gone
away and laptop's connection to my wireless network is still fine. I'll be
using it over the New Year holiday. If anything negative happens I'll let you
know.

Thanks again.

Chris

> ----
>
> diff --git a/drivers/net/wireless/rt2x00/rt2x00dev.c
> b/drivers/net/wireless/rt2x00/rt2x00dev.c index ff399f8..4f8a37b 100644
> --- a/drivers/net/wireless/rt2x00/rt2x00dev.c
> +++ b/drivers/net/wireless/rt2x00/rt2x00dev.c
> @@ -386,6 +386,7 @@ void rt2x00lib_rxdone(struct data_entry *entry, struct
> sk_buff *skb, struct ieee80211_rx_status *rx_status =
> &rt2x00dev->rx_status;
> struct ieee80211_hw_mode *mode;
> struct ieee80211_rate *rate;
> + unsigned int header_size;
> unsigned int i;
> int val = 0;
>
> @@ -412,6 +413,26 @@ void rt2x00lib_rxdone(struct data_entry *entry, struct
> sk_buff *skb, }
> }
>
> + /*
> + * Properly align the ieee80211 frame and make sure the
> + * data behind the ieee80211 header is on a 4 byte boundrary.
> + */
> + header_size = ieee80211_get_hdrlen_from_skb(skb);
> + if (!header_size) {
> + /*
> + * Frame is too short to contain a valid header,
> + * drop the entire frame since it is useless.
> + */
> + kfree_skb(skb);
> + return;
> + } else if (header_size % 4 == 0) {
> + /*
> + * Move entire frame 2 bytes to the front.
> + */
> + skb_push(skb, 2);
> + memmove(skb->data, skb->data + 2, skb->len - 2);
> + }
> +
> rt2x00_update_link_rssi(&rt2x00dev->link, desc->rssi);
> rt2x00dev->link.rx_success++;
> rx_status->rate = val;

--
Beer is proof that God loves us and wants us to be happy - Benjamin Franklin

2007-12-29 10:21:16

[permalink] [raw]

Subject: Re: Warning emited by 2.6.24-rc6-git5

On Saturday 29 December 2007, Johannes Berg wrote:
> I didn't think that many drivers had this problem... The warning is
> harmless on machines that are ok with unaligned memory accesses.
>
> > WARNING: at net/mac80211/rx.c:1486 __ieee80211_rx()
> > Pid: 0, comm: swapper Not tainted 2.6.24-rc6-git5 #11
>
> comes from
> /*
> * Drivers are required to align the payload data to a four-byte
> * boundary, so the last two bits of the address where it starts
> * may not be set. The header is required to be directly before
> * the payload data, padding like atheros hardware adds which is
> * inbetween the 802.11 header and the payload is not supported,
> * the driver is required to move the 802.11 header further back
> * in that case.
> */
> hdrlen = ieee80211_get_hdrlen(rx.fc);
> WARN_ON_ONCE(((unsigned long)(skb->data + hdrlen)) & 3);
>
>
> Although I'm starting to doubt my own sanity here with this sanity check
> if so many drivers can trigger it.

If it helps as reference, rt2x00 allocates and initializes the data as follows:

skb_reserve(skb, NET_IP_ALIGN);
skb_put(skb, desc.size); /* Always a multiple of 4 bytes */
memcpy(skb->data, entry->data_addr, desc.size);

Should there have done more for the aligning of the data?

Ivo

2007-12-29 14:40:22

[permalink] [raw]

Subject: Re: Warning emited by 2.6.24-rc6-git5

> Well Ralink doesn't seem to add this padding since this bug appeared,
> remember all bytes from the DMA was copied to the skb buffer so if there
> was any padding included it would have been copied as well. ;)

Not necessarily, Broadcom hardware adds the padding in front of the
802.11 header so if you'd start copying with the 802.11 header you'd run
into the same thing. A quick look at the rt2x00pci.c file doesn't
suggest that there's anything variable about the RX header though so I
guess that indeed this may be a problem.

> Anyway, I have worked on a fix for the padding and I'll commit it to
> rt2x00.git first to see if anybody reports any problems with it before
> sending it to wireless-dev.

Great. I just posted a similar fix in the other thread for zd1211, does
that look similar to yours as well? Should we have a static inline with
this code "ieee80211_needs_padding()" or something?

johannes

Attachments:

signature.asc (828.00 B)
This is a digitally signed message part

2007-12-29 15:14:40

[permalink] [raw]

Subject: Re: Warning emited by 2.6.24-rc6-git5

On Saturday 29 December 2007, Johannes Berg wrote:
>
> > Well Ralink doesn't seem to add this padding since this bug appeared,
> > remember all bytes from the DMA was copied to the skb buffer so if there
> > was any padding included it would have been copied as well. ;)
>
> Not necessarily, Broadcom hardware adds the padding in front of the
> 802.11 header so if you'd start copying with the 802.11 header you'd run
> into the same thing. A quick look at the rt2x00pci.c file doesn't
> suggest that there's anything variable about the RX header though so I
> guess that indeed this may be a problem.
>
> > Anyway, I have worked on a fix for the padding and I'll commit it to
> > rt2x00.git first to see if anybody reports any problems with it before
> > sending it to wireless-dev.
>
> Great. I just posted a similar fix in the other thread for zd1211, does
> that look similar to yours as well? Should we have a static inline with
> this code "ieee80211_needs_padding()" or something?

My code looks more like:

header_size = ieee80211_get_hdrlen_from_skb(entry->skb);
if (header_size % 4 == 2) {
/*
* Move entire frame 2 bytes to the front.
*/
skb_push(entry->skb, 2);
memmove(entry->skb->data, entry->skb->data + 2,
entry->skb->len - 2);
}

Ivo

2007-12-31 07:56:18

[permalink] [raw]

Subject: Re: Warning emited by 2.6.24-rc6-git5

On Dec 31, 2007 8:27 AM, Kalle Valo <[email protected]> wrote:
> Ivo van Doorn <[email protected]> writes:
>
> > + /*
> > + * Move entire frame 2 bytes to the front.
> > + */
> > + skb_push(skb, 2);
> > + memmove(skb->data, skb->data + 2, skb->len - 2);
>
> No skb_trim()? Shoudn't there be skb_trim(skb, 2) after memmove()?

Good point, I completely forgot about that. :S
Thanks. :)

Ivo

2007-12-30 12:11:50

[permalink] [raw]

Subject: Re: Warning emited by 2.6.24-rc6-git5

On Sunday 30 December 2007, Chris Clayton wrote:
> On Saturday 29 December 2007, Ivo van Doorn wrote:
> > On Saturday 29 December 2007, Johannes Berg wrote:
> > > > Well Ralink doesn't seem to add this padding since this bug appeared,
> > > > remember all bytes from the DMA was copied to the skb buffer so if
> > > > there was any padding included it would have been copied as well. ;)
> > >
> > > Not necessarily, Broadcom hardware adds the padding in front of the
> > > 802.11 header so if you'd start copying with the 802.11 header you'd run
> > > into the same thing. A quick look at the rt2x00pci.c file doesn't
> > > suggest that there's anything variable about the RX header though so I
> > > guess that indeed this may be a problem.
> > >
> > > > Anyway, I have worked on a fix for the padding and I'll commit it to
> > > > rt2x00.git first to see if anybody reports any problems with it before
> > > > sending it to wireless-dev.
> > >
>
> If it would help, I'd be happy to test your fix, but I'm not a git user, so
> you would need to post a patch that I could use...

Here is the patch against the latest vanilla kernel,
the previous piece of code was bugged since the header size check was incorrect,
but that is fixed in this one.

----

diff --git a/drivers/net/wireless/rt2x00/rt2x00dev.c b/drivers/net/wireless/rt2x00/rt2x00dev.c
index ff399f8..4f8a37b 100644
--- a/drivers/net/wireless/rt2x00/rt2x00dev.c
+++ b/drivers/net/wireless/rt2x00/rt2x00dev.c
@@ -386,6 +386,7 @@ void rt2x00lib_rxdone(struct data_entry *entry, struct sk_buff *skb,
struct ieee80211_rx_status *rx_status = &rt2x00dev->rx_status;
struct ieee80211_hw_mode *mode;
struct ieee80211_rate *rate;
+ unsigned int header_size;
unsigned int i;
int val = 0;

@@ -412,6 +413,26 @@ void rt2x00lib_rxdone(struct data_entry *entry, struct sk_buff *skb,
}
}

+ /*
+ * Properly align the ieee80211 frame and make sure the
+ * data behind the ieee80211 header is on a 4 byte boundrary.
+ */
+ header_size = ieee80211_get_hdrlen_from_skb(skb);
+ if (!header_size) {
+ /*
+ * Frame is too short to contain a valid header,
+ * drop the entire frame since it is useless.
+ */
+ kfree_skb(skb);
+ return;
+ } else if (header_size % 4 == 0) {
+ /*
+ * Move entire frame 2 bytes to the front.
+ */
+ skb_push(skb, 2);
+ memmove(skb->data, skb->data + 2, skb->len - 2);
+ }
+
rt2x00_update_link_rssi(&rt2x00dev->link, desc->rssi);
rt2x00dev->link.rx_success++;
rx_status->rate = val;

2007-12-31 07:28:17

by Kalle Valo

[permalink] [raw]

Subject: Re: Warning emited by 2.6.24-rc6-git5

Ivo van Doorn <[email protected]> writes:

> + /*
> + * Move entire frame 2 bytes to the front.
> + */
> + skb_push(skb, 2);
> + memmove(skb->data, skb->data + 2, skb->len - 2);

No skb_trim()? Shoudn't there be skb_trim(skb, 2) after memmove()?

--
Kalle Valo

2008-01-08 23:18:21

by Tomas Winkler

[permalink] [raw]

Subject: Re: Warning emited by 2.6.24-rc6-git5

On Jan 8, 2008 10:02 PM, Michael Buesch <[email protected]> wrote:
> On Tuesday 08 January 2008 20:43:53 Tomas Winkler wrote:
> > On Jan 8, 2008 7:22 PM, Johannes Berg <[email protected]> wrote:
> > >
> > > On Tue, 2008-01-08 at 18:09 +0100, Johannes Berg wrote:
> > > > > I didn't test the copy solution yet but in 11n rates I see as a show
> > > > > stopper. All 11n packets are QoS packets.
> > > >
> > > > Really, go fix your firmware.
> > >
> >
> > I don't think it is really broken. The packet it self is aligned. I'm
> > not sure who has to be responsible for payload alignment you may have
> > 10 other protocols packed into it, each with variable length
> > unaligned header.
>
> Yeah sure. You can move data around in the kernel and waste CPU,
> or you can be clever and let the NIC firmware do it. Whatever you want
> at intel. It's up to you. ;)

You are twisting my answer please read it again.
Frankly I will try to fix it in FW but if not I rather fail on 2
platforms then making 11n useless on 4 others.
Maybe we should ask some company to fix ther CPU to support unaligned
access. Or ask the guys that 'saved' two bytes in QoS field to change
the spec. Same goes for designers of 4965 that never dreamed someone
will stick it to SPARC machine. And I can bet the same goes for other
wireless HW companies.

Cheers, please don't replay to me I''m really going to try fix it :)

Tomas

> --
> Greetings Michael.
>

2008-01-02 19:13:03

[permalink] [raw]

Subject: Re: Warning emited by 2.6.24-rc6-git5

Hi,

> Happy New Year!

You too. :)

> I was out since Saturday just before you sent your
> message, sorry for the late reply.
>
> > header_size = ieee80211_get_hdrlen_from_skb(entry->skb);
> > if (header_size % 4 == 2) {
> > /*
> > * Move entire frame 2 bytes to the front.
> > */
> > skb_push(entry->skb, 2);
> > memmove(entry->skb->data, entry->skb->data + 2,
> > entry->skb->len - 2);
> > }
>
> That doesn't really look right, I'd think the skb will be two bytes too
> long after this.

Yeah, fixed this in rt2x00.git :)

> It's probably more efficient to decide where to copy
> the frame and do the realignment while you're copying it anyway rather
> than doing a copy and then a memmove.
>
> I guess you should also try talk to Ralink to get firmware to do it
> (where possible), it's probably not too hard to insert padding before
> the frame.

Well the problem is a bit bigger than this, the bug is reported very rarely,
so I can't say for sure which drivers are affected by this. But since most
Ralink chipsets look alike, I think the safest assumption is that all are
affected.

rt2400pci, rt2500pci and rt2500usb don't have firmware, for rt61pci and
rt73usb there is, but looking at how the TX and RX registers are working
I *assume* the firmware isn't doing much in this area. (My personal
guess for the firmware is that it only handles the hardware encryption
and perhaps beaconing & virtual interfaces)
Additionally firmware for rt61 and rt73 have been discontinued,
all their work is currently going to rt2860 and rt2870. (Porting these to
rt2x00 has already begun, but I haven't arrived at the header alignment yet)

For PCI drivers I could indeed optimize the code by making the memcpy
perform the operation on the 4 byte aligned code. But for this I need
to check what is better, RX directly into a skbuff and use memmove
or RX into DMA and use memcpy to a skbuff.
But for USB this is not possible, since it directly performs the RX into
the skbuff already, so I either have to change that or perform memmove anyway.

> If you absolutely can't get the hardware to do it and would otherwise do
> DMA right into the skb we should try to evaluate the performance hit on
> platforms where unaligned access *is* possible to be able to balance it
> against the performance hit caused by the memmove(). Ultimately, though,
> I value correctness on all platforms over performance on some, hence the
> warning when unaligned packets are handed up to mac80211.

Overall correctness sounds as the best solution to me too.

Ivo

2008-01-06 07:55:38

by Andrew Price

[permalink] [raw]

Subject: Re: Warning emited by 2.6.24-rc6-git5

On 04/01/08 17:09, Chris Clayton wrote:
> On Wednesday 02 January 2008, Ivo van Doorn wrote:
>> On Wednesday 02 January 2008, Chris Clayton wrote:
>>> Ah, does the patch I have applied and am running need enhancement,
>>> please?
>> ---
>>
>> diff --git a/drivers/net/wireless/rt2x00/rt2x00dev.c
>> b/drivers/net/wireless/rt2x00/rt2x00dev.c index ff399f8..3c10a68 100644
>> --- a/drivers/net/wireless/rt2x00/rt2x00dev.c
>> +++ b/drivers/net/wireless/rt2x00/rt2x00dev.c
>
> <snip>

I was seeing the same warning so I've applied the patch to the latest
mainline git tree. It's cured the warning and everything seems to be ok
so far. I'll let you know if I see any side effects. In case you're
interested I've got one of these:

02:00.0 Network controller: RaLink RT2500 802.11g Cardbus/mini-PCI (rev 01)
Subsystem: Belkin F5D7010 Wireless G Notebook Network Card
Flags: bus master, slow devsel, latency 64, IRQ 16
Memory at 54000000 (32-bit, non-prefetchable) [size=8K]
Capabilities: [40] Power Management version 2

Regards,

--
Andy Price

2008-01-02 06:32:55

[permalink] [raw]

Subject: Re: Warning emited by 2.6.24-rc6-git5

On Monday 31 December 2007, Ivo Van Doorn wrote:
> On Dec 31, 2007 8:27 AM, Kalle Valo <[email protected]> wrote:
> > Ivo van Doorn <[email protected]> writes:
> > > + /*
> > > + * Move entire frame 2 bytes to the front.
> > > + */
> > > + skb_push(skb, 2);
> > > + memmove(skb->data, skb->data + 2, skb->len - 2);
> >
> > No skb_trim()? Shoudn't there be skb_trim(skb, 2) after memmove()?
>
> Good point, I completely forgot about that. :S
> Thanks. :)
>
> Ivo

Ah, does the patch I have applied and am running need enhancement, please?

Thanks

--
Beer is proof that God loves us and wants us to be happy - Benjamin Franklin

2008-01-08 17:14:29

[permalink] [raw]

Subject: Re: Warning emited by 2.6.24-rc6-git5

> I didn't test the copy solution yet but in 11n rates I see as a show
> stopper. All 11n packets are QoS packets.

Really, go fix your firmware.

> I haven't encounter a problems in net stack yet so I think this
> solution is a bit drastic.

You're not running on arm or sparc I guess. But the fact that pretty
much *nobody* was aware of this problem until now tells me that I did
the right thing and this solution is not "a bit drastic" *at all*.

johannes

Attachments:

signature.asc (828.00 B)
This is a digitally signed message part

2008-01-08 17:23:11

[permalink] [raw]

Subject: Re: Warning emited by 2.6.24-rc6-git5

On Tue, 2008-01-08 at 18:09 +0100, Johannes Berg wrote:
> > I didn't test the copy solution yet but in 11n rates I see as a show
> > stopper. All 11n packets are QoS packets.
>
> Really, go fix your firmware.

I meant to write more here but accidentally deleted parts when
restructuring. The firmware should insert padding to make sure the
payload is aligned on a four-byte boundary at all times when the start
of the DMA area is aligned that way.

johannes

Attachments:

signature.asc (828.00 B)
This is a digitally signed message part

2008-01-08 19:43:54

by Tomas Winkler

[permalink] [raw]

Subject: Re: Warning emited by 2.6.24-rc6-git5

On Jan 8, 2008 7:22 PM, Johannes Berg <[email protected]> wrote:
>
> On Tue, 2008-01-08 at 18:09 +0100, Johannes Berg wrote:
> > > I didn't test the copy solution yet but in 11n rates I see as a show
> > > stopper. All 11n packets are QoS packets.
> >
> > Really, go fix your firmware.
>

I don't think it is really broken. The packet it self is aligned. I'm
not sure who has to be responsible for payload alignment you may have
10 other protocols packed into it, each with variable length
unaligned header.

> I meant to write more here but accidentally deleted parts when
> restructuring. The firmware should insert padding to make sure the
> payload is aligned on a four-byte boundary at all times when the start
> of the DMA area is aligned that way.
>
I understand that I'll see what can be done, yet I don't like it

Tomas

> johannes
>

2008-01-02 19:02:43

[permalink] [raw]

Subject: Re: Warning emited by 2.6.24-rc6-git5

On Wednesday 02 January 2008, Chris Clayton wrote:
> On Monday 31 December 2007, Ivo Van Doorn wrote:
> > On Dec 31, 2007 8:27 AM, Kalle Valo <[email protected]> wrote:
> > > Ivo van Doorn <[email protected]> writes:
> > > > + /*
> > > > + * Move entire frame 2 bytes to the front.
> > > > + */
> > > > + skb_push(skb, 2);
> > > > + memmove(skb->data, skb->data + 2, skb->len - 2);
> > >
> > > No skb_trim()? Shoudn't there be skb_trim(skb, 2) after memmove()?
> >
> > Good point, I completely forgot about that. :S
> > Thanks. :)
> >
> > Ivo
>
> Ah, does the patch I have applied and am running need enhancement, please?

---

diff --git a/drivers/net/wireless/rt2x00/rt2x00dev.c b/drivers/net/wireless/rt2x00/rt2x00dev.c
index ff399f8..3c10a68 100644
--- a/drivers/net/wireless/rt2x00/rt2x00dev.c
+++ b/drivers/net/wireless/rt2x00/rt2x00dev.c
@@ -386,6 +386,7 @@ void rt2x00lib_rxdone(struct data_entry *entry, struct sk_buff *skb,
struct ieee80211_rx_status *rx_status = &rt2x00dev->rx_status;
struct ieee80211_hw_mode *mode;
struct ieee80211_rate *rate;
+ unsigned int header_size;
unsigned int i;
int val = 0;

@@ -412,6 +413,27 @@ void rt2x00lib_rxdone(struct data_entry *entry, struct sk_buff *skb,
}
}

+ /*
+ * Properly align the ieee80211 frame and make sure the
+ * data behind the ieee80211 header is on a 4 byte boundrary.
+ */
+ header_size = ieee80211_get_hdrlen_from_skb(skb);
+ if (!header_size) {
+ /*
+ * Frame is too short to contain a valid header,
+ * drop the entire frame since it is useless.
+ */
+ kfree_skb(skb);
+ return;
+ } else if (header_size % 4 == 0) {
+ /*
+ * Move entire frame 2 bytes to the front.
+ */
+ skb_push(skb, 2);
+ memmove(skb->data, skb->data + 2, skb->len - 2);
+ skb_trim(skb, skb->len - 2);
+ }
+
rt2x00_update_link_rssi(&rt2x00dev->link, desc->rssi);
rt2x00dev->link.rx_success++;
rx_status->rate = val;

2008-01-08 15:44:10

by Tomas Winkler

[permalink] [raw]

Subject: Re: Warning emited by 2.6.24-rc6-git5

On Jan 2, 2008 9:12 PM, Ivo van Doorn <[email protected]> wrote:
> Hi,
>
> > Happy New Year!
>
> You too. :)
>
> > I was out since Saturday just before you sent your
> > message, sorry for the late reply.
> >
> > > header_size = ieee80211_get_hdrlen_from_skb(entry->skb);
> > > if (header_size % 4 == 2) {
> > > /*
> > > * Move entire frame 2 bytes to the front.
> > > */
> > > skb_push(entry->skb, 2);
> > > memmove(entry->skb->data, entry->skb->data + 2,
> > > entry->skb->len - 2);
> > > }
> >
> > That doesn't really look right, I'd think the skb will be two bytes too
> > long after this.

> For PCI drivers I could indeed optimize the code by making the memcpy
> perform the operation on the 4 byte aligned code. But for this I need
> to check what is better, RX directly into a skbuff and use memmove
> or RX into DMA and use memcpy to a skbuff.
> But for USB this is not possible, since it directly performs the RX into
> the skbuff already, so I either have to change that or perform memmove anyway.
>
> > If you absolutely can't get the hardware to do it and would otherwise do
> > DMA right into the skb we should try to evaluate the performance hit on
> > platforms where unaligned access *is* possible to be able to balance it
> > against the performance hit caused by the memmove(). Ultimately, though,
> > I value correctness on all platforms over performance on some, hence the
> > warning when unaligned packets are handed up to mac80211.
>
> Overall correctness sounds as the best solution to me too.

I see it now happening in iw4965
I didn't test the copy solution yet but in 11n rates I see as a show
stopper. All 11n packets are QoS packets.
I haven't encounter a problems in net stack yet so I think this
solution is a bit drastic.

Thanks
Tomas

Tomas

> Ivo

2008-01-08 20:03:53

by Michael Büsch

[permalink] [raw]

Subject: Re: Warning emited by 2.6.24-rc6-git5

On Tuesday 08 January 2008 20:43:53 Tomas Winkler wrote:
> On Jan 8, 2008 7:22 PM, Johannes Berg <[email protected]> wrote:
> >
> > On Tue, 2008-01-08 at 18:09 +0100, Johannes Berg wrote:
> > > > I didn't test the copy solution yet but in 11n rates I see as a show
> > > > stopper. All 11n packets are QoS packets.
> > >
> > > Really, go fix your firmware.
> >
>
> I don't think it is really broken. The packet it self is aligned. I'm
> not sure who has to be responsible for payload alignment you may have
> 10 other protocols packed into it, each with variable length
> unaligned header.

Yeah sure. You can move data around in the kernel and waste CPU,
or you can be clever and let the NIC firmware do it. Whatever you want
at intel. It's up to you. ;)

--
Greetings Michael.

2008-01-04 17:09:38

[permalink] [raw]

Subject: Re: Warning emited by 2.6.24-rc6-git5

On Wednesday 02 January 2008, Ivo van Doorn wrote:

<snip>

> On Wednesday 02 January 2008, Chris Clayton wrote:
> >
> > Ah, does the patch I have applied and am running need enhancement,
> > please?
>

Thanks, I'll run 2.6.24-rc6-git10 with this patch over the weekend and let you
know if there are any problems

Chris
> ---
>
> diff --git a/drivers/net/wireless/rt2x00/rt2x00dev.c
> b/drivers/net/wireless/rt2x00/rt2x00dev.c index ff399f8..3c10a68 100644
> --- a/drivers/net/wireless/rt2x00/rt2x00dev.c
> +++ b/drivers/net/wireless/rt2x00/rt2x00dev.c

<snip>

--
Beer is proof that God loves us and wants us to be happy - Benjamin Franklin

2008-01-02 08:38:14

[permalink] [raw]

Subject: Re: Warning emited by 2.6.24-rc6-git5

Hi,

Happy New Year! I was out since Saturday just before you sent your
message, sorry for the late reply.

> header_size = ieee80211_get_hdrlen_from_skb(entry->skb);
> if (header_size % 4 == 2) {
> /*
> * Move entire frame 2 bytes to the front.
> */
> skb_push(entry->skb, 2);
> memmove(entry->skb->data, entry->skb->data + 2,
> entry->skb->len - 2);
> }

That doesn't really look right, I'd think the skb will be two bytes too
long after this. It's probably more efficient to decide where to copy
the frame and do the realignment while you're copying it anyway rather
than doing a copy and then a memmove.

I guess you should also try talk to Ralink to get firmware to do it
(where possible), it's probably not too hard to insert padding before
the frame.

If you absolutely can't get the hardware to do it and would otherwise do
DMA right into the skb we should try to evaluate the performance hit on
platforms where unaligned access *is* possible to be able to balance it
against the performance hit caused by the memmove(). Ultimately, though,
I value correctness on all platforms over performance on some, hence the
warning when unaligned packets are handed up to mac80211.

johannes

Attachments:

signature.asc (828.00 B)
This is a digitally signed message part

2008-01-08 06:03:41

[permalink] [raw]

Subject: Re: Warning emited by 2.6.24-rc6-git5

On Friday 04 January 2008, Chris Clayton wrote:
> On Wednesday 02 January 2008, Ivo van Doorn wrote:
>
> <snip>
>
> > On Wednesday 02 January 2008, Chris Clayton wrote:
> > > Ah, does the patch I have applied and am running need enhancement,
> > > please?
>
> Thanks, I'll run 2.6.24-rc6-git10 with this patch over the weekend and let
> you know if there are any problems

OK, I've been running 2.6.24-rc6-git kernels (rc7 last night) with the patch
applied and my wireless connection has been fine. So here's my

Tested-by: Chris Clayton <[email protected]>

Thanks

>
> Chris
>
> > ---
> >
> > diff --git a/drivers/net/wireless/rt2x00/rt2x00dev.c
> > b/drivers/net/wireless/rt2x00/rt2x00dev.c index ff399f8..3c10a68 100644
> > --- a/drivers/net/wireless/rt2x00/rt2x00dev.c
> > +++ b/drivers/net/wireless/rt2x00/rt2x00dev.c
>
> <snip>

--
Beer is proof that God loves us and wants us to be happy - Benjamin Franklin

2008-01-09 12:50:12