usbnet transmit path problems

usbnet transmit path problems

[David Laight]

I've been looking at the code in drivers/net/usb/usbnet.c that
processes tx data after the tx_fixup() function has run.

The code currently looks like:

	usb_fill_bulk_urb (urb, dev->udev, dev->out,
			skb->data, skb->len, tx_complete, skb);
	if (dev->can_dma_sg) {
		if (build_dma_sg(skb, urb) < 0)
			goto drop;
	}
	entry->length = length = urb->transfer_buffer_length;

	/* don't assume the hardware handles USB_ZERO_PACKET
	 * NOTE:  strictly conforming cdc-ether devices should expect
	 * the ZLP here, but ignore the one-byte packet.
	 * NOTE2: CDC NCM specification is different from CDC ECM when
	 * handling ZLP/short packets, so cdc_ncm driver will make short
	 * packet itself if needed.
	 */
	if (length % dev->maxpacket == 0) {
		if (!(info->flags & FLAG_SEND_ZLP)) {
			if (!(info->flags & FLAG_MULTI_PACKET)) {
				urb->transfer_buffer_length++;
				if (skb_tailroom(skb)) {
					skb->data[skb->len] = 0;
					__skb_put(skb, 1);
				}
			}
		} else
			urb->transfer_flags |= URB_ZERO_PACKET;
	}

1) I can't see where skb_linearize() gets called if 'can_dma_sg' is unset.

2) If 'length % dev->maxpacket == 0' for a multi-fragment packet then
   the extra byte isn't added correctly (the code probably falls off
   the end of the scatter-gather list).

3) If 'length % dev->maxpacket == 0' for a single-fragment packet then
   shouldn't there be a check that the skb isn't shared before modifying it?
   Also if there is no tailroom increasing the usb transfer length might
   result in reading an unmapped memory location.

Have I missed something, or is all this code in the wrong order?

4) I read that USB3 has a different scheme for terminating bulk data
   that is a multiple of the packet size.
   Does this mean that the pad byte isn't needed for USB3?
   (Or are USB3 controllers/targets just as buggy?)

	David

Re: usbnet transmit path problems

[Oliver Neukum]

On Wed, 2013-09-11 at 10:10 +0100, David Laight wrote:
> I've been looking at the code in drivers/net/usb/usbnet.c that
> processes tx data after the tx_fixup() function has run.
> 
> The code currently looks like:

> 1) I can't see where skb_linearize() gets called if 'can_dma_sg' is unset.

That is the job of subdrivers.

> 2) If 'length % dev->maxpacket == 0' for a multi-fragment packet then
>    the extra byte isn't added correctly (the code probably falls off
>    the end of the scatter-gather list).

Indeed. Ming Lei, should usbnet handle this in the sg case or better
leave it to the subdriver you introduced this for?

> 4) I read that USB3 has a different scheme for terminating bulk data
>    that is a multiple of the packet size.
>    Does this mean that the pad byte isn't needed for USB3?
>    (Or are USB3 controllers/targets just as buggy?)

We don't have enough examples to tell.

	Regards
		Oliver

Re: usbnet transmit path problems

[Ming Lei]

On Wed, 11 Sep 2013 12:11:52 +0200
Oliver Neukum <oliver@neukum.org> wrote:

> On Wed, 2013-09-11 at 10:10 +0100, David Laight wrote:
> > I've been looking at the code in drivers/net/usb/usbnet.c that
> > processes tx data after the tx_fixup() function has run.
> > 
> > The code currently looks like:
> 
> > 1) I can't see where skb_linearize() gets called if 'can_dma_sg' is unset.
> 
> That is the job of subdrivers.
> 
> > 2) If 'length % dev->maxpacket == 0' for a multi-fragment packet then
> >    the extra byte isn't added correctly (the code probably falls off
> >    the end of the scatter-gather list).
> 
> Indeed. Ming Lei, should usbnet handle this in the sg case or better
> leave it to the subdriver you introduced this for?

IMO, it should be handled by usbnet, could you comment on below patch?

diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c
index 534b60b..929270b 100644
--- a/drivers/net/usb/usbnet.c
+++ b/drivers/net/usb/usbnet.c
@@ -1241,7 +1241,9 @@ static int build_dma_sg(const struct sk_buff *skb, struct urb *urb)
 	if (num_sgs == 1)
 		return 0;
 
-	urb->sg = kmalloc(num_sgs * sizeof(struct scatterlist), GFP_ATOMIC);
+	/* reserve one for zero packet */
+	urb->sg = kmalloc((num_sgs + 1) * sizeof(struct scatterlist),
+			  GFP_ATOMIC);
 	if (!urb->sg)
 		return -ENOMEM;
 
@@ -1305,7 +1307,7 @@ netdev_tx_t usbnet_start_xmit (struct sk_buff *skb,
 		if (build_dma_sg(skb, urb) < 0)
 			goto drop;
 	}
-	entry->length = length = urb->transfer_buffer_length;
+	length = urb->transfer_buffer_length;
 
 	/* don't assume the hardware handles USB_ZERO_PACKET
 	 * NOTE:  strictly conforming cdc-ether devices should expect
@@ -1317,15 +1319,18 @@ netdev_tx_t usbnet_start_xmit (struct sk_buff *skb,
 	if (length % dev->maxpacket == 0) {
 		if (!(info->flags & FLAG_SEND_ZLP)) {
 			if (!(info->flags & FLAG_MULTI_PACKET)) {
-				urb->transfer_buffer_length++;
-				if (skb_tailroom(skb)) {
+				length++;
+				if (skb_tailroom(skb) && !dev->can_dma_sg) {
 					skb->data[skb->len] = 0;
 					__skb_put(skb, 1);
-				}
+				} else if (dev->can_dma_sg)
+					sg_set_buf(&urb->sg[urb->num_sgs++],
+							dev->zlp, 1);
 			}
 		} else
 			urb->transfer_flags |= URB_ZERO_PACKET;
 	}
+	entry->length = urb->transfer_buffer_length = length;
 
 	spin_lock_irqsave(&dev->txq.lock, flags);
 	retval = usb_autopm_get_interface_async(dev->intf);
@@ -1509,6 +1514,7 @@ void usbnet_disconnect (struct usb_interface *intf)
 
 	usb_kill_urb(dev->interrupt);
 	usb_free_urb(dev->interrupt);
+	kfree(dev->zlp);
 
 	free_netdev(net);
 }
@@ -1675,9 +1681,16 @@ usbnet_probe (struct usb_interface *udev, const struct usb_device_id *prod)
 	/* initialize max rx_qlen and tx_qlen */
 	usbnet_update_max_qlen(dev);
 
+	if (dev->can_dma_sg && !(info->flags & FLAG_SEND_ZLP) &&
+		!(info->flags & FLAG_MULTI_PACKET)) {
+		dev->zlp = kzalloc(1, GFP_KERNEL);
+		if (!dev->zlp)
+			goto out4;
+	}
+
 	status = register_netdev (net);
 	if (status)
-		goto out4;
+		goto out5;
 	netif_info(dev, probe, dev->net,
 		   "register '%s' at usb-%s-%s, %s, %pM\n",
 		   udev->dev.driver->name,
@@ -1695,6 +1708,8 @@ usbnet_probe (struct usb_interface *udev, const struct usb_device_id *prod)
 
 	return 0;
 
+out5:
+	kfree(dev->zlp);
 out4:
 	usb_free_urb(dev->interrupt);
 out3:
diff --git a/include/linux/usb/usbnet.h b/include/linux/usb/usbnet.h
index 9cb2fe8..3f1b081 100644
--- a/include/linux/usb/usbnet.h
+++ b/include/linux/usb/usbnet.h
@@ -42,6 +42,7 @@ struct usbnet {
 	struct usb_host_endpoint *status;
 	unsigned		maxpacket;
 	struct timer_list	delay;
+	char			*zlp;
 
 	/* protocol/interface state */
 	struct net_device	*net;


> 
> > 4) I read that USB3 has a different scheme for terminating bulk data
> >    that is a multiple of the packet size.
> >    Does this mean that the pad byte isn't needed for USB3?
> >    (Or are USB3 controllers/targets just as buggy?)
> 
> We don't have enough examples to tell.

I should depend on how device deals with termination of bulk data.


Thanks,
-- 
Ming Lei

RE: usbnet transmit path problems

[David Laight]

> > > 2) If 'length % dev->maxpacket == 0' for a multi-fragment packet then
> > >    the extra byte isn't added correctly (the code probably falls off
> > >    the end of the scatter-gather list).
> >
> > Indeed. Ming Lei, should usbnet handle this in the sg case or better
> > leave it to the subdriver you introduced this for?

Is the ZLP issue a problem with the host or with the target?
If it is a host problem then the necessity comes from the host,
but the fix needs to be target dependant.
If it is a common target problem then generic code can apply
a common fix.

AFICT there are at least 3 fixes:
1) Extend the ethernet frame by one byte and hope the receiving
   system doesn't object to the padding.
   This is probably the only option if tx_fixup() doesn't
   add a header.
2) Put the ethernet frame length in the header and have the
   target discard the added pad byte (ax88179_178a.c).
3) Add a second zero-length frame in the same USB data block
   (ax88172a.c).

Only the third requires that tx_fixup() append to the packet.
For the other two actual pad can be added by usbnet.
 
> IMO, it should be handled by usbnet, could you comment on below patch?

Seems excessive to kmalloc() one byte!
If you can't assume that the 'dev' structure itself can be dma'd from
allocate the extra byte in the sg list.

	David

RE: usbnet transmit path problems

[David Laight]

> > 4) I read that USB3 has a different scheme for terminating bulk data
> >    that is a multiple of the packet size.
> >    Does this mean that the pad byte isn't needed for USB3?
> >    (Or are USB3 controllers/targets just as buggy?)
> 
> We don't have enough examples to tell.

Perhaps there should be a separate flag for USB3?
A device might have issues on USB2, but not USB3!

The flag is target specific - so I presume it isn't
a host/sending problem.

	David

Re: usbnet transmit path problems

[Ming Lei]

On Wed, Sep 11, 2013 at 8:56 PM, David Laight <David.Laight@aculab.com> wrote:
>> > > 2) If 'length % dev->maxpacket == 0' for a multi-fragment packet then
>> > >    the extra byte isn't added correctly (the code probably falls off
>> > >    the end of the scatter-gather list).
>> >
>> > Indeed. Ming Lei, should usbnet handle this in the sg case or better
>> > leave it to the subdriver you introduced this for?
>
> Is the ZLP issue a problem with the host or with the target?

Sorry, what do you mean the ZLP issue here? I understand Oliver
thinks one commit from me may break ZLP handling, are you discussing
this problem? If not, could you explain it in a bit detail?

> If it is a host problem then the necessity comes from the host,
> but the fix needs to be target dependant.
> If it is a common target problem then generic code can apply
> a common fix.

All usbnet device should have sent one ZLP in case the size of
bulk out transfer can be divided by max packet size, but the one
byte transfer might be introduced for avoiding some target problem
(can't deal with zlp well), as said by David, see below discussion:

   http://marc.info/?l=linux-usb&m=127067487604112&w=2

>
> AFICT there are at least 3 fixes:
> 1) Extend the ethernet frame by one byte and hope the receiving
>    system doesn't object to the padding.
>    This is probably the only option if tx_fixup() doesn't
>    add a header.
> 2) Put the ethernet frame length in the header and have the
>    target discard the added pad byte (ax88179_178a.c).
> 3) Add a second zero-length frame in the same USB data block
>    (ax88172a.c).

Why do we need the above 3 fixes? The patch in my last email can
fix the problem which is introduced recently, can't it?

>
> Only the third requires that tx_fixup() append to the packet.
> For the other two actual pad can be added by usbnet.
>
>> IMO, it should be handled by usbnet, could you comment on below patch?
>
> Seems excessive to kmalloc() one byte!

It isn't strange, many usb drivers have to do that(maybe kmalloc
two or three, or four bytes) since the buffer is involved into DMA.

> If you can't assume that the 'dev' structure itself can be dma'd from
> allocate the extra byte in the sg list.

It is better to always obey rule of DMA-API, so don't do that since
one extra kmalloc() per device is needed, even though we can allocate
only one global buffer for this purpose.


Thanks,
--
Ming Lei

RE: usbnet transmit path problems

[David Laight]

> On Wed, Sep 11, 2013 at 8:56 PM, David Laight <David.Laight-JxhZ9S5GRejQT0dZR+AlfA@public.gmane.org> wrote:
> >> > > 2) If 'length % dev->maxpacket == 0' for a multi-fragment packet then
> >> > >    the extra byte isn't added correctly (the code probably falls off
> >> > >    the end of the scatter-gather list).
> >> >
> >> > Indeed. Ming Lei, should usbnet handle this in the sg case or better
> >> > leave it to the subdriver you introduced this for?
> >
> > Is the ZLP issue a problem with the host or with the target?
> 
> Sorry, what do you mean the ZLP issue here? I understand Oliver
> thinks one commit from me may break ZLP handling, are you discussing
> this problem? If not, could you explain it in a bit detail?

I was thinking of the general ZLP problem.
 
> > If it is a host problem then the necessity comes from the host,
> > but the fix needs to be target dependant.
> > If it is a common target problem then generic code can apply
> > a common fix.
> 
> All usbnet device should have sent one ZLP in case the size of
> bulk out transfer can be divided by max packet size, but the one
> byte transfer might be introduced for avoiding some target problem
> (can't deal with zlp well), as said by David, see below discussion:
> 
>    http://marc.info/?l=linux-usb&m=127067487604112&w=2

AFAICT the code avoids sending a zero length packet (that would
terminate a USB bulk transfer packet) by increasing the length
of the bulk packet by (at least) one byte.

> > AFICT there are at least 3 fixes:
> > 1) Extend the ethernet frame by one byte and hope the receiving
> >    system doesn't object to the padding.
> >    This is probably the only option if tx_fixup() doesn't
> >    add a header.
> > 2) Put the ethernet frame length in the header and have the
> >    target discard the added pad byte (ax88179_178a.c).
> > 3) Add a second zero-length frame in the same USB data block
> >    (ax88172a.c).
> 
> Why do we need the above 3 fixes? The patch in my last email can
> fix the problem which is introduced recently, can't it?

I meant there are 3 ways of avoiding the ZLP, each driver will
pick one of them.

I've just looked at all the drivers in net/usb.
It doesn't look like they all handle fragmented skb, shared skb,
or ZLP properly.

A lot of common code could be removed if usbnet knew the size of the
header and allocated it before calling tx_fixup().

None of this is helping me sort out why netperf udp rr tests with
burst 19 are losing all the packets at once :-(

	David



--
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: usbnet transmit path problems

[Ming Lei]

On Thu, Sep 12, 2013 at 12:05 AM, David Laight <David.Laight@aculab.com> wrote:
>> On Wed, Sep 11, 2013 at 8:56 PM, David Laight <David.Laight@aculab.com> wrote:
>> >> > > 2) If 'length % dev->maxpacket == 0' for a multi-fragment packet then
>> >> > >    the extra byte isn't added correctly (the code probably falls off
>> >> > >    the end of the scatter-gather list).
>> >> >
>> >> > Indeed. Ming Lei, should usbnet handle this in the sg case or better
>> >> > leave it to the subdriver you introduced this for?
>> >
>> > Is the ZLP issue a problem with the host or with the target?
>>
>> Sorry, what do you mean the ZLP issue here? I understand Oliver
>> thinks one commit from me may break ZLP handling, are you discussing
>> this problem? If not, could you explain it in a bit detail?
>
> I was thinking of the general ZLP problem.

IMO the current approach should be general enough.

>
>> > If it is a host problem then the necessity comes from the host,
>> > but the fix needs to be target dependant.
>> > If it is a common target problem then generic code can apply
>> > a common fix.
>>
>> All usbnet device should have sent one ZLP in case the size of
>> bulk out transfer can be divided by max packet size, but the one
>> byte transfer might be introduced for avoiding some target problem
>> (can't deal with zlp well), as said by David, see below discussion:
>>
>>    http://marc.info/?l=linux-usb&m=127067487604112&w=2
>
> AFAICT the code avoids sending a zero length packet (that would
> terminate a USB bulk transfer packet) by increasing the length
> of the bulk packet by (at least) one byte.

No, the code on the link supports URB_ZERO_PACKET which
should be the decent approach for the problem.

>
>> > AFICT there are at least 3 fixes:
>> > 1) Extend the ethernet frame by one byte and hope the receiving
>> >    system doesn't object to the padding.

This is what usbnet is doing at default if both FLAG_SEND_ZLP and
FLAG_MULTI_PACKET aren't set.

>> >    This is probably the only option if tx_fixup() doesn't
>> >    add a header.
>> > 2) Put the ethernet frame length in the header and have the
>> >    target discard the added pad byte (ax88179_178a.c).

That is probably because the target need the padding flag, so that
it can skip the one byte padding frame.

>> > 3) Add a second zero-length frame in the same USB data block
>> >    (ax88172a.c).

Actually it is a 4byte frame added by the subdriver, instead of adding
one zlp.  Since the subdriver adds the termination packet by itself, usbnet
won't handle the case at all.

There is no much difference among the 3 appoaches, all of which take
the padding trick, and the difference is that if padding flag is required, and
if the padding length is one byte or others, and both are device-dependent.

So current approach is still general enough to cover all cases, isn't it?

>>
>> Why do we need the above 3 fixes? The patch in my last email can
>> fix the problem which is introduced recently, can't it?
>
> I meant there are 3 ways of avoiding the ZLP, each driver will
> pick one of them.

All the 3 ways are basically same or very similar, and the difference is
that drivers may need the padding flag or take different length termination
packet.

>
> I've just looked at all the drivers in net/usb.
> It doesn't look like they all handle fragmented skb, shared skb,
> or ZLP properly.
>
> A lot of common code could be removed if usbnet knew the size of the
> header and allocated it before calling tx_fixup().

Patches are always welcome, :-)

> None of this is helping me sort out why netperf udp rr tests with
> burst 19 are losing all the packets at once :-(

If you can share your test case, guys in list may help you to troubleshoot
it, :-)


Thanks,
--
Ming Lei

Re: usbnet transmit path problems

[Oliver Neukum]

On Thu, 2013-09-12 at 09:56 +0800, Ming Lei wrote:
> On Thu, Sep 12, 2013 at 12:05 AM, David Laight <David.Laight-JxhZ9S5GRejQT0dZR+AlfA@public.gmane.org> wrote:

> Patches are always welcome, :-)

Indeed, I think your patch, if no better alternatives come up soon,
should be taken.

> > None of this is helping me sort out why netperf udp rr tests with
> > burst 19 are losing all the packets at once :-(
> 
> If you can share your test case, guys in list may help you to troubleshoot
> it, :-)

Indeed.

	Regards
		Oliver


--
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: usbnet transmit path problems

[Ming Lei]

On Mon, Sep 16, 2013 at 4:13 PM, Oliver Neukum <oliver-GvhC2dPhHPQdnm+yROfE0A@public.gmane.org> wrote:
> On Thu, 2013-09-12 at 09:56 +0800, Ming Lei wrote:
>> On Thu, Sep 12, 2013 at 12:05 AM, David Laight <David.Laight-JxhZ9S5GRejQT0dZR+AlfA@public.gmane.org> wrote:
>
>> Patches are always welcome, :-)
>
> Indeed, I think your patch, if no better alternatives come up soon,
> should be taken.

OK, will submit it later.

Thanks,
--
Ming Lei
--
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