Re: [Qemu-devel] [PATCH] usb: cancel async packets on unplug

[Hans de Goede <hdegoede@redhat.com>]

Hi,

Looks good to me, good way to fix this!

Acked-by: Hans de Goede <hdegoede@redhat.com>

Regards,

Hans

On 05/23/2011 05:40 PM, Gerd Hoffmann wrote:
> This patch adds USBBusOps struct with (for now) only a single callback
> which is called when a device is about to be destroyed.  The USB Host
> adapters are implementing this callback and use it to cancel any async
> requests which might be in flight before the device actually goes away.
>
> Signed-off-by: Gerd Hoffmann<kraxel@redhat.com>
> ---
>   hw/usb-bus.c  |    5 ++++-
>   hw/usb-ehci.c |   25 ++++++++++++++++++++++++-
>   hw/usb-musb.c |   23 ++++++++++++++++++++++-
>   hw/usb-ohci.c |   16 +++++++++++++++-
>   hw/usb-uhci.c |   26 +++++++++++++++++++++++++-
>   hw/usb.h      |    8 +++++++-
>   6 files changed, 97 insertions(+), 6 deletions(-)
>
> diff --git a/hw/usb-bus.c b/hw/usb-bus.c
> index abc7e61..874c253 100644
> --- a/hw/usb-bus.c
> +++ b/hw/usb-bus.c
> @@ -39,9 +39,10 @@ const VMStateDescription vmstate_usb_device = {
>       }
>   };
>
> -void usb_bus_new(USBBus *bus, DeviceState *host)
> +void usb_bus_new(USBBus *bus, USBBusOps *ops, DeviceState *host)
>   {
>       qbus_create_inplace(&bus->qbus,&usb_bus_info, host, NULL);
> +    bus->ops = ops;
>       bus->busnr = next_usb_bus++;
>       bus->qbus.allow_hotplug = 1; /* Yes, we can */
>       QTAILQ_INIT(&bus->free);
> @@ -81,8 +82,10 @@ static int usb_qdev_init(DeviceState *qdev, DeviceInfo *base)
>   static int usb_qdev_exit(DeviceState *qdev)
>   {
>       USBDevice *dev = DO_UPCAST(USBDevice, qdev, qdev);
> +    USBBus *bus = usb_bus_from_device(dev);
>
>       usb_device_detach(dev);
> +    bus->ops->device_destroy(bus, dev);
>       if (dev->info->handle_destroy) {
>           dev->info->handle_destroy(dev);
>       }
> diff --git a/hw/usb-ehci.c b/hw/usb-ehci.c
> index ac9763f..e8bcb93 100644
> --- a/hw/usb-ehci.c
> +++ b/hw/usb-ehci.c
> @@ -687,6 +687,18 @@ static void ehci_queues_rip_unused(EHCIState *ehci)
>       }
>   }
>
> +static void ehci_queues_rip_device(EHCIState *ehci, USBDevice *dev)
> +{
> +    EHCIQueue *q, *tmp;
> +
> +    QTAILQ_FOREACH_SAFE(q,&ehci->queues, next, tmp) {
> +        if (q->packet.owner != dev) {
> +            continue;
> +        }
> +        ehci_free_queue(q);
> +    }
> +}
> +
>   static void ehci_queues_rip_all(EHCIState *ehci)
>   {
>       EHCIQueue *q, *tmp;
> @@ -2087,6 +2099,13 @@ static void ehci_map(PCIDevice *pci_dev, int region_num,
>       cpu_register_physical_memory(addr, size, s->mem);
>   }
>
> +static void ehci_device_destroy(USBBus *bus, USBDevice *dev)
> +{
> +    EHCIState *s = container_of(bus, EHCIState, bus);
> +
> +    ehci_queues_rip_device(s, dev);
> +}
> +
>   static int usb_ehci_initfn(PCIDevice *dev);
>
>   static USBPortOps ehci_port_ops = {
> @@ -2095,6 +2114,10 @@ static USBPortOps ehci_port_ops = {
>       .complete = ehci_async_complete_packet,
>   };
>
> +static USBBusOps ehci_bus_ops = {
> +    .device_destroy = ehci_device_destroy,
> +};
> +
>   static PCIDeviceInfo ehci_info = {
>       .qdev.name    = "usb-ehci",
>       .qdev.size    = sizeof(EHCIState),
> @@ -2157,7 +2180,7 @@ static int usb_ehci_initfn(PCIDevice *dev)
>
>       s->irq = s->dev.irq[3];
>
> -    usb_bus_new(&s->bus,&s->dev.qdev);
> +    usb_bus_new(&s->bus,&ehci_bus_ops,&s->dev.qdev);
>       for(i = 0; i<  NB_PORTS; i++) {
>           usb_register_port(&s->bus,&s->ports[i], s, i,&ehci_port_ops,
>                             USB_SPEED_MASK_HIGH);
> diff --git a/hw/usb-musb.c b/hw/usb-musb.c
> index 6037193..21f35af 100644
> --- a/hw/usb-musb.c
> +++ b/hw/usb-musb.c
> @@ -262,6 +262,7 @@
>   static void musb_attach(USBPort *port);
>   static void musb_detach(USBPort *port);
>   static void musb_schedule_cb(USBDevice *dev, USBPacket *p);
> +static void musb_device_destroy(USBBus *bus, USBDevice *dev);
>
>   static USBPortOps musb_port_ops = {
>       .attach = musb_attach,
> @@ -269,6 +270,10 @@ static USBPortOps musb_port_ops = {
>       .complete = musb_schedule_cb,
>   };
>
> +static USBBusOps musb_bus_ops = {
> +    .device_destroy = musb_device_destroy,
> +};
> +
>   typedef struct MUSBPacket MUSBPacket;
>   typedef struct MUSBEndPoint MUSBEndPoint;
>
> @@ -361,7 +366,7 @@ struct MUSBState *musb_init(qemu_irq *irqs)
>           s->ep[i].epnum = i;
>       }
>
> -    usb_bus_new(&s->bus, NULL /* FIXME */);
> +    usb_bus_new(&s->bus,&musb_bus_ops, NULL /* FIXME */);
>       usb_register_port(&s->bus,&s->port, s, 0,&musb_port_ops,
>                         USB_SPEED_MASK_LOW | USB_SPEED_MASK_FULL);
>       usb_port_location(&s->port, NULL, 1);
> @@ -778,6 +783,22 @@ static void musb_rx_packet_complete(USBPacket *packey, void *opaque)
>       musb_rx_intr_set(s, epnum, 1);
>   }
>
> +static void musb_device_destroy(USBBus *bus, USBDevice *dev)
> +{
> +    MUSBState *s = container_of(bus, MUSBState, bus);
> +    int ep, dir;
> +
> +    for (ep = 0; ep<  16; ep++) {
> +        for (dir = 0; dir<  2; dir++) {
> +            if (s->ep[ep].packey[dir].p.owner != dev) {
> +                continue;
> +            }
> +            usb_cancel_packet(&s->ep[ep].packey[dir].p);
> +            /* status updates needed here? */
> +        }
> +    }
> +}
> +
>   static void musb_tx_rdy(MUSBState *s, int epnum)
>   {
>       MUSBEndPoint *ep = s->ep + epnum;
> diff --git a/hw/usb-ohci.c b/hw/usb-ohci.c
> index 8b966f7..401045a 100644
> --- a/hw/usb-ohci.c
> +++ b/hw/usb-ohci.c
> @@ -1644,6 +1644,16 @@ static void ohci_mem_write(void *ptr, target_phys_addr_t addr, uint32_t val)
>       }
>   }
>
> +static void ohci_device_destroy(USBBus *bus, USBDevice *dev)
> +{
> +    OHCIState *ohci = container_of(bus, OHCIState, bus);
> +
> +    if (ohci->async_td&&  ohci->usb_packet.owner == dev) {
> +        usb_cancel_packet(&ohci->usb_packet);
> +        ohci->async_td = 0;
> +    }
> +}
> +
>   /* Only dword reads are defined on OHCI register space */
>   static CPUReadMemoryFunc * const ohci_readfn[3]={
>       ohci_mem_read,
> @@ -1664,6 +1674,10 @@ static USBPortOps ohci_port_ops = {
>       .complete = ohci_async_complete_packet,
>   };
>
> +static USBBusOps ohci_bus_ops = {
> +    .device_destroy = ohci_device_destroy,
> +};
> +
>   static void usb_ohci_init(OHCIState *ohci, DeviceState *dev,
>                             int num_ports, uint32_t localmem_base)
>   {
> @@ -1691,7 +1705,7 @@ static void usb_ohci_init(OHCIState *ohci, DeviceState *dev,
>
>       ohci->name = dev->info->name;
>
> -    usb_bus_new(&ohci->bus, dev);
> +    usb_bus_new(&ohci->bus,&ohci_bus_ops, dev);
>       ohci->num_ports = num_ports;
>       for (i = 0; i<  num_ports; i++) {
>           usb_register_port(&ohci->bus,&ohci->rhport[i].port, ohci, i,&ohci_port_ops,
> diff --git a/hw/usb-uhci.c b/hw/usb-uhci.c
> index bc31850..e9993a7 100644
> --- a/hw/usb-uhci.c
> +++ b/hw/usb-uhci.c
> @@ -234,6 +234,19 @@ static void uhci_async_validate_end(UHCIState *s)
>       }
>   }
>
> +static void uhci_async_cancel_device(UHCIState *s, USBDevice *dev)
> +{
> +    UHCIAsync *curr, *n;
> +
> +    QTAILQ_FOREACH_SAFE(curr,&s->async_pending, next, n) {
> +        if (curr->packet.owner != dev) {
> +            continue;
> +        }
> +        uhci_async_unlink(s, curr);
> +        uhci_async_cancel(s, curr);
> +    }
> +}
> +
>   static void uhci_async_cancel_all(UHCIState *s)
>   {
>       UHCIAsync *curr, *n;
> @@ -1097,6 +1110,13 @@ static void uhci_map(PCIDevice *pci_dev, int region_num,
>       register_ioport_read(addr, 32, 1, uhci_ioport_readb, s);
>   }
>
> +static void uhci_device_destroy(USBBus *bus, USBDevice *dev)
> +{
> +    UHCIState *s = container_of(bus, UHCIState, bus);
> +
> +    uhci_async_cancel_device(s, dev);
> +}
> +
>   static USBPortOps uhci_port_ops = {
>       .attach = uhci_attach,
>       .detach = uhci_detach,
> @@ -1104,6 +1124,10 @@ static USBPortOps uhci_port_ops = {
>       .complete = uhci_async_complete,
>   };
>
> +static USBBusOps uhci_bus_ops = {
> +    .device_destroy = uhci_device_destroy,
> +};
> +
>   static int usb_uhci_common_initfn(UHCIState *s)
>   {
>       uint8_t *pci_conf = s->dev.config;
> @@ -1116,7 +1140,7 @@ static int usb_uhci_common_initfn(UHCIState *s)
>       pci_conf[PCI_INTERRUPT_PIN] = 4; // interrupt pin 3
>       pci_conf[0x60] = 0x10; // release number
>
> -    usb_bus_new(&s->bus,&s->dev.qdev);
> +    usb_bus_new(&s->bus,&uhci_bus_ops,&s->dev.qdev);
>       for(i = 0; i<  NB_PORTS; i++) {
>           usb_register_port(&s->bus,&s->ports[i].port, s, i,&uhci_port_ops,
>                             USB_SPEED_MASK_LOW | USB_SPEED_MASK_FULL);
> diff --git a/hw/usb.h b/hw/usb.h
> index 0fa86a3..097d789 100644
> --- a/hw/usb.h
> +++ b/hw/usb.h
> @@ -139,6 +139,7 @@
>   #define USB_ENDPOINT_XFER_INT		3
>
>   typedef struct USBBus USBBus;
> +typedef struct USBBusOps USBBusOps;
>   typedef struct USBPort USBPort;
>   typedef struct USBDevice USBDevice;
>   typedef struct USBDeviceInfo USBDeviceInfo;
> @@ -330,6 +331,7 @@ void musb_set_size(MUSBState *s, int epnum, int size, int is_tx);
>
>   struct USBBus {
>       BusState qbus;
> +    USBBusOps *ops;
>       int busnr;
>       int nfree;
>       int nused;
> @@ -338,7 +340,11 @@ struct USBBus {
>       QTAILQ_ENTRY(USBBus) next;
>   };
>
> -void usb_bus_new(USBBus *bus, DeviceState *host);
> +struct USBBusOps {
> +    void (*device_destroy)(USBBus *bus, USBDevice *dev);
> +};
> +
> +void usb_bus_new(USBBus *bus, USBBusOps *ops, DeviceState *host);
>   USBBus *usb_bus_find(int busnr);
>   void usb_qdev_register(USBDeviceInfo *info);
>   void usb_qdev_register_many(USBDeviceInfo *info);