From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:59482) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Tkdtx-0002nW-HX for qemu-devel@nongnu.org; Mon, 17 Dec 2012 11:50:53 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Tkdtu-0006eJ-K7 for qemu-devel@nongnu.org; Mon, 17 Dec 2012 11:50:49 -0500 Received: from mx1.redhat.com ([209.132.183.28]:14778) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Tkdtu-0006eA-AL for qemu-devel@nongnu.org; Mon, 17 Dec 2012 11:50:46 -0500 Date: Mon, 17 Dec 2012 18:53:41 +0200 From: "Michael S. Tsirkin" Message-ID: <20121217165340.GA29744@redhat.com> References: <1355761490-10073-1-git-send-email-pbonzini@redhat.com> <1355761490-10073-2-git-send-email-pbonzini@redhat.com> <20121217165203.GB29085@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20121217165203.GB29085@redhat.com> Subject: Re: [Qemu-devel] [PATCH 01/15] qdev: do not reset a device until the parent has been initialized List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Paolo Bonzini Cc: aliguori@us.ibm.com, qemu-devel@nongnu.org On Mon, Dec 17, 2012 at 06:52:03PM +0200, Michael S. Tsirkin wrote: > On Mon, Dec 17, 2012 at 05:24:36PM +0100, Paolo Bonzini wrote: > > When a device creates a bus and more devices as part of its init callback, the > > child device could be reset while the parent is still only partly initialized. > > In this case, the right thing to do is to delay resetting the child. Do not > > do it at all in qdev_init, instead use qdev_reset_all to reset already-created > > devices when the state goes from CREATED to INITIALIZED. > > > > This happens when hotplugging a usb-storage device. Without this patch, > > initialization of a hotplugged usb-storage device would run in pre-order. > > Initialization of a coldplugged usb-storage device would run according to > > qdev_reset_all semantics (pre-order right now, post-order later in the > > series). > > Is this a problem or just not pretty? > > > This patch makes things consistent. > > > > Signed-off-by: Paolo Bonzini > > --- > > hw/qdev.c | 5 +++-- > > 1 file changed, 3 insertions(+), 2 deletions(-) > > > > diff --git a/hw/qdev.c b/hw/qdev.c > > index 599382c..2fdf4ac 100644 > > --- a/hw/qdev.c > > +++ b/hw/qdev.c > > @@ -176,8 +176,9 @@ int qdev_init(DeviceState *dev) > > dev->alias_required_for_version); > > } > > dev->state = DEV_STATE_INITIALIZED; > > - if (dev->hotplugged) { > > - device_reset(dev); > > + if (dev->hotplugged && > > + dev->parent_bus->parent->state == DEV_STATE_INITIALIZED) { > > + qdev_reset_all(dev); > > Let's add a comment explaining the why of this test, and when > will reset happen if it does not trigger here. Also - shouldn't device init be delayed as well? What do you think? > > } > > return 0; > > } > > -- > > 1.8.0.2 > >