[PATCH v2 0/4] virtio-rng: contribute to early randomness requests

[PATCH v2 0/4] virtio-rng: contribute to early randomness requests

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

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

-- 
1.9.3

[PATCH v2 1/4] virtio: rng: remove unused struct element

[Amit Shah]

vdev is unused in struct virtrng_info, remove it.

CC: Amos Kong <akong@redhat.com>
Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
 drivers/char/hw_random/virtio-rng.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/char/hw_random/virtio-rng.c b/drivers/char/hw_random/virtio-rng.c
index e9b15bc..d8ffebd 100644
--- a/drivers/char/hw_random/virtio-rng.c
+++ b/drivers/char/hw_random/virtio-rng.c
@@ -28,7 +28,6 @@
 static DEFINE_IDA(rng_index_ida);
 
 struct virtrng_info {
-	struct virtio_device *vdev;
 	struct hwrng hwrng;
 	struct virtqueue *vq;
 	unsigned int data_avail;
-- 
1.9.3

[PATCH v2 2/4] virtio: rng: re-arrange struct elements for better packing

[Amit Shah]

Re-arrange the elements of the virtrng_info struct to pack it better.

Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
 drivers/char/hw_random/virtio-rng.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/char/hw_random/virtio-rng.c b/drivers/char/hw_random/virtio-rng.c
index d8ffebd..a156284 100644
--- a/drivers/char/hw_random/virtio-rng.c
+++ b/drivers/char/hw_random/virtio-rng.c
@@ -30,11 +30,11 @@ static DEFINE_IDA(rng_index_ida);
 struct virtrng_info {
 	struct hwrng hwrng;
 	struct virtqueue *vq;
-	unsigned int data_avail;
 	struct completion have_data;
-	bool busy;
 	char name[25];
+	unsigned int data_avail;
 	int index;
+	bool busy;
 };
 
 static bool probe_done;
-- 
1.9.3

[PATCH v2 3/4] virtio: rng: delay hwrng_register() till driver is ready

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

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 <amit.shah@redhat.com>
---
 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 a156284..d9927eb 100644
--- a/drivers/char/hw_random/virtio-rng.c
+++ b/drivers/char/hw_random/virtio-rng.c
@@ -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;
 	}
 
-	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;
 }
@@ -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);
 }
 
+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)
 {
@@ -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,
-- 
1.9.3

Re: [PATCH v2 3/4] virtio: rng: delay hwrng_register() till driver is ready

[Jason Cooper]

On Mon, Jul 21, 2014 at 05:15:51PM +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_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...

> Signed-off-by: Amit Shah <amit.shah@redhat.com>
> ---
>  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 a156284..d9927eb 100644
> --- a/drivers/char/hw_random/virtio-rng.c
> +++ b/drivers/char/hw_random/virtio-rng.c
> @@ -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;
>  	}
>  
> -	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.

>  	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);
>  }
>  
> +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?

thx,

Jason.

> +}
> +
>  #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,
> -- 
> 1.9.3
> 

Re: [PATCH v2 3/4] virtio: rng: delay hwrng_register() till driver is ready

[Amit Shah]

On (Mon) 21 Jul 2014 [08:11:16], Jason Cooper wrote:
> On Mon, Jul 21, 2014 at 05:15:51PM +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_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...
> 
> > Signed-off-by: Amit Shah <amit.shah@redhat.com>
> > ---
> >  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 a156284..d9927eb 100644
> > --- a/drivers/char/hw_random/virtio-rng.c
> > +++ b/drivers/char/hw_random/virtio-rng.c
> > @@ -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;
> >  	}
> >  
> > -	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.

Can you elaborate why?

> >  	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);
> >  }
> >  
> > +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?

No way hwrng knows the difference between probe and scan for
virtio-rng, so it's back to the delayed workqueue idea, if this isn't
usable..

But I need to understand why this isn't workable.

Thanks,

		Amit

Re: [PATCH v2 3/4] virtio: rng: delay hwrng_register() till driver is ready

[Herbert Xu]

On Mon, Jul 21, 2014 at 08:11:16AM -0400, Jason Cooper wrote:
>
> > @@ -136,15 +137,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;
> > -	}
> > -
> 
> This needs to stay.  register, and failure to do so, should occur in the
> probe routine.

Why? Probing involves finding out whether the hardware is present.
While hwrng_register is about adding an entry in the hwrng system
which may only be one aspect of the given hardware.

For example, the same hardware may support other features that are
used outside of the hwrng system, e.g., crypto.

So there's nothing special about probe that requires us to have the
hwrng_register call there.  In practice, the only difference between
having it in probe vs. scan is that you can return errors.  The only
error that can be returned is ENOMEM which isn't of much interest to
the caller of probe anyway.

On the other hand, if you are calling hwrng_register you better be
damn sure that your hardware is ready to answer requests from the
hwrng system.  Please don't add silly flags to work around this.

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

Re: [PATCH v2 3/4] virtio: rng: delay hwrng_register() till driver is ready

[Jason Cooper]

On Mon, Jul 21, 2014 at 08:41:24PM +0800, Herbert Xu wrote:
> On Mon, Jul 21, 2014 at 08:11:16AM -0400, Jason Cooper wrote:
> >
> > > @@ -136,15 +137,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;
> > > -	}
> > > -
> > 
> > This needs to stay.  register, and failure to do so, should occur in the
> > probe routine.
> 
> Why? 

hmm, yes. On reviewing the code, my objection was silly.  It mostly
stemmed from the commit message's mentioning of reduced error handling.

thx,

Jason.

Re: [PATCH v2 3/4] virtio: rng: delay hwrng_register() till driver is ready

[Amit Shah]

On (Mon) 21 Jul 2014 [17:15:51], 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_unregister() failed, the probe() routine would

typo: should be hwrng_register().

Please fix this up when picking up the patch.

Thanks,

> 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@redhat.com>

		Amit

Re: [PATCH v2 3/4] virtio: rng: delay hwrng_register() till driver is ready

[Rusty Russell]

Amit Shah <amit.shah@redhat.com> writes:
> On (Mon) 21 Jul 2014 [17:15:51], 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_unregister() failed, the probe() routine would
>
> typo: should be hwrng_register().
>
> Please fix this up when picking up the patch.

Done.

All 4 patches applied!
Rusty.

[PATCH v2 4/4] Revert "hwrng: virtio - ensure reads happen after successful probe"

[Amit Shah]

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@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 d9927eb..0027137 100644
--- a/drivers/char/hw_random/virtio-rng.c
+++ b/drivers/char/hw_random/virtio-rng.c
@@ -38,7 +38,6 @@ struct virtrng_info {
 	bool hwrng_register_done;
 };
 
-static bool probe_done;
 
 static void random_recv_done(struct virtqueue *vq)
 {
@@ -69,13 +68,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);
@@ -137,7 +129,6 @@ static int probe_common(struct virtio_device *vdev)
 		return err;
 	}
 
-	probe_done = true;
 	return 0;
 }
 
-- 
1.9.3

Re: [PATCH v2 0/4] virtio-rng: contribute to early randomness requests

[Jason Cooper]

On Mon, Jul 21, 2014 at 05:15:48PM +0530, Amit Shah wrote:
> 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(-)

fwiw,

Reviewed-by: Jason Cooper <jason@lakedaemon.net>

Thanks for putting this together Amit!

thx,

Jason.

Re: [PATCH v2 0/4] virtio-rng: contribute to early randomness requests

[Amit Shah]

On (Tue) 22 Jul 2014 [07:46:23], Jason Cooper wrote:
> On Mon, Jul 21, 2014 at 05:15:48PM +0530, Amit Shah wrote:
> > 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(-)
> 
> fwiw,
> 
> Reviewed-by: Jason Cooper <jason@lakedaemon.net>
> 
> Thanks for putting this together Amit!

Thanks, Jason!

		Amit