Re: [Qemu-devel] [PATCH 1/6] usb: add usb_bus_release function

Re: [Qemu-devel] [PATCH 1/6] usb: add usb_bus_release function

[Gerd Hoffmann]

  Hi,

> +void usb_bus_release(USBBus *bus)
> +{
> +    assert(next_usb_bus > 0);
> +
> +    next_usb_bus--;
> +    QTAILQ_REMOVE(&busses, bus, next);
> +}

That breaks when not hotplugging in last-in-first-out order.  I'd
suggest to simply leave next_usb_bus alone on unplug.  It is only used
for bus numbering.  We'll get holes in our bus numbering then, but we
can't avoid that with hotplug anyway.

cheers,
  Gerd

Re: [Qemu-devel] [PATCH 2/6] usb-ohci: add exit function

[Gerd Hoffmann]

  Hi,

> +static void usb_ohci_exit(PCIDevice *dev)
> +{
> +    OHCIPCIState *ohci = PCI_OHCI(dev);
> +    OHCIState *s = &ohci->state;
> +
> +    memory_region_destroy(&s->mem);
> +
> +    if (!ohci->masterbus) {
> +        usb_bus_release(&s->bus);
> +    }
> +}

This is incomplete.  At minimum you have to care about s->eof_timer.

Same goes for the other host adapters.  There can be timers running and
there might be in-flight usb requests which must be property teared
down, to make sure we don't use-after-free hostadapter state in
callbacks.

cheers,
  Gerd

Re: [Qemu-devel] [PATCH 6/6] usb: tag usb host controller as hotpluggable

[Gerd Hoffmann]

On Fr, 2014-05-30 at 20:22 +0800, arei.gonglei@huawei.com wrote:
> From: Gonglei <arei.gonglei@huawei.com>
> 
> usb host controller should be able to support hotplug/unplug,
> as the same as the other pci devices, which not enable
> multifunction capability.
> 
> BTW, the qemu have not the capability to support
> hotplug mulitfuncition pci devices at present.

I'd like to keep hotplug disabled for the multifunction configurations.
Which means we have to switch that per device instance, not per device
class.  Not sure what the best way to do that is.  mst, suggestions?

cheers,
  Gerd

Re: [Qemu-devel] [PATCH 6/6] usb: tag usb host controller as hotpluggable

[Michael S. Tsirkin]

On Mon, Jun 02, 2014 at 09:58:18AM +0200, Gerd Hoffmann wrote:
> On Fr, 2014-05-30 at 20:22 +0800, arei.gonglei@huawei.com wrote:
> > From: Gonglei <arei.gonglei@huawei.com>
> > 
> > usb host controller should be able to support hotplug/unplug,
> > as the same as the other pci devices, which not enable
> > multifunction capability.
> > 
> > BTW, the qemu have not the capability to support
> > hotplug mulitfuncition pci devices at present.
> 
> I'd like to keep hotplug disabled for the multifunction configurations.
> Which means we have to switch that per device instance, not per device
> class.  Not sure what the best way to do that is.  mst, suggestions?
> 
> cheers,
>   Gerd

Override the "hotpluggable" property in some way,
and teach qdev to check it?

Alternatively, teach hotplug that it can fail?
Again it's a question of
teaching device_set_realized and qdev_unplug
that these can fail.


-- 
MST

Re: [Qemu-devel] [PATCH 1/6] usb: add usb_bus_release function

[Gonglei (Arei)]

> -----Original Message-----
> From: Gerd Hoffmann [mailto:kraxel@redhat.com]
> Sent: Monday, June 02, 2014 3:36 PM
> To: Gonglei (Arei)
> Cc: qemu-devel@nongnu.org; Luonengjun; Huangweidong (C); Huangpeng
> (Peter)
> Subject: Re: [PATCH 1/6] usb: add usb_bus_release function
> 
>   Hi,
> 
> > +void usb_bus_release(USBBus *bus)
> > +{
> > +    assert(next_usb_bus > 0);
> > +
> > +    next_usb_bus--;
> > +    QTAILQ_REMOVE(&busses, bus, next);
> > +}
> 
> That breaks when not hotplugging in last-in-first-out order.  I'd
> suggest to simply leave next_usb_bus alone on unplug.  It is only used
> for bus numbering.  We'll get holes in our bus numbering then, but we
> can't avoid that with hotplug anyway.
> 
Indeed. Thanks!


Best regards,
-Gonglei

Re: [Qemu-devel] [PATCH 2/6] usb-ohci: add exit function

[Gonglei (Arei)]

> -----Original Message-----
> From: Gerd Hoffmann [mailto:kraxel@redhat.com]
> Sent: Monday, June 02, 2014 3:45 PM
> To: Gonglei (Arei)
> Cc: qemu-devel@nongnu.org; Luonengjun; Huangweidong (C); Huangpeng
> (Peter)
> Subject: Re: [PATCH 2/6] usb-ohci: add exit function
> 
>   Hi,
> 
> > +static void usb_ohci_exit(PCIDevice *dev)
> > +{
> > +    OHCIPCIState *ohci = PCI_OHCI(dev);
> > +    OHCIState *s = &ohci->state;
> > +
> > +    memory_region_destroy(&s->mem);
> > +
> > +    if (!ohci->masterbus) {
> > +        usb_bus_release(&s->bus);
> > +    }
> > +}
> 
> This is incomplete.  At minimum you have to care about s->eof_timer.
> 
Agreed. 

> Same goes for the other host adapters.  There can be timers running and
> there might be in-flight usb requests which must be property teared
> down, to make sure we don't use-after-free hostadapter state in
> callbacks.
> 
I will check and complete them. Thanks.


Best regards,
-Gonglei