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

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

[Manivannan Sadhasivam via B4 Relay]

Hi,

This series reworks the PCI/pwrctl integration to ensure that the pwrctl drivers
are always probed before the PCI client drivers. This series addresses a race
condition when both pwrctl and pwrctl/pwrseq drivers probe parallely (even when
the later one probes last). One such issue was reported for the Qcom X13s
platform with WLAN module and fixed with 'commit a9aaf1ff88a8 ("power:
sequencing: request the WLAN enable GPIO as-is")'.

Though the issue was fixed with a hack in the pwrseq driver, it was clear that
the issue is applicable to all pwrctl drivers. Hence, this series tries to
address the issue in the PCI/pwrctl integration.

- Mani

Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---
Changes in v2:
- Used for_each_available_child_of_node_scoped() to iterate over child nodes
  (Bartosz)
- Collected tags
- Link to v1: https://lore.kernel.org/r/20241022-pci-pwrctl-rework-v1-0-94a7e90f58c5@linaro.org

---
Manivannan Sadhasivam (5):
      PCI/pwrctl: Use of_platform_device_create() to create pwrctl devices
      PCI/pwrctl: Create pwrctl devices only if at least one power supply is present
      PCI/pwrctl: Ensure that the pwrctl drivers are probed before the PCI client drivers
      PCI/pwrctl: Move pwrctl device creation to its own helper function
      PCI/pwrctl: Remove pwrctl device without iterating over all children of pwrctl parent

 drivers/pci/bus.c         | 64 +++++++++++++++++++++++++++++++++++++++++------
 drivers/pci/of.c          | 27 ++++++++++++++++++++
 drivers/pci/pci.h         |  5 ++++
 drivers/pci/pwrctl/core.c | 10 --------
 drivers/pci/remove.c      | 17 ++++++-------
 5 files changed, 96 insertions(+), 27 deletions(-)
---
base-commit: 48dc7986beb60522eb217c0016f999cc7afaf0b7
change-id: 20241022-pci-pwrctl-rework-a1b024158555

Best regards,
-- 
Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

[PATCH v2 1/5] PCI/pwrctl: Use of_platform_device_create() to create pwrctl devices

[Manivannan Sadhasivam via B4 Relay]

From: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

of_platform_populate() API creates platform devices by descending through
the children of the parent node. But it provides no control over the child
nodes, which makes it difficult to add checks for the child nodes in the
future. So use of_platform_device_create() API together with
for_each_child_of_node_scoped() so that it is possible to add checks for
each node before creating the platform device.

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 | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/drivers/pci/bus.c b/drivers/pci/bus.c
index 55c853686051..9d278d3a19ff 100644
--- a/drivers/pci/bus.c
+++ b/drivers/pci/bus.c
@@ -13,6 +13,7 @@
 #include <linux/ioport.h>
 #include <linux/of.h>
 #include <linux/of_platform.h>
+#include <linux/platform_device.h>
 #include <linux/proc_fs.h>
 #include <linux/slab.h>
 
@@ -329,6 +330,7 @@ void __weak pcibios_bus_add_device(struct pci_dev *pdev) { }
 void pci_bus_add_device(struct pci_dev *dev)
 {
 	struct device_node *dn = dev->dev.of_node;
+	struct platform_device *pdev;
 	int retval;
 
 	/*
@@ -351,11 +353,11 @@ void pci_bus_add_device(struct pci_dev *dev)
 	pci_dev_assign_added(dev, true);
 
 	if (dev_of_node(&dev->dev) && pci_is_bridge(dev)) {
-		retval = of_platform_populate(dev_of_node(&dev->dev), NULL, NULL,
-					      &dev->dev);
-		if (retval)
-			pci_err(dev, "failed to populate child OF nodes (%d)\n",
-				retval);
+		for_each_available_child_of_node_scoped(dn, child) {
+			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);

-- 
2.25.1

[PATCH v2 2/5] PCI/pwrctl: Create pwrctl devices only if at least one power supply is present

[Manivannan Sadhasivam via B4 Relay]

From: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

Currently, pwrctl devices are created if the corresponding PCI nodes are
defined in devicetree. But this is not correct, because not all PCI nodes
defined in devicetree require pwrctl support. Pwrctl comes into picture
only when the device requires kernel to manage its power state. This can
be determined using the power supply properties present in the devicetree
node of the device.

So add a new API, of_pci_is_supply_present() that checks the devicetree
node if at least one power supply property is present or not. If present,
then the pwrctl device will be created for that PCI node. Otherwise, it
will be skipped.

Cc: stable+noautosel@kernel.org # Depends on of_platform_device_create() rework
Fixes: 8fb18619d910 ("PCI/pwrctl: Create platform devices for child OF nodes of the port node")
Suggested-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
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 | 11 +++++++++++
 drivers/pci/of.c  | 27 +++++++++++++++++++++++++++
 drivers/pci/pci.h |  5 +++++
 3 files changed, 43 insertions(+)

diff --git a/drivers/pci/bus.c b/drivers/pci/bus.c
index 9d278d3a19ff..02a492aa5f17 100644
--- a/drivers/pci/bus.c
+++ b/drivers/pci/bus.c
@@ -354,6 +354,17 @@ void pci_bus_add_device(struct pci_dev *dev)
 
 	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_is_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);
diff --git a/drivers/pci/of.c b/drivers/pci/of.c
index dacea3fc5128..1f6a15a35a82 100644
--- a/drivers/pci/of.c
+++ b/drivers/pci/of.c
@@ -728,6 +728,33 @@ void of_pci_make_dev_node(struct pci_dev *pdev)
 }
 #endif
 
+/**
+ * of_pci_is_supply_present() - Check if the power supply is present for the PCI
+ *				device
+ * @np: Device tree node
+ *
+ * Check if the power supply for the PCI device is present in the device tree
+ * node or not.
+ *
+ * Return: true if at least one power supply exists; false otherwise.
+ */
+bool of_pci_is_supply_present(struct device_node *np)
+{
+	struct property *prop;
+	char *supply;
+
+	if (!np)
+		return false;
+
+	for_each_property_of_node(np, prop) {
+		supply = strrchr(prop->name, '-');
+		if (supply && !strcmp(supply, "-supply"))
+			return true;
+	}
+
+	return false;
+}
+
 #endif /* CONFIG_PCI */
 
 /**
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index 14d00ce45bfa..c8081b427267 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -746,6 +746,7 @@ void pci_set_bus_of_node(struct pci_bus *bus);
 void pci_release_bus_of_node(struct pci_bus *bus);
 
 int devm_of_pci_bridge_init(struct device *dev, struct pci_host_bridge *bridge);
+bool of_pci_is_supply_present(struct device_node *np);
 
 #else
 static inline int
@@ -793,6 +794,10 @@ static inline int devm_of_pci_bridge_init(struct device *dev, struct pci_host_br
 	return 0;
 }
 
+static inline bool of_pci_is_supply_present(struct device_node *np);
+{
+	return false;
+}
 #endif /* CONFIG_OF */
 
 struct of_changeset;

-- 
2.25.1

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

[Manivannan Sadhasivam via B4 Relay]

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

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;

-- 
2.25.1

[PATCH v2 4/5] PCI/pwrctl: Move pwrctl device creation to its own helper function

[Manivannan Sadhasivam via B4 Relay]

From: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

This makes the pci_bus_add_device() API easier to maintain. Also add more
comments to the helper to describe how the devices are created.

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 | 59 ++++++++++++++++++++++++++++++++++++++-----------------
 1 file changed, 41 insertions(+), 18 deletions(-)

diff --git a/drivers/pci/bus.c b/drivers/pci/bus.c
index 645bbb75f8f9..6e38984e576d 100644
--- 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_is_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,24 +385,7 @@ void pci_bus_add_device(struct pci_dev *dev)
 	pci_proc_attach_device(dev);
 	pci_bridge_d3_update(dev);
 
-	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_is_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);
-		}
-	}
+	pci_pwrctl_create_devices(dev);
 
 	/*
 	 * Create a device link between the PCI device and pwrctl device (if

-- 
2.25.1

[PATCH v2 5/5] PCI/pwrctl: Remove pwrctl device without iterating over all children of pwrctl parent

[Manivannan Sadhasivam via B4 Relay]

From: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

There is no need to iterate over all children of the pwrctl device parent
to remove the pwrctl device. Since the pwrctl device associated with the
PCI device can be found using of_find_device_by_node() API, use it directly
instead.

If any pwrctl devices lying around without getting associated with the PCI
devices, then those will be removed once their parent device gets removed.

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/remove.c | 17 ++++++++---------
 1 file changed, 8 insertions(+), 9 deletions(-)

diff --git a/drivers/pci/remove.c b/drivers/pci/remove.c
index e4ce1145aa3e..3dd9b3024956 100644
--- a/drivers/pci/remove.c
+++ b/drivers/pci/remove.c
@@ -17,16 +17,16 @@ static void pci_free_resources(struct pci_dev *dev)
 	}
 }
 
-static int pci_pwrctl_unregister(struct device *dev, void *data)
+static void pci_pwrctl_unregister(struct device *dev)
 {
-	struct device_node *pci_node = data, *plat_node = dev_of_node(dev);
+	struct platform_device *pdev;
 
-	if (dev_is_platform(dev) && plat_node && plat_node == pci_node) {
-		of_device_unregister(to_platform_device(dev));
-		of_node_clear_flag(plat_node, OF_POPULATED);
-	}
+	pdev = of_find_device_by_node(dev_of_node(dev));
+	if (!pdev)
+		return;
 
-	return 0;
+	of_device_unregister(pdev);
+	of_node_clear_flag(dev_of_node(dev), OF_POPULATED);
 }
 
 static void pci_stop_dev(struct pci_dev *dev)
@@ -34,8 +34,7 @@ static void pci_stop_dev(struct pci_dev *dev)
 	pci_pme_active(dev, false);
 
 	if (pci_dev_is_added(dev)) {
-		device_for_each_child(dev->dev.parent, dev_of_node(&dev->dev),
-				      pci_pwrctl_unregister);
+		pci_pwrctl_unregister(&dev->dev);
 		device_release_driver(&dev->dev);
 		pci_proc_detach_device(dev);
 		pci_remove_sysfs_dev_files(dev);

-- 
2.25.1

Re: [PATCH v2 1/5] PCI/pwrctl: Use of_platform_device_create() to create pwrctl devices

[Bartosz Golaszewski]

On Fri, Oct 25, 2024 at 9:55 AM Manivannan Sadhasivam via B4 Relay
<devnull+manivannan.sadhasivam.linaro.org@kernel.org> wrote:
>
> From: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
>
> of_platform_populate() API creates platform devices by descending through
> the children of the parent node. But it provides no control over the child
> nodes, which makes it difficult to add checks for the child nodes in the
> future. So use of_platform_device_create() API together with
> for_each_child_of_node_scoped() so that it is possible to add checks for
> each node before creating the platform device.
>
> 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 | 12 +++++++-----
>  1 file changed, 7 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/pci/bus.c b/drivers/pci/bus.c
> index 55c853686051..9d278d3a19ff 100644
> --- a/drivers/pci/bus.c
> +++ b/drivers/pci/bus.c
> @@ -13,6 +13,7 @@
>  #include <linux/ioport.h>
>  #include <linux/of.h>
>  #include <linux/of_platform.h>
> +#include <linux/platform_device.h>
>  #include <linux/proc_fs.h>
>  #include <linux/slab.h>
>
> @@ -329,6 +330,7 @@ void __weak pcibios_bus_add_device(struct pci_dev *pdev) { }
>  void pci_bus_add_device(struct pci_dev *dev)
>  {
>         struct device_node *dn = dev->dev.of_node;
> +       struct platform_device *pdev;
>         int retval;
>
>         /*
> @@ -351,11 +353,11 @@ void pci_bus_add_device(struct pci_dev *dev)
>         pci_dev_assign_added(dev, true);
>
>         if (dev_of_node(&dev->dev) && pci_is_bridge(dev)) {
> -               retval = of_platform_populate(dev_of_node(&dev->dev), NULL, NULL,
> -                                             &dev->dev);
> -               if (retval)
> -                       pci_err(dev, "failed to populate child OF nodes (%d)\n",
> -                               retval);
> +               for_each_available_child_of_node_scoped(dn, child) {
> +                       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);
>
> --
> 2.25.1
>
>

Reviewed-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

Re: [PATCH v2 4/5] PCI/pwrctl: Move pwrctl device creation to its own helper function

[Bartosz Golaszewski]

On Fri, Oct 25, 2024 at 9:55 AM Manivannan Sadhasivam via B4 Relay
<devnull+manivannan.sadhasivam.linaro.org@kernel.org> wrote:
>
> From: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
>
> This makes the pci_bus_add_device() API easier to maintain. Also add more
> comments to the helper to describe how the devices are created.
>
> 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>
> ---

Reviewed-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

Re: [PATCH v2 2/5] PCI/pwrctl: Create pwrctl devices only if at least one power supply is present

[Bjorn Helgaas]

On Fri, Oct 25, 2024 at 01:24:52PM +0530, Manivannan Sadhasivam via B4 Relay wrote:
> From: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> 
> Currently, pwrctl devices are created if the corresponding PCI nodes are
> defined in devicetree. But this is not correct, because not all PCI nodes
> defined in devicetree require pwrctl support. Pwrctl comes into picture
> only when the device requires kernel to manage its power state. This can
> be determined using the power supply properties present in the devicetree
> node of the device.

> +bool of_pci_is_supply_present(struct device_node *np)
> +{
> +	struct property *prop;
> +	char *supply;
> +
> +	if (!np)
> +		return false;

Why do we need to test !np here?  It should always be non-NULL.

> +	for_each_property_of_node(np, prop) {
> +		supply = strrchr(prop->name, '-');
> +		if (supply && !strcmp(supply, "-supply"))
> +			return true;
> +	}
> +
> +	return false;
> +}

Re: [PATCH v2 2/5] PCI/pwrctl: Create pwrctl devices only if at least one power supply is present

[Bartosz Golaszewski]

On Wed, Nov 6, 2024 at 10:28 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
>
> On Fri, Oct 25, 2024 at 01:24:52PM +0530, Manivannan Sadhasivam via B4 Relay wrote:
> > From: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> >
> > Currently, pwrctl devices are created if the corresponding PCI nodes are
> > defined in devicetree. But this is not correct, because not all PCI nodes
> > defined in devicetree require pwrctl support. Pwrctl comes into picture
> > only when the device requires kernel to manage its power state. This can
> > be determined using the power supply properties present in the devicetree
> > node of the device.
>
> > +bool of_pci_is_supply_present(struct device_node *np)
> > +{
> > +     struct property *prop;
> > +     char *supply;
> > +
> > +     if (!np)
> > +             return false;
>
> Why do we need to test !np here?  It should always be non-NULL.
>

Right, I think this can be dropped. We check for the OF node in the
function above.

Bart

Re: [PATCH v2 2/5] PCI/pwrctl: Create pwrctl devices only if at least one power supply is present

[Manivannan Sadhasivam]

On Thu, Nov 07, 2024 at 10:52:35AM +0100, Bartosz Golaszewski wrote:
> On Wed, Nov 6, 2024 at 10:28 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
> >
> > On Fri, Oct 25, 2024 at 01:24:52PM +0530, Manivannan Sadhasivam via B4 Relay wrote:
> > > From: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > >
> > > Currently, pwrctl devices are created if the corresponding PCI nodes are
> > > defined in devicetree. But this is not correct, because not all PCI nodes
> > > defined in devicetree require pwrctl support. Pwrctl comes into picture
> > > only when the device requires kernel to manage its power state. This can
> > > be determined using the power supply properties present in the devicetree
> > > node of the device.
> >
> > > +bool of_pci_is_supply_present(struct device_node *np)
> > > +{
> > > +     struct property *prop;
> > > +     char *supply;
> > > +
> > > +     if (!np)
> > > +             return false;
> >
> > Why do we need to test !np here?  It should always be non-NULL.
> >
> 
> Right, I think this can be dropped. We check for the OF node in the
> function above.
> 

I think it was a leftover that I didn't cleanup. But I do plan to move this API
to drivers/of once 6.13-rc1 is out. So even if it didn't get dropped now, I will
do it later.

- Mani

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

Re: [PATCH v2 2/5] PCI/pwrctl: Create pwrctl devices only if at least one power supply is present

[Krzysztof Wilczyński]

Hello,

[...]
> > > > +bool of_pci_is_supply_present(struct device_node *np)
> > > > +{
> > > > +     struct property *prop;
> > > > +     char *supply;
> > > > +
> > > > +     if (!np)
> > > > +             return false;
> > >
> > > Why do we need to test !np here?  It should always be non-NULL.
> > >
> > 
> > Right, I think this can be dropped. We check for the OF node in the
> > function above.
> > 
> 
> I think it was a leftover that I didn't cleanup. But I do plan to move this API
> to drivers/of once 6.13-rc1 is out. So even if it didn't get dropped now, I will
> do it later.

I removed the NULL check directly on the branch.  Thank you!

	Krzysztof

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

[Bjorn Helgaas]

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?

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

[Manivannan Sadhasivam]

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

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

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

[Bjorn Helgaas]

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);
 	}

Re: [PATCH v2 2/5] PCI/pwrctl: Create pwrctl devices only if at least one power supply is present

[Klara Modin]

[-- Attachment #1: Type: text/plain, Size: 926 bytes --]

Hi,

On 2024-11-16 19:41, Krzysztof Wilczyński wrote:
> Hello,
> 
> [...]
>>>>> +bool of_pci_is_supply_present(struct device_node *np)
>>>>> +{
>>>>> +     struct property *prop;
>>>>> +     char *supply;
>>>>> +
>>>>> +     if (!np)
>>>>> +             return false;
>>>>
>>>> Why do we need to test !np here?  It should always be non-NULL.
>>>>

This doesn't appear to be the case, at least on my x86 machine and on 
x86 QEMU with CONFIG_OF enabled.

>>>
>>> Right, I think this can be dropped. We check for the OF node in the
>>> function above.
>>>
>>
>> I think it was a leftover that I didn't cleanup. But I do plan to move this API
>> to drivers/of once 6.13-rc1 is out. So even if it didn't get dropped now, I will
>> do it later.
> 
> I removed the NULL check directly on the branch.  Thank you!
> 
> 	Krzysztof
> 

I get a NULL pointer exception at boot which disappears if I readd the 
check.

Regards,
Klara Modin

[-- Attachment #2: of_pci_null_ptr_decoded --]
[-- Type: text/plain, Size: 30776 bytes --]

[    0.000000] Linux version 6.12.0-next-20241121-00021-g08d9c92ab3f3 (klara@soda.int.kasm.eu) (gcc (Gentoo 14.2.1_p20241026 p1) 14.2.1 20241026, GNU ld (Gentoo 2.43 p3) 2.43.1) #247 SMP PREEMPT_DYNAMIC Thu Nov 21 09:23:44 CET 2024
[    0.000000] Command line: root=/dev/nvme0n1 console=tty0 console=ttyS0,115200  overlay=/dev/nvme0n2 overlayflags=compress=zstd initrd=initrd
[    0.000000] BIOS-provided physical RAM map:
[    0.000000] BIOS-e820: [mem 0x0000000000000000-0x000000000009ffff] usable
[    0.000000] BIOS-e820: [mem 0x0000000000100000-0x00000000007fffff] usable
[    0.000000] BIOS-e820: [mem 0x0000000000800000-0x0000000000807fff] ACPI NVS
[    0.000000] BIOS-e820: [mem 0x0000000000808000-0x000000000080afff] usable
[    0.000000] BIOS-e820: [mem 0x000000000080b000-0x000000000080bfff] ACPI NVS
[    0.000000] BIOS-e820: [mem 0x000000000080c000-0x000000000080ffff] usable
[    0.000000] BIOS-e820: [mem 0x0000000000810000-0x00000000008fffff] ACPI NVS
[    0.000000] BIOS-e820: [mem 0x0000000000900000-0x000000003f8eefff] usable
[    0.000000] BIOS-e820: [mem 0x000000003f8ef000-0x000000003fb6efff] reserved
[    0.000000] BIOS-e820: [mem 0x000000003fb6f000-0x000000003fb7efff] ACPI data
[    0.000000] BIOS-e820: [mem 0x000000003fb7f000-0x000000003fbfefff] ACPI NVS
[    0.000000] BIOS-e820: [mem 0x000000003fbff000-0x000000003ff4ffff] usable
[    0.000000] BIOS-e820: [mem 0x000000003ff50000-0x000000003ff6ffff] reserved
[    0.000000] BIOS-e820: [mem 0x000000003ff70000-0x000000003fffffff] ACPI NVS
[    0.000000] BIOS-e820: [mem 0x00000000b0000000-0x00000000bfffffff] reserved
[    0.000000] NX (Execute Disable) protection: active
[    0.000000] APIC: Static calls initialized
[    0.000000] extended physical RAM map:
[    0.000000] reserve setup_data: [mem 0x0000000000000000-0x000000000009ffff] usable
[    0.000000] reserve setup_data: [mem 0x0000000000100000-0x00000000007fffff] usable
[    0.000000] reserve setup_data: [mem 0x0000000000800000-0x0000000000807fff] ACPI NVS
[    0.000000] reserve setup_data: [mem 0x0000000000808000-0x000000000080afff] usable
[    0.000000] reserve setup_data: [mem 0x000000000080b000-0x000000000080bfff] ACPI NVS
[    0.000000] reserve setup_data: [mem 0x000000000080c000-0x000000000080ffff] usable
[    0.000000] reserve setup_data: [mem 0x0000000000810000-0x00000000008fffff] ACPI NVS
[    0.000000] reserve setup_data: [mem 0x0000000000900000-0x000000003ccf6017] usable
[    0.000000] reserve setup_data: [mem 0x000000003ccf6018-0x000000003cd1ce57] usable
[    0.000000] reserve setup_data: [mem 0x000000003cd1ce58-0x000000003cd1d017] usable
[    0.000000] reserve setup_data: [mem 0x000000003cd1d018-0x000000003cd26a57] usable
[    0.000000] reserve setup_data: [mem 0x000000003cd26a58-0x000000003f8eefff] usable
[    0.000000] reserve setup_data: [mem 0x000000003f8ef000-0x000000003fb6efff] reserved
[    0.000000] reserve setup_data: [mem 0x000000003fb6f000-0x000000003fb7efff] ACPI data
[    0.000000] reserve setup_data: [mem 0x000000003fb7f000-0x000000003fbfefff] ACPI NVS
[    0.000000] reserve setup_data: [mem 0x000000003fbff000-0x000000003ff4ffff] usable
[    0.000000] reserve setup_data: [mem 0x000000003ff50000-0x000000003ff6ffff] reserved
[    0.000000] reserve setup_data: [mem 0x000000003ff70000-0x000000003fffffff] ACPI NVS
[    0.000000] reserve setup_data: [mem 0x00000000b0000000-0x00000000bfffffff] reserved
[    0.000000] efi: EFI v2.7 by EDK II
[    0.000000] efi: SMBIOS=0x3f9ab000 ACPI=0x3fb7e000 ACPI 2.0=0x3fb7e014 MEMATTR=0x3d7b9018 INITRD=0x3d7baf18
[    0.000000] SMBIOS 2.8 present.
[    0.000000] DMI: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 0.0.0 02/06/2015
[    0.000000] DMI: Memory slots populated: 1/1
[    0.000000] Hypervisor detected: KVM
[    0.000000] kvm-clock: Using msrs 4b564d01 and 4b564d00
[    0.000000] kvm-clock: using sched offset of 2260527937 cycles
[    0.000001] clocksource: kvm-clock: mask: 0xffffffffffffffff max_cycles: 0x1cd42e4dffb, max_idle_ns: 881590591483 ns
[    0.000003] tsc: Detected 3799.998 MHz processor
[    0.000088] last_pfn = 0x3ff50 max_arch_pfn = 0x400000000
[    0.000109] MTRR map: 4 entries (2 fixed + 2 variable; max 18), built from 8 variable MTRRs
[    0.000111] x86/PAT: Configuration [0-7]: WB  WC  UC- UC  WB  WP  UC- WT
[    0.000156] Using GB pages for direct mapping
[    0.000426] Secure boot disabled
[    0.000426] RAMDISK: [mem 0x3cd27000-0x3d5e9fff]
[    0.000427] ACPI: Early table checksum verification disabled
[    0.000429] ACPI: RSDP 0x000000003FB7E014 000024 (v02 BOCHS )
[    0.000432] ACPI: XSDT 0x000000003FB7D0E8 000054 (v01 BOCHS  BXPC     00000001      01000013)
[    0.000435] ACPI: FACP 0x000000003FB79000 0000F4 (v03 BOCHS  BXPC     00000001 BXPC 00000001)
[    0.000438] ACPI: DSDT 0x000000003FB7A000 002118 (v01 BOCHS  BXPC     00000001 BXPC 00000001)
[    0.000440] ACPI: FACS 0x000000003FBDD000 000040
[    0.000441] ACPI: APIC 0x000000003FB78000 000080 (v03 BOCHS  BXPC     00000001 BXPC 00000001)
[    0.000443] ACPI: HPET 0x000000003FB77000 000038 (v01 BOCHS  BXPC     00000001 BXPC 00000001)
[    0.000444] ACPI: MCFG 0x000000003FB76000 00003C (v01 BOCHS  BXPC     00000001 BXPC 00000001)
[    0.000446] ACPI: WAET 0x000000003FB75000 000028 (v01 BOCHS  BXPC     00000001 BXPC 00000001)
[    0.000447] ACPI: BGRT 0x000000003FB74000 000038 (v01 INTEL  EDK2     00000002      01000013)
[    0.000449] ACPI: Reserving FACP table memory at [mem 0x3fb79000-0x3fb790f3]
[    0.000450] ACPI: Reserving DSDT table memory at [mem 0x3fb7a000-0x3fb7c117]
[    0.000450] ACPI: Reserving FACS table memory at [mem 0x3fbdd000-0x3fbdd03f]
[    0.000451] ACPI: Reserving APIC table memory at [mem 0x3fb78000-0x3fb7807f]
[    0.000451] ACPI: Reserving HPET table memory at [mem 0x3fb77000-0x3fb77037]
[    0.000452] ACPI: Reserving MCFG table memory at [mem 0x3fb76000-0x3fb7603b]
[    0.000452] ACPI: Reserving WAET table memory at [mem 0x3fb75000-0x3fb75027]
[    0.000453] ACPI: Reserving BGRT table memory at [mem 0x3fb74000-0x3fb74037]
[    0.000476] No NUMA configuration found
[    0.000477] Faking a node at [mem 0x0000000000000000-0x000000003ff4ffff]
[    0.000479] NODE_DATA(0) allocated [mem 0x3fec9200-0x3fecbdff]
[    0.000489] Zone ranges:
[    0.000490]   DMA      [mem 0x0000000000001000-0x0000000000ffffff]
[    0.000491]   DMA32    [mem 0x0000000001000000-0x000000003ff4ffff]
[    0.000492]   Normal   empty
[    0.000492]   Device   empty
[    0.000493] Movable zone start for each node
[    0.000493] Early memory node ranges
[    0.000494]   node   0: [mem 0x0000000000001000-0x000000000009ffff]
[    0.000494]   node   0: [mem 0x0000000000100000-0x00000000007fffff]
[    0.000495]   node   0: [mem 0x0000000000808000-0x000000000080afff]
[    0.000496]   node   0: [mem 0x000000000080c000-0x000000000080ffff]
[    0.000496]   node   0: [mem 0x0000000000900000-0x000000003f8eefff]
[    0.000497]   node   0: [mem 0x000000003fbff000-0x000000003ff4ffff]
[    0.000498] Initmem setup node 0 [mem 0x0000000000001000-0x000000003ff4ffff]
[    0.000631] On node 0, zone DMA: 1 pages in unavailable ranges
[    0.000638] On node 0, zone DMA: 96 pages in unavailable ranges
[    0.000639] On node 0, zone DMA: 8 pages in unavailable ranges
[    0.000639] On node 0, zone DMA: 1 pages in unavailable ranges
[    0.000646] On node 0, zone DMA: 240 pages in unavailable ranges
[    0.001737] On node 0, zone DMA32: 784 pages in unavailable ranges
[    0.001740] On node 0, zone DMA32: 176 pages in unavailable ranges
[    0.001934] ACPI: PM-Timer IO Port: 0x608
[    0.001939] ACPI: LAPIC_NMI (acpi_id[0xff] dfl dfl lint[0x1])
[    0.001958] IOAPIC[0]: apic_id 0, version 17, address 0xfec00000, GSI 0-23
[    0.001960] ACPI: INT_SRC_OVR (bus 0 bus_irq 0 global_irq 2 dfl dfl)
[    0.001961] ACPI: INT_SRC_OVR (bus 0 bus_irq 5 global_irq 5 high level)
[    0.001961] ACPI: INT_SRC_OVR (bus 0 bus_irq 9 global_irq 9 high level)
[    0.001962] ACPI: INT_SRC_OVR (bus 0 bus_irq 10 global_irq 10 high level)
[    0.001963] ACPI: INT_SRC_OVR (bus 0 bus_irq 11 global_irq 11 high level)
[    0.001965] ACPI: Using ACPI (MADT) for SMP configuration information
[    0.001966] ACPI: HPET id: 0x8086a201 base: 0xfed00000
[    0.001981] TSC deadline timer available
[    0.001984] CPU topo: Max. logical packages:   1
[    0.001985] CPU topo: Max. logical dies:       1
[    0.001985] CPU topo: Max. dies per package:   1
[    0.001989] CPU topo: Max. threads per core:   1
[    0.001989] CPU topo: Num. cores per package:     2
[    0.001990] CPU topo: Num. threads per package:   2
[    0.001990] CPU topo: Allowing 2 present CPUs plus 0 hotplug CPUs
[    0.002000] kvm-guest: APIC: eoi() replaced with kvm_guest_apic_eoi_write()
[    0.002008] kvm-guest: KVM setup pv remote TLB flush
[    0.002010] kvm-guest: setup PV sched yield
[    0.002019] PM: hibernation: Registered nosave memory: [mem 0x00000000-0x00000fff]
[    0.002020] PM: hibernation: Registered nosave memory: [mem 0x000a0000-0x000fffff]
[    0.002021] PM: hibernation: Registered nosave memory: [mem 0x00800000-0x00807fff]
[    0.002022] PM: hibernation: Registered nosave memory: [mem 0x0080b000-0x0080bfff]
[    0.002023] PM: hibernation: Registered nosave memory: [mem 0x00810000-0x008fffff]
[    0.002024] PM: hibernation: Registered nosave memory: [mem 0x3ccf6000-0x3ccf6fff]
[    0.002025] PM: hibernation: Registered nosave memory: [mem 0x3cd1c000-0x3cd1cfff]
[    0.002025] PM: hibernation: Registered nosave memory: [mem 0x3cd1d000-0x3cd1dfff]
[    0.002026] PM: hibernation: Registered nosave memory: [mem 0x3cd26000-0x3cd26fff]
[    0.002027] PM: hibernation: Registered nosave memory: [mem 0x3d7bc000-0x3d7c4fff]
[    0.002028] PM: hibernation: Registered nosave memory: [mem 0x3f8ef000-0x3fb6efff]
[    0.002029] PM: hibernation: Registered nosave memory: [mem 0x3fb6f000-0x3fb7efff]
[    0.002029] PM: hibernation: Registered nosave memory: [mem 0x3fb7f000-0x3fbfefff]
[    0.002030] [mem 0x40000000-0xafffffff] available for PCI devices
[    0.002031] Booting paravirtualized kernel on KVM
[    0.002033] clocksource: refined-jiffies: mask: 0xffffffff max_cycles: 0xffffffff, max_idle_ns: 7645519600211568 ns
[    0.006377] setup_percpu: NR_CPUS:64 nr_cpumask_bits:2 nr_cpu_ids:2 nr_node_ids:1
[    0.006538] percpu: Embedded 94 pages/cpu s290816 r65536 d28672 u1048576
[    0.006565] Kernel command line: root=/dev/nvme0n1 console=tty0 console=ttyS0,115200  overlay=/dev/nvme0n2 overlayflags=compress=zstd initrd=initrd
[    0.006598] Unknown kernel command line parameters "overlay=/dev/nvme0n2 overlayflags=compress=zstd", will be passed to user space.
[    0.006614] random: crng init done
[    0.006614] printk: log buffer data + meta data: 262144 + 917504 = 1179648 bytes
[    0.006671] Dentry cache hash table entries: 131072 (order: 8, 1048576 bytes, linear)
[    0.006708] Inode-cache hash table entries: 65536 (order: 7, 524288 bytes, linear)
[    0.006725] Fallback order for Node 0: 0
[    0.006727] Built 1 zonelists, mobility grouping on.  Total pages: 260838
[    0.006728] Policy zone: DMA32
[    0.006856] mem auto-init: stack:off, heap alloc:off, heap free:off
[    0.008133] SLUB: HWalign=64, Order=0-3, MinObjects=0, CPUs=2, Nodes=1
[    0.008338] allocated 2097152 bytes of page_ext
[    0.008347] ftrace: allocating 51498 entries in 202 pages
[    0.020955] ftrace: allocated 202 pages with 4 groups
[    0.021339] Dynamic Preempt: lazy
[    0.021474] rcu: Preemptible hierarchical RCU implementation.
[    0.021474] rcu: 	RCU event tracing is enabled.
[    0.021474] rcu: 	RCU restricting CPUs from NR_CPUS=64 to nr_cpu_ids=2.
[    0.021475] 	Trampoline variant of Tasks RCU enabled.
[    0.021476] 	Rude variant of Tasks RCU enabled.
[    0.021476] 	Tracing variant of Tasks RCU enabled.
[    0.021476] rcu: RCU calculated value of scheduler-enlistment delay is 25 jiffies.
[    0.021477] rcu: Adjusting geometry for rcu_fanout_leaf=16, nr_cpu_ids=2
[    0.021481] RCU Tasks: Setting shift to 1 and lim to 1 rcu_task_cb_adjust=1 rcu_task_cpu_ids=2.
[    0.021482] RCU Tasks Rude: Setting shift to 1 and lim to 1 rcu_task_cb_adjust=1 rcu_task_cpu_ids=2.
[    0.021483] RCU Tasks Trace: Setting shift to 1 and lim to 1 rcu_task_cb_adjust=1 rcu_task_cpu_ids=2.
[    0.024849] NR_IRQS: 4352, nr_irqs: 440, preallocated irqs: 16
[    0.025020] rcu: srcu_init: Setting srcu_struct sizes based on contention.
[    0.025082] kfence: initialized - using 2097152 bytes for 255 objects at 0x(____ptrval____)-0x(____ptrval____)
[    0.025103] Console: colour dummy device 80x25
[    0.025104] printk: legacy console [tty0] enabled
[    0.025326] printk: legacy console [ttyS0] enabled
[    0.133937] ACPI: Core revision 20240827
[    0.134383] clocksource: hpet: mask: 0xffffffff max_cycles: 0xffffffff, max_idle_ns: 19112604467 ns
[    0.135320] APIC: Switch to symmetric I/O mode setup
[    0.135818] kvm-guest: APIC: send_IPI_mask() replaced with kvm_send_ipi_mask()
[    0.136589] kvm-guest: APIC: send_IPI_mask_allbutself() replaced with kvm_send_ipi_mask_allbutself()
[    0.137451] kvm-guest: setup PV IPIs
[    0.138526] ..TIMER: vector=0x30 apic1=0 pin1=2 apic2=-1 pin2=-1
[    0.139115] clocksource: tsc-early: mask: 0xffffffffffffffff max_cycles: 0x6d8cab05ec1, max_idle_ns: 881590646801 ns
[    0.140167] Calibrating delay loop (skipped) preset value.. 7599.99 BogoMIPS (lpj=15199992)
[    0.141032] x86/cpu: User Mode Instruction Prevention (UMIP) activated
[    0.141713] Last level iTLB entries: 4KB 512, 2MB 255, 4MB 127
[    0.142274] Last level dTLB entries: 4KB 512, 2MB 255, 4MB 127, 1GB 0
[    0.142901] Spectre V1 : Mitigation: usercopy/swapgs barriers and __user pointer sanitization
[    0.144169] Spectre V2 : Mitigation: Retpolines
[    0.144607] Spectre V2 : Spectre v2 / SpectreRSB mitigation: Filling RSB on context switch
[    0.145401] Spectre V2 : Spectre v2 / SpectreRSB : Filling RSB on VMEXIT
[    0.146049] Spectre V2 : Enabling Speculation Barrier for firmware calls
[    0.146692] RETBleed: Mitigation: untrained return thunk
[    0.147205] Spectre V2 : mitigation: Enabling conditional Indirect Branch Prediction Barrier
[    0.148168] Speculative Store Bypass: Mitigation: Speculative Store Bypass disabled via prctl
[    0.148958] Speculative Return Stack Overflow: IBPB-extending microcode not applied!
[    0.149680] Speculative Return Stack Overflow: WARNING: See https://kernel.org/doc/html/latest/admin-guide/hw-vuln/srso.html for mitigation options.
[    0.149681] Speculative Return Stack Overflow: Vulnerable: Safe RET, no microcode
[    0.151703] x86/fpu: Supporting XSAVE feature 0x001: 'x87 floating point registers'
[    0.152169] x86/fpu: Supporting XSAVE feature 0x002: 'SSE registers'
[    0.152756] x86/fpu: Supporting XSAVE feature 0x004: 'AVX registers'
[    0.153349] x86/fpu: xstate_offset[2]:  576, xstate_sizes[2]:  256
[    0.153930] x86/fpu: Enabled xstate features 0x7, context size is 832 bytes, using 'compacted' format.
[    0.169962] Freeing SMP alternatives memory: 48K
[    0.170414] pid_max: default: 32768 minimum: 301
[    0.174491] LSM: initializing lsm=capability,bpf
[    0.175102] LSM support for eBPF active
[    0.175494] Mount-cache hash table entries: 2048 (order: 2, 16384 bytes, linear)
[    0.176173] Mountpoint-cache hash table entries: 2048 (order: 2, 16384 bytes, linear)
[    0.177113] smpboot: CPU0: AMD Ryzen 9 3900X 12-Core Processor (family: 0x17, model: 0x71, stepping: 0x0)
[    0.178242] Performance Events: Fam17h+ core perfctr, AMD PMU driver.
[    0.178876] ... version:                0
[    0.179264] ... bit width:              48
[    0.179659] ... generic registers:      6
[    0.180046] ... value mask:             0000ffffffffffff
[    0.180166] ... max period:             00007fffffffffff
[    0.180166] ... fixed-purpose events:   0
[    0.180174] ... event mask:             000000000000003f
[    0.180764] signal: max sigframe size: 1776
[    0.181195] rcu: Hierarchical SRCU implementation.
[    0.181659] rcu: 	Max phase no-delay instances is 1000.
[    0.182185] Timer migration: 1 hierarchy levels; 8 children per group; 1 crossnode level
[    0.186632] smp: Bringing up secondary CPUs ...
[    0.187193] smpboot: x86: Booting SMP configuration:
[    0.187671] .... node  #0, CPUs:      #1
[    0.187765] smp: Brought up 1 node, 2 CPUs
[    0.188571] smpboot: Total of 2 processors activated (15199.99 BogoMIPS)
[    0.189314] Memory: 956380K/1043352K available (18432K kernel code, 2891K rwdata, 12548K rodata, 3368K init, 4652K bss, 80124K reserved, 0K cma-reserved)
[    0.190696] devtmpfs: initialized
[    0.190696] x86/mm: Memory block size: 128MB
[    0.190696] ACPI: PM: Registering ACPI NVS region [mem 0x00800000-0x00807fff] (32768 bytes)
[    0.192176] ACPI: PM: Registering ACPI NVS region [mem 0x0080b000-0x0080bfff] (4096 bytes)
[    0.192963] ACPI: PM: Registering ACPI NVS region [mem 0x00810000-0x008fffff] (983040 bytes)
[    0.193781] ACPI: PM: Registering ACPI NVS region [mem 0x3fb7f000-0x3fbfefff] (524288 bytes)
[    0.194595] ACPI: PM: Registering ACPI NVS region [mem 0x3ff70000-0x3fffffff] (589824 bytes)
[    0.195429] clocksource: jiffies: mask: 0xffffffff max_cycles: 0xffffffff, max_idle_ns: 7645041785100000 ns
[    0.196176] futex hash table entries: 512 (order: 3, 32768 bytes, linear)
[    0.196845] pinctrl core: initialized pinctrl subsystem
[    0.197432] PM: RTC time: 08:28:47, date: 2024-11-21
[    0.198508] NET: Registered PF_NETLINK/PF_ROUTE protocol family
[    0.199180] DMA: preallocated 128 KiB GFP_KERNEL pool for atomic allocations
[    0.199871] DMA: preallocated 128 KiB GFP_KERNEL|GFP_DMA pool for atomic allocations
[    0.200176] DMA: preallocated 128 KiB GFP_KERNEL|GFP_DMA32 pool for atomic allocations
[    0.201011] thermal_sys: Registered thermal governor 'step_wise'
[    0.201012] thermal_sys: Registered thermal governor 'user_space'
[    0.201616] cpuidle: using governor menu
[    0.202906] PCI: ECAM [mem 0xb0000000-0xbfffffff] (base 0xb0000000) for domain 0000 [bus 00-ff]
[    0.202906] PCI: ECAM [mem 0xb0000000-0xbfffffff] reserved as E820 entry
[    0.204185] PCI: Using configuration type 1 for base access
[    0.204824] kprobes: kprobe jump-optimization is enabled. All kprobes are optimized if possible.
[    0.220413] HugeTLB: registered 1.00 GiB page size, pre-allocated 0 pages
[    0.221149] HugeTLB: 16380 KiB vmemmap can be freed for a 1.00 GiB page
[    0.221846] HugeTLB: registered 2.00 MiB page size, pre-allocated 0 pages
[    0.222563] HugeTLB: 28 KiB vmemmap can be freed for a 2.00 MiB page
[    0.296168] raid6: avx2x4   gen() 40358 MB/s
[    0.364169] raid6: avx2x2   gen() 41162 MB/s
[    0.432168] raid6: avx2x1   gen() 34399 MB/s
[    0.432632] raid6: using algorithm avx2x2 gen() 41162 MB/s
[    0.500168] raid6: .... xor() 26429 MB/s, rmw enabled
[    0.500667] raid6: using avx2x2 recovery algorithm
[    0.501195] ACPI: Added _OSI(Module Device)
[    0.501611] ACPI: Added _OSI(Processor Device)
[    0.502051] ACPI: Added _OSI(3.0 _SCP Extensions)
[    0.502518] ACPI: Added _OSI(Processor Aggregator Device)
[    0.504421] ACPI: 1 ACPI AML tables successfully acquired and loaded
[    0.505331] ACPI: Interpreter enabled
[    0.505331] ACPI: PM: (supports S0 S3 S4 S5)
[    0.505331] ACPI: Using IOAPIC for interrupt routing
[    0.505644] PCI: Using host bridge windows from ACPI; if necessary, use "pci=nocrs" and report a bug
[    0.506529] PCI: Using E820 reservations for host bridge windows
[    0.508254] ACPI: Enabled 2 GPEs in block 00 to 3F
[    0.510776] ACPI: PCI Root Bridge [PCI0] (domain 0000 [bus 00-ff])
[    0.511381] acpi PNP0A08:00: _OSC: OS supports [ExtendedConfig ASPM ClockPM Segments MSI HPX-Type3]
[    0.512222] acpi PNP0A08:00: _OSC: platform does not support [PCIeHotplug LTR]
[    0.513027] acpi PNP0A08:00: _OSC: OS now controls [PME AER PCIeCapability]
[    0.513761] PCI host bridge to bus 0000:00
[    0.514177] pci_bus 0000:00: root bus resource [io  0x0000-0x0cf7 window]
[    0.514839] pci_bus 0000:00: root bus resource [io  0x0d00-0xffff window]
[    0.515493] pci_bus 0000:00: root bus resource [mem 0x000a0000-0x000bffff window]
[    0.516173] pci_bus 0000:00: root bus resource [mem 0x40000000-0xafffffff window]
[    0.516900] pci_bus 0000:00: root bus resource [mem 0xc0000000-0xfebfffff window]
[    0.517619] pci_bus 0000:00: root bus resource [mem 0x800000000-0xfffffffff window]
[    0.518358] pci_bus 0000:00: root bus resource [bus 00-ff]
[    0.518914] pci 0000:00:00.0: [8086:29c0] type 00 class 0x060000 conventional PCI endpoint
[    0.519971] pci 0000:00:01.0: [1234:1111] type 00 class 0x030000 conventional PCI endpoint
[    0.522260] pci 0000:00:01.0: BAR 0 [mem 0xc0000000-0xc0ffffff pref]
[    0.524523] pci 0000:00:01.0: BAR 2 [mem 0xc1085000-0xc1085fff]
[    0.528527] pci 0000:00:01.0: ROM [mem 0xffff0000-0xffffffff pref]
[    0.529160] pci 0000:00:01.0: Video device with shadowed ROM at [mem 0x000c0000-0x000dffff]
[    0.530558] pci 0000:00:02.0: [8086:10d3] type 00 class 0x020000 PCIe Root Complex Integrated Endpoint
[    0.532169] pci 0000:00:02.0: BAR 0 [mem 0xc1060000-0xc107ffff]
[    0.533438] pci 0000:00:02.0: BAR 1 [mem 0xc1040000-0xc105ffff]
[    0.534698] pci 0000:00:02.0: BAR 2 [io  0x6060-0x607f]
[    0.536129] pci 0000:00:02.0: BAR 3 [mem 0xc1080000-0xc1083fff]
[    0.539900] pci 0000:00:02.0: ROM [mem 0xfffc0000-0xffffffff pref]
[    0.540641] pci 0000:00:03.0: [1b36:0010] type 00 class 0x010802 PCIe Root Complex Integrated Endpoint
[    0.541802] pci 0000:00:03.0: BAR 0 [mem 0x800000000-0x800003fff 64bit]
[    0.543779] pci 0000:00:1f.0: [8086:2918] type 00 class 0x060100 conventional PCI endpoint
[    0.544424] pci 0000:00:1f.0: quirk: [io  0x0600-0x067f] claimed by ICH6 ACPI/GPIO/TCO
[    0.545352] pci 0000:00:1f.2: [8086:2922] type 00 class 0x010601 conventional PCI endpoint
[    0.547901] pci 0000:00:1f.2: BAR 4 [io  0x6040-0x605f]
[    0.548523] pci 0000:00:1f.2: BAR 5 [mem 0xc1084000-0xc1084fff]
[    0.550416] pci 0000:00:1f.3: [8086:2930] type 00 class 0x0c0500 conventional PCI endpoint
[    0.552241] pci 0000:00:1f.3: BAR 4 [io  0x6000-0x603f]
[    0.553307] BUG: kernel NULL pointer dereference, address: 0000000000000058
[    0.554054] #PF: supervisor read access in kernel mode
[    0.554609] #PF: error_code(0x0000) - not-present page
[    0.555155] PGD 0 P4D 0
[    0.555429] Oops: Oops: 0000 [#1] PREEMPT SMP NOPTI
[    0.555947] CPU: 0 UID: 0 PID: 1 Comm: swapper/0 Not tainted 6.12.0-next-20241121-00021-g08d9c92ab3f3 #247
[    0.556166] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 0.0.0 02/06/2015
[    0.556166] RIP: 0010:of_pci_supply_present (drivers/pci/of.c:746)
[ 0.556166] Code: a5 c8 00 00 00 48 89 ab 40 03 00 00 eb c6 66 90 90 90 90 90 90 90 90 90 90 90 90 90 90 90 90 90 66 0f 1f 00 0f 1f 44 00 00 53 <48> 8b 5f 58 48 85 db 74 2e 48 8b 3b be 2d 00 00 00 e8 20 d8 63 00
All code
========
   0:	a5                   	movsl  %ds:(%rsi),%es:(%rdi)
   1:	c8 00 00 00          	enter  $0x0,$0x0
   5:	48 89 ab 40 03 00 00 	mov    %rbp,0x340(%rbx)
   c:	eb c6                	jmp    0xffffffffffffffd4
   e:	66 90                	xchg   %ax,%ax
  10:	90                   	nop
  11:	90                   	nop
  12:	90                   	nop
  13:	90                   	nop
  14:	90                   	nop
  15:	90                   	nop
  16:	90                   	nop
  17:	90                   	nop
  18:	90                   	nop
  19:	90                   	nop
  1a:	90                   	nop
  1b:	90                   	nop
  1c:	90                   	nop
  1d:	90                   	nop
  1e:	90                   	nop
  1f:	90                   	nop
  20:	66 0f 1f 00          	nopw   (%rax)
  24:	0f 1f 44 00 00       	nopl   0x0(%rax,%rax,1)
  29:	53                   	push   %rbx
  2a:*	48 8b 5f 58          	mov    0x58(%rdi),%rbx		<-- trapping instruction
  2e:	48 85 db             	test   %rbx,%rbx
  31:	74 2e                	je     0x61
  33:	48 8b 3b             	mov    (%rbx),%rdi
  36:	be 2d 00 00 00       	mov    $0x2d,%esi
  3b:	e8 20 d8 63 00       	call   0x63d860

Code starting with the faulting instruction
===========================================
   0:	48 8b 5f 58          	mov    0x58(%rdi),%rbx
   4:	48 85 db             	test   %rbx,%rbx
   7:	74 2e                	je     0x37
   9:	48 8b 3b             	mov    (%rbx),%rdi
   c:	be 2d 00 00 00       	mov    $0x2d,%esi
  11:	e8 20 d8 63 00       	call   0x63d836
[    0.556166] RSP: 0018:ffffb6510001fb00 EFLAGS: 00010286
[    0.556166] RAX: ffff9c4d02126000 RBX: ffff9c4d02648000 RCX: ffffffff84c59080
[    0.556166] RDX: ffff9c4d02126000 RSI: 0000000000000000 RDI: 0000000000000000
[    0.556166] RBP: ffff9c4d02126000 R08: 0000000000000000 R09: 0000000000000008
[    0.556166] R10: 0000000000000002 R11: 0000000000000000 R12: 0000000000000000
[    0.556166] R13: ffff9c4d026480c8 R14: 0000000000000000 R15: ffff9c4d022eae18
[    0.556166] FS:  0000000000000000(0000) GS:ffff9c4d3ca00000(0000) knlGS:0000000000000000
[    0.556166] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[    0.556166] CR2: 0000000000000058 CR3: 0000000030634000 CR4: 0000000000350ef0
[    0.556166] Call Trace:
[    0.556166]  <TASK>
[    0.556166] ? __die_body.cold (arch/x86/kernel/dumpstack.c:478 (discriminator 1) arch/x86/kernel/dumpstack.c:465 (discriminator 1) arch/x86/kernel/dumpstack.c:420 (discriminator 1))
[    0.556166] ? page_fault_oops (arch/x86/mm/fault.c:711 (discriminator 1))
[    0.556166] ? exc_page_fault (arch/x86/include/asm/irqflags.h:37 arch/x86/include/asm/irqflags.h:92 arch/x86/mm/fault.c:1489 arch/x86/mm/fault.c:1539)
[    0.556166] ? asm_exc_page_fault (arch/x86/include/asm/idtentry.h:623)
[    0.556166] ? __pfx_device_match_of_node (drivers/base/core.c:5248)
[    0.556166] ? of_pci_supply_present (drivers/pci/of.c:746)
[    0.556166] pci_bus_add_device (drivers/pci/bus.c:408 (discriminator 1))
[    0.556166] pci_bus_add_devices (drivers/pci/bus.c:435 (discriminator 2))
[    0.556166] acpi_pci_root_add (drivers/acpi/pci_root.c:762)
[    0.556166] ? acpi_device_is_battery (drivers/acpi/scan.c:1249 (discriminator 1))
[    0.556166] acpi_bus_attach (drivers/acpi/scan.c:2261 drivers/acpi/scan.c:2309)
[    0.556166] ? __pfx_acpi_dev_for_one_check (drivers/acpi/bus.c:1139)
[    0.556166] device_for_each_child (drivers/base/core.c:3994)
[    0.556166] acpi_dev_for_each_child (drivers/acpi/bus.c:1158)
[    0.556166] ? __pfx_acpi_bus_attach (drivers/acpi/scan.c:2274)
[    0.556166] acpi_bus_attach (drivers/acpi/scan.c:2331 (discriminator 1))
[    0.556166] ? __pfx_acpi_dev_for_one_check (drivers/acpi/bus.c:1139)
[    0.556166] device_for_each_child (drivers/base/core.c:3994)
[    0.556166] acpi_dev_for_each_child (drivers/acpi/bus.c:1158)
[    0.556166] ? __pfx_acpi_bus_attach (drivers/acpi/scan.c:2274)
[    0.556166] acpi_bus_attach (drivers/acpi/scan.c:2331 (discriminator 1))
[    0.556166] acpi_bus_scan (drivers/acpi/scan.c:2541 drivers/acpi/scan.c:2614)
[    0.556166] acpi_scan_init (drivers/acpi/scan.c:2747 (discriminator 1))
[    0.556166] ? srso_return_thunk (arch/x86/lib/retpoline.S:224)
[    0.556166] acpi_init (drivers/acpi/bus.c:1467)
[    0.556166] ? add_device_randomness (drivers/char/random.c:950)
[    0.556166] ? __pfx_acpi_init (drivers/acpi/bus.c:1438)
[    0.556166] do_one_initcall (init/main.c:1266)
[    0.556166] kernel_init_freeable (init/main.c:1327 (discriminator 3) init/main.c:1344 (discriminator 3) init/main.c:1363 (discriminator 3) init/main.c:1577 (discriminator 3))
[    0.556166] ? __pfx_kernel_init (init/main.c:1458)
[    0.556166] kernel_init (init/main.c:1468)
[    0.556166] ret_from_fork (arch/x86/kernel/process.c:153)
[    0.556166] ? __pfx_kernel_init (init/main.c:1458)
[    0.556166] ret_from_fork_asm (arch/x86/entry/entry_64.S:254)
[    0.556166]  </TASK>
[    0.556166] Modules linked in:
[    0.556166] CR2: 0000000000000058
[    0.556166] ---[ end trace 0000000000000000 ]---
[    0.556166] RIP: 0010:of_pci_supply_present (drivers/pci/of.c:746)
[ 0.556166] Code: a5 c8 00 00 00 48 89 ab 40 03 00 00 eb c6 66 90 90 90 90 90 90 90 90 90 90 90 90 90 90 90 90 90 66 0f 1f 00 0f 1f 44 00 00 53 <48> 8b 5f 58 48 85 db 74 2e 48 8b 3b be 2d 00 00 00 e8 20 d8 63 00
All code
========
   0:	a5                   	movsl  %ds:(%rsi),%es:(%rdi)
   1:	c8 00 00 00          	enter  $0x0,$0x0
   5:	48 89 ab 40 03 00 00 	mov    %rbp,0x340(%rbx)
   c:	eb c6                	jmp    0xffffffffffffffd4
   e:	66 90                	xchg   %ax,%ax
  10:	90                   	nop
  11:	90                   	nop
  12:	90                   	nop
  13:	90                   	nop
  14:	90                   	nop
  15:	90                   	nop
  16:	90                   	nop
  17:	90                   	nop
  18:	90                   	nop
  19:	90                   	nop
  1a:	90                   	nop
  1b:	90                   	nop
  1c:	90                   	nop
  1d:	90                   	nop
  1e:	90                   	nop
  1f:	90                   	nop
  20:	66 0f 1f 00          	nopw   (%rax)
  24:	0f 1f 44 00 00       	nopl   0x0(%rax,%rax,1)
  29:	53                   	push   %rbx
  2a:*	48 8b 5f 58          	mov    0x58(%rdi),%rbx		<-- trapping instruction
  2e:	48 85 db             	test   %rbx,%rbx
  31:	74 2e                	je     0x61
  33:	48 8b 3b             	mov    (%rbx),%rdi
  36:	be 2d 00 00 00       	mov    $0x2d,%esi
  3b:	e8 20 d8 63 00       	call   0x63d860

Code starting with the faulting instruction
===========================================
   0:	48 8b 5f 58          	mov    0x58(%rdi),%rbx
   4:	48 85 db             	test   %rbx,%rbx
   7:	74 2e                	je     0x37
   9:	48 8b 3b             	mov    (%rbx),%rdi
   c:	be 2d 00 00 00       	mov    $0x2d,%esi
  11:	e8 20 d8 63 00       	call   0x63d836
[    0.556166] RSP: 0018:ffffb6510001fb00 EFLAGS: 00010286
[    0.556166] RAX: ffff9c4d02126000 RBX: ffff9c4d02648000 RCX: ffffffff84c59080
[    0.556166] RDX: ffff9c4d02126000 RSI: 0000000000000000 RDI: 0000000000000000
[    0.556166] RBP: ffff9c4d02126000 R08: 0000000000000000 R09: 0000000000000008
[    0.556166] R10: 0000000000000002 R11: 0000000000000000 R12: 0000000000000000
[    0.556166] R13: ffff9c4d026480c8 R14: 0000000000000000 R15: ffff9c4d022eae18
[    0.556166] FS:  0000000000000000(0000) GS:ffff9c4d3ca00000(0000) knlGS:0000000000000000
[    0.556166] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[    0.556166] CR2: 0000000000000058 CR3: 0000000030634000 CR4: 0000000000350ef0
[    0.556166] note: swapper/0[1] exited with irqs disabled
[    0.556170] Kernel panic - not syncing: Attempted to kill init! exitcode=0x00000009
[    0.557012] ---[ end Kernel panic - not syncing: Attempted to kill init! exitcode=0x00000009 ]---

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

[Manivannan Sadhasivam]

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

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

Re: [PATCH v2 2/5] PCI/pwrctl: Create pwrctl devices only if at least one power supply is present

[Krzysztof Wilczyński]

Hello,

> > > > > > +bool of_pci_is_supply_present(struct device_node *np)
> > > > > > +{
> > > > > > +     struct property *prop;
> > > > > > +     char *supply;
> > > > > > +
> > > > > > +     if (!np)
> > > > > > +             return false;
> > > > > 
> > > > > Why do we need to test !np here?  It should always be non-NULL.
> > > > > 
> 
> This doesn't appear to be the case, at least on my x86 machine and on x86
> QEMU with CONFIG_OF enabled.

Thank you for testing!  Much appreciated.

We've also received other signal about this, too, per:

  - https://lore.kernel.org/r/673f39b0.050a0220.363a1b.0126.GAE@google.com
  - https://lore.kernel.org/r/CA+G9fYurbY3B6ahZ+k+Syp5bZ3a+YQdeX8DRb6Twi4BDEFbUsw@mail.gmail.com

> > > > Right, I think this can be dropped. We check for the OF node in the
> > > > function above.
> > > > 
> > > 
> > > I think it was a leftover that I didn't cleanup. But I do plan to move this API
> > > to drivers/of once 6.13-rc1 is out. So even if it didn't get dropped now, I will
> > > do it later.
> > 
> > I removed the NULL check directly on the branch.  Thank you!

I added the NULL check back.

	Krzysztof

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

[Krzysztof Wilczyński]

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

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

[Krzysztof Wilczyński]

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