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

Commit Message

Manivannan Sadhasivam via B4 Relay Aug. 2, 2024, 5:55 a.m. UTC

From: Manivannan Sadhasivam <[e-mail]>

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, let's make use of the cached value.

Tested-by: Hsin-Yi Wang <[e-mail]>
Signed-off-by: Manivannan Sadhasivam <[e-mail]>
---
 drivers/pci/pcie/portdrv.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

Comments

Lukas Wunner Aug. 2, 2024, 9:49 a.m. UTC | #1

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, let's make use of the cached value.
[...]
> --- a/drivers/pci/pcie/portdrv.c
> +++ b/drivers/pci/pcie/portdrv.c
> @@ -702,7 +702,7 @@ static int pcie_portdrv_probe(struct pci_dev *dev,
>  	dev_pm_set_driver_flags(&dev->dev, DPM_FLAG_NO_DIRECT_COMPLETE |
>  					   DPM_FLAG_SMART_SUSPEND);
>  
> -	if (pci_bridge_d3_possible(dev)) {
> +	if (dev->bridge_d3) {

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.

Thanks,

Lukas

Rafael J. Wysocki Aug. 2, 2024, 11:19 a.m. UTC | #2

On Fri, Aug 2, 2024 at 11:49 AM Lukas Wunner <[e-mail]> 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()?

> let's make use of the cached value.
> [...]
> > --- a/drivers/pci/pcie/portdrv.c
> > +++ b/drivers/pci/pcie/portdrv.c
> > @@ -702,7 +702,7 @@ static int pcie_portdrv_probe(struct pci_dev *dev,
> >       dev_pm_set_driver_flags(&dev->dev, DPM_FLAG_NO_DIRECT_COMPLETE |
> >                                          DPM_FLAG_SMART_SUSPEND);
> >
> > -     if (pci_bridge_d3_possible(dev)) {
> > +     if (dev->bridge_d3) {
>
> 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.

Lukas Wunner Aug. 2, 2024, 8:07 p.m. UTC | #3

On Fri, Aug 02, 2024 at 01:19:24PM +0200, Rafael J. Wysocki wrote:
> On Fri, Aug 2, 2024 at 11:49AM Lukas Wunner <[e-mail]> 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.

> > 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.

Thanks,

Lukas

Manivannan Sadhasivam Aug. 5, 2024, 1:24 p.m. UTC | #4

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 <[e-mail]> 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

Lukas Wunner Aug. 6, 2024, 6:46 a.m. UTC | #5

On Mon, Aug 05, 2024 at 06:54:42PM +0530, Manivannan Sadhasivam wrote:
> So what is wrong in using pci_dev::bridge_d3?

The bridge_d3 flag may change at runtime, e.g. when writing to the
d3cold_allowed attribute in sysfs.

If e.g. bridge_d3 is set when pcie_portdrv_probe() runs but no longer
set when pcie_portdrv_remove() runs, there would be a runtime PM ref
imbalance.  (Ref would be dropped on probe, but not reacquired on remove.)


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

Which is precisely the reason why it (and not the bridge_d3 flag) is
used by pcie_portdrv_{probe,remove,shutdown}().

Thanks,

Lukas

Manivannan Sadhasivam Aug. 6, 2024, 11:48 a.m. UTC | #6

On Tue, Aug 06, 2024 at 08:46:48AM +0200, Lukas Wunner wrote:
> On Mon, Aug 05, 2024 at 06:54:42PM +0530, Manivannan Sadhasivam wrote:
> > So what is wrong in using pci_dev::bridge_d3?
> 
> The bridge_d3 flag may change at runtime, e.g. when writing to the
> d3cold_allowed attribute in sysfs.
> 
> If e.g. bridge_d3 is set when pcie_portdrv_probe() runs but no longer
> set when pcie_portdrv_remove() runs, there would be a runtime PM ref
> imbalance.  (Ref would be dropped on probe, but not reacquired on remove.)
> 

Ah, so it is the other way around. Thanks for clearing it up!

- Mani

Patch

diff --git a/drivers/pci/pcie/portdrv.c b/drivers/pci/pcie/portdrv.c
index 14a4b89a3b83..1f02e5d7b2e9 100644
--- a/drivers/pci/pcie/portdrv.c
+++ b/drivers/pci/pcie/portdrv.c
@@ -702,7 +702,7 @@  static int pcie_portdrv_probe(struct pci_dev *dev,
 	dev_pm_set_driver_flags(&dev->dev, DPM_FLAG_NO_DIRECT_COMPLETE |
 					   DPM_FLAG_SMART_SUSPEND);
 
-	if (pci_bridge_d3_possible(dev)) {
+	if (dev->bridge_d3) {
 		/*
 		 * Keep the port resumed 100ms to make sure things like
 		 * config space accesses from userspace (lspci) will not
@@ -720,7 +720,7 @@  static int pcie_portdrv_probe(struct pci_dev *dev,
 
 static void pcie_portdrv_remove(struct pci_dev *dev)
 {
-	if (pci_bridge_d3_possible(dev)) {
+	if (dev->bridge_d3) {
 		pm_runtime_forbid(&dev->dev);
 		pm_runtime_get_noresume(&dev->dev);
 		pm_runtime_dont_use_autosuspend(&dev->dev);
@@ -733,7 +733,7 @@  static void pcie_portdrv_remove(struct pci_dev *dev)
 
 static void pcie_portdrv_shutdown(struct pci_dev *dev)
 {
-	if (pci_bridge_d3_possible(dev)) {
+	if (dev->bridge_d3) {
 		pm_runtime_forbid(&dev->dev);
 		pm_runtime_get_noresume(&dev->dev);
 		pm_runtime_dont_use_autosuspend(&dev->dev);