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 04:23:32PM +0000, Jan Beulich wrote:
> >>> On 10.03.14 at 17:13, Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> 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@oracle.com>
	[v1: Also remove the irq override spotted by Jan]
	[v2: Add more details to the commit description]
> 
> Jan
>