Re: [PATCH] : CDC EEM driver patch to be applied to 2.6.30 kernel

[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.

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