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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20241106171604.GA1529996@bhelgaas>
Date: Wed, 6 Nov 2024 11:16:04 -0600
From: Bjorn Helgaas <helgaas@...nel.org>
To: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
Cc: Jian-Hong Pan <jhp@...lessos.org>, Johan Hovold <johan@...nel.org>,
	David Box <david.e.box@...ux.intel.com>,
	Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@...ux.intel.com>,
	Nirmal Patel <nirmal.patel@...ux.intel.com>,
	Jonathan Derrick <jonathan.derrick@...ux.dev>,
	linux-pci@...r.kernel.org, LKML <linux-kernel@...r.kernel.org>,
	linux@...lessos.org
Subject: Re: [PATCH v12 3/3] PCI/ASPM: Make pci_save_aspm_l1ss_state save
 both child and parent's L1SS configuration

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