Re: [PATCH v1] xen/drivers: ns16550: Fix the return logic for pci_uart_config()

[Jan Beulich <jbeulich@xxxxxxxx>]

On 07.07.2023 13:35, Ayan Kumar Halder wrote:
> --- a/xen/drivers/char/ns16550.c
> +++ b/xen/drivers/char/ns16550.c
> @@ -1342,13 +1342,9 @@ pci_uart_config(struct ns16550 *uart, bool_t skip_amt, 
> unsigned int idx)
>          }
>      }
>  
> -    if ( !skip_amt )
> -        return -1;

This special case probably needs retaining in the new model (with an
altered return value), such that ...

> -    /* No AMT found, fallback to the defaults. */
>      uart->io_base = orig_base;
>  
> -    return 0;
> +    return -ENODEV;
>  }
>  
>  static void enable_exar_enhanced_bits(const struct ns16550 *uart)
> @@ -1527,13 +1523,13 @@ static bool __init parse_positional(struct ns16550 
> *uart, char **str)
>  #ifdef CONFIG_HAS_PCI
>          if ( strncmp(conf, "pci", 3) == 0 )
>          {
> -            if ( pci_uart_config(uart, 1/* skip AMT */, uart - ns16550_com) )
> +            if ( !pci_uart_config(uart, 1/* skip AMT */, uart - ns16550_com) 
> )
>                  return true;
>              conf += 3;
>          }
>          else if ( strncmp(conf, "amt", 3) == 0 )
>          {
> -            if ( pci_uart_config(uart, 0, uart - ns16550_com) )
> +            if ( !pci_uart_config(uart, 0, uart - ns16550_com) )
>                  return true;
>              conf += 3;
>          }

... e.g. here you don't suddenly change behavior in unintended ways.
Prior to your change the earlier of the return paths was impossible
to be taken. That's likely wrong, but you now returning in the success
case can't be correct either: Further items may need parsing, first
and foremost the IRQ to use.

Jan

> @@ -1642,13 +1638,17 @@ static bool __init parse_namevalue_pairs(char *str, 
> struct ns16550 *uart)
>          case device:
>              if ( strncmp(param_value, "pci", 3) == 0 )
>              {
> -                pci_uart_config(uart, 1/* skip AMT */, uart - ns16550_com);
> -                dev_set = true;
> +                if ( !pci_uart_config(uart, 1/* skip AMT */, uart - 
> ns16550_com) )
> +                    dev_set = true;
> +                else
> +                    return false;
>              }
>              else if ( strncmp(param_value, "amt", 3) == 0 )
>              {
> -                pci_uart_config(uart, 0, uart - ns16550_com);
> -                dev_set = true;
> +                if ( !pci_uart_config(uart, 0, uart - ns16550_com) )
> +                    dev_set = true;
> +                else
> +                    return false;
>              }
>              break;
>