| Message ID | 1304364912-15444-1-git-send-email-agimenez@sysvalve.es |
|---|---|
| State | Rejected, archived |
| Delegated to: | David Miller |
| Headers | show |
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? -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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); >
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,
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,
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@sysvalve.es] Sent: 3 mai 2011 12:58 To: Ben Hutchings Cc: linux-kernel@vger.kernel.org; dgiagio@gmail.com; dborca@yahoo.com; davem@davemloft.net; pmcenery@gmail.com; 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,
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. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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);
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(-)