Re: [PATCH] PCI uart: fix boot hang, and second S3 resume inactive timer list corruption

[Tomasz Wroblewski <tomasz.wroblewski@citrix.com>]

>>> And this was observed with 4.4-unstable? I'm asking because I
>>> would at a first glance have thought that taking care of this
>>> ought to be a desirable side effect of calling pci_hide_device().
>> This was observed with stable 4.3 - it seems to be doing the
>> pci_hide_device as well, so I don't think this affects, or was it
>> bugfixed later? I'm not entirely sure how is pci_hide_device supposed to
>> work though - in my dom0, on 4.3, I am seeing the pci serial card used
>> by xen console, so maybe it is bugged? (or i misunderstand it).
> Wait, yes, pci_ro_device() is what would be needed to drop
> Dom0 writes to the device's config space. But we don't want
> this if at all possible, as there may be other devices (more
> serial ports and/or one or more parallel ports) on the same
> card, and we want to allow Dom0 to drive those.
>
> Nevertheless, the approach of your patch in simply giving up
> the device (even if only termporarily) looks questionable to me
> We'd rather need to restore full access to it I would think. But
> yes, this hypervisor and Dom0 playing with the same device is
> sort of a gray area.
Restore ioport access at the start of poll routine (if not on) and 
disable it again at the end (if was not on)? I might do that (if you 
really prefer), but intuitively that seems more likely to produce side 
effects in dom0 kernel than skipping a poll in xen

>>>> +static int ns16550_ioport_invalid(struct ns16550 *uart)
>>>> +{
>>>> +    return (((unsigned char)ns_read_reg(uart, UART_IER)) == 0xff);
>>>> +}
>>> Why checking just one register is sufficient when originally
>>>
>>>> -static int ns16550_ioport_invalid(struct ns16550 *uart)
>>>> -{
>>>> -    return ((((unsigned char)ns_read_reg(uart, UART_LSR)) == 0xff)&&
>>>> -            (((unsigned char)ns_read_reg(uart, UART_MCR)) == 0xff)&&
>>>> -            (((unsigned char)ns_read_reg(uart, UART_IER)) == 0xff)&&
>>>> -            (((unsigned char)ns_read_reg(uart, UART_IIR)) == 0xff)&&
>>>> -            (((unsigned char)ns_read_reg(uart, UART_LCR)) == 0xff));
>>>> -}
>>> we checked five also needs some better explanation.
>> I believe it's enough to test IER register since it contains 3 reserved
>> bits which are always 0 during normal operation, therefore the condition
>> will never hit then. Made this as a mini optimisation since this
>> function would now be called more frequently.
> I assumed it was something like this. But that needs to be said in
> the patch description.
Yeah, I did mention it in the desc though, to quote: "Amended 
ns16550_ioport_invalid function to only check IER register, which 
contains three reservered (always 0) bits, therefore it's sufficient for 
this test", thought that was enough

> Jan
>