On Wed, Sep 18, 2013 at 9:59 PM, Bjørn Mork <bjorn@xxxxxxx> wrote:
> Ming Lei <ming.lei@xxxxxxxxxxxxx> 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@xxxxxxxxxx>
>> Cc: Oliver Neukum <oliver@xxxxxxxxxx>
>> Signed-off-by: Ming Lei <ming.lei@xxxxxxxxxxxxx>
>> ---
>> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
