Re: [Qemu-devel] [PATCH 23/35] acpi:piix4: make plug/unlug callbacks generic

[Igor Mammedov]

On Mon, 7 Apr 2014 14:32:41 +0300
"Michael S. Tsirkin" <address@hidden> wrote:

> On Fri, Apr 04, 2014 at 03:36:48PM +0200, Igor Mammedov wrote:
> > ... and report error if plugged in device is not supported.
> > Later generic callbacks will be used by memory hotplug.
> > 
> > Signed-off-by: Igor Mammedov <address@hidden>
> 
> 
> OK in that case, how about teaching all hotplug callbacks about this?
> 
> There are two ATM:
> shpc_device_hotplug_cb
> pcie_cap_slot_hotplug_cb
> 
> Teach them both to fail gracefully if they get
> an object that is not a pci device.
> 
> Afterwards, simply iterate over all objects of type
> TYPE_HOTPLUG_HANDLER
> and look for one that will accept your object.
Then you would never know if any hotplug handler has actually
handled event.

I think hotplug handler should return error if unsupported
device passed in rather than ignore it. It makes catching
wiring errors easier.
Dropping error so that we could not care which hotplug handler
should be notified, looks like a wrong direction and makes
system more fragile.

It shouldn't be up to consumer to determine that event should
be routed to it, but rather by external routing that knows
what and when should be notified.

> 
> 
> > ---
> >  hw/acpi/piix4.c | 31 ++++++++++++++++++++++---------
> >  1 file changed, 22 insertions(+), 9 deletions(-)
> > 
> > diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
> > index 67dc075..4341f82 100644
> > --- a/hw/acpi/piix4.c
> > +++ b/hw/acpi/piix4.c
> > @@ -310,19 +310,32 @@ static void piix4_pm_powerdown_req(Notifier *n, void 
> > *opaque)
> >      acpi_pm1_evt_power_down(&s->ar);
> >  }
> >  
> > -static void piix4_pci_device_plug_cb(HotplugHandler *hotplug_dev,
> > -                                     DeviceState *dev, Error **errp)
> > +static void piix4_device_plug_cb(HotplugHandler *hotplug_dev,
> > +                                 DeviceState *dev, Error **errp)
> >  {
> >      PIIX4PMState *s = PIIX4_PM(hotplug_dev);
> > -    acpi_pcihp_device_plug_cb(&s->ar, s->irq, &s->acpi_pci_hotplug, dev, 
> > errp);
> > +
> > +    if (object_dynamic_cast(OBJECT(dev), TYPE_PCI_DEVICE)) {
> > +        acpi_pcihp_device_plug_cb(&s->ar, s->irq, &s->acpi_pci_hotplug, 
> > dev,
> > +                                  errp);
> > +    } else {
> > +        error_setg(errp, "acpi: device plug request for not supported 
> > device"
> > +                   " type: %s", object_get_typename(OBJECT(dev)));
> > +    }
> >  }
> >  
> > -static void piix4_pci_device_unplug_cb(HotplugHandler *hotplug_dev,
> > -                                       DeviceState *dev, Error **errp)
> > +static void piix4_device_unplug_cb(HotplugHandler *hotplug_dev,
> > +                                   DeviceState *dev, Error **errp)
> >  {
> >      PIIX4PMState *s = PIIX4_PM(hotplug_dev);
> > -    acpi_pcihp_device_unplug_cb(&s->ar, s->irq, &s->acpi_pci_hotplug, dev,
> > -                                errp);
> > +
> > +    if (object_dynamic_cast(OBJECT(dev), TYPE_PCI_DEVICE)) {
> > +        acpi_pcihp_device_unplug_cb(&s->ar, s->irq, &s->acpi_pci_hotplug, 
> > dev,
> > +                                    errp);
> > +    } else {
> > +        error_setg(errp, "acpi: device unplug request for not supported 
> > device"
> > +                   " type: %s", object_get_typename(OBJECT(dev)));
> > +    }
> >  }
> >  
> >  static void piix4_update_bus_hotplug(PCIBus *pci_bus, void *opaque)
> > @@ -553,8 +566,8 @@ static void piix4_pm_class_init(ObjectClass *klass, 
> > void *data)
> >       */
> >      dc->cannot_instantiate_with_device_add_yet = true;
> >      dc->hotpluggable = false;
> > -    hc->plug = piix4_pci_device_plug_cb;
> > -    hc->unplug = piix4_pci_device_unplug_cb;
> > +    hc->plug = piix4_device_plug_cb;
> > +    hc->unplug = piix4_device_unplug_cb;
> >  }
> >  
> >  static const TypeInfo piix4_pm_info = {
> > -- 
> > 1.9.0