* [PATCH] lspci: Don't report PCIe link downgrades for downstream ports
@ 2021-03-18 17:02 Bjorn Helgaas
2021-03-18 17:11 ` Bjorn Helgaas
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Bjorn Helgaas @ 2021-03-18 17:02 UTC (permalink / raw)
To: Martin Mares
Cc: Pali Rohár, Krzysztof Wilczyński, Matthew Wilcox,
linux-pci, Bjorn Helgaas
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)";
}
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
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH] lspci: Don't report PCIe link downgrades for downstream ports
2021-03-18 17:02 [PATCH] lspci: Don't report PCIe link downgrades for downstream ports Bjorn Helgaas
@ 2021-03-18 17:11 ` Bjorn Helgaas
2021-03-18 17:33 ` Matthew Wilcox
2021-03-21 16:09 ` Pali Rohár
2 siblings, 0 replies; 6+ messages in thread
From: Bjorn Helgaas @ 2021-03-18 17:11 UTC (permalink / raw)
To: Martin Mares
Cc: Pali Rohár, Krzysztof Wilczyński, Matthew Wilcox,
linux-pci, Bjorn Helgaas
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
>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] lspci: Don't report PCIe link downgrades for downstream ports
2021-03-18 17:02 [PATCH] lspci: Don't report PCIe link downgrades for downstream ports Bjorn Helgaas
2021-03-18 17:11 ` Bjorn Helgaas
@ 2021-03-18 17:33 ` Matthew Wilcox
2022-01-21 13:20 ` Martin Mareš
2021-03-21 16:09 ` Pali Rohár
2 siblings, 1 reply; 6+ messages in thread
From: Matthew Wilcox @ 2021-03-18 17:33 UTC (permalink / raw)
To: Bjorn Helgaas
Cc: Martin Mares, Pali Rohár, Krzysztof Wilczyński,
linux-pci, Bjorn Helgaas
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.
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] lspci: Don't report PCIe link downgrades for downstream ports
2021-03-18 17:33 ` Matthew Wilcox
@ 2022-01-21 13:20 ` Martin Mareš
0 siblings, 0 replies; 6+ messages in thread
From: Martin Mareš @ 2022-01-21 13:20 UTC (permalink / raw)
To: Matthew Wilcox
Cc: Bjorn Helgaas, Pali Rohár, Krzysztof Wilczyński,
linux-pci, Bjorn Helgaas
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),
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] lspci: Don't report PCIe link downgrades for downstream ports
2021-03-18 17:02 [PATCH] lspci: Don't report PCIe link downgrades for downstream ports Bjorn Helgaas
2021-03-18 17:11 ` Bjorn Helgaas
2021-03-18 17:33 ` Matthew Wilcox
@ 2021-03-21 16:09 ` Pali Rohár
2021-03-21 16:32 ` Matthew Wilcox
2 siblings, 1 reply; 6+ messages in thread
From: Pali Rohár @ 2021-03-21 16:09 UTC (permalink / raw)
To: Bjorn Helgaas
Cc: Martin Mares, Krzysztof Wilczyński, Matthew Wilcox,
linux-pci, Bjorn Helgaas
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
>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] lspci: Don't report PCIe link downgrades for downstream ports
2021-03-21 16:09 ` Pali Rohár
@ 2021-03-21 16:32 ` Matthew Wilcox
0 siblings, 0 replies; 6+ messages in thread
From: Matthew Wilcox @ 2021-03-21 16:32 UTC (permalink / raw)
To: Pali Rohár
Cc: Bjorn Helgaas, Martin Mares, Krzysztof Wilczyński,
linux-pci, Bjorn Helgaas, Dmitry Monakhov
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?
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2022-01-21 13:21 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-03-18 17:02 [PATCH] lspci: Don't report PCIe link downgrades for downstream ports Bjorn Helgaas
2021-03-18 17:11 ` Bjorn Helgaas
2021-03-18 17:33 ` Matthew Wilcox
2022-01-21 13:20 ` Martin Mareš
2021-03-21 16:09 ` Pali Rohár
2021-03-21 16:32 ` Matthew Wilcox
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).