| Message ID | 20240222174436.3565146-1-vidyas@nvidia.com (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
| Series | [V4] PCI/ASPM: Update saved buffers with latest ASPM | expand |
On 2/22/24 9:44 AM, Vidya Sagar wrote: > Many PCIe device drivers save the configuration state of their respective > devices during probe and restore the same when their 'slot_reset' hook > is called through PCIe Error Recovery Handler. > > If the system has a change in ASPM policy after the driver's probe is > called and before error event occurred, 'slot_reset' hook restores the > PCIe configuration state to what it was at the time of probe but not to > what it was just before the occurrence of the error event. > This effectively leads to a mismatch in the ASPM configuration between > the device and its upstream parent device. > > Update the saved configuration state of the device with the latest info > whenever there is a change w.r.t ASPM policy. > > Signed-off-by: Vidya Sagar <vidyas@nvidia.com> > --- > V4: > * Rebased on top of pci/aspm branch > > V3: > * Addressed sathyanarayanan.kuppuswamy's review comments > > V2: > * Rebased on top of the tree code > * Addressed Bjorn's review comments > > drivers/pci/pcie/aspm.c | 28 ++++++++++++++++++++++++++-- > 3 files changed, 28 insertions(+), 4 deletions(-) > > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c > index cfc5b84dc9c9..3db606ba9344 100644 > --- a/drivers/pci/pci.c > +++ b/drivers/pci/pci.c > @@ -1648,7 +1648,7 @@ static int pci_save_pcie_state(struct pci_dev *dev) > pcie_capability_read_word(dev, PCI_EXP_LNKCTL2, &cap[i++]); > pcie_capability_read_word(dev, PCI_EXP_SLTCTL2, &cap[i++]); > > - pci_save_aspm_state(dev); > + pci_save_aspm_l1ss_state(dev); > pci_save_ltr_state(dev); > > return 0; > diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h > index b217e74966eb..9fe78eb8b07d 100644 > --- a/drivers/pci/pci.h > +++ b/drivers/pci/pci.h > @@ -95,7 +95,7 @@ void pci_msix_init(struct pci_dev *dev); > bool pci_bridge_d3_possible(struct pci_dev *dev); > void pci_bridge_d3_update(struct pci_dev *dev); > void pci_aspm_get_l1ss(struct pci_dev *pdev); > -void pci_save_aspm_state(struct pci_dev *pdev); > +void pci_save_aspm_l1ss_state(struct pci_dev *pdev); is this rename a review request? It is not clear from the commit log why you are doing it? > void pci_restore_aspm_state(struct pci_dev *pdev); > void pci_save_ltr_state(struct pci_dev *dev); > void pci_restore_ltr_state(struct pci_dev *dev); > diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c > index 7f1d674ff171..a62648dd52bc 100644 > --- a/drivers/pci/pcie/aspm.c > +++ b/drivers/pci/pcie/aspm.c > @@ -24,13 +24,29 @@ > > #include "../pci.h" > > +static void pci_save_aspm_state(struct pci_dev *dev) > +{ > + struct pci_cap_saved_state *save_state; > + u16 *cap; > + > + if (!pci_is_pcie(dev)) > + return; > + > + save_state = pci_find_saved_cap(dev, PCI_CAP_ID_EXP); > + if (!save_state) > + return; > + > + cap = (u16 *)&save_state->cap.data[0]; > + pcie_capability_read_word(dev, PCI_EXP_LNKCTL, &cap[1]); > +} > + > void pci_aspm_get_l1ss(struct pci_dev *pdev) > { > /* Read L1 PM substate capabilities */ > pdev->l1ss = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_L1SS); > } > > -void pci_save_aspm_state(struct pci_dev *pdev) > +void pci_save_aspm_l1ss_state(struct pci_dev *pdev) > { > struct pci_cap_saved_state *save_state; > u16 l1ss = pdev->l1ss; > @@ -309,10 +325,12 @@ static void pcie_set_clkpm_nocheck(struct pcie_link_state *link, int enable) > struct pci_bus *linkbus = link->pdev->subordinate; > u32 val = enable ? PCI_EXP_LNKCTL_CLKREQ_EN : 0; > > - list_for_each_entry(child, &linkbus->devices, bus_list) > + list_for_each_entry(child, &linkbus->devices, bus_list) { > pcie_capability_clear_and_set_word(child, PCI_EXP_LNKCTL, > PCI_EXP_LNKCTL_CLKREQ_EN, > val); > + pci_save_aspm_state(child); > + } > link->clkpm_enabled = !!enable; > } > > @@ -931,6 +949,12 @@ static void pcie_config_aspm_link(struct pcie_link_state *link, u32 state) > pcie_config_aspm_dev(parent, upstream); > > link->aspm_enabled = state; > + > + /* Update latest ASPM configuration in saved context */ > + pci_save_aspm_state(link->downstream); > + pci_save_aspm_l1ss_state(link->downstream); > + pci_save_aspm_state(parent); > + pci_save_aspm_l1ss_state(parent); > } > > static void pcie_config_aspm_path(struct pcie_link_state *link)
On Thu, Feb 22, 2024 at 10:20:06AM -0800, Kuppuswamy Sathyanarayanan wrote: > On 2/22/24 9:44 AM, Vidya Sagar wrote: > > Many PCIe device drivers save the configuration state of their respective > > devices during probe and restore the same when their 'slot_reset' hook > > is called through PCIe Error Recovery Handler. > > > > If the system has a change in ASPM policy after the driver's probe is > > called and before error event occurred, 'slot_reset' hook restores the > > PCIe configuration state to what it was at the time of probe but not to > > what it was just before the occurrence of the error event. > > This effectively leads to a mismatch in the ASPM configuration between > > the device and its upstream parent device. > > > > Update the saved configuration state of the device with the latest info > > whenever there is a change w.r.t ASPM policy. > > > > Signed-off-by: Vidya Sagar <vidyas@nvidia.com> > > --- > > V4: > > * Rebased on top of pci/aspm branch > > > > V3: > > * Addressed sathyanarayanan.kuppuswamy's review comments > > > > V2: > > * Rebased on top of the tree code > > * Addressed Bjorn's review comments > > > > drivers/pci/pcie/aspm.c | 28 ++++++++++++++++++++++++++-- > > 3 files changed, 28 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c > > index cfc5b84dc9c9..3db606ba9344 100644 > > --- a/drivers/pci/pci.c > > +++ b/drivers/pci/pci.c > > @@ -1648,7 +1648,7 @@ static int pci_save_pcie_state(struct pci_dev *dev) > > pcie_capability_read_word(dev, PCI_EXP_LNKCTL2, &cap[i++]); > > pcie_capability_read_word(dev, PCI_EXP_SLTCTL2, &cap[i++]); > > > > - pci_save_aspm_state(dev); > > + pci_save_aspm_l1ss_state(dev); > > pci_save_ltr_state(dev); > > > > return 0; > > diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h > > index b217e74966eb..9fe78eb8b07d 100644 > > --- a/drivers/pci/pci.h > > +++ b/drivers/pci/pci.h > > @@ -95,7 +95,7 @@ void pci_msix_init(struct pci_dev *dev); > > bool pci_bridge_d3_possible(struct pci_dev *dev); > > void pci_bridge_d3_update(struct pci_dev *dev); > > void pci_aspm_get_l1ss(struct pci_dev *pdev); > > -void pci_save_aspm_state(struct pci_dev *pdev); > > +void pci_save_aspm_l1ss_state(struct pci_dev *pdev); > > is this rename a review request? It is not clear from the commit log > why you are doing it? David's changes already on pci/aspm added pci_save_aspm_state(), but it actually only saves L1SS data, and Vidya needs to save the non-L1SS data also. I think I'm going to rework David's changes a little bit so this is named pci_save_aspm_l1ss_state() from the beginning so we won't need the rename here. Bjorn
On 2/22/24 10:54 AM, Bjorn Helgaas wrote: > On Thu, Feb 22, 2024 at 10:20:06AM -0800, Kuppuswamy Sathyanarayanan wrote: >> On 2/22/24 9:44 AM, Vidya Sagar wrote: >>> Many PCIe device drivers save the configuration state of their respective >>> devices during probe and restore the same when their 'slot_reset' hook >>> is called through PCIe Error Recovery Handler. >>> >>> If the system has a change in ASPM policy after the driver's probe is >>> called and before error event occurred, 'slot_reset' hook restores the >>> PCIe configuration state to what it was at the time of probe but not to >>> what it was just before the occurrence of the error event. >>> This effectively leads to a mismatch in the ASPM configuration between >>> the device and its upstream parent device. >>> >>> Update the saved configuration state of the device with the latest info >>> whenever there is a change w.r.t ASPM policy. >>> >>> Signed-off-by: Vidya Sagar <vidyas@nvidia.com> >>> --- >>> V4: >>> * Rebased on top of pci/aspm branch >>> >>> V3: >>> * Addressed sathyanarayanan.kuppuswamy's review comments >>> >>> V2: >>> * Rebased on top of the tree code >>> * Addressed Bjorn's review comments >>> >>> drivers/pci/pcie/aspm.c | 28 ++++++++++++++++++++++++++-- >>> 3 files changed, 28 insertions(+), 4 deletions(-) >>> >>> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c >>> index cfc5b84dc9c9..3db606ba9344 100644 >>> --- a/drivers/pci/pci.c >>> +++ b/drivers/pci/pci.c >>> @@ -1648,7 +1648,7 @@ static int pci_save_pcie_state(struct pci_dev *dev) >>> pcie_capability_read_word(dev, PCI_EXP_LNKCTL2, &cap[i++]); >>> pcie_capability_read_word(dev, PCI_EXP_SLTCTL2, &cap[i++]); >>> >>> - pci_save_aspm_state(dev); >>> + pci_save_aspm_l1ss_state(dev); >>> pci_save_ltr_state(dev); >>> >>> return 0; >>> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h >>> index b217e74966eb..9fe78eb8b07d 100644 >>> --- a/drivers/pci/pci.h >>> +++ b/drivers/pci/pci.h >>> @@ -95,7 +95,7 @@ void pci_msix_init(struct pci_dev *dev); >>> bool pci_bridge_d3_possible(struct pci_dev *dev); >>> void pci_bridge_d3_update(struct pci_dev *dev); >>> void pci_aspm_get_l1ss(struct pci_dev *pdev); >>> -void pci_save_aspm_state(struct pci_dev *pdev); >>> +void pci_save_aspm_l1ss_state(struct pci_dev *pdev); >> is this rename a review request? It is not clear from the commit log >> why you are doing it? > David's changes already on pci/aspm added pci_save_aspm_state(), but > it actually only saves L1SS data, and Vidya needs to save the non-L1SS > data also. > > I think I'm going to rework David's changes a little bit so this is > named pci_save_aspm_l1ss_state() from the beginning so we won't need > the rename here. Got it. Change wise, this patch looks fine to me. Reviewed-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com> > > Bjorn
On 22-02-2024 23:50, Kuppuswamy Sathyanarayanan wrote: > External email: Use caution opening links or attachments > > > On 2/22/24 9:44 AM, Vidya Sagar wrote: >> Many PCIe device drivers save the configuration state of their respective >> devices during probe and restore the same when their 'slot_reset' hook >> is called through PCIe Error Recovery Handler. >> >> If the system has a change in ASPM policy after the driver's probe is >> called and before error event occurred, 'slot_reset' hook restores the >> PCIe configuration state to what it was at the time of probe but not to >> what it was just before the occurrence of the error event. >> This effectively leads to a mismatch in the ASPM configuration between >> the device and its upstream parent device. >> >> Update the saved configuration state of the device with the latest info >> whenever there is a change w.r.t ASPM policy. >> >> Signed-off-by: Vidya Sagar <vidyas@nvidia.com> >> --- >> V4: >> * Rebased on top of pci/aspm branch >> >> V3: >> * Addressed sathyanarayanan.kuppuswamy's review comments >> >> V2: >> * Rebased on top of the tree code >> * Addressed Bjorn's review comments >> >> drivers/pci/pcie/aspm.c | 28 ++++++++++++++++++++++++++-- >> 3 files changed, 28 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c >> index cfc5b84dc9c9..3db606ba9344 100644 >> --- a/drivers/pci/pci.c >> +++ b/drivers/pci/pci.c >> @@ -1648,7 +1648,7 @@ static int pci_save_pcie_state(struct pci_dev *dev) >> pcie_capability_read_word(dev, PCI_EXP_LNKCTL2, &cap[i++]); >> pcie_capability_read_word(dev, PCI_EXP_SLTCTL2, &cap[i++]); >> >> - pci_save_aspm_state(dev); >> + pci_save_aspm_l1ss_state(dev); >> pci_save_ltr_state(dev); >> >> return 0; >> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h >> index b217e74966eb..9fe78eb8b07d 100644 >> --- a/drivers/pci/pci.h >> +++ b/drivers/pci/pci.h >> @@ -95,7 +95,7 @@ void pci_msix_init(struct pci_dev *dev); >> bool pci_bridge_d3_possible(struct pci_dev *dev); >> void pci_bridge_d3_update(struct pci_dev *dev); >> void pci_aspm_get_l1ss(struct pci_dev *pdev); >> -void pci_save_aspm_state(struct pci_dev *pdev); >> +void pci_save_aspm_l1ss_state(struct pci_dev *pdev); > is this rename a review request? It is not clear from the commit log > why you are doing it? I rebased my changes on top of pci/aspm branch which got the support for save/restore of ASPM L1-SubStates registers merged recently. As I see, the name of the function that saves the L1SS registers is given as pci_save_aspm_state() instead of pci_save_aspm_l1ss_state(). My original change already adds a function to save the ASPM state (only L0s and L1) info from the link control register and I called it pci_save_aspm_state(). I thought it makes sense to use pci_save_aspm_state() for saving the info of ASPM-L0s/L1 related registers whereas use pci_save_aspm_l1ss_state() to save the info of ASPM-L1SS. Thanks, Vidya Sagar > >> void pci_restore_aspm_state(struct pci_dev *pdev); >> void pci_save_ltr_state(struct pci_dev *dev); >> void pci_restore_ltr_state(struct pci_dev *dev); >> diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c >> index 7f1d674ff171..a62648dd52bc 100644 >> --- a/drivers/pci/pcie/aspm.c >> +++ b/drivers/pci/pcie/aspm.c >> @@ -24,13 +24,29 @@ >> >> #include "../pci.h" >> >> +static void pci_save_aspm_state(struct pci_dev *dev) >> +{ >> + struct pci_cap_saved_state *save_state; >> + u16 *cap; >> + >> + if (!pci_is_pcie(dev)) >> + return; >> + >> + save_state = pci_find_saved_cap(dev, PCI_CAP_ID_EXP); >> + if (!save_state) >> + return; >> + >> + cap = (u16 *)&save_state->cap.data[0]; >> + pcie_capability_read_word(dev, PCI_EXP_LNKCTL, &cap[1]); >> +} >> + >> void pci_aspm_get_l1ss(struct pci_dev *pdev) >> { >> /* Read L1 PM substate capabilities */ >> pdev->l1ss = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_L1SS); >> } >> >> -void pci_save_aspm_state(struct pci_dev *pdev) >> +void pci_save_aspm_l1ss_state(struct pci_dev *pdev) >> { >> struct pci_cap_saved_state *save_state; >> u16 l1ss = pdev->l1ss; >> @@ -309,10 +325,12 @@ static void pcie_set_clkpm_nocheck(struct pcie_link_state *link, int enable) >> struct pci_bus *linkbus = link->pdev->subordinate; >> u32 val = enable ? PCI_EXP_LNKCTL_CLKREQ_EN : 0; >> >> - list_for_each_entry(child, &linkbus->devices, bus_list) >> + list_for_each_entry(child, &linkbus->devices, bus_list) { >> pcie_capability_clear_and_set_word(child, PCI_EXP_LNKCTL, >> PCI_EXP_LNKCTL_CLKREQ_EN, >> val); >> + pci_save_aspm_state(child); >> + } >> link->clkpm_enabled = !!enable; >> } >> >> @@ -931,6 +949,12 @@ static void pcie_config_aspm_link(struct pcie_link_state *link, u32 state) >> pcie_config_aspm_dev(parent, upstream); >> >> link->aspm_enabled = state; >> + >> + /* Update latest ASPM configuration in saved context */ >> + pci_save_aspm_state(link->downstream); >> + pci_save_aspm_l1ss_state(link->downstream); >> + pci_save_aspm_state(parent); >> + pci_save_aspm_l1ss_state(parent); >> } >> >> static void pcie_config_aspm_path(struct pcie_link_state *link) > -- > Sathyanarayanan Kuppuswamy > Linux Kernel Developer >
On Thu, 2024-02-22 at 11:54 -0800, Kuppuswamy Sathyanarayanan wrote: > > On 2/22/24 10:54 AM, Bjorn Helgaas wrote: > > On Thu, Feb 22, 2024 at 10:20:06AM -0800, Kuppuswamy Sathyanarayanan wrote: > > > On 2/22/24 9:44 AM, Vidya Sagar wrote: > > > > Many PCIe device drivers save the configuration state of their > > > > respective > > > > devices during probe and restore the same when their 'slot_reset' hook > > > > is called through PCIe Error Recovery Handler. > > > > > > > > If the system has a change in ASPM policy after the driver's probe is > > > > called and before error event occurred, 'slot_reset' hook restores the > > > > PCIe configuration state to what it was at the time of probe but not to > > > > what it was just before the occurrence of the error event. > > > > This effectively leads to a mismatch in the ASPM configuration between > > > > the device and its upstream parent device. > > > > > > > > Update the saved configuration state of the device with the latest info > > > > whenever there is a change w.r.t ASPM policy. > > > > > > > > Signed-off-by: Vidya Sagar <vidyas@nvidia.com> > > > > --- > > > > V4: > > > > * Rebased on top of pci/aspm branch > > > > > > > > V3: > > > > * Addressed sathyanarayanan.kuppuswamy's review comments > > > > > > > > V2: > > > > * Rebased on top of the tree code > > > > * Addressed Bjorn's review comments > > > > > > > > drivers/pci/pcie/aspm.c | 28 ++++++++++++++++++++++++++-- > > > > 3 files changed, 28 insertions(+), 4 deletions(-) > > > > > > > > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c > > > > index cfc5b84dc9c9..3db606ba9344 100644 > > > > --- a/drivers/pci/pci.c > > > > +++ b/drivers/pci/pci.c > > > > @@ -1648,7 +1648,7 @@ static int pci_save_pcie_state(struct pci_dev > > > > *dev) > > > > pcie_capability_read_word(dev, PCI_EXP_LNKCTL2, &cap[i++]); > > > > pcie_capability_read_word(dev, PCI_EXP_SLTCTL2, &cap[i++]); > > > > > > > > - pci_save_aspm_state(dev); > > > > + pci_save_aspm_l1ss_state(dev); > > > > pci_save_ltr_state(dev); > > > > > > > > return 0; > > > > diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h > > > > index b217e74966eb..9fe78eb8b07d 100644 > > > > --- a/drivers/pci/pci.h > > > > +++ b/drivers/pci/pci.h > > > > @@ -95,7 +95,7 @@ void pci_msix_init(struct pci_dev *dev); > > > > bool pci_bridge_d3_possible(struct pci_dev *dev); > > > > void pci_bridge_d3_update(struct pci_dev *dev); > > > > void pci_aspm_get_l1ss(struct pci_dev *pdev); > > > > -void pci_save_aspm_state(struct pci_dev *pdev); > > > > +void pci_save_aspm_l1ss_state(struct pci_dev *pdev); > > > is this rename a review request? It is not clear from the commit log > > > why you are doing it? > > David's changes already on pci/aspm added pci_save_aspm_state(), but > > it actually only saves L1SS data, and Vidya needs to save the non-L1SS > > data also. > > > > I think I'm going to rework David's changes a little bit so this is > > named pci_save_aspm_l1ss_state() from the beginning so we won't need > > the rename here. Ack > > Got it. > > Change wise, this patch looks fine to me. > > Reviewed-by: Kuppuswamy Sathyanarayanan > <sathyanarayanan.kuppuswamy@linux.intel.com> LGTM too. Reviewed-by: David E. Box <david.e.box@linux.intel.com>
[+to Sathy, David in case you want to update your Reviewed-by] On Thu, Feb 22, 2024 at 11:14:36PM +0530, Vidya Sagar wrote: > Many PCIe device drivers save the configuration state of their respective > devices during probe and restore the same when their 'slot_reset' hook > is called through PCIe Error Recovery Handler. > > If the system has a change in ASPM policy after the driver's probe is > called and before error event occurred, 'slot_reset' hook restores the > PCIe configuration state to what it was at the time of probe but not to > what it was just before the occurrence of the error event. > This effectively leads to a mismatch in the ASPM configuration between > the device and its upstream parent device. > > Update the saved configuration state of the device with the latest info > whenever there is a change w.r.t ASPM policy. > > Signed-off-by: Vidya Sagar <vidyas@nvidia.com> > -void pci_save_aspm_state(struct pci_dev *pdev); > +void pci_save_aspm_l1ss_state(struct pci_dev *pdev); I rebased this again on top of my pci/aspm updates to remove the need for the rename above. > +static void pci_save_aspm_state(struct pci_dev *dev) > +{ > + struct pci_cap_saved_state *save_state; > + u16 *cap; > + > + if (!pci_is_pcie(dev)) > + return; > + > + save_state = pci_find_saved_cap(dev, PCI_CAP_ID_EXP); > + if (!save_state) > + return; > + > + cap = (u16 *)&save_state->cap.data[0]; > + pcie_capability_read_word(dev, PCI_EXP_LNKCTL, &cap[1]); And I changed this part so it only updates the PCI_EXP_LNKCTL_ASPMC bits, not the entire LNKCTL. Updating the entire saved register probably wouldn't *break* anything, but it could randomly hide other LNKCTL changes depending on whether or not ASPM configuration was changed in the interim. For example: - driver .probe() saves LNKCTL - LNKCTL changes some non-ASPMC thing via setpci or other mechanism - save_state updated via pcie_config_aspm_link() A restore in .slot_reset() would restore different LNKCTL values for the non-ASPMC change depending on whether pcie_config_aspm_link() was used. I applied it on pci/aspm for v6.9. Please take a look and make sure it still does what you need: https://git.kernel.org/pub/scm/linux/kernel/git/pci/pci.git/commit/?h=aspm&id=a6315434436d587f70e489e6365c5b7e20176a71 Sathy and David, I didn't add your Reviewed-by because I didn't want to presume that you were OK with my changes. But I'd be more than happy to add them if you take a look. Bjorn > +} > + > void pci_aspm_get_l1ss(struct pci_dev *pdev) > { > /* Read L1 PM substate capabilities */ > pdev->l1ss = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_L1SS); > } > > -void pci_save_aspm_state(struct pci_dev *pdev) > +void pci_save_aspm_l1ss_state(struct pci_dev *pdev) > { > struct pci_cap_saved_state *save_state; > u16 l1ss = pdev->l1ss; > @@ -309,10 +325,12 @@ static void pcie_set_clkpm_nocheck(struct pcie_link_state *link, int enable) > struct pci_bus *linkbus = link->pdev->subordinate; > u32 val = enable ? PCI_EXP_LNKCTL_CLKREQ_EN : 0; > > - list_for_each_entry(child, &linkbus->devices, bus_list) > + list_for_each_entry(child, &linkbus->devices, bus_list) { > pcie_capability_clear_and_set_word(child, PCI_EXP_LNKCTL, > PCI_EXP_LNKCTL_CLKREQ_EN, > val); > + pci_save_aspm_state(child); > + } > link->clkpm_enabled = !!enable; > } > > @@ -931,6 +949,12 @@ static void pcie_config_aspm_link(struct pcie_link_state *link, u32 state) > pcie_config_aspm_dev(parent, upstream); > > link->aspm_enabled = state; > + > + /* Update latest ASPM configuration in saved context */ > + pci_save_aspm_state(link->downstream); > + pci_save_aspm_l1ss_state(link->downstream); > + pci_save_aspm_state(parent); > + pci_save_aspm_l1ss_state(parent); > } > > static void pcie_config_aspm_path(struct pcie_link_state *link) > -- > 2.25.1 >
On 3/5/24 2:03 PM, Bjorn Helgaas wrote: > [+to Sathy, David in case you want to update your Reviewed-by] > > On Thu, Feb 22, 2024 at 11:14:36PM +0530, Vidya Sagar wrote: >> Many PCIe device drivers save the configuration state of their respective >> devices during probe and restore the same when their 'slot_reset' hook >> is called through PCIe Error Recovery Handler. >> >> If the system has a change in ASPM policy after the driver's probe is >> called and before error event occurred, 'slot_reset' hook restores the >> PCIe configuration state to what it was at the time of probe but not to >> what it was just before the occurrence of the error event. >> This effectively leads to a mismatch in the ASPM configuration between >> the device and its upstream parent device. >> >> Update the saved configuration state of the device with the latest info >> whenever there is a change w.r.t ASPM policy. >> >> Signed-off-by: Vidya Sagar <vidyas@nvidia.com> >> -void pci_save_aspm_state(struct pci_dev *pdev); >> +void pci_save_aspm_l1ss_state(struct pci_dev *pdev); > I rebased this again on top of my pci/aspm updates to remove the need > for the rename above. > >> +static void pci_save_aspm_state(struct pci_dev *dev) >> +{ >> + struct pci_cap_saved_state *save_state; >> + u16 *cap; >> + >> + if (!pci_is_pcie(dev)) >> + return; >> + >> + save_state = pci_find_saved_cap(dev, PCI_CAP_ID_EXP); >> + if (!save_state) >> + return; >> + >> + cap = (u16 *)&save_state->cap.data[0]; >> + pcie_capability_read_word(dev, PCI_EXP_LNKCTL, &cap[1]); > And I changed this part so it only updates the PCI_EXP_LNKCTL_ASPMC > bits, not the entire LNKCTL. > > Updating the entire saved register probably wouldn't *break* anything, > but it could randomly hide other LNKCTL changes depending on whether > or not ASPM configuration was changed in the interim. For example: > > - driver .probe() saves LNKCTL > - LNKCTL changes some non-ASPMC thing via setpci or other mechanism > - save_state updated via pcie_config_aspm_link() > > A restore in .slot_reset() would restore different LNKCTL values for > the non-ASPMC change depending on whether pcie_config_aspm_link() was > used. > > I applied it on pci/aspm for v6.9. Please take a look and make sure > it still does what you need: > https://git.kernel.org/pub/scm/linux/kernel/git/pci/pci.git/commit/?h=aspm&id=a6315434436d587f70e489e6365c5b7e20176a71 > > Sathy and David, I didn't add your Reviewed-by because I didn't want > to presume that you were OK with my changes. But I'd be more than > happy to add them if you take a look. Your update looks fine to me. Reviewed-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com> > > Bjorn > >> +} >> + >> void pci_aspm_get_l1ss(struct pci_dev *pdev) >> { >> /* Read L1 PM substate capabilities */ >> pdev->l1ss = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_L1SS); >> } >> >> -void pci_save_aspm_state(struct pci_dev *pdev) >> +void pci_save_aspm_l1ss_state(struct pci_dev *pdev) >> { >> struct pci_cap_saved_state *save_state; >> u16 l1ss = pdev->l1ss; >> @@ -309,10 +325,12 @@ static void pcie_set_clkpm_nocheck(struct pcie_link_state *link, int enable) >> struct pci_bus *linkbus = link->pdev->subordinate; >> u32 val = enable ? PCI_EXP_LNKCTL_CLKREQ_EN : 0; >> >> - list_for_each_entry(child, &linkbus->devices, bus_list) >> + list_for_each_entry(child, &linkbus->devices, bus_list) { >> pcie_capability_clear_and_set_word(child, PCI_EXP_LNKCTL, >> PCI_EXP_LNKCTL_CLKREQ_EN, >> val); >> + pci_save_aspm_state(child); >> + } >> link->clkpm_enabled = !!enable; >> } >> >> @@ -931,6 +949,12 @@ static void pcie_config_aspm_link(struct pcie_link_state *link, u32 state) >> pcie_config_aspm_dev(parent, upstream); >> >> link->aspm_enabled = state; >> + >> + /* Update latest ASPM configuration in saved context */ >> + pci_save_aspm_state(link->downstream); >> + pci_save_aspm_l1ss_state(link->downstream); >> + pci_save_aspm_state(parent); >> + pci_save_aspm_l1ss_state(parent); >> } >> >> static void pcie_config_aspm_path(struct pcie_link_state *link) >> -- >> 2.25.1 >>
On Tue, 2024-03-05 at 16:03 -0600, Bjorn Helgaas wrote: > [+to Sathy, David in case you want to update your Reviewed-by] > > On Thu, Feb 22, 2024 at 11:14:36PM +0530, Vidya Sagar wrote: > > Many PCIe device drivers save the configuration state of their respective > > devices during probe and restore the same when their 'slot_reset' hook > > is called through PCIe Error Recovery Handler. > > > > If the system has a change in ASPM policy after the driver's probe is > > called and before error event occurred, 'slot_reset' hook restores the > > PCIe configuration state to what it was at the time of probe but not to > > what it was just before the occurrence of the error event. > > This effectively leads to a mismatch in the ASPM configuration between > > the device and its upstream parent device. > > > > Update the saved configuration state of the device with the latest info > > whenever there is a change w.r.t ASPM policy. > > > > Signed-off-by: Vidya Sagar <vidyas@nvidia.com> > > > -void pci_save_aspm_state(struct pci_dev *pdev); > > +void pci_save_aspm_l1ss_state(struct pci_dev *pdev); > > I rebased this again on top of my pci/aspm updates to remove the need > for the rename above. > > > +static void pci_save_aspm_state(struct pci_dev *dev) > > +{ > > + struct pci_cap_saved_state *save_state; > > + u16 *cap; > > + > > + if (!pci_is_pcie(dev)) > > + return; > > + > > + save_state = pci_find_saved_cap(dev, PCI_CAP_ID_EXP); > > + if (!save_state) > > + return; > > + > > + cap = (u16 *)&save_state->cap.data[0]; > > + pcie_capability_read_word(dev, PCI_EXP_LNKCTL, &cap[1]); > > And I changed this part so it only updates the PCI_EXP_LNKCTL_ASPMC > bits, not the entire LNKCTL. > > Updating the entire saved register probably wouldn't *break* anything, > but it could randomly hide other LNKCTL changes depending on whether > or not ASPM configuration was changed in the interim. For example: > > - driver .probe() saves LNKCTL > - LNKCTL changes some non-ASPMC thing via setpci or other mechanism > - save_state updated via pcie_config_aspm_link() > > A restore in .slot_reset() would restore different LNKCTL values for > the non-ASPMC change depending on whether pcie_config_aspm_link() was > used. > > I applied it on pci/aspm for v6.9. Please take a look and make sure > it still does what you need: > https://git.kernel.org/pub/scm/linux/kernel/git/pci/pci.git/commit/?h=aspm&id=a6315434436d587f70e489e6365c5b7e20176a71 > > Sathy and David, I didn't add your Reviewed-by because I didn't want > to presume that you were OK with my changes. But I'd be more than > happy to add them if you take a look. > > Bjorn > > > +} > > + > > void pci_aspm_get_l1ss(struct pci_dev *pdev) > > { > > /* Read L1 PM substate capabilities */ > > pdev->l1ss = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_L1SS); > > } > > > > -void pci_save_aspm_state(struct pci_dev *pdev) > > +void pci_save_aspm_l1ss_state(struct pci_dev *pdev) > > { > > struct pci_cap_saved_state *save_state; > > u16 l1ss = pdev->l1ss; > > @@ -309,10 +325,12 @@ static void pcie_set_clkpm_nocheck(struct > > pcie_link_state *link, int enable) > > struct pci_bus *linkbus = link->pdev->subordinate; > > u32 val = enable ? PCI_EXP_LNKCTL_CLKREQ_EN : 0; > > > > - list_for_each_entry(child, &linkbus->devices, bus_list) > > + list_for_each_entry(child, &linkbus->devices, bus_list) { > > pcie_capability_clear_and_set_word(child, PCI_EXP_LNKCTL, > > PCI_EXP_LNKCTL_CLKREQ_EN, > > val); > > + pci_save_aspm_state(child); > > + } > > link->clkpm_enabled = !!enable; > > } > > > > @@ -931,6 +949,12 @@ static void pcie_config_aspm_link(struct > > pcie_link_state *link, u32 state) > > pcie_config_aspm_dev(parent, upstream); > > > > link->aspm_enabled = state; > > + > > + /* Update latest ASPM configuration in saved context */ > > + pci_save_aspm_state(link->downstream); > > + pci_save_aspm_l1ss_state(link->downstream); > > + pci_save_aspm_state(parent); > > + pci_save_aspm_l1ss_state(parent); > > } > > > > static void pcie_config_aspm_path(struct pcie_link_state *link) > > -- > > 2.25.1 > > Reviewed-by: David E. Box <david.e.box@linux.intel.com>
On Tue, Mar 05, 2024 at 04:03:42PM -0600, Bjorn Helgaas wrote: > [+to Sathy, David in case you want to update your Reviewed-by] > > On Thu, Feb 22, 2024 at 11:14:36PM +0530, Vidya Sagar wrote: > > Many PCIe device drivers save the configuration state of their respective > > devices during probe and restore the same when their 'slot_reset' hook > > is called through PCIe Error Recovery Handler. > > > > If the system has a change in ASPM policy after the driver's probe is > > called and before error event occurred, 'slot_reset' hook restores the > > PCIe configuration state to what it was at the time of probe but not to > > what it was just before the occurrence of the error event. > > This effectively leads to a mismatch in the ASPM configuration between > > the device and its upstream parent device. > > > > Update the saved configuration state of the device with the latest info > > whenever there is a change w.r.t ASPM policy. > > > > Signed-off-by: Vidya Sagar <vidyas@nvidia.com> > > > -void pci_save_aspm_state(struct pci_dev *pdev); > > +void pci_save_aspm_l1ss_state(struct pci_dev *pdev); > > I rebased this again on top of my pci/aspm updates to remove the need > for the rename above. > > > +static void pci_save_aspm_state(struct pci_dev *dev) > > +{ > > + struct pci_cap_saved_state *save_state; > > + u16 *cap; > > + > > + if (!pci_is_pcie(dev)) > > + return; > > + > > + save_state = pci_find_saved_cap(dev, PCI_CAP_ID_EXP); > > + if (!save_state) > > + return; > > + > > + cap = (u16 *)&save_state->cap.data[0]; > > + pcie_capability_read_word(dev, PCI_EXP_LNKCTL, &cap[1]); > > And I changed this part so it only updates the PCI_EXP_LNKCTL_ASPMC > bits, not the entire LNKCTL. > > Updating the entire saved register probably wouldn't *break* anything, > but it could randomly hide other LNKCTL changes depending on whether > or not ASPM configuration was changed in the interim. For example: > > - driver .probe() saves LNKCTL > - LNKCTL changes some non-ASPMC thing via setpci or other mechanism > - save_state updated via pcie_config_aspm_link() > > A restore in .slot_reset() would restore different LNKCTL values for > the non-ASPMC change depending on whether pcie_config_aspm_link() was > used. Oops, I blew it here. I think it's good to limit the LNKCTL changes we put in save_state, but PCI_EXP_LNKCTL_ASPMC is not enough. We should include PCI_EXP_LNKCTL_CLKREQ_EN as well since that may be updated in many of the same paths that update PCI_EXP_LNKCTL_ASPMC. I updated it to the patch at the bottom; here's the interdiff first: ------------------------------------------------------------------------- diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c index 46352132bb14..10160d82c10a 100644 --- a/drivers/pci/pcie/aspm.c +++ b/drivers/pci/pcie/aspm.c @@ -304,18 +304,25 @@ static int policy_to_clkpm_state(struct pcie_link_state *link) static void pci_update_aspm_saved_state(struct pci_dev *dev) { struct pci_cap_saved_state *save_state; - u16 *cap, lnkctl, aspmc; + u16 *cap, lnkctl, aspm_ctl; save_state = pci_find_saved_cap(dev, PCI_CAP_ID_EXP); if (!save_state) return; + pcie_capability_read_word(dev, PCI_EXP_LNKCTL, &lnkctl); + + /* + * Update ASPM and CLKREQ bits of LNKCTL in save_state. We only + * write PCI_EXP_LNKCTL_CCC during enumeration, so it shouldn't + * change after being captured in save_state. + */ + aspm_ctl = lnkctl & (PCI_EXP_LNKCTL_ASPMC | PCI_EXP_LNKCTL_CLKREQ_EN); + lnkctl &= ~(PCI_EXP_LNKCTL_ASPMC | PCI_EXP_LNKCTL_CLKREQ_EN); + /* Depends on pci_save_pcie_state(): cap[1] is LNKCTL */ cap = (u16 *)&save_state->cap.data[0]; - pcie_capability_read_word(dev, PCI_EXP_LNKCTL, &lnkctl); - aspmc = FIELD_GET(PCI_EXP_LNKCTL_ASPMC, lnkctl); - cap[1] = (lnkctl & ~PCI_EXP_LNKCTL_ASPMC) | - FIELD_PREP(PCI_EXP_LNKCTL_ASPMC, aspmc); + cap[1] = lnkctl | aspm_ctl; } static void pcie_set_clkpm_nocheck(struct pcie_link_state *link, int enable) ------------------------------------------------------------------------- And here's the entire patch: commit ff92b9348534 ("PCI/ASPM: Update save_state when configuration changes") Author: Vidya Sagar <vidyas@nvidia.com> Date: Fri Feb 23 13:36:24 2024 -0600 PCI/ASPM: Update save_state when configuration changes Many PCIe device drivers save the configuration state of their device during probe and restore it when their .slot_reset() hook is called during PCIe error recovery. If the ASPM configuration is changed after the driver's probe is called and before an error event occurs, .slot_reset() restores the ASPM configuration to what it was at the time of probe, not to what it was just before the occurrence of the error event. This leads to a mismatch in ASPM configuration between the device and its upstream device. Update the saved configuration of the device when the ASPM configuration changes. Link: https://lore.kernel.org/r/20240222174436.3565146-1-vidyas@nvidia.com Signed-off-by: Vidya Sagar <vidyas@nvidia.com> [bhelgaas: commit log, rebase to pci/aspm, rename to pci_update_aspm_saved_state() since it updates only LNKCTL, update only ASPMC and CLKREQ_EN in LNKCTL] Signed-off-by: Bjorn Helgaas <bhelgaas@google.com> Reviewed-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com> Reviewed-by: David E. Box <david.e.box@linux.intel.com> diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c index 1379b8decdf1..10160d82c10a 100644 --- a/drivers/pci/pcie/aspm.c +++ b/drivers/pci/pcie/aspm.c @@ -301,16 +301,42 @@ static int policy_to_clkpm_state(struct pcie_link_state *link) return 0; } +static void pci_update_aspm_saved_state(struct pci_dev *dev) +{ + struct pci_cap_saved_state *save_state; + u16 *cap, lnkctl, aspm_ctl; + + save_state = pci_find_saved_cap(dev, PCI_CAP_ID_EXP); + if (!save_state) + return; + + pcie_capability_read_word(dev, PCI_EXP_LNKCTL, &lnkctl); + + /* + * Update ASPM and CLKREQ bits of LNKCTL in save_state. We only + * write PCI_EXP_LNKCTL_CCC during enumeration, so it shouldn't + * change after being captured in save_state. + */ + aspm_ctl = lnkctl & (PCI_EXP_LNKCTL_ASPMC | PCI_EXP_LNKCTL_CLKREQ_EN); + lnkctl &= ~(PCI_EXP_LNKCTL_ASPMC | PCI_EXP_LNKCTL_CLKREQ_EN); + + /* Depends on pci_save_pcie_state(): cap[1] is LNKCTL */ + cap = (u16 *)&save_state->cap.data[0]; + cap[1] = lnkctl | aspm_ctl; +} + static void pcie_set_clkpm_nocheck(struct pcie_link_state *link, int enable) { struct pci_dev *child; struct pci_bus *linkbus = link->pdev->subordinate; u32 val = enable ? PCI_EXP_LNKCTL_CLKREQ_EN : 0; - list_for_each_entry(child, &linkbus->devices, bus_list) + list_for_each_entry(child, &linkbus->devices, bus_list) { pcie_capability_clear_and_set_word(child, PCI_EXP_LNKCTL, PCI_EXP_LNKCTL_CLKREQ_EN, val); + pci_update_aspm_saved_state(child); + } link->clkpm_enabled = !!enable; } @@ -929,6 +955,12 @@ static void pcie_config_aspm_link(struct pcie_link_state *link, u32 state) pcie_config_aspm_dev(parent, upstream); link->aspm_enabled = state; + + /* Update latest ASPM configuration in saved context */ + pci_save_aspm_l1ss_state(link->downstream); + pci_update_aspm_saved_state(link->downstream); + pci_save_aspm_l1ss_state(parent); + pci_update_aspm_saved_state(parent); } static void pcie_config_aspm_path(struct pcie_link_state *link)
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c index cfc5b84dc9c9..3db606ba9344 100644 --- a/drivers/pci/pci.c +++ b/drivers/pci/pci.c @@ -1648,7 +1648,7 @@ static int pci_save_pcie_state(struct pci_dev *dev) pcie_capability_read_word(dev, PCI_EXP_LNKCTL2, &cap[i++]); pcie_capability_read_word(dev, PCI_EXP_SLTCTL2, &cap[i++]); - pci_save_aspm_state(dev); + pci_save_aspm_l1ss_state(dev); pci_save_ltr_state(dev); return 0; diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h index b217e74966eb..9fe78eb8b07d 100644 --- a/drivers/pci/pci.h +++ b/drivers/pci/pci.h @@ -95,7 +95,7 @@ void pci_msix_init(struct pci_dev *dev); bool pci_bridge_d3_possible(struct pci_dev *dev); void pci_bridge_d3_update(struct pci_dev *dev); void pci_aspm_get_l1ss(struct pci_dev *pdev); -void pci_save_aspm_state(struct pci_dev *pdev); +void pci_save_aspm_l1ss_state(struct pci_dev *pdev); void pci_restore_aspm_state(struct pci_dev *pdev); void pci_save_ltr_state(struct pci_dev *dev); void pci_restore_ltr_state(struct pci_dev *dev); diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c index 7f1d674ff171..a62648dd52bc 100644 --- a/drivers/pci/pcie/aspm.c +++ b/drivers/pci/pcie/aspm.c @@ -24,13 +24,29 @@ #include "../pci.h" +static void pci_save_aspm_state(struct pci_dev *dev) +{ + struct pci_cap_saved_state *save_state; + u16 *cap; + + if (!pci_is_pcie(dev)) + return; + + save_state = pci_find_saved_cap(dev, PCI_CAP_ID_EXP); + if (!save_state) + return; + + cap = (u16 *)&save_state->cap.data[0]; + pcie_capability_read_word(dev, PCI_EXP_LNKCTL, &cap[1]); +} + void pci_aspm_get_l1ss(struct pci_dev *pdev) { /* Read L1 PM substate capabilities */ pdev->l1ss = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_L1SS); } -void pci_save_aspm_state(struct pci_dev *pdev) +void pci_save_aspm_l1ss_state(struct pci_dev *pdev) { struct pci_cap_saved_state *save_state; u16 l1ss = pdev->l1ss; @@ -309,10 +325,12 @@ static void pcie_set_clkpm_nocheck(struct pcie_link_state *link, int enable) struct pci_bus *linkbus = link->pdev->subordinate; u32 val = enable ? PCI_EXP_LNKCTL_CLKREQ_EN : 0; - list_for_each_entry(child, &linkbus->devices, bus_list) + list_for_each_entry(child, &linkbus->devices, bus_list) { pcie_capability_clear_and_set_word(child, PCI_EXP_LNKCTL, PCI_EXP_LNKCTL_CLKREQ_EN, val); + pci_save_aspm_state(child); + } link->clkpm_enabled = !!enable; } @@ -931,6 +949,12 @@ static void pcie_config_aspm_link(struct pcie_link_state *link, u32 state) pcie_config_aspm_dev(parent, upstream); link->aspm_enabled = state; + + /* Update latest ASPM configuration in saved context */ + pci_save_aspm_state(link->downstream); + pci_save_aspm_l1ss_state(link->downstream); + pci_save_aspm_state(parent); + pci_save_aspm_l1ss_state(parent); } static void pcie_config_aspm_path(struct pcie_link_state *link)
Many PCIe device drivers save the configuration state of their respective devices during probe and restore the same when their 'slot_reset' hook is called through PCIe Error Recovery Handler. If the system has a change in ASPM policy after the driver's probe is called and before error event occurred, 'slot_reset' hook restores the PCIe configuration state to what it was at the time of probe but not to what it was just before the occurrence of the error event. This effectively leads to a mismatch in the ASPM configuration between the device and its upstream parent device. Update the saved configuration state of the device with the latest info whenever there is a change w.r.t ASPM policy. Signed-off-by: Vidya Sagar <vidyas@nvidia.com> --- V4: * Rebased on top of pci/aspm branch V3: * Addressed sathyanarayanan.kuppuswamy's review comments V2: * Rebased on top of the tree code * Addressed Bjorn's review comments drivers/pci/pcie/aspm.c | 28 ++++++++++++++++++++++++++-- 3 files changed, 28 insertions(+), 4 deletions(-)