[PATCH v3 0/2] PCI/pwrctl: fixes for v6.11

[PATCH v3 0/2] PCI/pwrctl: fixes for v6.11

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

Bjorn,

Here's a respin of the PCI/pwrctl fixes that should go into v6.11. I
eventually found a solution that doesn't require Krishna's topology
change but Krishna: please make sure to update the code in
drivers/pci/remove.c as well when rebasing your work once this gets
upstream.

v2 -> v3:
- use the correct device for unregistering the platform pwrctl device in
  patch 1/2
- make patch 1/2 easier to read by using device_for_each_child()

v1 -> v2:
- use the scoped variant of for_each_child_of_node() to fix a memory
  leak in patch 1/2

Bartosz Golaszewski (2):
  PCI: don't rely on of_platform_depopulate() for reused OF-nodes
  PCI/pwrctl: put the bus rescan on a different thread

 drivers/pci/pwrctl/core.c              | 26 +++++++++++++++++++++++---
 drivers/pci/pwrctl/pci-pwrctl-pwrseq.c |  2 +-
 drivers/pci/remove.c                   | 18 +++++++++++++++++-
 include/linux/pci-pwrctl.h             |  3 +++
 4 files changed, 44 insertions(+), 5 deletions(-)

-- 
2.43.0

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

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

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 <bartosz.golaszewski@linaro.org>
---
 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

[PATCH v3 2/2] PCI/pwrctl: put the bus rescan on a different thread

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

If we trigger the bus rescan from sysfs, we'll try to lock the PCI
rescan mutex recursively and deadlock - the platform device will be
populated and probed on the same thread that handles the sysfs write.

Add a workqueue to the pwrctl code on which we schedule the rescan for
controlled PCI devices. While at it: add a new interface for
initializing the pwrctl context where we'd now assign the parent device
address and initialize the workqueue.

Fixes: 4565d2652a37 ("PCI/pwrctl: Add PCI power control core code")
Reported-by: Konrad Dybcio <konradybcio@kernel.org>
Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
 drivers/pci/pwrctl/core.c              | 26 +++++++++++++++++++++++---
 drivers/pci/pwrctl/pci-pwrctl-pwrseq.c |  2 +-
 include/linux/pci-pwrctl.h             |  3 +++
 3 files changed, 27 insertions(+), 4 deletions(-)

diff --git a/drivers/pci/pwrctl/core.c b/drivers/pci/pwrctl/core.c
index feca26ad2f6a..01d913b60316 100644
--- a/drivers/pci/pwrctl/core.c
+++ b/drivers/pci/pwrctl/core.c
@@ -48,6 +48,28 @@ static int pci_pwrctl_notify(struct notifier_block *nb, unsigned long action,
 	return NOTIFY_DONE;
 }
 
+static void rescan_work_func(struct work_struct *work)
+{
+	struct pci_pwrctl *pwrctl = container_of(work, struct pci_pwrctl, work);
+
+	pci_lock_rescan_remove();
+	pci_rescan_bus(to_pci_dev(pwrctl->dev->parent)->bus);
+	pci_unlock_rescan_remove();
+}
+
+/**
+ * pci_pwrctl_init() - Initialize the PCI power control context struct
+ *
+ * @pwrctl: PCI power control data
+ * @dev: Parent device
+ */
+void pci_pwrctl_init(struct pci_pwrctl *pwrctl, struct device *dev)
+{
+	pwrctl->dev = dev;
+	INIT_WORK(&pwrctl->work, rescan_work_func);
+}
+EXPORT_SYMBOL_GPL(pci_pwrctl_init);
+
 /**
  * pci_pwrctl_device_set_ready() - Notify the pwrctl subsystem that the PCI
  * device is powered-up and ready to be detected.
@@ -74,9 +96,7 @@ int pci_pwrctl_device_set_ready(struct pci_pwrctl *pwrctl)
 	if (ret)
 		return ret;
 
-	pci_lock_rescan_remove();
-	pci_rescan_bus(to_pci_dev(pwrctl->dev->parent)->bus);
-	pci_unlock_rescan_remove();
+	schedule_work(&pwrctl->work);
 
 	return 0;
 }
diff --git a/drivers/pci/pwrctl/pci-pwrctl-pwrseq.c b/drivers/pci/pwrctl/pci-pwrctl-pwrseq.c
index c7a113a76c0c..f07758c9edad 100644
--- a/drivers/pci/pwrctl/pci-pwrctl-pwrseq.c
+++ b/drivers/pci/pwrctl/pci-pwrctl-pwrseq.c
@@ -50,7 +50,7 @@ static int pci_pwrctl_pwrseq_probe(struct platform_device *pdev)
 	if (ret)
 		return ret;
 
-	data->ctx.dev = dev;
+	pci_pwrctl_init(&data->ctx, dev);
 
 	ret = devm_pci_pwrctl_device_set_ready(dev, &data->ctx);
 	if (ret)
diff --git a/include/linux/pci-pwrctl.h b/include/linux/pci-pwrctl.h
index 45e9cfe740e4..0d23dddf59ec 100644
--- a/include/linux/pci-pwrctl.h
+++ b/include/linux/pci-pwrctl.h
@@ -7,6 +7,7 @@
 #define __PCI_PWRCTL_H__
 
 #include <linux/notifier.h>
+#include <linux/workqueue.h>
 
 struct device;
 struct device_link;
@@ -41,8 +42,10 @@ struct pci_pwrctl {
 	/* Private: don't use. */
 	struct notifier_block nb;
 	struct device_link *link;
+	struct work_struct work;
 };
 
+void pci_pwrctl_init(struct pci_pwrctl *pwrctl, struct device *dev);
 int pci_pwrctl_device_set_ready(struct pci_pwrctl *pwrctl);
 void pci_pwrctl_device_unset_ready(struct pci_pwrctl *pwrctl);
 int devm_pci_pwrctl_device_set_ready(struct device *dev,
-- 
2.43.0

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

[Bjorn Helgaas]

[+to Rob]

On Fri, Aug 23, 2024 at 11:33:22AM +0200, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> 
> 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 <bartosz.golaszewski@linaro.org>
> ---
>  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
> 

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

[Bartosz Golaszewski]

On Sat, Aug 24, 2024 at 12:10 AM Bjorn Helgaas <helgaas@kernel.org> wrote:
>
> [+to Rob]
>
> On Fri, Aug 23, 2024 at 11:33:22AM +0200, Bartosz Golaszewski wrote:
> > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> >
> > 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

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

[Manivannan Sadhasivam]

+ Rob

On Fri, Aug 23, 2024 at 11:33:22AM +0200, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> 
> 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 <bartosz.golaszewski@linaro.org>
> ---
>  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
> 

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

Re: [PATCH v3 2/2] PCI/pwrctl: put the bus rescan on a different thread

[Manivannan Sadhasivam]

On Fri, Aug 23, 2024 at 11:33:23AM +0200, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> 
> If we trigger the bus rescan from sysfs, we'll try to lock the PCI

I think the first 'we' is user and second 'we' is PCI and pwrctl drivers. If so,
it should be spelled out to make it clear.

> rescan mutex recursively and deadlock - the platform device will be
> populated and probed on the same thread that handles the sysfs write.
> 

A little bit rewording could help here:

'When a user triggers a rescan from sysfs, sysfs code acquires the
pci_rescan_remove_lock during the start of the rescan. Then if a platform
device is created, pwrctl driver may get probed to control the power to the
device and it will also try to acquire the same lock to do the rescan after
powering up the device. And this will cause a deadlock.'

> Add a workqueue to the pwrctl code on which we schedule the rescan for
> controlled PCI devices. While at it: add a new interface for
> initializing the pwrctl context where we'd now assign the parent device
> address and initialize the workqueue.
> 
> Fixes: 4565d2652a37 ("PCI/pwrctl: Add PCI power control core code")
> Reported-by: Konrad Dybcio <konradybcio@kernel.org>

Don't we need 'Closes' link these days? I hope this is reported in ML.

> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

With above changes,

Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

- Mani

> ---
>  drivers/pci/pwrctl/core.c              | 26 +++++++++++++++++++++++---
>  drivers/pci/pwrctl/pci-pwrctl-pwrseq.c |  2 +-
>  include/linux/pci-pwrctl.h             |  3 +++
>  3 files changed, 27 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/pci/pwrctl/core.c b/drivers/pci/pwrctl/core.c
> index feca26ad2f6a..01d913b60316 100644
> --- a/drivers/pci/pwrctl/core.c
> +++ b/drivers/pci/pwrctl/core.c
> @@ -48,6 +48,28 @@ static int pci_pwrctl_notify(struct notifier_block *nb, unsigned long action,
>  	return NOTIFY_DONE;
>  }
>  
> +static void rescan_work_func(struct work_struct *work)
> +{
> +	struct pci_pwrctl *pwrctl = container_of(work, struct pci_pwrctl, work);
> +
> +	pci_lock_rescan_remove();
> +	pci_rescan_bus(to_pci_dev(pwrctl->dev->parent)->bus);
> +	pci_unlock_rescan_remove();
> +}
> +
> +/**
> + * pci_pwrctl_init() - Initialize the PCI power control context struct
> + *
> + * @pwrctl: PCI power control data
> + * @dev: Parent device
> + */
> +void pci_pwrctl_init(struct pci_pwrctl *pwrctl, struct device *dev)
> +{
> +	pwrctl->dev = dev;
> +	INIT_WORK(&pwrctl->work, rescan_work_func);
> +}
> +EXPORT_SYMBOL_GPL(pci_pwrctl_init);
> +
>  /**
>   * pci_pwrctl_device_set_ready() - Notify the pwrctl subsystem that the PCI
>   * device is powered-up and ready to be detected.
> @@ -74,9 +96,7 @@ int pci_pwrctl_device_set_ready(struct pci_pwrctl *pwrctl)
>  	if (ret)
>  		return ret;
>  
> -	pci_lock_rescan_remove();
> -	pci_rescan_bus(to_pci_dev(pwrctl->dev->parent)->bus);
> -	pci_unlock_rescan_remove();
> +	schedule_work(&pwrctl->work);
>  
>  	return 0;
>  }
> diff --git a/drivers/pci/pwrctl/pci-pwrctl-pwrseq.c b/drivers/pci/pwrctl/pci-pwrctl-pwrseq.c
> index c7a113a76c0c..f07758c9edad 100644
> --- a/drivers/pci/pwrctl/pci-pwrctl-pwrseq.c
> +++ b/drivers/pci/pwrctl/pci-pwrctl-pwrseq.c
> @@ -50,7 +50,7 @@ static int pci_pwrctl_pwrseq_probe(struct platform_device *pdev)
>  	if (ret)
>  		return ret;
>  
> -	data->ctx.dev = dev;
> +	pci_pwrctl_init(&data->ctx, dev);
>  
>  	ret = devm_pci_pwrctl_device_set_ready(dev, &data->ctx);
>  	if (ret)
> diff --git a/include/linux/pci-pwrctl.h b/include/linux/pci-pwrctl.h
> index 45e9cfe740e4..0d23dddf59ec 100644
> --- a/include/linux/pci-pwrctl.h
> +++ b/include/linux/pci-pwrctl.h
> @@ -7,6 +7,7 @@
>  #define __PCI_PWRCTL_H__
>  
>  #include <linux/notifier.h>
> +#include <linux/workqueue.h>
>  
>  struct device;
>  struct device_link;
> @@ -41,8 +42,10 @@ struct pci_pwrctl {
>  	/* Private: don't use. */
>  	struct notifier_block nb;
>  	struct device_link *link;
> +	struct work_struct work;
>  };
>  
> +void pci_pwrctl_init(struct pci_pwrctl *pwrctl, struct device *dev);
>  int pci_pwrctl_device_set_ready(struct pci_pwrctl *pwrctl);
>  void pci_pwrctl_device_unset_ready(struct pci_pwrctl *pwrctl);
>  int devm_pci_pwrctl_device_set_ready(struct device *dev,
> -- 
> 2.43.0
> 

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

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

[Bartosz Golaszewski]

On Tue, Aug 27, 2024 at 10:40 AM Manivannan Sadhasivam
<manivannan.sadhasivam@linaro.org> wrote:
>
> + Rob
>
> On Fri, Aug 23, 2024 at 11:33:22AM +0200, Bartosz Golaszewski wrote:
> > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> >
> > 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

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

[Rob Herring]

On Fri, Aug 23, 2024 at 5:10 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
>
> [+to Rob]
>
> On Fri, Aug 23, 2024 at 11:33:22AM +0200, Bartosz Golaszewski wrote:
> > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> >
> > 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

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

[Manivannan Sadhasivam]

On Tue, Aug 27, 2024 at 02:25:58PM +0200, Bartosz Golaszewski wrote:
> On Tue, Aug 27, 2024 at 10:40 AM Manivannan Sadhasivam
> <manivannan.sadhasivam@linaro.org> wrote:
> >
> > + Rob
> >
> > On Fri, Aug 23, 2024 at 11:33:22AM +0200, Bartosz Golaszewski wrote:
> > > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> > >
> > > 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 <manivannan.sadhasivam@linaro.org>

- Mani

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

Re: [PATCH v3 2/2] PCI/pwrctl: put the bus rescan on a different thread

[Bartosz Golaszewski]

On Tue, Aug 27, 2024 at 10:56 AM Manivannan Sadhasivam
<manivannan.sadhasivam@linaro.org> wrote:
>
> On Fri, Aug 23, 2024 at 11:33:23AM +0200, Bartosz Golaszewski wrote:
> > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> >
> > If we trigger the bus rescan from sysfs, we'll try to lock the PCI
>
> I think the first 'we' is user and second 'we' is PCI and pwrctl drivers. If so,
> it should be spelled out to make it clear.
>
> > rescan mutex recursively and deadlock - the platform device will be
> > populated and probed on the same thread that handles the sysfs write.
> >
>
> A little bit rewording could help here:
>
> 'When a user triggers a rescan from sysfs, sysfs code acquires the
> pci_rescan_remove_lock during the start of the rescan. Then if a platform
> device is created, pwrctl driver may get probed to control the power to the
> device and it will also try to acquire the same lock to do the rescan after
> powering up the device. And this will cause a deadlock.'
>
> > Add a workqueue to the pwrctl code on which we schedule the rescan for
> > controlled PCI devices. While at it: add a new interface for
> > initializing the pwrctl context where we'd now assign the parent device
> > address and initialize the workqueue.
> >
> > Fixes: 4565d2652a37 ("PCI/pwrctl: Add PCI power control core code")
> > Reported-by: Konrad Dybcio <konradybcio@kernel.org>
>
> Don't we need 'Closes' link these days? I hope this is reported in ML.
>

Nope, unfortunately on IRC. But the tag is unformally optional it seems.

Bart

> > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>
> With above changes,
>
> Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

Re: [PATCH v3 0/2] PCI/pwrctl: fixes for v6.11

[Bartosz Golaszewski]

On Fri, Aug 23, 2024 at 11:33 AM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
>
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>
> Bjorn,
>
> Here's a respin of the PCI/pwrctl fixes that should go into v6.11. I
> eventually found a solution that doesn't require Krishna's topology
> change but Krishna: please make sure to update the code in
> drivers/pci/remove.c as well when rebasing your work once this gets
> upstream.
>
> v2 -> v3:
> - use the correct device for unregistering the platform pwrctl device in
>   patch 1/2
> - make patch 1/2 easier to read by using device_for_each_child()
>
> v1 -> v2:
> - use the scoped variant of for_each_child_of_node() to fix a memory
>   leak in patch 1/2
>
> Bartosz Golaszewski (2):
>   PCI: don't rely on of_platform_depopulate() for reused OF-nodes
>   PCI/pwrctl: put the bus rescan on a different thread
>
>  drivers/pci/pwrctl/core.c              | 26 +++++++++++++++++++++++---
>  drivers/pci/pwrctl/pci-pwrctl-pwrseq.c |  2 +-
>  drivers/pci/remove.c                   | 18 +++++++++++++++++-
>  include/linux/pci-pwrctl.h             |  3 +++
>  4 files changed, 44 insertions(+), 5 deletions(-)
>
> --
> 2.43.0
>

Bjorn: gentle ping. Neither Rob nor Mani are opposed to these and it's
getting close to release. Could you please pick them up?

Bart

Re: [PATCH v3 0/2] PCI/pwrctl: fixes for v6.11

[Bjorn Helgaas]

On Fri, Aug 23, 2024 at 11:33:21AM +0200, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> 
> Bjorn,
> 
> Here's a respin of the PCI/pwrctl fixes that should go into v6.11. I
> eventually found a solution that doesn't require Krishna's topology
> change but Krishna: please make sure to update the code in
> drivers/pci/remove.c as well when rebasing your work once this gets
> upstream.
> 
> v2 -> v3:
> - use the correct device for unregistering the platform pwrctl device in
>   patch 1/2
> - make patch 1/2 easier to read by using device_for_each_child()
> 
> v1 -> v2:
> - use the scoped variant of for_each_child_of_node() to fix a memory
>   leak in patch 1/2
> 
> Bartosz Golaszewski (2):
>   PCI: don't rely on of_platform_depopulate() for reused OF-nodes
>   PCI/pwrctl: put the bus rescan on a different thread
> 
>  drivers/pci/pwrctl/core.c              | 26 +++++++++++++++++++++++---
>  drivers/pci/pwrctl/pci-pwrctl-pwrseq.c |  2 +-
>  drivers/pci/remove.c                   | 18 +++++++++++++++++-
>  include/linux/pci-pwrctl.h             |  3 +++
>  4 files changed, 44 insertions(+), 5 deletions(-)

Applied to pci/for-linus for v6.11, thanks.