> Date: Fri, 21 May 2021 19:01:39 +0200 > From: Patrick Wildt <patr...@blueri.se> > > Am Fri, May 21, 2021 at 06:18:40PM +0200 schrieb Martin Pieuchot: > > 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(). > > Hm, that's weird, since on usb(4) attach there's an > > if (cold) > sc->sc_bus->use_polling++; > [...] > sc->sc_bus->soft = softintr_establish(IPL_SOFTUSB, > [...] > if polling -> explore > if (cold) > sc->sc_bus->use_polling--; > usb needs explore > > And the xhci softintr does: if polling, call > xhci-bus-methods->soft_intr, else sched softintr. > > Point being, softintr should already be established. > > Unless(!) xhci(4) at this point already throws a real interrupt > which calls the xhci interrupt handler, even before usb(4) was > able to attach. > > In that case I'd wonder, are we even allowed to receive interrupts > at that point? I thought we only poll when attaching stuff.
attach can happen when when !cold, e.g. when we're doing PCI hotplug.
