Re: [XEN v4 04/11] xen/drivers: ns16550: Use paddr_t for io_base/io_size

[Jan Beulich <jbeulich@xxxxxxxx>]

On 21.03.2023 15:03, Ayan Kumar Halder wrote:
> @@ -1163,10 +1163,16 @@ static const struct ns16550_config __initconst 
> uart_config[] =
>      },
>  };
>  
> +#define PARSE_ERR_RET(_f, _a...)             \
> +    do {                                     \
> +        printk( "ERROR: " _f "\n" , ## _a ); \
> +        return false;                        \
> +    } while ( 0 )

You can't really re-use this construct unchanged (and perhaps it's also
not worth changing for this single use that you need): Note the "return
false", which ...

>  static int __init
>  pci_uart_config(struct ns16550 *uart, bool_t skip_amt, unsigned int idx)

... for a function returning "int" is equivalent to "return 0", which
is kind of a success indicator here. Whatever adjustment you make
needs to be in line with (at least) the two callers checking the
return value (the other two not doing so is suspicious, but then the
way the return values are used is somewhat odd, too).

> @@ -1235,6 +1241,8 @@ pci_uart_config(struct ns16550 *uart, bool_t skip_amt, 
> unsigned int idx)
>                  /* MMIO based */
>                  if ( param->mmio && !(bar & PCI_BASE_ADDRESS_SPACE_IO) )
>                  {
> +                    uint64_t pci_uart_io_base;
> +
>                      pci_conf_write32(PCI_SBDF(0, b, d, f),
>                                       PCI_BASE_ADDRESS_0 + bar_idx*4, ~0u);
>                      len = pci_conf_read32(PCI_SBDF(0, b, d, f),
> @@ -1259,8 +1267,14 @@ pci_uart_config(struct ns16550 *uart, bool_t skip_amt, 
> unsigned int idx)
>                      else
>                          size = len & PCI_BASE_ADDRESS_MEM_MASK;
>  
> -                    uart->io_base = ((u64)bar_64 << 32) |
> -                                    (bar & PCI_BASE_ADDRESS_MEM_MASK);
> +                    pci_uart_io_base = ((u64)bar_64 << 32) |

As you touch this code, please be so kind and also switch to using
uint64_t here.

Also why do you change parse_positional() but not (also)
parse_namevalue_pairs()?

Jan