On 21/06/2024 11:54, Harald Freudenberger wrote: > When there are rng sources registering at the hwrng core via > hwrng_register() a struct hwrng is delivered. There is a quality > field in there which is used to decide which of the registered > hw rng sources will be used by the hwrng core. > > With commit 16bdbae39428 ("hwrng: core - treat default_quality as > a maximum and default to 1024") there came in a new default of > 1024 in case this field is empty and all the known hw rng sources > at that time had been reworked to not fill this field and thus > use the default of 1024. > > The code choosing the 'better' hw rng source during registration > of a new hw rng source has never been adapted to this and thus > used 0 if the hw rng implementation does not fill the quality field. > So when two rng sources register, one with 0 (meaning 1024) and > the other one with 999, the 999 hw rng will be chosen. > > This patch simple takes into account that a quality field value > of 0 is to be treated as 1024 and then the decision about which > hw rng to use works as expected. > > Tested on s390 with two hardware rng sources: crypto cards and > trng true random generator device driver. > > Signed-off-by: Harald Freudenberger <freude@xxxxxxxxxxxxx> > > Reported-by: Christian Rund <Christian.Rund@xxxxxxxxxx> > Fixes: 16bdbae39428 ("hwrng: core - treat default_quality as a maximum and default to 1024") > --- > drivers/char/hw_random/core.c | 9 ++++++++- > 1 file changed, 8 insertions(+), 1 deletion(-) > > diff --git a/drivers/char/hw_random/core.c b/drivers/char/hw_random/core.c > index 4084df65c9fa..993b8a1f1d19 100644 > --- a/drivers/char/hw_random/core.c > +++ b/drivers/char/hw_random/core.c > @@ -525,6 +525,7 @@ static int hwrng_fillfn(void *unused) > > int hwrng_register(struct hwrng *rng) > { > + unsigned short rng_quality, cur_quality; In my opinion, we no not need these variables. > int err = -EINVAL; > struct hwrng *tmp; > > @@ -545,8 +546,14 @@ int hwrng_register(struct hwrng *rng) > complete(&rng->cleanup_done); > init_completion(&rng->dying); > > + /* Quality field not set in struct hwrng means 1024 */ > + rng_quality = rng->quality ? rng->quality : 1024; Please use the shortcut "(a) ?: (b)" for "(a) ? (a) : (b)", also remove non-necessary parenthesis. rng_quality = rng->quality ?: 1024; Because this variable is only used once, you can also change it directly in the if statement below. > + cur_quality = current_rng ? > + (current_rng->quality ? current_rng->quality : 1024) : > + 0; > + This one is not necessary. The quality field of current_rng is has been updated already by the hwrng_init() function. > if (!current_rng || > - (!cur_rng_set_by_user && rng->quality > current_rng->quality)) { > + (!cur_rng_set_by_user && rng_quality > cur_quality)) { Unfortunately, the quality field of rng is read here, before the quality field is updated by hwrng_init(). Maybe we can use the following: if (!current_rng || (!cur_rng_set_by_user && (rng->quality ?: 1024) > current_rng->quality)) { > /* > * Set new rng as current as the new rng source > * provides better entropy quality and was not -- Mit freundlichen Grüßen / Kind regards Holger Dengler -- IBM Systems, Linux on IBM Z Development dengler@xxxxxxxxxxxxx