| Message ID | 20171019204506.25090-1-Jason@zx2c4.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Delegated to: | Herbert Xu |
| Headers | show |
These are mostly untested, but I wanted to spec it out for a taste of what it looks like.
On Thu, Oct 19, 2017 at 1:45 PM, Jason A. Donenfeld <Jason@zx2c4.com> wrote: > As this interface becomes more heavily used, it will be painful for > callers to always need to check for -EALREADY. > > Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com> > --- > drivers/char/random.c | 24 ++++++++++++++++-------- > 1 file changed, 16 insertions(+), 8 deletions(-) > > diff --git a/drivers/char/random.c b/drivers/char/random.c > index 8ad92707e45f..e161acf35d4a 100644 > --- a/drivers/char/random.c > +++ b/drivers/char/random.c > @@ -1556,40 +1556,45 @@ EXPORT_SYMBOL(wait_for_random_bytes); > > /* > * Add a callback function that will be invoked when the nonblocking > - * pool is initialised. > + * pool is initialised, or calls that function if it already is. > * > * returns: 0 if callback is successfully added > - * -EALREADY if pool is already initialised (callback not called) > * -ENOENT if module for callback is not alive > */ > int add_random_ready_callback(struct random_ready_callback *rdy) > { > struct module *owner; > unsigned long flags; > - int err = -EALREADY; > > - if (crng_ready()) > - return err; > + BUG_ON(!rdy->func); Better to WARN_ON() and return a failure. > + > + if (crng_ready()) { > + rdy->func(rdy); > + rdy->func = NULL; > + return 0; > + } > > owner = rdy->owner; > if (!try_module_get(owner)) > return -ENOENT; > > spin_lock_irqsave(&random_ready_list_lock, flags); > - if (crng_ready()) > + if (crng_ready()) { > + rdy->func(rdy); > + rdy->func = NULL; > goto out; > + } > > owner = NULL; > > list_add(&rdy->list, &random_ready_list); > - err = 0; > > out: > spin_unlock_irqrestore(&random_ready_list_lock, flags); > > module_put(owner); > > - return err; > + return 0; > } > EXPORT_SYMBOL(add_random_ready_callback); > > @@ -1601,6 +1606,9 @@ void del_random_ready_callback(struct random_ready_callback *rdy) > unsigned long flags; > struct module *owner = NULL; > > + if (!rdy->func) > + return; Perhaps add a note here about what a NULL func means here? (e.g. "Already run, not in the list.") > + > spin_lock_irqsave(&random_ready_list_lock, flags); > if (!list_empty(&rdy->list)) { > list_del_init(&rdy->list); > -- > 2.14.2 > Otherwise, yeah, looks sensible. -Kees
Good tips, thanks. I'll wait for more comments before resubmitting v2, but in-progress changes live here: https://git.zx2c4.com/linux-dev/log/?h=jd/cleaner-add-random-ready
diff --git a/drivers/char/random.c b/drivers/char/random.c index 8ad92707e45f..e161acf35d4a 100644 --- a/drivers/char/random.c +++ b/drivers/char/random.c @@ -1556,40 +1556,45 @@ EXPORT_SYMBOL(wait_for_random_bytes); /* * Add a callback function that will be invoked when the nonblocking - * pool is initialised. + * pool is initialised, or calls that function if it already is. * * returns: 0 if callback is successfully added - * -EALREADY if pool is already initialised (callback not called) * -ENOENT if module for callback is not alive */ int add_random_ready_callback(struct random_ready_callback *rdy) { struct module *owner; unsigned long flags; - int err = -EALREADY; - if (crng_ready()) - return err; + BUG_ON(!rdy->func); + + if (crng_ready()) { + rdy->func(rdy); + rdy->func = NULL; + return 0; + } owner = rdy->owner; if (!try_module_get(owner)) return -ENOENT; spin_lock_irqsave(&random_ready_list_lock, flags); - if (crng_ready()) + if (crng_ready()) { + rdy->func(rdy); + rdy->func = NULL; goto out; + } owner = NULL; list_add(&rdy->list, &random_ready_list); - err = 0; out: spin_unlock_irqrestore(&random_ready_list_lock, flags); module_put(owner); - return err; + return 0; } EXPORT_SYMBOL(add_random_ready_callback); @@ -1601,6 +1606,9 @@ void del_random_ready_callback(struct random_ready_callback *rdy) unsigned long flags; struct module *owner = NULL; + if (!rdy->func) + return; + spin_lock_irqsave(&random_ready_list_lock, flags); if (!list_empty(&rdy->list)) { list_del_init(&rdy->list);
As this interface becomes more heavily used, it will be painful for callers to always need to check for -EALREADY. Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com> --- drivers/char/random.c | 24 ++++++++++++++++-------- 1 file changed, 16 insertions(+), 8 deletions(-)