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-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
