Re: [PATCH v12 3/3] PCI/ASPM: Make pci_save_aspm_l1ss_state save both child and parent's L1SS configuration

[Bjorn Helgaas <helgaas@kernel.org>]

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