lkml.org 
[lkml]   [2010]   [Apr]   [2]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCHv3] drivers/net/usb: Add new driver ipheth
    On 03/31/2010 10:33 PM, Oliver Neukum wrote:
    > Am Mittwoch, 31. März 2010 21:42:07 schrieb L. Alberto Giménez:

    Hi Oliver,

    Just like with Ben's comments I still have a couple of doubts about your
    comments.


    >> +
    >> +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.

    I understand that usb_clear_halt is only needed when the device has
    transmitted data, and as it is "open" time, we can assume that no
    transmissions ere made, so we don't need to clear anything (aka: remove
    both lines), am I right?


    >> +
    >> + 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?

    Well, I have no clue about this one. I think that upstream developers
    should take a look into this (Dario, Daniel, could you?) since I don't
    have the knowledge to decide what to do about it.

    But I assume that as with the previous one, we have just opened the
    device and we aren't (yet) doing anything with it, so we shouldn't start rx?

    >> +static void ipheth_disconnect(struct usb_interface *intf)
    >> +{
    >> + struct ipheth_device *dev;
    >> +
    >> + dev = usb_get_intfdata(intf);
    >> + if (dev != NULL) {
    >
    > is this check needed?

    Does usb_get_infdata always return not NULL? I haven't found anything
    about it (just manual pages for the function, but can't spot if it
    cannot return NULL). We disconnected the device, but I understand that
    the kernel still has the information and the allocated memory, so the
    cleanup code is still needed, isn't it?


    >
    >> +static struct usb_driver ipheth_driver = {
    >> + .name = "ipheth",
    >> + .probe = ipheth_probe,
    >> + .disconnect = ipheth_disconnect,
    >> + .id_table = ipheth_table,
    >> + .supports_autosuspend = 0,
    >
    > redundant

    Why?

    Thanks a lot in advance.


    Best regards,
    --
    L. Alberto Giménez
    JabberID agimenez@jabber.sysvalve.es
    GnuPG key ID 0x3BAABDE1
    --
    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-02 20:25    [W:2.324 / U:0.008 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site