| Message ID | 20171027170404.21262-1-prasannatsmkumar@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Herbert Xu |
| Headers | show |
On 10/27/2017 07:04 PM, PrasannaKumar Muralidharan wrote: > User is able to select a chosen rng by writing its name to rng_current > but there is no way to reset it without unbinding the rng. Let user > write "" to rng_current and delesect the chosen rng. > > Signed-off-by: PrasannaKumar Muralidharan <prasannatsmkumar@gmail.com> > --- > drivers/char/hw_random/core.c | 51 +++++++++++++++++++++++++++---------------- > 1 file changed, 32 insertions(+), 19 deletions(-) > > diff --git a/drivers/char/hw_random/core.c b/drivers/char/hw_random/core.c > index 9701ac7..be03024 100644 > --- a/drivers/char/hw_random/core.c > +++ b/drivers/char/hw_random/core.c > @@ -292,26 +292,48 @@ static struct miscdevice rng_miscdev = { > .groups = rng_dev_groups, > }; > > +static int enable_best_rng(void) > +{ > + int ret = -ENODEV; > + > + BUG_ON(!mutex_is_locked(&rng_mutex)); > + > + /* rng_list is sorted by quality, use the best (=first) one */ > + if (!list_empty(&rng_list)) { > + struct hwrng *new_rng; > + > + new_rng = list_entry(rng_list.next, struct hwrng, list); > + ret = ((new_rng == current_rng) ? 0 : set_current_rng(new_rng)); > + if (!ret) > + cur_rng_set_by_user = 0; > + } > + > + return ret; > +} > + > static ssize_t hwrng_attr_current_store(struct device *dev, > struct device_attribute *attr, > const char *buf, size_t len) > { > - int err; > + int err = -ENODEV; > struct hwrng *rng; > > err = mutex_lock_interruptible(&rng_mutex); > if (err) > return -ERESTARTSYS; > - err = -ENODEV; > - list_for_each_entry(rng, &rng_list, list) { > - if (sysfs_streq(rng->name, buf)) { > - err = 0; > - cur_rng_set_by_user = 1; > - if (rng != current_rng) > + > + if (sysfs_streq(buf, "")) { > + err = enable_best_rng(); > + } else { > + list_for_each_entry(rng, &rng_list, list) { > + if (sysfs_streq(rng->name, buf)) { > + cur_rng_set_by_user = 1; > err = set_current_rng(rng); > - break; > + break; > + } > } > } > + > mutex_unlock(&rng_mutex); > > return err ? : len; > @@ -493,17 +515,8 @@ void hwrng_unregister(struct hwrng *rng) > mutex_lock(&rng_mutex); > > list_del(&rng->list); > - if (current_rng == rng) { > - drop_current_rng(); > - cur_rng_set_by_user = 0; > - /* rng_list is sorted by quality, use the best (=first) one */ > - if (!list_empty(&rng_list)) { > - struct hwrng *new_rng; > - > - new_rng = list_entry(rng_list.next, struct hwrng, list); > - set_current_rng(new_rng); > - } > - } > + if (current_rng == rng) > + enable_best_rng(); > > if (list_empty(&rng_list)) { > mutex_unlock(&rng_mutex); That's a really good idea. I also thought about something like that. regards Harald Freudenberger
Hi Harald,
On 30 October 2017 at 13:40, Harald Freudenberger
<freude@linux.vnet.ibm.com> wrote:
> That's a really good idea. I also thought about something like that.
Do you mind reviewing the code?
Thanks,
PrasannaKumar
On 10/27/2017 07:04 PM, PrasannaKumar Muralidharan wrote: > User is able to select a chosen rng by writing its name to rng_current > but there is no way to reset it without unbinding the rng. Let user > write "" to rng_current and delesect the chosen rng. > > Signed-off-by: PrasannaKumar Muralidharan <prasannatsmkumar@gmail.com> > --- > drivers/char/hw_random/core.c | 51 +++++++++++++++++++++++++++---------------- > 1 file changed, 32 insertions(+), 19 deletions(-) > > diff --git a/drivers/char/hw_random/core.c b/drivers/char/hw_random/core.c > index 9701ac7..be03024 100644 > --- a/drivers/char/hw_random/core.c > +++ b/drivers/char/hw_random/core.c > @@ -292,26 +292,48 @@ static struct miscdevice rng_miscdev = { > .groups = rng_dev_groups, > }; > > +static int enable_best_rng(void) > +{ > + int ret = -ENODEV; > + > + BUG_ON(!mutex_is_locked(&rng_mutex)); > + > + /* rng_list is sorted by quality, use the best (=first) one */ > + if (!list_empty(&rng_list)) { > + struct hwrng *new_rng; > + > + new_rng = list_entry(rng_list.next, struct hwrng, list); > + ret = ((new_rng == current_rng) ? 0 : set_current_rng(new_rng)); > + if (!ret) > + cur_rng_set_by_user = 0; > + } > + > + return ret; > +} > + > static ssize_t hwrng_attr_current_store(struct device *dev, > struct device_attribute *attr, > const char *buf, size_t len) > { > - int err; > + int err = -ENODEV; Two lines later err is assigned the return value of mutex_lock_interruptible(). So the -ENODEV value assignment does nothing. > struct hwrng *rng; > > err = mutex_lock_interruptible(&rng_mutex); > if (err) > return -ERESTARTSYS; > - err = -ENODEV; > - list_for_each_entry(rng, &rng_list, list) { > - if (sysfs_streq(rng->name, buf)) { > - err = 0; > - cur_rng_set_by_user = 1; > - if (rng != current_rng) > + > + if (sysfs_streq(buf, "")) { > + err = enable_best_rng(); > + } else { > + list_for_each_entry(rng, &rng_list, list) { > + if (sysfs_streq(rng->name, buf)) { > + cur_rng_set_by_user = 1; > err = set_current_rng(rng); > - break; > + break; > + } > } > } > + > mutex_unlock(&rng_mutex); > > return err ? : len; > @@ -493,17 +515,8 @@ void hwrng_unregister(struct hwrng *rng) > mutex_lock(&rng_mutex); > > list_del(&rng->list); > - if (current_rng == rng) { > - drop_current_rng(); > - cur_rng_set_by_user = 0; > - /* rng_list is sorted by quality, use the best (=first) one */ > - if (!list_empty(&rng_list)) { > - struct hwrng *new_rng; > - > - new_rng = list_entry(rng_list.next, struct hwrng, list); > - set_current_rng(new_rng); > - } > - } > + if (current_rng == rng) > + enable_best_rng(); > > if (list_empty(&rng_list)) { > mutex_unlock(&rng_mutex); looks like for me. reviewed-by: Harald Freudenberger <freude@linux.vnet.ibm.com>
Hi Harald, On 30 October 2017 at 18:58, Harald Freudenberger <freude@linux.vnet.ibm.com> wrote: > On 10/27/2017 07:04 PM, PrasannaKumar Muralidharan wrote: >> User is able to select a chosen rng by writing its name to rng_current >> but there is no way to reset it without unbinding the rng. Let user >> write "" to rng_current and delesect the chosen rng. >> >> Signed-off-by: PrasannaKumar Muralidharan <prasannatsmkumar@gmail.com> >> --- >> drivers/char/hw_random/core.c | 51 +++++++++++++++++++++++++++---------------- >> 1 file changed, 32 insertions(+), 19 deletions(-) >> >> diff --git a/drivers/char/hw_random/core.c b/drivers/char/hw_random/core.c >> index 9701ac7..be03024 100644 >> --- a/drivers/char/hw_random/core.c >> +++ b/drivers/char/hw_random/core.c >> @@ -292,26 +292,48 @@ static struct miscdevice rng_miscdev = { >> .groups = rng_dev_groups, >> }; >> >> +static int enable_best_rng(void) >> +{ >> + int ret = -ENODEV; >> + >> + BUG_ON(!mutex_is_locked(&rng_mutex)); >> + >> + /* rng_list is sorted by quality, use the best (=first) one */ >> + if (!list_empty(&rng_list)) { >> + struct hwrng *new_rng; >> + >> + new_rng = list_entry(rng_list.next, struct hwrng, list); >> + ret = ((new_rng == current_rng) ? 0 : set_current_rng(new_rng)); >> + if (!ret) >> + cur_rng_set_by_user = 0; >> + } >> + >> + return ret; >> +} >> + >> static ssize_t hwrng_attr_current_store(struct device *dev, >> struct device_attribute *attr, >> const char *buf, size_t len) >> { >> - int err; >> + int err = -ENODEV; > Two lines later err is assigned the return value of mutex_lock_interruptible(). > So the -ENODEV value assignment does nothing. Indeed true. Will change this when I make a v2. I am even thinking if it makes sense to return error when user writes "" to rng_current and there is no rng device registered. I think returning 0 should be fine. >> struct hwrng *rng; >> >> err = mutex_lock_interruptible(&rng_mutex); >> if (err) >> return -ERESTARTSYS; >> - err = -ENODEV; >> - list_for_each_entry(rng, &rng_list, list) { >> - if (sysfs_streq(rng->name, buf)) { >> - err = 0; >> - cur_rng_set_by_user = 1; >> - if (rng != current_rng) >> + >> + if (sysfs_streq(buf, "")) { >> + err = enable_best_rng(); >> + } else { >> + list_for_each_entry(rng, &rng_list, list) { >> + if (sysfs_streq(rng->name, buf)) { >> + cur_rng_set_by_user = 1; >> err = set_current_rng(rng); >> - break; >> + break; >> + } >> } >> } >> + >> mutex_unlock(&rng_mutex); >> >> return err ? : len; >> @@ -493,17 +515,8 @@ void hwrng_unregister(struct hwrng *rng) >> mutex_lock(&rng_mutex); >> >> list_del(&rng->list); >> - if (current_rng == rng) { >> - drop_current_rng(); >> - cur_rng_set_by_user = 0; >> - /* rng_list is sorted by quality, use the best (=first) one */ >> - if (!list_empty(&rng_list)) { >> - struct hwrng *new_rng; >> - >> - new_rng = list_entry(rng_list.next, struct hwrng, list); >> - set_current_rng(new_rng); >> - } >> - } >> + if (current_rng == rng) >> + enable_best_rng(); >> >> if (list_empty(&rng_list)) { >> mutex_unlock(&rng_mutex); > looks like for me. > reviewed-by: Harald Freudenberger <freude@linux.vnet.ibm.com> > Thank you. Thanks, PrasannaKumar
On Fri, Oct 27, 2017 at 10:34:04PM +0530, PrasannaKumar Muralidharan wrote: > User is able to select a chosen rng by writing its name to rng_current > but there is no way to reset it without unbinding the rng. Let user > write "" to rng_current and delesect the chosen rng. > > Signed-off-by: PrasannaKumar Muralidharan <prasannatsmkumar@gmail.com> Patch applied. Thanks.
diff --git a/drivers/char/hw_random/core.c b/drivers/char/hw_random/core.c index 9701ac7..be03024 100644 --- a/drivers/char/hw_random/core.c +++ b/drivers/char/hw_random/core.c @@ -292,26 +292,48 @@ static struct miscdevice rng_miscdev = { .groups = rng_dev_groups, }; +static int enable_best_rng(void) +{ + int ret = -ENODEV; + + BUG_ON(!mutex_is_locked(&rng_mutex)); + + /* rng_list is sorted by quality, use the best (=first) one */ + if (!list_empty(&rng_list)) { + struct hwrng *new_rng; + + new_rng = list_entry(rng_list.next, struct hwrng, list); + ret = ((new_rng == current_rng) ? 0 : set_current_rng(new_rng)); + if (!ret) + cur_rng_set_by_user = 0; + } + + return ret; +} + static ssize_t hwrng_attr_current_store(struct device *dev, struct device_attribute *attr, const char *buf, size_t len) { - int err; + int err = -ENODEV; struct hwrng *rng; err = mutex_lock_interruptible(&rng_mutex); if (err) return -ERESTARTSYS; - err = -ENODEV; - list_for_each_entry(rng, &rng_list, list) { - if (sysfs_streq(rng->name, buf)) { - err = 0; - cur_rng_set_by_user = 1; - if (rng != current_rng) + + if (sysfs_streq(buf, "")) { + err = enable_best_rng(); + } else { + list_for_each_entry(rng, &rng_list, list) { + if (sysfs_streq(rng->name, buf)) { + cur_rng_set_by_user = 1; err = set_current_rng(rng); - break; + break; + } } } + mutex_unlock(&rng_mutex); return err ? : len; @@ -493,17 +515,8 @@ void hwrng_unregister(struct hwrng *rng) mutex_lock(&rng_mutex); list_del(&rng->list); - if (current_rng == rng) { - drop_current_rng(); - cur_rng_set_by_user = 0; - /* rng_list is sorted by quality, use the best (=first) one */ - if (!list_empty(&rng_list)) { - struct hwrng *new_rng; - - new_rng = list_entry(rng_list.next, struct hwrng, list); - set_current_rng(new_rng); - } - } + if (current_rng == rng) + enable_best_rng(); if (list_empty(&rng_list)) { mutex_unlock(&rng_mutex);
User is able to select a chosen rng by writing its name to rng_current but there is no way to reset it without unbinding the rng. Let user write "" to rng_current and delesect the chosen rng. Signed-off-by: PrasannaKumar Muralidharan <prasannatsmkumar@gmail.com> --- drivers/char/hw_random/core.c | 51 +++++++++++++++++++++++++++---------------- 1 file changed, 32 insertions(+), 19 deletions(-)