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

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

[Jim Quigley]

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

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

[PrasannaKumar Muralidharan]

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?

Thanks,
PrasannaKumar

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

[Jim Quigley]

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

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

[PrasannaKumar Muralidharan]

Did reply instead of reply all. Forwarding my previous message.

---------- Forwarded message ----------
From: PrasannaKumar Muralidharan <prasannatsmkumar@gmail.com>
Date: 3 November 2017 at 20:19
Subject: Re: virtio:rng: Virtio RNG devices need to be re-registered
after suspend/resume
To: Jim Quigley <jim.quigley@oracle.com>


Hi Jim,

On 3 November 2017 at 20:11, Jim Quigley <jim.quigley@oracle.com> wrote:
>
>
> 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.

True. As restore uses probe_common calling hwrng_register is required.

But I think it is not correct way to do.

>
>     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

That's correct.

> 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 ?

Personally I would prefer to do the cleaner way. It is up to the
virtio and hwrng maintainer.

Regards,
PrasannaKumar

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

[PrasannaKumar Muralidharan]

Hi Jim,

Have second thoughts on this.

On 3 November 2017 at 20:55, PrasannaKumar Muralidharan
<prasannatsmkumar@gmail.com> wrote:
>>
>>     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
>
> That's correct.
>
>> 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 ?
>
> Personally I would prefer to do the cleaner way. It is up to the
> virtio and hwrng maintainer.

You are trying to restore the status quo of the driver that was before
the commit 5c06273401. It is really important and valid. The driver's
suspend/resume code is not in best shape but that is not what you are
trying to solve. I am fine with this change.

Regards,
PrasannaKumar

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

[PrasannaKumar Muralidharan]

Jim,

Minor comment below.

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;

Hwrng core does not call read method till hwrng_register has finished.
Is this really required? I think it has no effect.

> +               err = hwrng_register(&vi->hwrng);
> +               if (!err) {
> +                       vi->hwrng_register_done = true;
> +                       vi->hwrng_removed = false;
> +               }
> +       }
> +
> +       return err;
>  }
>  #endif
>
> --
> 1.8.3.1
>

I am fine with this change as is. Reviewed-by: PrasannaKumar
Muralidharan <prasannatsmkumar@gmail.com>

Regards,
PrasannaKumar

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

[Herbert Xu]

On Fri, Nov 03, 2017 at 09:57:21AM +0000, Jim Quigley 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>

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

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

[PrasannaKumar Muralidharan]

Hi Herbert,

On 6 November 2017 at 12:39, Herbert Xu <herbert@gondor.apana.org.au> wrote:
> On Fri, Nov 03, 2017 at 09:57:21AM +0000, Jim Quigley wrote:
>> 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().
>
> Patch applied.  Thanks.

My rb tag is missing in the commit.

Regards,
PrasannaKumar

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

[Herbert Xu]

On Tue, Nov 07, 2017 at 08:44:17AM +0530, PrasannaKumar Muralidharan wrote:
> Hi Herbert,
> 
> On 6 November 2017 at 12:39, Herbert Xu <herbert@gondor.apana.org.au> wrote:
> > On Fri, Nov 03, 2017 at 09:57:21AM +0000, Jim Quigley wrote:
> >> 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().
> >
> > Patch applied.  Thanks.
> 
> My rb tag is missing in the commit.

That's because you didn't start it on a new line.

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt