* virtio:rng: Virtio RNG devices need to be re-registered after suspend/resume
@ 2017-11-03 9:57 Jim Quigley
2017-11-03 13:06 ` PrasannaKumar Muralidharan
` (2 more replies)
0 siblings, 3 replies; 9+ messages in thread
From: Jim Quigley @ 2017-11-03 9:57 UTC (permalink / raw)
To: mpm, herbert; +Cc: linux-crypto
The patch for
commit: 5c06273401f2eb7b290cadbae18ee00f8f65e893
Author: Amit Shah <amit.shah@redhat.com>
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 <Liam.Merwick@oracle.com>
Signed-off-by: Jim Quigley <Jim.Quigley@oracle.com>
---
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
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: virtio:rng: Virtio RNG devices need to be re-registered after suspend/resume
2017-11-03 9:57 virtio:rng: Virtio RNG devices need to be re-registered after suspend/resume Jim Quigley
@ 2017-11-03 13:06 ` PrasannaKumar Muralidharan
2017-11-03 14:41 ` Jim Quigley
2017-11-03 15:36 ` PrasannaKumar Muralidharan
2017-11-06 7:09 ` Herbert Xu
2 siblings, 1 reply; 9+ messages in thread
From: PrasannaKumar Muralidharan @ 2017-11-03 13:06 UTC (permalink / raw)
To: Jim Quigley
Cc: Matt Mackall, Herbert Xu,
open list:HARDWARE RANDOM NUMBER GENERATOR CORE
Hi Jim,
On 3 November 2017 at 15:27, Jim Quigley <Jim.Quigley@oracle.com> wrote:
> The patch for
>
> commit: 5c06273401f2eb7b290cadbae18ee00f8f65e893
> Author: Amit Shah <amit.shah@redhat.com>
> 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 <Liam.Merwick@oracle.com>
> Signed-off-by: Jim Quigley <Jim.Quigley@oracle.com>
> ---
> 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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: virtio:rng: Virtio RNG devices need to be re-registered after suspend/resume
2017-11-03 13:06 ` PrasannaKumar Muralidharan
@ 2017-11-03 14:41 ` Jim Quigley
[not found] ` <CANc+2y4cEAHudxdK6Y0TbskGMH6jpLg0cfWt77JR44fzYU94HA@mail.gmail.com>
0 siblings, 1 reply; 9+ messages in thread
From: Jim Quigley @ 2017-11-03 14:41 UTC (permalink / raw)
To: PrasannaKumar Muralidharan
Cc: Matt Mackall, Herbert Xu,
open list:HARDWARE RANDOM NUMBER GENERATOR CORE, Jim Quigley
On 03/11/2017 13:06, PrasannaKumar Muralidharan wrote:
> Hi Jim,
>
> On 3 November 2017 at 15:27, Jim Quigley <Jim.Quigley@oracle.com> wrote:
>> The patch for
>>
>> commit: 5c06273401f2eb7b290cadbae18ee00f8f65e893
>> Author: Amit Shah <amit.shah@redhat.com>
>> 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 <Liam.Merwick@oracle.com>
>> Signed-off-by: Jim Quigley <Jim.Quigley@oracle.com>
>> ---
>> 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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Fwd: virtio:rng: Virtio RNG devices need to be re-registered after suspend/resume
[not found] ` <CANc+2y4cEAHudxdK6Y0TbskGMH6jpLg0cfWt77JR44fzYU94HA@mail.gmail.com>
@ 2017-11-03 15:25 ` PrasannaKumar Muralidharan
2017-11-03 15:33 ` PrasannaKumar Muralidharan
0 siblings, 1 reply; 9+ messages in thread
From: PrasannaKumar Muralidharan @ 2017-11-03 15:25 UTC (permalink / raw)
To: Matt Mackall, Herbert Xu,
open list:HARDWARE RANDOM NUMBER GENERATOR CORE, Jim Quigley
Did reply instead of reply all. Forwarding my previous message.
---------- Forwarded message ----------
From: PrasannaKumar Muralidharan <prasannatsmkumar@gmail.com>
Date: 3 November 2017 at 20:19
Subject: Re: virtio:rng: Virtio RNG devices need to be re-registered
after suspend/resume
To: Jim Quigley <jim.quigley@oracle.com>
Hi Jim,
On 3 November 2017 at 20:11, Jim Quigley <jim.quigley@oracle.com> wrote:
>
>
> On 03/11/2017 13:06, PrasannaKumar Muralidharan wrote:
>>
>> Hi Jim,
>>
>> On 3 November 2017 at 15:27, Jim Quigley <Jim.Quigley@oracle.com> wrote:
>>>
>>> The patch for
>>>
>>> commit: 5c06273401f2eb7b290cadbae18ee00f8f65e893
>>> Author: Amit Shah <amit.shah@redhat.com>
>>> 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 <Liam.Merwick@oracle.com>
>>> Signed-off-by: Jim Quigley <Jim.Quigley@oracle.com>
>>> ---
>>> 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.
True. As restore uses probe_common calling hwrng_register is required.
But I think it is not correct way to do.
>
> 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.
Regards,
PrasannaKumar
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: virtio:rng: Virtio RNG devices need to be re-registered after suspend/resume
2017-11-03 15:25 ` Fwd: " PrasannaKumar Muralidharan
@ 2017-11-03 15:33 ` PrasannaKumar Muralidharan
0 siblings, 0 replies; 9+ messages in thread
From: PrasannaKumar Muralidharan @ 2017-11-03 15:33 UTC (permalink / raw)
To: Matt Mackall, Herbert Xu,
open list:HARDWARE RANDOM NUMBER GENERATOR CORE, Jim Quigley
Hi Jim,
Have second thoughts on this.
On 3 November 2017 at 20:55, PrasannaKumar Muralidharan
<prasannatsmkumar@gmail.com> 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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: virtio:rng: Virtio RNG devices need to be re-registered after suspend/resume
2017-11-03 9:57 virtio:rng: Virtio RNG devices need to be re-registered after suspend/resume Jim Quigley
2017-11-03 13:06 ` PrasannaKumar Muralidharan
@ 2017-11-03 15:36 ` PrasannaKumar Muralidharan
2017-11-06 7:09 ` Herbert Xu
2 siblings, 0 replies; 9+ messages in thread
From: PrasannaKumar Muralidharan @ 2017-11-03 15:36 UTC (permalink / raw)
To: Jim Quigley
Cc: Matt Mackall, Herbert Xu,
open list:HARDWARE RANDOM NUMBER GENERATOR CORE
Jim,
Minor comment below.
On 3 November 2017 at 15:27, Jim Quigley <Jim.Quigley@oracle.com> wrote:
> The patch for
>
> commit: 5c06273401f2eb7b290cadbae18ee00f8f65e893
> Author: Amit Shah <amit.shah@redhat.com>
> 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 <Liam.Merwick@oracle.com>
> Signed-off-by: Jim Quigley <Jim.Quigley@oracle.com>
> ---
> 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 <prasannatsmkumar@gmail.com>
Regards,
PrasannaKumar
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: virtio:rng: Virtio RNG devices need to be re-registered after suspend/resume
2017-11-03 9:57 virtio:rng: Virtio RNG devices need to be re-registered after suspend/resume Jim Quigley
2017-11-03 13:06 ` PrasannaKumar Muralidharan
2017-11-03 15:36 ` PrasannaKumar Muralidharan
@ 2017-11-06 7:09 ` Herbert Xu
2017-11-07 3:14 ` PrasannaKumar Muralidharan
2 siblings, 1 reply; 9+ messages in thread
From: Herbert Xu @ 2017-11-06 7:09 UTC (permalink / raw)
To: Jim Quigley; +Cc: mpm, linux-crypto
On Fri, Nov 03, 2017 at 09:57:21AM +0000, Jim Quigley wrote:
> The patch for
>
> commit: 5c06273401f2eb7b290cadbae18ee00f8f65e893
> Author: Amit Shah <amit.shah@redhat.com>
> 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 <Liam.Merwick@oracle.com>
> Signed-off-by: Jim Quigley <Jim.Quigley@oracle.com>
Patch applied. Thanks.
--
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: virtio:rng: Virtio RNG devices need to be re-registered after suspend/resume
2017-11-06 7:09 ` Herbert Xu
@ 2017-11-07 3:14 ` PrasannaKumar Muralidharan
2017-11-07 3:36 ` Herbert Xu
0 siblings, 1 reply; 9+ messages in thread
From: PrasannaKumar Muralidharan @ 2017-11-07 3:14 UTC (permalink / raw)
To: Herbert Xu
Cc: Jim Quigley, Matt Mackall,
open list:HARDWARE RANDOM NUMBER GENERATOR CORE
Hi Herbert,
On 6 November 2017 at 12:39, Herbert Xu <herbert@gondor.apana.org.au> wrote:
> On Fri, Nov 03, 2017 at 09:57:21AM +0000, 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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: virtio:rng: Virtio RNG devices need to be re-registered after suspend/resume
2017-11-07 3:14 ` PrasannaKumar Muralidharan
@ 2017-11-07 3:36 ` Herbert Xu
0 siblings, 0 replies; 9+ messages in thread
From: Herbert Xu @ 2017-11-07 3:36 UTC (permalink / raw)
To: PrasannaKumar Muralidharan
Cc: Jim Quigley, Matt Mackall,
open list:HARDWARE RANDOM NUMBER GENERATOR CORE
On Tue, Nov 07, 2017 at 08:44:17AM +0530, PrasannaKumar Muralidharan wrote:
> Hi Herbert,
>
> On 6 November 2017 at 12:39, Herbert Xu <herbert@gondor.apana.org.au> wrote:
> > On Fri, Nov 03, 2017 at 09:57:21AM +0000, 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 <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2017-11-07 3:37 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-11-03 9:57 virtio:rng: Virtio RNG devices need to be re-registered after suspend/resume Jim Quigley
2017-11-03 13:06 ` PrasannaKumar Muralidharan
2017-11-03 14:41 ` Jim Quigley
[not found] ` <CANc+2y4cEAHudxdK6Y0TbskGMH6jpLg0cfWt77JR44fzYU94HA@mail.gmail.com>
2017-11-03 15:25 ` Fwd: " PrasannaKumar Muralidharan
2017-11-03 15:33 ` PrasannaKumar Muralidharan
2017-11-03 15:36 ` PrasannaKumar Muralidharan
2017-11-06 7:09 ` Herbert Xu
2017-11-07 3:14 ` PrasannaKumar Muralidharan
2017-11-07 3:36 ` Herbert Xu
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox