On Mon, Jul 27, 2020 at 11:30:45PM +0200, Ian Kumlien wrote: > Currently we check the maximum latency of upstream and downstream > per link, not the maximum for the path > > This would work if all links have the same latency, but: > endpoint -> c -> b -> a -> root (in the order we walk the path) > > If c or b has the higest latency, it will not register > > Fix this by maintaining the maximum latency value for the path > > This change fixes a regression introduced by: > 66ff14e59e8a (PCI/ASPM: Allow ASPM on links to PCIe-to-PCI/PCI-X Bridges) Hi Ian, Sorry about the regression, and thank you very much for doing the hard work of debugging and fixing it! My guess is that 66ff14e59e8a isn't itself buggy, but it allowed ASPM to be enabled on a longer path, and we weren't computing the maximum latency correctly, so ASPM on that longer path exceeded the amount we could tolerate. If that's the case, 66ff14e59e8a probably just exposed an existing problem that could occur in other topologies even without 66ff14e59e8a. I'd like to work through this latency code with concrete examples. Can you collect the "sudo lspci -vv" output and attach it to an entry at https://bugzilla.kernel.org? If it's convenient, it would be really nice to compare it with similar output from before this patch. Bjorn > Signed-off-by: Ian Kumlien <ian.kumlien@xxxxxxxxx> > --- > drivers/pci/pcie/aspm.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c > index b17e5ffd31b1..bd53fba7f382 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; > > @@ -470,8 +470,9 @@ static void pcie_aspm_check_latency(struct pci_dev *endpoint) > * substate latencies (and hence do not do any check). > */ > latency = max_t(u32, link->latency_up.l1, link->latency_dw.l1); > + l1_max_latency = max_t(u32, latency, l1_max_latency); > if ((link->aspm_capable & ASPM_STATE_L1) && > - (latency + l1_switch_latency > acceptable->l1)) > + (l1_max_latency + l1_switch_latency > acceptable->l1)) > link->aspm_capable &= ~ASPM_STATE_L1; > l1_switch_latency += 1000; > > -- > 2.27.0 >
