lkml.org 
[lkml]   [2010]   [Mar]   [31]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: [PATCHv3] drivers/net/usb: Add new driver ipheth
    From
    Date
    On Wed, 2010-03-31 at 21:42 +0200, L. Alberto Giménez wrote:
    [...]
    > --- /dev/null
    > +++ b/drivers/net/usb/ipheth.c
    [...]
    > +#include <linux/kernel.h>
    > +#include <linux/errno.h>
    > +#include <linux/init.h>
    > +#include <linux/slab.h>
    > +#include <linux/module.h>
    > +#include <linux/netdevice.h>
    > +#include <linux/etherdevice.h>
    > +#include <linux/ethtool.h>
    > +#include <linux/uaccess.h>

    I don't think you need this header.

    > +#include <linux/usb.h>
    > +#include <linux/workqueue.h>
    > +
    > +#define USB_VENDOR_APPLE 0x05ac
    > +#define USB_PRODUCT_IPHETH 0x1290
    > +#define USB_PRODUCT_IPHETH_3G 0x1292
    > +#define USB_PRODUCT_IPHETH_3GS 0x1294

    Apple doesn't assign device ids to ipheth so either the names are
    incorrect or you should get proper device ids. I believe the Linux
    Foundation has a USB vendor id and could assign device ids under that.

    > +struct ipheth_device {
    > + struct usb_device *udev;
    > + struct usb_interface *intf;
    > + struct net_device *net;
    > + struct net_device_stats stats;

    You can store stats in the net_device.

    > + struct sk_buff *tx_skb;
    > + struct urb *tx_urb;
    > + struct urb *rx_urb;
    > + unsigned char *tx_buf;
    > + unsigned char *rx_buf;
    > + unsigned char *ctrl_buf;
    > + __u8 bulk_in;
    > + __u8 bulk_out;

    No need for the double-underscores; that's a convention for use in
    definitions shared with user-space.

    > + struct delayed_work carrier_work;
    > +};
    [...]
    > +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));
    > +
    > + 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);
    > + netif_start_queue(net);
    > +error:
    > + return retval;
    > +}

    There is no cleanup here (and none appears to be needed) so you could
    take out the label and replace the gotos with returns.

    > +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;
    > + }

    Use WARN here as this would indicate a bug.

    Again the goto is unnecessary.

    > + memset(dev->tx_buf, 0, IPHETH_BUF_SIZE);
    > + memcpy(dev->tx_buf, skb->data, skb->len);

    This is wasteful. If you really must pad the buffer then do so, but
    don't clear the area that is going to be filled with real data.

    > + usb_fill_bulk_urb(dev->tx_urb, udev,
    > + usb_sndbulkpipe(udev, dev->bulk_out),
    > + dev->tx_buf, IPHETH_BUF_SIZE,
    > + ipheth_sndbulk_callback,
    > + dev);
    > + dev->tx_urb->transfer_flags |= URB_NO_TRANSFER_DMA_MAP;
    > +
    > + retval = usb_submit_urb(dev->tx_urb, GFP_ATOMIC);
    > + if (retval) {
    > + err("%s: usb_submit_urb: %d", __func__, retval);
    > + dev->stats.tx_errors++;
    > + dev_kfree_skb_irq(skb);
    > + } else {
    > + net->trans_start = jiffies;

    No longer needed.

    > + dev->tx_skb = skb;
    > +
    > + dev->stats.tx_packets++;
    > + dev->stats.tx_bytes += skb->len;
    > + netif_stop_queue(net);
    > + }
    > +exit:
    > + return NETDEV_TX_OK;
    > +}
    [...]
    > +#ifdef HAVE_NET_DEVICE_OPS
    > +static const struct net_device_ops ipheth_netdev_ops = {
    > + .ndo_open = &ipheth_open,
    > + .ndo_stop = &ipheth_close,
    > + .ndo_start_xmit = &ipheth_tx,
    > + .ndo_tx_timeout = &ipheth_tx_timeout,
    > + .ndo_get_stats = &ipheth_stats,
    > +};
    > +#endif

    Remove the #ifdef, there is no question whether we have net_device_ops.

    > +static int ipheth_probe(struct usb_interface *intf,
    > + const struct usb_device_id *id)
    > +{
    [...]
    > +#ifdef HAVE_NET_DEVICE_OPS
    > + netdev->netdev_ops = &ipheth_netdev_ops;
    > +#else /* CONFIG_COMPAT_NET_DEV_OPS */
    > + netdev->open = &ipheth_open;
    > + netdev->stop = &ipheth_close;
    > + netdev->hard_start_xmit = &ipheth_tx;
    > + netdev->tx_timeout = &ipheth_tx_timeout;
    > + netdev->get_stats = &ipheth_stats;
    > +#endif
    [...]

    Similarly here.

    As a general point, you should now be using netdev_err(), netdev_info(),
    etc. for logging messages related to net devices.

    I have no idea about USB so I haven't checked the USB API usage at all.

    Ben.

    --
    Ben Hutchings, Senior Software Engineer, Solarflare Communications
    Not speaking for my employer; that's the marketing department's job.
    They asked us to note that Solarflare product names are trademarked.

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