acrn-dev@lists.projectacrn.org | [PATCH 5/6] DM USB: xHCI: change flow of creation of virtual USB device

Home Messages Hashtags Subgroups

Date Date 1 - 3 of 3

[PATCH 5/6] DM USB: xHCI: change flow of creation of virtual USB device

Wu, Xiaoguang #10406 The xHCI emulation greatly depends on the user space library libusb which is based on the usbfs module in Linux kernel. The libusb will bind usbfs to physical USB device which makes hardware control over libusb in user space possible. The pci_xhci_dev_create is called in pci_xhci_native_usb_dev_conn_cb which is a callback function triggered by physical USB device plugging. This function will bind the physical USB device to usbfs in SOS, which we depend to create the communication between UOS xHCI driver with physical USB device. This design will fail if the disconnection happened in the SOS, which will bind class driver to the physical USB device instead of usbfs, hence the libusb device handle in DM is invalid. Currently when the SOS do the resuming from S3, there are some uncertain bugs which unbind the usbfs for the USB devices, so the DM will lose control and can't continue emulation. To fix this issue, place the pci_xhci_dev_create in the function pci_xhci_cmd_enable_slot. According to the xHCI spec 4.5.3 Figure 10, the UOS always send Enable Slot command when a device is attached or recovered from errors (by Disable Slot command). So every time the SOS can't resuming normally or some unexpected disconnection happens, this desigen will always survive by Disable Slot and Enable Slot command series from UOS xHCI driver. Signed-off-by: Xiaoguang Wu <xiaoguang.wu@...> Reviewed-by: Liang Yang <liang3.yang@...> --- devicemodel/hw/pci/xhci.c \| 220 +++++++++++++++++++++++++++++++++------------- 1 file changed, 158 insertions(+), 62 deletions(-) diff --git a/devicemodel/hw/pci/xhci.c b/devicemodel/hw/pci/xhci.c index 2df7ce3..579021a 100644 --- a/devicemodel/hw/pci/xhci.c +++ b/devicemodel/hw/pci/xhci.c @@ -383,6 +383,7 @@ struct pci_xhci_vdev { * is stored in native_assign_ports[x][y]. / int8_t native_assign_ports[USB_NATIVE_NUM_BUS]; + struct usb_native_devinfo native_assign_info[USB_NATIVE_NUM_BUS]; struct timespec mf_prev_time; / previous time of accessing MFINDEX / }; @@ -475,36 +476,21 @@ static struct pci_xhci_option_elem xhci_option_table[] = { static int pci_xhci_native_usb_dev_conn_cb(void hci_data, void dev_data) { - struct pci_xhci_dev_emu de; struct pci_xhci_vdev xdev; - struct usb_devemu ue; struct usb_native_devinfo di; int port_start, port_end; - int slot_start, slot_end; - int port, slot; - void ud; + int port; int intr = 1; + int i, j; xdev = hci_data; - di = dev_data; assert(xdev); assert(dev_data); assert(xdev->devices); assert(xdev->slots); - de = pci_xhci_dev_create(xdev, di->priv_data); - if (!de) { - UPRINTF(LFTL, "fail to create device\r\n"); - return -1; - } - - /* find free port and slot for the new usb device / - ud = de->dev_instance; - ue = de->dev_ue; - - assert(ud); - assert(ue); + di = dev_data; / print physical information about new device / UPRINTF(LDBG, "%04x:%04x %d-%d connecting.\r\n", @@ -517,43 +503,53 @@ pci_xhci_native_usb_dev_conn_cb(void hci_data, void dev_data) goto errout; } + assert(xdev->native_assign_info[di->bus]); UPRINTF(LDBG, "%04x:%04x %d-%d belong to this vm.\r\n", di->vid, di->pid, di->bus, di->port); - if (di->bcd < 0x300) + if (di->bcd < 0x300) { port_start = xdev->usb2_port_start; - else + port_end = port_start + (XHCI_MAX_DEVS / 2); + } else { port_start = xdev->usb3_port_start; - - slot_start = 1; - port_end = port_start + (XHCI_MAX_DEVS / 2); - slot_end = XHCI_MAX_SLOTS; + port_end = port_start + (XHCI_MAX_DEVS / 2); + } / find free port / - for (port = port_start; port < port_end; port++) - if (!xdev->devices[port]) - break; + / FIXME: + * Will find a decent way to deal with the relationship among Slot, + * Port and Device. + / + for (port = port_start; port < port_end; port++) { + if (xdev->devices[port]) + continue; + + for (i = 0; i < USB_NATIVE_NUM_BUS; ++i) { + if (!xdev->native_assign_ports[i]) + continue; + + for (j = 0; j < USB_NATIVE_NUM_PORT; ++j) + if (xdev->native_assign_ports[i][j] == port) + break; - / find free slot / - for (slot = slot_start; slot < slot_end; slot++) - if (!xdev->slots[slot]) + if (j < USB_NATIVE_NUM_PORT) + break; + } + if (i >= USB_NATIVE_NUM_BUS) break; + } - if (port >= port_end \|\| slot >= slot_end) { - UPRINTF(LFTL, "no free resource: port %d slot %d\r\n", - port, slot); + if (port >= port_end) { + UPRINTF(LFTL, "no free virtual port for native device %d-%d" + "\r\n", di->bus, di->port); goto errout; } - / use index of devices as port number / - xdev->devices[port] = de; - xdev->slots[slot] = de; - xdev->ndevices++; + UPRINTF(LDBG, "%X:%X %d-%d is attached to virtual port %d.\r\n", + di->vid, di->pid, di->bus, di->port, port); - pci_xhci_reset_slot(xdev, slot); - UPRINTF(LDBG, "%X:%X %d-%d locates in slot %d port %d.\r\n", - di->vid, di->pid, di->bus, di->port, - slot, port); + xdev->native_assign_ports[di->bus][di->port] = port; + xdev->native_assign_info[di->bus][di->port] = di; /* Trigger port change event for the arriving device / if (pci_xhci_port_connect(xdev, port, di->speed, intr)) @@ -561,7 +557,6 @@ pci_xhci_native_usb_dev_conn_cb(void hci_data, void dev_data) return 0; errout: - pci_xhci_dev_destroy(de); return -1; } @@ -571,8 +566,8 @@ pci_xhci_native_usb_dev_disconn_cb(void hci_data, void dev_data) struct pci_xhci_vdev xdev; struct pci_xhci_dev_emu edev; struct usb_dev udev; - uint8_t port, native_port; - int intr = 1; + uint8_t port, slot, native_port; + int i, j, intr = 1; assert(hci_data); assert(dev_data); @@ -601,6 +596,24 @@ pci_xhci_native_usb_dev_disconn_cb(void hci_data, void dev_data) return -1; } + for (slot = 1; slot < XHCI_MAX_SLOTS; ++slot) + if (xdev->slots[slot] == edev) + break; + + /* FIXME: again, find a better relationship for Slot, Port and Device / + for (i = 0; i < USB_NATIVE_NUM_BUS; ++i) { + if (!xdev->native_assign_ports[i]) + continue; + + for (j = 0; j < USB_NATIVE_NUM_PORT; ++j) + if (xdev->native_assign_ports[i][j] == port) + break; + + if (j < USB_NATIVE_NUM_PORT) + break; + } + assert(i < USB_NATIVE_NUM_BUS); + xdev->native_assign_ports[i][j] = -1; UPRINTF(LDBG, "report virtual port %d status\r\n", port); if (pci_xhci_port_disconnect(xdev, port, intr)) { UPRINTF(LFTL, "fail to report event\r\n"); @@ -1405,21 +1418,61 @@ static uint32_t pci_xhci_cmd_enable_slot(struct pci_xhci_vdev xdev, uint32_t slot) { struct pci_xhci_dev_emu dev; - uint32_t cmderr; - int i; + uint32_t cmderr; + struct usb_native_devinfo di; + int i, j, port; cmderr = XHCI_TRB_ERROR_NO_SLOTS; - if (xdev->portregs != NULL) - for (i = 1; i <= XHCI_MAX_SLOTS; i++) { - dev = XHCI_SLOTDEV_PTR(xdev, i); - if (dev && dev->dev_slotstate == XHCI_ST_DISABLED) { - slot = i; - dev->dev_slotstate = XHCI_ST_ENABLED; - cmderr = XHCI_TRB_ERROR_SUCCESS; - dev->hci.hci_address = i; - break; + + for (i = 0; i < USB_NATIVE_NUM_BUS; ++i) { + if (!xdev->native_assign_ports[i]) + continue; + + for (j = 0; j < USB_NATIVE_NUM_PORT; ++j) { + if (xdev->native_assign_ports[i][j] > 0) { + port = xdev->native_assign_ports[i][j]; + assert(port <= XHCI_MAX_DEVS); + + if (!xdev->devices[port]) + break; } } + if (j < USB_NATIVE_NUM_PORT) + break; + } + + if (i >= USB_NATIVE_NUM_BUS) { + UPRINTF(LWRN, "unexpected Enable Slot commnad\r\n"); + return -1; + } + + assert(xdev->native_assign_info[i]); + di = &xdev->native_assign_info[i][j]; + + assert(di->priv_data); + dev = pci_xhci_dev_create(xdev, di->priv_data); + if (!dev) { + UPRINTF(LFTL, "fail to create device\r\n"); + return -1; + } + + port = xdev->native_assign_ports[i][j]; + assert(port > 0); + assert(!xdev->devices[port]); + + xdev->devices[port] = dev; + xdev->ndevices++; + + for (i = 1; i <= XHCI_MAX_SLOTS; i++) { + if (XHCI_SLOTDEV_PTR(xdev, i) == NULL) { + xdev->slots[i] = dev; + slot = i; + dev->dev_slotstate = XHCI_ST_ENABLED; + cmderr = XHCI_TRB_ERROR_SUCCESS; + dev->hci.hci_address = i; + break; + } + } UPRINTF(LDBG, "enable slot (error=%d) slot %u\r\n", cmderr != XHCI_TRB_ERROR_SUCCESS, slot); @@ -1431,7 +1484,10 @@ static uint32_t pci_xhci_cmd_disable_slot(struct pci_xhci_vdev xdev, uint32_t slot) { struct pci_xhci_dev_emu dev; + struct usb_devemu ue; + uint8_t native_port, native_bus; uint32_t cmderr; + void ud; int i; UPRINTF(LDBG, "pci_xhci disable slot %u\r\n", slot); @@ -1454,6 +1510,9 @@ pci_xhci_cmd_disable_slot(struct pci_xhci_vdev xdev, uint32_t slot) cmderr = XHCI_TRB_ERROR_SUCCESS; / TODO: reset events and endpoints / } + } else { + UPRINTF(LDBG, "disable NULL device, slot %d\r\n", slot); + goto done; } for (i = 1; i <= XHCI_MAX_DEVS; ++i) @@ -1463,8 +1522,25 @@ pci_xhci_cmd_disable_slot(struct pci_xhci_vdev xdev, uint32_t slot) if (i <= XHCI_MAX_DEVS && XHCI_PORTREG_PTR(xdev, i)) { XHCI_PORTREG_PTR(xdev, i)->portsc &= ~(XHCI_PS_CSC \| XHCI_PS_CCS \| XHCI_PS_PED \| XHCI_PS_PP); + + ud = dev->dev_instance; + ue = dev->dev_ue; + + assert(ud); + assert(ue); + + ue->ue_info(ud, USB_INFO_BUS, &native_bus, sizeof(uint8_t)); + ue->ue_info(ud, USB_INFO_PORT, &native_port, sizeof(uint8_t)); + xdev->devices[i] = NULL; xdev->slots[slot] = NULL; + + assert(xdev->native_assign_ports[native_bus]); + assert(xdev->native_assign_ports[native_bus][native_port]); + UPRINTF(LINF, "disable slot %d for native device %d-%d" + "\r\n", slot, native_bus, native_port); + + xdev->native_assign_ports[native_bus][native_port] = -1; pci_xhci_dev_destroy(dev); } else UPRINTF(LWRN, "invalid slot %d\r\n", slot); @@ -1632,7 +1708,10 @@ pci_xhci_cmd_config_ep(struct pci_xhci_vdev xdev, UPRINTF(LDBG, "config_ep slot %u\r\n", slot); dev = XHCI_SLOTDEV_PTR(xdev, slot); - assert(dev != NULL); + if (dev == NULL) { + cmderr = XHCI_TRB_ERROR_SLOT_NOT_ON; + goto done; + } if ((trb->dwTrb3 & XHCI_TRB_3_DCEP_BIT) != 0) { UPRINTF(LDBG, "config_ep - deconfigure ep slot %u\r\n", slot); @@ -3352,8 +3431,10 @@ errout: static int pci_xhci_parse_bus_port(struct pci_xhci_vdev xdev, char opts) { - int rc = 0, cnt; + int rc = 0, cnt, portcnt; uint32_t port, bus; + int8_t pports; + struct usb_native_devinfo pinfo; assert(xdev); assert(opts); @@ -3372,15 +3453,30 @@ pci_xhci_parse_bus_port(struct pci_xhci_vdev xdev, char opts) goto errout; } - if (!xdev->native_assign_ports[bus]) { - xdev->native_assign_ports[bus] = calloc(USB_NATIVE_NUM_PORT, - sizeof(int8_t)); - if (!xdev->native_assign_ports[bus]) { + pports = xdev->native_assign_ports[bus]; + pinfo = xdev->native_assign_info[bus]; + + / those two pointers should always be consistant */ + assert((!pports) == (!pinfo)); + + if (!pports) { + portcnt = USB_NATIVE_NUM_PORT; + pports = calloc(portcnt, sizeof(uint8_t)); + pinfo = calloc(portcnt, sizeof(struct usb_native_devinfo)); + if (!pports \|\| !pinfo) { rc = -3; - goto errout; + goto release_out; } + + xdev->native_assign_ports[bus] = pports; + xdev->native_assign_info[bus] = pinfo; } xdev->native_assign_ports[bus][port] = -1; + return 0; + +release_out: + free(pinfo); + free(pports); errout: if (rc) -- 2.7.4
More All Messages By This Member
Yu Wang #10429 On 18-08-13 16:48:34, Wu, Xiaoguang wrote: The xHCI emulation greatly depends on the user space library libusb which is based on the usbfs module in Linux kernel. The libusb will bind usbfs to physical USB device which makes hardware control over libusb in user space possible. The pci_xhci_dev_create is called in pci_xhci_native_usb_dev_conn_cb which is a callback function triggered by physical USB device plugging. This function will bind the physical USB device to usbfs in SOS, which we depend to create the communication between UOS xHCI driver with physical USB device. This design will fail if the disconnection happened in the SOS, which I think it should be re-connection instead of disconnection, right? will bind class driver to the physical USB device instead of usbfs, hence the libusb device handle in DM is invalid. Currently when the SOS do the resuming from S3, there are some uncertain bugs which unbind the usbfs for the USB devices, so the DM will lose control and can't continue emulation. Currently, the native S3 will disable the vbus for all xHCI ports and re-drive during S3 resume. This behavior cause native USB driver unbind the usbfs and bind to related class driver, then made the DM lost control and failed to continue emulation. To fix this issue, place the pci_xhci_dev_create in the function pci_xhci_cmd_enable_slot. According to the xHCI spec 4.5.3 Figure 10, the UOS always send Enable Slot command when a device is attached or recovered from errors (by Disable Slot command). So every time the SOS can't resuming normally or some unexpected disconnection happens, this desigen will always survive by Disable Slot and Enable Slot command series from UOS xHCI driver. Signed-off-by: Xiaoguang Wu <xiaoguang.wu@...> Reviewed-by: Liang Yang <liang3.yang@...> --- devicemodel/hw/pci/xhci.c \| 220 +++++++++++++++++++++++++++++++++------------- 1 file changed, 158 insertions(+), 62 deletions(-) diff --git a/devicemodel/hw/pci/xhci.c b/devicemodel/hw/pci/xhci.c index 2df7ce3..579021a 100644 --- a/devicemodel/hw/pci/xhci.c +++ b/devicemodel/hw/pci/xhci.c @@ -383,6 +383,7 @@ struct pci_xhci_vdev { * is stored in native_assign_ports[x][y]. / int8_t native_assign_ports[USB_NATIVE_NUM_BUS]; + struct usb_native_devinfo native_assign_info[USB_NATIVE_NUM_BUS]; Can we merge the native_assign_ports and usb_native_devinfo?... Why the usb_native_devinfo can't include the port info... And I just realized the USB_NATIVE_NUM_BUS... Why it is too huge.. 255 USB buses?... struct timespec mf_prev_time; / previous time of accessing MFINDEX / }; @@ -475,36 +476,21 @@ static struct pci_xhci_option_elem xhci_option_table[] = { static int pci_xhci_native_usb_dev_conn_cb(void hci_data, void dev_data) { - struct pci_xhci_dev_emu de; struct pci_xhci_vdev xdev; - struct usb_devemu ue; struct usb_native_devinfo di; int port_start, port_end; - int slot_start, slot_end; - int port, slot; - void ud; + int port; int intr = 1; + int i, j; xdev = hci_data; - di = dev_data; assert(xdev); assert(dev_data); assert(xdev->devices); assert(xdev->slots); - de = pci_xhci_dev_create(xdev, di->priv_data); - if (!de) { - UPRINTF(LFTL, "fail to create device\r\n"); - return -1; - } - - /* find free port and slot for the new usb device / - ud = de->dev_instance; - ue = de->dev_ue; - - assert(ud); - assert(ue); + di = dev_data; / print physical information about new device / UPRINTF(LDBG, "%04x:%04x %d-%d connecting.\r\n", @@ -517,43 +503,53 @@ pci_xhci_native_usb_dev_conn_cb(void hci_data, void dev_data) goto errout; } + assert(xdev->native_assign_info[di->bus]); UPRINTF(LDBG, "%04x:%04x %d-%d belong to this vm.\r\n", di->vid, di->pid, di->bus, di->port); - if (di->bcd < 0x300) + if (di->bcd < 0x300) { port_start = xdev->usb2_port_start; - else + port_end = port_start + (XHCI_MAX_DEVS / 2); + } else { port_start = xdev->usb3_port_start; Please change the port_start/port_end to vport_start/vport_end. Otherwise, it is easy messed with native ports.. - - slot_start = 1; - port_end = port_start + (XHCI_MAX_DEVS / 2); - slot_end = XHCI_MAX_SLOTS; + port_end = port_start + (XHCI_MAX_DEVS / 2); + } / find free port / - for (port = port_start; port < port_end; port++) - if (!xdev->devices[port]) - break; + / FIXME: + * Will find a decent way to deal with the relationship among Slot, + * Port and Device. + / + for (port = port_start; port < port_end; port++) { + if (xdev->devices[port]) + continue; + + for (i = 0; i < USB_NATIVE_NUM_BUS; ++i) { + if (!xdev->native_assign_ports[i]) + continue; + + for (j = 0; j < USB_NATIVE_NUM_PORT; ++j) + if (xdev->native_assign_ports[i][j] == port) + break; Confused, you are trying to find one free virtual port? Why every native bus only allow to as one virtual port? Totally confused now. - / find free slot / - for (slot = slot_start; slot < slot_end; slot++) - if (!xdev->slots[slot]) + if (j < USB_NATIVE_NUM_PORT) + break; + } + if (i >= USB_NATIVE_NUM_BUS) break; + } We need to rework this loop-loop-loop-if-if-if code... It is too ugly and confused.. At least, please define one function for it with readily comprehensible function name. - if (port >= port_end \|\| slot >= slot_end) { - UPRINTF(LFTL, "no free resource: port %d slot %d\r\n", - port, slot); + if (port >= port_end) { + UPRINTF(LFTL, "no free virtual port for native device %d-%d" + "\r\n", di->bus, di->port); goto errout; } - / use index of devices as port number / - xdev->devices[port] = de; - xdev->slots[slot] = de; - xdev->ndevices++; + UPRINTF(LDBG, "%X:%X %d-%d is attached to virtual port %d.\r\n", + di->vid, di->pid, di->bus, di->port, port); - pci_xhci_reset_slot(xdev, slot); - UPRINTF(LDBG, "%X:%X %d-%d locates in slot %d port %d.\r\n", - di->vid, di->pid, di->bus, di->port, - slot, port); + xdev->native_assign_ports[di->bus][di->port] = port; + xdev->native_assign_info[di->bus][di->port] = di; /* Trigger port change event for the arriving device / if (pci_xhci_port_connect(xdev, port, di->speed, intr)) @@ -561,7 +557,6 @@ pci_xhci_native_usb_dev_conn_cb(void hci_data, void dev_data) return 0; errout: - pci_xhci_dev_destroy(de); return -1; } @@ -571,8 +566,8 @@ pci_xhci_native_usb_dev_disconn_cb(void hci_data, void dev_data) struct pci_xhci_vdev xdev; struct pci_xhci_dev_emu edev; struct usb_dev udev; - uint8_t port, native_port; - int intr = 1; + uint8_t port, slot, native_port; + int i, j, intr = 1; assert(hci_data); assert(dev_data); @@ -601,6 +596,24 @@ pci_xhci_native_usb_dev_disconn_cb(void hci_data, void dev_data) return -1; } + for (slot = 1; slot < XHCI_MAX_SLOTS; ++slot) + if (xdev->slots[slot] == edev) + break; + + /* FIXME: again, find a better relationship for Slot, Port and Device / + for (i = 0; i < USB_NATIVE_NUM_BUS; ++i) { + if (!xdev->native_assign_ports[i]) + continue; + + for (j = 0; j < USB_NATIVE_NUM_PORT; ++j) + if (xdev->native_assign_ports[i][j] == port) + break; + + if (j < USB_NATIVE_NUM_PORT) + break; + } Ditto. + assert(i < USB_NATIVE_NUM_BUS); + xdev->native_assign_ports[i][j] = -1; Define one macro instead of -1. UPRINTF(LDBG, "report virtual port %d status\r\n", port); if (pci_xhci_port_disconnect(xdev, port, intr)) { UPRINTF(LFTL, "fail to report event\r\n"); @@ -1405,21 +1418,61 @@ static uint32_t pci_xhci_cmd_enable_slot(struct pci_xhci_vdev xdev, uint32_t slot) { struct pci_xhci_dev_emu dev; - uint32_t cmderr; - int i; + uint32_t cmderr; + struct usb_native_devinfo di; + int i, j, port; cmderr = XHCI_TRB_ERROR_NO_SLOTS; - if (xdev->portregs != NULL) - for (i = 1; i <= XHCI_MAX_SLOTS; i++) { - dev = XHCI_SLOTDEV_PTR(xdev, i); - if (dev && dev->dev_slotstate == XHCI_ST_DISABLED) { - slot = i; - dev->dev_slotstate = XHCI_ST_ENABLED; - cmderr = XHCI_TRB_ERROR_SUCCESS; - dev->hci.hci_address = i; - break; + + for (i = 0; i < USB_NATIVE_NUM_BUS; ++i) { + if (!xdev->native_assign_ports[i]) + continue; + + for (j = 0; j < USB_NATIVE_NUM_PORT; ++j) { + if (xdev->native_assign_ports[i][j] > 0) { + port = xdev->native_assign_ports[i][j]; + assert(port <= XHCI_MAX_DEVS); + + if (!xdev->devices[port]) + break; } } + if (j < USB_NATIVE_NUM_PORT) + break; Ditto + } + + if (i >= USB_NATIVE_NUM_BUS) { + UPRINTF(LWRN, "unexpected Enable Slot commnad\r\n"); + return -1; + } + + assert(xdev->native_assign_info[i]); + di = &xdev->native_assign_info[i][j]; + + assert(di->priv_data); + dev = pci_xhci_dev_create(xdev, di->priv_data); + if (!dev) { + UPRINTF(LFTL, "fail to create device\r\n"); + return -1; + } + + port = xdev->native_assign_ports[i][j]; + assert(port > 0); + assert(!xdev->devices[port]); + + xdev->devices[port] = dev; + xdev->ndevices++; + + for (i = 1; i <= XHCI_MAX_SLOTS; i++) { + if (XHCI_SLOTDEV_PTR(xdev, i) == NULL) { + xdev->slots[i] = dev; + slot = i; + dev->dev_slotstate = XHCI_ST_ENABLED; + cmderr = XHCI_TRB_ERROR_SUCCESS; + dev->hci.hci_address = i; + break; + } + } UPRINTF(LDBG, "enable slot (error=%d) slot %u\r\n", cmderr != XHCI_TRB_ERROR_SUCCESS, slot); @@ -1431,7 +1484,10 @@ static uint32_t pci_xhci_cmd_disable_slot(struct pci_xhci_vdev xdev, uint32_t slot) { struct pci_xhci_dev_emu dev; + struct usb_devemu ue; + uint8_t native_port, native_bus; uint32_t cmderr; + void ud; int i; UPRINTF(LDBG, "pci_xhci disable slot %u\r\n", slot); @@ -1454,6 +1510,9 @@ pci_xhci_cmd_disable_slot(struct pci_xhci_vdev xdev, uint32_t slot) cmderr = XHCI_TRB_ERROR_SUCCESS; / TODO: reset events and endpoints / } + } else { + UPRINTF(LDBG, "disable NULL device, slot %d\r\n", slot); + goto done; } for (i = 1; i <= XHCI_MAX_DEVS; ++i) @@ -1463,8 +1522,25 @@ pci_xhci_cmd_disable_slot(struct pci_xhci_vdev xdev, uint32_t slot) if (i <= XHCI_MAX_DEVS && XHCI_PORTREG_PTR(xdev, i)) { XHCI_PORTREG_PTR(xdev, i)->portsc &= ~(XHCI_PS_CSC \| XHCI_PS_CCS \| XHCI_PS_PED \| XHCI_PS_PP); + + ud = dev->dev_instance; + ue = dev->dev_ue; + + assert(ud); + assert(ue); + + ue->ue_info(ud, USB_INFO_BUS, &native_bus, sizeof(uint8_t)); + ue->ue_info(ud, USB_INFO_PORT, &native_port, sizeof(uint8_t)); + xdev->devices[i] = NULL; xdev->slots[slot] = NULL; + + assert(xdev->native_assign_ports[native_bus]); + assert(xdev->native_assign_ports[native_bus][native_port]); + UPRINTF(LINF, "disable slot %d for native device %d-%d" + "\r\n", slot, native_bus, native_port); + + xdev->native_assign_ports[native_bus][native_port] = -1; pci_xhci_dev_destroy(dev); } else UPRINTF(LWRN, "invalid slot %d\r\n", slot); @@ -1632,7 +1708,10 @@ pci_xhci_cmd_config_ep(struct pci_xhci_vdev xdev, UPRINTF(LDBG, "config_ep slot %u\r\n", slot); dev = XHCI_SLOTDEV_PTR(xdev, slot); - assert(dev != NULL); + if (dev == NULL) { + cmderr = XHCI_TRB_ERROR_SLOT_NOT_ON; + goto done; + } if ((trb->dwTrb3 & XHCI_TRB_3_DCEP_BIT) != 0) { UPRINTF(LDBG, "config_ep - deconfigure ep slot %u\r\n", slot); @@ -3352,8 +3431,10 @@ errout: static int pci_xhci_parse_bus_port(struct pci_xhci_vdev xdev, char opts) { - int rc = 0, cnt; + int rc = 0, cnt, portcnt; uint32_t port, bus; + int8_t pports; + struct usb_native_devinfo pinfo; assert(xdev); assert(opts); @@ -3372,15 +3453,30 @@ pci_xhci_parse_bus_port(struct pci_xhci_vdev xdev, char opts) goto errout; } - if (!xdev->native_assign_ports[bus]) { - xdev->native_assign_ports[bus] = calloc(USB_NATIVE_NUM_PORT, - sizeof(int8_t)); - if (!xdev->native_assign_ports[bus]) { + pports = xdev->native_assign_ports[bus]; + pinfo = xdev->native_assign_info[bus]; + + / those two pointers should always be consistant */ + assert((!pports) == (!pinfo)); + + if (!pports) { + portcnt = USB_NATIVE_NUM_PORT; + pports = calloc(portcnt, sizeof(uint8_t)); + pinfo = calloc(portcnt, sizeof(struct usb_native_devinfo)); + if (!pports \|\| !pinfo) { rc = -3; - goto errout; + goto release_out; } + + xdev->native_assign_ports[bus] = pports; + xdev->native_assign_info[bus] = pinfo; } xdev->native_assign_ports[bus][port] = -1; + return 0; + +release_out: + free(pinfo); + free(pports); errout: if (rc) -- 2.7.4
More All Messages By This Member
Wu, Xiaoguang #10453 On 18-08-13 22:35:32, Yu Wang wrote: On 18-08-13 16:48:34, Wu, Xiaoguang wrote: The xHCI emulation greatly depends on the user space library libusb which is based on the usbfs module in Linux kernel. The libusb will bind usbfs to physical USB device which makes hardware control over libusb in user space possible. The pci_xhci_dev_create is called in pci_xhci_native_usb_dev_conn_cb which is a callback function triggered by physical USB device plugging. This function will bind the physical USB device to usbfs in SOS, which we depend to create the communication between UOS xHCI driver with physical USB device. This design will fail if the disconnection happened in the SOS, which I think it should be re-connection instead of disconnection, right? xgwu: ok, will change. thx. will bind class driver to the physical USB device instead of usbfs, hence the libusb device handle in DM is invalid. Currently when the SOS do the resuming from S3, there are some uncertain bugs which unbind the usbfs for the USB devices, so the DM will lose control and can't continue emulation. Currently, the native S3 will disable the vbus for all xHCI ports and re-drive during S3 resume. This behavior cause native USB driver unbind the usbfs and bind to related class driver, then made the DM lost control and failed to continue emulation. xguw: ok, will change. thx. To fix this issue, place the pci_xhci_dev_create in the function pci_xhci_cmd_enable_slot. According to the xHCI spec 4.5.3 Figure 10, the UOS always send Enable Slot command when a device is attached or recovered from errors (by Disable Slot command). So every time the SOS can't resuming normally or some unexpected disconnection happens, this desigen will always survive by Disable Slot and Enable Slot command series from UOS xHCI driver. Signed-off-by: Xiaoguang Wu <xiaoguang.wu@...> Reviewed-by: Liang Yang <liang3.yang@...> --- devicemodel/hw/pci/xhci.c \| 220 +++++++++++++++++++++++++++++++++------------- 1 file changed, 158 insertions(+), 62 deletions(-) diff --git a/devicemodel/hw/pci/xhci.c b/devicemodel/hw/pci/xhci.c index 2df7ce3..579021a 100644 --- a/devicemodel/hw/pci/xhci.c +++ b/devicemodel/hw/pci/xhci.c @@ -383,6 +383,7 @@ struct pci_xhci_vdev { * is stored in native_assign_ports[x][y]. / int8_t native_assign_ports[USB_NATIVE_NUM_BUS]; + struct usb_native_devinfo native_assign_info[USB_NATIVE_NUM_BUS]; Can we merge the native_assign_ports and usb_native_devinfo?... Why the usb_native_devinfo can't include the port info... xgwu: change it to: native_assign_ports -> port_map_tbl native_assign_info -> native_dev_info In futurn, should refine it. thanks. And I just realized the USB_NATIVE_NUM_BUS... Why it is too huge.. 255 USB buses?... xgwu: ok, as discussed, change it to 4. and change USB_NATIVE_NUM_PORT to 20 thanks. struct timespec mf_prev_time; / previous time of accessing MFINDEX / }; @@ -475,36 +476,21 @@ static struct pci_xhci_option_elem xhci_option_table[] = { static int pci_xhci_native_usb_dev_conn_cb(void hci_data, void dev_data) { - struct pci_xhci_dev_emu de; struct pci_xhci_vdev xdev; - struct usb_devemu ue; struct usb_native_devinfo di; int port_start, port_end; - int slot_start, slot_end; - int port, slot; - void ud; + int port; int intr = 1; + int i, j; xdev = hci_data; - di = dev_data; assert(xdev); assert(dev_data); assert(xdev->devices); assert(xdev->slots); - de = pci_xhci_dev_create(xdev, di->priv_data); - if (!de) { - UPRINTF(LFTL, "fail to create device\r\n"); - return -1; - } - - /* find free port and slot for the new usb device / - ud = de->dev_instance; - ue = de->dev_ue; - - assert(ud); - assert(ue); + di = dev_data; / print physical information about new device / UPRINTF(LDBG, "%04x:%04x %d-%d connecting.\r\n", @@ -517,43 +503,53 @@ pci_xhci_native_usb_dev_conn_cb(void hci_data, void dev_data) goto errout; } + assert(xdev->native_assign_info[di->bus]); UPRINTF(LDBG, "%04x:%04x %d-%d belong to this vm.\r\n", di->vid, di->pid, di->bus, di->port); - if (di->bcd < 0x300) + if (di->bcd < 0x300) { port_start = xdev->usb2_port_start; - else + port_end = port_start + (XHCI_MAX_DEVS / 2); + } else { port_start = xdev->usb3_port_start; Please change the port_start/port_end to vport_start/vport_end. Otherwise, it is easy messed with native ports.. xgwu: ok, will change it. thanks. - - slot_start = 1; - port_end = port_start + (XHCI_MAX_DEVS / 2); - slot_end = XHCI_MAX_SLOTS; + port_end = port_start + (XHCI_MAX_DEVS / 2); + } / find free port / - for (port = port_start; port < port_end; port++) - if (!xdev->devices[port]) - break; + / FIXME: + * Will find a decent way to deal with the relationship among Slot, + * Port and Device. + / + for (port = port_start; port < port_end; port++) { + if (xdev->devices[port]) + continue; + + for (i = 0; i < USB_NATIVE_NUM_BUS; ++i) { + if (!xdev->native_assign_ports[i]) + continue; + + for (j = 0; j < USB_NATIVE_NUM_PORT; ++j) + if (xdev->native_assign_ports[i][j] == port) + break; Confused, you are trying to find one free virtual port? Why every native bus only allow to as one virtual port? Totally confused now. xgwu: the code is not good, will change to simpler way. thanks. - / find free slot / - for (slot = slot_start; slot < slot_end; slot++) - if (!xdev->slots[slot]) + if (j < USB_NATIVE_NUM_PORT) + break; + } + if (i >= USB_NATIVE_NUM_BUS) break; + } We need to rework this loop-loop-loop-if-if-if code... It is too ugly and confused.. At least, please define one function for it with readily comprehensible function name. xgwu: ok, will change. thanks. - if (port >= port_end \|\| slot >= slot_end) { - UPRINTF(LFTL, "no free resource: port %d slot %d\r\n", - port, slot); + if (port >= port_end) { + UPRINTF(LFTL, "no free virtual port for native device %d-%d" + "\r\n", di->bus, di->port); goto errout; } - / use index of devices as port number / - xdev->devices[port] = de; - xdev->slots[slot] = de; - xdev->ndevices++; + UPRINTF(LDBG, "%X:%X %d-%d is attached to virtual port %d.\r\n", + di->vid, di->pid, di->bus, di->port, port); - pci_xhci_reset_slot(xdev, slot); - UPRINTF(LDBG, "%X:%X %d-%d locates in slot %d port %d.\r\n", - di->vid, di->pid, di->bus, di->port, - slot, port); + xdev->native_assign_ports[di->bus][di->port] = port; + xdev->native_assign_info[di->bus][di->port] = di; /* Trigger port change event for the arriving device / if (pci_xhci_port_connect(xdev, port, di->speed, intr)) @@ -561,7 +557,6 @@ pci_xhci_native_usb_dev_conn_cb(void hci_data, void dev_data) return 0; errout: - pci_xhci_dev_destroy(de); return -1; } @@ -571,8 +566,8 @@ pci_xhci_native_usb_dev_disconn_cb(void hci_data, void dev_data) struct pci_xhci_vdev xdev; struct pci_xhci_dev_emu edev; struct usb_dev udev; - uint8_t port, native_port; - int intr = 1; + uint8_t port, slot, native_port; + int i, j, intr = 1; assert(hci_data); assert(dev_data); @@ -601,6 +596,24 @@ pci_xhci_native_usb_dev_disconn_cb(void hci_data, void dev_data) return -1; } + for (slot = 1; slot < XHCI_MAX_SLOTS; ++slot) + if (xdev->slots[slot] == edev) + break; + + /* FIXME: again, find a better relationship for Slot, Port and Device / + for (i = 0; i < USB_NATIVE_NUM_BUS; ++i) { + if (!xdev->native_assign_ports[i]) + continue; + + for (j = 0; j < USB_NATIVE_NUM_PORT; ++j) + if (xdev->native_assign_ports[i][j] == port) + break; + + if (j < USB_NATIVE_NUM_PORT) + break; + } Ditto. xgwu: will remove the loop. thx. + assert(i < USB_NATIVE_NUM_BUS); + xdev->native_assign_ports[i][j] = -1; Define one macro instead of -1. xgwu: ok, will change.thx. UPRINTF(LDBG, "report virtual port %d status\r\n", port); if (pci_xhci_port_disconnect(xdev, port, intr)) { UPRINTF(LFTL, "fail to report event\r\n"); @@ -1405,21 +1418,61 @@ static uint32_t pci_xhci_cmd_enable_slot(struct pci_xhci_vdev xdev, uint32_t slot) { struct pci_xhci_dev_emu dev; - uint32_t cmderr; - int i; + uint32_t cmderr; + struct usb_native_devinfo di; + int i, j, port; cmderr = XHCI_TRB_ERROR_NO_SLOTS; - if (xdev->portregs != NULL) - for (i = 1; i <= XHCI_MAX_SLOTS; i++) { - dev = XHCI_SLOTDEV_PTR(xdev, i); - if (dev && dev->dev_slotstate == XHCI_ST_DISABLED) { - slot = i; - dev->dev_slotstate = XHCI_ST_ENABLED; - cmderr = XHCI_TRB_ERROR_SUCCESS; - dev->hci.hci_address = i; - break; + + for (i = 0; i < USB_NATIVE_NUM_BUS; ++i) { + if (!xdev->native_assign_ports[i]) + continue; + + for (j = 0; j < USB_NATIVE_NUM_PORT; ++j) { + if (xdev->native_assign_ports[i][j] > 0) { + port = xdev->native_assign_ports[i][j]; + assert(port <= XHCI_MAX_DEVS); + + if (!xdev->devices[port]) + break; } } + if (j < USB_NATIVE_NUM_PORT) + break; Ditto xgwu: will use a function do this. thanks. + } + + if (i >= USB_NATIVE_NUM_BUS) { + UPRINTF(LWRN, "unexpected Enable Slot commnad\r\n"); + return -1; + } + + assert(xdev->native_assign_info[i]); + di = &xdev->native_assign_info[i][j]; + + assert(di->priv_data); + dev = pci_xhci_dev_create(xdev, di->priv_data); + if (!dev) { + UPRINTF(LFTL, "fail to create device\r\n"); + return -1; + } + + port = xdev->native_assign_ports[i][j]; + assert(port > 0); + assert(!xdev->devices[port]); + + xdev->devices[port] = dev; + xdev->ndevices++; + + for (i = 1; i <= XHCI_MAX_SLOTS; i++) { + if (XHCI_SLOTDEV_PTR(xdev, i) == NULL) { + xdev->slots[i] = dev; + slot = i; + dev->dev_slotstate = XHCI_ST_ENABLED; + cmderr = XHCI_TRB_ERROR_SUCCESS; + dev->hci.hci_address = i; + break; + } + } UPRINTF(LDBG, "enable slot (error=%d) slot %u\r\n", cmderr != XHCI_TRB_ERROR_SUCCESS, slot); @@ -1431,7 +1484,10 @@ static uint32_t pci_xhci_cmd_disable_slot(struct pci_xhci_vdev xdev, uint32_t slot) { struct pci_xhci_dev_emu dev; + struct usb_devemu ue; + uint8_t native_port, native_bus; uint32_t cmderr; + void ud; int i; UPRINTF(LDBG, "pci_xhci disable slot %u\r\n", slot); @@ -1454,6 +1510,9 @@ pci_xhci_cmd_disable_slot(struct pci_xhci_vdev xdev, uint32_t slot) cmderr = XHCI_TRB_ERROR_SUCCESS; / TODO: reset events and endpoints / } + } else { + UPRINTF(LDBG, "disable NULL device, slot %d\r\n", slot); + goto done; } for (i = 1; i <= XHCI_MAX_DEVS; ++i) @@ -1463,8 +1522,25 @@ pci_xhci_cmd_disable_slot(struct pci_xhci_vdev xdev, uint32_t slot) if (i <= XHCI_MAX_DEVS && XHCI_PORTREG_PTR(xdev, i)) { XHCI_PORTREG_PTR(xdev, i)->portsc &= ~(XHCI_PS_CSC \| XHCI_PS_CCS \| XHCI_PS_PED \| XHCI_PS_PP); + + ud = dev->dev_instance; + ue = dev->dev_ue; + + assert(ud); + assert(ue); + + ue->ue_info(ud, USB_INFO_BUS, &native_bus, sizeof(uint8_t)); + ue->ue_info(ud, USB_INFO_PORT, &native_port, sizeof(uint8_t)); + xdev->devices[i] = NULL; xdev->slots[slot] = NULL; + + assert(xdev->native_assign_ports[native_bus]); + assert(xdev->native_assign_ports[native_bus][native_port]); + UPRINTF(LINF, "disable slot %d for native device %d-%d" + "\r\n", slot, native_bus, native_port); + + xdev->native_assign_ports[native_bus][native_port] = -1; pci_xhci_dev_destroy(dev); } else UPRINTF(LWRN, "invalid slot %d\r\n", slot); @@ -1632,7 +1708,10 @@ pci_xhci_cmd_config_ep(struct pci_xhci_vdev xdev, UPRINTF(LDBG, "config_ep slot %u\r\n", slot); dev = XHCI_SLOTDEV_PTR(xdev, slot); - assert(dev != NULL); + if (dev == NULL) { + cmderr = XHCI_TRB_ERROR_SLOT_NOT_ON; + goto done; + } if ((trb->dwTrb3 & XHCI_TRB_3_DCEP_BIT) != 0) { UPRINTF(LDBG, "config_ep - deconfigure ep slot %u\r\n", slot); @@ -3352,8 +3431,10 @@ errout: static int pci_xhci_parse_bus_port(struct pci_xhci_vdev xdev, char opts) { - int rc = 0, cnt; + int rc = 0, cnt, portcnt; uint32_t port, bus; + int8_t pports; + struct usb_native_devinfo pinfo; assert(xdev); assert(opts); @@ -3372,15 +3453,30 @@ pci_xhci_parse_bus_port(struct pci_xhci_vdev xdev, char opts) goto errout; } - if (!xdev->native_assign_ports[bus]) { - xdev->native_assign_ports[bus] = calloc(USB_NATIVE_NUM_PORT, - sizeof(int8_t)); - if (!xdev->native_assign_ports[bus]) { + pports = xdev->native_assign_ports[bus]; + pinfo = xdev->native_assign_info[bus]; + + / those two pointers should always be consistant */ + assert((!pports) == (!pinfo)); + + if (!pports) { + portcnt = USB_NATIVE_NUM_PORT; + pports = calloc(portcnt, sizeof(uint8_t)); + pinfo = calloc(portcnt, sizeof(struct usb_native_devinfo)); + if (!pports \|\| !pinfo) { rc = -3; - goto errout; + goto release_out; } + + xdev->native_assign_ports[bus] = pports; + xdev->native_assign_info[bus] = pinfo; } xdev->native_assign_ports[bus][port] = -1; + return 0; + +release_out: + free(pinfo); + free(pports); errout: if (rc) -- 2.7.4
More All Messages By This Member

1 - 3 of 3

1

Previous Topic Next Topic

Home Hashtags Subgroups Terms