Re: [PATCH v2 2/2] xen/ns16550: drain TX before reinitializing

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

On 10/10/26 5:14 AM, Zhang Zheng wrote:
> An SBI console call may return before the UART finishes transmitting.
> Wait for UART_LSR_TEMT before ns16550 reprograms the UART and clears its
> FIFO, preserving pending early-console output and legacy SBI v0.1 support.
The problem isn't really specific to SBI: anything that wrote to the 
UART before ns16550 takes over (firmware, a bootloader, or Xen's own 
early console) can leave data in the TX FIFO or the shift register. I'd
describe it in those terms.
> Suggested-by: Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>
> Link: 
> https://lore.kernel.org/xen-devel/8836d1e3-77c8-4447-b1ef-132620a71441@xxxxxxxxx/
> Signed-off-by: Zhang Zheng <zhangzheng@xxxxxxxxxxx>
> ---
>   xen/drivers/char/ns16550.c | 4 ++++
>   1 file changed, 4 insertions(+)
>
> diff --git a/xen/drivers/char/ns16550.c b/xen/drivers/char/ns16550.c
> index c174465059..278d38822d 100644
> --- a/xen/drivers/char/ns16550.c
> +++ b/xen/drivers/char/ns16550.c
> @@ -427,6 +427,10 @@ static void __init cf_check ns16550_init_preirq(struct 
> serial_port *port)
>   #endif
>       }
>   
> +    /* Preserve output queued by firmware before taking over the UART. */
я'
Could the comment say why the wait is needed at this point? Something
like:

    /*
     * Output written before ns16550 takes over (e.g. by firmware or the
     * early console) may still be in the transmitter. Let it drain before
     * ns16550_setup_preirq() reprograms the line settings and resets the
     * FIFOs.
     */

> +    while ( !(ns_read_reg(uart, UART_LSR) & UART_LSR_TEMT) )
> +        cpu_relax();
> +
IMO waiting for TEMT rather than THRE is the right choice: besides
resetting the FIFOs, ns16550_setup_preirq() reprograms the divisor and
LCR, which would corrupt a character still being shifted out. (Linux
waits for UART_LSR_BOTH_EMPTY, i.e. THRE | TEMT, but on a 16550 TEMT
already implies THRE.)

The unbounded wait can hang early Xen boot if the UART never reports 
TEMT for some reason(?). Bounding the wait would make console 
initialization more robust, allowing boot to continue when the UART is 
unavailable
    for ( i = 0; i < 50000; i++ )
    {
        if ( ns_read_reg(uart, UART_LSR) & UART_LSR_TEMT )
            break;
        udelay(10);
    }

would need udelay() on RISC-V, which is still a BUG_ON() stub, so a
separate patch would be needed for that. On x86, udelay() doesn't delay
yet at this point (cpu_khz is only set by early_time_init()), so there
the loop would just be an iteration count. Perhaps someone has a better
idea of how to implement a timeout here.

~ Oleksii

>       ns16550_setup_preirq(uart);
>   
>       /* Check this really is a 16550+. Otherwise we have no FIFOs. */