[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v2 2/2] xen/ns16550: drain TX before reinitializing
- To: Zhang Zheng <zhangzheng@xxxxxxxxxxx>
- From: Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>
- Date: Thu, 15 Oct 2026 09:54:46 +0200
- Authentication-results: eu.smtp.expurgate.cloud; dkim=pass header.s=20251104 header.d=gmail.com header.i="@gmail.com" header.h="Content-Transfer-Encoding:Content-Type:In-Reply-To:From:Content-Language:References:Cc:To:Subject:User-Agent:MIME-Version:Date:Message-ID"
- Cc: xen-devel@xxxxxxxxxxxxxxxxxxxx, Alistair Francis <alistair.francis@xxxxxxx>, Connor Davis <connojdavis@xxxxxxxxx>, Baptiste Le Duc <baptiste.le-duc@xxxxxxxxxx>, Andrew Cooper <andrew.cooper3@xxxxxxxxxx>, Anthony PERARD <anthony.perard@xxxxxxxxxx>, Michal Orzel <michal.orzel@xxxxxxx>, Jan Beulich <jbeulich@xxxxxxxx>, Julien Grall <julien@xxxxxxx>, Roger Pau Monné <roger@xxxxxxxxxxxxxx>, Stefano Stabellini <sstabellini@xxxxxxxxxx>
- Delivery-date: Thu, 15 Oct 2026 07:55:14 +0000
- List-id: Xen developer discussion <xen-devel.lists.xenproject.org>
On 10/15/26 8:44 AM, Zhang Zheng wrote:
On Mon, Oct 12, 2026 at 12:20:50PM +0200, Oleksii Kurochko wrote:
--- a/xen/drivers/char/ns16550.c
+++ b/xen/drivers/char/ns16550.c
[...]
+ 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.)
Yes, I followed the Linux implementation, which waits in an unbounded loop
and theck UART_LSR_BOTH_EMPTY like you said:
for (;;) {
status = serial8250_early_in(port, UART_LSR);
if (uart_lsr_tx_empty(status)) /* checks UART_LSR_BOTH_EMPTY */
break;
cpu_relax();
}
So I didn't have timeout handling
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
Leaving aside the unavailability of udelay() at this point, is a timeout
considered necessary here?
In the way Jan B. mentioned here:
https://lore.kernel.org/xen-devel/20261010031406.2057735-1-zhangzheng@xxxxxxxxxxx/T/#meaf24c3bb039dc6aaecffab752d2b107581816e0
I think we still can have timeout check.
~ Oleksii
|
