Message ID | 3a98b4410905040851i20d4192ax8f9ff3953e635979@mail.gmail.com |
---|---|
State | RFC, archived |
Delegated to: | David Miller |
Headers | show |
On Monday 04 May 2009, Omar Laazimani wrote: > Thanks for your quick feedbacks. > I have tested your patch with our device and it's working well. Great, then I'll send something to David Miller and maybe it can merge before 2.6.30-final. > I have also added the TX side support for ZLP (see patch herein). > Please note that I can't test this issue as our device doesn't > support it yet. All your device needs to do is ignore them properly. :) > By the way, Just for curiosity, I have two questions about your patch > (see bellow) : > > > + put_unaligned_le16(BIT(15) | (1 << 11) | len, > > + skb_push(skb2, 2)); > > why did you use 1 << 11 instead of BIT(11) ? To me, BIT(x) is for one-bit fields. That's a three-bit field, and I'd write "2 << 11" for another opcode not BIT(12), or even "3 << 11" instead of (BIT(11) | BIT(12)). Some folk define special macros for "bitfield of length N at offset O, value V" ... that can be overdone, but in any case it's not standardized like BIT(). > > - usbnet_skb_return(dev, skb2); > > + if (is_last) > > + return crc == crc2; > > Why do you prefer returning 0 and not incrementing > "dev->stats.rx_errors" instead of returning 1 (in all the cases) and > incrementing "dev->stats.rx_errors" in the error cases? To follow the standard calling convention as much as possible. Look at what usbnet.c does: if (dev->driver_info->rx_fixup && !dev->driver_info->rx_fixup (dev, skb)) goto error; Returning 0 is the error path (for better or worse), while returning 1 is the success path. So rx_fixup() routines should not normally touch rx_errors, since that's handled in the error path. Plus, the other entry to the error path is returning with skb->len == 0. You'll notice I changed things to avoid doing that ... and that in some cases you were both incrementing rx_error and emptying the SKB, causing *two* errors to be reported. > ============== CUT HERE > fixed : > - Zero length EEM packet support: > * Handle on TX side > > > --- cdc_eem.c 2009-05-04 16:59:43.000000000 +0200 > +++ cdc_eem_v5.c 2009-05-04 17:07:20.000000000 +0200 > @@ -31,7 +31,6 @@ > #include <linux/usb/cdc.h> > #include <linux/usb/usbnet.h> > > - > /* > * This driver is an implementation of the CDC "Ethernet Emulation > * Model" (EEM) specification, which encapsulates Ethernet frames > @@ -122,11 +121,14 @@ > struct sk_buff *skb2 = NULL; > u16 len = skb->len; > u32 crc = 0; > + int padlen = 0; > > - /* FIXME when ((len + EEM_HEAD + ETH_FCS_LEN) % dev->maxpacket) > + /* When ((len + EEM_HEAD + ETH_FCS_LEN) % dev->maxpacket) > * is zero, stick two bytes of zero length EEM packet on the end > * (so the framework won't add invalid single byte padding). > */ > + if (!((len + EEM_HEAD + ETH_FCS_LEN) % dev->maxpacket)) > + padlen += 2; > > if (!skb_cloned(skb)) { > int headroom = skb_headroom(skb); Close, but you also have to use "padlen + ETH_FCS_LEN" when verifying there's enough space at the end of the packet. I'll fix that. > @@ -145,7 +147,7 @@ > } > } > > - skb2 = skb_copy_expand(skb, EEM_HEAD, ETH_FCS_LEN, flags); > + skb2 = skb_copy_expand(skb, EEM_HEAD, ETH_FCS_LEN + padlen, flags); > if (!skb2) > return NULL; > > @@ -167,6 +169,10 @@ > len = skb->len; > put_unaligned_le16(BIT(14) | len, skb_push(skb, 2)); > > + /* Add zero length EEM packet if needed */ > + if (padlen) > + *skb_put(skb, 2) = (u16) 0; > + > return skb; > } > > -- 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 Monday 04 May 2009, Omar Laazimani wrote: > Thanks for your quick feedbacks. > I have tested your patch with our device and it's working well. Hmm, one more issue. The eem_unbind() isn't needed at all, is it?? It seems leftover from the ACM or ECM code, which needed a separate control interface. Nothing even sets up the data used by that routine (other than zero-init). - Dave -- 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
2009/5/4 David Brownell <david-b@pacbell.net>: > On Monday 04 May 2009, Omar Laazimani wrote: >> Thanks for your quick feedbacks. >> I have tested your patch with our device and it's working well. > > Hmm, one more issue. The eem_unbind() isn't needed at all, > is it?? It seems leftover from the ACM or ECM code, which > needed a separate control interface. Nothing even sets up > the data used by that routine (other than zero-init). > You're right. The eem_unbind() isn't needed and I forgot to remove it. > - Dave > -- 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
============== CUT HERE fixed : - Zero length EEM packet support: * Handle on TX side --- cdc_eem.c 2009-05-04 16:59:43.000000000 +0200 +++ cdc_eem_v5.c 2009-05-04 17:07:20.000000000 +0200 @@ -31,7 +31,6 @@ #include <linux/usb/cdc.h> #include <linux/usb/usbnet.h> - /* * This driver is an implementation of the CDC "Ethernet Emulation * Model" (EEM) specification, which encapsulates Ethernet frames @@ -122,11 +121,14 @@ struct sk_buff *skb2 = NULL; u16 len = skb->len; u32 crc = 0; + int padlen = 0; - /* FIXME when ((len + EEM_HEAD + ETH_FCS_LEN) % dev->maxpacket) + /* When ((len + EEM_HEAD + ETH_FCS_LEN) % dev->maxpacket) * is zero, stick two bytes of zero length EEM packet on the end * (so the framework won't add invalid single byte padding). */ + if (!((len + EEM_HEAD + ETH_FCS_LEN) % dev->maxpacket)) + padlen += 2; if (!skb_cloned(skb)) { int headroom = skb_headroom(skb); @@ -145,7 +147,7 @@ } } - skb2 = skb_copy_expand(skb, EEM_HEAD, ETH_FCS_LEN, flags); + skb2 = skb_copy_expand(skb, EEM_HEAD, ETH_FCS_LEN + padlen, flags); if (!skb2) return NULL; @@ -167,6 +169,10 @@ len = skb->len; put_unaligned_le16(BIT(14) | len, skb_push(skb, 2)); + /* Add zero length EEM packet if needed */ + if (padlen) + *skb_put(skb, 2) = (u16) 0; + return skb; } -- To unsubscribe from this list: send the line "unsubscribe netdev" in