| Message ID | 20221005025809.2247547-3-helgaas@kernel.org |
|---|---|
| State | New |
| Headers | show |
| Series | PCI/ASPM: Fix L1SS issues | expand |
On 10/4/22 7:58 PM, Bjorn Helgaas wrote: > From: Bjorn Helgaas <bhelgaas@google.com> > > 187f91db8237 ("PCI/ASPM: Remove struct aspm_register_info.l1ss_cap") > inadvertently removed a check for existence of the L1 PM Substates (L1SS) > Capability before reading it. > > If there is no L1SS Capability, this means we mistakenly read PCI_COMMAND > and PCI_STATUS (config address 0x04) and interpret that as the PCI_L1SS_CAP > register, so we may incorrectly configure L1SS. > > Make sure the L1SS Capability exists before trying to read it. > > Fixes: 187f91db8237 ("PCI/ASPM: Remove struct aspm_register_info.l1ss_cap") > Signed-off-by: Bjorn Helgaas <bhelgaas@google.com> > --- > drivers/pci/pcie/aspm.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c > index 4535228e4a64..f12d117f44e0 100644 > --- a/drivers/pci/pcie/aspm.c > +++ b/drivers/pci/pcie/aspm.c > @@ -560,6 +560,9 @@ static void aspm_l1ss_init(struct pcie_link_state *link) > u32 parent_l1ss_cap, child_l1ss_cap; > u32 parent_l1ss_ctl1 = 0, child_l1ss_ctl1 = 0; > > + if (!parent->l1ss || !child->l1ss) > + return; I have noticed that l1ss state is initialized in pci_configure_ltr(). I am wondering whether it is the right place? Are L1SS and LTR related? > + > /* Setup L1 substate */ > pci_read_config_dword(parent, parent->l1ss + PCI_L1SS_CAP, > &parent_l1ss_cap);
On Tue, Oct 04, 2022 at 08:26:53PM -0700, Sathyanarayanan Kuppuswamy wrote: > On 10/4/22 7:58 PM, Bjorn Helgaas wrote: > > 187f91db8237 ("PCI/ASPM: Remove struct aspm_register_info.l1ss_cap") > > inadvertently removed a check for existence of the L1 PM Substates (L1SS) > > Capability before reading it. > > > > If there is no L1SS Capability, this means we mistakenly read PCI_COMMAND > > and PCI_STATUS (config address 0x04) and interpret that as the PCI_L1SS_CAP > > register, so we may incorrectly configure L1SS. > > > > Make sure the L1SS Capability exists before trying to read it. > > > > Fixes: 187f91db8237 ("PCI/ASPM: Remove struct aspm_register_info.l1ss_cap") > > Signed-off-by: Bjorn Helgaas <bhelgaas@google.com> > > --- > > drivers/pci/pcie/aspm.c | 3 +++ > > 1 file changed, 3 insertions(+) > > > > diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c > > index 4535228e4a64..f12d117f44e0 100644 > > --- a/drivers/pci/pcie/aspm.c > > +++ b/drivers/pci/pcie/aspm.c > > @@ -560,6 +560,9 @@ static void aspm_l1ss_init(struct pcie_link_state *link) > > u32 parent_l1ss_cap, child_l1ss_cap; > > u32 parent_l1ss_ctl1 = 0, child_l1ss_ctl1 = 0; > > > > + if (!parent->l1ss || !child->l1ss) > > + return; > > I have noticed that l1ss state is initialized in pci_configure_ltr(). I am > wondering whether it is the right place? Are L1SS and LTR related? L1SS and LTR are definitely related -- LTR supplies information that's needed for L1.2. I'm not sure why pci_configure_ltr() is in probe.c and pci_bridge_reconfigure_ltr() is in pci.c; maybe it would make sense to put them both in aspm.c. Bjorn
diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c index 4535228e4a64..f12d117f44e0 100644 --- a/drivers/pci/pcie/aspm.c +++ b/drivers/pci/pcie/aspm.c @@ -560,6 +560,9 @@ static void aspm_l1ss_init(struct pcie_link_state *link) u32 parent_l1ss_cap, child_l1ss_cap; u32 parent_l1ss_ctl1 = 0, child_l1ss_ctl1 = 0; + if (!parent->l1ss || !child->l1ss) + return; + /* Setup L1 substate */ pci_read_config_dword(parent, parent->l1ss + PCI_L1SS_CAP, &parent_l1ss_cap);