Brijesh-Thakkar opened a new issue, #20870:
URL: https://github.com/apache/nuttx/issues/20870
<!--
DRAFT, NOT POSTED. Suggested labels: Area: Drivers, Board: x86_64, bug.
Delete this comment before posting.
-->
**Title:** qemu-intel64:citest_smp (and jumbo): PCI UART1 can never attach;
defconfig mismatch, and the PCI 16550 probe aborts at the first unconfigured
port
## Description
`qemu-intel64:citest_smp` and `qemu-intel64:jumbo` enable a PCI 16550 UART as
`UART1`, but `/dev/ttyS1` is never registered, whatever QEMU PCI serial
device
is attached. There are two independent causes:
1. **The defconfigs are not satisfiable.** They select the QEMU
*single-port*
card (`1b36:0002`) but do not set `CONFIG_16550_PCI_UART1_PORT`, whose
Kconfig default for UART1 is `1`. A single-port card has only port 0, so
no card can match "device `0x0002`, port 1".
2. **`pci_u16550_probe()` gives up at the first unconfigured port.** The
loop
starts at port 0 and does `return -ENODEV` when no `UARTn` is configured
for that port, so a configuration that only uses a later port of a
multi-port card (for example only `UART1` = port 1 of a 2-port card)
cannot work either.
Nothing fails in CI because the CI QEMU command for `citest_smp` does not add
any PCI serial device (`config.yaml`: `-m 2G -smp 4 -cpu max -nographic
-serial mon:stdio`), so the PCI UART is simply never exercised. I found this
while testing an unrelated change.
## Environment
- master at `d0886af95d`, x86_64 host, QEMU 10.2.1 with KVM, GCC 15.2.
- Reproduced on master (no local changes).
## Configuration (as shipped)
`boards/x86_64/qemu/qemu-intel64/configs/citest_smp/defconfig` (the same
`PCI_UART1` lines are in `jumbo/defconfig`):
```
CONFIG_16550_PCI_UART=y
CONFIG_16550_PCI_UART1=y
CONFIG_16550_PCI_UART1_VENDOR=0x1b36
CONFIG_16550_PCI_UART1_DEVICE=0x0002
CONFIG_16550_PCI_UART1_CLOCK=1843200
CONFIG_16550_UART0=y
CONFIG_16550_UART0_SERIAL_CONSOLE=y
CONFIG_16550_UART0_BASE=0x3f8
CONFIG_16550_UART0_IRQ=36
```
There is no `CONFIG_16550_PCI_UART1_PORT` line, so the Kconfig default
applies
(`drivers/serial/Kconfig-pci:116-121`):
```
116 config 16550_PCI_UART1_PORT
117 int "16550 UART1 PCI port"
118 default 1
119 ---help---
120 Port in multi-port device. Starts from 0.
121 Must be set to 0 for single port devices .
```
So the resulting `.config` has `CONFIG_16550_PCI_UART1_PORT=1`.
## Steps to reproduce
```
./tools/configure.sh qemu-intel64:citest_smp
make -j
qemu-system-x86_64 -cpu host -enable-kvm -m 2G -smp 4 -kernel nuttx \
-nographic -serial mon:stdio \
-chardev null,id=c1 -device pci-serial,chardev=c1
nsh> ls /dev
```
(`citest_smp` has `CONFIG_ARCH_PVHBOOT=y`, so `-kernel` works.)
## Expected
`UART1` is the PCI UART, so `/dev/ttyS1` exists:
```
/dev:
console
null
pci
random
telnet
ttyS0
ttyS1
zero
```
## Actual
```
/dev:
console
null
pci
random
telnet
ttyS0
zero
```
The kernel log shows that the driver probes the device but never registers a
port: there are `pci_u16550_probe: Enabled bus mastering` and `Enabled memory
resources` lines, and no `pci_u16550_register: Register /dev/ttyS1`. The
result is identical with `-device pci-serial`, `pci-serial-2x` and
`pci-serial-4x`.
## Analysis
`drivers/serial/uart_pci_16550.c` at `d0886af95d`:
```
698 /* Get available ports for this device */
699
700 ports = type->ports * (PCI_FUNC(dev->devfn) + 1);
701 port = type->ports * PCI_FUNC(dev->devfn);
702
703 /* Register all ports for this device */
704
705 for (; port < ports; port++)
706 {
707 /* Get port address */
708
709 base += type->portincr * port;
...
714 for (i = 0; i < nitems(g_pci_u16550_dev); i++)
715 {
...
719 if ((priv->vendor == PCI_ANY_ID ||
720 priv->vendor == dev->vendor) &&
721 (priv->device == PCI_ANY_ID ||
722 priv->device == dev->device) &&
723 priv->port == port)
724 {
725 break;
726 }
...
732 /* Not found */
733
734 if (udev == NULL)
735 {
736 return -ENODEV;
737 }
```
- Cause 1: the QEMU device ids in `g_pci_u16550_id_table` are `1b36:0002`
(1 port), `1b36:0003` (2 ports) and `1b36:0004` (4 ports). With
`DEVICE=0x0002` the card has one port (port 0), but `UART1` asks for port
1,
so line 723 never matches (`pci-serial-2x`/`4x` do not match at all,
because their device ids are `0x0003`/`0x0004`).
- Cause 2: when no instance is configured for the current port,
line 736 returns from the whole probe instead of moving to the next port,
so
a configuration that has no `UART0` but has `UART1` (port 1) never reaches
port 1, even on a card that has it.
- Related, not needed for this report: line 709 accumulates (`base +=`)
instead of computing the port address from the BAR base, so for a card with
`portincr != 0` and 3 or more ports the third and fourth ports get offsets
24 and 48 instead of 16 and 24 (with `portincr = 8`). I only checked this
by reading the code; I did not test it on a 4-port card.
## What I verified
All builds are `citest_smp` with only the stated change; "probe fix" is the
patch below. `ttyS1` = `/dev/ttyS1` was registered.
| Build | `UART1` config | QEMU device | `ttyS1` |
|---|---|---|---|
| master | as shipped (`0x0002`, port 1 by default) | `pci-serial` | no |
| master | as shipped | `pci-serial-2x` | no |
| master | as shipped | `pci-serial-4x` | no |
| master | `PORT=0` added (`0x0002`) | `pci-serial` | **yes** |
| master | `DEVICE=0x0003`, `PORT=1` | `pci-serial-2x` | no |
| master + probe fix | as shipped | `pci-serial`, `-2x`, `-4x` | no |
| master + probe fix | `DEVICE=0x0003`, `PORT=1` | `pci-serial-2x` | **yes**
|
So cause 1 alone explains the `citest_smp`/`jumbo` symptom, and cause 2 is a
separate bug that only shows up for a card whose first port is not
configured.
## Proposed fix
1. **Defconfigs** (`citest_smp`, `jumbo`): add
`CONFIG_16550_PCI_UART1_PORT=0`, which matches the single-port
`1b36:0002` card they already name. Optionally also add
`-device pci-serial` to the CI `exec_args` so the port is actually tested.
2. **Driver**: do not abort the probe at the first unconfigured port, and
compute each port address from the BAR base:
```diff
@@ pci_u16550_probe()
uintptr_t base = 0;
+ uintptr_t portbase;
+ unsigned int registered = 0;
@@
- base += type->portincr * port;
+ portbase = base + type->portincr * port;
@@
- /* Not found */
+ /* Not configured: skip this port, others may still be configured */
if (udev == NULL)
{
- return -ENODEV;
+ continue;
}
@@
- ret = pci_u16550_initialize(priv, type, base, dev, mmio);
+ ret = pci_u16550_initialize(priv, type, portbase, dev, mmio);
@@
ret = pci_u16550_register(udev);
if (ret < 0)
{
return ret;
}
+
+ registered++;
}
- return OK;
+ /* Fail only if no port of this device is configured */
+
+ return registered > 0 ? OK : -ENODEV;
```
This keeps `-ENODEV` for a device none of whose ports is configured. It
builds
and passes `tools/checkpatch.sh`; I booted it only on QEMU (table above). I
have not tested it on an AX99100 card.
I can send a PR for the defconfig part, the driver part, or both, whichever
the maintainers prefer.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
