lkml.org 
[lkml]   [2011]   [Jul]   [13]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: RFC: Boiler plate functions for ida / idr allocation?
On Jul 13 Jonathan Cameron wrote:
> Taking ida's first, how about the following patch? I'm not at
> all attached to the form it takes, merely to cutting out on the
> cut and paste.

Not a big-picture opinion here whether this is a good thing; only some
small comments on side issues:

[...]
> The other thing this highlights is that I suspect quite a few are protected by
> spin locks when a mutex would be fine. Hence that might be worth tidying up first.

It seems to be the other way around in this case: Why use a mutex if a
spinlock is fine?

[...]
> --- a/drivers/misc/cb710/core.c
> +++ b/drivers/misc/cb710/core.c
> @@ -254,18 +254,9 @@ static int __devinit cb710_probe(struct pci_dev *pdev,
> if (err)
> return err;
>
> - do {
> - if (!ida_pre_get(&cb710_ida, GFP_KERNEL))
> - return -ENOMEM;
> -
> - spin_lock_irqsave(&cb710_ida_lock, flags);
> - err = ida_get_new(&cb710_ida, &chip->platform_id);
> - spin_unlock_irqrestore(&cb710_ida_lock, flags);
> -
> - if (err && err != -EAGAIN)
> - return err;
> - } while (err);
> -
> + err = ida_get_id(&cb710_ida_lock, &cb710_ida, &chip->platform_id);
> + if (err)
> + return err;
>
> dev_info(&pdev->dev, "id %d, IO 0x%p, IRQ %d\n",
> chip->platform_id, chip->iobase, pdev->irq);

To balance this change to cb710_probe, also switch from spin_lock_irqsave/
spin_unlock_irqrestore to spin_lock/spin_unlock in cb710_remove_one for
clarity.

[...]
> --- a/lib/idr.c
> +++ b/lib/idr.c
> @@ -939,3 +939,23 @@ void ida_init(struct ida *ida)
>
> }
> EXPORT_SYMBOL(ida_init);
> +
> +int ida_get_id(spinlock_t *lock, struct ida *ida, int *val)
> +{
> + int ret = 0;
> +ida_again:
> + if (unlikely(ida_pre_get(ida, GFP_KERNEL) == 0))
> + return -ENOMEM;
> +
> + spin_lock(lock);
> + ret = ida_get_new(ida, val);
> + spin_unlock(lock);
> +
> + if (unlikely (ret == -EAGAIN))
> + goto ida_again;
> + else if (likely(!ret))
> + *val = *val & MAX_ID_MASK;
> +
> + return ret;
> +}
> +EXPORT_SYMBOL(ida_get_id);

A new exported function (in lib/ even) should come with a kerneldoc comment
of course. Here it is among else noteworthy that the caller must provide
GFP_KERNEL allocations capable context and that @lock cannot be shared
with users in IRQ or softIRQ contexts.
--
Stefan Richter
-=====-==-== -=== -==-=
http://arcgraph.de/sr/


\
 
 \ /
  Last update: 2011-07-13 15:17    [W:0.108 / U:0.416 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site