virtio-rng: contribute to early randomness requests

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_unregister() 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 <[email protected]>

---
 drivers/char/hw_random/virtio-rng.c | 25 +++++++++++++++----------
 1 file changed, 15 insertions(+), 10 deletions(-)

drivers/char/hw_random/virtio-rng.c +1 -0

index a156284..d9927eb 100644
@@ -35,6 +35,7 @@ struct virtrng_info {
 	unsigned int data_avail;
 	int index;
 	bool busy;
+	bool hwrng_register_done;
 };
 
 static bool probe_done;

@@ -136,15 +137,6 @@ static int probe_common(struct virtio_device *vdev)
 		return err;
 	}

drivers/char/hw_random/virtio-rng.c +3 -1

 	probe_done = true;
 	return 0;
 }

@@ -152,9 +144,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);

@@ -170,6 +164,16 @@ static void virtrng_remove(struct virtio_device *vdev)
 	remove_common(vdev);

drivers/char/hw_random/virtio-rng.c +3 -0

+}
+
 #ifdef CONFIG_PM_SLEEP
 static int virtrng_freeze(struct virtio_device *vdev)
 {

@@ -194,6 +198,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,

Signature · 1 line

1.9.3

Quoted 9 lines · Amit Shah

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.

Quoted 8 lines

There's a slight behaviour change here on unsuccessful hwrng_register(). Previously, when hwrng_unregister() 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.

I'm not too comfortable with this. I'll try to take a closer look tonight, but in the meantime...

Quoted 21 lines

Signed-off-by Amit Shah <[email protected]>

---
 drivers/char/hw_random/virtio-rng.c | 25 +++++++++++++++----------
 1 file changed, 15 insertions(+), 10 deletions(-)

drivers/char/hw_random/virtio-rng.c +1 -0

index a156284..d9927eb 100644
@@ -35,6 +35,7 @@ struct virtrng_info {
 	unsigned int data_avail;
 	int index;
 	bool busy;
+	bool hwrng_register_done;
 };
 
 static bool probe_done;

@@ -136,15 +137,6 @@ static int probe_common(struct virtio_device *vdev)
 		return err;
 	}
 

drivers/char/hw_random/virtio-rng.c static int probe_common(struct virtio_device *vdev) · line 147

Quoted 9 lines

drivers/char/hw_random/virtio-rng.c +0 -9

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

This needs to stay. register, and failure to do so, should occur in the probe routine.

Quoted 20 lines

drivers/char/hw_random/virtio-rng.c +3 -1

 	probe_done = true;
 	return 0;
 }

@@ -152,9 +144,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);

@@ -170,6 +164,16 @@ static void virtrng_remove(struct virtio_device *vdev)
 	remove_common(vdev);
 }
 

drivers/char/hw_random/virtio-rng.c static void virtrng_remove(struct virtio_device *vdev) · line 174

Quoted 8 lines

drivers/char/hw_random/virtio-rng.c +8 -0

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

Instead, perhaps we should just feed the entropy pool from here? We would still need to prevent the core from doing so. Perhaps back to the flag idea?

Quoted 5 lines

drivers/char/hw_random/virtio-rng.c +0 -0

@@ -136,15 +137,6 @@ static int probe_common(struct virtio_device *vdev)
 		return err;
 	}
 

drivers/char/hw_random/virtio-rng.c static int probe_common(struct virtio_device *vdev) · line 147

Quoted 9 lines

drivers/char/hw_random/virtio-rng.c +0 -9

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

This needs to stay. register, and failure to do so, should occur in the probe routine.

Quoted 5 lines · Jason Cooper

Quoted 5 lines

drivers/char/hw_random/virtio-rng.c +0 -0

@@ -136,15 +137,6 @@ static int probe_common(struct virtio_device *vdev)
 		return err;
 	}
 

Quoted 12 lines

drivers/char/hw_random/virtio-rng.c static int probe_common(struct virtio_device *vdev) · line 147

Quoted 9 lines

drivers/char/hw_random/virtio-rng.c +0 -9

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

This needs to stay. register, and failure to do so, should occur in the probe routine.

Why?

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_unregister() 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 <[email protected]>

Quoted 9 lines · Amit Shah

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.

Quoted 2 lines

There's a slight behaviour change here on unsuccessful hwrng_register(). Previously, when hwrng_unregister() failed, the probe() routine would

typo: should be hwrng_register().

Please fix this up when picking up the patch.

v2:
- update patch 3 to store the hwrng_register_done bool per-device rather than global
- add patch 2 that re-arranges struct elems for better packing.

Hi,

This series enables virtio-rng to service the early randomness requests made by the hwrng core (patch 3), with Herbert's idea of using the scan routine.

Patch 4 reverts the previous restriction, which no longer applies, to not send read requests to the host before successful probe.

Patches 1 and 2 are minor cleanups.

Please review and apply,

Amit Shah (4):
virtio: rng: remove unused struct element
virtio: rng: re-arrange struct elements for better packing
virtio: rng: delay hwrng_register() till driver is ready Revert "hwrng: virtio - ensure reads happen after successful probe"

 drivers/char/hw_random/core.c       |  6 ------
 drivers/char/hw_random/virtio-rng.c | 39 ++++++++++++++++---------------------
 2 files changed, 17 insertions(+), 28 deletions(-)

Quoted 24 lines · Amit Shah

v2:
- update patch 3 to store the hwrng_register_done bool per-device rather than global
- add patch 2 that re-arranges struct elems for better packing.

Hi,

This series enables virtio-rng to service the early randomness requests made by the hwrng core (patch 3), with Herbert's idea of using the scan routine.

Patch 4 reverts the previous restriction, which no longer applies, to not send read requests to the host before successful probe.

Patches 1 and 2 are minor cleanups.

Please review and apply,

Amit Shah (4):
virtio: rng: remove unused struct element
virtio: rng: re-arrange struct elements for better packing
virtio: rng: delay hwrng_register() till driver is ready Revert "hwrng: virtio - ensure reads happen after successful probe"

Quoted 3 lines

 drivers/char/hw_random/core.c       |  6 ------
 drivers/char/hw_random/virtio-rng.c | 39 ++++++++++++++++---------------------
 2 files changed, 17 insertions(+), 28 deletions(-)

fwiw,

Reviewed-by Jason Cooper <[email protected]>

Thanks for putting this together Amit!