[v3,1/2] PCI: don't rely on of_platform_depopulate() for reused OF-nodes

Commit Message

Bartosz Golaszewski Aug. 23, 2024, 9:33 a.m. UTC

From: Bartosz Golaszewski <[e-mail]>

of_platform_depopulate() doesn't play nice with reused OF nodes - it
ignores the ones that are not marked explicitly as populated and it may
happen that the PCI device goes away before the platform device in which
case the PCI core clears the OF_POPULATED bit. We need to
unconditionally unregister the platform devices for child nodes when
stopping the PCI device.

Fixes: 8fb18619d910 ("PCI/pwrctl: Create platform devices for child OF nodes of the port node")
Signed-off-by: Bartosz Golaszewski <[e-mail]>
---
 drivers/pci/remove.c | 18 +++++++++++++++++-
 1 file changed, 17 insertions(+), 1 deletion(-)

Comments

Bjorn Helgaas Aug. 23, 2024, 10:10 p.m. UTC | #1

[+to Rob]

On Fri, Aug 23, 2024 at 11:33:22AM +0200, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <[e-mail]>
> 
> of_platform_depopulate() doesn't play nice with reused OF nodes - it
> ignores the ones that are not marked explicitly as populated and it may
> happen that the PCI device goes away before the platform device in which
> case the PCI core clears the OF_POPULATED bit. We need to
> unconditionally unregister the platform devices for child nodes when
> stopping the PCI device.

Rob, any concerns with this?

> Fixes: 8fb18619d910 ("PCI/pwrctl: Create platform devices for child OF nodes of the port node")
> Signed-off-by: Bartosz Golaszewski <[e-mail]>
> ---
>  drivers/pci/remove.c | 18 +++++++++++++++++-
>  1 file changed, 17 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/remove.c b/drivers/pci/remove.c
> index 910387e5bdbf..4770cb87e3f0 100644
> --- a/drivers/pci/remove.c
> +++ b/drivers/pci/remove.c
> @@ -1,7 +1,10 @@
>  // SPDX-License-Identifier: GPL-2.0
>  #include <linux/pci.h>
>  #include <linux/module.h>
> +#include <linux/of.h>
>  #include <linux/of_platform.h>
> +#include <linux/platform_device.h>
> +
>  #include "pci.h"
>  
>  static void pci_free_resources(struct pci_dev *dev)
> @@ -14,12 +17,25 @@ static void pci_free_resources(struct pci_dev *dev)
>  	}
>  }
>  
> +static int pci_pwrctl_unregister(struct device *dev, void *data)
> +{
> +	struct device_node *pci_node = data, *plat_node = dev_of_node(dev);
> +
> +	if (dev_is_platform(dev) && plat_node && plat_node == pci_node) {
> +		of_device_unregister(to_platform_device(dev));
> +		of_node_clear_flag(plat_node, OF_POPULATED);
> +	}
> +
> +	return 0;
> +}
> +
>  static void pci_stop_dev(struct pci_dev *dev)
>  {
>  	pci_pme_active(dev, false);
>  
>  	if (pci_dev_is_added(dev)) {
> -		of_platform_depopulate(&dev->dev);
> +		device_for_each_child(dev->dev.parent, dev_of_node(&dev->dev),
> +				      pci_pwrctl_unregister);
>  		device_release_driver(&dev->dev);
>  		pci_proc_detach_device(dev);
>  		pci_remove_sysfs_dev_files(dev);
> -- 
> 2.43.0
>

Bartosz Golaszewski Aug. 24, 2024, 7:49 a.m. UTC | #2

On Sat, Aug 24, 2024 at 12:10 AM Bjorn Helgaas <[e-mail]> wrote:
>
> [+to Rob]
>
> On Fri, Aug 23, 2024 at 11:33:22AM +0200, Bartosz Golaszewski wrote:
> > From: Bartosz Golaszewski <[e-mail]>
> >
> > of_platform_depopulate() doesn't play nice with reused OF nodes - it
> > ignores the ones that are not marked explicitly as populated and it may
> > happen that the PCI device goes away before the platform device in which
> > case the PCI core clears the OF_POPULATED bit. We need to
> > unconditionally unregister the platform devices for child nodes when
> > stopping the PCI device.
>
> Rob, any concerns with this?
>

If there will be any concerns: I'm OoO next week so I'll only be able
to address them on September 2nd.

Bart

Manivannan Sadhasivam Aug. 27, 2024, 8:40 a.m. UTC | #3

+ Rob

On Fri, Aug 23, 2024 at 11:33:22AM +0200, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <[e-mail]>
> 
> of_platform_depopulate() doesn't play nice with reused OF nodes - it
> ignores the ones that are not marked explicitly as populated and it may
> happen that the PCI device goes away before the platform device in which
> case the PCI core clears the OF_POPULATED bit. We need to
> unconditionally unregister the platform devices for child nodes when
> stopping the PCI device.
> 

It sounds like the fix is in of_platform_depopulate() itself and this patch
works around the API issue in PCI driver.

Rob, is that correct?

- Mani

> Fixes: 8fb18619d910 ("PCI/pwrctl: Create platform devices for child OF nodes of the port node")
> Signed-off-by: Bartosz Golaszewski <[e-mail]>
> ---
>  drivers/pci/remove.c | 18 +++++++++++++++++-
>  1 file changed, 17 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/remove.c b/drivers/pci/remove.c
> index 910387e5bdbf..4770cb87e3f0 100644
> --- a/drivers/pci/remove.c
> +++ b/drivers/pci/remove.c
> @@ -1,7 +1,10 @@
>  // SPDX-License-Identifier: GPL-2.0
>  #include <linux/pci.h>
>  #include <linux/module.h>
> +#include <linux/of.h>
>  #include <linux/of_platform.h>
> +#include <linux/platform_device.h>
> +
>  #include "pci.h"
>  
>  static void pci_free_resources(struct pci_dev *dev)
> @@ -14,12 +17,25 @@ static void pci_free_resources(struct pci_dev *dev)
>  	}
>  }
>  
> +static int pci_pwrctl_unregister(struct device *dev, void *data)
> +{
> +	struct device_node *pci_node = data, *plat_node = dev_of_node(dev);
> +
> +	if (dev_is_platform(dev) && plat_node && plat_node == pci_node) {
> +		of_device_unregister(to_platform_device(dev));
> +		of_node_clear_flag(plat_node, OF_POPULATED);
> +	}
> +
> +	return 0;
> +}
> +
>  static void pci_stop_dev(struct pci_dev *dev)
>  {
>  	pci_pme_active(dev, false);
>  
>  	if (pci_dev_is_added(dev)) {
> -		of_platform_depopulate(&dev->dev);
> +		device_for_each_child(dev->dev.parent, dev_of_node(&dev->dev),
> +				      pci_pwrctl_unregister);
>  		device_release_driver(&dev->dev);
>  		pci_proc_detach_device(dev);
>  		pci_remove_sysfs_dev_files(dev);
> -- 
> 2.43.0
>

Bartosz Golaszewski Aug. 27, 2024, 12:25 p.m. UTC | #4

On Tue, Aug 27, 2024 at 10:40 AM Manivannan Sadhasivam
<[e-mail]> wrote:
>
> + Rob
>
> On Fri, Aug 23, 2024 at 11:33:22AM +0200, Bartosz Golaszewski wrote:
> > From: Bartosz Golaszewski <[e-mail]>
> >
> > of_platform_depopulate() doesn't play nice with reused OF nodes - it
> > ignores the ones that are not marked explicitly as populated and it may
> > happen that the PCI device goes away before the platform device in which
> > case the PCI core clears the OF_POPULATED bit. We need to
> > unconditionally unregister the platform devices for child nodes when
> > stopping the PCI device.
> >
>
> It sounds like the fix is in of_platform_depopulate() itself and this patch
> works around the API issue in PCI driver.
>
> Rob, is that correct?
>
> - Mani

of_platform_depopulate() has more issues than just that. For one: it's
asymmetric to of_platform_populate() as it takes a struct device as
argument and not a device node. This causes issues for users like TI
aemif that call of_platform_populate() on nodes without the compatible
property that are never consumed by any device. AFAIK there's
currently no way to depopulate them.

In this particular case I think that the OF_POPULATED bit should not
be set when the PCI device is created but only when the platform
device is.

However I'm afraid to change the semantics of of_platform_depopulate()
et al for all users so I'm more inclined to have this fix in v6.11 to
avoid releasing non functional code (pwrctl devices not being removed)
and then possibly introduce a new variant of of_platform_depopulate()
that would work slightly differently.

Bart

Rob Herring Aug. 27, 2024, 12:55 p.m. UTC | #5

On Fri, Aug 23, 2024 at 5:10 PM Bjorn Helgaas <[e-mail]> wrote:
>
> [+to Rob]
>
> On Fri, Aug 23, 2024 at 11:33:22AM +0200, Bartosz Golaszewski wrote:
> > From: Bartosz Golaszewski <[e-mail]>
> >
> > of_platform_depopulate() doesn't play nice with reused OF nodes - it
> > ignores the ones that are not marked explicitly as populated and it may
> > happen that the PCI device goes away before the platform device in which
> > case the PCI core clears the OF_POPULATED bit. We need to
> > unconditionally unregister the platform devices for child nodes when
> > stopping the PCI device.
>
> Rob, any concerns with this?

Yes, the flag bits are a mess. I don't have a better solution other
than perhaps what Bartosz suggests.

I was going to say we shouldn't really be mucking with the
OF_POPULATED flag outside the DT code, but then I grep'ed the tree. :(

Rob

Manivannan Sadhasivam Aug. 27, 2024, 2:09 p.m. UTC | #6

On Tue, Aug 27, 2024 at 02:25:58PM +0200, Bartosz Golaszewski wrote:
> On Tue, Aug 27, 2024 at 10:40 AM Manivannan Sadhasivam
> <[e-mail]> wrote:
> >
> > + Rob
> >
> > On Fri, Aug 23, 2024 at 11:33:22AM +0200, Bartosz Golaszewski wrote:
> > > From: Bartosz Golaszewski <[e-mail]>
> > >
> > > of_platform_depopulate() doesn't play nice with reused OF nodes - it
> > > ignores the ones that are not marked explicitly as populated and it may
> > > happen that the PCI device goes away before the platform device in which
> > > case the PCI core clears the OF_POPULATED bit. We need to
> > > unconditionally unregister the platform devices for child nodes when
> > > stopping the PCI device.
> > >
> >
> > It sounds like the fix is in of_platform_depopulate() itself and this patch
> > works around the API issue in PCI driver.
> >
> > Rob, is that correct?
> >
> > - Mani
> 
> of_platform_depopulate() has more issues than just that. For one: it's
> asymmetric to of_platform_populate() as it takes a struct device as
> argument and not a device node. This causes issues for users like TI
> aemif that call of_platform_populate() on nodes without the compatible
> property that are never consumed by any device. AFAIK there's
> currently no way to depopulate them.
> 

Oouch!

> In this particular case I think that the OF_POPULATED bit should not
> be set when the PCI device is created but only when the platform
> device is.
> 
> However I'm afraid to change the semantics of of_platform_depopulate()
> et al for all users so I'm more inclined to have this fix in v6.11 to
> avoid releasing non functional code (pwrctl devices not being removed)
> and then possibly introduce a new variant of of_platform_depopulate()
> that would work slightly differently.
> 

Ok, sounds like a plan. Since Rob is also in favor of this patch, it is good to
get this series merged for 6.11.

Acked-by: Manivannan Sadhasivam <[e-mail]>

- Mani

Patch

diff --git a/drivers/pci/remove.c b/drivers/pci/remove.c
index 910387e5bdbf..4770cb87e3f0 100644
--- a/drivers/pci/remove.c
+++ b/drivers/pci/remove.c
@@ -1,7 +1,10 @@ 
 // SPDX-License-Identifier: GPL-2.0
 #include <linux/pci.h>
 #include <linux/module.h>
+#include <linux/of.h>
 #include <linux/of_platform.h>
+#include <linux/platform_device.h>
+
 #include "pci.h"
 
 static void pci_free_resources(struct pci_dev *dev)
@@ -14,12 +17,25 @@  static void pci_free_resources(struct pci_dev *dev)
 	}
 }
 
+static int pci_pwrctl_unregister(struct device *dev, void *data)
+{
+	struct device_node *pci_node = data, *plat_node = dev_of_node(dev);
+
+	if (dev_is_platform(dev) && plat_node && plat_node == pci_node) {
+		of_device_unregister(to_platform_device(dev));
+		of_node_clear_flag(plat_node, OF_POPULATED);
+	}
+
+	return 0;
+}
+
 static void pci_stop_dev(struct pci_dev *dev)
 {
 	pci_pme_active(dev, false);
 
 	if (pci_dev_is_added(dev)) {
-		of_platform_depopulate(&dev->dev);
+		device_for_each_child(dev->dev.parent, dev_of_node(&dev->dev),
+				      pci_pwrctl_unregister);
 		device_release_driver(&dev->dev);
 		pci_proc_detach_device(dev);
 		pci_remove_sysfs_dev_files(dev);