| Message ID | 1411559299-19042-29-git-send-email-imammedo@redhat.com |
|---|---|
| State | New |
| Headers | show |
> @@ -81,7 +81,7 @@ void usb_bus_new(USBBus *bus, size_t bus_size, > qbus_create_inplace(bus, bus_size, TYPE_USB_BUS, host, NULL); > bus->ops = ops; > bus->busnr = next_usb_bus++; > - bus->qbus.allow_hotplug = 1; /* Yes, we can */ > + qbus_set_hotplug_handler(BUS(bus), host, &error_abort); > QTAILQ_INIT(&bus->free); > QTAILQ_INIT(&bus->used); > QTAILQ_INSERT_TAIL(&busses, bus, next); > diff --git a/hw/usb/dev-smartcard-reader.c b/hw/usb/dev-smartcard-reader.c > index d37ed02..91f7d87 100644 > --- a/hw/usb/dev-smartcard-reader.c > +++ b/hw/usb/dev-smartcard-reader.c > @@ -1313,7 +1313,7 @@ static void ccid_realize(USBDevice *dev, Error **errp) > qbus_create_inplace(&s->bus, sizeof(s->bus), TYPE_CCID_BUS, DEVICE(dev), > NULL); > s->intr = usb_ep_get(dev, USB_TOKEN_IN, CCID_INT_IN_EP); > - s->bus.qbus.allow_hotplug = 1; > + qbus_set_hotplug_handler(BUS(&s->bus), DEVICE(dev), &error_abort); That is the ccid (smartcard) bus, please make it a separate patch. > diff --git a/hw/usb/hcd-ehci-pci.c b/hw/usb/hcd-ehci-pci.c > index 490f2b6..b7990da 100644 > --- a/hw/usb/hcd-ehci-pci.c > +++ b/hw/usb/hcd-ehci-pci.c > @@ -138,6 +138,7 @@ static void ehci_class_init(ObjectClass *klass, void *data) > { > DeviceClass *dc = DEVICE_CLASS(klass); > PCIDeviceClass *k = PCI_DEVICE_CLASS(klass); > + HotplugHandlerClass *hc = HOTPLUG_HANDLER_CLASS(klass); > > k->init = usb_ehci_pci_initfn; > k->exit = usb_ehci_pci_exit; > @@ -145,6 +146,7 @@ static void ehci_class_init(ObjectClass *klass, void *data) > k->config_write = usb_ehci_pci_write_config; > dc->vmsd = &vmstate_ehci_pci; > dc->props = ehci_pci_properties; > + hc->unplug = qdev_simple_device_unplug_cb; > } Can't we do this in usb_bus_new instead of duplicating in every host adapter? cheers, Gerd
Il 24/09/2014 15:00, Gerd Hoffmann ha scritto: >> @@ -81,7 +81,7 @@ void usb_bus_new(USBBus *bus, size_t bus_size, >> qbus_create_inplace(bus, bus_size, TYPE_USB_BUS, host, NULL); >> bus->ops = ops; >> bus->busnr = next_usb_bus++; >> - bus->qbus.allow_hotplug = 1; /* Yes, we can */ >> + qbus_set_hotplug_handler(BUS(bus), host, &error_abort); >> QTAILQ_INIT(&bus->free); >> QTAILQ_INIT(&bus->used); >> QTAILQ_INSERT_TAIL(&busses, bus, next); > >> diff --git a/hw/usb/dev-smartcard-reader.c b/hw/usb/dev-smartcard-reader.c >> index d37ed02..91f7d87 100644 >> --- a/hw/usb/dev-smartcard-reader.c >> +++ b/hw/usb/dev-smartcard-reader.c >> @@ -1313,7 +1313,7 @@ static void ccid_realize(USBDevice *dev, Error **errp) >> qbus_create_inplace(&s->bus, sizeof(s->bus), TYPE_CCID_BUS, DEVICE(dev), >> NULL); >> s->intr = usb_ep_get(dev, USB_TOKEN_IN, CCID_INT_IN_EP); >> - s->bus.qbus.allow_hotplug = 1; >> + qbus_set_hotplug_handler(BUS(&s->bus), DEVICE(dev), &error_abort); > > That is the ccid (smartcard) bus, please make it a separate patch. > >> diff --git a/hw/usb/hcd-ehci-pci.c b/hw/usb/hcd-ehci-pci.c >> index 490f2b6..b7990da 100644 >> --- a/hw/usb/hcd-ehci-pci.c >> +++ b/hw/usb/hcd-ehci-pci.c >> @@ -138,6 +138,7 @@ static void ehci_class_init(ObjectClass *klass, void *data) >> { >> DeviceClass *dc = DEVICE_CLASS(klass); >> PCIDeviceClass *k = PCI_DEVICE_CLASS(klass); >> + HotplugHandlerClass *hc = HOTPLUG_HANDLER_CLASS(klass); >> >> k->init = usb_ehci_pci_initfn; >> k->exit = usb_ehci_pci_exit; >> @@ -145,6 +146,7 @@ static void ehci_class_init(ObjectClass *klass, void *data) >> k->config_write = usb_ehci_pci_write_config; >> dc->vmsd = &vmstate_ehci_pci; >> dc->props = ehci_pci_properties; >> + hc->unplug = qdev_simple_device_unplug_cb; >> } > > Can't we do this in usb_bus_new instead of duplicating in every host > adapter? So you would make TYPE_USB_BUS the hotplug handler itself, instead of the controller? That would make sense since TYPE_USB_BUS already coordinates hotplug/unplug with the HCD via the USBPortOps. Paolo
Hi, > > Can't we do this in usb_bus_new instead of duplicating in every host > > adapter? > > So you would make TYPE_USB_BUS the hotplug handler itself, instead of > the controller? I was more thinking of just setting the callback in common code, but if we can attach the hotplug interface to the usb bus itself not the usb host adapters that would be even better. And it'll probably kill some headache for the companion controller case. cheers, Gerd
On Wed, 24 Sep 2014 15:23:41 +0200 Gerd Hoffmann <kraxel@redhat.com> wrote: > Hi, > > > > Can't we do this in usb_bus_new instead of duplicating in every host > > > adapter? > > > > So you would make TYPE_USB_BUS the hotplug handler itself, instead of > > the controller? > > I was more thinking of just setting the callback in common code, but if > we can attach the hotplug interface to the usb bus itself not the usb > host adapters that would be even better. And it'll probably kill some > headache for the companion controller case. How making bus a HotplugHandler itself will help with companion controller? > > cheers, > Gerd > >
On Wed, 24 Sep 2014 15:00:41 +0200 Gerd Hoffmann <kraxel@redhat.com> wrote: > > @@ -81,7 +81,7 @@ void usb_bus_new(USBBus *bus, size_t bus_size, > > qbus_create_inplace(bus, bus_size, TYPE_USB_BUS, host, NULL); > > bus->ops = ops; > > bus->busnr = next_usb_bus++; > > - bus->qbus.allow_hotplug = 1; /* Yes, we can */ > > + qbus_set_hotplug_handler(BUS(bus), host, &error_abort); > > QTAILQ_INIT(&bus->free); > > QTAILQ_INIT(&bus->used); > > QTAILQ_INSERT_TAIL(&busses, bus, next); > > > diff --git a/hw/usb/dev-smartcard-reader.c b/hw/usb/dev-smartcard-reader.c > > index d37ed02..91f7d87 100644 > > --- a/hw/usb/dev-smartcard-reader.c > > +++ b/hw/usb/dev-smartcard-reader.c > > @@ -1313,7 +1313,7 @@ static void ccid_realize(USBDevice *dev, Error **errp) > > qbus_create_inplace(&s->bus, sizeof(s->bus), TYPE_CCID_BUS, DEVICE(dev), > > NULL); > > s->intr = usb_ep_get(dev, USB_TOKEN_IN, CCID_INT_IN_EP); > > - s->bus.qbus.allow_hotplug = 1; > > + qbus_set_hotplug_handler(BUS(&s->bus), DEVICE(dev), &error_abort); ok > > That is the ccid (smartcard) bus, please make it a separate patch. > > > diff --git a/hw/usb/hcd-ehci-pci.c b/hw/usb/hcd-ehci-pci.c > > index 490f2b6..b7990da 100644 > > --- a/hw/usb/hcd-ehci-pci.c > > +++ b/hw/usb/hcd-ehci-pci.c > > @@ -138,6 +138,7 @@ static void ehci_class_init(ObjectClass *klass, void *data) > > { > > DeviceClass *dc = DEVICE_CLASS(klass); > > PCIDeviceClass *k = PCI_DEVICE_CLASS(klass); > > + HotplugHandlerClass *hc = HOTPLUG_HANDLER_CLASS(klass); > > > > k->init = usb_ehci_pci_initfn; > > k->exit = usb_ehci_pci_exit; > > @@ -145,6 +146,7 @@ static void ehci_class_init(ObjectClass *klass, void *data) > > k->config_write = usb_ehci_pci_write_config; > > dc->vmsd = &vmstate_ehci_pci; > > dc->props = ehci_pci_properties; > > + hc->unplug = qdev_simple_device_unplug_cb; > > } > > Can't we do this in usb_bus_new instead of duplicating in every host > adapter? sure > > cheers, > Gerd > >
On Mi, 2014-09-24 at 17:39 +0200, Igor Mammedov wrote: > On Wed, 24 Sep 2014 15:23:41 +0200 > Gerd Hoffmann <kraxel@redhat.com> wrote: > > > Hi, > > > > > > Can't we do this in usb_bus_new instead of duplicating in every host > > > > adapter? > > > > > > So you would make TYPE_USB_BUS the hotplug handler itself, instead of > > > the controller? > > > > I was more thinking of just setting the callback in common code, but if > > we can attach the hotplug interface to the usb bus itself not the usb > > host adapters that would be even better. And it'll probably kill some > > headache for the companion controller case. > How making bus a HotplugHandler itself will help with companion controller? When uhci acts as ehci companion it registers the ports with ehci and doesn't manage its own usb bus. ehci will call uhci port ops as needed (depending on ehci configuration). When attaching the hotplug interface to the host controller you'll have to explicitly handle the companion case somehow. When attaching the hotplug interface to the usb bus everything should just work. Additional bonus is that you also don't have to touch the host controller code at all then, it should be doable by changing hw/usb/bus.c only. cheers, Gerd
On Thu, 25 Sep 2014 09:50:58 +0200 Gerd Hoffmann <kraxel@redhat.com> wrote: > On Mi, 2014-09-24 at 17:39 +0200, Igor Mammedov wrote: > > On Wed, 24 Sep 2014 15:23:41 +0200 > > Gerd Hoffmann <kraxel@redhat.com> wrote: > > > > > Hi, > > > > > > > > Can't we do this in usb_bus_new instead of duplicating in every host > > > > > adapter? > > > > > > > > So you would make TYPE_USB_BUS the hotplug handler itself, instead of > > > > the controller? > > > > > > I was more thinking of just setting the callback in common code, but if > > > we can attach the hotplug interface to the usb bus itself not the usb > > > host adapters that would be even better. And it'll probably kill some > > > headache for the companion controller case. > > How making bus a HotplugHandler itself will help with companion controller? > > When uhci acts as ehci companion it registers the ports with ehci and > doesn't manage its own usb bus. ehci will call uhci port ops as needed > (depending on ehci configuration). > > When attaching the hotplug interface to the host controller you'll have > to explicitly handle the companion case somehow. > > When attaching the hotplug interface to the usb bus everything should > just work. Additional bonus is that you also don't have to touch the > host controller code at all then, it should be doable by changing > hw/usb/bus.c only. hotplug-handler.[plug|unplug] callbacks are class wide, so if USB bus might ever need to have different callbacks depending on host it might not work. But since so far it uses the only qdev_simple_device_unplug_cb(), having BUS as hotplug-handler should work too. > > cheers, > Gerd > >
Il 25/09/2014 12:55, Igor Mammedov ha scritto: > hotplug-handler.[plug|unplug] callbacks are class wide, so if > USB bus might ever need to have different callbacks depending on > host it might not work. > > But since so far it uses the only qdev_simple_device_unplug_cb(), > having BUS as hotplug-handler should work too. Yeah, in effect the USB bus is using attach/detach as the controller-specific part of the hotplug handler. Perhaps the same can work for SCSI as well? I'm waiting for v2. :) Paolo
On Thu, 25 Sep 2014 14:47:31 +0200 Paolo Bonzini <pbonzini@redhat.com> wrote: > Il 25/09/2014 12:55, Igor Mammedov ha scritto: > > hotplug-handler.[plug|unplug] callbacks are class wide, so if > > USB bus might ever need to have different callbacks depending on > > host it might not work. > > > > But since so far it uses the only qdev_simple_device_unplug_cb(), > > having BUS as hotplug-handler should work too. > > Yeah, in effect the USB bus is using attach/detach as the > controller-specific part of the hotplug handler. > > Perhaps the same can work for SCSI as well? I'm waiting for v2. :) Hopefully it should be ready by the end of this day. > > Paolo
diff --git a/hw/usb/bus.c b/hw/usb/bus.c index da1eba9..16c2bb3 100644 --- a/hw/usb/bus.c +++ b/hw/usb/bus.c @@ -81,7 +81,7 @@ void usb_bus_new(USBBus *bus, size_t bus_size, qbus_create_inplace(bus, bus_size, TYPE_USB_BUS, host, NULL); bus->ops = ops; bus->busnr = next_usb_bus++; - bus->qbus.allow_hotplug = 1; /* Yes, we can */ + qbus_set_hotplug_handler(BUS(bus), host, &error_abort); QTAILQ_INIT(&bus->free); QTAILQ_INIT(&bus->used); QTAILQ_INSERT_TAIL(&busses, bus, next); @@ -701,7 +701,6 @@ static void usb_device_class_init(ObjectClass *klass, void *data) { DeviceClass *k = DEVICE_CLASS(klass); k->bus_type = TYPE_USB_BUS; - k->unplug = qdev_simple_unplug_cb; k->realize = usb_qdev_realize; k->unrealize = usb_qdev_unrealize; k->props = usb_props; diff --git a/hw/usb/dev-smartcard-reader.c b/hw/usb/dev-smartcard-reader.c index d37ed02..91f7d87 100644 --- a/hw/usb/dev-smartcard-reader.c +++ b/hw/usb/dev-smartcard-reader.c @@ -1313,7 +1313,7 @@ static void ccid_realize(USBDevice *dev, Error **errp) qbus_create_inplace(&s->bus, sizeof(s->bus), TYPE_CCID_BUS, DEVICE(dev), NULL); s->intr = usb_ep_get(dev, USB_TOKEN_IN, CCID_INT_IN_EP); - s->bus.qbus.allow_hotplug = 1; + qbus_set_hotplug_handler(BUS(&s->bus), DEVICE(dev), &error_abort); s->card = NULL; s->migration_state = MIGRATION_NONE; s->migration_target_ip = 0; @@ -1439,6 +1439,7 @@ static void ccid_class_initfn(ObjectClass *klass, void *data) { DeviceClass *dc = DEVICE_CLASS(klass); USBDeviceClass *uc = USB_DEVICE_CLASS(klass); + HotplugHandlerClass *hc = HOTPLUG_HANDLER_CLASS(klass); uc->realize = ccid_realize; uc->product_desc = "QEMU USB CCID"; @@ -1451,6 +1452,7 @@ static void ccid_class_initfn(ObjectClass *klass, void *data) dc->vmsd = &ccid_vmstate; dc->props = ccid_properties; set_bit(DEVICE_CATEGORY_INPUT, dc->categories); + hc->unplug = qdev_simple_device_unplug_cb; } static const TypeInfo ccid_info = { @@ -1458,6 +1460,10 @@ static const TypeInfo ccid_info = { .parent = TYPE_USB_DEVICE, .instance_size = sizeof(USBCCIDState), .class_init = ccid_class_initfn, + .interfaces = (InterfaceInfo[]) { + { TYPE_HOTPLUG_HANDLER }, + { } + } }; static void ccid_card_class_init(ObjectClass *klass, void *data) diff --git a/hw/usb/hcd-ehci-pci.c b/hw/usb/hcd-ehci-pci.c index 490f2b6..b7990da 100644 --- a/hw/usb/hcd-ehci-pci.c +++ b/hw/usb/hcd-ehci-pci.c @@ -138,6 +138,7 @@ static void ehci_class_init(ObjectClass *klass, void *data) { DeviceClass *dc = DEVICE_CLASS(klass); PCIDeviceClass *k = PCI_DEVICE_CLASS(klass); + HotplugHandlerClass *hc = HOTPLUG_HANDLER_CLASS(klass); k->init = usb_ehci_pci_initfn; k->exit = usb_ehci_pci_exit; @@ -145,6 +146,7 @@ static void ehci_class_init(ObjectClass *klass, void *data) k->config_write = usb_ehci_pci_write_config; dc->vmsd = &vmstate_ehci_pci; dc->props = ehci_pci_properties; + hc->unplug = qdev_simple_device_unplug_cb; } static const TypeInfo ehci_pci_type_info = { @@ -154,6 +156,10 @@ static const TypeInfo ehci_pci_type_info = { .instance_init = usb_ehci_pci_init, .abstract = true, .class_init = ehci_class_init, + .interfaces = (InterfaceInfo[]) { + { TYPE_HOTPLUG_HANDLER }, + { } + } }; static void ehci_data_class_init(ObjectClass *klass, void *data) diff --git a/hw/usb/hcd-ehci-sysbus.c b/hw/usb/hcd-ehci-sysbus.c index 19ed2c2..25eb885 100644 --- a/hw/usb/hcd-ehci-sysbus.c +++ b/hw/usb/hcd-ehci-sysbus.c @@ -63,6 +63,7 @@ static void ehci_sysbus_class_init(ObjectClass *klass, void *data) { DeviceClass *dc = DEVICE_CLASS(klass); SysBusEHCIClass *sec = SYS_BUS_EHCI_CLASS(klass); + HotplugHandlerClass *hc = HOTPLUG_HANDLER_CLASS(klass); sec->portscbase = 0x44; sec->portnr = NB_PORTS; @@ -71,6 +72,7 @@ static void ehci_sysbus_class_init(ObjectClass *klass, void *data) dc->vmsd = &vmstate_ehci_sysbus; dc->props = ehci_sysbus_properties; set_bit(DEVICE_CATEGORY_USB, dc->categories); + hc->unplug = qdev_simple_device_unplug_cb; } static const TypeInfo ehci_type_info = { @@ -81,6 +83,10 @@ static const TypeInfo ehci_type_info = { .abstract = true, .class_init = ehci_sysbus_class_init, .class_size = sizeof(SysBusEHCIClass), + .interfaces = (InterfaceInfo[]) { + { TYPE_HOTPLUG_HANDLER }, + { } + } }; static void ehci_xlnx_class_init(ObjectClass *oc, void *data) diff --git a/hw/usb/hcd-ohci.c b/hw/usb/hcd-ohci.c index 7ea871d..5b2552f 100644 --- a/hw/usb/hcd-ohci.c +++ b/hw/usb/hcd-ohci.c @@ -2083,6 +2083,7 @@ static void ohci_pci_class_init(ObjectClass *klass, void *data) { DeviceClass *dc = DEVICE_CLASS(klass); PCIDeviceClass *k = PCI_DEVICE_CLASS(klass); + HotplugHandlerClass *hc = HOTPLUG_HANDLER_CLASS(klass); k->init = usb_ohci_initfn_pci; k->exit = usb_ohci_exit; @@ -2094,6 +2095,7 @@ static void ohci_pci_class_init(ObjectClass *klass, void *data) dc->props = ohci_pci_properties; dc->hotpluggable = false; dc->vmsd = &vmstate_ohci; + hc->unplug = qdev_simple_device_unplug_cb; } static const TypeInfo ohci_pci_info = { @@ -2101,6 +2103,10 @@ static const TypeInfo ohci_pci_info = { .parent = TYPE_PCI_DEVICE, .instance_size = sizeof(OHCIPCIState), .class_init = ohci_pci_class_init, + .interfaces = (InterfaceInfo[]) { + { TYPE_HOTPLUG_HANDLER }, + { } + } }; static Property ohci_sysbus_properties[] = { @@ -2112,11 +2118,13 @@ static Property ohci_sysbus_properties[] = { static void ohci_sysbus_class_init(ObjectClass *klass, void *data) { DeviceClass *dc = DEVICE_CLASS(klass); + HotplugHandlerClass *hc = HOTPLUG_HANDLER_CLASS(klass); dc->realize = ohci_realize_pxa; set_bit(DEVICE_CATEGORY_USB, dc->categories); dc->desc = "OHCI USB Controller"; dc->props = ohci_sysbus_properties; + hc->unplug = qdev_simple_device_unplug_cb; } static const TypeInfo ohci_sysbus_info = { @@ -2124,6 +2132,10 @@ static const TypeInfo ohci_sysbus_info = { .parent = TYPE_SYS_BUS_DEVICE, .instance_size = sizeof(OHCISysBusState), .class_init = ohci_sysbus_class_init, + .interfaces = (InterfaceInfo[]) { + { TYPE_HOTPLUG_HANDLER }, + { } + } }; static void ohci_register_types(void) diff --git a/hw/usb/hcd-uhci.c b/hw/usb/hcd-uhci.c index 5b88f30..76f6deb 100644 --- a/hw/usb/hcd-uhci.c +++ b/hw/usb/hcd-uhci.c @@ -1298,6 +1298,7 @@ static void uhci_class_init(ObjectClass *klass, void *data) PCIDeviceClass *k = PCI_DEVICE_CLASS(klass); UHCIPCIDeviceClass *u = container_of(k, UHCIPCIDeviceClass, parent_class); UHCIInfo *info = data; + HotplugHandlerClass *hc = HOTPLUG_HANDLER_CLASS(klass); k->init = info->initfn ? info->initfn : usb_uhci_common_initfn; k->exit = info->unplug ? usb_uhci_exit : NULL; @@ -1315,6 +1316,7 @@ static void uhci_class_init(ObjectClass *klass, void *data) } set_bit(DEVICE_CATEGORY_USB, dc->categories); u->info = *info; + hc->unplug = qdev_simple_device_unplug_cb; } static UHCIInfo uhci_info[] = { @@ -1392,6 +1394,10 @@ static void uhci_register_types(void) .instance_size = sizeof(UHCIState), .class_size = sizeof(UHCIPCIDeviceClass), .class_init = uhci_class_init, + .interfaces = (InterfaceInfo[]) { + { TYPE_HOTPLUG_HANDLER }, + { } + } }; int i; diff --git a/hw/usb/hcd-xhci.c b/hw/usb/hcd-xhci.c index c556367..5ef74ee 100644 --- a/hw/usb/hcd-xhci.c +++ b/hw/usb/hcd-xhci.c @@ -3870,6 +3870,7 @@ static void xhci_class_init(ObjectClass *klass, void *data) { PCIDeviceClass *k = PCI_DEVICE_CLASS(klass); DeviceClass *dc = DEVICE_CLASS(klass); + HotplugHandlerClass *hc = HOTPLUG_HANDLER_CLASS(klass); dc->vmsd = &vmstate_xhci; dc->props = xhci_properties; @@ -3882,6 +3883,7 @@ static void xhci_class_init(ObjectClass *klass, void *data) k->class_id = PCI_CLASS_SERIAL_USB; k->revision = 0x03; k->is_express = 1; + hc->unplug = qdev_simple_device_unplug_cb; } static const TypeInfo xhci_info = { @@ -3889,6 +3891,10 @@ static const TypeInfo xhci_info = { .parent = TYPE_PCI_DEVICE, .instance_size = sizeof(XHCIState), .class_init = xhci_class_init, + .interfaces = (InterfaceInfo[]) { + { TYPE_HOTPLUG_HANDLER }, + { } + } }; static void xhci_register_types(void)
Signed-off-by: Igor Mammedov <imammedo@redhat.com> --- hw/usb/bus.c | 3 +-- hw/usb/dev-smartcard-reader.c | 8 +++++++- hw/usb/hcd-ehci-pci.c | 6 ++++++ hw/usb/hcd-ehci-sysbus.c | 6 ++++++ hw/usb/hcd-ohci.c | 12 ++++++++++++ hw/usb/hcd-uhci.c | 6 ++++++ hw/usb/hcd-xhci.c | 6 ++++++ 7 files changed, 44 insertions(+), 3 deletions(-)