On 21/05/21(Fri) 10:48, Patrick Wildt wrote: > Am Wed, May 19, 2021 at 07:15:50AM +0000 schrieb Christian Ludwig: > > The usb(4) driver allows to enumerate the bus early during boot by > > setting its driver flags to 0x1 in UKC. This mechanism can enable a USB > > console keyboard early during autoconf(9), which can come in handy at > > times. This needs USB polling mode to work, which is a bit broken. Here > > is my attempt to fix it for xhci(4) controllers. > > > > According to the xHCI specification section 4.2 "Host Controller > > Initalization", the host controller must be fully initialized before > > descending into device enumeration. Then xhci(4) sends command TRBs to > > open new pipes during enumeration. They wait for completion using > > tsleep(). This is bad when in polling mode at boot. And finally, the > > behavior should be the same on resume as it is at boot. Therefore also > > enumerate USB devices during resume when the flag is set. > > > > I am specifically looking for tests on xhci controllers with usb(4) > > flags set to 1 in UKC. > > > > So long, > > > > > > - Christian > > > > > > diff --git a/sys/arch/armv7/marvell/mvxhci.c > > b/sys/arch/armv7/marvell/mvxhci.c > > index 38a636fd123..2137f68b816 100644 > > --- a/sys/arch/armv7/marvell/mvxhci.c > > +++ b/sys/arch/armv7/marvell/mvxhci.c > > @@ -155,12 +155,12 @@ mvxhci_attach(struct device *parent, struct device > > *self, void *aux) > > goto disestablish_ret; > > } > > > > - /* Attach usb device. */ > > - config_found(self, &sc->sc.sc_bus, usbctlprint); > > - > > /* Now that the stack is ready, config' the HC and enable interrupts. */ > > xhci_config(&sc->sc); > > > > + /* Attach usb device. */ > > + config_found(self, &sc->sc.sc_bus, usbctlprint); > > + > > return; > > > > disestablish_ret: > > diff --git a/sys/dev/acpi/xhci_acpi.c b/sys/dev/acpi/xhci_acpi.c > > index 95e69cee896..d762f69a00e 100644 > > --- a/sys/dev/acpi/xhci_acpi.c > > +++ b/sys/dev/acpi/xhci_acpi.c > > @@ -112,12 +112,12 @@ xhci_acpi_attach(struct device *parent, struct device > > *self, void *aux) > > goto disestablish_ret; > > } > > > > - /* Attach usb device. */ > > - config_found(self, &sc->sc.sc_bus, usbctlprint); > > - > > /* Now that the stack is ready, config' the HC and enable interrupts. */ > > xhci_config(&sc->sc); > > > > + /* Attach usb device. */ > > + config_found(self, &sc->sc.sc_bus, usbctlprint); > > + > > return; > > > > disestablish_ret: > > diff --git a/sys/dev/fdt/xhci_fdt.c b/sys/dev/fdt/xhci_fdt.c > > index 38c976a6b24..84e00bdadc5 100644 > > --- a/sys/dev/fdt/xhci_fdt.c > > +++ b/sys/dev/fdt/xhci_fdt.c > > @@ -116,12 +116,12 @@ xhci_fdt_attach(struct device *parent, struct device > > *self, void *aux) > > goto disestablish_ret; > > } > > > > - /* Attach usb device. */ > > - config_found(self, &sc->sc.sc_bus, usbctlprint); > > - > > /* Now that the stack is ready, config' the HC and enable interrupts. */ > > xhci_config(&sc->sc);
> > > > + /* Attach usb device. */ > > + config_found(self, &sc->sc.sc_bus, usbctlprint); > > + > > return; > > > > disestablish_ret: > > diff --git a/sys/dev/pci/xhci_pci.c b/sys/dev/pci/xhci_pci.c > > index fa3271b0d30..0b46083b705 100644 > > --- a/sys/dev/pci/xhci_pci.c > > +++ b/sys/dev/pci/xhci_pci.c > > @@ -195,12 +195,12 @@ xhci_pci_attach(struct device *parent, struct device > > *self, void *aux) > > if (PCI_VENDOR(psc->sc_id) == PCI_VENDOR_INTEL) > > xhci_pci_port_route(psc); > > > > - /* Attach usb device. */ > > - config_found(self, &psc->sc.sc_bus, usbctlprint); > > - > > /* Now that the stack is ready, config' the HC and enable interrupts. */ > > xhci_config(&psc->sc); > > > > + /* Attach usb device. */ > > + config_found(self, &psc->sc.sc_bus, usbctlprint); > > + > > return; > > > > disestablish_ret: > > The interesting thing is that xhci_config() used to be part of > xhci_init() and was explicitly taken out from it to fix a panic > that showed up when enumeration happened afterwards. > > https://github.com/openbsd/src/commit/48155c88d2b90737b892a715e56d81bc73254308 > > Is it possible that this works in polling mode, but not without? > > While I agree that moving xhci_config() before enumeration creates > consistency with the others, this change was done deliberately and > we should find out why. > > mpi, do you still happen to have the logs or the machine for that > particular issue? I don't. If I recall correctly the problem what about calling the interrupt handler calling usb_schedsoftintr() before usb_attach() had the time to execute softintr_establish(). > > diff --git a/sys/dev/usb/usb.c b/sys/dev/usb/usb.c > > index b8943882d0a..f9aff94bfee 100644 > > --- a/sys/dev/usb/usb.c > > +++ b/sys/dev/usb/usb.c > > @@ -911,8 +911,19 @@ usb_activate(struct device *self, int act) > > * hub transfers do not need to sleep. > > */ > > sc->sc_bus->use_polling++; > > - if (!usb_attach_roothub(sc)) > > + if (!usb_attach_roothub(sc)) { > > + struct usbd_device *dev = sc->sc_bus->root_hub; > > +#if 1 > > + /* > > + * Turning this code off will delay attachment of USB > > devices > > + * until the USB task thread is running, which means > > that > > + * the keyboard will not work until after resume. > > + */ > > + if (cold && (sc->sc_dev.dv_cfdata->cf_flags & 1)) > > + dev->hub->explore(sc->sc_bus->root_hub); > > +#endif I don't understand why this code isn't enabled by default, isn't there a race with the USB_BUS_CONFIG_PENDING logic? Maybe this should be folded into usb_needs_explore() with a 'bus->use_polling' logic similar to what is done in usb_schedsoftintr()? > > usb_needs_explore(sc->sc_bus->root_hub, 0); > > + } > > sc->sc_bus->use_polling--; > > break; > > default: > > diff --git a/sys/dev/usb/xhci.c b/sys/dev/usb/xhci.c > > index e6f4a802dd7..d8e870cac4e 100644 > > --- a/sys/dev/usb/xhci.c > > +++ b/sys/dev/usb/xhci.c > > @@ -1896,7 +1896,21 @@ xhci_command_submit(struct xhci_softc *sc, struct > > xhci_trb *trb0, int timeout) > > s = splusb(); > > sc->sc_cmd_trb = trb; > > XDWRITE4(sc, XHCI_DOORBELL(0), 0); > > - error = tsleep_nsec(&sc->sc_cmd_trb, PZERO, "xhcicmd", timeout); > > + if (sc->sc_bus.use_polling) { > > + int timo; > > + > > + for (timo = timeout; timo >= 0; timo--) { > > + usb_delay_ms(&sc->sc_bus, 1); > > + xhci_poll(&sc->sc_bus); > > + if (sc->sc_cmd_trb == NULL) > > + break; > > + } > > + > > + if (timo < 0) > > + error = ETIMEDOUT; > > + } else { > > + error = tsleep_nsec(&sc->sc_cmd_trb, PZERO, "xhcicmd", timeout); > > + } > > if (error) { > > #ifdef XHCI_DEBUG > > printf("%s: tsleep() = %d\n", __func__, error);