From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:35636) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Tke9H-0004KV-GV for qemu-devel@nongnu.org; Mon, 17 Dec 2012 12:06:44 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Tke9F-0002hv-Ph for qemu-devel@nongnu.org; Mon, 17 Dec 2012 12:06:39 -0500 Received: from mx1.redhat.com ([209.132.183.28]:33545) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Tke9F-0002hl-G2 for qemu-devel@nongnu.org; Mon, 17 Dec 2012 12:06:37 -0500 Message-ID: <50CF5118.5080209@redhat.com> Date: Mon, 17 Dec 2012 18:06:32 +0100 From: Paolo Bonzini MIME-Version: 1.0 References: <1355761490-10073-1-git-send-email-pbonzini@redhat.com> <1355761490-10073-2-git-send-email-pbonzini@redhat.com> <20121217165203.GB29085@redhat.com> <20121217165340.GA29744@redhat.com> In-Reply-To: <20121217165340.GA29744@redhat.com> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit 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: "Michael S. Tsirkin" Cc: aliguori@us.ibm.com, qemu-devel@nongnu.org Il 17/12/2012 17:53, Michael S. Tsirkin ha scritto: >>> 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? Not pretty. The child device doesn't exist until the parent has been initialized, it doesn't make sense to reset it. I can add an assertion to make it a problem, of course. :) >>> 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? Initialization should run in pre-order (unlike reset): the parent needs to be ready in order to start the child. What you typically have, is that the parent device initializes itself, and then calls qdev_create/qdev_init on the child before returning. So it is already being delayed. The delay is explicit in program flow, not hidden in qdev semantics. Paolo >>> } >>> return 0; >>> } >>> -- >>> 1.8.0.2 >>>