| Message ID | 1270599787.8900.8.camel@Linuxdev4-laptop |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
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. -- 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
> -----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 -- 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 Tue, 2010-04-06 at 22:46 -0700, Maulik wrote: > > > -----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 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 netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
--- 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);