Messages in this thread Patch in this message | | | Date | Tue, 1 Apr 2008 23:45:22 +0200 | From | Rodolfo Giometti <> | Subject | Re: [PATCH 1/7] LinuxPPS core support. |
| |
On Tue, Apr 01, 2008 at 01:55:55AM -0700, Andrew Morton wrote: > > This can all be handled with suitable locking and refcounting. The device > which is delivering PPS interrupts has a reference on the PPS data > structures. If userspace has PPS open then it also has a reference. > > The thread of control which releases the last reference to the PPS data > structures also frees them all up. This may require a schedule_work() if > we need to support release-from-interrupt (as it appears that we do), but > that's OK - we just need to be able to make the PPS data structures > ineligible for new lookups while the schedule_work() is pending. > > There should be no need for any thread of control to wait for any other thread > of control to do anything. Get the refcounting right and everything > can be done synchronously.
Here my solution by using get/put functions:
diff --git a/drivers/pps/kapi.c b/drivers/pps/kapi.c index d75c8c8..61c1569 100644 --- a/drivers/pps/kapi.c +++ b/drivers/pps/kapi.c @@ -59,6 +59,62 @@ static void pps_add_offset(struct pps_ktime *ts, struct pps_ktime *offset) * Exported functions */ +/* pps_get_source - find a PPS source + * + * source: the PPS source ID. + * + * This function is used to find an already registered PPS source into the + * system. + * + * The function returns NULL if found nothing, otherwise it returns a pointer + * to the PPS source data struct (the refcounter is incremented by 1). + */ + +struct pps_device *pps_get_source(int source) +{ + struct pps_device *pps; + unsigned long flags; + + spin_lock_irqsave(&pps_idr_lock, flags); + + pps = idr_find(&pps_idr, source); + if (pps != NULL) + atomic_inc(&pps->usage); + + spin_unlock_irqrestore(&pps_idr_lock, flags); + + return pps; +} + +/* pps_put_source - free the PPS source data + * + * pps: a pointer to the PPS source. + * + * This function is used to free a PPS data struct if its refcount is 0. + */ + +void pps_put_source(struct pps_device *pps) +{ + unsigned long flags; + + spin_lock_irqsave(&pps_idr_lock, flags); + + BUG_ON(atomic_read(&pps->usage) == 0); + + if (!atomic_dec_and_test(&pps->usage)) + goto exit; + + /* No more reference to the PPS source. We can safely remove the + * PPS data struct. + */ + idr_remove(&pps_idr, pps->id); + + kfree(pps); + +exit: + spin_unlock_irqrestore(&pps_idr_lock, flags); +} + /* pps_register_source - add a PPS source in the system * * info: the PPS info struct @@ -133,8 +189,7 @@ 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); + atomic_set(&pps->usage, 1); /* Create the char device */ err = pps_register_cdev(pps); @@ -179,21 +234,14 @@ void pps_unregister_source(int source) pps = idr_find(&pps_idr, source); if (!pps) { + BUG(); spin_unlock_irq(&pps_idr_lock); return; } - - /* This should be done first in order to deny IRQ handlers - * to access PPS structs - */ - - idr_remove(&pps_idr, pps->id); spin_unlock_irq(&pps_idr_lock); - wait_event(pps->usage_queue, atomic_read(&pps->usage) == 0); - pps_unregister_cdev(pps); - kfree(pps); + pps_put_source(pps); } EXPORT_SYMBOL(pps_unregister_source); @@ -231,16 +279,7 @@ void pps_event(int source, int event, void *data) return; } - spin_lock_irqsave(&pps_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(&pps_idr_lock, flags); - + pps = pps_get_source(source); if (!pps) return; @@ -286,9 +325,6 @@ void pps_event(int source, int event, void *data) spin_unlock_irqrestore(&pps->lock, flags); /* Now we can release the PPS source for (possible) deregistration */ - spin_lock_irqsave(&pps_idr_lock, flags); - atomic_dec(&pps->usage); - wake_up_all(&pps->usage_queue); - spin_unlock_irqrestore(&pps_idr_lock, flags); + pps_put_source(pps); } EXPORT_SYMBOL(pps_event); diff --git a/drivers/pps/pps.c b/drivers/pps/pps.c index 5cbfeb9..a46f8f4 100644 --- a/drivers/pps/pps.c +++ b/drivers/pps/pps.c @@ -214,15 +214,7 @@ static int pps_cdev_open(struct inode *inode, struct file *file) struct pps_device, cdev); int found; - spin_lock_irq(&pps_idr_lock); - found = idr_find(&pps_idr, pps->id) != NULL; - - /* Lock the PPS source against (possible) deregistration */ - if (found) - atomic_inc(&pps->usage); - - spin_unlock_irq(&pps_idr_lock); - + found = pps_get_source(pps->id) != 0; if (!found) return -ENODEV; @@ -236,8 +228,7 @@ static int pps_cdev_release(struct inode *inode, struct file *file) 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); + pps_put_source(pps); return 0; } diff --git a/include/linux/pps.h b/include/linux/pps.h index aca0e77..e23aaa6 100644 --- a/include/linux/pps.h +++ b/include/linux/pps.h @@ -173,7 +173,6 @@ struct pps_device { spinlock_t lock; atomic_t usage; /* usage count */ - wait_queue_head_t usage_queue; }; /* @@ -189,6 +188,8 @@ extern struct device_attribute pps_attrs[]; * Exported functions */ +struct pps_device *pps_get_source(int source); +extern void pps_put_source(struct pps_device *pps); extern int pps_register_source(struct pps_source_info *info, int default_params); extern void pps_unregister_source(int source);
I'll send a new patchset ASAP!
Rodolfo
--
GNU/Linux Solutions e-mail: giometti@enneenne.com Linux Device Driver giometti@gnudd.com Embedded Systems giometti@linux.it UNIX programming phone: +39 349 2432127
| |