Hi,
Here is a patch that fixes the disconnect handling and locking for the
vicam driver. It does the following.
1.) Change the parameters of send_control_msg to take a struct
vicam_camera instead of struct usb_device to allow for locking
of the device. Note that __send_control_msg does not lock the
camera. send_control_msg locks the camera before calling
__send_control_msg.
2.) Remove all instances of busy_lock. busy_lock was renamed to
cam_lock and used to lock out simultaneous uses of the camera
and handle disconnects. We may want to add back a different lock
to handle smp type stuff[1].
3.) Separate read_frame and vicam_decode_color. This should move us
along toward asynchronous urbs.
This patch does not address the locking of the camera that is still
needed by the proc interface.[2]
Patch is against 2.5.49 which I believe is the same as usb-current.
It works for me.
(I am still aware that there are some functions that fail that don't
return values and some that do but aren't checked... one thing at a
time. :) )
John
[1] I don't know that smp type locking is necessary since we only allow
a single user at a time. I think I could be wrong on this, though. Is it
possible for a process to open the camera and then fork giving us two
users of the camera?
[2] Right now we destroy the proc interface in disconnect under the
cam_lock. If we make the proc interface take the cam_lock for
synchronization, then disconnect holds the lock, proc is waiting on it,
and since the destroy_proc function was called, I can only assume that
it will wait for the proc_read/write to finish which will never happen.
I think that calling destroy_proc from disconnect before grabbing the
cam_lock will solve this problem. If everyone agrees on that, then I'll
submit another patch for it; I would like this one to go in first,
though.
Also, I'm getting an error that the proc interface is unsafe and the
module cannot be removed?
--- drivers/usb/media/vicam-2.5.49.c 2002-11-22 20:10:37.000000000 -0800
+++ drivers/usb/media/vicam.c 2002-11-23 08:12:35.000000000 -0800
@@ -1,7 +1,7 @@
/*
* USB ViCam WebCam driver
* Copyright (c) 2002 Joe Burks ([EMAIL PROTECTED]),
- * John Tyner (fill in email address)
+ * John Tyner ([EMAIL PROTECTED])
*
* Supports 3COM HomeConnect PC Digital WebCam
*
@@ -29,7 +29,7 @@
* Andy Armstrong who reverse engineered the color encoding and
* Pavel Machek and Chris Cheney who worked on reverse engineering the
* camera controls and wrote the first generation driver.
- * */
+ */
#include <linux/kernel.h>
#include <linux/wrapper.h>
@@ -408,7 +408,8 @@
struct video_device vdev; // v4l video device
struct usb_device *udev; // usb device
- struct semaphore busy_lock; // guard against SMP multithreading
+ /* guard against simultaneous accesses to the camera */
+ struct semaphore cam_lock;
int is_initialized;
u8 open_count;
@@ -424,17 +425,21 @@
static int vicam_probe( struct usb_interface *intf, const struct usb_device_id *id);
static void vicam_disconnect(struct usb_interface *intf);
static void read_frame(struct vicam_camera *cam, int framenum);
+static void vicam_decode_color( char *data, char *rgb);
-static int
-send_control_msg(struct usb_device *udev, u8 request, u16 value, u16 index,
- unsigned char *cp, u16 size)
+static int __send_control_msg(struct vicam_camera *cam,
+ u8 request,
+ u16 value,
+ u16 index,
+ unsigned char *cp,
+ u16 size)
{
int status;
/* cp must be memory that has been allocated by kmalloc */
- status = usb_control_msg(udev,
- usb_sndctrlpipe(udev, 0),
+ status = usb_control_msg(cam->udev,
+ usb_sndctrlpipe(cam->udev, 0),
request,
USB_DIR_OUT | USB_TYPE_VENDOR |
USB_RECIP_DEVICE, value, index,
@@ -450,6 +455,22 @@
return status;
}
+static int send_control_msg(struct vicam_camera *cam,
+ u8 request,
+ u16 value,
+ u16 index,
+ unsigned char *cp,
+ u16 size)
+{
+ int status = -ENODEV;
+ down(&cam->cam_lock);
+ if (cam->udev) {
+ status = __send_control_msg(cam, request, value,
+ index, cp, size);
+ }
+ up(&cam->cam_lock);
+ return status;
+}
static int
initialize_camera(struct vicam_camera *cam)
{
@@ -465,14 +486,13 @@
{ .data = setup3, .size = sizeof(setup3) },
{ .data = NULL, .size = 0 }
};
-
- struct usb_device *udev = cam->udev;
+
int err, i;
for (i = 0, err = 0; firmware[i].data && !err; i++) {
memcpy(cam->cntrlbuf, firmware[i].data, firmware[i].size);
- err = send_control_msg(udev, 0xff, 0, 0,
+ err = send_control_msg(cam, 0xff, 0, 0,
cam->cntrlbuf, firmware[i].size);
}
@@ -484,11 +504,11 @@
{
int status;
- if ((status = send_control_msg(cam->udev, 0x50, state, 0, NULL, 0)) < 0)
+ if ((status = send_control_msg(cam, 0x50, state, 0, NULL, 0)) < 0)
return status;
if (state) {
- send_control_msg(cam->udev, 0x55, 1, 0, NULL, 0);
+ send_control_msg(cam, 0x55, 1, 0, NULL, 0);
}
return 0;
@@ -504,10 +524,6 @@
if (!cam)
return -ENODEV;
- /* make this _really_ smp-safe */
- if (down_interruptible(&cam->busy_lock))
- return -EINTR;
-
switch (ioctlnr) {
/* query capabilites */
case VIDIOCGCAP:
@@ -694,6 +710,9 @@
DBG("VIDIOCSYNC: %d\n", frame);
read_frame(cam, frame);
+ vicam_decode_color(cam->raw_image,
+ cam->framebuf +
+ frame * VICAM_MAX_FRAME_SIZE );
break;
}
@@ -724,7 +743,6 @@
break;
}
- up(&cam->busy_lock);
return retval;
}
@@ -741,26 +759,25 @@
"vicam video_device improperly initialized");
}
- if ( down_interruptible(&cam->busy_lock) )
- return -EINTR;
+ /* the videodev_lock held above us protects us from
+ * simultaneous opens...for now. we probably shouldn't
+ * rely on this fact forever.
+ */
if (cam->open_count > 0) {
printk(KERN_INFO
"vicam_open called on already opened camera");
- up(&cam->busy_lock);
return -EBUSY;
}
cam->raw_image = kmalloc(VICAM_MAX_READ_SIZE, GFP_KERNEL);
if (!cam->raw_image) {
- up(&cam->busy_lock);
return -ENOMEM;
}
cam->framebuf = rvmalloc(VICAM_MAX_FRAME_SIZE * VICAM_FRAMES);
if (!cam->framebuf) {
kfree(cam->raw_image);
- up(&cam->busy_lock);
return -ENOMEM;
}
@@ -768,7 +785,6 @@
if (!cam->cntrlbuf) {
kfree(cam->raw_image);
rvfree(cam->framebuf, VICAM_MAX_FRAME_SIZE * VICAM_FRAMES);
- up(&cam->busy_lock);
return -ENOMEM;
}
@@ -785,8 +801,6 @@
cam->needsDummyRead = 1;
cam->open_count++;
- up(&cam->busy_lock);
-
file->private_data = cam;
return 0;
@@ -796,14 +810,32 @@
vicam_close(struct inode *inode, struct file *file)
{
struct vicam_camera *cam = file->private_data;
+ int open_count;
+ struct usb_device *udev;
+
DBG("close\n");
+
+ /* it's not the end of the world if
+ * we fail to turn the camera off.
+ */
+
set_camera_power(cam, 0);
kfree(cam->raw_image);
rvfree(cam->framebuf, VICAM_MAX_FRAME_SIZE * VICAM_FRAMES);
kfree(cam->cntrlbuf);
+ down(&cam->cam_lock);
+
cam->open_count--;
+ open_count = cam->open_count;
+ udev = cam->udev;
+
+ up(&cam->cam_lock);
+
+ if (!open_count && !udev) {
+ kfree(cam);
+ }
return 0;
}
@@ -953,12 +985,18 @@
request[8] = 0;
// bytes 9-15 do not seem to affect exposure or image quality
- n = send_control_msg(cam->udev, 0x51, 0x80, 0, request, 16);
+ down(&cam->cam_lock);
+
+ if (!cam->udev) {
+ goto done;
+ }
+
+ n = __send_control_msg(cam, 0x51, 0x80, 0, request, 16);
if (n < 0) {
printk(KERN_ERR
" Problem sending frame capture control message");
- return;
+ goto done;
}
n = usb_bulk_msg(cam->udev,
@@ -971,9 +1009,8 @@
n);
}
- vicam_decode_color(cam->raw_image,
- cam->framebuf +
- framenum * VICAM_MAX_FRAME_SIZE );
+ done:
+ up(&cam->cam_lock);
}
static int
@@ -983,17 +1020,17 @@
DBG("read %d bytes.\n", (int) count);
- if ( down_interruptible(&cam->busy_lock) )
- return -EINTR;
-
if (*ppos >= VICAM_MAX_FRAME_SIZE) {
*ppos = 0;
- up(&cam->busy_lock);
return 0;
}
if (*ppos == 0) {
read_frame(cam, 0);
+ vicam_decode_color(cam->raw_image,
+ cam->framebuf +
+ 0 * VICAM_MAX_FRAME_SIZE);
+
}
count = min_t(size_t, count, VICAM_MAX_FRAME_SIZE - *ppos);
@@ -1008,8 +1045,6 @@
*ppos = 0;
}
- up(&cam->busy_lock);
-
return count;
}
@@ -1034,10 +1069,6 @@
return -EINVAL;
*/
- /* make this _really_ smp-safe */
- if (down_interruptible(&cam->busy_lock))
- return -EINTR;
-
pos = (unsigned long)cam->framebuf;
while (size > 0) {
page = kvirt_to_pa(pos);
@@ -1052,8 +1083,6 @@
size = 0;
}
- up(&cam->busy_lock);
-
return 0;
}
@@ -1285,7 +1314,7 @@
cam->shutter_speed = 15;
- init_MUTEX(&cam->busy_lock);
+ init_MUTEX(&cam->cam_lock);
memcpy(&cam->vdev, &vicam_template,
sizeof (vicam_template));
@@ -1312,17 +1341,43 @@
static void
vicam_disconnect(struct usb_interface *intf)
{
+ int open_count;
struct vicam_camera *cam = dev_get_drvdata(&intf->dev);
-
dev_set_drvdata ( &intf->dev, NULL );
-
- cam->udev = NULL;
-
+
+ /* we must unregister the device before taking its
+ * cam_lock. This is because the video open call
+ * holds the same lock as video unregister. if we
+ * unregister inside of the cam_lock and open also
+ * uses the cam_lock, we get deadlock.
+ */
+
video_unregister_device(&cam->vdev);
+ /* stop the camera from being used */
+
+ down(&cam->cam_lock);
+
+ /* mark the camera as gone */
+
+ cam->udev = NULL;
+
vicam_destroy_proc_entry(cam);
- kfree(cam);
+ /* the only thing left to do is synchronize with
+ * our close/release function on who should release
+ * the camera memory. if there are any users using the
+ * camera, it's their job. if there are no users,
+ * it's ours.
+ */
+
+ open_count = cam->open_count;
+
+ up(&cam->cam_lock);
+
+ if (!open_count) {
+ kfree(cam);
+ }
printk(KERN_DEBUG "ViCam-based WebCam disconnected\n");
}
