Attached is a driver for the 3Com HomeConnect USB Camera. The code is
based somewhat on the vicam driver in the kernel (which crashes my
computer hard) and also relies heavily on the information obtained by
the guys doing the reverse engineering and kernel driver work at
homeconnectusb.sourceforge.net. The fact that this driver works speaks
very highly of them.
The driver gets the "It Works for Me" approval, but can definitely use
some wider testing and review. Please test, review, and comment.
The code is for for 2.4.19, and I'll port it forward to 2.5 if it
seems like it would be useful.
Apologies:
1. The files are not a patch but should build outside of the tree.
2. The file is woefully underdocumented
3. There aren't any printk's in the driver.
I'm not subscribed to the list but rather lurk on the archives, so please
try to include me in any replies.
Thanks,
John
On Thu, Oct 03, 2002 at 08:59:30PM -0700, John Tyner wrote:
>
> The code is for for 2.4.19, and I'll port it forward to 2.5 if it
> seems like it would be useful.
>
> Apologies:
> 1. The files are not a patch but should build outside of the tree.
If you send me a patch for 2.5, I'd be glad to add it to the tree.
Right now, I'm not accepting USB drivers that don't show up in 2.5
first.
Other than that, the code looks nice. Did you look at how the usb video
drivers do their memory management in 2.4.20-pre like I mentioned
before?
thanks,
greg k-h
> If you send me a patch for 2.5, I'd be glad to add it to the tree.
> Right now, I'm not accepting USB drivers that don't show up in 2.5
> first.
I'll get to work.
> Other than that, the code looks nice. Did you look at how the usb video
> drivers do their memory management in 2.4.20-pre like I mentioned
> before?
I did. The ov511 as well as the bttv and cpia drivers still use the
rvmalloc/rvfree methods. They are minor'ly updated from the version I was
using, and I've already incorporated those changes to the driver submitted
in my previous post.
Thanks,
John
On Thu, Oct 03, 2002 at 10:05:05PM -0700, John Tyner wrote:
> > If you send me a patch for 2.5, I'd be glad to add it to the tree.
> > Right now, I'm not accepting USB drivers that don't show up in 2.5
> > first.
>
> I'll get to work.
Great!
> > Other than that, the code looks nice. Did you look at how the usb video
> > drivers do their memory management in 2.4.20-pre like I mentioned
> > before?
>
> I did. The ov511 as well as the bttv and cpia drivers still use the
> rvmalloc/rvfree methods. They are minor'ly updated from the version I was
> using, and I've already incorporated those changes to the driver submitted
> in my previous post.
Ah, good, I just wanted to make sure, as I know there were some changes
there.
thanks,
greg k-h
This time with the patch actually attached!
> > If you send me a patch for 2.5, I'd be glad to add it to the tree.
>
> As promised, here is a patch against 2.5.40.
>
> The 2.5 V4L interface looks like it will provide a way for multiple users
to
> use the same camera simultaneously, but that's a fairly drastic change,
and
> I'd like to start with something that is pretty well tested [at least by
> me]. So, this patch is a sraight forward-port of the 2.4 version posted a
> few days ago.
>
> > Right now, I'm not accepting USB drivers that don't show up in 2.5
> > first.
>
> If desired, I'll create a patch of the 2.4 version as well.
>
> Thanks,
> John
>
> If you send me a patch for 2.5, I'd be glad to add it to the tree.
As promised, here is a patch against 2.5.40.
The 2.5 V4L interface looks like it will provide a way for multiple users to
use the same camera simultaneously, but that's a fairly drastic change, and
I'd like to start with something that is pretty well tested [at least by
me]. So, this patch is a sraight forward-port of the 2.4 version posted a
few days ago.
> Right now, I'm not accepting USB drivers that don't show up in 2.5
> first.
If desired, I'll create a patch of the 2.4 version as well.
Thanks,
John
On Sunday 06 October 2002 04:58, John Tyner wrote:
> This time with the patch actually attached!
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?
> In vicam_v4l_open:
>
> Why is only the first control message checked for errors?
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.
> vicam_usb_probe:
>
> __devinit ???
>
> vicam_usb_disconnect:
>
> __devexit ???
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.
> And you should probably kill the tasklet before you unregister the video
> device.
Makes sense.
> PS: Is that just me, or did diff produce particularly unreadable output
> this time?
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
> And you should probably kill the tasklet before you unregister the video
> device.
The more I think about this, the more I think that killing the tasklet after
unregistering the device is the correct way.
>From what I can tell, there are two ways that the disconnect function can be
called: a physical disconnect or a module removal.
In the case of a physical disconnect, the ordering probably doesn't matter
because the tasklet won't be scheduled again because urb's would fail to
complete successfully.
The case of module removal becomes a bit more complicated (for reasons
concerning module unload races that are being discussed by people far
smarter than I). But in any event, I think that it makes more sense to
unregister the open/close/etc. interface so that there is less chance of
trying to send another urb (thus causing another schedule of the tasklet)
before actually killing the tasklet.
This also brings up the (somewhat) rhetorical question I posed in the
driver's disconnect function. What happens when a disconnect occurs while
the device is open?
Thanks,
John
On Sunday 06 October 2002 23:32, John Tyner wrote:
> > And you should probably kill the tasklet before you unregister the video
> > device.
>
> The more I think about this, the more I think that killing the tasklet
> after unregistering the device is the correct way.
>
> From what I can tell, there are two ways that the disconnect function can
> be called: a physical disconnect or a module removal.
And as a result of an ioctl() through usbfs.
> In the case of a physical disconnect, the ordering probably doesn't matter
> because the tasklet won't be scheduled again because urb's would fail to
> complete successfully.
>
> The case of module removal becomes a bit more complicated (for reasons
> concerning module unload races that are being discussed by people far
> smarter than I). But in any event, I think that it makes more sense to
> unregister the open/close/etc. interface so that there is less chance of
> trying to send another urb (thus causing another schedule of the tasklet)
> before actually killing the tasklet.
>
> This also brings up the (somewhat) rhetorical question I posed in the
> driver's disconnect function. What happens when a disconnect occurs while
> the device is open?
I can't tell right now. I'll sync up after returning home and look into the
matter.
Regards
Oliver
On Sunday 06 October 2002 19:35, John Tyner wrote:
> > In vicam_v4l_open:
> >
> > Why is only the first control message checked for errors?
>
> 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.
>
Exactly.
> > vicam_usb_probe:
> >
> > __devinit ???
> >
> > vicam_usb_disconnect:
> >
> > __devexit ???
>
> 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.
But usbcore will happily call probe if HOTPLUG is not defined and AFAICT
usb will compile without HOTPLUG.
Regards
Oliver
> > 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.
>
> Exactly.
Fixed.
> But usbcore will happily call probe if HOTPLUG is not defined and AFAICT
> usb will compile without HOTPLUG.
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
> 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.
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
> 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.
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
Hi!
> Attached is a driver for the 3Com HomeConnect USB Camera. The code is
> based somewhat on the vicam driver in the kernel (which crashes my
> computer hard) and also relies heavily on the information obtained by
> the guys doing the reverse engineering and kernel driver work at
> homeconnectusb.sourceforge.net. The fact that this driver works speaks
> very highly of them.
>
> The driver gets the "It Works for Me" approval, but can definitely use
> some wider testing and review. Please test, review, and comment.
>
> The code is for for 2.4.19, and I'll port it forward to 2.5 if it
> seems like it would be useful.
How is this different from 3comhc.c from sourceforge?
Anyway, this is *good stuff* (tm), as old vicam.c is not too
functional (to say at least). Basically anything is better than old
vicam.c
Pavel
--
I'm [email protected]. "In my country we have almost anarchy and I don't care."
Panos Katsaloulis describing me w.r.t. patents at [email protected]
> How is this different from 3comhc.c from sourceforge?
Lots of little fixes, speed and memory allocation improvements and
generally more understandable (in my opinion). The one I've written
actually implements an asynchronous send/receive and decode so
the camera works a bit faster.
I had actually been developing this driver in parallel with the
sourceforge guys before I found out about them. I combined their work with
mine to finish the driver.
See these two threads. The first provides a little more background and the
second is the thread that you replied to.
http://marc.theaimsgroup.com/?t=103344771400001&r=1&w=2
http://marc.theaimsgroup.com/?t=103370436400001&r=1&w=2
Thanks,
John
On Mon, 7 Oct 2002, Oliver Neukum wrote:
> Well, here we go again, there are other issues as well.
> There's a race between open() and disconnect(). The best way to deal
[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