From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:59776) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QOZ1H-0003ll-4Q for qemu-devel@nongnu.org; Mon, 23 May 2011 13:34:20 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1QOZ1F-0001iU-Lg for qemu-devel@nongnu.org; Mon, 23 May 2011 13:34:19 -0400 Received: from mx1.redhat.com ([209.132.183.28]:24682) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QOZ1F-0001iJ-9g for qemu-devel@nongnu.org; Mon, 23 May 2011 13:34:17 -0400 Received: from int-mx01.intmail.prod.int.phx2.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id p4NHYGF4003188 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Mon, 23 May 2011 13:34:16 -0400 Message-ID: <4DDA9A9C.30300@redhat.com> Date: Mon, 23 May 2011 19:34:20 +0200 From: Hans de Goede MIME-Version: 1.0 References: <1306165234-6999-1-git-send-email-kraxel@redhat.com> In-Reply-To: <1306165234-6999-1-git-send-email-kraxel@redhat.com> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH] usb: cancel async packets on unplug List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Gerd Hoffmann Cc: qemu-devel@nongnu.org Hi, Looks good to me, good way to fix this! Acked-by: Hans de Goede 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 > --- > 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);