Re: [PATCH] serial: 8250_pci: Add support for ASPEED BMC VUARTs over PCIe

[Phil Rosenthal]

> On Oct 8, 2026, at 9:38 AM, sashiko-bot@xxxxxxxxxx wrote:
> 
>> 
>> diff --git a/drivers/tty/serial/8250/8250_pci.c b/drivers/tty/serial/8250/8250_pci.c
>> index 6e53d6d72a8e2..8c243c4fd58a9 100644
>> --- a/drivers/tty/serial/8250/8250_pci.c
>> +++ b/drivers/tty/serial/8250/8250_pci.c
> 
> [ ... ]
> 
>> +/*
>> + * A live 16550 can read LSR as 0xff, but never IIR, whose bits 4-5 are
>> + * reserved. Only read IIR when LSR looks absent: reading IIR clears a
>> + * pending THRI, and this also runs on resume while the port may be in use.
>> + */
>> +static bool pci_aspeed_vuart_present(void __iomem *base, unsigned int offset)
>> +{
>> + void __iomem *regs = base + offset;
>> +
>> + return readl(regs + (UART_LSR << PCI_ASPEED_VUART_REGSHIFT)) != ~0U ||
>> +        readl(regs + (UART_IIR << PCI_ASPEED_VUART_REGSHIFT)) != ~0U;
>> +}
> 
> [Severity: Medium]
> Does this lockless read of UART_LSR destructively clear receiver error bits?
> 
> While the comment correctly notes that reading UART_IIR clears a pending THRI,
> reading the Line Status Register (UART_LSR) on a 16550-compatible UART will
> instantly clear the receiver error flags (Overrun, Parity, Framing, Break).
> 
>> +
>> +static int pci_aspeed_vuart_init(struct pci_dev *dev)
>> +{
>> + void __iomem *base;
>> + int i, n = 0;
>> +
>> + if (!(pci_resource_flags(dev, PCI_ASPEED_VUART_BAR) & IORESOURCE_MEM) ||
>> +     pci_resource_len(dev, PCI_ASPEED_VUART_BAR) < SZ_4K)
>> + return -ENODEV;
>> +
>> + base = pci_iomap_range(dev, PCI_ASPEED_VUART_BAR, 0, SZ_4K);
>> + if (!base)
>> + return -ENOMEM;
>> +
>> + for (i = 0; i < ARRAY_SIZE(pci_aspeed_vuart_io); i++)
>> + if (pci_aspeed_vuart_present(base, pci_aspeed_vuart_offset(i)))
>> + n++;
> 
> [Severity: Medium]
> Because pci_aspeed_vuart_init() is mapped to quirk->init, won't it be invoked
> by pciserial_resume_ports() during system resume?
> 
> If the port receives malformed data or experiences FIFO overruns while the
> system is resuming, will this iteration clear those error flags locklessly
> before the driver's interrupt handler or polling routine can process them?

  Sashiko pointed out that the presence test reads LSR, which clears the
  receiver error bits, and that pci_aspeed_vuart_init() also runs from
  pciserial_resume_ports() while the port may be in use.  That is right,
  and the IIR read has a similar side effect.

     v2 tests only MCR instead: reading it has no side effects, and its bits
  6-7 are reserved as zero, so a present VUART never reads it as all
  ones.  I have tested that on the same board and will send it after any
  other review comments, or now if you prefer.

Best Regards,
-Phil Rosenthal