broken locking in CPiA driver

This patch fixes a locking issue with the CPiA driver, and cleans up the locking. There is a potential race condition in cpia_pp_detach(). The REMOVE_FROM_LIST macro uses the BKL to provide protection for the cam_list. However, the BKL is not held during the whole for loop, only during the macro.

This patch does several things:
1. rename REMOVE_FROM_LIST and ADD_TO_LIST to cpia_remove_from_list and cpia_add_to_list, respectively
2. make them "static inline" functions instead of #define macros. (take a look at this, they probably should be functions)
3. add one static spinlock cam_list_lock_{pp|usb} for each of the pp and usb drivers to replace BKL
4. do locking around all cam_list references to replace BKL locking from the macros
5. fix race condition
6. initialize cam_list to NULL in pp driver, just like the usb driver.

--
David C. Hansen
[email protected]
IBM LTC Base/OS Group
(503)578-4080

--- ../clean/linux/drivers/media/video/cpia_pp.c Fri Mar 2 11:12:10
2001
+++ linux/drivers/media/video/cpia_pp.c Mon Sep 10 16:02:42 2001
@@ -158,6 +158,7 @@
};

static struct cam_data *cam_list;
+static spinlock_t cam_list_lock_pp;

#ifdef _CPIA_DEBUG_
#define DEB_PORT(port) { \
@@ -582,7 +583,9 @@
kfree(cam);
return -ENXIO;
}
- ADD_TO_LIST(cam_list, cpia);
+ spin_lock( &cam_list_lock_pp );
+ cpia_add_to_list(cam_list, cpia);
+ spin_unlock( &cam_list_lock_pp );

return 0;
}
@@ -591,11 +594,12 @@
{
struct cam_data *cpia;

+ spin_lock( &cam_list_lock_pp );
for(cpia = cam_list; cpia != NULL; cpia = cpia->next) {
struct pp_cam_entry *cam = cpia->lowlevel_data; if (cam && cam->port->number == port->number) {
- REMOVE_FROM_LIST(cpia);
-
+ cpia_remove_from_list(cpia);
+ spin_unlock( &cam_list_lock_pp );
cpia_unregister_camera(cpia);

if(cam->open_count > 0) {
@@ -606,9 +610,10 @@

kfree(cam); cpia->lowlevel_data = NULL;
- break;
+ return;
}
}
+ spin_unlock( &cam_list_lock_pp );
}

static void cpia_pp_attach (struct parport *port)
@@ -660,6 +665,9 @@
LOG ("unable to register with parport\n"); return -EIO;
}
+
+ cam_list = NULL;
+ spinlock_init( cam_list_lock_pp );

return 0;
}
--- ../clean/linux/drivers/media/video/cpia_usb.c Mon Feb 19 10:18:18
2001
+++ linux/drivers/media/video/cpia_usb.c Mon Sep 10 15:19:19 2001
@@ -104,6 +104,7 @@
};

static struct cam_data *cam_list;
+static spinlock_t cam_list_lock_usb;

static void cpia_usb_complete(struct urb *urb)
{
@@ -536,7 +537,9 @@
goto fail_all;
}

- ADD_TO_LIST(cam_list, cam);
+ spin_lock( &cam_list_lock_usb );
+ cpia_add_to_list(cam_list, cam);
+ spin_unlock( &cam_list_lock_usb );

return cam;

@@ -579,8 +582,10 @@
struct cam_data *cam = (struct cam_data *) ptr; struct usb_cpia *ucpia = (struct usb_cpia *) cam->lowlevel_data;

- REMOVE_FROM_LIST(cam);
-
+ spin_lock( &cam_list_lock_usb );
+ cpia_remove_from_list(cam);
+ spin_unlock( &cam_list_lock_usb );
+
/* Don't even try to reset the altsetting if we're disconnected */ cpia_usb_free_resources(ucpia, 0);

@@ -620,7 +625,7 @@
static int __init usb_cpia_init(void)
{
cam_list = NULL;
-
+ spin_lock_init(&cam_list_lock_usb);
return usb_register(&cpia_driver);
}

@@ -393,27 +393,24 @@
       (p)&0x80?1:0, (p)&0x40?1:0, (p)&0x20?1:0, (p)&0x10?1:0,\
         (p)&0x08?1:0, (p)&0x04?1:0, (p)&0x02?1:0, (p)&0x01?1:0);
 
-#define ADD_TO_LIST(l, drv) \
-  {\
-    lock_kernel();\
-    (drv)->next = l;\
-    (drv)->previous = &(l);\
-    (l) = drv;\
-    unlock_kernel();\
-  } while(0)
+static inline void cpia_add_to_list(struct cam_data* l, struct

cam_data* drv)
+{
+ drv->next = l;
+ drv->previous = &l;
+ l = drv;
+}

-#define REMOVE_FROM_LIST(drv) \
- {\
- if ((drv)->previous != NULL) {\
- lock_kernel();\
- if ((drv)->next != NULL)\
- (drv)->next->previous = (drv)->previous;\

-      *((drv)->previous) = (drv)->next;\
-      (drv)->previous = NULL;\
-      (drv)->next = NULL;\
-      unlock_kernel();\

- }\
- } while (0)
+
+static inline void cpia_remove_from_list(struct cam_data* drv) +{
+ if (drv->previous != NULL) {
+ if (drv->next != NULL)
+ drv->next->previous = drv->previous;
+ *(drv->previous) = drv->next;
+ drv->previous = NULL;
+ drv->next = NULL;
+ }
+}

#endif /* __KERNEL__ */

Technically this isn't a problem with USB since all connects and disconnects are serialized, but it's probably worth the effort to maintain consistency.

One lock for both PP and USB is probably sufficient.

They should also probably use the generic list.h routines.

JE