On Tue, May 02, 2023 at 12:53:15PM +0200, Jan Beulich wrote:
> On 25.04.2023 16:39, Marek Marczykowski-Górecki wrote:
> > pci_serial_early_init() enables PCI_COMMAND_IO for IO-based UART
> > devices, add setting PCI_COMMAND_MEMORY for MMIO-based UART devices too.
>
> This sentence is odd, as by its grammar it looks to describe the current
> situation only. The respective sentence in v1 did not have this issue.
>
> > --- a/xen/drivers/char/ns16550.c
> > +++ b/xen/drivers/char/ns16550.c
> > @@ -272,7 +272,15 @@ static int cf_check ns16550_getc(struct serial_port
> > *port, char *pc)
> > static void pci_serial_early_init(struct ns16550 *uart)
> > {
> > #ifdef NS16550_PCI
> > - if ( !uart->ps_bdf_enable || uart->io_base >= 0x10000 )
> > + if ( uart->bar )
> > + {
> > + pci_conf_write16(PCI_SBDF(0, uart->ps_bdf[0], uart->ps_bdf[1],
> > + uart->ps_bdf[2]),
> > + PCI_COMMAND, PCI_COMMAND_MEMORY);
> > + return;
> > + }
> > +
> > + if ( !uart->ps_bdf_enable )
> > return;
> >
> > if ( uart->pb_bdf_enable )
>
> While I did suggest using uart->bar, my implication was that the io_base
> check would then remain in place. Otherwise, if I'm not mistaken, MMIO-
> based devices not specified via "com<N>=...,pci" would then wrongly take
> the I/O port path.I don't think MMIO-based devices specified manually have great chance to work anyway (see the commit message), but indeed I shouldn't have broken them even more. > Furthermore - you can't use uart->bar alone here, can you? The field is > set equally for MMIO and port based cards in pci_uart_config(). Right, I'll restore the io_base check. -- Best Regards, Marek Marczykowski-Górecki Invisible Things Lab
signature.asc
Description: PGP signature
