First of all these were inverted: "bridge=" caused the port coordinates
to be established, while "port=" controlled the bridge coordinates. And
then the error messages being identical also wasn't helpful. While
correcting this also move both case blocks close together.

Fixes: 97fd49a7e074 ("ns16550: add support for UART parameters to be specifed with name-value pairs")
Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
pci_serial_early_init() only dealing with I/O port variants is also
bogus, but that's a separate topic (which arguably would better be taken
care of by someone actually in the position of testing it). Additionally
I think it would be a good idea if the function left the bridge windows
alone if they're already configured suitably (perhaps with a wider
range).

_ns16550_resume() also doesn't care about configuring the bridge
correctly.

It further looks to be a shortcoming that pci_uart_config() doesn't
determine the bridge behind which the selected device may be located
(let alone a hierarchy of bridges).

--- a/xen/drivers/char/ns16550.c
+++ b/xen/drivers/char/ns16550.c
@@ -1631,13 +1631,6 @@ static bool __init parse_namevalue_pairs
             break;
 
 #ifdef CONFIG_HAS_PCI
-        case bridge_bdf:
-            if ( !parse_pci(param_value, NULL, &uart->ps_bdf[0],
-                            &uart->ps_bdf[1], &uart->ps_bdf[2]) )
-                PARSE_ERR_RET("Bad port PCI coordinates\n");
-            uart->ps_bdf_enable = true;
-            break;
-
         case device:
             if ( strncmp(param_value, "pci", 3) == 0 )
             {
@@ -1652,9 +1645,16 @@ static bool __init parse_namevalue_pairs
             break;
 
         case port_bdf:
+            if ( !parse_pci(param_value, NULL, &uart->ps_bdf[0],
+                            &uart->ps_bdf[1], &uart->ps_bdf[2]) )
+                PARSE_ERR_RET("Bad port PCI coordinates\n");
+            uart->ps_bdf_enable = true;
+            break;
+
+        case bridge_bdf:
             if ( !parse_pci(param_value, NULL, &uart->pb_bdf[0],
                             &uart->pb_bdf[1], &uart->pb_bdf[2]) )
-                PARSE_ERR_RET("Bad port PCI coordinates\n");
+                PARSE_ERR_RET("Bad bridge PCI coordinates\n");
             uart->pb_bdf_enable = true;
             break;
 #endif

On 29/03/2023 7:50 am, Jan Beulich wrote:
> First of all these were inverted: "bridge=" caused the port coordinates
> to be established, while "port=" controlled the bridge coordinates. And
> then the error messages being identical also wasn't helpful. While
> correcting this also move both case blocks close together.
>
> Fixes: 97fd49a7e074 ("ns16550: add support for UART parameters to be specifed with name-value pairs")
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Acked-by: Andrew Cooper <andrew.cooper3@citirx.com>

On 29.03.2023 09:19, Andrew Cooper wrote:
> On 29/03/2023 7:50 am, Jan Beulich wrote:
>> First of all these were inverted: "bridge=" caused the port coordinates
>> to be established, while "port=" controlled the bridge coordinates. And
>> then the error messages being identical also wasn't helpful. While
>> correcting this also move both case blocks close together.
>>
>> Fixes: 97fd49a7e074 ("ns16550: add support for UART parameters to be specifed with name-value pairs")
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> Acked-by: Andrew Cooper <andrew.cooper3@citirx.com>

Thanks; I'll assume you don't mind if I correct the domain name spelling
while applying the tag.

Jan