On Wed, Jan 25, 2023 at 07:08:30PM +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 System. This strategy of simply restoring config space after a reset is common, but I think it's only a 90% solution. After reset, the device is basically in a "fresh poweron" state [1]. At boot-time or for a hot-added device, we do a lot of setup when we enumerate the device, and assuming that: - device reset, plus - current state in the struct pci_dev, plus - restoring config space gets all the device and kernel state to the same place is a pretty big assumption. That said, we're pretty invested in this strategy for now, and I think what you propose here is definitely an improvement. Minor comments on the implementation below. Bjorn [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/PCI/pci-error-recovery.rst?id=v6.1#n277 > 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. > This patch addresses that issue by updating 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@xxxxxxxxxx> > --- > drivers/pci/pci.h | 4 ++++ > drivers/pci/pcie/aspm.c | 40 ++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 44 insertions(+) > +++ b/drivers/pci/pci.h > +void pci_save_aspm_state(struct pci_dev *dev); > +void pci_restore_aspm_state(struct pci_dev *dev); This patch only adds calls to these functions in aspm.c, so it doesn't look like we need declarations here or stubs below. > +static inline void pci_save_aspm_state(struct pci_dev *dev) { } > +static inline void pci_restore_aspm_state(struct pci_dev *dev) { } > diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c > index 53a1fa306e1e..f25e0440d36b 100644 > --- a/drivers/pci/pcie/aspm.c > +++ b/drivers/pci/pcie/aspm.c > @@ -151,6 +151,7 @@ static void pcie_set_clkpm_nocheck(struct pcie_link_state *link, int enable) > PCI_EXP_LNKCTL_CLKREQ_EN, > val); > link->clkpm_enabled = !!enable; > + pci_save_aspm_state(child); > } > > static void pcie_set_clkpm(struct pcie_link_state *link, int enable) > @@ -757,6 +758,39 @@ static void pcie_config_aspm_l1ss(struct pcie_link_state *link, u32 state) > PCI_L1SS_CTL1_L1SS_MASK, val); > } > > +void pci_save_aspm_state(struct pci_dev *dev) I might be missing something because these look like they should be static. But the declarations and these being non-static suggest that you might have something more in mind that isn't part of this patch? Move these save-state functions higher up if necessary to resolve the forward reference from pcie_set_clkpm_nocheck(). > +{ > + int i = 0; > + 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]; > + i++; "i" looks unnecessary, but I guess I see what you're doing -- mirroring the structure of pci_save_pcie_state() to make sure we put LNKCTL in the correct element of cap[]. > + pcie_capability_read_word(dev, PCI_EXP_LNKCTL, &cap[i++]); > +} > + > +void pci_restore_aspm_state(struct pci_dev *dev) No callers for this? And I don't see why you would *need* callers; this should be restored by pci_restore_pcie_state() already. So this looks like it could be removed completely. > +{ > + int i = 0; > + struct pci_cap_saved_state *save_state; > + u16 *cap; > + > + save_state = pci_find_saved_cap(dev, PCI_CAP_ID_EXP); > + if (!save_state) > + return; > + > + cap = (u16 *)&save_state->cap.data[0]; > + i++; > + pcie_capability_write_word(dev, PCI_EXP_LNKCTL, cap[i++]); > +} > + > void pci_save_aspm_l1ss_state(struct pci_dev *dev) > { > struct pci_cap_saved_state *save_state; > @@ -849,6 +883,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.17.1 >
