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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20240827085616.v3xzrgyojxd746bv@thinkpad>
Date: Tue, 27 Aug 2024 14:26:16 +0530
From: Manivannan Sadhasivam <manivannan.sadhasivam@...aro.org>
To: Bartosz Golaszewski <brgl@...ev.pl>
Cc: Bjorn Helgaas <bhelgaas@...gle.com>,
	Krishna chaitanya chundru <quic_krichai@...cinc.com>,
	linux-pci@...r.kernel.org, linux-kernel@...r.kernel.org,
	Bartosz Golaszewski <bartosz.golaszewski@...aro.org>,
	Konrad Dybcio <konradybcio@...nel.org>
Subject: Re: [PATCH v3 2/2] PCI/pwrctl: put the bus rescan on a different
 thread

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

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

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

With above changes,

Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@...aro.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
> 

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