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);

Reply via email to