Re: [PATCH] serial: Skip over PCIe device which have no quirks (fix AMT regression).

Re: [PATCH] serial: Skip over PCIe device which have no quirks (fix AMT regression).

[Konrad Rzeszutek Wilk]

On Mar 5, 2014 5:46 PM, Aravind Gopalakrishnan <aravind.gopalakrishnan@amd.com> wrote:
>
> On 3/5/2014 3:09 PM, Konrad Rzeszutek Wilk wrote: 
> > See: 
> > 00:16.0 Communication controller: Intel Corporation Cougar Point HECI Controller #1 (rev 04) 
> >          Subsystem: Intel Corporation Device 2008 
> >          Flags: bus master, fast devsel, latency 0, IRQ 11 
> >          Memory at fb12a000 (64-bit, non-prefetchable) [size=16] 
> > 00:16.3 Serial controller: Intel Corporation Cougar Point KT Controller (rev 04) (prog-if 02 [16550]) 
> >          Subsystem: Intel Corporation Device 2008 
> >          Flags: bus master, 66MHz, fast devsel, latency 0, IRQ 17 
> >          I/O ports at f0e0 [size=8] 
> >          Memory at fb129000 (32-bit, non-prefetchable) [size=4K] 
> > 
> > pci 0000:00:16.0: [8086:1c3a] type 00 class 0x078000 
> > pci 0000:00:16.3: [8086:1c3d] type 00 class 0x070002 
> > 
> > And Xen picks 00:16.0 as its console when using 'com1=115200,8n1,amt'. 
> > 
> > This patch fixes it and allows us to use AMT again by zeroing 
> > out io_base to zero. If the scan did not work, the io_base is 
> > set back to a default value. 
>
> If you provide 'amt' on command line => 'skip_amt' = 0 
> Then we'd just return -1 without setting io_base to 0x3f8 due to this 
> condition- 
> if ( !skip_amt ) 
>     return -1; 
>
> (or am I missing something?)

That particular piece of code is executed after the loop. In the loop is where the AMT is found.
>
> > 
> > diff --git a/xen/drivers/char/ns16550.c b/xen/drivers/char/ns16550.c 
> > index 487bf7d..9f87018 100644 
> > --- a/xen/drivers/char/ns16550.c 
> > +++ b/xen/drivers/char/ns16550.c 
> > @@ -829,6 +829,7 @@ pci_uart_config (struct ns16550 *uart, int skip_amt, int bar_idx) 
> >       unsigned int b, d, f, nextf, i; 
> >       u16 vendor, device; 
> >   
> > +    uart->io_base = 0; 
>
> Why not set ns16550.io_base = 0; in arch/x86/setup.c ?

Because then you would break the default COM ports which are not find via PCI scanning.

Say: com1=9600,8n1

Which will use by default the 3f8 io base.
> AFAICT, ns16550.c is the only one that uses ns16550_defaults structure, 
> so it should be safe.. 
>
>
> Thanks, 
> -Aravind. 
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH] serial: Skip over PCIe device which have no quirks (fix AMT regression).

[Aravind Gopalakrishnan]

On 3/5/2014 6:43 PM, Konrad Rzeszutek Wilk wrote:
> On Mar 5, 2014 5:46 PM, Aravind Gopalakrishnan <aravind.gopalakrishnan@amd.com> wrote:
>> On 3/5/2014 3:09 PM, Konrad Rzeszutek Wilk wrote:
>>> This patch fixes it and allows us to use AMT again by zeroing
>>> out io_base to zero. If the scan did not work, the io_base is
>>> set back to a default value.
>> If you provide 'amt' on command line => 'skip_amt' = 0
>> Then we'd just return -1 without setting io_base to 0x3f8 due to this
>> condition-
>> if ( !skip_amt )
>>      return -1;
>>
>> (or am I missing something?)
> That particular piece of code is executed after the loop. In the loop is where the AMT is found.
>>>     +    uart->io_base = 0;
>> Why not set ns16550.io_base = 0; in arch/x86/setup.c ?
> Because then you would break the default COM ports which are not find via PCI scanning.
>
> Say: com1=9600,8n1
>
> Which will use by default the 3f8 io base.

Ah. Okay, I see it now..
Thanks for the clarifications.

-Aravind.

[PATCH] serial: Skip over PCIe device which have no quirks (fix AMT regression).

[Konrad Rzeszutek Wilk]

The "ns16550: Add support for UART present in Broadcom TruManage
capable NetXtreme chips" implies that only devices that are have
an MMIO BAR and are in the quirks table should be processed.

Even the comment at the end says so:

 If we have an io_base, then we succeeded in the lookup

But the code was checking for the !io_base - which is to say if
the io_base was NULL then we would skip scanning.

This means that if we have an communication device followed by
a serial AMT device we would pick the communication device instead
of the MMIO.

See:
00:16.0 Communication controller: Intel Corporation Cougar Point HECI Controller #1 (rev 04)
        Subsystem: Intel Corporation Device 2008
        Flags: bus master, fast devsel, latency 0, IRQ 11
        Memory at fb12a000 (64-bit, non-prefetchable) [size=16]
00:16.3 Serial controller: Intel Corporation Cougar Point KT Controller (rev 04) (prog-if 02 [16550])
        Subsystem: Intel Corporation Device 2008
        Flags: bus master, 66MHz, fast devsel, latency 0, IRQ 17
        I/O ports at f0e0 [size=8]
        Memory at fb129000 (32-bit, non-prefetchable) [size=4K]

pci 0000:00:16.0: [8086:1c3a] type 00 class 0x078000
pci 0000:00:16.3: [8086:1c3d] type 00 class 0x070002

This patch fixes it and allows us to use AMT again.

CC: Aravind Gopalakrishnan <Aravind.Gopalakrishnan@amd.com>
CC: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com>
CC: Thomas Lendacky <Thomas.Lendacky@amd.com>
CC: Jan Beulich <jbeulich@suse.com>
CC: Keir Fraser <keir@xen.org>
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 xen/drivers/char/ns16550.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/xen/drivers/char/ns16550.c b/xen/drivers/char/ns16550.c
index 53e49a1..8a908bd 100644
--- a/xen/drivers/char/ns16550.c
+++ b/xen/drivers/char/ns16550.c
@@ -707,7 +707,7 @@ pci_uart_config (struct ns16550 *uart, int skip_amt, int bar_idx)
                     }
 
                     /* If we have an io_base, then we succeeded in the lookup */
-                    if ( !uart->io_base )
+                    if ( uart->io_base )
                         continue;
                 }
                 /* IO based */
-- 
1.7.7.6

Re: [PATCH] serial: Skip over PCIe device which have no quirks (fix AMT regression).

[Konrad Rzeszutek Wilk]

On Wed, Mar 05, 2014 at 02:45:39PM -0500, Konrad Rzeszutek Wilk wrote:
> The "ns16550: Add support for UART present in Broadcom TruManage
> capable NetXtreme chips" implies that only devices that are have
> an MMIO BAR and are in the quirks table should be processed.
> 
> Even the comment at the end says so:
> 
>  If we have an io_base, then we succeeded in the lookup
> 
> But the code was checking for the !io_base - which is to say if
> the io_base was NULL then we would skip scanning.
> 
> This means that if we have an communication device followed by
> a serial AMT device we would pick the communication device instead
> of the MMIO.
> 
> See:
> 00:16.0 Communication controller: Intel Corporation Cougar Point HECI Controller #1 (rev 04)
>         Subsystem: Intel Corporation Device 2008
>         Flags: bus master, fast devsel, latency 0, IRQ 11
>         Memory at fb12a000 (64-bit, non-prefetchable) [size=16]
> 00:16.3 Serial controller: Intel Corporation Cougar Point KT Controller (rev 04) (prog-if 02 [16550])
>         Subsystem: Intel Corporation Device 2008
>         Flags: bus master, 66MHz, fast devsel, latency 0, IRQ 17
>         I/O ports at f0e0 [size=8]
>         Memory at fb129000 (32-bit, non-prefetchable) [size=4K]
> 
> pci 0000:00:16.0: [8086:1c3a] type 00 class 0x078000
> pci 0000:00:16.3: [8086:1c3d] type 00 class 0x070002
> 
> This patch fixes it and allows us to use AMT again.

Lets ignore this patch. It would break the PCIe card with MMIO as
now we don't setup the rest of the uart configuration for
cards that are found to be on the quirk list.  

> 
> CC: Aravind Gopalakrishnan <Aravind.Gopalakrishnan@amd.com>
> CC: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com>
> CC: Thomas Lendacky <Thomas.Lendacky@amd.com>
> CC: Jan Beulich <jbeulich@suse.com>
> CC: Keir Fraser <keir@xen.org>
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> ---
>  xen/drivers/char/ns16550.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/xen/drivers/char/ns16550.c b/xen/drivers/char/ns16550.c
> index 53e49a1..8a908bd 100644
> --- a/xen/drivers/char/ns16550.c
> +++ b/xen/drivers/char/ns16550.c
> @@ -707,7 +707,7 @@ pci_uart_config (struct ns16550 *uart, int skip_amt, int bar_idx)
>                      }
>  
>                      /* If we have an io_base, then we succeeded in the lookup */
> -                    if ( !uart->io_base )
> +                    if ( uart->io_base )
>                          continue;
>                  }
>                  /* IO based */
> -- 
> 1.7.7.6
> 

Re: [PATCH] serial: Skip over PCIe device which have no quirks (fix AMT regression).

[Konrad Rzeszutek Wilk]

On Wed, Mar 05, 2014 at 02:56:57PM -0500, Konrad Rzeszutek Wilk wrote:
> On Wed, Mar 05, 2014 at 02:45:39PM -0500, Konrad Rzeszutek Wilk wrote:
> > The "ns16550: Add support for UART present in Broadcom TruManage
> > capable NetXtreme chips" implies that only devices that are have
> > an MMIO BAR and are in the quirks table should be processed.
> > 
> > Even the comment at the end says so:
> > 
> >  If we have an io_base, then we succeeded in the lookup
> > 
> > But the code was checking for the !io_base - which is to say if
> > the io_base was NULL then we would skip scanning.
> > 
> > This means that if we have an communication device followed by
> > a serial AMT device we would pick the communication device instead
> > of the MMIO.
> > 
> > See:
> > 00:16.0 Communication controller: Intel Corporation Cougar Point HECI Controller #1 (rev 04)
> >         Subsystem: Intel Corporation Device 2008
> >         Flags: bus master, fast devsel, latency 0, IRQ 11
> >         Memory at fb12a000 (64-bit, non-prefetchable) [size=16]
> > 00:16.3 Serial controller: Intel Corporation Cougar Point KT Controller (rev 04) (prog-if 02 [16550])
> >         Subsystem: Intel Corporation Device 2008
> >         Flags: bus master, 66MHz, fast devsel, latency 0, IRQ 17
> >         I/O ports at f0e0 [size=8]
> >         Memory at fb129000 (32-bit, non-prefetchable) [size=4K]
> > 
> > pci 0000:00:16.0: [8086:1c3a] type 00 class 0x078000
> > pci 0000:00:16.3: [8086:1c3d] type 00 class 0x070002
> > 
> > This patch fixes it and allows us to use AMT again.
> 
> Lets ignore this patch. It would break the PCIe card with MMIO as
> now we don't setup the rest of the uart configuration for
> cards that are found to be on the quirk list.  

And instead look at this one:

>From 5d8e4a05b93879e27f5839c6b1efe82242c51b6f Mon Sep 17 00:00:00 2001
From: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Date: Wed, 5 Mar 2014 14:38:33 -0500
Subject: [PATCH] serial: Skip over PCIe device which have no quirks (fix AMT
 regression).

The "ns16550: Add support for UART present in Broadcom TruManage
capable NetXtreme chips" implies that only devices that are have
an MMIO BAR and are in the quirks table should be processed.

Even the comment at the end says so:

 If we have an io_base, then we succeeded in the lookup

But the code was checking for the !io_base - which is to say if
the io_base was 0 then we would skip scanning. But io_base
always has a value - it is set by 'ns16550_init' to a default
value - so it would never hit the 'continue' path.

This means that if we have an communication device followed by
a serial AMT device we would pick the communication device instead
of the AMT device.

See:
00:16.0 Communication controller: Intel Corporation Cougar Point HECI Controller #1 (rev 04)
        Subsystem: Intel Corporation Device 2008
        Flags: bus master, fast devsel, latency 0, IRQ 11
        Memory at fb12a000 (64-bit, non-prefetchable) [size=16]
00:16.3 Serial controller: Intel Corporation Cougar Point KT Controller (rev 04) (prog-if 02 [16550])
        Subsystem: Intel Corporation Device 2008
        Flags: bus master, 66MHz, fast devsel, latency 0, IRQ 17
        I/O ports at f0e0 [size=8]
        Memory at fb129000 (32-bit, non-prefetchable) [size=4K]

pci 0000:00:16.0: [8086:1c3a] type 00 class 0x078000
pci 0000:00:16.3: [8086:1c3d] type 00 class 0x070002

And Xen picks 00:16.0 as its console when using 'com1=115200,8n1,amt'.

This patch fixes it and allows us to use AMT again by zeroing
out io_base to zero. If the scan did not work, the io_base is
set back to a default value.

CC: Aravind Gopalakrishnan <Aravind.Gopalakrishnan@amd.com>
CC: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com>
CC: Thomas Lendacky <Thomas.Lendacky@amd.com>
CC: Jan Beulich <jbeulich@suse.com>
CC: Keir Fraser <keir@xen.org>
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 xen/drivers/char/ns16550.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/xen/drivers/char/ns16550.c b/xen/drivers/char/ns16550.c
index 487bf7d..9f87018 100644
--- a/xen/drivers/char/ns16550.c
+++ b/xen/drivers/char/ns16550.c
@@ -829,6 +829,7 @@ pci_uart_config (struct ns16550 *uart, int skip_amt, int bar_idx)
     unsigned int b, d, f, nextf, i;
     u16 vendor, device;
 
+    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++ )
     {
-- 
1.7.7.6

Re: [PATCH] serial: Skip over PCIe device which have no quirks (fix AMT regression).

[Aravind Gopalakrishnan]

On 3/5/2014 3:09 PM, Konrad Rzeszutek Wilk wrote:
> See:
> 00:16.0 Communication controller: Intel Corporation Cougar Point HECI Controller #1 (rev 04)
>          Subsystem: Intel Corporation Device 2008
>          Flags: bus master, fast devsel, latency 0, IRQ 11
>          Memory at fb12a000 (64-bit, non-prefetchable) [size=16]
> 00:16.3 Serial controller: Intel Corporation Cougar Point KT Controller (rev 04) (prog-if 02 [16550])
>          Subsystem: Intel Corporation Device 2008
>          Flags: bus master, 66MHz, fast devsel, latency 0, IRQ 17
>          I/O ports at f0e0 [size=8]
>          Memory at fb129000 (32-bit, non-prefetchable) [size=4K]
>
> pci 0000:00:16.0: [8086:1c3a] type 00 class 0x078000
> pci 0000:00:16.3: [8086:1c3d] type 00 class 0x070002
>
> And Xen picks 00:16.0 as its console when using 'com1=115200,8n1,amt'.
>
> This patch fixes it and allows us to use AMT again by zeroing
> out io_base to zero. If the scan did not work, the io_base is
> set back to a default value.

If you provide 'amt' on command line => 'skip_amt' = 0
Then we'd just return -1 without setting io_base to 0x3f8 due to this 
condition-
if ( !skip_amt )
    return -1;

(or am I missing something?)

>
> diff --git a/xen/drivers/char/ns16550.c b/xen/drivers/char/ns16550.c
> index 487bf7d..9f87018 100644
> --- a/xen/drivers/char/ns16550.c
> +++ b/xen/drivers/char/ns16550.c
> @@ -829,6 +829,7 @@ pci_uart_config (struct ns16550 *uart, int skip_amt, int bar_idx)
>       unsigned int b, d, f, nextf, i;
>       u16 vendor, device;
>   
> +    uart->io_base = 0;

Why not set ns16550.io_base = 0; in arch/x86/setup.c ?
AFAICT, ns16550.c is the only one that uses ns16550_defaults structure, 
so it should be safe..


Thanks,
-Aravind.