From: Jim Quigley Subject: Re: virtio:rng: Virtio RNG devices need to be re-registered after suspend/resume Date: Fri, 3 Nov 2017 14:41:27 +0000 Message-ID: References: <1509703041-6281-1-git-send-email-Jim.Quigley@oracle.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit Cc: Matt Mackall , Herbert Xu , "open list:HARDWARE RANDOM NUMBER GENERATOR CORE" , Jim Quigley To: PrasannaKumar Muralidharan Return-path: Received: from aserp1040.oracle.com ([141.146.126.69]:33066 "EHLO aserp1040.oracle.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751370AbdKCOlY (ORCPT ); Fri, 3 Nov 2017 10:41:24 -0400 In-Reply-To: Content-Language: en-US Sender: linux-crypto-owner@vger.kernel.org List-ID: 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