> On Oct 7, 2020, at 21:28, Ian Kumlien <ian.kumlien@xxxxxxxxx> wrote:
>
> Make pcie_aspm_check_latency comply with the PCIe spec, specifically:
> "5.4.1.2.2. Exit from the L1 State"
>
> Which makes it clear that each switch is required to initiate a
> transition within 1μs from receiving it, accumulating this latency and
> then we have to wait for the slowest link along the path before
> entering L0 state from L1.
>
> The current code doesn't take the maximum latency into account.
>
> From the example:
> +----------------+
> | |
> | Root complex |
> | |
> | +-----+ |
> | |32 μs| |
> +----------------+
> |
> | Link 1
> |
> +----------------+
> | |8 μs| |
> | +----+ |
> | Switch A |
> | +----+ |
> | |8 μs| |
> +----------------+
> |
> | Link 2
> |
> +----------------+
> | |32 μs| |
> | +-----+ |
> | Switch B |
> | +-----+ |
> | |32 μs| |
> +----------------+
> |
> | Link 3
> |
> +----------------+
> | |8μs| |
> | +---+ |
> | Endpoint C |
> | |
> | |
> +----------------+
>
> Links 1, 2 and 3 are all in L1 state - endpoint C initiates the
> transition to L0 at time T. Since switch B takes 32 μs to exit L1 on
> it's ports, Link 3 will transition to L0 at T+32 (longest time
> considering T+8 for endpoint C and T+32 for switch B).
>
> Switch B is required to initiate a transition from the L1 state on it's
> upstream port after no more than 1 μs from the beginning of the
> transition from L1 state on the downstream port. Therefore, transition from
> L1 to L0 will begin on link 2 at T+1, this will cascade up the path.
>
> The path will exit L1 at T+34.
>
> On my specific system:
> lspci -PP -s 04:00.0
> 00:01.2/01:00.0/02:04.0/04:00.0 Unassigned class [ff00]: Realtek Semiconductor Co., Ltd. Device 816e (rev 1a)
>
> lspci -vvv -s 04:00.0
> DevCap: MaxPayload 128 bytes, PhantFunc 0, Latency L0s <512ns, L1 <64us
> ...
> LnkCap: Port #0, Speed 5GT/s, Width x1, ASPM L0s L1, Exit Latency L0s unlimited, L1 <64us
> ...
>
> Which means that it can't be followed by any switch that is in L1 state.
>
> This patch fixes it by disabling L1 on 02:04.0, 01:00.0 and 00:01.2.
>
> LnkCtl LnkCtl
> ------DevCap------- ----LnkCap------- -Before- -After--
> 00:01.2 L1 <32us L1+ L1-
> 01:00.0 L1 <32us L1+ L1-
> 02:04.0 L1 <32us L1+ L1-
> 04:00.0 L0s <512 L1 <64us L1 <64us L1+ L1-
>
> Signed-off-by: Ian Kumlien <ian.kumlien@xxxxxxxxx>
Solves an issue that enabling ASPM on Dell Latitude 5495 causes system freeze.
Tested-by: Kai-Heng Feng <kai.heng.feng@xxxxxxxxxxxxx>
> ---
> drivers/pci/pcie/aspm.c | 23 +++++++++++++++--------
> 1 file changed, 15 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
> index 253c30cc1967..893b37669087 100644
> --- a/drivers/pci/pcie/aspm.c
> +++ b/drivers/pci/pcie/aspm.c
> @@ -434,7 +434,7 @@ static void pcie_get_aspm_reg(struct pci_dev *pdev,
>
> static void pcie_aspm_check_latency(struct pci_dev *endpoint)
> {
> - u32 latency, l1_switch_latency = 0;
> + u32 latency, l1_max_latency = 0, l1_switch_latency = 0;
> struct aspm_latency *acceptable;
> struct pcie_link_state *link;
>
> @@ -456,10 +456,14 @@ static void pcie_aspm_check_latency(struct pci_dev *endpoint)
> if ((link->aspm_capable & ASPM_STATE_L0S_DW) &&
> (link->latency_dw.l0s > acceptable->l0s))
> link->aspm_capable &= ~ASPM_STATE_L0S_DW;
> +
> /*
> * Check L1 latency.
> - * Every switch on the path to root complex need 1
> - * more microsecond for L1. Spec doesn't mention L0s.
> + *
> + * PCIe r5.0, sec 5.4.1.2.2 states:
> + * A Switch is required to initiate an L1 exit transition on its
> + * Upstream Port Link after no more than 1 μs from the beginning of an
> + * L1 exit transition on any of its Downstream Port Links.
> *
> * The exit latencies for L1 substates are not advertised
> * by a device. Since the spec also doesn't mention a way
> @@ -469,11 +473,14 @@ static void pcie_aspm_check_latency(struct pci_dev *endpoint)
> * L1 exit latencies advertised by a device include L1
> * substate latencies (and hence do not do any check).
> */
> - latency = max_t(u32, link->latency_up.l1, link->latency_dw.l1);
> - if ((link->aspm_capable & ASPM_STATE_L1) &&
> - (latency + l1_switch_latency > acceptable->l1))
> - link->aspm_capable &= ~ASPM_STATE_L1;
> - l1_switch_latency += 1000;
> + if (link->aspm_capable & ASPM_STATE_L1) {
> + latency = max_t(u32, link->latency_up.l1, link->latency_dw.l1);
> + l1_max_latency = max_t(u32, latency, l1_max_latency);
> + if (l1_max_latency + l1_switch_latency > acceptable->l1)
> + link->aspm_capable &= ~ASPM_STATE_L1;
> +
> + l1_switch_latency += 1000;
> + }
>
> link = link->parent;
> }
> --
> 2.28.0
>
