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

[Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>]

On Mon, Mar 10, 2014 at 09:35:16AM +0000, Jan Beulich wrote:
> >>> On 07.03.14 at 20:01, Konrad Rzeszutek Wilk <konrad@kernel.org> wrote:
> > 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.
> > 
> > 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'
> > and the device did not have an PCI serial card, instead of using 0x2f8
> > for the io_base it ends up using 0x3f8 - and we don't get the
> > output on COM2.
> > 
> > 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 then
> > assign the original io_base value back.
> 
> While reviewing patch 1 I was specifically thinking of why you didn't
> do this at once. But then I realized that this is done only in the AMT
> case, and was assuming that you, when originally adding AMT
> support, specifically did it that way knowing that if anything it could
> only sit on port 0x3f8. If that wasn't the original intention, the

That (AMT support) was eons ago. I sadly don't remember why it was
done that way :-(

> 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.

	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.

	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@oracle.com>
	[v1: Also remove the irq override spotted by Jan]