Message ID | 8e96dec5-1575-3561-c61f-ee188c884b9a@suse.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | ns16550: correct name/value pair parsing for PCI port/bridge | expand |
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
--- 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
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).