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 [email protected]
GnuPG key ID 0x3BAABDE1
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
On 04/04/2010 09:24 AM, Oliver Neukum wrote:
> 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:
>>>> +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.
Heh, I thought that you meant that the whole struct was redundant, and
that puzzled me a little bit. Now everything is clear (my fault for not
realizing the redundant initialization to 0).
Regards,
--
L. Alberto Giménez
JabberID [email protected]
GnuPG key ID 0x3BAABDE1