Re: [Xen-devel] [PATCH v2 2/7] serial: Fix COM1 assumption if pci_uart_config did not find the PCI serial card.

[Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>]

On Mon, Mar 10, 2014 at 04:23:32PM +0000, Jan Beulich wrote:
> >>> On 10.03.14 at 17:13, Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx> 
> >>> wrote:
> > On Mon, Mar 10, 2014 at 09:35:16AM +0000, Jan Beulich wrote:
> >> change is fine, but the description should be updated to say that
> >> this only affects the AMT case.
> > 
> > Yes, let me update the commit description. Since I already had sent an
> > updated patch, let me just copy in the new description:
> > 
> >     serial: Fix COM1 assumption if pci_uart_config did not find the PCI 
> > serial card (AMT one).
> > 
> >     The io_base by default is set to be 0x3f8 for COM1 and 0x2f8 for COM2
> >     in __setup_xen. Then we call 'ns16550_init' which copies those in
> >     the appropiate uart, which then calls 'ns16550_parse_port_config'
> >     to deal with parameter parsing. If the 'pci' or 'amt' parameter
> >     has been specified we further call 'pci_uart_config code' which
> >     scans the PCI bus. If it does not find any relevant devices
> >     it will overwrite io_base with 0x3f8 regardless whether this is
> >     COM1 or COM2. The overwrite is a way to set it back to the
> >     failsafe defaults - except for COM2 of course.
> 
> But this still gives the (false) impression that the overwrite is
> happening in the PCI and AMT cases.
> 
> >     This in theory (as I don't have a machine with two COM ports
> >     readily available) means that if the user specified 'com2=9600,8n1,pci'
> >     or 'com2=9600,8n1,amt' and the device did not have an PCI serial card
> >     (or AMT), instead of using 0x2f8 for the io_base it ends up using 0x3f8
> >     - and we don't get the output on COM2.
> > 
> >     Worst yet, we also uncondionally reset the IRQ value - so we would
> >     never get the proper interrupt when falling back to the legacy
> >     0x3f8 and 0x2f8 COM ports.
> 
> Which isn't _that_ bad - we'd run in polling mode.

Ah, right. How does this update look to you? I tried as best to make it clear
what works and what does not.

        serial: Fix COM1 assumption if pci_uart_config did not find the PCI 
serial card (or AMT one).

        The io_base by default is set to be 0x3f8 for COM1 and 0x2f8 for COM2
        in __setup_xen. Then we call 'ns16550_init' which copies those in
        the appropiate uart, which then calls 'ns16550_parse_port_config'
        to deal with parameter parsing. If the 'pci' or 'amt' parameter
        has been specified we further call 'pci_uart_config code' which
        scans the PCI bus. If it does not find any relevant devices
        (no PCI/PCIe or AMT devices are found) it will overwrite io_base with
        0x3f8 regardless whether this is COM1 or COM2 - but only if 'pci'
        or 'amt' parameter had been specified.

        The overwrite is a way to set it back to the failsafe defaults -
        except for COM2 of course.

        Note again - if an PCI or AMT card is found, this over-write
        will not happen.

        This in theory (as I don't have a machine with two COM ports
        readily available) means that if the user specified 'com2=9600,8n1,pci'
        or 'com2=9600,8n1,amt' and the device did not have an PCI serial card
        (or AMT), instead of using 0x2f8 for the io_base it ends up using 0x3f8
        - and we don't get the output on COM2. If the user had done
        'com2=9600,8n1' we would never get in this path so this bug would
        never manifset itself (because we don't end up scanning for AMT or
        PCI serial devices).

        We also uncondionally reset the IRQ value - so we would never get the
        proper interrupt when falling back to the legacy 0x3f8 and 0x2f8 COM 
ports.
        That is OK - as we would end up using the polling mode - while 
        not the best - it still would work. 

        This bug was introduced with the original AMT support and I cannot
        recall why it was done that way - it is a bug.

        Fix it by saving the original io_base before starting the
        scan of the PCI bus. If we don't find an serial PCI device (because
        we did not exit out of the loop using return) then
        assign the original io_base value back.

        Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>
        [v1: Also remove the irq override spotted by Jan]
        [v2: Add more details to the commit description]
> 
> Jan
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel