| Message ID | 20240108124248.1552420-1-vidyas@nvidia.com |
|---|---|
| State | New |
| Headers | show |
| Series | [V3] PCI/ASPM: Update saved buffers with latest ASPM | expand |
Hi Bjorn, Any comments for this patch? Thanks, Vidya Sagar On 08-01-2024 18:12, 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 with > 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> > --- > 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 | 24 +++++++++++++++++++++++- > 1 file changed, 23 insertions(+), 1 deletion(-) > > diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c > index 67b13f26ba7c..1b4f03044ce2 100644 > --- a/drivers/pci/pcie/aspm.c > +++ b/drivers/pci/pcie/aspm.c > @@ -138,16 +138,34 @@ static int policy_to_clkpm_state(struct pcie_link_state *link) > return 0; > } > > +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]); > +} > + > 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_save_aspm_state(child); > + } > link->clkpm_enabled = !!enable; > } > > @@ -767,6 +785,10 @@ 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_state(parent); > } > > static void pcie_config_aspm_path(struct pcie_link_state *link)
[+cc David] On Mon, Jan 08, 2024 at 06:12:48PM +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 with > 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> This overlaps with David's patches that are currently queued for v6.9: https://git.kernel.org/pub/scm/linux/kernel/git/pci/pci.git/log/?h=be00f078ad2a Can you rebase this to apply on top of those (this is the 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 | 24 +++++++++++++++++++++++- > 1 file changed, 23 insertions(+), 1 deletion(-) > > diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c > index 67b13f26ba7c..1b4f03044ce2 100644 > --- a/drivers/pci/pcie/aspm.c > +++ b/drivers/pci/pcie/aspm.c > @@ -138,16 +138,34 @@ static int policy_to_clkpm_state(struct pcie_link_state *link) > return 0; > } > > +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]); > +} > + > 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_save_aspm_state(child); > + } > link->clkpm_enabled = !!enable; > } > > @@ -767,6 +785,10 @@ 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_state(parent); > } > > static void pcie_config_aspm_path(struct pcie_link_state *link) > -- > 2.25.1 >
diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c index 67b13f26ba7c..1b4f03044ce2 100644 --- a/drivers/pci/pcie/aspm.c +++ b/drivers/pci/pcie/aspm.c @@ -138,16 +138,34 @@ static int policy_to_clkpm_state(struct pcie_link_state *link) return 0; } +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]); +} + 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_save_aspm_state(child); + } link->clkpm_enabled = !!enable; } @@ -767,6 +785,10 @@ 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_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 with 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> --- 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 | 24 +++++++++++++++++++++++- 1 file changed, 23 insertions(+), 1 deletion(-)