Thread (10 messages) 10 messages, 3 authors, 2009-05-04

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;
 }
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help