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.
Patrick
>
> > > 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
Heh, was wondering that as well.
> 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);
>
