xen-devel.lists.xenproject.org archive mirror
 help / color / mirror / Atom feed
From: Konrad Rzeszutek Wilk <konrad@kernel.org>
To: andrew.cooper3@citrix.com, aravind.gopalakrishnan@amd.com,
	xen-devel@lists.xenproject.org, jbeulich@suse.com, keir@xen.org
Subject: [PATCH v3 2/6] serial: Fix COM1 assumption if pci_uart_config did not find the AMT card.
Date: Wed, 12 Mar 2014 11:27:48 -0400	[thread overview]
Message-ID: <1394638072-11331-3-git-send-email-konrad.wilk@oracle.com> (raw)
In-Reply-To: <1394638072-11331-1-git-send-email-konrad.wilk@oracle.com>

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 appropriate uart, which then calls 'ns16550_parse_port_config'
to deal with parameter parsing. If the 'amt' parameter has been
specified we further call 'pci_uart_config code' which scans the PCI bus.

If it does not find the AMT device it would overwrite the io_base with
0x3f8 regardless whether this is COM1 or COM2 - but only if 'amt'
parameter had been specified.

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

Note again - if an 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,amt'
and the device did not have an AMT serial device, 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 manifest itself
(because we don't end up scanning for the AMT device).

We also unconditionally 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.

Lastly the clock_hz is also set to the default one (UART_CLOCK_HZ,
which is the same for legacy COM1 and COM2 ports)- that is strictly
not a bug, but it is redundant and not needed.

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]
Reviewed-by: Jan Beulich <jbeulich@suse.com>
---
 xen/drivers/char/ns16550.c |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/xen/drivers/char/ns16550.c b/xen/drivers/char/ns16550.c
index 2fded08..d5fe689 100644
--- a/xen/drivers/char/ns16550.c
+++ b/xen/drivers/char/ns16550.c
@@ -612,10 +612,11 @@ static int __init
 pci_uart_config (struct ns16550 *uart, int skip_amt, int bar_idx)
 {
     uint32_t bar, bar_64 = 0, len, len_64;
-    u64 size, mask;
+    u64 size, mask, orig_base;
     unsigned int b, d, f, nextf, i;
     u16 vendor, device;
 
+    orig_base = uart->io_base;
     uart->io_base = 0;
     /* NB. Start at bus 1 to avoid AMT: a plug-in card cannot be on bus 0. */
     for ( b = skip_amt ? 1 : 0; b < 0x100; b++ )
@@ -747,9 +748,8 @@ pci_uart_config (struct ns16550 *uart, int skip_amt, int bar_idx)
     if ( !skip_amt )
         return -1;
 
-    uart->io_base = 0x3f8;
-    uart->irq = 0;
-    uart->clock_hz  = UART_CLOCK_HZ;
+    /* No AMT found, fallback to the defaults. */
+    uart->io_base = orig_base;
 
     return 0;
 }
-- 
1.7.7.6

  parent reply	other threads:[~2014-03-12 15:27 UTC|newest]

Thread overview: 7+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2014-03-12 15:27 [PATCH v3]/[GIT PULL] Enable serial output for Oxford Semiconductor PCIe cards and fix bugs Konrad Rzeszutek Wilk
2014-03-12 15:27 ` [PATCH v3 1/6] serial: Skip over PCIe device which have no quirks (fix AMT regression) Konrad Rzeszutek Wilk
2014-03-12 15:27 ` Konrad Rzeszutek Wilk [this message]
2014-03-12 15:27 ` [PATCH v3 3/6] serial: Support OXPCIe952 aka Oxford Semiconductor Ltd Device c138 (1415:c138) Konrad Rzeszutek Wilk
2014-03-12 15:27 ` [PATCH v3 4/6] serial: Seperate the PCI device ids and parameters (v1) Konrad Rzeszutek Wilk
2014-03-12 15:27 ` [PATCH v3 5/6] pci: Use #defines for PCI vendors Konrad Rzeszutek Wilk
2014-03-12 15:27 ` [PATCH v3 6/6] serial: Expand the PCI serial quirks for OXPCIe200 and OXPCIe952 1 Native UART Konrad Rzeszutek Wilk

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=1394638072-11331-3-git-send-email-konrad.wilk@oracle.com \
    --to=konrad@kernel.org \
    --cc=andrew.cooper3@citrix.com \
    --cc=aravind.gopalakrishnan@amd.com \
    --cc=jbeulich@suse.com \
    --cc=keir@xen.org \
    --cc=xen-devel@lists.xenproject.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).