Re: [PATCH] : CDC EEM driver patch to be applied to 2.6.30 kernel
From: Omar Laazimani <hidden>
Date: 2009-05-04 15:51:38
Thanks for your quick feedbacks. I have tested your patch with our device and it's working well. 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. By the way, Just for curiosity, I have two questions about your patch (see bellow) : Thanks, Omar
/*
- * EEM command packet:
+ * EEM (link) command packet:
* b0..10: bmEEMCmdParam
* b11..13: bmEEMCmd
- * b14: bmReserved (0 by default)
- * b15: 1
+ * b14: bmReserved (must be 0)
+ * b15: 1 (EEM command)
*/
+ if (header & BIT(14)) {
+ devdbg(dev, "reserved command %04x\n", header);
+ continue;
+ }
- skb2 = skb_clone(skb, GFP_ATOMIC);
- if (unlikely(!skb2))
- goto Continue;
+ bmEEMCmd = (header >> 11) & 0x7;
+ switch (bmEEMCmd) {
- bmEEMCmd = (BIT(11) & header)
- | (BIT(12) & header)
- | (BIT(13) & header);
+ /* Responding to echo requests is mandatory. */
+ case 0: /* Echo command */
+ len = header & 0x7FF;
+
+ /* bogus command? */
+ if (skb->len < len)
+ return 0;
+
+ skb2 = skb_clone(skb, GFP_ATOMIC);
+ if (unlikely(!skb2))
+ goto next;
+ skb_trim(skb2, len);
+ put_unaligned_le16(BIT(15) | (1 << 11) | len,
+ skb_push(skb2, 2));why did you use 1 << 11 instead of BIT(11) ?
+ eem_linkcmd(dev, skb2); + break;
+ /*
+ * The bmCRC helps to denote when the CRC field in
+ * the Ethernet frame contains a calculated CRC:
* bmCRC = 1 : CRC is calculated
* bmCRC = 0 : CRC = 0xDEADBEEF
*/
- if (header & BIT(14)) {
- u32 crc2;
- crc2 = crc32_le(~0, skb2->data, len);
- crc2 = ~crc2;
- if (unlikely(crc != crc2)) {
- dev->stats.rx_errors++;
- goto Continue;
- }
- } else {
- if (unlikely(crc != 0xdeadbeef)) {
- dev->stats.rx_errors++;
- goto Continue;
- }
- }
+ if (header & BIT(14))
+ crc2 = ~crc32_le(~0, skb2->data, len);
+ else
+ crc2 = 0xdeadbeef;
- 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?
+
+ if (unlikely(crc != crc2)) {
+ dev->stats.rx_errors++;
+ } else
+ usbnet_skb_return(dev, skb2);
============== 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; }