[PATCH] ipheth.c: Enable IP header alignment

[PATCH] ipheth.c: Enable IP header alignment

[L. Alberto Giménez]

From: David Hill <david.hill@ubisoft.com>

Since commit ea812ca1b06113597adcd8e70c0f84a413d97544, NET_IP_ALIGN changed from
2 to 0. Some people have reported that tethering stopped working and David Hill
submited a patch that seems to fix the problem.

I have no more an iPhone device to test it, so it is only compile-tested.

Signed-off-by: L. Alberto Giménez <agimenez@sysvalve.es>
---
 drivers/net/usb/ipheth.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/drivers/net/usb/ipheth.c b/drivers/net/usb/ipheth.c
index 7d42f9a..711346b 100644
--- a/drivers/net/usb/ipheth.c
+++ b/drivers/net/usb/ipheth.c
@@ -54,6 +54,9 @@
 #include <linux/usb.h>
 #include <linux/workqueue.h>
 
+#undef  NET_IP_ALIGN
+#define NET_IP_ALIGN 2
+
 #define USB_VENDOR_APPLE        0x05ac
 #define USB_PRODUCT_IPHONE      0x1290
 #define USB_PRODUCT_IPHONE_3G   0x1292
-- 
1.7.5

Re: [PATCH] ipheth.c: Enable IP header alignment

[Ben Hutchings]

On Sun, 2011-05-01 at 13:00 +0200, L. Alberto Giménez wrote:
> From: David Hill <david.hill-+VOaU5BgciZBDgjK7y7TUQ@public.gmane.org>
> 
> Since commit ea812ca1b06113597adcd8e70c0f84a413d97544, NET_IP_ALIGN changed from
> 2 to 0. Some people have reported that tethering stopped working and David Hill
> submited a patch that seems to fix the problem.
> 
> I have no more an iPhone device to test it, so it is only compile-tested.
> 
> Signed-off-by: L. Alberto Giménez <agimenez-lqZFv/KUvpAxAGwisGp4zA@public.gmane.org>
> ---
>  drivers/net/usb/ipheth.c |    3 +++
>  1 files changed, 3 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/net/usb/ipheth.c b/drivers/net/usb/ipheth.c
> index 7d42f9a..711346b 100644
> --- a/drivers/net/usb/ipheth.c
> +++ b/drivers/net/usb/ipheth.c
> @@ -54,6 +54,9 @@
>  #include <linux/usb.h>
>  #include <linux/workqueue.h>
>  
> +#undef  NET_IP_ALIGN
> +#define NET_IP_ALIGN 2
> +
>  #define USB_VENDOR_APPLE        0x05ac
>  #define USB_PRODUCT_IPHONE      0x1290
>  #define USB_PRODUCT_IPHONE_3G   0x1292

No, you can't do this.

If there is some reason to use a fixed alignment of 2 (which I find hard
to believe; this is a USB device after all) then that should be
specified as a private constant.

Ben.

-- 
Ben Hutchings, Senior Software Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] ipheth.c: Enable IP header alignment

[L. Alberto Giménez]

Since commit ea812ca1b06113597adcd8e70c0f84a413d97544 (x86: Align skb w/ start
of cacheline on newer core 2/Xeon Arch), NET_IP_ALIGN changed from 2 to 0, and
the constant is used to reserve more room for the socket buffer.

Some people have reported that tethering stopped working and David Hill
submited a patch that redefined NET_IP_ALIGN. Pointed by Ben Hutchings, the
patch has been reworked to use a private constant.

I have no more an iPhone device to test it, so it is only compile-tested.

Signed-off-by: L. Alberto Giménez <agimenez@sysvalve.es>
---
 drivers/net/usb/ipheth.c |    8 +++++---
 1 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/net/usb/ipheth.c b/drivers/net/usb/ipheth.c
index 7d42f9a..8f1ffc7 100644
--- a/drivers/net/usb/ipheth.c
+++ b/drivers/net/usb/ipheth.c
@@ -80,6 +80,8 @@
 #define IPHETH_CARRIER_CHECK_TIMEOUT round_jiffies_relative(1 * HZ)
 #define IPHETH_CARRIER_ON       0x04
 
+#define IPHETH_IP_ALIGN 2
+
 static struct usb_device_id ipheth_table[] = {
 	{ USB_DEVICE_AND_INTERFACE_INFO(
 		USB_VENDOR_APPLE, USB_PRODUCT_IPHONE,
@@ -205,15 +207,15 @@ static void ipheth_rcvbulk_callback(struct urb *urb)
 	len = urb->actual_length;
 	buf = urb->transfer_buffer;
 
-	skb = dev_alloc_skb(NET_IP_ALIGN + len);
+	skb = dev_alloc_skb(IPHETH_IP_ALIGN + len);
 	if (!skb) {
 		err("%s: dev_alloc_skb: -ENOMEM", __func__);
 		dev->net->stats.rx_dropped++;
 		return;
 	}
 
-	skb_reserve(skb, NET_IP_ALIGN);
-	memcpy(skb_put(skb, len), buf + NET_IP_ALIGN, len - NET_IP_ALIGN);
+	skb_reserve(skb, IPHETH_IP_ALIGN);
+	memcpy(skb_put(skb, len), buf + IPHETH_IP_ALIGN, len - IPHETH_IP_ALIGN);
 	skb->dev = dev->net;
 	skb->protocol = eth_type_trans(skb, dev->net);
 
-- 
1.7.5

Re: [PATCH] ipheth.c: Enable IP header alignment

[David Miller]

From: L. Alberto Giménez <agimenez@sysvalve.es>
Date: Mon,  2 May 2011 21:35:12 +0200

> Since commit ea812ca1b06113597adcd8e70c0f84a413d97544 (x86: Align skb w/ start
> of cacheline on newer core 2/Xeon Arch), NET_IP_ALIGN changed from 2 to 0, and
> the constant is used to reserve more room for the socket buffer.
> 
> Some people have reported that tethering stopped working and David Hill
> submited a patch that redefined NET_IP_ALIGN. Pointed by Ben Hutchings, the
> patch has been reworked to use a private constant.
> 
> I have no more an iPhone device to test it, so it is only compile-tested.
> 
> Signed-off-by: L. Alberto Giménez <agimenez@sysvalve.es>

Why did this break things?

I'm not applying a fix when nobody can explain the reason why:

1) Things broke in the first place
2) Forcing reservation of 2 bytes fixes things

Where is the built in assumption about "2" and why does it exist?  Why
can't we fix this code not to have such assumptions in the first
place?

Re: [PATCH] ipheth.c: Enable IP header alignment

[L. Alberto Giménez]

On Mon, May 02, 2011 at 12:46:22PM -0700, David Miller wrote:
> 
> Why did this break things?

Hi, I don't know. As upstream is unresponsive and is applying patches to his
private repo without submitting them to the list (which I can understand), I
decided to submit the particular fix so mainline users can get tethering working
again.

I received a forwarded email with the patch (I think that's because I submitted
the driver to mainline) asking for the mainline driver status and if it was
being maintained.

> 
> I'm not applying a fix when nobody can explain the reason why:
> 
> 1) Things broke in the first place
> 2) Forcing reservation of 2 bytes fixes things

Honestly, I can't answer either of those ones. I just submitted a patch that
*seemed* to fix the problem (I don't own an iPhone device since long time ago),
after explictly requesting upstream to submit by himself, and getting a
negative.


> Where is the built in assumption about "2" and why does it exist?  Why
> can't we fix this code not to have such assumptions in the first
> place?

Ditto.

At this point, I think that David, Diego or Daniel should step in if they want
to keep on with this discussion. I won't have problems if you want to take this
off-list.

Best regards,

-- 
L. Alberto Giménez
JabberID agimenez@jabber.sysvalve.es
GnuPG key ID 0x3BAABDE1

Re: [PATCH] ipheth.c: Enable IP header alignment

[Ben Hutchings]

On Mon, 2011-05-02 at 21:35 +0200, L. Alberto Giménez wrote:
> Since commit ea812ca1b06113597adcd8e70c0f84a413d97544 (x86: Align skb w/ start
> of cacheline on newer core 2/Xeon Arch), NET_IP_ALIGN changed from 2 to 0, and
> the constant is used to reserve more room for the socket buffer.
> 
> Some people have reported that tethering stopped working and David Hill
> submited a patch that redefined NET_IP_ALIGN. Pointed by Ben Hutchings, the
> patch has been reworked to use a private constant.
> 
> I have no more an iPhone device to test it, so it is only compile-tested.
> 
> Signed-off-by: L. Alberto Giménez <agimenez@sysvalve.es>
> ---
>  drivers/net/usb/ipheth.c |    8 +++++---
>  1 files changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/usb/ipheth.c b/drivers/net/usb/ipheth.c
> index 7d42f9a..8f1ffc7 100644
> --- a/drivers/net/usb/ipheth.c
> +++ b/drivers/net/usb/ipheth.c
> @@ -80,6 +80,8 @@
>  #define IPHETH_CARRIER_CHECK_TIMEOUT round_jiffies_relative(1 * HZ)
>  #define IPHETH_CARRIER_ON       0x04
>  
> +#define IPHETH_IP_ALIGN 2
> +
>  static struct usb_device_id ipheth_table[] = {
>  	{ USB_DEVICE_AND_INTERFACE_INFO(
>  		USB_VENDOR_APPLE, USB_PRODUCT_IPHONE,
> @@ -205,15 +207,15 @@ static void ipheth_rcvbulk_callback(struct urb *urb)
>  	len = urb->actual_length;
>  	buf = urb->transfer_buffer;
>  
> -	skb = dev_alloc_skb(NET_IP_ALIGN + len);
> +	skb = dev_alloc_skb(IPHETH_IP_ALIGN + len);
>  	if (!skb) {
>  		err("%s: dev_alloc_skb: -ENOMEM", __func__);
>  		dev->net->stats.rx_dropped++;
>  		return;
>  	}
>  
> -	skb_reserve(skb, NET_IP_ALIGN);
> -	memcpy(skb_put(skb, len), buf + NET_IP_ALIGN, len - NET_IP_ALIGN);
> +	skb_reserve(skb, IPHETH_IP_ALIGN);
> +	memcpy(skb_put(skb, len), buf + IPHETH_IP_ALIGN, len - IPHETH_IP_ALIGN);
[...]

So this was using NET_IP_ALIGN as an offset into the URB.  Which was
totally bogus, as its value has long been architecture-dependent.  The
code is also claiming to put len bytes but only copying len - delta.

The correct code would be something like:

	if (urb->actual_length <= IPHETH_IP_ALIGN) {
		dev->net->stats.rx_length_errors++;
		return;
	}
	len = urb->actual_length - IPHETH_IP_ALIGN;
	buf = urb->transfer_buffer + IPHETH_IP_ALIGN;
	
	dev_alloc_skb(len);
	...
	memcpy(skb_put(skb, len), buf, len);

Ben.

>  	skb->dev = dev->net;
>  	skb->protocol = eth_type_trans(skb, dev->net);
>  

-- 
Ben Hutchings, Senior Software Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

Re: [PATCH] ipheth.c: Enable IP header alignment

[L. Alberto Giménez]

On Mon, May 02, 2011 at 10:04:34PM +0100, Ben Hutchings wrote:
> So this was using NET_IP_ALIGN as an offset into the URB.  Which was
> totally bogus, as its value has long been architecture-dependent.  The
> code is also claiming to put len bytes but only copying len - delta.
> 
> The correct code would be something like:
> 
> 	if (urb->actual_length <= IPHETH_IP_ALIGN) {
> 		dev->net->stats.rx_length_errors++;
> 		return;
> 	}
> 	len = urb->actual_length - IPHETH_IP_ALIGN;
> 	buf = urb->transfer_buffer + IPHETH_IP_ALIGN;
> 	
> 	dev_alloc_skb(len);
> 	...
> 	memcpy(skb_put(skb, len), buf, len);

Thanks for the response Ben.

I can try to change the code, but I don't own the device anymore. Changing the
code without being able to test it would be walking blindfolded :-/

If upstrem (everyone involved is in CC) can't do it, I can submit the changes
advised by Ben, but I can't warantee anything beyond successful compilation. I
don't think that it would be acceptable here.

Regards,
-- 
L. Alberto Giménez
JabberID agimenez@jabber.sysvalve.es
GnuPG key ID 0x3BAABDE1

RE: [PATCH] ipheth.c: Enable IP header alignment

[David Hill]

Maybe I can help on this part.

Git ?

I'm using ubuntu natty ... The part  I'm not sure of is , do I patch ubuntu's kernel or I start from scratch?


-----Original Message-----
From: L. Alberto Giménez [mailto:agimenez-lqZFv/KUvpAxAGwisGp4zA@public.gmane.org] 
Sent: 3 mai 2011 12:58
To: Ben Hutchings
Cc: linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; dgiagio-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org; dborca-/E1597aS9LQAvxtiuMwx3w@public.gmane.org; davem-fT/PcQaiUtIeIZ0/mPfg9Q@public.gmane.org; pmcenery-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org; David Hill; open list:USB SUBSYSTEM; open list:NETWORKING DRIVERS
Subject: Re: [PATCH] ipheth.c: Enable IP header alignment

On Mon, May 02, 2011 at 10:04:34PM +0100, Ben Hutchings wrote:
> So this was using NET_IP_ALIGN as an offset into the URB.  Which was
> totally bogus, as its value has long been architecture-dependent.  The
> code is also claiming to put len bytes but only copying len - delta.
> 
> The correct code would be something like:
> 
> 	if (urb->actual_length <= IPHETH_IP_ALIGN) {
> 		dev->net->stats.rx_length_errors++;
> 		return;
> 	}
> 	len = urb->actual_length - IPHETH_IP_ALIGN;
> 	buf = urb->transfer_buffer + IPHETH_IP_ALIGN;
> 	
> 	dev_alloc_skb(len);
> 	...
> 	memcpy(skb_put(skb, len), buf, len);

Thanks for the response Ben.

I can try to change the code, but I don't own the device anymore. Changing the
code without being able to test it would be walking blindfolded :-/

If upstrem (everyone involved is in CC) can't do it, I can submit the changes
advised by Ben, but I can't warantee anything beyond successful compilation. I
don't think that it would be acceptable here.

Regards,
-- 
L. Alberto Giménez
JabberID agimenez-eu7EghD4TOHJ13y34KW5H97lo5+wdyHW@public.gmane.org
GnuPG key ID 0x3BAABDE1
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] ipheth.c: Enable IP header alignment

[Paul McEnery]

On 3 May 2011 18:18, David Hill <david.hill@ubisoft.com> wrote:
>
> Maybe I can help on this part.
>
> Git ?
>
> I'm using ubuntu natty ... The part  I'm not sure of is , do I patch ubuntu's kernel or I start from scratch?
>

If we can get the patch committed to the Github ipheth repo, I'll
package it in my Ubuntu PPA. You can then simply install ipheth-dkms
which allow it to replace the in-tree module for testing...

I unfortunately cannot test either, since I no longer have an iPhone.

Paul.

RE: [PATCH] ipheth.c: Enable IP header alignment

[David Hill]

That's not a problem, I can test it :)


-----Original Message-----
From: Paul McEnery [mailto:pmcenery@gmail.com] 
Sent: 3 mai 2011 13:34
To: David Hill
Cc: L. Alberto Giménez; Ben Hutchings; linux-kernel@vger.kernel.org; dgiagio@gmail.com; dborca@yahoo.com; davem@davemloft.net; open list:USB SUBSYSTEM; open list:NETWORKING DRIVERS
Subject: Re: [PATCH] ipheth.c: Enable IP header alignment

On 3 May 2011 18:18, David Hill <david.hill@ubisoft.com> wrote:
>
> Maybe I can help on this part.
>
> Git ?
>
> I'm using ubuntu natty ... The part  I'm not sure of is , do I patch ubuntu's kernel or I start from scratch?
>

If we can get the patch committed to the Github ipheth repo, I'll
package it in my Ubuntu PPA. You can then simply install ipheth-dkms
which allow it to replace the in-tree module for testing...

I unfortunately cannot test either, since I no longer have an iPhone.

Paul.

Re: [PATCH] ipheth.c: Enable IP header alignment

[Paul McEnery]

On 3 May 2011 18:41, David Hill <david.hill@ubisoft.com> wrote:
> That's not a problem, I can test it :)
>

I have applied Ben Hutching's patch [1] to the Github repository [2].
Updated Debian/Ubuntu packages are now available for testing [3].

Please test and report back. I'm sure Ben would also like to know if
this fix work :)

[1] - https://lkml.org/lkml/2011/5/3/283
[2] - https://github.com/dgiagio/ipheth/commit/46d6db65e0054cfae6f7355200b83f04e2fb9042
[3] - https://launchpad.net/~pmcenery/+archive/ppa

[PATCH net-next-2.6] ipheth: Properly distinguish length and alignment in URBs and skbs

[Ben Hutchings]

The USB protocol this driver implements appears to require 2 bytes of
padding in front of each received packet.  This used to be equal to
the value of NET_IP_ALIGN on x86, so the driver abused that constant
and mostly worked, but this is no longer the case.  The driver also
mixed up the URB and packet lengths, resulting in 2 bytes of junk at
the end of the skb.

Introduce a private constant for the 2 bytes of padding; fix this
confusion and check for the under-length case.

Signed-off-by: Ben Hutchings <bhutchings@solarflare.com>
---
Compile-tested only, as I'm not cool enough for an iPhone either.
This is applicable to net-next-2.6 or v2.6.38.

Ben.

 drivers/net/usb/ipheth.c |   14 +++++++++-----
 1 files changed, 9 insertions(+), 5 deletions(-)

diff --git a/drivers/net/usb/ipheth.c b/drivers/net/usb/ipheth.c
index 7d42f9a..81126ff 100644
--- a/drivers/net/usb/ipheth.c
+++ b/drivers/net/usb/ipheth.c
@@ -65,6 +65,7 @@
 #define IPHETH_USBINTF_PROTO    1
 
 #define IPHETH_BUF_SIZE         1516
+#define IPHETH_IP_ALIGN		2	/* padding at front of URB */
 #define IPHETH_TX_TIMEOUT       (5 * HZ)
 
 #define IPHETH_INTFNUM          2
@@ -202,18 +203,21 @@ static void ipheth_rcvbulk_callback(struct urb *urb)
 		return;
 	}
 
-	len = urb->actual_length;
-	buf = urb->transfer_buffer;
+	if (urb->actual_length <= IPHETH_IP_ALIGN) {
+		dev->net->stats.rx_length_errors++;
+		return;
+	}
+	len = urb->actual_length - IPHETH_IP_ALIGN;
+	buf = urb->transfer_buffer + IPHETH_IP_ALIGN;
 
-	skb = dev_alloc_skb(NET_IP_ALIGN + len);
+	skb = dev_alloc_skb(len);
 	if (!skb) {
 		err("%s: dev_alloc_skb: -ENOMEM", __func__);
 		dev->net->stats.rx_dropped++;
 		return;
 	}
 
-	skb_reserve(skb, NET_IP_ALIGN);
-	memcpy(skb_put(skb, len), buf + NET_IP_ALIGN, len - NET_IP_ALIGN);
+	memcpy(skb_put(skb, len), buf, len);
 	skb->dev = dev->net;
 	skb->protocol = eth_type_trans(skb, dev->net);
 
-- 
1.7.4


-- 
Ben Hutchings, Senior Software Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

Re: [PATCH net-next-2.6] ipheth: Properly distinguish length and alignment in URBs and skbs

[David Miller]

From: Ben Hutchings <bhutchings@solarflare.com>
Date: Tue, 03 May 2011 18:49:25 +0100

> The USB protocol this driver implements appears to require 2 bytes of
> padding in front of each received packet.  This used to be equal to
> the value of NET_IP_ALIGN on x86, so the driver abused that constant
> and mostly worked, but this is no longer the case.  The driver also
> mixed up the URB and packet lengths, resulting in 2 bytes of junk at
> the end of the skb.
> 
> Introduce a private constant for the 2 bytes of padding; fix this
> confusion and check for the under-length case.
> 
> Signed-off-by: Ben Hutchings <bhutchings@solarflare.com>
> ---
> Compile-tested only, as I'm not cool enough for an iPhone either.
> This is applicable to net-next-2.6 or v2.6.38.

I've applied this to net-2.6 and will conditionally queue it up for
-stable, if we need further fixups we can add relative patches.

Thanks.