Re: virtio:rng: Virtio RNG devices need to be re-registered after suspend/resume

[Jim Quigley <jim.quigley@oracle.com>]

On 03/11/2017 13:06, PrasannaKumar Muralidharan wrote:
> Hi Jim,
>
> On 3 November 2017 at 15:27, Jim Quigley <Jim.Quigley@oracle.com> wrote:
>> The patch for
>>
>> commit: 5c06273401f2eb7b290cadbae18ee00f8f65e893
>> Author: Amit Shah <amit.shah@redhat.com>
>> Date:   Sun Jul 27 07:34:01 2014 +0930
>>
>>      virtio: rng: delay hwrng_register() till driver is ready
>>
>> moved the call to hwrng_register() out of the probe routine into the scan
>> routine. We need to call hwrng_register() after a suspend/restore cycle
>> to re-register the device, but the scan function is not invoked for the
>> restore. Add the call to hwrng_register() to virtio_restore().
>>
>> Reviewed-by: Liam Merwick <Liam.Merwick@oracle.com>
>> Signed-off-by: Jim Quigley <Jim.Quigley@oracle.com>
>> ---
>>   drivers/char/hw_random/virtio-rng.c | 21 ++++++++++++++++++++-
>>   1 file changed, 20 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/char/hw_random/virtio-rng.c b/drivers/char/hw_random/virtio-rng.c
>> index 3fa2f8a..b89df66 100644
>> --- a/drivers/char/hw_random/virtio-rng.c
>> +++ b/drivers/char/hw_random/virtio-rng.c
>> @@ -184,7 +184,26 @@ static int virtrng_freeze(struct virtio_device *vdev)
>>
>>   static int virtrng_restore(struct virtio_device *vdev)
>>   {
>> -       return probe_common(vdev);
>> +       int err;
>> +
>> +       err = probe_common(vdev);
>> +       if (!err) {
>> +               struct virtrng_info *vi = vdev->priv;
>> +
>> +               /*
>> +                * Set hwrng_removed to ensure that virtio_read()
>> +                * does not block waiting for data before the
>> +                * registration is complete.
>> +                */
>> +               vi->hwrng_removed = true;
>> +               err = hwrng_register(&vi->hwrng);
>> +               if (!err) {
>> +                       vi->hwrng_register_done = true;
>> +                       vi->hwrng_removed = false;
>> +               }
>> +       }
>> +
>> +       return err;
>>   }
>>   #endif
>>
>> --
>> 1.8.3.1
>>
> This patch makes me wonder why hwrng_unregister is required in
> virtrng_freeze. Looks strange and unusual. May be that is not required
> and it can be removed. If it is required can you please add a comment
> on why it is required?

     The reason it's required is because the virtrng_restore() uses 
probe_common() which allocates
     a new virtrng_info struct, changing  the devices private pointer . 
This virtrng struct is used in
     hwrng_register() to set the current RNG etc.  If we don't 
unregister/re-register then we would
     need to split probe_common() to avoid

                     vi = kzalloc(sizeof(struct virtrng_info), GFP_KERNEL);

     overwriting vdev->priv on a restore.

     It would be cleaner to just get rid of probe_common() altogether in 
that case, and do whatever
     needs to be done in virtrng_probe()/virtrng_restore() respectively, 
but I didn't want to change code
     affecting the normal probe path as well as suspend/resume. Is it OK 
to leave it that way to avoid
     the more extensive changes ?

     thanks

     regards

     Jim Q.





> Thanks,
> PrasannaKumar