Messages in this thread | | | Date | Tue, 1 Apr 2008 14:57:14 -0700 | From | Andrew Morton <> | Subject | Re: [PATCH 1/7] LinuxPPS core support. |
| |
On Tue, 1 Apr 2008 23:45:22 +0200 Rodolfo Giometti <giometti@enneenne.com> wrote:
> 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:
That's looking promising.
> 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);
Please don't forget to use checkpatch.
> + 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); > +}
It'd be slightly moer efficient to move the kfree outside the spinlock:
if (!atomic_dec_and_test(&pps->usage)) { pps = NULL; goto exit; }
/* No more reference to the PPS source. We can safely remove the * PPS data struct. */ idr_remove(&pps_idr, pps->id); exit: spin_unlock_irqrestore(&pps_idr_lock, flags); kfree(pps); }
| |