Am Freitag, 2. April 2010 20:23:21 schrieb L. Alberto Giménez:
> 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?
Clearing a halt is necessary only when a device has stalled due to an
error condition. Unless the device is buggy and produces errors for
no good reason you don't need these lines.
> >> +
> >> + 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?
Your code as is is correct, I just wondered whether it could be made more
efficient.
> >> +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
It returns what you gave it with usb_set_intfdata().
> 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?
It is definitely needed.
> >> +static struct usb_driver ipheth_driver = {
> >> + .name = "ipheth",
> >> + .probe = ipheth_probe,
> >> + .disconnect = ipheth_disconnect,
> >> + .id_table = ipheth_table,
> >> + .supports_autosuspend = 0,
> >
> > redundant
>
> Why?
0 is the default.
Regards
Oliver
--
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
