Re: [PATCH v5 1/4] PCI/portdrv: Make use of pci_dev::bridge_d3 for checking the D3 possibility

[Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>]

On Fri, Aug 02, 2024 at 10:07:39PM +0200, Lukas Wunner wrote:
> On Fri, Aug 02, 2024 at 01:19:24PM +0200, Rafael J. Wysocki wrote:
> > On Fri, Aug 2, 2024 at 11:49AM Lukas Wunner <lukas@wunner.de> wrote:
> > >
> > > On Fri, Aug 02, 2024 at 11:25:00AM +0530, Manivannan Sadhasivam via B4 Relay wrote:
> > > > PCI core is already caching the value of pci_bridge_d3_possible() in
> > > > pci_dev::bridge_d3 during pci_pm_init(). Since the value is not going to
> > > > change,
> > 
> > Is that really the case?
> > 
> > Have you seen pci_bridge_d3_update()?
> 
> Okay the value may change at runtime, e.g. due to user space
> manipulating d3cold_allowed in sysfs.
> 

The last part of the commit message is wrong, but pci_bridge_d3_update() is
already updating pci_dev::bridge_d3. And pci_bridge_d3_possible() is not making
use of any checks that could change dynamically IIUC. So what is wrong in using
pci_dev::bridge_d3?

Even more, if the client drivers have changed the state of pci_dev::bridge_d3
during runtime, then pci_bridge_d3_possible() won't catch that. Or is there a
reason to not do so purposefully?

> > > I don't know if there was a reason to call pci_bridge_d3_possible()
> > > (instead of using the cached value) on probe, remove and shutdown.
> > >
> > > The change is probably safe but it would still be good to get some
> > > positive test results with Thunderbolt laptops etc to raise the
> > > confidence.
> > 
> > If I'm not mistaken, the change is not correct.
> 
> You're right.  Because the value may change, different code paths
> may be chosen on probe, remove and shutdown.  Sorry for missing that.
> 

Again, pci_bridge_d3_possible() is not making use of values that could change
dynamically.

- Mani

-- 
மணிவண்ணன் சதாசிவம்