Patchwork : CDC EEM driver patch to be applied to 2.6.30 kernel

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);


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.

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