[PATCH] USBNET: fix handling padding packet

[PATCH] USBNET: fix handling padding packet

[Ming Lei]

Commit 638c5115a7949(USBNET: support DMA SG) introduces DMA SG
if the usb host controller is capable of building packet from
discontinuous buffers, but missed handling padding packet when
building DMA SG.

This patch attachs the pre-allocated padding packet at the
end of the sg list, so padding packet can be sent to device
if drivers require that.

Reported-by: David Laight <David.Laight-JxhZ9S5GRejQT0dZR+AlfA@public.gmane.org>
Cc: Oliver Neukum <oliver-GvhC2dPhHPQdnm+yROfE0A@public.gmane.org>
Signed-off-by: Ming Lei <ming.lei-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org>
---
 drivers/net/usb/usbnet.c   |   27 +++++++++++++++++++++------
 include/linux/usb/usbnet.h |    1 +
 2 files changed, 22 insertions(+), 6 deletions(-)

diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c
index 7b331e6..4a9bed3 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->padding_pkt, 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->padding_pkt);
 
 	free_netdev(net);
 }
@@ -1679,9 +1685,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->padding_pkt = kzalloc(1, GFP_KERNEL);
+		if (!dev->padding_pkt)
+			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,
@@ -1699,6 +1712,8 @@ usbnet_probe (struct usb_interface *udev, const struct usb_device_id *prod)
 
 	return 0;
 
+out5:
+	kfree(dev->padding_pkt);
 out4:
 	usb_free_urb(dev->interrupt);
 out3:
diff --git a/include/linux/usb/usbnet.h b/include/linux/usb/usbnet.h
index 9cb2fe8..e303eef 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;
+	const char		*padding_pkt;
 
 	/* protocol/interface state */
 	struct net_device	*net;
-- 
1.7.9.5

--
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] USBNET: fix handling padding packet

[David Miller]

From: Ming Lei <ming.lei-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org>
Date: Tue, 17 Sep 2013 17:10:02 +0800

> Commit 638c5115a7949(USBNET: support DMA SG) introduces DMA SG
> if the usb host controller is capable of building packet from
> discontinuous buffers, but missed handling padding packet when
> building DMA SG.
> 
> This patch attachs the pre-allocated padding packet at the
> end of the sg list, so padding packet can be sent to device
> if drivers require that.
> 
> Reported-by: David Laight <David.Laight-JxhZ9S5GRejQT0dZR+AlfA@public.gmane.org>
> Cc: Oliver Neukum <oliver-GvhC2dPhHPQdnm+yROfE0A@public.gmane.org>
> Signed-off-by: Ming Lei <ming.lei-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org>

Some reviews and ACKs would be appreciated on this one, thanks.
--
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] USBNET: fix handling padding packet

[Oliver Neukum]

On Tue, 2013-09-17 at 17:10 +0800, Ming Lei wrote:
> Commit 638c5115a7949(USBNET: support DMA SG) introduces DMA SG
> if the usb host controller is capable of building packet from
> discontinuous buffers, but missed handling padding packet when
> building DMA SG.
> 
> This patch attachs the pre-allocated padding packet at the
> end of the sg list, so padding packet can be sent to device
> if drivers require that.
> 
> Reported-by: David Laight <David.Laight-JxhZ9S5GRejQT0dZR+AlfA@public.gmane.org>
> Cc: Oliver Neukum <oliver-GvhC2dPhHPQdnm+yROfE0A@public.gmane.org>
> Signed-off-by: Ming Lei <ming.lei-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org>
Acked-by: Oliver Neukum <oliver-GvhC2dPhHPQdnm+yROfE0A@public.gmane.org>


--
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] USBNET: fix handling padding packet

[Bjørn Mork]

Ming Lei <ming.lei@canonical.com> writes:

> Commit 638c5115a7949(USBNET: support DMA SG) introduces DMA SG
> if the usb host controller is capable of building packet from
> discontinuous buffers, but missed handling padding packet when
> building DMA SG.
>
> This patch attachs the pre-allocated padding packet at the
> end of the sg list, so padding packet can be sent to device
> if drivers require that.
>
> Reported-by: David Laight <David.Laight@aculab.com>
> Cc: Oliver Neukum <oliver@neukum.org>
> Signed-off-by: Ming Lei <ming.lei@canonical.com>
> ---
>  drivers/net/usb/usbnet.c   |   27 +++++++++++++++++++++------
>  include/linux/usb/usbnet.h |    1 +
>  2 files changed, 22 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c
> index 7b331e6..4a9bed3 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->padding_pkt, 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->padding_pkt);
>  
>  	free_netdev(net);
>  }
> @@ -1679,9 +1685,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->padding_pkt = kzalloc(1, GFP_KERNEL);
> +		if (!dev->padding_pkt)
> +			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,
> @@ -1699,6 +1712,8 @@ usbnet_probe (struct usb_interface *udev, const struct usb_device_id *prod)
>  
>  	return 0;
>  
> +out5:
> +	kfree(dev->padding_pkt);
>  out4:
>  	usb_free_urb(dev->interrupt);
>  out3:
> diff --git a/include/linux/usb/usbnet.h b/include/linux/usb/usbnet.h
> index 9cb2fe8..e303eef 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;
> +	const char		*padding_pkt;
>  
>  	/* protocol/interface state */
>  	struct net_device	*net;


The code handling the frame padding was already unecessarily complex
IMHO, and this does not improve it...

Are you really sure that the one driver/device using this really need
the padding byte?  If you could just make FLAG_SEND_ZLP part of the
condition for enabling can_dma_sg, then all this extra complexity would
be unnecessary.  As the comment in front of the padding code says:

  "strictly conforming cdc-ether devices should expect the ZLP here"

There shouldn't be any problems requiring this conformance as a
precondition for allowing SG.  The requirements are already strict.


I also believe it would be nice to move the initialisation of can_dma_sg
away from the minidriver and into usbnet_probe.  It's confusing that
this field is used "uninitialized" (well, defaulting to zero) in all but
one minidriver.  It would much nicer if the logic was more like

usbnet_probe:
 if (...)
    dev->can_dma_sg = 1;

minidriver_bind:
  if (dev->can_dma_sg) {
     dev->net->features |= NETIF_F_SG | NETIF_F_TSO;
     dev->net->hw_features |= NETIF_F_SG | NETIF_F_TSO;
  }

usbnet_start_xmit:
 if (skb_shinfo(skb)->nr_frags) {
    ...
 }

This would create a natural flow of events, where probing sets the
capability, minidriver enables features based on capability, and xmit
just does what it has to do based on the skb it was given.

Or maybe even better: factor out common parts of usbnet_start_xmit and
create two versions of it - one using SG and one linear.  The per packet
testing seems unnecessary expensive and complex given that the choice is
made during device initialization anyway.  You could easily replace the
whole can_dma_sg stuff with a simple 

 if (...)
	net->netdev_ops = &usbnet_sg_netdev_ops;
 else
	net->netdev_ops = &usbnet_netdev_ops;


in usbnet_probe.  But I guess the same can be said about the info->flags
testing in usbnet_start_xmit...

Just a few random thougths.  I don't really feel strongly about any of
this, except that I would prefer usbnet becoming *more* readable instead
of less...


Bjørn

Re: [PATCH] USBNET: fix handling padding packet

[Ming Lei]

On Wed, Sep 18, 2013 at 9:59 PM, Bjørn Mork <bjorn@mork.no> wrote:
> Ming Lei <ming.lei@canonical.com> writes:
>
>> Commit 638c5115a7949(USBNET: support DMA SG) introduces DMA SG
>> if the usb host controller is capable of building packet from
>> discontinuous buffers, but missed handling padding packet when
>> building DMA SG.
>>
>> This patch attachs the pre-allocated padding packet at the
>> end of the sg list, so padding packet can be sent to device
>> if drivers require that.
>>
>> Reported-by: David Laight <David.Laight@aculab.com>
>> Cc: Oliver Neukum <oliver@neukum.org>
>> Signed-off-by: Ming Lei <ming.lei@canonical.com>
>> ---
>>  drivers/net/usb/usbnet.c   |   27 +++++++++++++++++++++------
>>  include/linux/usb/usbnet.h |    1 +
>>  2 files changed, 22 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c
>> index 7b331e6..4a9bed3 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->padding_pkt, 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->padding_pkt);
>>
>>       free_netdev(net);
>>  }
>> @@ -1679,9 +1685,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->padding_pkt = kzalloc(1, GFP_KERNEL);
>> +             if (!dev->padding_pkt)
>> +                     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,
>> @@ -1699,6 +1712,8 @@ usbnet_probe (struct usb_interface *udev, const struct usb_device_id *prod)
>>
>>       return 0;
>>
>> +out5:
>> +     kfree(dev->padding_pkt);
>>  out4:
>>       usb_free_urb(dev->interrupt);
>>  out3:
>> diff --git a/include/linux/usb/usbnet.h b/include/linux/usb/usbnet.h
>> index 9cb2fe8..e303eef 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;
>> +     const char              *padding_pkt;
>>
>>       /* protocol/interface state */
>>       struct net_device       *net;
>
>
> The code handling the frame padding was already unecessarily complex
> IMHO, and this does not improve it...
>
> Are you really sure that the one driver/device using this really need
> the padding byte?  If you could just make FLAG_SEND_ZLP part of the
> condition for enabling can_dma_sg, then all this extra complexity would
> be unnecessary.  As the comment in front of the padding code says:

At least for the effected driver of ax88179, the padding packet is needed
since the driver does set the padding flag in the header, see
ax88179_tx_fixup().

>
>   "strictly conforming cdc-ether devices should expect the ZLP here"
>
> There shouldn't be any problems requiring this conformance as a
> precondition for allowing SG.  The requirements are already strict.

There is no reason to forbid DMA SG for one driver which requires
padding, right?

>
>
> I also believe it would be nice to move the initialisation of can_dma_sg
> away from the minidriver and into usbnet_probe.  It's confusing that
> this field is used "uninitialized" (well, defaulting to zero) in all but
> one minidriver.  It would much nicer if the logic was more like

Looks the above and below isn't related with the patch closely, and
patch is welcome to simplify code.

>
> usbnet_probe:
>  if (...)
>     dev->can_dma_sg = 1;
>
> minidriver_bind:
>   if (dev->can_dma_sg) {
>      dev->net->features |= NETIF_F_SG | NETIF_F_TSO;
>      dev->net->hw_features |= NETIF_F_SG | NETIF_F_TSO;
>   }
>
> usbnet_start_xmit:
>  if (skb_shinfo(skb)->nr_frags) {
>     ...
>  }
>
> This would create a natural flow of events, where probing sets the
> capability, minidriver enables features based on capability, and xmit
> just does what it has to do based on the skb it was given.
>
> Or maybe even better: factor out common parts of usbnet_start_xmit and
> create two versions of it - one using SG and one linear.  The per packet
> testing seems unnecessary expensive and complex given that the choice is
> made during device initialization anyway.  You could easily replace the
> whole can_dma_sg stuff with a simple
>
>  if (...)
>         net->netdev_ops = &usbnet_sg_netdev_ops;
>  else
>         net->netdev_ops = &usbnet_netdev_ops;
>
>
> in usbnet_probe.  But I guess the same can be said about the info->flags
> testing in usbnet_start_xmit...
>
> Just a few random thougths.  I don't really feel strongly about any of
> this, except that I would prefer usbnet becoming *more* readable instead
> of less...


Thanks,
--
Ming Lei

RE: [PATCH] USBNET: fix handling padding packet

[David Laight]

> I also believe it would be nice to move the initialisation of can_dma_sg
> away from the minidriver and into usbnet_probe.  It's confusing that
> this field is used "uninitialized" (well, defaulting to zero) in all but
> one minidriver.  It would much nicer if the logic was more like
> 
> usbnet_probe:
>  if (...)
>     dev->can_dma_sg = 1;
> 
> minidriver_bind:
>   if (dev->can_dma_sg) {
>      dev->net->features |= NETIF_F_SG | NETIF_F_TSO;
>      dev->net->hw_features |= NETIF_F_SG | NETIF_F_TSO;
>   }

Actually it would probably be nicer if the minidriver set
a flag to indicate that it could support fragmented skb
(a lot can't because of the way they add trailers)
and also provided the length of the header it will add.

The usbnet code could then allocate the header space.
If scatter-gather dma is available (a host feature) then
the header can be allocated outside the skb data area
to avoid having to copy the entire skb data.
The check for ZLP avoidance could then be done once only
(not sure how the info would be passed to teh minidriver apart
from adding more additional parameters to teh tx_fixup function).

I did a quick scan of the minidrivers, some of them don't
seem to have the correct checks for fragmented skb (etc).

Most of them only add a header.

	David

Re: [PATCH] USBNET: fix handling padding packet

[Bjørn Mork]

Ming Lei <ming.lei@canonical.com> writes:

>> Are you really sure that the one driver/device using this really need
>> the padding byte?  If you could just make FLAG_SEND_ZLP part of the
>> condition for enabling can_dma_sg, then all this extra complexity would
>> be unnecessary.  As the comment in front of the padding code says:
>
> At least for the effected driver of ax88179, the padding packet is needed
> since the driver does set the padding flag in the header, see
> ax88179_tx_fixup().

Yes, I noticed that the driver doesn't set the flag. I just didn't put
too much into that.  I don't think that necessarily means that the
padding is needed. It probably just means that the driver worked with
the default padding enabled, so the author left it there.

This flag should really have been inverted.  ZLP should be the default
for all new usbnet drivers.

Why don't you test it on the device you tested the SG patch with?  I am
pretty sure it works just fine using proper ZLP transfer termination.

>>   "strictly conforming cdc-ether devices should expect the ZLP here"
>>
>> There shouldn't be any problems requiring this conformance as a
>> precondition for allowing SG.  The requirements are already strict.
>
> There is no reason to forbid DMA SG for one driver which requires
> padding, right?

Yes there is: Added complexity for everybody, based on a combination of
features which just does not make any sense.

No modern device should need the padding.  No old device will be able to
use the SG feature as implemented. You only enable it on USB3, don't
you? If this feature is restricted to USB3 capable devices, then it most
certainly can be restricted to ZLP capable devices with absolutely no
difference in the resulting set of supported devices.

Anyway, if you want to keep the padding for SG then maybe this will work
and allow you to drop the extra struct usbnet field and allocation:

                        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++], skb->data, 1);
                        }

I.e. cheat and use the skb->data buffer twice, if that is allowed?  The
actual value of the padding byte should not matter, I believe?



Bjørn

Re: [PATCH] USBNET: fix handling padding packet

[David Miller]

From: Bjørn Mork <bjorn@mork.no>
Date: Wed, 18 Sep 2013 17:52:42 +0200

> Ming Lei <ming.lei@canonical.com> writes:
> 
>> There is no reason to forbid DMA SG for one driver which requires
>> padding, right?
> 
> Yes there is: Added complexity for everybody, based on a combination of
> features which just does not make any sense.
> 
> No modern device should need the padding.  No old device will be able to
> use the SG feature as implemented. You only enable it on USB3, don't
> you? If this feature is restricted to USB3 capable devices, then it most
> certainly can be restricted to ZLP capable devices with absolutely no
> difference in the resulting set of supported devices.

I completely agree.

Re: [PATCH] USBNET: fix handling padding packet

[Ming Lei]

On Wed, Sep 18, 2013 at 11:52 PM, Bjørn Mork <bjorn-yOkvZcmFvRU@public.gmane.org> wrote:
> Ming Lei <ming.lei-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org> writes:
>
>>> Are you really sure that the one driver/device using this really need
>>> the padding byte?  If you could just make FLAG_SEND_ZLP part of the
>>> condition for enabling can_dma_sg, then all this extra complexity would
>>> be unnecessary.  As the comment in front of the padding code says:
>>
>> At least for the effected driver of ax88179, the padding packet is needed
>> since the driver does set the padding flag in the header, see
>> ax88179_tx_fixup().
>
> Yes, I noticed that the driver doesn't set the flag. I just didn't put
> too much into that.  I don't think that necessarily means that the
> padding is needed. It probably just means that the driver worked with
> the default padding enabled, so the author left it there.
>
> This flag should really have been inverted.  ZLP should be the default
> for all new usbnet drivers.

In theory, it is, but who knows the reality.

>
> Why don't you test it on the device you tested the SG patch with?  I am
> pretty sure it works just fine using proper ZLP transfer termination.

I should have planned to test it, but didn't know how to build TCP TSO
to make the whole frame length plus 8 dividable by 1024.

Could you or other guys share how to build such packet so that I can
do the test?

Once we can confirm the padding isn't needed, we can remove the
padding flag. But if the padding flag is still there, this patch should be
dropped.

>
>>>   "strictly conforming cdc-ether devices should expect the ZLP here"
>>>
>>> There shouldn't be any problems requiring this conformance as a
>>> precondition for allowing SG.  The requirements are already strict.
>>
>> There is no reason to forbid DMA SG for one driver which requires
>> padding, right?
>
> Yes there is: Added complexity for everybody, based on a combination of
> features which just does not make any sense.
>
> No modern device should need the padding.  No old device will be able to
> use the SG feature as implemented. You only enable it on USB3, don't
> you? If this feature is restricted to USB3 capable devices, then it most
> certainly can be restricted to ZLP capable devices with absolutely no
> difference in the resulting set of supported devices.

See above, if we can prove that padding isn't needed, we can remove
the padding, then the patch can be dropped. If we can't, the patch is needed.

>
> Anyway, if you want to keep the padding for SG then maybe this will work
> and allow you to drop the extra struct usbnet field and allocation:
>
>                         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++], skb->data, 1);
>                         }
>
> I.e. cheat and use the skb->data buffer twice, if that is allowed?  The
> actual value of the padding byte should not matter, I believe?

If so, we can remove the assignment of zero to skb->data[skb->len],
but probably it might cause regression, that is why I wouldn't like to
do that.  Also looks introducing one extra dev->padding_frame doesn't
cause much complexity.

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

Re: [PATCH] USBNET: fix handling padding packet

[Oliver Neukum]

On Wed, 2013-09-18 at 17:52 +0200, Bjørn Mork wrote:

> No modern device should need the padding.  No old device will be able to
> use the SG feature as implemented. You only enable it on USB3, don't

On XHCI.

> you? If this feature is restricted to USB3 capable devices, then it most
> certainly can be restricted to ZLP capable devices with absolutely no
> difference in the resulting set of supported devices.

No, USB 3.0 uses no companion controllers, so you can have devices
of any speed connected to it.

> Anyway, if you want to keep the padding for SG then maybe this will work
> and allow you to drop the extra struct usbnet field and allocation:
> 
>                         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++], skb->data, 1);
>                         }
> 
> I.e. cheat and use the skb->data buffer twice, if that is allowed?  The
> actual value of the padding byte should not matter, I believe?

That makes me immediately suspect a violation of the DMA rules.

	Regards
		Oliver

Re: [PATCH] USBNET: fix handling padding packet

[Bjørn Mork]

Ming Lei <ming.lei@canonical.com> writes:
> On Wed, Sep 18, 2013 at 11:52 PM, Bjørn Mork <bjorn@mork.no> wrote:
>
>> Why don't you test it on the device you tested the SG patch with?  I am
>> pretty sure it works just fine using proper ZLP transfer termination.
>
> I should have planned to test it, but didn't know how to build TCP TSO
> to make the whole frame length plus 8 dividable by 1024.

You could test this without enabling TSO and just send IP packets of the
appriopriate size, taking any additional framing into account


Bjørn

Re: [PATCH] USBNET: fix handling padding packet

[Bjørn Mork]

Oliver Neukum <oneukum@suse.de> wrote:
>On Wed, 2013-09-18 at 17:52 +0200, Bjørn Mork wrote:
>
>> No modern device should need the padding.  No old device will be able
>to
>> use the SG feature as implemented. You only enable it on USB3, don't
>
>On XHCI.
>
>> you? If this feature is restricted to USB3 capable devices, then it
>most
>> certainly can be restricted to ZLP capable devices with absolutely no
>> difference in the resulting set of supported devices.
>
>No, USB 3.0 uses no companion controllers, so you can have devices
>of any speed connected to it.
>

Ah, right. I don't own such modern hardware, but I should have known this anyway. 

This still doesn't change the fact that the driver is brand new for brand new devices. I believe we should assume such devices will support ZLPs unless we have documentation stating anything else.

>> Anyway, if you want to keep the padding for SG then maybe this will
>work
>> and allow you to drop the extra struct usbnet field and allocation:
>> 
>>                         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++],
>skb->data, 1);
>>                         }
>> 
>> I.e. cheat and use the skb->data buffer twice, if that is allowed? 
>The
>> actual value of the padding byte should not matter, I believe?
>
>That makes me immediately suspect a violation of the DMA rules.

Sounds likely. And it's an ugly hack in any case. Probably not a good idea. Just one of the many random thoughts I should have kept to myself :-)


Bjørn 

Re: [PATCH] USBNET: fix handling padding packet

[Oliver Neukum]

On Wed, 2013-09-18 at 20:56 +0200, Bjørn Mork wrote:
> Oliver Neukum <oneukum@suse.de> wrote:

> >No, USB 3.0 uses no companion controllers, so you can have devices
> >of any speed connected to it.
> >
> 
> Ah, right. I don't own such modern hardware, but I should have known this anyway. 
> 
> This still doesn't change the fact that the driver is brand new for brand new devices. I believe we should assume such devices will support ZLPs unless we have documentation stating anything else.

For such devices we might assume it. But how does that matter for
generic code? As any kind of device may be connected to XHCI, the sg
code is relevant for every driver. And I certainly don't want trouble
for older devices' drivers converted to sg.

	Regards
		Oliver

Re: [PATCH] USBNET: fix handling padding packet

[Bjørn Mork]

Oliver Neukum <oneukum-l3A5Bk7waGM@public.gmane.org> writes:
> On Wed, 2013-09-18 at 20:56 +0200, Bjørn Mork wrote:
>> Oliver Neukum <oneukum-l3A5Bk7waGM@public.gmane.org> wrote:
>
>> >No, USB 3.0 uses no companion controllers, so you can have devices
>> >of any speed connected to it.
>> >
>> 
>> Ah, right. I don't own such modern hardware, but I should have known
>> this anyway.
>> 
>> This still doesn't change the fact that the driver is brand new for
>> brand new devices. I believe we should assume such devices will
>> support ZLPs unless we have documentation stating anything else.
>
> For such devices we might assume it. But how does that matter for
> generic code?

The code isn't generic yet.  Most of it is placed inside the
ax88179_178a minidriver.

But I do agree that adding this padding support can be seen as one step
towards making the code generic.  So if you really anticipate this being
enabled for e.g. the ECM class driver, then yes, padding must be
supported.

I would have had less trouble understanding the value if this patch was
part of a generalisation series, though...

> As any kind of device may be connected to XHCI, the sg
> code is relevant for every driver. And I certainly don't want trouble
> for older devices' drivers converted to sg.

I wonder what the gain of that really is?  Yes, I can see the advantage
of making the class drivers more effective.  But padding is only
relevant for the ECM class, isn't it? And are there any ECM class
devices where SG support matters?


Bjørn
--
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] USBNET: fix handling padding packet

[David Laight]

> I wonder what the gain of that really is?  Yes, I can see the advantage
> of making the class drivers more effective.  But padding is only
> relevant for the ECM class, isn't it? And are there any ECM class
> devices where SG support matters?

AFAICT the requirement for avoiding ZLP is a property of the slave.
Support for scatter-gather is a property of the host.

So connect an old slave to a modern host and you may be able to use
sg to add the header or padding (and possibly have a fragmented skb).

Generating frames that would have a ZLP should just be a matter of
sending UDP frames with ever increasing lengths - although a
printf in the driver would confirm it.

However the is a report (somewhere) that it would work sometimes
and not others with certain targets.

I've got one of the ASIX USB3 devices here, but the system is running
ubuntu 12.04 - which doesn't have the sg changes. I've not looked far
enough to see if the sg changes in usbnet.c can be applied to that
kernel.

	David

Re: [PATCH] USBNET: fix handling padding packet

[Ming Lei]

On Thu, Sep 19, 2013 at 3:18 PM, Bjørn Mork <bjorn-yOkvZcmFvRU@public.gmane.org> wrote:
> Oliver Neukum <oneukum-l3A5Bk7waGM@public.gmane.org> writes:
>> On Wed, 2013-09-18 at 20:56 +0200, Bjørn Mork wrote:
>>> Oliver Neukum <oneukum-l3A5Bk7waGM@public.gmane.org> wrote:
>>
>>> >No, USB 3.0 uses no companion controllers, so you can have devices
>>> >of any speed connected to it.
>>> >
>>>
>>> Ah, right. I don't own such modern hardware, but I should have known
>>> this anyway.
>>>
>>> This still doesn't change the fact that the driver is brand new for
>>> brand new devices. I believe we should assume such devices will
>>> support ZLPs unless we have documentation stating anything else.
>>
>> For such devices we might assume it. But how does that matter for
>> generic code?
>
> The code isn't generic yet.  Most of it is placed inside the
> ax88179_178a minidriver.

No, the patch doesn't touch code of ax99179_178a.

And it is really generic to fix the padding in case of dma sg.

>
> But I do agree that adding this padding support can be seen as one step
> towards making the code generic.  So if you really anticipate this being
> enabled for e.g. the ECM class driver, then yes, padding must be
> supported.

1byte padding frame is already supported by usbnet before, isn't it?

>
> I would have had less trouble understanding the value if this patch was
> part of a generalisation series, though...

As my below test on ax99179_178a, I believe the patch should fix padding
for dma sg, but need a little update, and I will send out v1 later:

           $ping -s 974 another_machine  #from host with ax99179_178a attached

If FLAG_SEND_ZLP is set for ax99179_178a, the above ping won't work any
more either on USB3.0 or USB 2.0 host controller.

So don't assume that these brand new devices can support ZLP well.

>
>> As any kind of device may be connected to XHCI, the sg
>> code is relevant for every driver. And I certainly don't want trouble
>> for older devices' drivers converted to sg.
>
> I wonder what the gain of that really is?  Yes, I can see the advantage
> of making the class drivers more effective.  But padding is only
> relevant for the ECM class, isn't it? And are there any ECM class
> devices where SG support matters?

Why is padding only relevant for the ECM? Of course, other devices
need it too, such as asix devices.

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

RE: [PATCH] USBNET: fix handling padding packet

[David Laight]

> As my below test on ax99179_178a, I believe the patch should fix padding
> for dma sg, but need a little update, and I will send out v1 later:
> 
>            $ping -s 974 another_machine  #from host with ax99179_178a attached
> 
> If FLAG_SEND_ZLP is set for ax99179_178a, the above ping won't work any
> more either on USB3.0 or USB 2.0 host controller.
> 
> So don't assume that these brand new devices can support ZLP well.

I've just posted a fix to the xhci driver to implement URB_ZERO_PACKET.
With that fix (and ZLP enabled in the ax88179_178a driver) the above
ping works fine (ax88179 Ge card on USB3).

I wonder how many other usb drivers fail to support URB_ZERO_PACKET.
Maybe the driver should pass a flag to its users (along with SG support).

I've also posted (to linux-usb) another fix to the xhci driver that is
needed to get SG (and one that cross 64k boundaries) transfers working.

	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