[PATCH 3/3] 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.s...@redhat.com

Conflicts:
drivers/char/hw_random/virtio-rng.c
(Conflict due to change in context around the revert)
---
 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 32e6373..2080630 100644
--- a/drivers/char/hw_random/virtio-rng.c
+++ b/drivers/char/hw_random/virtio-rng.c
@@ -37,7 +37,6 @@ struct virtrng_info {
int index;
 };
 
-static bool probe_done;
 static bool hwrng_register_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

___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

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

[Amit Shah]

vdev is unused in struct virtrng_info, remove it.

CC: Amos Kong ak...@redhat.com
Signed-off-by: Amit Shah amit.s...@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

___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

[PATCH 2/3] 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.

Signed-off-by: Amit Shah amit.s...@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 d8ffebd..32e6373 100644
--- a/drivers/char/hw_random/virtio-rng.c
+++ b/drivers/char/hw_random/virtio-rng.c
@@ -38,6 +38,7 @@ struct virtrng_info {
 };
 
 static bool probe_done;
+static bool hwrng_register_done;
 
 static void random_recv_done(struct virtqueue *vq)
 {
@@ -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 (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)
+   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

___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

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

[Amit Shah]

Hi,

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

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

Patch 1 is a minor cleanup.

Please review and apply,

Amit Shah (3):
  virtio: rng: remove unused struct element
  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 | 35 +++
 2 files changed, 15 insertions(+), 26 deletions(-)

-- 
1.9.3

___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

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

[Amit Shah]

On (Mon) 21 Jul 2014 [15:01:00], 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.
 
 Signed-off-by: Amit Shah amit.s...@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 d8ffebd..32e6373 100644
 --- a/drivers/char/hw_random/virtio-rng.c
 +++ b/drivers/char/hw_random/virtio-rng.c
 @@ -38,6 +38,7 @@ struct virtrng_info {
  };
  
  static bool probe_done;
 +static bool hwrng_register_done;

Ah; this needs to be per-device after the recent multi-device work,
rather than a global.

New patches coming up.


Amit
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

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

[Amit Shah]

vdev is unused in struct virtrng_info, remove it.

CC: Amos Kong ak...@redhat.com
Signed-off-by: Amit Shah amit.s...@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

___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

[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

___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

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

___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

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

___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

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

___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

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.s...@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
 
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

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.s...@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
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

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 herb...@gondor.apana.org.au
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH 0/25] Replace DEFINE_PCI_DEVICE_TABLE macro use

[Bjorn Helgaas]

[+cc Jingoo]

On Fri, Jul 18, 2014 at 12:50 PM, James Bottomley
james.bottom...@hansenpartnership.com wrote:
 On Fri, 2014-07-18 at 11:17 -0700, Greg KH wrote:
 On Fri, Jul 18, 2014 at 09:54:32AM -0700, James Bottomley wrote:
  On Fri, 2014-07-18 at 09:43 -0700, Greg KH wrote:
   On Fri, Jul 18, 2014 at 12:22:13PM -0400, John W. Linville wrote:
On Fri, Jul 18, 2014 at 05:26:47PM +0200, Benoit Taine wrote:
 We should prefer `const struct pci_device_id` over
 `DEFINE_PCI_DEVICE_TABLE` to meet kernel coding style guidelines.
 This issue was reported by checkpatch.
   
Honestly, I prefer the macro -- it stands-out more.  Maybe the style
guidelines and/or checkpatch should change instead?
  
   The macro is horrid, no other bus has this type of thing just to save a
   few characters in typing
 
  OK, so this is the macro:
 
  #define DEFINE_PCI_DEVICE_TABLE(_table) \
  const struct pci_device_id _table[]
 
  Could you explain what's so horrible?
 
  The reason it's useful today is that people forget the const (and
  sometimes the [] making it a true table instead of a pointer).  If you
  use the DEFINE_PCI_DEVICE_TABLE macro, the compile breaks if you use it
  wrongly (good) and you automatically get the correct annotations.

 We have almost 1000 more uses of the non-macro version than the macro
 version in the kernel today:
 $ git grep -w DEFINE_PCI_DEVICE_TABLE | wc -l
 262
 $ git grep const struct pci_device_id | wc -l
 1254

 My big complaint is that we need to be consistant, either pick one or
 the other and stick to it.  As the macro is the least used, it's easiest
 to fix up, and it also is more consistant with all other kernel
 subsystems which do not have such a macro.

 I've a weak preference for consistency, but not at the expense of
 hundreds of patches churning the kernel to remove an innocuous macro.
 Churn costs time and effort.

 As there is no need for the __init macro mess anymore, there's no real
 need for the DEFINE_PCI_DEVICE_TABLE macro either.  I think checkpatch
 will catch the use of non-const users for the id table already today, it
 catches lots of other uses like this already.

   , so why should PCI be special in this regard
   anymore?
 
  I think the PCI usage dwarfs most other bus types now, so you could turn
  the question around.  However, I don't think majority voting is a good
  guide to best practise; lets debate the merits for their own sake.

 Not really dwarf, USB is close with over 700 such structures:
 $ git grep const struct usb_device_id | wc -l
 725

 And i2c is almost just as big as PCI:
 $ git grep const struct i2c_device_id | wc -l
 1223

 So again, this macro is not consistent with the majority of PCI drivers,
 nor with any other type of device id declaration in the kernel, which
 is why I feel it should be removed.

 And finally, the PCI documentation itself says to not use this macro, so
 this isn't a new thing.  From Documentation/PCI/pci.txt:

   The ID table is an array of struct pci_device_id entries ending with an
   all-zero entry.  Definitions with static const are generally preferred.
   Use of the deprecated macro DEFINE_PCI_DEVICE_TABLE should be avoided.

 That wording went into the file last December, when we last talked about
 this and everyone in that discussion agreed to remove the macro for the
 above reasons.

 Consistency matters.

 In this case, I don't think it does that much ... a cut and paste either
 way (from a macro or non-macro based driver) yields correct code.  Since
 there's no bug here and no apparent way to misuse the macro, why bother?

 Anyway, it's PCI code ... let the PCI maintainer decide.  However, if he
 does want this do it as one big bang patch via either the PCI or Trivial
 tree (latter because Jiří has experience doing this, but the former
 might be useful so the decider feels the pain ...)

I don't feel strongly either way, so I guess I'm OK with this, and in
the spirit of feeling the pain, I'm willing to handle it.  Jingoo
proposed similar patches, so it might be nice to give him some credit.

Benoit, how about if you wait until about half-way through the merge
window after v3.16 releases, generate an up-to-date single patch, and
post that?  Then we can try to get it in before v3.17-rc1 to minimize
merge hassles.

Bjorn
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization