Re: [PATCH] hw_random: core: Reset user selected rng by writing "" to rng_current

[Herbert Xu]

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 

Patch applied.  Thanks.
-- 
Email: Herbert Xu 
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH] hw_random: core: Reset user selected rng by writing "" to rng_current

[PrasannaKumar Muralidharan]

Hi Harald,

On 30 October 2017 at 18:58, Harald Freudenberger
 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 
>> ---
>>  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(_mutex));
>> +
>> + /* rng_list is sorted by quality, use the best (=first) one */
>> + if (!list_empty(_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(_mutex);
>>   if (err)
>>   return -ERESTARTSYS;
>> - err = -ENODEV;
>> - list_for_each_entry(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, _list, list) {
>> + if (sysfs_streq(rng->name, buf)) {
>> + cur_rng_set_by_user = 1;
>>   err = set_current_rng(rng);
>> - break;
>> + break;
>> + }
>>   }
>>   }
>> +
>>   mutex_unlock(_mutex);
>>
>>   return err ? : len;
>> @@ -493,17 +515,8 @@ void hwrng_unregister(struct hwrng *rng)
>>   mutex_lock(_mutex);
>>
>>   list_del(>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(_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(_list)) {
>>   mutex_unlock(_mutex);
> looks like for me.
> reviewed-by: Harald Freudenberger 
>

Thank you.

Thanks,
PrasannaKumar

Re: [PATCH] hw_random: core: Reset user selected rng by writing "" to rng_current

[Harald Freudenberger]

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 
> ---
>  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(_mutex));
> +
> + /* rng_list is sorted by quality, use the best (=first) one */
> + if (!list_empty(_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(_mutex);
>   if (err)
>   return -ERESTARTSYS;
> - err = -ENODEV;
> - list_for_each_entry(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, _list, list) {
> + if (sysfs_streq(rng->name, buf)) {
> + cur_rng_set_by_user = 1;
>   err = set_current_rng(rng);
> - break;
> + break;
> + }
>   }
>   }
> +
>   mutex_unlock(_mutex);
>
>   return err ? : len;
> @@ -493,17 +515,8 @@ void hwrng_unregister(struct hwrng *rng)
>   mutex_lock(_mutex);
>
>   list_del(>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(_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(_list)) {
>   mutex_unlock(_mutex);
looks like for me.
reviewed-by: Harald Freudenberger 

Re: [PATCH] hw_random: core: Reset user selected rng by writing "" to rng_current

[PrasannaKumar Muralidharan]

Hi Harald,

On 30 October 2017 at 13:40, Harald Freudenberger
 wrote:
> That's a really good idea. I also thought about something like that.

Do you mind reviewing the code?

Thanks,
PrasannaKumar

Re: [PATCH] hw_random: core: Reset user selected rng by writing "" to rng_current

[Harald Freudenberger]

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 
> ---
>  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(_mutex));
> +
> + /* rng_list is sorted by quality, use the best (=first) one */
> + if (!list_empty(_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(_mutex);
>   if (err)
>   return -ERESTARTSYS;
> - err = -ENODEV;
> - list_for_each_entry(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, _list, list) {
> + if (sysfs_streq(rng->name, buf)) {
> + cur_rng_set_by_user = 1;
>   err = set_current_rng(rng);
> - break;
> + break;
> + }
>   }
>   }
> +
>   mutex_unlock(_mutex);
>
>   return err ? : len;
> @@ -493,17 +515,8 @@ void hwrng_unregister(struct hwrng *rng)
>   mutex_lock(_mutex);
>
>   list_del(>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(_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(_list)) {
>   mutex_unlock(_mutex);
That's a really good idea. I also thought about something like that.
regards
Harald Freudenberger