Linux Virtualization - Aug 2014 - [3.16 stable PATCH 1/2] virtio: rng: delay hwrng_register() till driver is ready

Instead of calling hwrng_register() in the probe routing, call it in the
scan routine.  This ensures that when hwrng_register() is successful,
and it requests a few random bytes to seed the kernel's pool at init,
we're ready to service that request.

This will also enable us to remove the workaround added previously to
check whether probe was completed, and only then ask for data from the
host.  The revert follows in the next commit.

There's a slight behaviour change here on unsuccessful hwrng_register().
Previously, when hwrng_register() failed, the probe() routine would
fail, and the vqs would be torn down, and driver would be marked not
initialized.  Now, the vqs will remain initialized, driver would be
marked initialized as well, but won't be available in the list of RNGs
available to hwrng core.  To fix the failures, the procedure remains the
same, i.e. unload and re-load the module, and hope things succeed the
next time around.

Signed-off-by: Amit Shah <amit.shah at redhat.com>
Signed-off-by: Rusty Russell <rusty at rustcorp.com.au>
(cherry picked from commit 5c06273401f2eb7b290cadbae18ee00f8f65e893)
Signed-off-by: Amit Shah <amit.shah at redhat.com>

Conflicts:
	drivers/char/hw_random/virtio-rng.c

  Context conflict due to not backporting two commits that changed
  struct layout.
---
 drivers/char/hw_random/virtio-rng.c | 25 +++++++++++++++----------
 1 file changed, 15 insertions(+), 10 deletions(-)

diff --git a/drivers/char/hw_random/virtio-rng.c
b/drivers/char/hw_random/virtio-rng.c
index e9b15bc..f4e04f3 100644
--- a/drivers/char/hw_random/virtio-rng.c
+++ b/drivers/char/hw_random/virtio-rng.c
@@ -36,6 +36,7 @@ struct virtrng_info {
 	bool busy;
 	char name[25];
 	int index;
+	bool hwrng_register_done;
 };
 
 static bool probe_done;
@@ -137,15 +138,6 @@ static int probe_common(struct virtio_device *vdev)
 		return err;
 	}
 
-	err = hwrng_register(&vi->hwrng);
-	if (err) {
-		vdev->config->del_vqs(vdev);
-		vi->vq = NULL;
-		kfree(vi);
-		ida_simple_remove(&rng_index_ida, index);
-		return err;
-	}
-
 	probe_done = true;
 	return 0;
 }
@@ -153,9 +145,11 @@ static int probe_common(struct virtio_device *vdev)
 static void remove_common(struct virtio_device *vdev)
 {
 	struct virtrng_info *vi = vdev->priv;
+
 	vdev->config->reset(vdev);
 	vi->busy = false;
-	hwrng_unregister(&vi->hwrng);
+	if (vi->hwrng_register_done)
+		hwrng_unregister(&vi->hwrng);
 	vdev->config->del_vqs(vdev);
 	ida_simple_remove(&rng_index_ida, vi->index);
 	kfree(vi);
@@ -171,6 +165,16 @@ static void virtrng_remove(struct virtio_device *vdev)
 	remove_common(vdev);
 }
 
+static void virtrng_scan(struct virtio_device *vdev)
+{
+	struct virtrng_info *vi = vdev->priv;
+	int err;
+
+	err = hwrng_register(&vi->hwrng);
+	if (!err)
+		vi->hwrng_register_done = true;
+}
+
 #ifdef CONFIG_PM_SLEEP
 static int virtrng_freeze(struct virtio_device *vdev)
 {
@@ -195,6 +199,7 @@ static struct virtio_driver virtio_rng_driver = {
 	.id_table =	id_table,
 	.probe =	virtrng_probe,
 	.remove =	virtrng_remove,
+	.scan =		virtrng_scan,
 #ifdef CONFIG_PM_SLEEP
 	.freeze =	virtrng_freeze,
 	.restore =	virtrng_restore,
-- 
1.9.3

This reverts commit e052dbf554610e2104c5a7518c4d8374bed701bb.

Now that we use the virtio ->scan() function to register with the hwrng
core, we will not get read requests till probe is successfully finished.

So revert the workaround we had in place to refuse read requests while
we were not yet setup completely.

Signed-off-by: Amit Shah <amit.shah at redhat.com>
Signed-off-by: Rusty Russell <rusty at rustcorp.com.au>
(cherry picked from commit eeec626366ffe558fc3d5685bd2b49a962acf57d)
Signed-off-by: Amit Shah <amit.shah at redhat.com>
---
 drivers/char/hw_random/core.c       | 6 ------
 drivers/char/hw_random/virtio-rng.c | 9 ---------
 2 files changed, 15 deletions(-)

diff --git a/drivers/char/hw_random/core.c b/drivers/char/hw_random/core.c
index c4419ea..2a451b1 100644
--- a/drivers/char/hw_random/core.c
+++ b/drivers/char/hw_random/core.c
@@ -68,12 +68,6 @@ static void add_early_randomness(struct hwrng *rng)
 	unsigned char bytes[16];
 	int bytes_read;
 
-	/*
-	 * Currently only virtio-rng cannot return data during device
-	 * probe, and that's handled in virtio-rng.c itself.  If there
-	 * are more such devices, this call to rng_get_data can be
-	 * made conditional here instead of doing it per-device.
-	 */
 	bytes_read = rng_get_data(rng, bytes, sizeof(bytes), 1);
 	if (bytes_read > 0)
 		add_device_randomness(bytes, bytes_read);
diff --git a/drivers/char/hw_random/virtio-rng.c
b/drivers/char/hw_random/virtio-rng.c
index f4e04f3..f1aa13b 100644
--- a/drivers/char/hw_random/virtio-rng.c
+++ b/drivers/char/hw_random/virtio-rng.c
@@ -39,7 +39,6 @@ struct virtrng_info {
 	bool hwrng_register_done;
 };
 
-static bool probe_done;
 
 static void random_recv_done(struct virtqueue *vq)
 {
@@ -70,13 +69,6 @@ static int virtio_read(struct hwrng *rng, void *buf, size_t
size, bool wait)
 	int ret;
 	struct virtrng_info *vi = (struct virtrng_info *)rng->priv;
 
-	/*
-	 * Don't ask host for data till we're setup.  This call can
-	 * happen during hwrng_register(), after commit d9e7972619.
-	 */
-	if (unlikely(!probe_done))
-		return 0;
-
 	if (!vi->busy) {
 		vi->busy = true;
 		init_completion(&vi->have_data);
@@ -138,7 +130,6 @@ static int probe_common(struct virtio_device *vdev)
 		return err;
 	}
 
-	probe_done = true;
 	return 0;
 }
 
-- 
1.9.3

On Tue, Aug 12, 2014 at 12:36:54PM +0530, Amit Shah
wrote:
> Instead of calling hwrng_register() in the probe routing, call it in the
> scan routine.  This ensures that when hwrng_register() is successful,
> and it requests a few random bytes to seed the kernel's pool at init,
> we're ready to service that request.
> 
> This will also enable us to remove the workaround added previously to
> check whether probe was completed, and only then ask for data from the
> host.  The revert follows in the next commit.
> 
> There's a slight behaviour change here on unsuccessful
hwrng_register().
> Previously, when hwrng_register() failed, the probe() routine would
> fail, and the vqs would be torn down, and driver would be marked not
> initialized.  Now, the vqs will remain initialized, driver would be
> marked initialized as well, but won't be available in the list of RNGs
> available to hwrng core.  To fix the failures, the procedure remains the
> same, i.e. unload and re-load the module, and hope things succeed the
> next time around.
> 
> Signed-off-by: Amit Shah <amit.shah at redhat.com>
> Signed-off-by: Rusty Russell <rusty at rustcorp.com.au>
> (cherry picked from commit 5c06273401f2eb7b290cadbae18ee00f8f65e893)
> Signed-off-by: Amit Shah <amit.shah at redhat.com>
> 
> Conflicts:
> 	drivers/char/hw_random/virtio-rng.c
> 
>   Context conflict due to not backporting two commits that changed
>   struct layout.
What is this "Conflicts:" stuff for?  I don't need that...

greg k-h

On (Tue) 12 Aug 2014 [15:41:35], Greg KH wrote:
> On Tue, Aug 12, 2014 at 12:36:54PM +0530, Amit Shah wrote:
> > Instead of calling hwrng_register() in the probe routing, call it in
the
> > scan routine.  This ensures that when hwrng_register() is successful,
> > and it requests a few random bytes to seed the kernel's pool at
init,
> > we're ready to service that request.
> > 
> > This will also enable us to remove the workaround added previously to
> > check whether probe was completed, and only then ask for data from the
> > host.  The revert follows in the next commit.
> > 
> > There's a slight behaviour change here on unsuccessful
hwrng_register().
> > Previously, when hwrng_register() failed, the probe() routine would
> > fail, and the vqs would be torn down, and driver would be marked not
> > initialized.  Now, the vqs will remain initialized, driver would be
> > marked initialized as well, but won't be available in the list of
RNGs
> > available to hwrng core.  To fix the failures, the procedure remains
the
> > same, i.e. unload and re-load the module, and hope things succeed the
> > next time around.
> > 
> > Signed-off-by: Amit Shah <amit.shah at redhat.com>
> > Signed-off-by: Rusty Russell <rusty at rustcorp.com.au>
> > (cherry picked from commit 5c06273401f2eb7b290cadbae18ee00f8f65e893)
> > Signed-off-by: Amit Shah <amit.shah at redhat.com>
> > 
> > Conflicts:
> > 	drivers/char/hw_random/virtio-rng.c
> > 
> >   Context conflict due to not backporting two commits that changed
> >   struct layout.
> 
> What is this "Conflicts:" stuff for?  I don't need that...
To note the patch didn't cherry-pick as-is; anyway, sent a v2 without
that text.

Thanks,
		Amit