On 06/30/2017 07:27 AM, PrasannaKumar Muralidharan wrote: > Hi Harald, > > Can you split this patch into two? One patch to choose rng based on > the quality and another for not overriding user decided rng. > > Some more minor comments below. > > On 29 June 2017 at 15:33, Harald Freudenberger > <freude@xxxxxxxxxxxxxxxxxx> wrote: >> The hwrng core implementation currently doesn't consider the >> quality field of the struct hwrng. So the first registered rng >> is the winner and further rng sources even with much better >> quality are ignored. >> >> The behavior should be that always the best rng with the highest >> quality rate should be used as current rng source. Only if the >> user explicitly chooses a rng source (via writing a rng name >> to /sys/class/misc/hw_random) the decision for the best quality >> should be suppressed. >> >> This patch makes hwrng always hold a list of registered rng >> sources sorted decreasing by quality. On registration of a new >> hwrng source the list is updated and if the current rng source >> was not chosen by user and the new rng provides better quality >> set as new current rng source. Similar on unregistration of an >> rng, if it was the current used rng source the one with the >> next highest quality is used. If a rng source has been set via >> sysfs from userland as long as this one doesn't unregister >> it is kept as current rng regardless of registration of 'better' >> rng sources. > Nice to see the patch. This is indeed required. > >> Signed-off-by: Harald Freudenberger <freude@xxxxxxxxxxxxxxxxxx> >> --- >> drivers/char/hw_random/core.c | 31 +++++++++++++++++++++++++------ >> 1 file changed, 25 insertions(+), 6 deletions(-) >> >> diff --git a/drivers/char/hw_random/core.c b/drivers/char/hw_random/core.c >> index 503a41d..7fe47f8 100644 >> --- a/drivers/char/hw_random/core.c >> +++ b/drivers/char/hw_random/core.c >> @@ -28,7 +28,10 @@ >> #define RNG_MODULE_NAME "hw_random" >> >> static struct hwrng *current_rng; >> +/* the current rng has been explicitly chosen by user via sysfs */ >> +static int cur_rng_set_by_user; > Letting the user know that the current rng was selected based on user > input would be a good option I guess. Any thoughts on this? > >> static struct task_struct *hwrng_fill; >> +/* list of registered rngs, sorted decending by quality */ >> static LIST_HEAD(rng_list); >> /* Protects rng_list and current_rng */ >> static DEFINE_MUTEX(rng_mutex); >> @@ -308,6 +311,8 @@ static ssize_t hwrng_attr_current_store(struct device *dev, >> break; >> } >> } >> + if (!err) >> + cur_rng_set_by_user = 1; > This can be put inside the loop. The if condition will go away in that case. > >> mutex_unlock(&rng_mutex); >> >> return err ? : len; >> @@ -417,6 +422,7 @@ int hwrng_register(struct hwrng *rng) >> { >> int err = -EINVAL; >> struct hwrng *old_rng, *tmp; >> + struct list_head *ptr; > Any better name instead of ptr? > >> if (!rng->name || (!rng->data_read && !rng->read)) >> goto out; >> @@ -432,14 +438,26 @@ int hwrng_register(struct hwrng *rng) >> init_completion(&rng->cleanup_done); >> complete(&rng->cleanup_done); >> >> + /* rng_list is sorted by decreasing quality */ >> + list_for_each(ptr, &rng_list) { >> + tmp = list_entry(ptr, struct hwrng, list); >> + if (tmp->quality < rng->quality) >> + break; >> + } >> + list_add_tail(&rng->list, ptr); >> + >> old_rng = current_rng; >> err = 0; >> - if (!old_rng) { >> + if (!old_rng || >> + (!cur_rng_set_by_user && rng->quality > old_rng->quality)) { >> + /* >> + * Set new rng as current if no current rng or rng was >> + * not chosen by user and the new one has better quality. >> + */ >> err = set_current_rng(rng); >> if (err) >> goto out_unlock; >> } >> - list_add_tail(&rng->list, &rng_list); >> >> if (old_rng && !rng->init) { >> /* >> @@ -466,12 +484,13 @@ void hwrng_unregister(struct hwrng *rng) >> 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 *tail; >> - >> - tail = list_entry(rng_list.prev, struct hwrng, list); >> + struct hwrng *new_rng; >> >> - set_current_rng(tail); >> + new_rng = list_entry(rng_list.next, struct hwrng, list); >> + set_current_rng(new_rng); >> } >> } >> >> -- >> 2.7.4 >> > This patch looks good. I am fine with this patch as is. Reviewed-by: > PrasannaKumar Muralidharan <prasannatsmkumar@xxxxxxxxx> > > If this patch is split into please go ahead and my reviewed-by tag. > > Regards, > PrasannaKumar > Thanks for this feedback. I will split into two and work in some improvements. regards Harald Freudenberger