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.quig...@oracle.com> wrote:
The patch for

commit: 5c06273401f2eb7b290cadbae18ee00f8f65e893
Author: Amit Shah <amit.s...@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.merw...@oracle.com>
Signed-off-by: Jim Quigley <jim.quig...@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