lkml.org 
[lkml]   [2010]   [Mar]   [31]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRe: [PATCHv3] drivers/net/usb: Add new driver ipheth
Date
Am Mittwoch, 31. März 2010 21:42:07 schrieb L. Alberto Giménez:

Hi,

a few comments below.

Regards
Oliver

> +
> +static struct usb_device_id ipheth_table[] = {
> + { USB_DEVICE(USB_VENDOR_APPLE, USB_PRODUCT_IPHETH) },
> + { USB_DEVICE(USB_VENDOR_APPLE, USB_PRODUCT_IPHETH_3G) },
> + { USB_DEVICE(USB_VENDOR_APPLE, USB_PRODUCT_IPHETH_3GS) },

Please use the macros specifying the interface.

> + { }
> +};
> +MODULE_DEVICE_TABLE(usb, ipheth_table);


> +static void ipheth_unlink_urbs(struct ipheth_device *dev)

The naming might be misleading.

> +{
> + usb_kill_urb(dev->tx_urb);
> + usb_kill_urb(dev->rx_urb);
> +}

> +
> +static void ipheth_sndbulk_callback(struct urb *urb)
> +{
> + struct ipheth_device *dev;
> +
> + dev = urb->context;
> + if (dev == NULL)
> + return;
> +
> + if (urb->status != 0 &&
> + urb->status != -ENOENT &&
> + urb->status != -ECONNRESET &&
> + urb->status != -ESHUTDOWN)
> + err("%s: urb status: %d", __func__, urb->status);
> +
> + netif_wake_queue(dev->net);
> + dev_kfree_skb_irq(dev->tx_skb);

Race condition. You must free the skb before you wake the queue.

> +}

> +
> +static int ipheth_open(struct net_device *net)
> +{
> + struct ipheth_device *dev = netdev_priv(net);
> + struct usb_device *udev = dev->udev;
> + int retval = 0;
> +
> + usb_set_interface(udev, IPHETH_INTFNUM, IPHETH_ALT_INTFNUM);
> + usb_clear_halt(udev, usb_rcvbulkpipe(udev, dev->bulk_in));
> + usb_clear_halt(udev, usb_sndbulkpipe(udev, dev->bulk_out));

Is this really needed? If so, please add a comment.

> +
> + retval = ipheth_carrier_set(dev);
> + if (retval)
> + goto error;
> +
> + retval = ipheth_rx_submit(dev, GFP_KERNEL);
> + if (retval)
> + goto error;
> +
> + schedule_delayed_work(&dev->carrier_work, IPHETH_CARRIER_CHECK_TIMEOUT);

Does it make sense to start rx while you have no carrier?

> + netif_start_queue(net);
> +error:
> + return retval;
> +}

> +static int ipheth_tx(struct sk_buff *skb, struct net_device *net)
> +{
> + struct ipheth_device *dev = netdev_priv(net);
> + struct usb_device *udev = dev->udev;
> + int retval;
> +
> + /* Paranoid */
> + if (skb->len > IPHETH_BUF_SIZE) {
> + err("%s: skb too large: %d bytes", __func__, skb->len);
> + dev->stats.tx_dropped++;
> + dev_kfree_skb_irq(skb);
> + goto exit;
> + }
> +
> + memset(dev->tx_buf, 0, IPHETH_BUF_SIZE);
> + memcpy(dev->tx_buf, skb->data, skb->len);

a bit wasteful

> +static int ipheth_probe(struct usb_interface *intf,
> + const struct usb_device_id *id)
> +{
> + struct usb_device *udev = interface_to_usbdev(intf);
> + struct usb_host_interface *hintf;
> + struct usb_endpoint_descriptor *endp;
> + struct ipheth_device *dev;
> + struct net_device *netdev;
> + int i;
> + int retval;
> +
> + /* Ensure we are probing the right interface */
> + if (intf->cur_altsetting->desc.bInterfaceClass != IPHETH_USBINTF_CLASS ||
> + intf->cur_altsetting->desc.bInterfaceSubClass != IPHETH_USBINTF_SUBCLASS)
> + return -ENODEV;

Please use the correct macro to specify the interface you are interested in

> +static void ipheth_disconnect(struct usb_interface *intf)
> +{
> + struct ipheth_device *dev;
> +
> + dev = usb_get_intfdata(intf);
> + if (dev != NULL) {

is this check needed?

> +static struct usb_driver ipheth_driver = {
> + .name = "ipheth",
> + .probe = ipheth_probe,
> + .disconnect = ipheth_disconnect,
> + .id_table = ipheth_table,
> + .supports_autosuspend = 0,

redundant
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

\
 
 \ /
  Last update: 2010-03-31 22:35    [from the cache]
©2003-2011 Jasper Spaans