Oliver, I appreciate your feedback.
On Mon, Sep 7, 2009 at 8:31 AM, Oliver Neukum<oliver@xxxxxxxxxx> wrote:
> static void ipheth_carrier_check_work(struct work_struct *work)
> {
> struct ipheth_device *dev = container_of(work, struct ipheth_device,
> carrier_work.work);
>
> ipheth_carrier_set(dev);
> schedule_delayed_work(&dev->carrier_work, IPHETH_CARRIER_CHECK_TIMEOUT);
> }
>
> Doing this means that it is impossible to have runtime power management
> for the device. Is there an alternative?
Yes, I believe there is. To be honest, since this is my first Linux
kernel driver, I didn't pay much attention to power management. I will
work on this.
> 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);
>
> This needs error checking.
Agreed.
> usb_clear_halt(udev, usb_rcvbulkpipe(udev, dev->bulk_in));
> usb_clear_halt(udev, usb_sndbulkpipe(udev, dev->bulk_out));
>
> Is this really needed?
Well, I tried to be as close as possible to the inner workings of
Apple proprietary driver on Windows, which also does this.
> exit:
> return NETDEV_TX_OK;
>
> Even in an error case?
>From what I've seen on existing network drivers, this appears to be a
common practice. Basically when experiencing an error on hard_xmit
function, you drop the packet, update counters (tx_dropped++), free
the skb and tell the network subsystem you toke care of that skb by
returning 0 (NETDEV_TX_OK).
> /* 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 extend macros to specify the interface you want instead of
> checking in probe()
Agreed.
> static void ipheth_disconnect(struct usb_interface *intf)
> {
> struct ipheth_device *dev;
>
> dev = usb_get_intfdata(intf);
> if (dev != NULL) {
>
> How can this be NULL?
I'm not sure, perhaps a race between probe/disconnect? Is that
possible? Or am I just being paranoid? :-)
--
Diego Giagio
diego@xxxxxxxxxx / diego@xxxxxxxxxxx
--
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
