Re: [PATCH] drivers/char: add timeout for XHCI console cable

[Marek Marczykowski-Górecki <marmarek@xxxxxxxxxxxxxxxxxxxxxx>]

On Thu, Oct 08, 2026 at 05:54:53PM +0200, Jan Beulich wrote:
> On 08.10.2026 17:05, Marek Marczykowski-Górecki wrote:
> > Do not wait indefinitely for XHCI to configure console connection. If
> > the cable is not plugged in, timeout after about 1s and continue boot.
> > If the cable is plugged in later, the console will be activated then
> > (see final part of dbc_ensure_running()). When timeout occurs, print a
> > warning message - it will be saved in the console ring buffer, to be
> > later retrieved from dom0, or possibly later printed when the cable is
> > finally connected. But, do not print the warning when waiting after
> > controller reset, as that may have been called from from console
> > dbc_putc() and printing here would result in (possibly infinite)
> > recursion.
> > 
> > Try the timeout to be about 1s, which is way above time needed for
> > configuring already connected cable. But, since this part runs very
> > early in the Xen startup, time is not calibrated yet, and functions like
> > NOW() do not work yet. Use raw TSC and estimate needed time to be at
> > least 1s on a fast CPU (5GHz), and possibly longer on a slower one. This
> > is not very accurate method, but fortunately it doesn't need to be.
> 
> A little fragile, but well...
> 
> > --- a/xen/drivers/char/xhci-dbc.c
> > +++ b/xen/drivers/char/xhci-dbc.c
> > @@ -851,9 +851,15 @@ static void dbc_reset_debug_port(struct dbc *dbc)
> >      }
> >  }
> >  
> > -static void dbc_enable_dbc(struct dbc *dbc)
> > +static bool dbc_enable_dbc(struct dbc *dbc)
> >  {
> >      struct dbc_reg *reg = dbc->dbc_reg;
> > +    s_time_t start;
> > +    /*
> > +     * Too early for proper time calibration, assume a 5GHz CPU and let it 
> > wait
> > +     * about 1s, with still reasonable timeout on a slower CPU.
> > +     */
> > +    unsigned long timeout_ticks = 5UL*1000*1000*1000;
> 
> Even if right now we don't use the driver in any 32-bit env, I think we
> better wouldn't deliberately break such a possible use. IOW unsigned
> long isn't a suitable type to use here.

So, uint64_t?

> Also, nit (style): Blanks please around *.
> 
> > @@ -870,8 +876,12 @@ static void dbc_enable_dbc(struct dbc *dbc)
> >      writel(readl(&reg->portsc) | (1U << DBC_PSC_PED), &reg->portsc);
> >      wmb();
> >  
> > -    while ( (readl(&reg->ctrl) & (1U << DBC_CTRL_DCR)) == 0 )
> > +    start = rdtsc_ordered();
> > +    while ( (readl(&reg->ctrl) & (1U << DBC_CTRL_DCR)) == 0 &&
> > +            rdtsc_ordered() - start < timeout_ticks )
> 
> Urgh - now the driver's truly becoming x86-specific. Is there a reason
> this cannot be get_cycles()?

Sure, it can be get_cycles(). I just need it to not try to apply scaling
that works only after calibration, but indeed get_cycles() should work.

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab

Attachment: signature.asc
Description: PGP signature