[PATCH] broken locking in CPiA driver

[David C. Hansen]

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
[e-mail]
IBM LTC Base/OS Group
[phone]

--- ../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);
 }
 
--- ../clean/linux/drivers/media/video/cpia.h	Fri Mar  2 11:12:10 2001
+++ linux/drivers/media/video/cpia.h	Mon Sep 10 15:24:03 2001
@@ -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__ */