NET: usb: Adding URB_ZERO_PACKET flag to usbnet.c

PATCH Applied Elina Pasheva 5 participants 6 messages Apr 7, 2010 2 messages not in the archive

Subsystems (4)

Networking Network drivers USB networking drivers USB "usbnet" driver framework

lore

Share & tools

lore

Elina Pasheva

author

2010-04-07 00:23 UTC #0 lore raw

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 <[email protected]>
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 <[email protected]>

Signed-off-by Rory Filer <[email protected]>

---

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

drivers/net/usb/usbnet.c +9 -6

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

David Miller

maintainer Applied

2010-04-07 02:50 UTC #1 lore raw

From: Elina Pasheva <[email protected]>
Date: Tue, 6 Apr 2010 17:23:07 -0700

Quoted 11 lines

Subject: [PATCH 1/1] NET: usb: Adding URB_ZERO_PACKET flag to usbnet.c
From: Elina Pasheva <[email protected]>
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.

Quoted 4 lines

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 <[email protected]>
Signed-off-by Rory Filer <[email protected]>

Applied to net-next-2.6, thanks.

Maulik

2010-04-07 05:46 UTC #2 lore raw

In reply to Elina Pasheva

Quoted 45 lines

-----Original Message-----
From: [email protected] [mailto:linux-usb- [email protected]] On Behalf Of Elina Pasheva
Sent: Wednesday, April 07, 2010 5:53 AM
To: [email protected]
Cc: [email protected]; [email protected]; [email protected]; [email protected]
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 <[email protected]>
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 <[email protected]>
Signed-off-by Rory Filer <[email protected]>
---
 drivers/net/usb/usbnet.c |   15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)
drivers/net/usb/usbnet.c +0 -5
@@ -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);

drivers/net/usb/usbnet.c netdev_tx_t usbnet_start_xmit (struct sk · line 1080

Quoted 10 lines

drivers/net/usb/usbnet.c +9 -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

Elina Pasheva

2010-04-07 21:13 UTC #4 lore raw

In reply to a message not in the archive

Quoted 58 lines · Maulik

Quoted 44 lines
-----Original Message-----
From: [email protected] [mailto:linux-usb- [email protected]] On Behalf Of Elina Pasheva
Sent: Wednesday, April 07, 2010 5:53 AM
To: [email protected]
Cc: [email protected]; [email protected]; [email protected]; [email protected]
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 <[email protected]> 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 <[email protected]>
Signed-off-by Rory Filer <[email protected]>
---
 drivers/net/usb/usbnet.c |   15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)
drivers/net/usb/usbnet.c +0 -5
@@ -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);
drivers/net/usb/usbnet.c netdev_tx_t usbnet_start_xmit (struct sk · line 1080
Quoted 10 lines
drivers/net/usb/usbnet.c +9 -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.

Quoted 4 lines

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 [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

David Brownell

2010-04-07 21:14 UTC #5 lore raw

In reply to Elina Pasheva

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 [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

Elina Pasheva

2010-04-07 23:07 UTC #7 lore raw

In reply to a message not in the archive

Quoted 14 lines · 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.

Quoted 6 lines

... 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 [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html