From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:46484) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aWWQg-0003Gi-B8 for qemu-devel@nongnu.org; Thu, 18 Feb 2016 16:48:07 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aWWQb-0004ls-Cc for qemu-devel@nongnu.org; Thu, 18 Feb 2016 16:48:06 -0500 Received: from mail-wm0-x241.google.com ([2a00:1450:400c:c09::241]:33209) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aWWQb-0004lf-5w for qemu-devel@nongnu.org; Thu, 18 Feb 2016 16:48:01 -0500 Received: by mail-wm0-x241.google.com with SMTP id c200so5117995wme.0 for ; Thu, 18 Feb 2016 13:48:01 -0800 (PST) Sender: Paolo Bonzini References: <4744c26fdfd911c44b58a9b6e1d0effc6cb39594.1455739133.git.alistair.francis@xilinx.com> <87lh6ijqa2.fsf@blackfin.pond.sub.org> From: Paolo Bonzini Message-ID: <56C63C0D.8030501@redhat.com> Date: Thu, 18 Feb 2016 22:47:57 +0100 MIME-Version: 1.0 In-Reply-To: <87lh6ijqa2.fsf@blackfin.pond.sub.org> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v1 1/2] qdev-monitor.c: Register reset function if the device has one List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Markus Armbruster , Alistair Francis Cc: peter.maydell@linaro.org, qemu-devel@nongnu.org, crosthwaitepeter@gmail.com, cov@codeaurora.org, =?UTF-8?Q?Andreas_F=c3=a4rber?= , lig.fnst@cn.fujitsu.com On 18/02/2016 10:56, Markus Armbruster wrote: > Alistair Francis writes: > >> If the device being added when running qdev_device_add() has >> a reset function, register it so that it can be called. >> >> Signed-off-by: Alistair Francis >> --- >> >> qdev-monitor.c | 2 ++ >> 1 file changed, 2 insertions(+) >> >> diff --git a/qdev-monitor.c b/qdev-monitor.c >> index 81e3ff3..0a99d01 100644 >> --- a/qdev-monitor.c >> +++ b/qdev-monitor.c >> @@ -561,6 +561,8 @@ DeviceState *qdev_device_add(QemuOpts *opts, Error **errp) >> >> if (bus) { >> qdev_set_parent_bus(dev, bus); >> + } else if (dc->reset) { >> + qemu_register_reset((void (*)(void *))dc->reset, dev); >> } >> >> id = qemu_opts_id(opts); > > This looks wrong to me. > > You stuff all the device reset methods into the global reset_handlers > list, where they get called in some semi-random order. This breaks when > there are reset order dependencies between devices, e.g. between a > device and the bus it plugs into. There is no bus here, see the "if" above the one that's being added. However, what devices have done so far is to register/unregister the reset in the realize/unrealize methods, and I suggest doing the same. If you really want to add the magic qemu_register_reset, you should at least do one of the following: * add a matching unregister (no idea where) * assert that the device is not hot-unpluggable, otherwise hot-unplug would leave a dangling pointer. Paolo > Propagating the reset signal to all the devices is a qdev problem. > Copying Andreas for further insight.