Re: [PATCH v2 2/3] xen/uart: be more careful with changes to the PCI command register

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Mon, Mar 30, 2026 at 11:09:10AM +0200, Jan Beulich wrote:
> On 30.03.2026 11:06, Roger Pau Monné wrote:
> > On Mon, Mar 30, 2026 at 10:00:05AM +0200, Jan Beulich wrote:
> >> On 27.03.2026 14:54, Roger Pau Monne wrote:
> >>> Read the existing PCI command register and only add the required bits to
> >>> it, as to avoid clearing bits that might be possibly set by the firmware
> >>> already, which might put the device into a non-working state.
> >>>
> >>> Fixes: f2ff5d6628b3 ("ns16550: enable PCI serial card usage")
> >>> Signed-off-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> >>
> >> Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
> >>
> >> I would have preferred if the description mentioned the particular case,
> >> turning this more into a workaround than an apparent bugfix. 
> > 
> > It turns out that the console does seem to work fine, even with memory
> > decoding disabled on the device (as expected).  I've updated the
> > firmware in the meantime, so I'm unsure whether that update has
> > changed the behavior of the device, or it simply was some other
> > instability that was causing the issue in the past.  This SOL AMT
> > device is not reliable at all I'm afraid.
> > 
> >> As mentioned,
> >> us driving the device generally means we're free to do whatever we want to
> >> the command register, as long as resulting device state is consistent
> >> overall (or else we may indeed have a non-working device). Having to keep
> >> memory decoding enabled in order for I/O ports to function is pretty
> >> clearly a bug in the device, and hence us "violating" that requirement
> >> isn't really o bug of ours.
> > 
> > I think given the fragility of some of those SOL devices it's best to
> > limit the number of bits Xen changes, as to having a bigger chances of
> > getting output working.
> 
> That's okay(ish); I merely would wish the patch description was less
> suggesting that Xen was actually buggy.

What about if I change the title to:

xen/uart: avoid clearing PCI command register bits set by the firmware

I think that's clearer and less blameful?

Thanks, Roger.