From: Oliver Neukum <oliver@neukum.org>
To: "L. Alberto Giménez" <agimenez@sysvalve.es>
Cc: linux-kernel@vger.kernel.org, netdev@vger.kernel.org,
linux-usb@vger.kernel.org, linville@tuxdriver.com,
j.dumon@option.com, steve.glendinning@smsc.com,
davem@davemloft.net, gregkh@suse.de, dgiagio@gmail.com,
dborca@yahoo.com
Subject: Re: [PATCHv3] drivers/net/usb: Add new driver ipheth
Date: Wed, 31 Mar 2010 22:33:26 +0200 [thread overview]
Message-ID: <201003312233.26130.oliver@neukum.org> (raw)
In-Reply-To: <1270064527-8485-1-git-send-email-agimenez@sysvalve.es>
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
next prev parent reply other threads:[~2010-03-31 20:33 UTC|newest]
Thread overview: 35+ messages / expand[flat|nested] mbox.gz Atom feed top
2010-03-30 21:34 [PATCH] Staging: Add new driver ipheth L. Alberto Giménez
2010-03-30 21:34 ` L. Alberto Giménez
2010-03-30 21:45 ` Greg KH
2010-03-30 21:58 ` "L. Alberto Giménez"
2010-03-30 22:11 ` Greg KH
2010-03-31 14:33 ` Pavel Machek
2010-03-31 14:44 ` Matthew Garrett
2010-03-31 19:47 ` "L. Alberto Giménez"
2010-03-30 23:01 ` [PATCH] drivers/net/usb: " L. Alberto Giménez
2010-03-30 23:25 ` Greg KH
2010-03-31 19:42 ` [PATCHv3] " L. Alberto Giménez
2010-03-31 20:33 ` Oliver Neukum [this message]
2010-03-31 21:38 ` "L. Alberto Giménez"
2010-04-02 18:23 ` "L. Alberto Giménez"
2010-04-04 7:24 ` Oliver Neukum
2010-04-05 18:51 ` "L. Alberto Giménez"
2010-03-31 23:18 ` Ben Hutchings
2010-03-31 23:25 ` Greg KH
2010-03-31 23:28 ` Ben Hutchings
2010-04-02 17:15 ` "L. Alberto Giménez"
2010-04-02 17:21 ` Ben Hutchings
2010-04-02 17:53 ` "L. Alberto Giménez"
2010-04-02 18:35 ` Ben Hutchings
2010-04-07 22:11 ` [PATCH Resubmission] " L. Alberto Giménez
2010-04-07 22:37 ` Roland Dreier
2010-04-08 6:35 ` Oliver Neukum
2010-04-13 8:15 ` David Miller
2010-04-13 19:03 ` "L. Alberto Giménez"
2010-04-13 21:29 ` David Miller
2010-04-15 19:46 ` [PATCH Resubmission v2] " L. Alberto Giménez
2010-04-16 6:44 ` David Miller
2010-04-18 18:35 ` [PATCH] " L. Alberto Giménez
2010-04-21 14:15 ` Diego Giagio
2010-04-22 5:44 ` David Miller
2010-03-31 22:10 [PATCHv3] " Daniel Borca
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=201003312233.26130.oliver@neukum.org \
--to=oliver@neukum.org \
--cc=agimenez@sysvalve.es \
--cc=davem@davemloft.net \
--cc=dborca@yahoo.com \
--cc=dgiagio@gmail.com \
--cc=gregkh@suse.de \
--cc=j.dumon@option.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-usb@vger.kernel.org \
--cc=linville@tuxdriver.com \
--cc=netdev@vger.kernel.org \
--cc=steve.glendinning@smsc.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).