| Message ID | 20170221141451.28305-24-marcandre.lureau@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 21/02/2017 15:14, Marc-André Lureau wrote: > Apparently, none of the bus owner give a reference to the hotplug > handler property, do not unref it on bus release. > > Furthermore, a bus is allowed to be its own hotplug handler, which can > be seen in qbus_set_bus_hotplug_handler() function. However, in this > case, the reference can't be given to the property, or this will create > a cyclic dependency and the bus will never be free. > > Each bus owner should manage the lifecycle of the hotplug handler. > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> Almost all qbus_set_hotplug_handler callers are using it to set the parent device (that is, the "adapter" or "bridge", whatever you want to call it) as the hotplug handler. The exception is piix4_update_bus_hotplug. This one is the only case where OBJ_PROP_LINK_UNREF_ON_RELEASE would be right. Luckily, there _is_ a reference to that device somewhere else to keep it alive, namely in /machine's acpi-device prop. So this case is a bit hacky (not your fault) but works as well. In addition the PIIX4_PM device is not hot(un)pluggable. Can you please add a comment to piix4_update_bus_hotplug explaining that the PIIX4PMState cannot outlive the PCIBus, because /machine keeps it alive? > > diff --git a/hw/core/bus.c b/hw/core/bus.c > index cf383fc1af..4651f24486 100644 > --- a/hw/core/bus.c > +++ b/hw/core/bus.c > @@ -197,7 +197,7 @@ static void qbus_initfn(Object *obj) > TYPE_HOTPLUG_HANDLER, > (Object **)&bus->hotplug_handler, > object_property_allow_set_link, > - OBJ_PROP_LINK_UNREF_ON_RELEASE, > + 0, > NULL); > object_property_add_bool(obj, "realized", > bus_get_realized, bus_set_realized, NULL); > -- > 2.11.0.295.gd7dffce1c.dirty > > >
Hi On Wed, Feb 22, 2017 at 3:39 PM Paolo Bonzini <pbonzini@redhat.com> wrote: > > > On 21/02/2017 15:14, Marc-André Lureau wrote: > > Apparently, none of the bus owner give a reference to the hotplug > > handler property, do not unref it on bus release. > > > > Furthermore, a bus is allowed to be its own hotplug handler, which can > > be seen in qbus_set_bus_hotplug_handler() function. However, in this > > case, the reference can't be given to the property, or this will create > > a cyclic dependency and the bus will never be free. > > > > Each bus owner should manage the lifecycle of the hotplug handler. > > > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> > > Almost all qbus_set_hotplug_handler callers are using it to set the > parent device (that is, the "adapter" or "bridge", whatever you want to > call it) as the hotplug handler. > > The exception is piix4_update_bus_hotplug. This one is the only case > where OBJ_PROP_LINK_UNREF_ON_RELEASE would be right. Luckily, there > _is_ a reference to that device somewhere else to keep it alive, namely > in /machine's acpi-device prop. So this case is a bit hacky (not your > fault) but works as well. In addition the PIIX4_PM device is not > hot(un)pluggable. > I understand you agree with my change, correct? > > Can you please add a comment to piix4_update_bus_hotplug explaining that > the PIIX4PMState cannot outlive the PCIBus, because /machine keeps it > alive? > Do you mean PCIBus cannot outlive PIIX4PMState? (ie the PIIX4PMState will always be there since it's linked from /machine) thanks > > > > > diff --git a/hw/core/bus.c b/hw/core/bus.c > > index cf383fc1af..4651f24486 100644 > > --- a/hw/core/bus.c > > +++ b/hw/core/bus.c > > @@ -197,7 +197,7 @@ static void qbus_initfn(Object *obj) > > TYPE_HOTPLUG_HANDLER, > > (Object **)&bus->hotplug_handler, > > object_property_allow_set_link, > > - OBJ_PROP_LINK_UNREF_ON_RELEASE, > > + 0, > > NULL); > > object_property_add_bool(obj, "realized", > > bus_get_realized, bus_set_realized, NULL); > > -- > > 2.11.0.295.gd7dffce1c.dirty > > > > > > > > -- Marc-André Lureau
On 22/02/2017 14:03, Marc-André Lureau wrote: > Hi > > On Wed, Feb 22, 2017 at 3:39 PM Paolo Bonzini <pbonzini@redhat.com > <mailto:pbonzini@redhat.com>> wrote: > > > > On 21/02/2017 15:14, Marc-André Lureau wrote: > > Apparently, none of the bus owner give a reference to the hotplug > > handler property, do not unref it on bus release. > > > > Furthermore, a bus is allowed to be its own hotplug handler, which can > > be seen in qbus_set_bus_hotplug_handler() function. However, in this > > case, the reference can't be given to the property, or this will > create > > a cyclic dependency and the bus will never be free. > > > > Each bus owner should manage the lifecycle of the hotplug handler. > > > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com > <mailto:marcandre.lureau@redhat.com>> > > Almost all qbus_set_hotplug_handler callers are using it to set the > parent device (that is, the "adapter" or "bridge", whatever you want to > call it) as the hotplug handler. > > The exception is piix4_update_bus_hotplug. This one is the only case > where OBJ_PROP_LINK_UNREF_ON_RELEASE would be right. Luckily, there > _is_ a reference to that device somewhere else to keep it alive, namely > in /machine's acpi-device prop. So this case is a bit hacky (not your > fault) but works as well. In addition the PIIX4_PM device is not > hot(un)pluggable. > > I understand you agree with my change, correct? Yes. > Can you please add a comment to piix4_update_bus_hotplug explaining that > the PIIX4PMState cannot outlive the PCIBus, because /machine keeps > it alive? > > > Do you mean PCIBus cannot outlive PIIX4PMState? (ie the PIIX4PMState > will always be there since it's linked from /machine) Yes (or just because it's not unpluggable). Paolo
diff --git a/hw/core/bus.c b/hw/core/bus.c index cf383fc1af..4651f24486 100644 --- a/hw/core/bus.c +++ b/hw/core/bus.c @@ -197,7 +197,7 @@ static void qbus_initfn(Object *obj) TYPE_HOTPLUG_HANDLER, (Object **)&bus->hotplug_handler, object_property_allow_set_link, - OBJ_PROP_LINK_UNREF_ON_RELEASE, + 0, NULL); object_property_add_bool(obj, "realized", bus_get_realized, bus_set_realized, NULL);
Apparently, none of the bus owner give a reference to the hotplug handler property, do not unref it on bus release. Furthermore, a bus is allowed to be its own hotplug handler, which can be seen in qbus_set_bus_hotplug_handler() function. However, in this case, the reference can't be given to the property, or this will create a cyclic dependency and the bus will never be free. Each bus owner should manage the lifecycle of the hotplug handler. Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> --- hw/core/bus.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)