Re: [PATCH] : CDC EEM driver patch to be applied to 2.6.30 kernel
From: David Brownell <hidden>
Date: 2009-05-04 16:38:51
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) :
quoted
+ 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().
quoted
- 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.
quoted hunk ↗ jump to hunk
============== 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.
quoted hunk ↗ jump to hunk
@@ -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; }