PCI/pwrctl: Ensure that pwrctl drivers are probed before PCI client drivers

As per the kernel device driver model, a pwrctl device is the supplier for
the PCI device, but the device link that enforces the supplier-consumer
relationship was previously created by the pwrctl driver. Therefore, the
driver model didn't prevent probing 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 a9aaf1ff88a8
("power: sequencing: request the WLAN enable GPIO as-is").

A cleaner way to fix the above mentioned race condition is to ensure that
the pwrctl drivers are always probed before the client drivers.

If the PCI device is associated with a pwrctl platform device with a power
supply, add a device link between the PCI device and the pwrctl 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 of by the driver core when the PCI device gets removed.

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")
Link: https://lore.kernel.org/r/20241025-pci-pwrctl-rework-v2-3-568756156cbe@linaro.org
Tested-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
Tested-by: Krishna chaitanya chundru <quic_krichai@quicinc.com>
Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
[bhelgaas: squash fix from
https://lore.kernel.org/r/20241120062459.6371-1-manivannan.sadhasivam@linaro.org
for SPARCv9 issue reported by Jonathan Currier <dullfire@yahoo.com>]
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
[kwilczynski: wrap code to 80 columns]
Signed-off-by: Krzysztof Wilczyński <kwilczynski@kernel.org>
Reviewed-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
Cc: stable+noautosel@kernel.org		# Depends on power supply check

diff --git a/drivers/pci/bus.c b/drivers/pci/bus.c
index 93bc421..90d7756 100644 (file)
--- a/drivers/pci/bus.c
+++ b/drivers/pci/bus.c
@@ -321,6 +321,46 @@ void __weak pcibios_resource_survey_bus(struct pci_bus *bus) { }
 
 void __weak pcibios_bus_add_device(struct pci_dev *pdev) { }
 
+/*
+ * Create pwrctl devices (if required) for the PCI devices to handle the power
+ * state.
+ */
+static void pci_pwrctl_create_devices(struct pci_dev *dev)
+{
+       struct device_node *np = dev_of_node(&dev->dev);
+       struct device *parent = &dev->dev;
+       struct platform_device *pdev;
+
+       /*
+        * First ensure that we are starting from a PCI bridge and it has a
+        * corresponding devicetree node.
+        */
+       if (np && pci_is_bridge(dev)) {
+               /*
+                * Now look for the child PCI device nodes and create pwrctl
+                * devices for them. The pwrctl device drivers will manage the
+                * power state of the devices.
+                */
+               for_each_available_child_of_node_scoped(np, child) {
+                       /*
+                        * First check whether the pwrctl device really needs to
+                        * be created or not. This is decided based on at least
+                        * one of the power supplies being defined in the
+                        * devicetree node of the device.
+                        */
+                       if (!of_pci_supply_present(child)) {
+                               pci_dbg(dev, "skipping OF node: %s\n", child->name);
+                               return;
+                       }
+
+                       /* Now create the pwrctl device */
+                       pdev = of_platform_device_create(child, NULL, parent);
+                       if (!pdev)
+                               pci_err(dev, "failed to create OF node: %s\n", child->name);
+               }
+       }
+}
+
 /**
  * pci_bus_add_device - start driver for a single device
  * @dev: device to add
@@ -345,31 +385,28 @@ void pci_bus_add_device(struct pci_dev *dev)
        pci_proc_attach_device(dev);
        pci_bridge_d3_update(dev);
 
+       pci_pwrctl_create_devices(dev);
+
+       /*
+        * If the PCI device is associated with a pwrctl device with a
+        * power supply, create a device link between the PCI device and
+        * pwrctl device.  This ensures that pwrctl drivers are probed
+        * before PCI client drivers.
+        */
+       pdev = of_find_device_by_node(dn);
+       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 to power control device %s\n",
+                               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);
-
-       if (dev_of_node(&dev->dev) && pci_is_bridge(dev)) {
-               for_each_available_child_of_node_scoped(dn, child) {
-                       /*
-                        * First check whether the pwrctl device needs to be
-                        * created or not. This is decided based on at least
-                        * one of the power supplies being defined in the
-                        * devicetree node of the device.
-                        */
-                       if (!of_pci_supply_present(child)) {
-                               pci_dbg(dev, "skipping OF node: %s\n", child->name);
-                               continue;
-                       }
-
-                       pdev = of_platform_device_create(child, NULL, &dev->dev);
-                       if (!pdev)
-                               pci_err(dev, "failed to create OF node: %s\n", child->name);
-               }
-       }
 }
 EXPORT_SYMBOL_GPL(pci_bus_add_device);
 

diff --git a/drivers/pci/pwrctl/core.c b/drivers/pci/pwrctl/core.c
index 01d913b..bb5a237 100644 (file)
--- 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;