Re: [PATCH 1/7] LinuxPPS core support.

[Andrew Morton <akpm@linux-foundation.org>]

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);
}