diff mbox series

lspci: Don't report PCIe link downgrades for downstream ports

Message ID 20210318170244.151240-1-helgaas@kernel.org (mailing list archive)
State Not Applicable
Headers show
Series lspci: Don't report PCIe link downgrades for downstream ports | expand

Commit Message

Bjorn Helgaas March 18, 2021, 5:02 p.m. UTC 
From: Bjorn Helgaas <bhelgaas@google.com>

After b47b5bd408e1 ("lspci: Report if the PCIe link speed/width is full or
downgraded"), we report "downgraded" or "ok" for PCIe links operating at
lower speed or width than they're capable of:

  LnkCap: Port #1, Speed 8GT/s, Width x1, ASPM L1, Exit Latency L1 <16us
  LnkSta: Speed 5GT/s (downgraded), Width x1 (ok)

Previously we did this for both ends of the link, but I don't think it's
very useful for Downstream Ports (the upstream end of the link) because we
claim the link is downgraded even if (1) there's no device on the other end
or (2) the other device doesn't support anything faster/wider.

Drop the "downgraded" reporting for Downstream Ports.  If there is a device
below, we'll still complain at that end if it supports a faster/wider link
than is available.

Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 ls-caps.c | 17 ++++++++++-------
 1 file changed, 10 insertions(+), 7 deletions(-)

Comments

Bjorn Helgaas March 18, 2021, 5:11 p.m. UTC | #1 
On Thu, Mar 18, 2021 at 12:02:44PM -0500, Bjorn Helgaas wrote:
> From: Bjorn Helgaas <bhelgaas@google.com>
> 
> After b47b5bd408e1 ("lspci: Report if the PCIe link speed/width is full or
> downgraded"), we report "downgraded" or "ok" for PCIe links operating at
> lower speed or width than they're capable of:
> 
>   LnkCap: Port #1, Speed 8GT/s, Width x1, ASPM L1, Exit Latency L1 <16us
>   LnkSta: Speed 5GT/s (downgraded), Width x1 (ok)
> 
> Previously we did this for both ends of the link, but I don't think it's
> very useful for Downstream Ports (the upstream end of the link) because we
> claim the link is downgraded even if (1) there's no device on the other end
> or (2) the other device doesn't support anything faster/wider.
> 
> Drop the "downgraded" reporting for Downstream Ports.  If there is a device
> below, we'll still complain at that end if it supports a faster/wider link
> than is available.
> 
> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> ---
>  ls-caps.c | 17 ++++++++++-------
>  1 file changed, 10 insertions(+), 7 deletions(-)
> 
> diff --git a/ls-caps.c b/ls-caps.c
> index db56556..dd17c6b 100644
> --- a/ls-caps.c
> +++ b/ls-caps.c
> @@ -758,13 +758,16 @@ static char *link_speed(int speed)
>      }
>  }
>  
> -static char *link_compare(int sta, int cap)
> +static char *link_compare(int type, int sta, int cap)
>  {
> +  if ((type == PCI_EXP_TYPE_ROOT_PORT) || (type == PCI_EXP_TYPE_DOWNSTREAM) ||
> +      (type == PCI_EXP_TYPE_PCIE_BRIDGE))
> +    return "";
>    if (sta < cap)
> -    return "downgraded";
> +    return " (downgraded)";
>    if (sta > cap)
> -    return "strange";
> -  return "ok";
> +    return " (strange)";
> +  return " (ok)";

Personally I would probably just return "" by default and drop the
"(ok)" notes because they don't really seem necessary.  But either way
is OK.

>  }
>  
>  static char *aspm_support(int code)
> @@ -837,11 +840,11 @@ static void cap_express_link(struct device *d, int where, int type)
>    w = get_conf_word(d, where + PCI_EXP_LNKSTA);
>    sta_speed = w & PCI_EXP_LNKSTA_SPEED;
>    sta_width = (w & PCI_EXP_LNKSTA_WIDTH) >> 4;
> -  printf("\t\tLnkSta:\tSpeed %s (%s), Width x%d (%s)\n",
> +  printf("\t\tLnkSta:\tSpeed %s%s, Width x%d%s\n",
>  	link_speed(sta_speed),
> -	link_compare(sta_speed, cap_speed),
> +	link_compare(type, sta_speed, cap_speed),
>  	sta_width,
> -	link_compare(sta_width, cap_width));
> +	link_compare(type, sta_width, cap_width));
>    printf("\t\t\tTrErr%c Train%c SlotClk%c DLActive%c BWMgmt%c ABWMgmt%c\n",
>  	FLAG(w, PCI_EXP_LNKSTA_TR_ERR),
>  	FLAG(w, PCI_EXP_LNKSTA_TRAIN),
> -- 
> 2.25.1
>
Matthew Wilcox March 18, 2021, 5:33 p.m. UTC | #2 
On Thu, Mar 18, 2021 at 12:02:44PM -0500, Bjorn Helgaas wrote:
> Drop the "downgraded" reporting for Downstream Ports.  If there is a device
> below, we'll still complain at that end if it supports a faster/wider link
> than is available.

This makes sense, but I think we should still report if training has
gone horribly wrong.  Maybe something like this ...

> +++ b/ls-caps.c
> @@ -758,13 +758,16 @@ static char *link_speed(int speed)
>      }
>  }
>  
> -static char *link_compare(int sta, int cap)
> +static char *link_compare(int type, int sta, int cap)
>  {
> +  if ((type == PCI_EXP_TYPE_ROOT_PORT) || (type == PCI_EXP_TYPE_DOWNSTREAM) ||
> +      (type == PCI_EXP_TYPE_PCIE_BRIDGE))
> +    return "";
>    if (sta < cap)
> -    return "downgraded";
> +    return " (downgraded)";
>    if (sta > cap)
> -    return "strange";
> -  return "ok";
> +    return " (strange)";
> +  return " (ok)";
>  }

{
  if (sta > cap)
    return " (overdriven)";
  if (sta == cap)
    return "";
  if ((type == PCI_EXP_TYPE_ROOT_PORT) ||
      ((type == PCI_EXP_TYPE_DOWNSTREAM) ||
      ((type == PCI_EXP_TYPE_PCIE_BRIDGE))
    return "";
  return " (downgraded)";
}

(i don't know if the PCIe spec has a better word than overdriven for this
situation, but i don't like "strange".  "invalid", maybe?)

The reason i say we should report it on the downstream port is that
we probably can't get to the config data on the upstream port/device,
so this may be our best chance to find out what's wrong.
Pali Rohár March 21, 2021, 4:09 p.m. UTC | #3 
On Thursday 18 March 2021 12:02:44 Bjorn Helgaas wrote:
> From: Bjorn Helgaas <bhelgaas@google.com>
> 
> After b47b5bd408e1 ("lspci: Report if the PCIe link speed/width is full or
> downgraded"), we report "downgraded" or "ok" for PCIe links operating at
> lower speed or width than they're capable of:
> 
>   LnkCap: Port #1, Speed 8GT/s, Width x1, ASPM L1, Exit Latency L1 <16us
>   LnkSta: Speed 5GT/s (downgraded), Width x1 (ok)
> 
> Previously we did this for both ends of the link, but I don't think it's
> very useful for Downstream Ports (the upstream end of the link) because we
> claim the link is downgraded even if (1) there's no device on the other end
> or (2) the other device doesn't support anything faster/wider.

If there is no device under Root or Downstream port then PCIe Bridge
reports initial speed (2.5 GT/s) independently of the maximal speed. So
I agree that we should not report "downgraded" state in this case as the
link is not technically downgraded. For hotplugging PCIe Bridge it is
just waiting until some PCIe card is inserted back.


Different thing is when you have Gen2 PCIe Bridge which can operate at
5 GT/s but you connect only Gen1 PCIe card under it (which operates only
at 2.5 GT/s).

In this case Root or Downstream port of PCIe Bridge is running at lower
"downgraded" speed but card (on the upstream end of the link) is running
at maximal speed (not downgraded).

So in this case proposed patch does not report "downgraded" state
neither on Root/Downstream Bridge part nor on card part.

Is it correct? Should not lspci report in this case _somewhere_ that
link is downgraded and is not running at the full speed?

I can image even more complicated PCIe topology with two PCIe packet
switches and one WiFi card where information about "downgraded" state
with proposed change would not be reported too:

     CPU / Host
         |
         |
    Root 8 GT/s --> Upstream 8 GT/s
                          |
                          |
                          v
                  Downstream 5 GT/s --> Upstream 5 GT/s
                                               |
                                               |
                                               v
                                   Downstream 2.5 GT/s --> WiFi card 2.5 GT/s

If we do not report "downgraded" state in Downstream or Root part then
in this topology no node would be marked as "downgraded" even when Host
or Root Bridge is capable of 8 GT/s and WiFi card is slow and its
maximal speed is just 2.5 GT/s.

I'm not sure but I think it could be possible to create somehow very
complicated topology with more PCIe to PCI Bridges and PCI to PCIe
bridges where information about "downgraded" state is lost when lspci
would not report "downgraded" state for Downstream parts of the link.

> Drop the "downgraded" reporting for Downstream Ports.  If there is a device
> below, we'll still complain at that end if it supports a faster/wider link
> than is available.
> 
> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> ---
>  ls-caps.c | 17 ++++++++++-------
>  1 file changed, 10 insertions(+), 7 deletions(-)
> 
> diff --git a/ls-caps.c b/ls-caps.c
> index db56556..dd17c6b 100644
> --- a/ls-caps.c
> +++ b/ls-caps.c
> @@ -758,13 +758,16 @@ static char *link_speed(int speed)
>      }
>  }
>  
> -static char *link_compare(int sta, int cap)
> +static char *link_compare(int type, int sta, int cap)
>  {
> +  if ((type == PCI_EXP_TYPE_ROOT_PORT) || (type == PCI_EXP_TYPE_DOWNSTREAM) ||
> +      (type == PCI_EXP_TYPE_PCIE_BRIDGE))
> +    return "";
>    if (sta < cap)
> -    return "downgraded";
> +    return " (downgraded)";
>    if (sta > cap)
> -    return "strange";
> -  return "ok";
> +    return " (strange)";
> +  return " (ok)";
>  }
>  
>  static char *aspm_support(int code)
> @@ -837,11 +840,11 @@ static void cap_express_link(struct device *d, int where, int type)
>    w = get_conf_word(d, where + PCI_EXP_LNKSTA);
>    sta_speed = w & PCI_EXP_LNKSTA_SPEED;
>    sta_width = (w & PCI_EXP_LNKSTA_WIDTH) >> 4;
> -  printf("\t\tLnkSta:\tSpeed %s (%s), Width x%d (%s)\n",
> +  printf("\t\tLnkSta:\tSpeed %s%s, Width x%d%s\n",
>  	link_speed(sta_speed),
> -	link_compare(sta_speed, cap_speed),
> +	link_compare(type, sta_speed, cap_speed),
>  	sta_width,
> -	link_compare(sta_width, cap_width));
> +	link_compare(type, sta_width, cap_width));
>    printf("\t\t\tTrErr%c Train%c SlotClk%c DLActive%c BWMgmt%c ABWMgmt%c\n",
>  	FLAG(w, PCI_EXP_LNKSTA_TR_ERR),
>  	FLAG(w, PCI_EXP_LNKSTA_TRAIN),
> -- 
> 2.25.1
>
Matthew Wilcox March 21, 2021, 4:32 p.m. UTC | #4 
On Sun, Mar 21, 2021 at 05:09:05PM +0100, Pali Rohár wrote:
> Different thing is when you have Gen2 PCIe Bridge which can operate at
> 5 GT/s but you connect only Gen1 PCIe card under it (which operates only
> at 2.5 GT/s).
> 
> In this case Root or Downstream port of PCIe Bridge is running at lower
> "downgraded" speed but card (on the upstream end of the link) is running
> at maximal speed (not downgraded).
> 
> So in this case proposed patch does not report "downgraded" state
> neither on Root/Downstream Bridge part nor on card part.
> 
> Is it correct? Should not lspci report in this case _somewhere_ that
> link is downgraded and is not running at the full speed?

I suppose it depends why you think it's important that "downgraded"
is reported.  If you want it to mean "you've plugged a slow card into a
port that's capable of more", then you're right.  If you want it to mean
"autonegotiation went wrong", then you actually don't want downgraded
to be reported in the scenarios you outlined.

Dmitry?  What did you /want/ it to mean?
Martin Mareš Jan. 21, 2022, 1:20 p.m. UTC | #5 
Hello!

This fell through the cracks in my mailbox, but I finally assembled
pieces and here is what I applied.

				Martin


commit 9f7681202fcfaefd02e202eb64c01eb9e962729d
Author: Martin Mares <mj@ucw.cz>
Date:   Fri Jan 21 14:16:37 2022 +0100

    lspci: Improvements to PCIe link speed downgrade reporting
    
    Do not report PCIe link downgrades for downstream ports.
    
    Changed wording so that "overdriven" is reported instead of
    "strange" for speeds greater than the maximum supported one.
    
    Also report nothing instead of "ok".
    
    Inspired by patches by Bjorn Helgaas and Matthew Wilcox.

diff --git a/ls-caps.c b/ls-caps.c
index 91acb59..79b61cd 100644
--- a/ls-caps.c
+++ b/ls-caps.c
@@ -771,13 +771,16 @@ static char *link_speed(int speed)
     }
 }
 
-static char *link_compare(int sta, int cap)
+static char *link_compare(int type, int sta, int cap)
 {
-  if (sta < cap)
-    return "downgraded";
   if (sta > cap)
-    return "strange";
-  return "ok";
+    return " (overdriven)";
+  if (sta == cap)
+    return "";
+  if ((type == PCI_EXP_TYPE_ROOT_PORT) || (type == PCI_EXP_TYPE_DOWNSTREAM) ||
+      (type == PCI_EXP_TYPE_PCIE_BRIDGE))
+    return "";
+  return " (downgraded)";
 }
 
 static char *aspm_support(int code)
@@ -850,11 +853,11 @@ static void cap_express_link(struct device *d, int where, int type)
   w = get_conf_word(d, where + PCI_EXP_LNKSTA);
   sta_speed = w & PCI_EXP_LNKSTA_SPEED;
   sta_width = (w & PCI_EXP_LNKSTA_WIDTH) >> 4;
-  printf("\t\tLnkSta:\tSpeed %s (%s), Width x%d (%s)\n",
+  printf("\t\tLnkSta:\tSpeed %s%s, Width x%d%s\n",
 	link_speed(sta_speed),
-	link_compare(sta_speed, cap_speed),
+	link_compare(type, sta_speed, cap_speed),
 	sta_width,
-	link_compare(sta_width, cap_width));
+	link_compare(type, sta_width, cap_width));
   printf("\t\t\tTrErr%c Train%c SlotClk%c DLActive%c BWMgmt%c ABWMgmt%c\n",
 	FLAG(w, PCI_EXP_LNKSTA_TR_ERR),
 	FLAG(w, PCI_EXP_LNKSTA_TRAIN),
diff mbox series

Patch

diff --git a/ls-caps.c b/ls-caps.c
index db56556..dd17c6b 100644
--- a/ls-caps.c
+++ b/ls-caps.c
@@ -758,13 +758,16 @@  static char *link_speed(int speed)
     }
 }
 
-static char *link_compare(int sta, int cap)
+static char *link_compare(int type, int sta, int cap)
 {
+  if ((type == PCI_EXP_TYPE_ROOT_PORT) || (type == PCI_EXP_TYPE_DOWNSTREAM) ||
+      (type == PCI_EXP_TYPE_PCIE_BRIDGE))
+    return "";
   if (sta < cap)
-    return "downgraded";
+    return " (downgraded)";
   if (sta > cap)
-    return "strange";
-  return "ok";
+    return " (strange)";
+  return " (ok)";
 }
 
 static char *aspm_support(int code)
@@ -837,11 +840,11 @@  static void cap_express_link(struct device *d, int where, int type)
   w = get_conf_word(d, where + PCI_EXP_LNKSTA);
   sta_speed = w & PCI_EXP_LNKSTA_SPEED;
   sta_width = (w & PCI_EXP_LNKSTA_WIDTH) >> 4;
-  printf("\t\tLnkSta:\tSpeed %s (%s), Width x%d (%s)\n",
+  printf("\t\tLnkSta:\tSpeed %s%s, Width x%d%s\n",
 	link_speed(sta_speed),
-	link_compare(sta_speed, cap_speed),
+	link_compare(type, sta_speed, cap_speed),
 	sta_width,
-	link_compare(sta_width, cap_width));
+	link_compare(type, sta_width, cap_width));
   printf("\t\t\tTrErr%c Train%c SlotClk%c DLActive%c BWMgmt%c ABWMgmt%c\n",
 	FLAG(w, PCI_EXP_LNKSTA_TR_ERR),
 	FLAG(w, PCI_EXP_LNKSTA_TRAIN),