On Wed, Nov 06, 2024 at 12:54:12PM +0200, Ilpo Järvinen wrote: > On Tue, 5 Nov 2024, Bjorn Helgaas wrote: > > On Tue, Oct 01, 2024 at 04:34:42PM +0800, Jian-Hong Pan wrote: > > > PCI devices' parameters on the VMD bus have been programmed properly > > > originally. But, cleared after pci_reset_bus() and have not been restored > > > correctly. This leads the link's L1.2 between PCIe Root Port and child > > > device gets wrong configs. > ... > > > So, if the PCI device has a parent, make pci_save_aspm_l1ss_state() save > > > the parent's L1SS configuration, too. This is symmetric on > > > pci_restore_aspm_l1ss_state(). > > I see the suggestion for a helper here, but I'm not convinced. > > pci_save_aspm_l1ss_state() and pci_restore_aspm_l1ss_state() should > > *look* similar, and a helper makes them less similar. > > > > I think you should go to some effort to follow the > > pci_restore_aspm_l1ss_state() structure, as much as possible doing the > > same declarations, checks, and lookups in the same order, e.g.: > > > > struct pci_cap_saved_state *pl_save_state, *cl_save_state; > > struct pci_dev *parent = pdev->bus->self; > > > > if (pcie_downstream_port(pdev) || !parent) > > return; > > > > if (!pdev->l1ss || !parent->l1ss) > > return; > > > > cl_save_state = pci_find_saved_ext_cap(pdev, PCI_EXT_CAP_ID_L1SS); > > pl_save_state = pci_find_saved_ext_cap(parent, PCI_EXT_CAP_ID_L1SS); > > if (!cl_save_state || !pl_save_state) > > return; > > I understand I'm not the one who has the final say in this, but the reason > why restore has to be done the way it is (the long way), is because of the > strict ordering requirement of operations it performs. > > There are no similar ordering requirements on the save side AFAIK. I'm not suggesting any change to the restore side. The commit log says we're making save/restore symmetric, but IMO they end up looking very asymmetric. Bjorn
