Did reply instead of reply all. Forwarding my previous message.
---------- Forwarded message ----------
From: PrasannaKumar Muralidharan <[email protected]>
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 <[email protected]>
Hi Jim,
On 3 November 2017 at 20:11, Jim Quigley <[email protected]> wrote:
>
>
> On 03/11/2017 13:06, PrasannaKumar Muralidharan wrote:
>>
>> Hi Jim,
>>
>> On 3 November 2017 at 15:27, Jim Quigley <[email protected]> wrote:
>>>
>>> The patch for
>>>
>>> commit: 5c06273401f2eb7b290cadbae18ee00f8f65e893
>>> Author: Amit Shah <[email protected]>
>>> 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 <[email protected]>
>>> Signed-off-by: Jim Quigley <[email protected]>
>>> ---
>>> 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
Hi Jim,
Have second thoughts on this.
On 3 November 2017 at 20:55, PrasannaKumar Muralidharan
<[email protected]> 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