Re: [PATCH v5 1/4] PCI/portdrv: Make use of pci_dev::bridge_d3 for checking the D3 possibility
From: Manivannan Sadhasivam
Date: Mon Aug 05 2024 - 09:25:01 EST
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@xxxxxxxxx> 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
--
மணிவண்ணன் சதாசிவம்