diff mbox series

[v2,3/5] PCI/pwrctl: Ensure that the pwrctl drivers are probed before the PCI client drivers

Message ID 20241025-pci-pwrctl-rework-v2-3-568756156cbe@linaro.org (mailing list archive)
State Accepted
Delegated to: Krzysztof Wilczyński
Headers show
Series PCI/pwrctl: Ensure that the pwrctl drivers are probed before PCI client drivers | expand

Commit Message

Manivannan Sadhasivam via B4 Relay Oct. 25, 2024, 7:54 a.m. UTC 
From: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

As per the kernel device driver model, pwrctl device is the supplier for
the PCI device. But the device link that enforces the supplier-consumer
relationship is created inside the pwrctl driver currently. Due to this,
the driver model doesn't prevent probing of the PCI client drivers before
probing the corresponding pwrctl drivers. This may lead to a race condition
if the PCI device was already powered on by the bootloader (before the
pwrctl driver).

If the bootloader did not power on the PCI device, this wouldn't create any
problem as the pwrctl driver will be the one powering on the device, so the
PCI client driver always gets probed afterward. But if the device was
already powered on, then the device will be seen by the PCI core and the
PCI client driver may get probed before its pwrctl driver. This creates a
race condition as the pwrctl driver may change the device power state while
the device is being accessed by the client driver.

One such issue was already reported on the Qcom X13s platform with the
WLAN device and fixed with a hack in the WCN pwrseq driver by the 'commit
a9aaf1ff88a8 ("power: sequencing: request the WLAN enable GPIO as-is")'.

But a cleaner way to fix the above mentioned race condition would be to
ensure that the pwrctl drivers are always probed before the client drivers.
This is achieved by creating the device link between pwrctl device and PCI
device before device_attach() in pci_bus_add_device().

Note that there is no need to explicitly remove the device link as that
will be taken care by the driver core when the PCI device gets removed.

Cc: stable+noautosel@kernel.org # Depends on power supply check
Fixes: 4565d2652a37 ("PCI/pwrctl: Add PCI power control core code")
Fixes: 8fb18619d910 ("PCI/pwrctl: Create platform devices for child OF nodes of the port node")
Reviewed-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
Tested-by: Krishna chaitanya chundru <quic_krichai@quicinc.com>
Tested-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---
 drivers/pci/bus.c         | 26 +++++++++++++++++++-------
 drivers/pci/pwrctl/core.c | 10 ----------
 2 files changed, 19 insertions(+), 17 deletions(-)

Comments

Bjorn Helgaas Nov. 20, 2024, 4:10 p.m. UTC | #1 
On Fri, Oct 25, 2024 at 01:24:53PM +0530, Manivannan Sadhasivam via B4 Relay wrote:
> From: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> 
> As per the kernel device driver model, pwrctl device is the supplier for
> the PCI device. But the device link that enforces the supplier-consumer
> relationship is created inside the pwrctl driver currently. Due to this,
> the driver model doesn't prevent probing of the PCI client drivers before
> probing the corresponding pwrctl drivers. This may lead to a race condition
> if the PCI device was already powered on by the bootloader (before the
> pwrctl driver).

> +	 * Create a device link between the PCI device and pwrctl device (if
> +	 * exists). This ensures that the pwrctl drivers are probed before the
> +	 * PCI client drivers.
> +	 */
> +	pdev = of_find_device_by_node(dn);
> +	if (pdev) {
> +		if (!device_link_add(&dev->dev, &pdev->dev, DL_FLAG_AUTOREMOVE_CONSUMER))
> +			pci_err(dev, "failed to add device link between %s and %s\n",
> +				dev_name(&dev->dev), pdev->name);

This prints the name for "dev" twice (once by pci_err(dev) and again
from dev_name(&dev->dev)).  Is it helpful to see it twice here?
Manivannan Sadhasivam Nov. 20, 2024, 5:02 p.m. UTC | #2 
On Wed, Nov 20, 2024 at 10:10:47AM -0600, Bjorn Helgaas wrote:
> On Fri, Oct 25, 2024 at 01:24:53PM +0530, Manivannan Sadhasivam via B4 Relay wrote:
> > From: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > 
> > As per the kernel device driver model, pwrctl device is the supplier for
> > the PCI device. But the device link that enforces the supplier-consumer
> > relationship is created inside the pwrctl driver currently. Due to this,
> > the driver model doesn't prevent probing of the PCI client drivers before
> > probing the corresponding pwrctl drivers. This may lead to a race condition
> > if the PCI device was already powered on by the bootloader (before the
> > pwrctl driver).
> 
> > +	 * Create a device link between the PCI device and pwrctl device (if
> > +	 * exists). This ensures that the pwrctl drivers are probed before the
> > +	 * PCI client drivers.
> > +	 */
> > +	pdev = of_find_device_by_node(dn);
> > +	if (pdev) {
> > +		if (!device_link_add(&dev->dev, &pdev->dev, DL_FLAG_AUTOREMOVE_CONSUMER))
> > +			pci_err(dev, "failed to add device link between %s and %s\n",
> > +				dev_name(&dev->dev), pdev->name);
> 
> This prints the name for "dev" twice (once by pci_err(dev) and again
> from dev_name(&dev->dev)).  Is it helpful to see it twice here?

Hmm, not very much. It could be reworded as below:

	pci_err(dev, "failed to link: %s\n", pdev->name);

- Mani
Bjorn Helgaas Nov. 20, 2024, 8:18 p.m. UTC | #3 
On Wed, Nov 20, 2024 at 10:32:32PM +0530, Manivannan Sadhasivam wrote:
> On Wed, Nov 20, 2024 at 10:10:47AM -0600, Bjorn Helgaas wrote:
> > On Fri, Oct 25, 2024 at 01:24:53PM +0530, Manivannan Sadhasivam via B4 Relay wrote:
> > > From: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > > 
> > > As per the kernel device driver model, pwrctl device is the supplier for
> > > the PCI device. But the device link that enforces the supplier-consumer
> > > relationship is created inside the pwrctl driver currently. Due to this,
> > > the driver model doesn't prevent probing of the PCI client drivers before
> > > probing the corresponding pwrctl drivers. This may lead to a race condition
> > > if the PCI device was already powered on by the bootloader (before the
> > > pwrctl driver).
> > 
> > > +	 * Create a device link between the PCI device and pwrctl device (if
> > > +	 * exists). This ensures that the pwrctl drivers are probed before the
> > > +	 * PCI client drivers.
> > > +	 */
> > > +	pdev = of_find_device_by_node(dn);
> > > +	if (pdev) {
> > > +		if (!device_link_add(&dev->dev, &pdev->dev, DL_FLAG_AUTOREMOVE_CONSUMER))
> > > +			pci_err(dev, "failed to add device link between %s and %s\n",
> > > +				dev_name(&dev->dev), pdev->name);
> > 
> > This prints the name for "dev" twice (once by pci_err(dev) and again
> > from dev_name(&dev->dev)).  Is it helpful to see it twice here?
> 
> Hmm, not very much. It could be reworded as below:
> 
> 	pci_err(dev, "failed to link: %s\n", pdev->name);

OK.  I updated the comment and the message like this (also renamed
of_pci_is_supply_present() to of_pci_supply_present() so it reads more
naturally in "if" statements):

-	 * Create a device link between the PCI device and pwrctrl device (if
-	 * exists). This ensures that the pwrctrl drivers are probed before the
-	 * PCI client drivers.
+	 * If the PCI device is associated with a pwrctrl device with a
+	 * power supply, create a device link between the PCI device and
+	 * pwrctrl device.  This ensures that pwrctrl drivers are probed
+	 * before PCI client drivers.
 	 */
 	pdev = of_find_device_by_node(dn);
-	if (pdev) {
+	if (pdev && of_pci_supply_present(dn)) {
 		if (!device_link_add(&dev->dev, &pdev->dev,
 				     DL_FLAG_AUTOREMOVE_CONSUMER))
-			pci_err(dev, "failed to add device link between %s and %s\n",
-				dev_name(&dev->dev), pdev->name);
+			pci_err(dev, "failed to add device link to power control device %s\n",
+				pdev->name);
 	}
Manivannan Sadhasivam Nov. 21, 2024, noon UTC | #4 
On Wed, Nov 20, 2024 at 02:18:39PM -0600, Bjorn Helgaas wrote:
> On Wed, Nov 20, 2024 at 10:32:32PM +0530, Manivannan Sadhasivam wrote:
> > On Wed, Nov 20, 2024 at 10:10:47AM -0600, Bjorn Helgaas wrote:
> > > On Fri, Oct 25, 2024 at 01:24:53PM +0530, Manivannan Sadhasivam via B4 Relay wrote:
> > > > From: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > > > 
> > > > As per the kernel device driver model, pwrctl device is the supplier for
> > > > the PCI device. But the device link that enforces the supplier-consumer
> > > > relationship is created inside the pwrctl driver currently. Due to this,
> > > > the driver model doesn't prevent probing of the PCI client drivers before
> > > > probing the corresponding pwrctl drivers. This may lead to a race condition
> > > > if the PCI device was already powered on by the bootloader (before the
> > > > pwrctl driver).
> > > 
> > > > +	 * Create a device link between the PCI device and pwrctl device (if
> > > > +	 * exists). This ensures that the pwrctl drivers are probed before the
> > > > +	 * PCI client drivers.
> > > > +	 */
> > > > +	pdev = of_find_device_by_node(dn);
> > > > +	if (pdev) {
> > > > +		if (!device_link_add(&dev->dev, &pdev->dev, DL_FLAG_AUTOREMOVE_CONSUMER))
> > > > +			pci_err(dev, "failed to add device link between %s and %s\n",
> > > > +				dev_name(&dev->dev), pdev->name);
> > > 
> > > This prints the name for "dev" twice (once by pci_err(dev) and again
> > > from dev_name(&dev->dev)).  Is it helpful to see it twice here?
> > 
> > Hmm, not very much. It could be reworded as below:
> > 
> > 	pci_err(dev, "failed to link: %s\n", pdev->name);
> 
> OK.  I updated the comment and the message like this (also renamed
> of_pci_is_supply_present() to of_pci_supply_present() so it reads more
> naturally in "if" statements):
> 
> -	 * Create a device link between the PCI device and pwrctrl device (if
> -	 * exists). This ensures that the pwrctrl drivers are probed before the
> -	 * PCI client drivers.
> +	 * If the PCI device is associated with a pwrctrl device with a
> +	 * power supply, create a device link between the PCI device and
> +	 * pwrctrl device.  This ensures that pwrctrl drivers are probed
> +	 * before PCI client drivers.
>  	 */
>  	pdev = of_find_device_by_node(dn);
> -	if (pdev) {
> +	if (pdev && of_pci_supply_present(dn)) {
>  		if (!device_link_add(&dev->dev, &pdev->dev,
>  				     DL_FLAG_AUTOREMOVE_CONSUMER))
> -			pci_err(dev, "failed to add device link between %s and %s\n",
> -				dev_name(&dev->dev), pdev->name);
> +			pci_err(dev, "failed to add device link to power control device %s\n",

Maybe use 'pwrctrl' instead of 'power control'?

- Mani
Krzysztof Wilczyński Nov. 21, 2024, 6:28 p.m. UTC | #5 
Hello,

[...]
> > -			pci_err(dev, "failed to add device link between %s and %s\n",
> > -				dev_name(&dev->dev), pdev->name);
> > +			pci_err(dev, "failed to add device link to power control device %s\n",
> 
> Maybe use 'pwrctrl' instead of 'power control'?

Changed, thank you!  This makes the verbiage consistent with other
messages, code comments, etc.

	Krzysztof
Krzysztof Wilczyński Nov. 21, 2024, 8:14 p.m. UTC | #6 
On 24-11-22 03:28:24, Krzysztof Wilczyński wrote:
> Hello,
> 
> [...]
> > > -			pci_err(dev, "failed to add device link between %s and %s\n",
> > > -				dev_name(&dev->dev), pdev->name);
> > > +			pci_err(dev, "failed to add device link to power control device %s\n",
> > 
> > Maybe use 'pwrctrl' instead of 'power control'?
> 
> Changed, thank you!  This makes the verbiage consistent with other
> messages, code comments, etc.

... this one might have slip into the after merge window.

Also, Bjorn and I, we had a conversation about the correct wording here in
user-facing messages.

The "power control" vs "pwrctrl", etc.  There is a single existing
precedent within the code base for the "pwrctrl" use per:

  55:	ret = devm_pci_pwrctrl_device_set_ready(dev, &data->ctx);
  56-	if (ret)
  57-		return dev_err_probe(dev, ret,
  58:				     "Failed to register the pwrctrl wrapper\n");

So, we should be consistent within the documentation and anything user-facing
around what wording we use, I believe.  Whichever it will be in the end.

	Krzysztof
diff mbox series

Patch

diff --git a/drivers/pci/bus.c b/drivers/pci/bus.c
index 02a492aa5f17..645bbb75f8f9 100644
--- a/drivers/pci/bus.c
+++ b/drivers/pci/bus.c
@@ -345,13 +345,6 @@  void pci_bus_add_device(struct pci_dev *dev)
 	pci_proc_attach_device(dev);
 	pci_bridge_d3_update(dev);
 
-	dev->match_driver = !dn || of_device_is_available(dn);
-	retval = device_attach(&dev->dev);
-	if (retval < 0 && retval != -EPROBE_DEFER)
-		pci_warn(dev, "device attach failed (%d)\n", retval);
-
-	pci_dev_assign_added(dev, true);
-
 	if (dev_of_node(&dev->dev) && pci_is_bridge(dev)) {
 		for_each_available_child_of_node_scoped(dn, child) {
 			/*
@@ -370,6 +363,25 @@  void pci_bus_add_device(struct pci_dev *dev)
 				pci_err(dev, "failed to create OF node: %s\n", child->name);
 		}
 	}
+
+	/*
+	 * Create a device link between the PCI device and pwrctl device (if
+	 * exists). This ensures that the pwrctl drivers are probed before the
+	 * PCI client drivers.
+	 */
+	pdev = of_find_device_by_node(dn);
+	if (pdev) {
+		if (!device_link_add(&dev->dev, &pdev->dev, DL_FLAG_AUTOREMOVE_CONSUMER))
+			pci_err(dev, "failed to add device link between %s and %s\n",
+				dev_name(&dev->dev), pdev->name);
+	}
+
+	dev->match_driver = !dn || of_device_is_available(dn);
+	retval = device_attach(&dev->dev);
+	if (retval < 0 && retval != -EPROBE_DEFER)
+		pci_warn(dev, "device attach failed (%d)\n", retval);
+
+	pci_dev_assign_added(dev, true);
 }
 EXPORT_SYMBOL_GPL(pci_bus_add_device);
 
diff --git a/drivers/pci/pwrctl/core.c b/drivers/pci/pwrctl/core.c
index 01d913b60316..bb5a23712434 100644
--- a/drivers/pci/pwrctl/core.c
+++ b/drivers/pci/pwrctl/core.c
@@ -33,16 +33,6 @@  static int pci_pwrctl_notify(struct notifier_block *nb, unsigned long action,
 		 */
 		dev->of_node_reused = true;
 		break;
-	case BUS_NOTIFY_BOUND_DRIVER:
-		pwrctl->link = device_link_add(dev, pwrctl->dev,
-					       DL_FLAG_AUTOREMOVE_CONSUMER);
-		if (!pwrctl->link)
-			dev_err(pwrctl->dev, "Failed to add device link\n");
-		break;
-	case BUS_NOTIFY_UNBOUND_DRIVER:
-		if (pwrctl->link)
-			device_link_remove(dev, pwrctl->dev);
-		break;
 	}
 
 	return NOTIFY_DONE;