Am Fri, May 21, 2021 at 07:24:59PM +0200 schrieb Mark Kettenis: > > 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.
That's a good point, forgotten about that. Maybe interrupt enable and 'start controller' should be split then? I mean, controllers can be used in polling mode and the important bit is that our polling code can see the status flag in the intr status register, but we don't need an actual interrupt to be triggered. Or maybe we should just establish the interrupt handler after all this has been set up?
