Web lists-archives.com

Re: [Spca50x-devs] PATCH: gspcav2: pac207 support: gspca_main fixes




Jean-Francois Moine wrote:
> On Sat, 19 Apr 2008 21:47:03 +0200, Hans de Goede <j.w.r.degoede@xxxxxx> 
> wrote:
>> Hi all,
> 
> Hi Hans,
> 
> 	[snip]
>> gspcav2-0.0.28-avoid-soft-lockup.patch:
>> ---------------------------------------
>> Due to a bug in pac207.c (which I introduced while working on it) the cam 
>> crashed, causing the entire ISOC urb flow to stop, so no more iso_irq's. This 
>> caused gspca_frame_wait, to wait undefinitely on the waitqueue causing a 
>> softlock up of the kernel (yeah lots of reboots to debug that one).
>>
>> This patch fixes this by adding a timeout, and it also adds a check for device 
>> disconnects and some other thread / user stopping the stream (yes this can 
>> happen with our current design, which imho warrant a rethink, more on that later).
> 
> I don't see the softlock at this level: wait_event_interruptible()
> should return on the intr signal. I also had softlocks after crash
> in the kernel, but this is due to mutexes: these ones cannot be
> unlocked by an other process.
> 

Yes its a wait_interruptible, but that won't help much, atleast not in my case, 
as I was unable to even move the mouse without this patch. Also an application 
should not lockup (unless killed) when the isoc flow stops instead (eventually) 
it should get an IO error.

>> gspcav2-0.0.28-only-allow-one-user.patch:
>> -----------------------------------------
>> ekiga, for some unknown reason does the following:
>> 1) open the device
>> 2) request buffers
>> 3) open the device again
>> 4) close the fd returned in 3)
>> 5) queue buffers
>>
>> 5) fails because the close in 4) will free all buffers / frames causing any 
>> index passed in 5) to be invalid.
>>
>> Now one may claim this is an ekiga bug, but it is not, the current 
>> gspca_main.ko wrongly claims to support multiple opens and multiple readers 
>> even. This clearly shows it doesn't, with multiple (possibly independent) 
>> readers each reader will want to be in charge of the frame/buffer queue, this 
>> will only work if there is a queue _per_ user and then some ugliness in the 
>> isoirq handling to feed received data to all users with available buffers. But 
>> currently we only have one queue per device, not per user. Since the v4l2 spec 
>> does not ask for support for multiple readers, the easiest (and cleanest / 
>> sanest) fix is to not allow multiple opens at all. This is ok with ekiga as its 
>> fine with its step 3) failing.
> 	[snip]
> 
> I don't agree! Yes, freing all resources on the first close is a bug,
> but having many opens is useful: you may run a control process, as
> the mixer for sound and use an application without controls, such as
> Skype; also, some guys like to have a waiting thread which does the
> file poll and sends a message to an other thread which does reading
> (BTW, one of your pac207 patches resets the default control values
> at open time, then, you cannot change these values running any other
> program...).
> 

I agree, I suggest the following to fix this:
1) use a per open() call piece of data (containing a pointer to
    the gspca_dev struct and atleast an int with flags)
2) We need a claimed flag in gspca->dev
3) If any application tries to do any streaming related fileop,
    we first check if that application has the stream_owner flag set in
    its per application / per fd data. If it has it can continue.
    If it doesn't we check if the device is claimed, if it is, we
    return with an error (only one stream owner / buffer manager allowed)
    If the claimed flag is not set we set it and we set the stream_owner
    flag in the per fd data
4) On close we only do stream related cleanup if the fd has the
    stream_owner flag set, and in this case we also clear the claimed flag
5) We remove the open() and close() callbacks, all device init should be done
    at config.

Does this sound like a plan?

Also can you make available a tarbal with your latest tree, preferably with as 
much as my work merged? Then I'll start implementing the above fix and some 
further locking work on top of that. Also it seems that we both work in burstsm 
as in sometimes we have a couple of days in a row to work on gspca and then 
we're busy with other stuff again. Perhaps we can try to coordinate things so 
that I work for a few days then drop you a tarbal (or a huge patch), then you 
do some work for a few days, etc. That or maybe set up something like cvs 
somewhere.

Regards,

Hans


-------------------------------------------------------------------------
This SF.net email is sponsored by the 2008 JavaOne(SM) Conference 
Don't miss this year's exciting event. There's still time to save $100. 
Use priority code J8TL2D2. 
http://ad.doubleclick.net/clk;198757673;13503038;p?http://java.sun.com/javaone
_______________________________________________
Spca50x-devs mailing list
Spca50x-devs@xxxxxxxxxxxxxxxxxxxxx
https://lists.sourceforge.net/lists/listinfo/spca50x-devs