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    [W:0.030 / U:30.440 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site