Re: [PATCH] ns16550: add "auto" to scan PCI only when the default base has no UART

[Jan Beulich <jbeulich@xxxxxxxx>]

On 13.10.2026 22:49, Benjamin Leggett wrote:
> @@ -737,9 +739,9 @@ static int __init check_existence(struct ns16550 *uart)
>       * if we're getting MMIO UARTs, the arch code knows what it's doing.
>       */
>      if ( uart->io_base >= 0x10000 )
> -        return 1;
> +        return -1;
>  #else
> -    return 1; /* Everything is MMIO */
> +    return -1; /* Everything is MMIO */
>  #endif

While this addresses my main concern towards false positives, the rest of
this function is still weak enough that I'm unconvinced it can be used for
the new purpose here. I wonder what others think.

> @@ -1785,6 +1801,7 @@ static void __init ns16550_parse_port_config(
>  {
>      char com_console_options[128];
>      char *str;
> +    int present;
>  
>      /* No user-specified configuration? */
>      if ( (conf == NULL) || (*conf == '\0') )
> @@ -1818,8 +1835,29 @@ static void __init ns16550_parse_port_config(
>          PARSE_ERR("%d stop bits are unsupported.", uart->stop_bits);
>      if ( uart->io_base == 0 )
>          PARSE_ERR("I/O base address must be specified.");
> -    if ( !check_existence(uart) )
> -        PARSE_ERR("16550-compatible serial UART not present");
> +    present = check_existence(uart);
> +#ifdef CONFIG_HAS_PCI
> +    if ( uart->pci_fallback && present < 0 )
> +        PARSE_ERR("\"auto\" needs a base that can be probed");

This, imo, is a misleading / unhelpful message. "auto" sits in the place where
a base would be specified, i.e. suggesting that a base is needed when auto-
detection was requested is odd. Furthermore, it is an implementation shortcoming
that devices accessed via MMIO aren't probed. Maybe "\"auto\" doesn't cope with
MMIO-based devices" or some such?

Yet then - this isn't about the to-be-auto-probed device anyway, it's about the
data that was put in by early arch startup code.

> +#endif
> +    if ( !present )
> +    {
> +#ifdef CONFIG_HAS_PCI
> +        if ( uart->pci_fallback )
> +            pci_uart_config(uart, 1 /* skip AMT */, uart - ns16550_com);
> +
> +        /*
> +         * A failed scan puts back the rejected base, and check_existence()
> +         * can't test an MMIO BAR, so only ->param says that a device was
> +         * found.
> +         */
> +        if ( uart->pci_fallback && uart->param && check_existence(uart) )

Don't you want to check for > 0 here? Yet then the comment looks to suggest
you deliberately want to accept -1 here as well. (The comment wants further
adjustment anyway, as "... only ->param says ..." contradicts your use of
check_existence().)

> +            printk("ns16550: com%zd: no UART at default base, using %pp\n",
> +                   uart - ns16550_com + 1, &uart->pci_device);
> +        else
> +#endif
> +            PARSE_ERR("16550-compatible serial UART not present");
> +    }
>  
>      /* Register with generic serial driver. */
>      serial_register_uart(uart - ns16550_com, &ns16550_driver, uart);

I think this wants doing with just a single #ifdef, it at all possible. Not
having actually tried it in practice (and leaving aside the other questions
above), something like

    present = check_existence(uart);
#ifdef CONFIG_HAS_PCI
    if ( present <= 0 && uart->pci_fallback )
    {
        if ( present < 0 )
            PARSE_ERR("\"auto\" needs a base that can be probed");
        else
        {
            pci_uart_config(uart, 1 /* skip AMT */, uart - ns16550_com);

            /*
             * A failed scan puts back the rejected base, and check_existence()
             * can't test an MMIO BAR, so only ->param says that a device was
             * found.
             */
            if ( uart->param && check_existence(uart) )
            {
                printk("ns16550: com%zd: no UART at default base, using %pp\n",
                       uart - ns16550_com + 1, &uart->pci_device);
                present = 1;
            }
        }
    }
#endif
    if ( !present )
        PARSE_ERR("16550-compatible serial UART not present");

may do.

Jan