On Thu, Feb 18, 2016 at 3:07 PM, Peter Crosthwaite <[email protected]> wrote: > On Thu, Feb 18, 2016 at 1:47 PM, Paolo Bonzini <[email protected]> wrote: >> >> >> On 18/02/2016 10:56, Markus Armbruster wrote: >>> Alistair Francis <[email protected]> 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 <[email protected]> >>>> --- >>>> >>>> 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.
Ok, I am happy to do it that way. It just seemed dodgy to me registering the reset in the realise. This also seemed like a feature worth having, as I thought this would come up again in the future. >> > > Does this assume the device itself knows whether it is bus-connected > or not? This way has the advantage of catchall-ing devices that have > no bus connected and the device may or may not know whether it is > bus-connected (nor should it need to know). Probably doesn't have in > tree precedent yet, but I thought we wanted to move away from > qdev/qbus managing the device-tree. So ideally, the new else should > become unconditional long term once we debusify (and properly QOMify) > the reset tree (and the if goes away). That was my general thinking as well. Thanks, Alistair > >> 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) >> > > You could add a boolean flag to DeviceState that is set by this > registration. It can then be checked at unrealize to remove reset > handler. > > Regards, > Peter > >> * 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. >
