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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] tty: lockless read of UART_LSR during resume destructively clears receiver error bits
--

commit c43e6e51b74f249795f1e627ed53624daf3d509b
Author: Phil Rosenthal <phil@phil.gs>

serial: 8250_pci: Add support for ASPEED BMC VUARTs over PCIe

This commit adds PCI driver support for ASPEED BMC virtual UARTs, allowing 
them to be used for the kernel console and getty. It polls the ports due to a 
lack of interrupts on the tested board and probes for the VUARTs using custom 
initialization and setup quirks.

Link: https://lore.kernel.org/r/20230823173104.3219128-2-ninad@linux.ibm.com

> 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 AI review · https://sashiko.dev/#/patchset/20261008-aspeed-vuart-v1-1-d2be17c99cd7@phil.gs?part=1