[LinuxPPS] safe pps_register_source()

[Rodolfo Giometti]

On Thu, Aug 09, 2007 at 09:40:41AM -0400, Fabio Checconi wrote:

> I think we need something to prevent the deregistration of the
> source while someone is waiting on it; wrt this patch I think that
> pps_event() is not the best place to signal that the source is no
> more used, since after pps_event() ends, in interrupt context, the
> unregistering task will free the chardev, while the task resuming
> from fetch will try to access the (already freed) source for its
> data.  Also using only the completion structure without an associated
> completion condition (i.e., the number of blocked tasks) I think
> can be racy.

What do you think about this solution?

diff --git a/drivers/pps/kapi.c b/drivers/pps/kapi.c
index ccfa123..c35e7d8 100644
--- a/drivers/pps/kapi.c
+++ b/drivers/pps/kapi.c
@@ -120,6 +120,8 @@ int pps_register_source(struct pps_source_info *info, int default_params)
 
 	init_waitqueue_head(&pps->queue);
 	spin_lock_init(&pps->lock);
+	atomic_set(&pps->usage, 0);
+	init_waitqueue_head(&pps->usage_queue);
 
 	/* Create the char device */
 	err = pps_register_cdev(pps);
@@ -178,6 +180,8 @@ void pps_unregister_source(int source)
 	idr_remove(&pps_idr, pps->id);
 	spin_unlock_irq(&idr_lock);
 
+	wait_event(pps->usage_queue, atomic_read(&pps->usage) == 0);
+
 	pps_sysfs_remove_source_entry(pps);
 	pps_unregister_cdev(pps);
 	kfree(pps);
@@ -206,6 +210,12 @@ void pps_event(int source, int event, void *data)
 
 	spin_lock_irqsave(&idr_lock, flags);
 	pps = idr_find(&pps_idr, source);
+
+	/* If we find a valid PPS source we lock it before leaving
+	 * the lock!
+	 */
+	if (!pps)
+		atomic_inc(&pps->usage);
 	spin_unlock_irqrestore(&idr_lock, flags);
 
 	if (!pps)
@@ -251,5 +261,9 @@ void pps_event(int source, int event, void *data)
 	kill_fasync(&pps->async_queue, SIGIO, POLL_IN);
 
 	spin_unlock_irqrestore(&pps->lock, flags);
+
+	/* Now we can release the PPS source for (possible) deregistration */
+	atomic_dec(&pps->usage);
+	wake_up_all(&pps->usage_queue);
 }
 EXPORT_SYMBOL(pps_event);
diff --git a/drivers/pps/pps.c b/drivers/pps/pps.c
index 41ae2fc..52de2f1 100644
--- a/drivers/pps/pps.c
+++ b/drivers/pps/pps.c
@@ -212,6 +212,9 @@ static int pps_cdev_open(struct inode *inode, struct file *file)
 	struct pps_device *pps = container_of(inode->i_cdev,
 						struct pps_device, cdev);
 
+	/* Lock the PPS source against (possible) deregistration */
+	atomic_inc(&pps->usage);
+
 	file->private_data = pps;
 
 	return 0;
@@ -219,7 +222,11 @@ static int pps_cdev_open(struct inode *inode, struct file *file)
 
 static int pps_cdev_release(struct inode *inode, struct file *file)
 {
-	/* Nop */
+	struct pps_device *pps = file->private_data;
+
+	/* Free the PPS source and wake up (possible) deregistration */
+	atomic_dec(&pps->usage);
+	wake_up_all(&pps->usage_queue);
 
 	return 0;
 }
diff --git a/include/linux/pps.h b/include/linux/pps.h
index a513da9..387b37f 100644
--- a/include/linux/pps.h
+++ b/include/linux/pps.h
@@ -171,6 +171,9 @@ struct pps_device {
 	struct fasync_struct *async_queue;	/* fasync method */
 	spinlock_t lock;
 
+	atomic_t usage;				/* usage count */
+	wait_queue_head_t usage_queue;
+
 	struct class_device class_dev;
 };

Rodolfo

-- 

GNU/Linux Solutions                  e-mail:    giometti at enneenne.com
Linux Device Driver                             giometti at gnudd.com
Embedded Systems                     		giometti at linux.it
UNIX programming                     phone:     +39 349 2432127