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

Commit Message

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

From: Bartosz Golaszewski <[e-mail]>

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 <[e-mail]>
Signed-off-by: Bartosz Golaszewski <[e-mail]>
---
 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(-)

Comments

Manivannan Sadhasivam Aug. 27, 2024, 8:56 a.m. UTC | #1

On Fri, Aug 23, 2024 at 11:33:23AM +0200, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <[e-mail]>
> 
> 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 <[e-mail]>

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

> Signed-off-by: Bartosz Golaszewski <[e-mail]>

With above changes,

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

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

Bartosz Golaszewski Sept. 2, 2024, 7:38 a.m. UTC | #2

On Tue, Aug 27, 2024 at 10:56 AM Manivannan Sadhasivam
<[e-mail]> wrote:
>
> On Fri, Aug 23, 2024 at 11:33:23AM +0200, Bartosz Golaszewski wrote:
> > From: Bartosz Golaszewski <[e-mail]>
> >
> > 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 <[e-mail]>
>
> 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 <[e-mail]>
>
> With above changes,
>
> Reviewed-by: Manivannan Sadhasivam <[e-mail]>

Patch

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,