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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 30.03.2026 11:57, Roger Pau Monné wrote:
> 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?

Sgtm, ideally also with an explaining sentence in the description.

Jan