Hi,
I'm removing Greg and the linux-kernel list from To/CC of this thread. I
think Greg gets it from the usb-devel list, and I don't want to take up
more bandwidth on the linux-kernel list with this single usb issue. :)
Now, on to the fun stuff.
> + 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.
That's what I thought, but some other drivers did it this way. I'll put it
back for unconditional wake_up.
> 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.
Well, that's why I used down_interruptible (so it could be Ctrl+C'd or
killed through some other mechanism). Even if no one up's the semaphore,
when the process gets some signal, it would return -EINTR. ENODEV is
probably more accurate, though.
> 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.
You're right that it doesn't stop a control message in flight. I don't
understand how to use a semaphore to do that, though. If it's possible to
submit an urb synchronously, that would work too, since the unlink in
disconnect would pick it up.
> + 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
If another user of the semaphore is woken first, they will fail when
trying to send a control message and return an error (up'ing the semaphore
on their way out).
> - 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 there is no urb to dequeue (eg. the completion handler is running),
then we wait for the semaphore which signals that the tasklet has been
scheduled and we can now kill it without fear of it being rescheduled.
> + if ( vdev->users ) {
> + sleep_on( &vicam_v4l_priv->no_users );
> + }
>
> _Very_ bad idea.
> First, never use sleep_on. Use the appropriate macro.
What's wrong with sleep_on? What is the appropriate macro?
> Second, you have blocked khubd without an upper time limit.
> That you _must_ _not_ in any case do.
I see the argument, but it's not unbounded. Since we've set all of the
flags to keep anyone from talking to the device, they'll all exit with an
error as soon as they run again. The PWC code looks like it does
essentially the same thing here (which is why I did it).
> + 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
I thought the same thing, but since the code just before this
unregisters the device and waits for any users to release the device, they
should all be gone by the time this memory is freed.
> Sorry for all that trouble.
Not at all. I actually really appreciate all the "trouble".
Thanks,
John
-------------------------------------------------------
This sf.net email is sponsored by:ThinkGeek
Welcome to geek heaven.
http://thinkgeek.com/sf
_______________________________________________
[EMAIL PROTECTED]
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
