Vicam/3com homeconnect usb camera driver

This time with the patch actually attached!

to

and

In vicam_v4l_open:

Why is only the first control message checked for errors?

vicam_usb_probe:

__devinit ???

vicam_usb_disconnect:

__devexit ???
And you should probably kill the tasklet before you unregister the video device.

Regards
Oliver

PS: Is that just me, or did diff produce particularly unreadable output this time?

The second one turns on the LED. I didn't check it because I figured the LED actually turning on was not a big deal. Though, I suppose that if an error occurred, that could be indicative of some other problem.

I'm not sure I see the problem here. __devinit is only defined when HOTPLUG is not defined, which seems right to me. If there is no HOTPLUG then we can throw away the code as soon as init is completed. ...similar argument for __devexit. Correct me if I'm wrong.

Makes sense.

Probably. The existing driver always crashed on me. I started fresh with this one partly as a learning experience. Therefore, the diff probably isn't very readable since it is completely removing the old driver and putting this one in its place.

I'll re-diff after the open and __devinit/__devexit discussion gets cleared up.

Thanks,
John

Exactly.

But usbcore will happily call probe if HOTPLUG is not defined and AFAICT usb will compile without HOTPLUG.

Regards
Oliver

Fixed.

I thought HOTPLUG affected the user's ability to add and remove USB devices while the kernel is running. It would seem that I misunderstood (or, in less formal terms, was wrong :)).

Attached is a patch against 2.5.40 with __dev* uses removed and the error checking in the open routine fixed.

Let me know if the ordering of the video_unregister_device and tasklet_kill is still an issue.

Thanks,
John

It isn't. But the disconnect is still wrong. You fail to unlink the current urb. This has to be done before you kill the tasklet. And you have to use a flag and a spinlock to guard against a race with the completion handler. There's a recent discussion on this in the usb archives. And you need to defer freeing the memory if the device is open.
Have a look at how pwc does it. It should be correct in that regard.

And while you at it, could you rename the tasklet from ...bh... to ...tl... ? It's no longer a bottom half.

Regards
Oliver

Well, here we go again, there are other issues as well.
There's a race between open() and disconnect(). The best way to deal with it is to introduce a common semaphore for all devices the driver handles and to take it as the first thing open() and disconnect() do and release it as the last thing. In addition every device needs a flag to show that it has been opened and a flag to show that it has been unplugged. Alternatively you could introduce device states, being "present and unused", "present and used" and "unplugged but used". As this needs to be checked in release(), it needs to take the semaphore as well.

Regards
Oliver

[snip]

I believe this addresses (and hopefully corrects) this issues raised about the race between disconnect and the device being open. It should also correct the unlinking of the urb problem you brought up.

I even removed the ...bh... from the tasklet name.

Thanks,
John

Hi,

+ if ( waitqueue_active( &vicam_v4l_priv->no_users ) ) {
+ wake_up( &vicam_v4l_priv->no_users );

never ever do this. Always do the wake_up unconditionally.

ioctl() - VIDIOSYNC: this may hang if you unplug at the wrong moment, as there's nobody to up the semaphore in that case.
You should do the up in the disconnect handler and check for disconnection in the ioctl so you can return -ENODEV in that case.

in disconnect:

+ /* make sure no one will submit another urb */
+ clear_bit( 0, &vicam_v4l_priv->vicam_present );

But it does not guard against control messages in flight. You need a semaphore to do that.

+ usb_unlink_urb( vicam_v4l_priv->urb );
+ down( &vicam_v4l_priv->busy_mutex );

This can hang for two reasons,
- you might not be the only user of that semaphore
- usb_unlink_urb may fail due to there being no queued urb, eg. if the completion handler is running - you need to check the return value

+ if ( vdev->users ) {
+ sleep_on( &vicam_v4l_priv->no_users );
+ }

_Very_ bad idea.
First, never use sleep_on. Use the appropriate macro. Second, you have blocked khubd without an upper time limit. That you _must_ _not_ in any case do.

+ kfree( vicam_v4l_priv );
+ kfree( vicam_usb_priv );

Potentionally deadly if the device is open, you need to defer it to release in that case

Sorry for all that trouble.

Regards
Oliver