On Wed, 26 Sep 2018 11:42:13 +0200
David Hildenbrand <da...@redhat.com> wrote:
> The unplug and unplug_request handlers are special: They are not
> executed when unrealizing a device, but rather trigger the removal of a
> device from device_del() via object_unparent() - to effectively
> unrealize a device.
>
> If such a device has a child bus and another device attached to
> that bus (e.g. how virtio devices are created with their proxy device),
> we will not get a call to the unplug handler. As we want to support
> hotplug handlers (and especially also some unplug logic to undo resource
> assignment) for such devices, we cannot simply call the unplug handler
> when unrealizing - it has a different semantic ("trigger removal").
>
> To handle this scenario, we need a do_unplug handler, that will be
> executed for all devices with a hotplug handler.
could you clarify what would be call flow for unplug in this case
starting from 'device_del'?
>
> While at it, introduce hotplug_fn_nofail and fix a spelling mistake in
> a comment.
>
> Signed-off-by: David Hildenbrand <da...@redhat.com>
> ---
> hw/core/hotplug.c | 10 ++++++++++
> hw/core/qdev.c | 6 ++++++
> include/hw/hotplug.h | 26 ++++++++++++++++++++++++--
> 3 files changed, 40 insertions(+), 2 deletions(-)
>
> diff --git a/hw/core/hotplug.c b/hw/core/hotplug.c
> index 2253072d0e..e7a68d5160 100644
> --- a/hw/core/hotplug.c
> +++ b/hw/core/hotplug.c
> @@ -45,6 +45,16 @@ void hotplug_handler_post_plug(HotplugHandler
> *plug_handler,
> }
> }
>
> +void hotplug_handler_do_unplug(HotplugHandler *plug_handler,
> + DeviceState *plugged_dev)
> +{
> + HotplugHandlerClass *hdc = HOTPLUG_HANDLER_GET_CLASS(plug_handler);
> +
> + if (hdc->do_unplug) {
> + hdc->do_unplug(plug_handler, plugged_dev);
> + }
> +}
> +
> void hotplug_handler_unplug_request(HotplugHandler *plug_handler,
> DeviceState *plugged_dev,
> Error **errp)
> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> index 36b788a66b..dde2726099 100644
> --- a/hw/core/qdev.c
> +++ b/hw/core/qdev.c
> @@ -873,6 +873,12 @@ static void device_set_realized(Object *obj, bool value,
> Error **errp)
> }
> } else if (!value && dev->realized) {
> Error **local_errp = NULL;
> +
> + hotplug_ctrl = qdev_get_hotplug_handler(dev);
> + if (hotplug_ctrl) {
> + hotplug_handler_do_unplug(hotplug_ctrl, dev);
> + }
> +
> QLIST_FOREACH(bus, &dev->child_bus, sibling) {
> local_errp = local_err ? NULL : &local_err;
> object_property_set_bool(OBJECT(bus), false, "realized",
> diff --git a/include/hw/hotplug.h b/include/hw/hotplug.h
> index 51541d63e1..2fa5833cf1 100644
> --- a/include/hw/hotplug.h
> +++ b/include/hw/hotplug.h
> @@ -31,13 +31,21 @@ typedef struct HotplugHandler {
>
> /**
> * hotplug_fn:
> - * @plug_handler: a device performing plug/uplug action
> + * @plug_handler: a device performing (un)plug action
> * @plugged_dev: a device that has been (un)plugged
> * @errp: returns an error if this function fails
> */
> typedef void (*hotplug_fn)(HotplugHandler *plug_handler,
> DeviceState *plugged_dev, Error **errp);
>
> +/**
> + * hotplug_fn_nofail:
> + * @plug_handler: a device performing un(plug) action
> + * @plugged_dev: a device that has been (un)plugged
> + */
> +typedef void (*hotplug_fn_nofail)(HotplugHandler *plug_handler,
> + DeviceState *plugged_dev);
> +
> /**
> * HotplugDeviceClass:
> *
> @@ -49,12 +57,17 @@ typedef void (*hotplug_fn)(HotplugHandler *plug_handler,
> * @plug: plug callback called at end of device.realize(true).
> * @post_plug: post plug callback called after device.realize(true) and
> device
> * reset
> + * @do_unplug: unplug callback called at start of device.realize(false)
> * @unplug_request: unplug request callback.
> * Used as a means to initiate device unplug for devices
> that
> * require asynchronous unplug handling.
> * @unplug: unplug callback.
> * Used for device removal with devices that implement
> * asynchronous and synchronous (surprise) removal.
> + * Note: unplug_request and unplug are only called for devices to initiate
> + * unplug of a device hierarchy (e.g. triggered by device_del). For
> + * devices that will be removed along with this device hierarchy only
> + * do_unplug will be called (e.g. to unassign resources).
> */
> typedef struct HotplugHandlerClass {
> /* <private> */
> @@ -63,7 +76,8 @@ typedef struct HotplugHandlerClass {
> /* <public> */
> hotplug_fn pre_plug;
> hotplug_fn plug;
> - void (*post_plug)(HotplugHandler *plug_handler, DeviceState
> *plugged_dev);
> + hotplug_fn_nofail post_plug;
> + hotplug_fn_nofail do_unplug;
> hotplug_fn unplug_request;
> hotplug_fn unplug;
> } HotplugHandlerClass;
> @@ -94,6 +108,14 @@ void hotplug_handler_pre_plug(HotplugHandler
> *plug_handler,
> void hotplug_handler_post_plug(HotplugHandler *plug_handler,
> DeviceState *plugged_dev);
>
> +/**
> + * hotplug_handler_do_unplug:
> + *
> + * Call #HotplugHandlerClass.do_unplug callback of @plug_handler.
> + */
> +void hotplug_handler_do_unplug(HotplugHandler *plug_handler,
> + DeviceState *plugged_dev);
> +
> /**
> * hotplug_handler_unplug_request:
> *
