linux-kernel - Re: [PATCH 1/1] PCI: Move reset and restore related code to reset-restore.c

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] [day] [month] [year] [list]

Message-ID: <096fbf51-cbbf-67ca-862d-9563f88d7f2d@linux.intel.com>
Date: Wed, 14 May 2025 15:09:44 +0300 (EEST)
From: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
To: Lukas Wunner <lukas@...ner.de>
cc: Bjorn Helgaas <bhelgaas@...gle.com>, LKML <linux-kernel@...r.kernel.org>, 
    linux-pci@...r.kernel.org
Subject: Re: [PATCH 1/1] PCI: Move reset and restore related code to
 reset-restore.c

On Wed, 14 May 2025, Lukas Wunner wrote:

> On Wed, May 14, 2025 at 02:29:42PM +0300, Ilpo Järvinen wrote:
> > On Wed, 14 May 2025, Lukas Wunner wrote:
> > > > --- a/drivers/pci/pci.c
> > > > +++ b/drivers/pci/pci.c
> > > > @@ -69,15 +69,7 @@ struct pci_pme_device {
> > > >   */
> > > >  #define PCI_RESET_WAIT 1000 /* msec */
> > > 
> > > I'd move PCI_RESET_WAIT, pci_dev_wait() and
> > > pci_bridge_wait_for_secondary_bus() to reset.c as well.
> > > Then pci_dev_d3_sleep() is the only function which is no longer static.
> > 
> > Okay I'll move those as well but that static statement is not exactly 
> > true, I'll these need to do these as well:
> > 
> > - move pci_bus_max_d3cold_delay() along with 
> >   pci_bridge_wait_for_secondary_bus() to keep that static, or turn that
> >   into a non-static.
> > 
> > - make pcie_wait_for_link_delay() non-static.
> 
> Sorry, missed that.  In that case I suggest moving pcie_wait_for_link()
> as well.  It is already non-static.  Then you can keep the static on
> pcie_wait_for_link_delay().
>
> Per PCIe r6.2 sec 5.8, a transition from D3cold to D0uninitialized
> implies a Fundamental Reset.  That could serve as a creative
> justification in the commit message or in the code comment at the
> top of reset.c why it contains D3cold-related functions. ;)

Great, this is mushrooming fast :-).

That implies making pcie_wait_for_link_status() non-static, regardless of 
location or moving also pcie_retrain_link() which I suppose can't really 
be excused to reside in reset.c :-). Now that I think about it, I recall 
I ran across this problem which is why the split ended where it was in 
this submission.

Maybe, it would make sense to have not only reset.c but also link.c 
(for waiting, retraining, and link speed related functions). Those two 
files (link.c and reset.c) are kind of inter-related still but the 
interface between looks relatively sane and should be one-way (I think, I 
didn't attempt this dual split yet).

-- 
 i.