Re: [PATCH v2] ns16550: enable memory decoding on MMIO-based PCI console card

[Marek Marczykowski-Górecki <marmarek@xxxxxxxxxxxxxxxxxxxxxx>]

On Wed, Apr 26, 2023 at 10:24:28AM +0200, Jan Beulich wrote:
> On 26.04.2023 09:48, Roger Pau Monné wrote:
> > On Tue, Apr 25, 2023 at 04:39:02PM +0200, Marek Marczykowski-Górecki wrote:
> >> --- 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);
> > 
> > Don't you want to read the current command register first and just
> > or PCI_COMMAND_MEMORY?
> > 
> > I see that for IO decoding we already do it this way, but I'm not sure
> > whether it could cause issues down the road by unintentionally
> > disabling command flags.
> 
> Quite some time ago I asked myself the same question when seeing that
> code, but I concluded that perhaps none of the bits are really sensible
> to be set for a device as simple as a serial one. I'm actually thinking
> that for such a device to be used during early boot, it might even be
> helpful if bits like PARITY or SERR get cleared. (Of course if any of
> that was really the intention of the change introducing that code, it
> should have come with a code comment.)

I have mirrored the approach used for IO ports, with similar thinking,
and I read the above as you agree. Does it mean this patch is okay,
or you request some change here?

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab

Attachment: signature.asc
Description: PGP signature