[PATCH] libertas: make if_sdio align packets

[PATCH] libertas: make if_sdio align packets

[Pierre Ossman]

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

Incoming packets have to be aligned or the IP stack becomes upset.
Make sure to shift them two bytes to achieve this.

Signed-off-by: Pierre Ossman <drzeus@drzeus.cx>
---

diff --git a/drivers/net/wireless/libertas/if_sdio.c b/drivers/net/wireless/libertas/if_sdio.c
index 5b13705..56dd568 100644
--- a/drivers/net/wireless/libertas/if_sdio.c
+++ b/drivers/net/wireless/libertas/if_sdio.c
@@ -175,12 +175,20 @@ static int if_sdio_handle_data(struct if_sdio_card *card,
 		goto out;
 	}
 
-	skb = dev_alloc_skb(MRVDRV_ETH_RX_PACKET_BUFFER_SIZE);
+	skb = dev_alloc_skb(MRVDRV_ETH_RX_PACKET_BUFFER_SIZE + NET_IP_ALIGN);
 	if (!skb) {
 		ret = -ENOMEM;
 		goto out;
 	}
 
+	/*
+	 * The IP stack is littered with silly assumptions on alignment,
+	 * so we need to do a bit of layering violation here and make
+	 * assumptions about the size of the headers between us and the
+	 * IP stack.
+	 */
+	skb_reserve(skb, NET_IP_ALIGN);
+
 	data = skb_put(skb, size);
 
 	memcpy(data, buffer, size);

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH] libertas: make if_sdio align packets

[Dan Williams]

On Mon, 2007-10-22 at 19:05 +0200, Pierre Ossman wrote:
> Incoming packets have to be aligned or the IP stack becomes upset.
> Make sure to shift them two bytes to achieve this.
> 
> Signed-off-by: Pierre Ossman <drzeus@drzeus.cx>

Acked-by: Dan Williams <dcbw@redhat.com>

John; if you don't have if_sdio.c, please rebase from Linus or something
since it went in with Pierre's SDIO stack.

> ---
> 
> diff --git a/drivers/net/wireless/libertas/if_sdio.c b/drivers/net/wireless/libertas/if_sdio.c
> index 5b13705..56dd568 100644
> --- a/drivers/net/wireless/libertas/if_sdio.c
> +++ b/drivers/net/wireless/libertas/if_sdio.c
> @@ -175,12 +175,20 @@ static int if_sdio_handle_data(struct if_sdio_card *card,
>  		goto out;
>  	}
>  
> -	skb = dev_alloc_skb(MRVDRV_ETH_RX_PACKET_BUFFER_SIZE);
> +	skb = dev_alloc_skb(MRVDRV_ETH_RX_PACKET_BUFFER_SIZE + NET_IP_ALIGN);
>  	if (!skb) {
>  		ret = -ENOMEM;
>  		goto out;
>  	}
>  
> +	/*
> +	 * The IP stack is littered with silly assumptions on alignment,
> +	 * so we need to do a bit of layering violation here and make
> +	 * assumptions about the size of the headers between us and the
> +	 * IP stack.
> +	 */
> +	skb_reserve(skb, NET_IP_ALIGN);
> +
>  	data = skb_put(skb, size);
>  
>  	memcpy(data, buffer, size);

Re: [PATCH] libertas: make if_sdio align packets

[David Miller]

From: Pierre Ossman <drzeus@drzeus.cx>
Date: Mon, 22 Oct 2007 19:05:32 +0200

> +	/*
> +	 * The IP stack is littered with silly assumptions on alignment,
> +	 * so we need to do a bit of layering violation here and make
> +	 * assumptions about the size of the headers between us and the
> +	 * IP stack.
> +	 */
> +	skb_reserve(skb, NET_IP_ALIGN);
> +

Please remove this comment.

Where else could we possibly ensure that post-link-level headers are
aligned properly, taking into account DMA engine restrictions and
whatnot, other than in the driver itself?

It's not about IP, even though the macro has "IP" in it's name.  It's
meant to ensure reasonable alignment after the link-level header,
regardless of upper-level protocol.  So your layering violation
commentary is simply rediculious and frankly starting to annoy the
crap out of me.

As you were also shown, all of THIS is fully documented in a huge
comment above the definition of NET_IP_ALIGN.

The irony in all of this is that NET_IP_ALIGN is in fact fully
documented even in books such as O'Reilly's Understanding Linux
Network Internals.  See chapter 10, sub-section 4.

The skb_reserve() sequence that was so terribly difficult for you to
discover is used in just about every network driver, and even used in
the drivers/net/pci-skeleton.c, a skeleton PCI ethernet driver that is
there exactly to show people how to do things properly.  It uses the
constant '2' instead of NET_IP_ALIGN, which should obviously be
corrected.

All of your complaining is simply because you didn't put much
effort into looking around to see what the existing practice
was and therefore you got surprising results.  We have nearly
a hundred ethernet drivers you could have studied to see how
things are supposed to be done.

I'm sorry for flaming you so much, but you are showing zero respect
for existing practice in the kernel networking and you behave as if
you're the only person in the world who has to deal with these issues.
You accuse us openly of mis-architecting the Linux networking with
these so-called "layering violations" and having maintained the stack
for more than 10 years I start to take things like that quite
personally.

Re: [PATCH] libertas: make if_sdio align packets

[Pierre Ossman]

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

On Mon, 22 Oct 2007 18:15:30 -0700 (PDT)
David Miller <davem@davemloft.net> wrote:

> From: Pierre Ossman <drzeus@drzeus.cx>
> Date: Mon, 22 Oct 2007 19:05:32 +0200
> 
> > +	/*
> > +	 * The IP stack is littered with silly assumptions on alignment,
> > +	 * so we need to do a bit of layering violation here and make
> > +	 * assumptions about the size of the headers between us and the
> > +	 * IP stack.
> > +	 */
> > +	skb_reserve(skb, NET_IP_ALIGN);
> > +
> 
> Please remove this comment.
> 
> Where else could we possibly ensure that post-link-level headers are
> aligned properly, taking into account DMA engine restrictions and
> whatnot, other than in the driver itself?
> 

If the assumption was that this driver was just below post-link-level, then fine. But this driver has a header of 4 bytes. The problem is one level up, and even has variable headers at that point.

> It's not about IP, even though the macro has "IP" in it's name.  It's
> meant to ensure reasonable alignment after the link-level header,
> regardless of upper-level protocol.  So your layering violation
> commentary is simply rediculious and frankly starting to annoy the
> crap out of me.
> 
> As you were also shown, all of THIS is fully documented in a huge
> comment above the definition of NET_IP_ALIGN.
> 

And digging through every header file is the norm when trying to figure out the broad details of writing a network driver? I looked at both of if_sdio's sister drivers, if_usb and if_cs; only one had the reserve call and it didn't have a single comment (and not even the right macro). if_cs now has a call, but with the bare minimum of a comment. I also tried looking through Documentation/, but all I could find were driver specific docs.

> I'm sorry for flaming you so much, but you are showing zero respect
> for existing practice in the kernel networking and you behave as if
> you're the only person in the world who has to deal with these issues.
> You accuse us openly of mis-architecting the Linux networking with
> these so-called "layering violations" and having maintained the stack
> for more than 10 years I start to take things like that quite
> personally.

It _is_ a layering violation, with the upside of a performance gain. You're free to make that trade-off, but that doesn't mean I have to like it. If you feel offended by that, then I'm sorry. It was never my intention to piss anyone off.

Feel free to remove the comment. I don't see this discussion going anywhere productive.

/Pierre

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH] libertas: make if_sdio align packets

[Holger Schurig]

Hi David !

I have been I a similar situation when I wrote if_cs.c. I had 
virtually no clue about SKBs and how to work with them. I've 
looked at lwn.net, LDD, googled around, and looked at the 
in-kernel documentation, which went into fine detail. But I 
failed to grasp the real picture, e.g. up until now I don't have 
any idea why there are so many different functions to modify 
skb's in subtle ways.

So one can say that I still have virtually no clue about SKBs. 
That if_cs.c works, might even be coincididence, thanks to 
try-and-error. :-)

I've not seen if_sdio.c yet, but if Pierre looked at my if_cs.c 
when surely he started from my incomplete view of things :-(

Re: [PATCH] libertas: make if_sdio align packets

[David Miller]

From: Holger Schurig <hs4233@mail.mn-solutions.de>
Date: Tue, 23 Oct 2007 08:57:59 +0200

> I have been I a similar situation when I wrote if_cs.c. I had 
> virtually no clue about SKBs and how to work with them. I've 
> looked at lwn.net, LDD, googled around, and looked at the 
> in-kernel documentation, which went into fine detail.

On, of all places, http://linux-net.osdl.org/ (which now redirects to
http://www.linux-foundation.org/en/Net) you'll find, of all things, a
link to a document about how SKBs work:

	http://vger.kernel.org/~davem/skb.html

Amazing!  There are even diagrams and figures!!!