| Message ID | 200902032313.17538.linux@baker-net.org.uk (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
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 -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
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 -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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...
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 -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
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 -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 */
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> --- -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html