Re: virtio:rng: Virtio RNG devices need to be re-registered after suspend/resume
On Tue, Nov 07, 2017 at 08:44:17AM +0530, PrasannaKumar Muralidharan wrote: > Hi Herbert, > > On 6 November 2017 at 12:39, Herbert Xu wrote: > > On Fri, Nov 03, 2017 at 09:57:21AM +, Jim Quigley wrote: > >> 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(). > > > > Patch applied. Thanks. > > My rb tag is missing in the commit. That's because you didn't start it on a new line. Cheers, -- Email: Herbert Xu Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Re: virtio:rng: Virtio RNG devices need to be re-registered after suspend/resume
Hi Herbert, On 6 November 2017 at 12:39, Herbert Xu wrote: > On Fri, Nov 03, 2017 at 09:57:21AM +, Jim Quigley wrote: >> 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(). > > Patch applied. Thanks. My rb tag is missing in the commit. Regards, PrasannaKumar
Re: virtio:rng: Virtio RNG devices need to be re-registered after suspend/resume
On Fri, Nov 03, 2017 at 09:57:21AM +, Jim Quigley wrote: > The patch for > > commit: 5c06273401f2eb7b290cadbae18ee00f8f65e893 > Author: Amit Shah > 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 > Signed-off-by: Jim Quigley Patch applied. Thanks. -- Email: Herbert Xu Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Re: virtio:rng: Virtio RNG devices need to be re-registered after suspend/resume
Jim, Minor comment below. On 3 November 2017 at 15:27, Jim Quigley wrote: > The patch for > > commit: 5c06273401f2eb7b290cadbae18ee00f8f65e893 > Author: Amit Shah > 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 > Signed-off-by: Jim Quigley > --- > 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; Hwrng core does not call read method till hwrng_register has finished. Is this really required? I think it has no effect. > + err = hwrng_register(&vi->hwrng); > + if (!err) { > + vi->hwrng_register_done = true; > + vi->hwrng_removed = false; > + } > + } > + > + return err; > } > #endif > > -- > 1.8.3.1 > I am fine with this change as is. Reviewed-by: PrasannaKumar Muralidharan Regards, PrasannaKumar
Re: virtio:rng: Virtio RNG devices need to be re-registered after suspend/resume
Hi Jim, Have second thoughts on this. On 3 November 2017 at 20:55, PrasannaKumar Muralidharan wrote: >> >> 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 > > That's correct. > >> 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 ? > > Personally I would prefer to do the cleaner way. It is up to the > virtio and hwrng maintainer. You are trying to restore the status quo of the driver that was before the commit 5c06273401. It is really important and valid. The driver's suspend/resume code is not in best shape but that is not what you are trying to solve. I am fine with this change. Regards, PrasannaKumar
Re: virtio:rng: Virtio RNG devices need to be re-registered after suspend/resume
On 03/11/2017 13:06, PrasannaKumar Muralidharan wrote: Hi Jim, On 3 November 2017 at 15:27, Jim Quigley wrote: The patch for commit: 5c06273401f2eb7b290cadbae18ee00f8f65e893 Author: Amit Shah 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 Signed-off-by: Jim Quigley --- 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
Re: virtio:rng: Virtio RNG devices need to be re-registered after suspend/resume
Hi Jim, On 3 November 2017 at 15:27, Jim Quigley wrote: > The patch for > > commit: 5c06273401f2eb7b290cadbae18ee00f8f65e893 > Author: Amit Shah > 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 > Signed-off-by: Jim Quigley > --- > 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? Thanks, PrasannaKumar