From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Michael S. Tsirkin" Subject: Re: [PATCH v4 09/12] virtio: net: Add freeze, restore handlers to support S4 Date: Wed, 7 Dec 2011 13:46:03 +0200 Message-ID: <20111207114603.GF8326@redhat.com> References: <712d8d78cf730161181562ed44d4fe08a370b85e.1323199985.git.amit.shah@redhat.com> <20111207102824.GC8326@redhat.com> <20111207103529.GG4651@amit-x200.redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Content-Disposition: inline In-Reply-To: <20111207103529.GG4651@amit-x200.redhat.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: virtualization-bounces@lists.linux-foundation.org Errors-To: virtualization-bounces@lists.linux-foundation.org To: Amit Shah Cc: linux-kernel@vger.kernel.org, levinsasha928@gmail.com, Virtualization List List-Id: virtualization@lists.linuxfoundation.org On Wed, Dec 07, 2011 at 04:05:29PM +0530, Amit Shah wrote: > On (Wed) 07 Dec 2011 [12:28:24], Michael S. Tsirkin wrote: > > On Wed, Dec 07, 2011 at 01:18:47AM +0530, Amit Shah wrote: > > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c > > > index 697a0fc..1378f3c 100644 > > > --- a/drivers/net/virtio_net.c > > > +++ b/drivers/net/virtio_net.c > > > @@ -1151,6 +1151,38 @@ static void __devexit virtnet_remove(struct virtio_device *vdev) > > > free_netdev(vi->dev); > > > } > > > > > > +#ifdef CONFIG_PM > > > +static int virtnet_freeze(struct virtio_device *vdev) > > > +{ > > > + struct virtnet_info *vi = vdev->priv; > > > + > > > + netif_device_detach(vi->dev); > > > + if (netif_running(vi->dev)) > > > + napi_disable(&vi->napi); > > > + > > > > Could refill_work still be running at this point? > > Yes, it could. So moving the cancel_delayed_work_sync() before > disabling napi would work fine? No, because napi poll can schedule that. Further, refill can reschedule itself. It also looks like we have a bug in virtio net cleanup now: cancel_delayed_work_sync is called after unregister, so it will be calling napi API on an invalid device. And, if it schedules itself it will run after device is gone. I think we need some locking to fix this. > Anything else that might similar treatment? > > Amit -- MST