[PATCH] Make sure gspca cleans up USB resources during disconnect

[PATCH] Make sure gspca cleans up USB resources during disconnect

[Adam Baker]

If a device using the gspca framework is unplugged while it is still streaming
then the call that is used to free the URBs that have been allocated occurs
after the pointer it uses becomes invalid at the end of gspca_disconnect.
Make another cleanup call in gspca_disconnect while the pointer is still
valid (multiple calls are OK as destroy_urbs checks for pointers already
being NULL.

Signed-off-by: Adam Baker <linux@baker-net.org.uk>

---
diff -r 4d0827823ebc linux/drivers/media/video/gspca/gspca.c
--- a/linux/drivers/media/video/gspca/gspca.c	Tue Feb 03 10:42:28 2009 +0100
+++ b/linux/drivers/media/video/gspca/gspca.c	Tue Feb 03 23:07:34 2009 +0000
@@ -434,6 +434,7 @@ static void destroy_urbs(struct gspca_de
 		if (urb == NULL)
 			break;
 
+		BUG_ON(!gspca_dev->dev);
 		gspca_dev->urb[i] = NULL;
 		if (gspca_dev->present)
 			usb_kill_urb(urb);
@@ -1953,8 +1954,12 @@ void gspca_disconnect(struct usb_interfa
 {
 	struct gspca_dev *gspca_dev = usb_get_intfdata(intf);
 
+	mutex_lock(&gspca_dev->usb_lock);
 	gspca_dev->present = 0;
+	mutex_unlock(&gspca_dev->usb_lock);
 
+	destroy_urbs(gspca_dev);
+	gspca_dev->dev = NULL;
 	usb_set_intfdata(intf, NULL);
 
 	/* release the device */

Re: [PATCH] Make sure gspca cleans up USB resources during disconnect

[kilgota]

On Tue, 3 Feb 2009, Adam Baker wrote:

> If a device using the gspca framework is unplugged while it is still streaming
> then the call that is used to free the URBs that have been allocated occurs
> after the pointer it uses becomes invalid at the end of gspca_disconnect.
> Make another cleanup call in gspca_disconnect while the pointer is still
> valid (multiple calls are OK as destroy_urbs checks for pointers already
> being NULL.
>
> Signed-off-by: Adam Baker <linux@baker-net.org.uk>
>
> ---
> diff -r 4d0827823ebc linux/drivers/media/video/gspca/gspca.c
> --- a/linux/drivers/media/video/gspca/gspca.c	Tue Feb 03 10:42:28 2009 +0100
> +++ b/linux/drivers/media/video/gspca/gspca.c	Tue Feb 03 23:07:34 2009 +0000
> @@ -434,6 +434,7 @@ static void destroy_urbs(struct gspca_de
> 		if (urb == NULL)
> 			break;
>
> +		BUG_ON(!gspca_dev->dev);
> 		gspca_dev->urb[i] = NULL;
> 		if (gspca_dev->present)
> 			usb_kill_urb(urb);
> @@ -1953,8 +1954,12 @@ void gspca_disconnect(struct usb_interfa
> {
> 	struct gspca_dev *gspca_dev = usb_get_intfdata(intf);
>
> +	mutex_lock(&gspca_dev->usb_lock);
> 	gspca_dev->present = 0;
> +	mutex_unlock(&gspca_dev->usb_lock);
>
> +	destroy_urbs(gspca_dev);
> +	gspca_dev->dev = NULL;
> 	usb_set_intfdata(intf, NULL);
>
> 	/* release the device */
>

Again, this solves the problem completely on the Pentium 4 Dual Core 
machine. Again, on the K8 machine there is the error message

libv4l2: error dequeuing buf: Resource temporarily unavailable

which, I strongly suspect, has nothing at all to do with the issue at 
hand.

I have at this point switched over to

create_singlethread_workqueue(MODULE_NAME)

on both boxes now. I think that your patch would run with this or without 
it. So the question is, whether singlethread is in fact better, or not. 
Me, I would tend to think it really makes no difference at all, because 
that was not what the problem was.

So, the next question is whether this patch gets accepted or not. It 
appears to solve *our* problem. Clearly, the big question is whether it 
could break something else.

Theodore Kilgore

Re: [PATCH] Make sure gspca cleans up USB resources during disconnect

[Jean-Francois Moine]

On Tue, 3 Feb 2009 23:13:17 +0000
Adam Baker <linux@baker-net.org.uk> wrote:

> If a device using the gspca framework is unplugged while it is still
> streaming then the call that is used to free the URBs that have been
> allocated occurs after the pointer it uses becomes invalid at the end
> of gspca_disconnect. Make another cleanup call in gspca_disconnect
> while the pointer is still valid (multiple calls are OK as
> destroy_urbs checks for pointers already being NULL.
> 
> Signed-off-by: Adam Baker <linux@baker-net.org.uk>
> 
> ---
> diff -r 4d0827823ebc linux/drivers/media/video/gspca/gspca.c
> --- a/linux/drivers/media/video/gspca/gspca.c	Tue Feb 03
> 10:42:28 2009 +0100 +++
> b/linux/drivers/media/video/gspca/gspca.c	Tue Feb 03 23:07:34
> 2009 +0000 @@ -434,6 +434,7 @@ static void destroy_urbs(struct
> gspca_de if (urb == NULL) break;
>  
> +		BUG_ON(!gspca_dev->dev);

No: this function is called on close after disconnect. when the pointer
is NULL.

>  		gspca_dev->urb[i] = NULL;
>  		if (gspca_dev->present)
>  			usb_kill_urb(urb);
> @@ -1953,8 +1954,12 @@ void gspca_disconnect(struct usb_interfa
>  {
>  	struct gspca_dev *gspca_dev = usb_get_intfdata(intf);
>  
> +	mutex_lock(&gspca_dev->usb_lock);
>  	gspca_dev->present = 0;
> +	mutex_unlock(&gspca_dev->usb_lock);

I do not see what is the use of the lock...

> +	destroy_urbs(gspca_dev);
> +	gspca_dev->dev = NULL;

As I understand, the usb device is freed at disconnection time after
the call to the (struct usb_driver *)->disconnect() function. I did not
know that and I could not find yet how! So, this is OK for me.

>  	usb_set_intfdata(intf, NULL);
>  
>  	/* release the device */

Now, as the pointer to the usb_driver may be NULL, I have to check if an
(other) oops may occur elsewhere...

Thank you.

-- 
Ken ar c'hentan	|	      ** Breizh ha Linux atav! **
Jef		|		http://moinejf.free.fr/

Re: [PATCH] Make sure gspca cleans up USB resources during disconnect

[Adam Baker]

On Wednesday 04 February 2009, Jean-Francois Moine wrote:
> On Tue, 3 Feb 2009 23:13:17 +0000
>
> Adam Baker <linux@baker-net.org.uk> wrote:
> > If a device using the gspca framework is unplugged while it is still
> > streaming then the call that is used to free the URBs that have been
> > allocated occurs after the pointer it uses becomes invalid at the end
> > of gspca_disconnect. Make another cleanup call in gspca_disconnect
> > while the pointer is still valid (multiple calls are OK as
> > destroy_urbs checks for pointers already being NULL.
> >
> > Signed-off-by: Adam Baker <linux@baker-net.org.uk>
> >
> > ---
> > diff -r 4d0827823ebc linux/drivers/media/video/gspca/gspca.c
> > --- a/linux/drivers/media/video/gspca/gspca.c	Tue Feb 03
> > 10:42:28 2009 +0100 +++
> > b/linux/drivers/media/video/gspca/gspca.c	Tue Feb 03 23:07:34
> > 2009 +0000 @@ -434,6 +434,7 @@ static void destroy_urbs(struct
> > gspca_de if (urb == NULL) break;
> >
> > +		BUG_ON(!gspca_dev->dev);
>
> No: this function is called on close after disconnect. when the pointer
> is NULL.

But at that time urb should be NULL so we don't get to the BUG_ON. If we urb 
is not NULL but gspca_dev->dv is then something has gone wrong and it is 
impossible to clean up properly.

>
> >  		gspca_dev->urb[i] = NULL;
> >  		if (gspca_dev->present)
> >  			usb_kill_urb(urb);
> > @@ -1953,8 +1954,12 @@ void gspca_disconnect(struct usb_interfa
> >  {
> >  	struct gspca_dev *gspca_dev = usb_get_intfdata(intf);
> >
> > +	mutex_lock(&gspca_dev->usb_lock);
> >  	gspca_dev->present = 0;
> > +	mutex_unlock(&gspca_dev->usb_lock);
>
> I do not see what is the use of the lock...

It ensure that if elsewhere there is the code sequence
mutex_lock
if (!dev->present)
	goto cleanup;
use usb or urb
mutex_unlock

then if it finds dev->present set it knows that the urb and usb pointers will 
remain valid pointers (even if they refer to a disconnected device) until the 
code has finished using them. 

>
> > +	destroy_urbs(gspca_dev);
> > +	gspca_dev->dev = NULL;
>
> As I understand, the usb device is freed at disconnection time after
> the call to the (struct usb_driver *)->disconnect() function. I did not
> know that and I could not find yet how! So, this is OK for me.
>
> >  	usb_set_intfdata(intf, NULL);
> >
> >  	/* release the device */
>
> Now, as the pointer to the usb_driver may be NULL, I have to check if an
> (other) oops may occur elsewhere...
>

As I said, I believe that is possible with the finepix driver and the sequence 
I quoted above with checking gspca_dev->present with usb_lock held is the fix 
but I'm not confident of fixing that completely without access to the 
hardware, especially as you can't do that in the completion handler. It 
shouldn't introduce a regression though as before you would be attempting to 
access freed memory and now you have a NULL pointer instead so such code was 
already buggy.

I have tested pulling the cable out during streaming after making this change 
on both sq905 and pac207 so whilst I can't say for certain they are OK 

Having thought about it a bit more I suspect that you should also now remove 
the if (gspca_dev->present) check on usb_kill_urb as it is possible there may 
be urbs still awaiting completion when disconnect happens.

> Thank you.

Thank You - If it wasn't for your work on gspca I'd still be using a buggy old 
driver that had no chance of making it to main line.

Adam

Re: [PATCH] Make sure gspca cleans up USB resources during disconnect

[Jean-Francois Moine]

On Wed, 4 Feb 2009 22:07:44 +0000
Adam Baker <linux@baker-net.org.uk> wrote:

> Thank You - If it wasn't for your work on gspca I'd still be using a
> buggy old driver that had no chance of making it to main line.

OK. It seems everything works fine with your webcam(s) (and the other
ones).

I added some more checks of the device presence and fixed a bit the API.

Are you ready to send me a patch for the driver?

I have just a remark: in sd_init (probe/resume), you do

	dev->work_thread = NULL;
	INIT_WORK(&dev->work_struct, sq905_dostream);

The first line is not needed, and the second should be done in
sd_config (probe only - on resume, the work will remain the same).

Also, the BUG_ON in sd_start is not needed.

About finepix, indeed, it asks for fixes, but also, it would be
simplified with a workqueue...

-- 
Ken ar c'hentañ	|	      ** Breizh ha Linux atav! **
Jef		|		http://moinejf.free.fr/

Re: [PATCH] Make sure gspca cleans up USB resources during disconnect

[kilgota]

On Thu, 5 Feb 2009, Jean-Francois Moine wrote:

> On Wed, 4 Feb 2009 22:07:44 +0000
> Adam Baker <linux@baker-net.org.uk> wrote:
>
>> Thank You - If it wasn't for your work on gspca I'd still be using a
>> buggy old driver that had no chance of making it to main line.

Well, I would not have been using it, actually. It was too much of a mess. 
As to the "thanks" I definitely second that. To put together over-arching 
projects which provide an API and a context for doing things like this is 
definitely the way to go, and this one has been put together with some 
thought, over a period of time. This brings up a question I have been 
nmeaning to ask for some time: Whie we are thanking people, it does occur 
to me to ask what has happened to Michel Xhaard. We used to correspond 
occasionally. We had different interests, with some intersection. I was 
specializing in still cameras, and he was doing the webcam project from 
which gspca has evolved. Our common interesection of interests, of course, 
were about such matters as decompression algorithms, as well as pointing 
each other to new cameras to look at. But the last time I sent Michel an 
e-mail, which was about a two months ago or so, I did not get any answer.

>
> OK. It seems everything works fine with your webcam(s) (and the other
> ones).
>
> I added some more checks of the device presence and fixed a bit the API.
>
> Are you ready to send me a patch for the driver?
>
> I have just a remark: in sd_init (probe/resume), you do
>
> 	dev->work_thread = NULL;
> 	INIT_WORK(&dev->work_struct, sq905_dostream);
>
> The first line is not needed, and the second should be done in
> sd_config (probe only - on resume, the work will remain the same).
>
> Also, the BUG_ON in sd_start is not needed.
>
> About finepix, indeed, it asks for fixes, but also, it would be
> simplified with a workqueue...

Just to be clear about this:

Jean-Francois, you sent out a separate mail about doing a pull, which, I 
assume contains the changes in gspca which you mention above, So, what you 
want now is a few minor revisions in the sq905 module, in accordance with 
what is above, and then some final testing.

Adam, I guess I will let you work on that first, unless I get nothing back 
in the next several hours. Right now, I need to take a rest. I can barely 
see the monitor. I went to a scheduled appointment this morning to the eye 
doctor and got the drops in the eyes. It takes a few hours to wear off.

Er, wait a minute. Adam, did we forget to put in the changes which will 
permit the resolution setting to be changed? Yes, I think we did, what 
with all this other excitement. So now we have the choice to try to do 
that right, once and for all, or just to send this in and try to do 
something like that later. What kind of a schedule are we supposed to 
keep, now?

That was really fun, yesterday, hooking up two cameras at once. Just 
think, we could go around with one camera on the forehead, like a miner's 
lamp, and the second one hung on the ass to see where one has been. I 
didn't know that would work. But it did. And that is kind of neat.

Theodore Kilgore

Re: [PATCH] Make sure gspca cleans up USB resources during disconnect

[Jean-Francois Moine]

On Thu, 5 Feb 2009 12:59:21 -0600 (CST)
kilgota@banach.math.auburn.edu wrote:

> 
> 
> On Thu, 5 Feb 2009, Jean-Francois Moine wrote:
> 
> > On Wed, 4 Feb 2009 22:07:44 +0000
> > Adam Baker <linux@baker-net.org.uk> wrote:
> >
> >> Thank You - If it wasn't for your work on gspca I'd still be using
> >> a buggy old driver that had no chance of making it to main line.
> 
> Well, I would not have been using it, actually. It was too much of a
> mess. As to the "thanks" I definitely second that. To put together
> over-arching projects which provide an API and a context for doing
> things like this is definitely the way to go, and this one has been
> put together with some thought, over a period of time. This brings up
> a question I have been nmeaning to ask for some time: Whie we are
> thanking people, it does occur to me to ask what has happened to
> Michel Xhaard. We used to correspond occasionally. We had different
> interests, with some intersection. I was specializing in still
> cameras, and he was doing the webcam project from which gspca has
> evolved. Our common interesection of interests, of course, were about
> such matters as decompression algorithms, as well as pointing each
> other to new cameras to look at. But the last time I sent Michel an
> e-mail, which was about a two months ago or so, I did not get any
> answer.

Same for me. Since 6 months, he did answer only one time, on the list...
I cannot even know if he is glad to have his baby in the Linux kernel.

> > OK. It seems everything works fine with your webcam(s) (and the
> > other ones).
	[snip]
> Just to be clear about this:
> 
> Jean-Francois, you sent out a separate mail about doing a pull,
> which, I assume contains the changes in gspca which you mention
> above, So, what you want now is a few minor revisions in the sq905
> module, in accordance with what is above, and then some final testing.
	[snip]

Yes. As I said before, I'm ready to insert your driver in the gspca
tree. The basic streaming is working. The video controls and the
other resolutions may be added later.

-- 
Ken ar c'hentan	|	      ** Breizh ha Linux atav! **
Jef		|		http://moinejf.free.fr/

Re: [PATCH] Make sure gspca cleans up USB resources during disconnect

[kilgota]

On Thu, 5 Feb 2009, Jean-Francois Moine wrote:

> On Thu, 5 Feb 2009 12:59:21 -0600 (CST)
> kilgota@banach.math.auburn.edu wrote:

<snip>

> [,,,] As I said before, I'm ready to insert your driver in the gspca
> tree. The basic streaming is working. The video controls and the
> other resolutions may be added later.

In fact, the ability to change the resolution is not exactly documented. 
No OEM driver that I know of ever did offer such a choice. They all just 
stream at 320x240, with no exceptions. I discovered this ability not from 
sniff logs, but by running experiments of my own. After that, my 
"documentation" consists pretty much of just mentioning it on the 
gphoto-devel mailing list if someone wrote in and asked if it is possible 
or not. I never felt safe using that information in the libgphoto2 driver, 
though. The problem was that some of the cams will only do max 352x288 and 
some will do max 640x480. But I could not figure out a way to tell the 
cameras apart. Still photos are listed in an allocation table which makes 
such things clear, for each individual photo. With streaming there is no 
equivalent to that. Even worse, in a way, is that the lower-res camera 
will go ahead and shoot a frame if you ask for a 640x480 but you will get 
a lower resolution instead. An obvious mess.

It was only while we are working here that I began to see a pattern 
relating the ID string to the ability to do 640x480. If you want to know, 
that ID looks like 09 05 00 xx and sometimes like 09 05 01 xx and 
sometimes like 09 05 02 xx. I am pretty sure now that, if the third byte 
in the string is not zero then the camera can do 640x380.

However, even now I can not be 100% certain. The SQ905 was, at one time, a 
very popular chip for cheap cameras. I can not even count the number of 
brand names and models in which it was used, or in which countries they 
were sold, given away as party favors, or breakfast cereal bonuses, or you 
name it. So perhaps it is indeed better to be safe about the resolution 
setting and hard-wire it to 320x240.

As to any other kinds of controls on streaming, other than on or off:

insofar as I am aware there are no such controls or setup commands at all 
with these cameras. None. All that I have ever seen one of these to do is 
to start streaming. At most, the chip or firmware ID gets read, nothing 
else.  Thus, there is neither opportunity nor mechanism for instructing 
the camera to do more than start or stop. It is of course logically 
possible that such command sequences may exist in some sort of metaphyical 
reality. But even if they do so exist, I have never those commands put to 
use and so can not know about them. So as far as that kind of thing is 
concerned, it appears to me that there is absolutely nothing left to do 
here.

Theodore Kilgore