[PATCH 1/1] NET: usb: Adding URB_ZERO_PACKET flag to usbnet.c

[PATCH 1/1] NET: usb: Adding URB_ZERO_PACKET flag to usbnet.c

[Elina Pasheva]

Subject: [PATCH 1/1] NET: usb: Adding URB_ZERO_PACKET flag to usbnet.c
From: Elina Pasheva <epasheva@sierrawireless.com>
This patch adds setting of the urb transfer flag URB_ZERO_PACKET  before
submitting an urb for drivers that have requested it (by advertising flag
FLAG_SEND_ZLP).
The modification is in usbnet.c function usbnet_start_xmit().
This patch only adds the zero length flag.
A subsequent patch will address the buggy code we found when devices do not
advertise FLAG_SEND_ZLP in which case there is a possibility of transferring
packets with non-deterministic length.

This patch has been tested on kernel-2.6.34-rc3.
This patch has been checked against net-2.6 tree.
Signed-off-by: Elina Pasheva <epasheva@sierrawireless.com>
Signed-off-by: Rory Filer <rfiler@sierrawireless.com>
---

 drivers/net/usb/usbnet.c |   15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

--- a/drivers/net/usb/usbnet.c	2010-04-06 10:52:54.000000000 -0700
+++ b/drivers/net/usb/usbnet.c	2010-04-06 16:54:44.000000000 -0700
@@ -1068,12 +1068,15 @@ netdev_tx_t usbnet_start_xmit (struct sk
 	 * NOTE:  strictly conforming cdc-ether devices should expect
 	 * the ZLP here, but ignore the one-byte packet.
 	 */
-	if (!(info->flags & FLAG_SEND_ZLP) && (length % dev->maxpacket) == 0) {
-		urb->transfer_buffer_length++;
-		if (skb_tailroom(skb)) {
-			skb->data[skb->len] = 0;
-			__skb_put(skb, 1);
-		}
+	if (length % dev->maxpacket == 0) {
+		if (!(info->flags & FLAG_SEND_ZLP)) {
+			urb->transfer_buffer_length++;
+			if (skb_tailroom(skb)) {
+				skb->data[skb->len] = 0;
+				__skb_put(skb, 1);
+			}
+		} else
+			urb->transfer_flags |= URB_ZERO_PACKET;
 	}
 
 	spin_lock_irqsave(&dev->txq.lock, flags);

Re: [PATCH 1/1] NET: usb: Adding URB_ZERO_PACKET flag to usbnet.c

[David Miller]

From: Elina Pasheva <epasheva@sierrawireless.com>
Date: Tue, 6 Apr 2010 17:23:07 -0700

> Subject: [PATCH 1/1] NET: usb: Adding URB_ZERO_PACKET flag to usbnet.c
> From: Elina Pasheva <epasheva@sierrawireless.com>
> This patch adds setting of the urb transfer flag URB_ZERO_PACKET  before
> submitting an urb for drivers that have requested it (by advertising flag
> FLAG_SEND_ZLP).
> The modification is in usbnet.c function usbnet_start_xmit().
> This patch only adds the zero length flag.
> A subsequent patch will address the buggy code we found when devices do not
> advertise FLAG_SEND_ZLP in which case there is a possibility of transferring
> packets with non-deterministic length.
> 
> This patch has been tested on kernel-2.6.34-rc3.
> This patch has been checked against net-2.6 tree.
> Signed-off-by: Elina Pasheva <epasheva@sierrawireless.com>
> Signed-off-by: Rory Filer <rfiler@sierrawireless.com>

Applied to net-next-2.6, thanks.

RE: [PATCH 1/1] NET: usb: Adding URB_ZERO_PACKET flag to usbnet.c

[Maulik]

> -----Original Message-----
> From: linux-usb-owner@vger.kernel.org [mailto:linux-usb-
> owner@vger.kernel.org] On Behalf Of Elina Pasheva
> Sent: Wednesday, April 07, 2010 5:53 AM
> To: dbrownell@users.sourceforge.net
> Cc: epasheva@sierrawireless.com; rfiler@sierrawireless.com;
> netdev@vger.kernel.org; linux-usb@vger.kernel.org
> Subject: [PATCH 1/1] NET: usb: Adding URB_ZERO_PACKET flag to usbnet.c
> 
> Subject: [PATCH 1/1] NET: usb: Adding URB_ZERO_PACKET flag to usbnet.c
> From: Elina Pasheva <epasheva@sierrawireless.com>
> This patch adds setting of the urb transfer flag URB_ZERO_PACKET  before
> submitting an urb for drivers that have requested it (by advertising flag
> FLAG_SEND_ZLP).
> The modification is in usbnet.c function usbnet_start_xmit().
> This patch only adds the zero length flag.
> A subsequent patch will address the buggy code we found when devices do
> not
> advertise FLAG_SEND_ZLP in which case there is a possibility of
> transferring
> packets with non-deterministic length.
> 
> This patch has been tested on kernel-2.6.34-rc3.
> This patch has been checked against net-2.6 tree.
> Signed-off-by: Elina Pasheva <epasheva@sierrawireless.com>
> Signed-off-by: Rory Filer <rfiler@sierrawireless.com>
> ---
> 
>  drivers/net/usb/usbnet.c |   15 +++++++++------
>  1 file changed, 9 insertions(+), 6 deletions(-)
> 
> --- a/drivers/net/usb/usbnet.c	2010-04-06 10:52:54.000000000 -0700
> +++ b/drivers/net/usb/usbnet.c	2010-04-06 16:54:44.000000000 -0700
> @@ -1068,12 +1068,15 @@ netdev_tx_t usbnet_start_xmit (struct sk
>  	 * NOTE:  strictly conforming cdc-ether devices should expect
>  	 * the ZLP here, but ignore the one-byte packet.
>  	 */
> -	if (!(info->flags & FLAG_SEND_ZLP) && (length % dev->maxpacket) ==
> 0) {
> -		urb->transfer_buffer_length++;
> -		if (skb_tailroom(skb)) {
> -			skb->data[skb->len] = 0;
> -			__skb_put(skb, 1);
> -		}
> +	if (length % dev->maxpacket == 0) {
> +		if (!(info->flags & FLAG_SEND_ZLP)) {
> +			urb->transfer_buffer_length++;
> +			if (skb_tailroom(skb)) {
> +				skb->data[skb->len] = 0;
> +				__skb_put(skb, 1);
> +			}
> +		} else
> +			urb->transfer_flags |= URB_ZERO_PACKET;

You should place braces for the else case as well. See
Documentation/CodingStyle.

It states to use braces in both the branches since the if() case 
contains multiple statements. 

Maulik

RE: [PATCH 1/1] NET: usb: Adding URB_ZERO_PACKET flag to usbnet.c

[Elina Pasheva]

On Tue, 2010-04-06 at 22:46 -0700, Maulik wrote:
> 
> > -----Original Message-----
> > From: linux-usb-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org [mailto:linux-usb-
> > owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org] On Behalf Of Elina Pasheva
> > Sent: Wednesday, April 07, 2010 5:53 AM
> > To: dbrownell-Rn4VEauK+AKRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
> > Cc: epasheva-ywE8TTl5eJHWpu6QEFMNjNBPR1lH4CV8@public.gmane.org; rfiler-ywE8TTl5eJHWpu6QEFMNjNBPR1lH4CV8@public.gmane.org;
> > netdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; linux-usb-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> > Subject: [PATCH 1/1] NET: usb: Adding URB_ZERO_PACKET flag to usbnet.c
> > 
> > Subject: [PATCH 1/1] NET: usb: Adding URB_ZERO_PACKET flag to usbnet.c
> > From: Elina Pasheva <epasheva-ywE8TTl5eJHWpu6QEFMNjNBPR1lH4CV8@public.gmane.org>
> > This patch adds setting of the urb transfer flag URB_ZERO_PACKET  before
> > submitting an urb for drivers that have requested it (by advertising flag
> > FLAG_SEND_ZLP).
> > The modification is in usbnet.c function usbnet_start_xmit().
> > This patch only adds the zero length flag.
> > A subsequent patch will address the buggy code we found when devices do
> > not
> > advertise FLAG_SEND_ZLP in which case there is a possibility of
> > transferring
> > packets with non-deterministic length.
> > 
> > This patch has been tested on kernel-2.6.34-rc3.
> > This patch has been checked against net-2.6 tree.
> > Signed-off-by: Elina Pasheva <epasheva-ywE8TTl5eJHWpu6QEFMNjNBPR1lH4CV8@public.gmane.org>
> > Signed-off-by: Rory Filer <rfiler-ywE8TTl5eJHWpu6QEFMNjNBPR1lH4CV8@public.gmane.org>
> > ---
> > 
> >  drivers/net/usb/usbnet.c |   15 +++++++++------
> >  1 file changed, 9 insertions(+), 6 deletions(-)
> > 
> > --- a/drivers/net/usb/usbnet.c	2010-04-06 10:52:54.000000000 -0700
> > +++ b/drivers/net/usb/usbnet.c	2010-04-06 16:54:44.000000000 -0700
> > @@ -1068,12 +1068,15 @@ netdev_tx_t usbnet_start_xmit (struct sk
> >  	 * NOTE:  strictly conforming cdc-ether devices should expect
> >  	 * the ZLP here, but ignore the one-byte packet.
> >  	 */
> > -	if (!(info->flags & FLAG_SEND_ZLP) && (length % dev->maxpacket) ==
> > 0) {
> > -		urb->transfer_buffer_length++;
> > -		if (skb_tailroom(skb)) {
> > -			skb->data[skb->len] = 0;
> > -			__skb_put(skb, 1);
> > -		}
> > +	if (length % dev->maxpacket == 0) {
> > +		if (!(info->flags & FLAG_SEND_ZLP)) {
> > +			urb->transfer_buffer_length++;
> > +			if (skb_tailroom(skb)) {
> > +				skb->data[skb->len] = 0;
> > +				__skb_put(skb, 1);
> > +			}
> > +		} else
> > +			urb->transfer_flags |= URB_ZERO_PACKET;
> 
> You should place braces for the else case as well. See
> Documentation/CodingStyle.
> 
> It states to use braces in both the branches since the if() case 
> contains multiple statements. 
> 
> Maulik

Thanks for your review.
The patch has been applied. 
We'll look at it next time.

Elina

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/1] NET: usb: Adding URB_ZERO_PACKET flag to usbnet.c

[David Brownell]

On Tuesday 06 April 2010, you wrote:
Recall that the reason to avoid sending zero length packts
(ZLPs) is that many systems don't cope well with them...

The ""don't cope well" can be at the hardware level,
or drivers not limited to device firmware.  I've seen
the failures be very context-dependent .... as in, one
standalone ZLP might work, but mix it in with back-to-back
delivery of other packets and trouble ensues...
  
In short, it's hard to know which combinations of
hardware an firmware would need it .... versus which
ones it would break.

... and thus risky to try sending ZLPs through systems
shere for many years) we've carefully avoided doing that.


- Dave
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/1] NET: usb: Adding URB_ZERO_PACKET flag to usbnet.c

[Elina Pasheva]

On Wed, 2010-04-07 at 14:14 -0700, David Brownell wrote:
> On Tuesday 06 April 2010, you wrote:
> Recall that the reason to avoid sending zero length packts
> (ZLPs) is that many systems don't cope well with them...
> 
> The ""don't cope well" can be at the hardware level,
> or drivers not limited to device firmware.  I've seen
> the failures be very context-dependent .... as in, one
> standalone ZLP might work, but mix it in with back-to-back
> delivery of other packets and trouble ensues...
>   
> In short, it's hard to know which combinations of
> hardware an firmware would need it .... versus which
> ones it would break.
> 
> ... and thus risky to try sending ZLPs through systems
> shere for many years) we've carefully avoided doing that.
> 
> 
> - Dave
> 
Hi Dave, 
Nice to hear your opinion on this matter. Are you recommending our patch
be retracted? If so, we can look at other ways to fix the problem when a
zero length packet is missing. 

Regards, 
Elina


 

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html