| Message ID | 20241022-pci-pwrctl-rework-v1-3-94a7e90f58c5@linaro.org (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
| Series | PCI/pwrctl: Ensure that the pwrctl drivers are probed before PCI client drivers | expand |
On Tue, 22 Oct 2024 at 12:28, Manivannan Sadhasivam via B4 Relay <devnull+manivannan.sadhasivam.linaro.org@kernel.org> 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). > > 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") > 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(-) > > diff --git a/drivers/pci/bus.c b/drivers/pci/bus.c > index 698ec98b9388..351af581904f 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_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; > > -- > 2.25.1 > > Reviewed-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
diff --git a/drivers/pci/bus.c b/drivers/pci/bus.c index 698ec98b9388..351af581904f 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_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;