[PATCH 0/4] PCI/pwrctrl: Rework pwrctrl driver integration and add driver for PCI slot

[PATCH 0/4] PCI/pwrctrl: Rework pwrctrl driver integration and add driver for PCI slot

[Manivannan Sadhasivam via B4 Relay]

Hi,

This series reworks the PCI pwrctrl integration (again) by moving the creation
and removal of pwrctrl devices to pci_scan_device() and pci_destroy_dev() APIs.
This is based on the suggestion provided by Lukas Wunner [1][2]. With this
change, it is now possible to create pwrctrl devices for PCI bridges as well.
This is required to control the power state of the PCI slots in a system. Since
the PCI slots are not explicitly defined in devicetree, the agreement is to
define the supplies for PCI slots in PCI bridge nodes itself [3].

Based on this, a pwrctrl driver to control the supplies of PCI slots are also
added in patch 4. With this driver, it is now possible to control the voltage
regulators powering the PCI slots defined in PCI bridge nodes as below:

```
pcie@0 {
	compatible "pciclass,0604"
	...

	vpcie12v-supply = <&vpcie12v_reg>;
	vpcie3v3-supply = <&vpcie3v3_reg>;
	vpcie3v3aux-supply = <&vpcie3v3aux_reg>;
};
```

To make use of this driver, the PCI bridge DT node should also have the
compatible "pciclass,0604". But adding this compatible triggers the following
checkpatch warning:

WARNING: DT compatible string vendor "pciclass" appears un-documented --
check ./Documentation/devicetree/bindings/vendor-prefixes.yaml

For fixing it, I added patch 3. But due to some reason, checkpatch is not
picking the 'pciclass' vendor prefix alone, and requires adding the full
compatible 'pciclass,0604' in the vendor-prefixes list. Since my perl skills are
not great, I'm leaving it in the hands of Rob to fix the checkpatch script.

[1] https://lore.kernel.org/linux-pci/Z0yLDBMAsh0yKWf2@wunner.de
[2] https://lore.kernel.org/linux-pci/Z0xAdQ2ozspEnV5g@wunner.de
[3] https://github.com/devicetree-org/dt-schema/issues/145

Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---
Manivannan Sadhasivam (4):
      PCI/pwrctrl: Move creation of pwrctrl devices to pci_scan_device()
      PCI/pwrctrl: Move pci_pwrctrl_unregister() to pci_destroy_dev()
      dt-bindings: vendor-prefixes: Document the 'pciclass' prefix
      PCI/pwrctrl: Add pwrctrl driver for PCI Slots

 .../devicetree/bindings/vendor-prefixes.yaml       |  2 +-
 drivers/pci/bus.c                                  | 43 ----------
 drivers/pci/probe.c                                | 34 ++++++++
 drivers/pci/pwrctrl/Kconfig                        | 11 +++
 drivers/pci/pwrctrl/Makefile                       |  3 +
 drivers/pci/pwrctrl/core.c                         |  2 +-
 drivers/pci/pwrctrl/slot.c                         | 93 ++++++++++++++++++++++
 drivers/pci/remove.c                               |  2 +-
 8 files changed, 144 insertions(+), 46 deletions(-)
---
base-commit: 40384c840ea1944d7c5a392e8975ed088ecf0b37
change-id: 20241210-pci-pwrctrl-slot-02c0ec63172f

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

[PATCH 1/4] PCI/pwrctrl: Move creation of pwrctrl devices to pci_scan_device()

[Manivannan Sadhasivam via B4 Relay]

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

Current way of creating pwrctrl devices requires iterating through the
child devicetree nodes of the PCI bridge in pci_pwrctrl_create_devices().
Even though it works, it creates confusion as there is no symmetry between
this and pci_pwrctrl_unregister() function that removes the pwrctrl
devices.

So to make these two functions symmetric, move the creation of pwrctrl
devices to pci_scan_device(). During the scan of each device in a slot,
the devicetree node (if exists) for the PCI device will be checked. If it
has the supplies populated, then the pwrctrl device will be created.

Since the PCI device scan happens so early, there would be no 'struct
pci_dev' available for the device. So the host bridge is used as the parent
of all pwrctrl devices.

One nice side effect of this move is that, it is now possible to have
pwrctrl devices for PCI bridges as well (to control the supplies of PCI
slots).

Suggested-by: Lukas Wunner <lukas@wunner.de>
Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---
 drivers/pci/bus.c          | 43 -------------------------------------------
 drivers/pci/probe.c        | 34 ++++++++++++++++++++++++++++++++++
 drivers/pci/pwrctrl/core.c |  2 +-
 3 files changed, 35 insertions(+), 44 deletions(-)

diff --git a/drivers/pci/bus.c b/drivers/pci/bus.c
index 98910bc0fcc4..b6851101ac36 100644
--- a/drivers/pci/bus.c
+++ b/drivers/pci/bus.c
@@ -331,47 +331,6 @@ void __weak pcibios_resource_survey_bus(struct pci_bus *bus) { }
 
 void __weak pcibios_bus_add_device(struct pci_dev *pdev) { }
 
-/*
- * Create pwrctrl devices (if required) for the PCI devices to handle the power
- * state.
- */
-static void pci_pwrctrl_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 pwrctrl
-		 * devices for them. The pwrctrl device drivers will manage the
-		 * power state of the devices.
-		 */
-		for_each_available_child_of_node_scoped(np, child) {
-			/*
-			 * First check whether the pwrctrl 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 pwrctrl 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
@@ -396,8 +355,6 @@ void pci_bus_add_device(struct pci_dev *dev)
 	pci_proc_attach_device(dev);
 	pci_bridge_d3_update(dev);
 
-	pci_pwrctrl_create_devices(dev);
-
 	/*
 	 * If the PCI device is associated with a pwrctrl device with a
 	 * power supply, create a device link between the PCI device and
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 2e81ab0f5a25..db928d5cb753 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -9,6 +9,8 @@
 #include <linux/pci.h>
 #include <linux/msi.h>
 #include <linux/of_pci.h>
+#include <linux/of_platform.h>
+#include <linux/platform_device.h>
 #include <linux/pci_hotplug.h>
 #include <linux/slab.h>
 #include <linux/module.h>
@@ -2446,6 +2448,36 @@ bool pci_bus_read_dev_vendor_id(struct pci_bus *bus, int devfn, u32 *l,
 }
 EXPORT_SYMBOL(pci_bus_read_dev_vendor_id);
 
+/*
+ * Create pwrctrl devices (if required) for the PCI devices to handle the power
+ * state.
+ */
+static void pci_pwrctrl_create_devices(struct pci_bus *bus, int devfn)
+{
+	struct pci_host_bridge *host = pci_find_host_bridge(bus);
+	struct platform_device *pdev;
+	struct device_node *np;
+
+	np = of_pci_find_child_device(dev_of_node(&bus->dev), devfn);
+	if (!np || of_find_device_by_node(np))
+		return;
+
+	/*
+	 * First check whether the pwrctrl 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(np)) {
+		pr_debug("PCI/pwrctrl: Skipping OF node: %s\n", np->name);
+		return;
+	}
+
+	/* Now create the pwrctrl device */
+	pdev = of_platform_device_create(np, NULL, &host->dev);
+	if (!pdev)
+		pr_err("PCI/pwrctrl: Failed to create pwrctrl device for node: %s\n", np->name);
+}
+
 /*
  * Read the config data for a PCI device, sanity-check it,
  * and fill in the dev structure.
@@ -2455,6 +2487,8 @@ static struct pci_dev *pci_scan_device(struct pci_bus *bus, int devfn)
 	struct pci_dev *dev;
 	u32 l;
 
+	pci_pwrctrl_create_devices(bus, devfn);
+
 	if (!pci_bus_read_dev_vendor_id(bus, devfn, &l, 60*1000))
 		return NULL;
 
diff --git a/drivers/pci/pwrctrl/core.c b/drivers/pci/pwrctrl/core.c
index 2fb174db91e5..9cc7e2b7f2b5 100644
--- a/drivers/pci/pwrctrl/core.c
+++ b/drivers/pci/pwrctrl/core.c
@@ -44,7 +44,7 @@ static void rescan_work_func(struct work_struct *work)
 						   struct pci_pwrctrl, work);
 
 	pci_lock_rescan_remove();
-	pci_rescan_bus(to_pci_dev(pwrctrl->dev->parent)->bus);
+	pci_rescan_bus(to_pci_host_bridge(pwrctrl->dev->parent)->bus);
 	pci_unlock_rescan_remove();
 }
 

-- 
2.25.1

Re: [PATCH 1/4] PCI/pwrctrl: Move creation of pwrctrl devices to pci_scan_device()

[Lukas Wunner]

On Tue, Dec 10, 2024 at 03:25:24PM +0530, Manivannan Sadhasivam via B4 Relay wrote:
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -2446,6 +2448,36 @@ bool pci_bus_read_dev_vendor_id(struct pci_bus *bus, int devfn, u32 *l,
>  }
>  EXPORT_SYMBOL(pci_bus_read_dev_vendor_id);
>  
> +/*
> + * Create pwrctrl devices (if required) for the PCI devices to handle the power
> + * state.
> + */
> +static void pci_pwrctrl_create_devices(struct pci_bus *bus, int devfn)

Nit:  AFAICS this only creates a *single* platform device, so the
"devices" (plural) in the function name and in the code comment
doesn't seem to be accurate anymore.


> diff --git a/drivers/pci/pwrctrl/core.c b/drivers/pci/pwrctrl/core.c
> index 2fb174db91e5..9cc7e2b7f2b5 100644
> --- a/drivers/pci/pwrctrl/core.c
> +++ b/drivers/pci/pwrctrl/core.c
> @@ -44,7 +44,7 @@ static void rescan_work_func(struct work_struct *work)
>  						   struct pci_pwrctrl, work);
>  
>  	pci_lock_rescan_remove();
> -	pci_rescan_bus(to_pci_dev(pwrctrl->dev->parent)->bus);
> +	pci_rescan_bus(to_pci_host_bridge(pwrctrl->dev->parent)->bus);
>  	pci_unlock_rescan_remove();
>  }

Remind me, what's the purpose of this?  I'm guessing that it
recursively creates the platform devices below the newly
powered up device, is that correct?  If so, is it still
necessary?  Doesn't the new approach automatically create
those devices upon their enumeration?

Overall it looks like a significant improvement, thanks for doing this!

Lukas

Re: [PATCH 1/4] PCI/pwrctrl: Move creation of pwrctrl devices to pci_scan_device()

[Manivannan Sadhasivam]

On Sun, Dec 15, 2024 at 06:19:22PM +0100, Lukas Wunner wrote:
> On Tue, Dec 10, 2024 at 03:25:24PM +0530, Manivannan Sadhasivam via B4 Relay wrote:
> > --- a/drivers/pci/probe.c
> > +++ b/drivers/pci/probe.c
> > @@ -2446,6 +2448,36 @@ bool pci_bus_read_dev_vendor_id(struct pci_bus *bus, int devfn, u32 *l,
> >  }
> >  EXPORT_SYMBOL(pci_bus_read_dev_vendor_id);
> >  
> > +/*
> > + * Create pwrctrl devices (if required) for the PCI devices to handle the power
> > + * state.
> > + */
> > +static void pci_pwrctrl_create_devices(struct pci_bus *bus, int devfn)
> 
> Nit:  AFAICS this only creates a *single* platform device, so the
> "devices" (plural) in the function name and in the code comment
> doesn't seem to be accurate anymore.
> 

I missed it, thanks for pointing out.

> 
> > diff --git a/drivers/pci/pwrctrl/core.c b/drivers/pci/pwrctrl/core.c
> > index 2fb174db91e5..9cc7e2b7f2b5 100644
> > --- a/drivers/pci/pwrctrl/core.c
> > +++ b/drivers/pci/pwrctrl/core.c
> > @@ -44,7 +44,7 @@ static void rescan_work_func(struct work_struct *work)
> >  						   struct pci_pwrctrl, work);
> >  
> >  	pci_lock_rescan_remove();
> > -	pci_rescan_bus(to_pci_dev(pwrctrl->dev->parent)->bus);
> > +	pci_rescan_bus(to_pci_host_bridge(pwrctrl->dev->parent)->bus);
> >  	pci_unlock_rescan_remove();
> >  }
> 
> Remind me, what's the purpose of this?  I'm guessing that it
> recursively creates the platform devices below the newly
> powered up device, is that correct?  If so, is it still
> necessary?  Doesn't the new approach automatically create
> those devices upon their enumeration?
> 

If the pwrctrl driver is available at the time of platform device creation, this
is not needed. But if the driver is not available at that time and probed
later, then we need to rescan the bus to enumerate the devices. This is pretty
much needed for platforms lacking hotplug support (most of the DT ones).

> Overall it looks like a significant improvement, thanks for doing this!
> 

Thanks a lot for your inputs too!

- Mani

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

Re: [PATCH 1/4] PCI/pwrctrl: Move creation of pwrctrl devices to pci_scan_device()

[Lukas Wunner]

On Mon, Dec 16, 2024 at 10:45:21AM +0530, Manivannan Sadhasivam wrote:
> On Sun, Dec 15, 2024 at 06:19:22PM +0100, Lukas Wunner wrote:
> > On Tue, Dec 10, 2024 at 03:25:24PM +0530, Manivannan Sadhasivam wrote:
> > > diff --git a/drivers/pci/pwrctrl/core.c b/drivers/pci/pwrctrl/core.c
> > > index 2fb174db91e5..9cc7e2b7f2b5 100644
> > > --- a/drivers/pci/pwrctrl/core.c
> > > +++ b/drivers/pci/pwrctrl/core.c
> > > @@ -44,7 +44,7 @@ static void rescan_work_func(struct work_struct *work)
> > >  						   struct pci_pwrctrl, work);
> > >  
> > >  	pci_lock_rescan_remove();
> > > -	pci_rescan_bus(to_pci_dev(pwrctrl->dev->parent)->bus);
> > > +	pci_rescan_bus(to_pci_host_bridge(pwrctrl->dev->parent)->bus);
> > >  	pci_unlock_rescan_remove();
> > >  }
> > 
> > Remind me, what's the purpose of this?  I'm guessing that it
> > recursively creates the platform devices below the newly
> > powered up device, is that correct?  If so, is it still
> > necessary?  Doesn't the new approach automatically create
> > those devices upon their enumeration?
> 
> If the pwrctrl driver is available at the time of platform device creation,
> this is not needed. But if the driver is not available at that time and
> probed later, then we need to rescan the bus to enumerate the devices.

I see.  Sounds like this can be made conditional on the caller
being a module.  I think you could achieve this with the following
in pci_pwrctl_device_set_ready():

-	schedule_work(&pwrctl->work);
+	if (is_module_address(_RET_IP_))
+		schedule_work(&pwrctl->work);

Though you'd also have to declare pci_pwrctl_device_set_ready()
"__attribute__((always_inline))" so that it gets inlined into
devm_pci_pwrctl_device_set_ready() and the condition works there
as well.

I'm wondering whether the bus notifier is still necessary with
the new scheme.  Since the power control device is instantiated
and destroyed in unison with the pci_dev, can't the device link
always be created on instantiation of the power control device?

Thanks,

Lukas

Re: [PATCH 1/4] PCI/pwrctrl: Move creation of pwrctrl devices to pci_scan_device()

[Manivannan Sadhasivam]

On Tue, Dec 17, 2024 at 02:14:34PM +0100, Lukas Wunner wrote:
> On Mon, Dec 16, 2024 at 10:45:21AM +0530, Manivannan Sadhasivam wrote:
> > On Sun, Dec 15, 2024 at 06:19:22PM +0100, Lukas Wunner wrote:
> > > On Tue, Dec 10, 2024 at 03:25:24PM +0530, Manivannan Sadhasivam wrote:
> > > > diff --git a/drivers/pci/pwrctrl/core.c b/drivers/pci/pwrctrl/core.c
> > > > index 2fb174db91e5..9cc7e2b7f2b5 100644
> > > > --- a/drivers/pci/pwrctrl/core.c
> > > > +++ b/drivers/pci/pwrctrl/core.c
> > > > @@ -44,7 +44,7 @@ static void rescan_work_func(struct work_struct *work)
> > > >  						   struct pci_pwrctrl, work);
> > > >  
> > > >  	pci_lock_rescan_remove();
> > > > -	pci_rescan_bus(to_pci_dev(pwrctrl->dev->parent)->bus);
> > > > +	pci_rescan_bus(to_pci_host_bridge(pwrctrl->dev->parent)->bus);
> > > >  	pci_unlock_rescan_remove();
> > > >  }
> > > 
> > > Remind me, what's the purpose of this?  I'm guessing that it
> > > recursively creates the platform devices below the newly
> > > powered up device, is that correct?  If so, is it still
> > > necessary?  Doesn't the new approach automatically create
> > > those devices upon their enumeration?
> > 
> > If the pwrctrl driver is available at the time of platform device creation,
> > this is not needed. But if the driver is not available at that time and
> > probed later, then we need to rescan the bus to enumerate the devices.
> 
> I see.  Sounds like this can be made conditional on the caller
> being a module.  I think you could achieve this with the following
> in pci_pwrctl_device_set_ready():
> 
> -	schedule_work(&pwrctl->work);
> +	if (is_module_address(_RET_IP_))
> +		schedule_work(&pwrctl->work);
> 
> Though you'd also have to declare pci_pwrctl_device_set_ready()
> "__attribute__((always_inline))" so that it gets inlined into
> devm_pci_pwrctl_device_set_ready() and the condition works there
> as well.
> 

I'd prefer to skip the rescan if the pwrctrl device is created and let the
pci_pwrctrl_device_set_ready() initiate rescan once the device is powered on.
This way, we could avoid scanning for the device twice.

> I'm wondering whether the bus notifier is still necessary with
> the new scheme.  Since the power control device is instantiated
> and destroyed in unison with the pci_dev, can't the device link
> always be created on instantiation of the power control device?
> 

I did move the devlink handling out of bus notifier callback with commit,
b458ff7e8176 ("PCI/pwrctl: Ensure that pwrctl drivers are probed before PCI
client drivers").

The bus notifier is only used to set 'of_node_reused' flag to indicate that the
corresponding DT node is already used.

- Mani

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

[PATCH 2/4] PCI/pwrctrl: Move pci_pwrctrl_unregister() to pci_destroy_dev()

[Manivannan Sadhasivam via B4 Relay]

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

PCI core will try to access the devices even after pci_stop_dev() for
things like Data Object Exchange (DOE), ASPM etc... So move
pci_pwrctrl_unregister() to the near end of pci_destroy_dev() to make sure
that the devices are powered down only after the PCI core is done with
them.

Suggested-by: Lukas Wunner <lukas@wunner.de>
Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---
 drivers/pci/remove.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/pci/remove.c b/drivers/pci/remove.c
index efc37fcb73e2..58859f9d92f7 100644
--- a/drivers/pci/remove.c
+++ b/drivers/pci/remove.c
@@ -41,7 +41,6 @@ static void pci_stop_dev(struct pci_dev *dev)
 	if (!pci_dev_test_and_clear_added(dev))
 		return;
 
-	pci_pwrctrl_unregister(&dev->dev);
 	device_release_driver(&dev->dev);
 	pci_proc_detach_device(dev);
 	pci_remove_sysfs_dev_files(dev);
@@ -64,6 +63,7 @@ static void pci_destroy_dev(struct pci_dev *dev)
 	pci_doe_destroy(dev);
 	pcie_aspm_exit_link_state(dev);
 	pci_bridge_d3_update(dev);
+	pci_pwrctrl_unregister(&dev->dev);
 	pci_free_resources(dev);
 	put_device(&dev->dev);
 }

-- 
2.25.1

Re: [PATCH 2/4] PCI/pwrctrl: Move pci_pwrctrl_unregister() to pci_destroy_dev()

[Lukas Wunner]

On Tue, Dec 10, 2024 at 03:25:25PM +0530, Manivannan Sadhasivam via B4 Relay wrote:
> PCI core will try to access the devices even after pci_stop_dev() for
> things like Data Object Exchange (DOE), ASPM etc... So move
> pci_pwrctrl_unregister() to the near end of pci_destroy_dev() to make sure
> that the devices are powered down only after the PCI core is done with
> them.
> 
> Suggested-by: Lukas Wunner <lukas@wunner.de>
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

Reviewed-by: Lukas Wunner <lukas@wunner.de>

[PATCH 3/4] dt-bindings: vendor-prefixes: Document the 'pciclass' prefix

[Manivannan Sadhasivam via B4 Relay]

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

'pciclass' is an existing prefix used to identify the PCI bridge devices,
but it is not a vendor prefix. So document it in the non-vendor prefix
list.

Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---
 Documentation/devicetree/bindings/vendor-prefixes.yaml | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/vendor-prefixes.yaml b/Documentation/devicetree/bindings/vendor-prefixes.yaml
index da01616802c7..0539aea6af5a 100644
--- a/Documentation/devicetree/bindings/vendor-prefixes.yaml
+++ b/Documentation/devicetree/bindings/vendor-prefixes.yaml
@@ -18,7 +18,7 @@ patternProperties:
   # DO NOT ADD NEW PROPERTIES TO THIS LIST
   "^(at25|bm|devbus|dmacap|dsa|exynos|fsi[ab]|gpio-fan|gpio-key|gpio|gpmc|hdmi|i2c-gpio),.*": true
   "^(keypad|m25p|max8952|max8997|max8998|mpmc),.*": true
-  "^(pinctrl-single|#pinctrl-single|PowerPC),.*": true
+  "^(pciclass|pinctrl-single|#pinctrl-single|PowerPC),.*": true
   "^(pl022|pxa-mmc|rcar_sound|rotary-encoder|s5m8767|sdhci),.*": true
   "^(simple-audio-card|st-plgpio|st-spics|ts),.*": true
 

-- 
2.25.1

Re: [PATCH 3/4] dt-bindings: vendor-prefixes: Document the 'pciclass' prefix

[Rob Herring (Arm)]

On Tue, 10 Dec 2024 15:25:26 +0530, Manivannan Sadhasivam wrote:
> 'pciclass' is an existing prefix used to identify the PCI bridge devices,
> but it is not a vendor prefix. So document it in the non-vendor prefix
> list.
> 
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> ---
>  Documentation/devicetree/bindings/vendor-prefixes.yaml | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 

Acked-by: Rob Herring (Arm) <robh@kernel.org>

[PATCH 4/4] PCI/pwrctrl: Add pwrctrl driver for PCI Slots

[Manivannan Sadhasivam via B4 Relay]

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

This driver is used to control the power state of the devices attached to
the PCI slots. Currently, it controls the voltage rails of the PCI slots
defined in the devicetree node of the root port.

The voltage rails for PCI slots are documented in the dt-schema:
https://github.com/devicetree-org/dt-schema/blob/v2024.11/dtschema/schemas/pci/pci-bus-common.yaml#L153

Since this driver has to work with different kind of slots (x{1/4/8/16}
PCIe, Mini PCIe, PCI etc...), the driver is using the
of_regulator_bulk_get_all() API to obtain the voltage regulators defined
in the DT node, instead of hardcoding them. The DT node of the root port
should define the relevant supply properties corresponding to the voltage
rails of the PCI slot.

Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---
 drivers/pci/pwrctrl/Kconfig  | 11 ++++++
 drivers/pci/pwrctrl/Makefile |  3 ++
 drivers/pci/pwrctrl/slot.c   | 93 ++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 107 insertions(+)

diff --git a/drivers/pci/pwrctrl/Kconfig b/drivers/pci/pwrctrl/Kconfig
index 54589bb2403b..990cab67d413 100644
--- a/drivers/pci/pwrctrl/Kconfig
+++ b/drivers/pci/pwrctrl/Kconfig
@@ -10,3 +10,14 @@ config PCI_PWRCTL_PWRSEQ
 	tristate
 	select POWER_SEQUENCING
 	select PCI_PWRCTL
+
+config PCI_PWRCTL_SLOT
+	tristate "PCI Power Control driver for PCI slots"
+	select PCI_PWRCTL
+	help
+	  Say Y here to enable the PCI Power Control driver to control the power
+	  state of PCI slots.
+
+	  This is a generic driver that controls the power state of different
+	  PCI slots. The voltage regulators powering the rails of the PCI slots
+	  are expected to be defined in the devicetree node of the PCI bridge.
diff --git a/drivers/pci/pwrctrl/Makefile b/drivers/pci/pwrctrl/Makefile
index 75c7ce531c7e..ddfb12c5aadf 100644
--- a/drivers/pci/pwrctrl/Makefile
+++ b/drivers/pci/pwrctrl/Makefile
@@ -4,3 +4,6 @@ obj-$(CONFIG_PCI_PWRCTL)		+= pci-pwrctrl-core.o
 pci-pwrctrl-core-y			:= core.o
 
 obj-$(CONFIG_PCI_PWRCTL_PWRSEQ)		+= pci-pwrctrl-pwrseq.o
+
+obj-$(CONFIG_PCI_PWRCTL_SLOT)		+= pci-pwrctl-slot.o
+pci-pwrctl-slot-y			:= slot.o
diff --git a/drivers/pci/pwrctrl/slot.c b/drivers/pci/pwrctrl/slot.c
new file mode 100644
index 000000000000..18becc144913
--- /dev/null
+++ b/drivers/pci/pwrctrl/slot.c
@@ -0,0 +1,93 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (C) 2024 Linaro Ltd.
+ * Author: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
+ */
+
+#include <linux/device.h>
+#include <linux/mod_devicetable.h>
+#include <linux/module.h>
+#include <linux/pci-pwrctrl.h>
+#include <linux/platform_device.h>
+#include <linux/regulator/consumer.h>
+#include <linux/slab.h>
+
+struct pci_pwrctrl_slot_data {
+	struct pci_pwrctrl ctx;
+	struct regulator_bulk_data *supplies;
+	int num_supplies;
+};
+
+static void devm_pci_pwrctrl_slot_power_off(void *data)
+{
+	struct pci_pwrctrl_slot_data *slot = data;
+
+	regulator_bulk_disable(slot->num_supplies, slot->supplies);
+	regulator_bulk_free(slot->num_supplies, slot->supplies);
+}
+
+static int pci_pwrctrl_slot_probe(struct platform_device *pdev)
+{
+	struct pci_pwrctrl_slot_data *slot;
+	struct device *dev = &pdev->dev;
+	int ret;
+
+	slot = devm_kzalloc(dev, sizeof(*slot), GFP_KERNEL);
+	if (!slot)
+		return -ENOMEM;
+
+	ret = of_regulator_bulk_get_all(dev, dev_of_node(dev),
+					&slot->supplies);
+	if (ret < 0) {
+		dev_err_probe(dev, ret, "Failed to get slot regulators\n");
+		return ret;
+	}
+
+	slot->num_supplies = ret;
+	ret = regulator_bulk_enable(slot->num_supplies, slot->supplies);
+	if (ret < 0) {
+		dev_err_probe(dev, ret, "Failed to enable slot regulators\n");
+		goto err_regulator_free;
+	}
+
+	ret = devm_add_action_or_reset(dev, devm_pci_pwrctrl_slot_power_off,
+				       slot);
+	if (ret)
+		goto err_regulator_disable;
+
+	pci_pwrctrl_init(&slot->ctx, dev);
+
+	ret = devm_pci_pwrctrl_device_set_ready(dev, &slot->ctx);
+	if (ret)
+		return dev_err_probe(dev, ret, "Failed to register pwrctrl driver\n");
+
+	return 0;
+
+err_regulator_disable:
+	regulator_bulk_disable(slot->num_supplies, slot->supplies);
+err_regulator_free:
+	regulator_bulk_free(slot->num_supplies, slot->supplies);
+
+	return ret;
+}
+
+static const struct of_device_id pci_pwrctrl_slot_of_match[] = {
+	{
+		.compatible = "pciclass,0604",
+	},
+	{ }
+};
+MODULE_DEVICE_TABLE(of, pci_pwrctrl_slot_of_match);
+
+static struct platform_driver pci_pwrctrl_slot_driver = {
+	.driver = {
+		.name = "pci-pwrctrl-slot",
+		.of_match_table = pci_pwrctrl_slot_of_match,
+	},
+	.probe = pci_pwrctrl_slot_probe,
+};
+module_platform_driver(pci_pwrctrl_slot_driver);
+
+MODULE_AUTHOR("Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>");
+MODULE_DESCRIPTION("Generic PCI Power Control driver for PCI Slots");
+MODULE_LICENSE("GPL");

-- 
2.25.1

Re: [PATCH 4/4] PCI/pwrctrl: Add pwrctrl driver for PCI Slots

[kernel test robot]

Hi Manivannan,

kernel test robot noticed the following build errors:

[auto build test ERROR on 40384c840ea1944d7c5a392e8975ed088ecf0b37]

url:    https://github.com/intel-lab-lkp/linux/commits/Manivannan-Sadhasivam-via-B4-Relay/PCI-pwrctrl-Move-creation-of-pwrctrl-devices-to-pci_scan_device/20241210-180256
base:   40384c840ea1944d7c5a392e8975ed088ecf0b37
patch link:    https://lore.kernel.org/r/20241210-pci-pwrctrl-slot-v1-4-eae45e488040%40linaro.org
patch subject: [PATCH 4/4] PCI/pwrctrl: Add pwrctrl driver for PCI Slots
config: x86_64-randconfig-004-20241218 (https://download.01.org/0day-ci/archive/20241218/202412181640.12Iufkvd-lkp@intel.com/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241218/202412181640.12Iufkvd-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202412181640.12Iufkvd-lkp@intel.com/

All errors (new ones prefixed by >>, old ones prefixed by <<):

WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/fpga/tests/fpga-mgr-test.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/fpga/tests/fpga-bridge-test.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/fpga/tests/fpga-region-test.o
>> ERROR: modpost: "of_regulator_bulk_get_all" [drivers/pci/pwrctrl/pci-pwrctl-slot.ko] undefined!

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH 0/4] PCI/pwrctrl: Rework pwrctrl driver integration and add driver for PCI slot

[Bartosz Golaszewski]

On Tue, Dec 10, 2024 at 10:55 AM Manivannan Sadhasivam via B4 Relay
<devnull+manivannan.sadhasivam.linaro.org@kernel.org> wrote:
>
> Hi,
>
> This series reworks the PCI pwrctrl integration (again) by moving the creation
> and removal of pwrctrl devices to pci_scan_device() and pci_destroy_dev() APIs.
> This is based on the suggestion provided by Lukas Wunner [1][2]. With this
> change, it is now possible to create pwrctrl devices for PCI bridges as well.
> This is required to control the power state of the PCI slots in a system. Since
> the PCI slots are not explicitly defined in devicetree, the agreement is to
> define the supplies for PCI slots in PCI bridge nodes itself [3].
>
> Based on this, a pwrctrl driver to control the supplies of PCI slots are also
> added in patch 4. With this driver, it is now possible to control the voltage
> regulators powering the PCI slots defined in PCI bridge nodes as below:
>
> ```
> pcie@0 {
>         compatible "pciclass,0604"
>         ...
>
>         vpcie12v-supply = <&vpcie12v_reg>;
>         vpcie3v3-supply = <&vpcie3v3_reg>;
>         vpcie3v3aux-supply = <&vpcie3v3aux_reg>;
> };
> ```
>
> To make use of this driver, the PCI bridge DT node should also have the
> compatible "pciclass,0604". But adding this compatible triggers the following
> checkpatch warning:
>
> WARNING: DT compatible string vendor "pciclass" appears un-documented --
> check ./Documentation/devicetree/bindings/vendor-prefixes.yaml
>
> For fixing it, I added patch 3. But due to some reason, checkpatch is not
> picking the 'pciclass' vendor prefix alone, and requires adding the full
> compatible 'pciclass,0604' in the vendor-prefixes list. Since my perl skills are
> not great, I'm leaving it in the hands of Rob to fix the checkpatch script.
>
> [1] https://lore.kernel.org/linux-pci/Z0yLDBMAsh0yKWf2@wunner.de
> [2] https://lore.kernel.org/linux-pci/Z0xAdQ2ozspEnV5g@wunner.de
> [3] https://github.com/devicetree-org/dt-schema/issues/145
>
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> ---
> Manivannan Sadhasivam (4):
>       PCI/pwrctrl: Move creation of pwrctrl devices to pci_scan_device()
>       PCI/pwrctrl: Move pci_pwrctrl_unregister() to pci_destroy_dev()
>       dt-bindings: vendor-prefixes: Document the 'pciclass' prefix
>       PCI/pwrctrl: Add pwrctrl driver for PCI Slots
>
>  .../devicetree/bindings/vendor-prefixes.yaml       |  2 +-
>  drivers/pci/bus.c                                  | 43 ----------
>  drivers/pci/probe.c                                | 34 ++++++++
>  drivers/pci/pwrctrl/Kconfig                        | 11 +++
>  drivers/pci/pwrctrl/Makefile                       |  3 +
>  drivers/pci/pwrctrl/core.c                         |  2 +-
>  drivers/pci/pwrctrl/slot.c                         | 93 ++++++++++++++++++++++
>  drivers/pci/remove.c                               |  2 +-
>  8 files changed, 144 insertions(+), 46 deletions(-)
> ---
> base-commit: 40384c840ea1944d7c5a392e8975ed088ecf0b37
> change-id: 20241210-pci-pwrctrl-slot-02c0ec63172f
>
> Best regards,
> --
> Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
>
>

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

Re: [PATCH 0/4] PCI/pwrctrl: Rework pwrctrl driver integration and add driver for PCI slot

[Qiang Yu]

On 12/10/2024 5:55 PM, Manivannan Sadhasivam via B4 Relay wrote:
> Hi,
>
> This series reworks the PCI pwrctrl integration (again) by moving the creation
> and removal of pwrctrl devices to pci_scan_device() and pci_destroy_dev() APIs.
> This is based on the suggestion provided by Lukas Wunner [1][2]. With this
> change, it is now possible to create pwrctrl devices for PCI bridges as well.
> This is required to control the power state of the PCI slots in a system. Since
> the PCI slots are not explicitly defined in devicetree, the agreement is to
> define the supplies for PCI slots in PCI bridge nodes itself [3].
>
> Based on this, a pwrctrl driver to control the supplies of PCI slots are also
> added in patch 4. With this driver, it is now possible to control the voltage
> regulators powering the PCI slots defined in PCI bridge nodes as below:
>
> ```
> pcie@0 {
> 	compatible "pciclass,0604"
> 	...
>
> 	vpcie12v-supply = <&vpcie12v_reg>;
> 	vpcie3v3-supply = <&vpcie3v3_reg>;
> 	vpcie3v3aux-supply = <&vpcie3v3aux_reg>;
> };
> ```
>
> To make use of this driver, the PCI bridge DT node should also have the
> compatible "pciclass,0604". But adding this compatible triggers the following
> checkpatch warning:
>
> WARNING: DT compatible string vendor "pciclass" appears un-documented --
> check ./Documentation/devicetree/bindings/vendor-prefixes.yaml
>
> For fixing it, I added patch 3. But due to some reason, checkpatch is not
> picking the 'pciclass' vendor prefix alone, and requires adding the full
> compatible 'pciclass,0604' in the vendor-prefixes list. Since my perl skills are
> not great, I'm leaving it in the hands of Rob to fix the checkpatch script.
>
> [1] https://lore.kernel.org/linux-pci/Z0yLDBMAsh0yKWf2@wunner.de
> [2] https://lore.kernel.org/linux-pci/Z0xAdQ2ozspEnV5g@wunner.de
> [3] https://github.com/devicetree-org/dt-schema/issues/145
>
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> ---
> Manivannan Sadhasivam (4):
>        PCI/pwrctrl: Move creation of pwrctrl devices to pci_scan_device()
>        PCI/pwrctrl: Move pci_pwrctrl_unregister() to pci_destroy_dev()
>        dt-bindings: vendor-prefixes: Document the 'pciclass' prefix
>        PCI/pwrctrl: Add pwrctrl driver for PCI Slots
>
>   .../devicetree/bindings/vendor-prefixes.yaml       |  2 +-
>   drivers/pci/bus.c                                  | 43 ----------
>   drivers/pci/probe.c                                | 34 ++++++++
>   drivers/pci/pwrctrl/Kconfig                        | 11 +++
>   drivers/pci/pwrctrl/Makefile                       |  3 +
>   drivers/pci/pwrctrl/core.c                         |  2 +-
>   drivers/pci/pwrctrl/slot.c                         | 93 ++++++++++++++++++++++
>   drivers/pci/remove.c                               |  2 +-
>   8 files changed, 144 insertions(+), 46 deletions(-)
> ---
> base-commit: 40384c840ea1944d7c5a392e8975ed088ecf0b37
> change-id: 20241210-pci-pwrctrl-slot-02c0ec63172f
>
> Best regards,

Hi Mani

PCIe3 is able to link up after applying your patch. Slot power is turned 
on correctly.
But see “NULL pointer dereference” when I try to remove device.

root@linaro-gnome:/sys/bus/pci/devices/0003:00:00.0# echo 1 > remove

[   38.752976] ------------[ cut here ]------------
[   38.757726] WARNING: CPU: 1 PID: 816 at drivers/regulator/core.c:5857 
regulator_unregister+0x13c/0x160
[   38.767288] Modules linked in: phy_qcom_qmp_combo aux_bridge 
drm_kms_helper drm nvme backlight pinctrl_sm8550_lpass_lpi 
pci_pwrctl_slot pci_pwrctrl_core nvme_core phy_qcom_edp 
phy_qcom_eusb2_repeater dispcc_x1e80100 pinctrl_lpass_lpi 
phy_qcom_snps_eusb2 lpasscc_sc8280xp typec gpucc_x1e80100 phy_qcom_qmp_pcie
[   38.795279] CPU: 1 UID: 0 PID: 816 Comm: bash Not tainted 
6.12.0-next-20241128-00005-g6178bf6ce3c2-dirty #50
[   38.805359] Hardware name: Qualcomm IDP, BIOS 
6.0.240607.BOOT.MXF.2.4-00348.1-HAMOA-1.67705.7 06/ 7/2024
[   38.815088] pstate: 61400005 (nZCv daif +PAN -UAO -TCO +DIT -SSBS 
BTYPE=--)
[   38.822239] pc : regulator_unregister+0x13c/0x160
[   38.827081] lr : regulator_unregister+0xc0/0x160
[   38.831829] sp : ffff800082ad39e0
[   38.835238] x29: ffff800082ad39e0 x28: ffff67874a4b1140 x27: 
0000000000000000
[   38.842563] x26: 0000000000000000 x25: 0000000000000000 x24: 
0000000000000000
[   38.849895] x23: 0000000000000001 x22: ffff800082ad3a88 x21: 
0000000000000000
[   38.857220] x20: ffffb72b1c7de2b0 x19: ffff678740f60000 x18: 
ffffffffffffffff
[   38.864545] x17: 2e30303030646231 x16: ffffb72b1a68d54c x15: 
3030303064424337
[   38.871867] x14: 000000000000000b x13: ffff6787402b6010 x12: 
0000000000000000
[   38.879200] x11: 0000000000000000 x10: ffffb72b1a689fe8 x9 : 
ffffb72b1a689bc8
[   38.886525] x8 : ffff678740e5b2c0 x7 : ffff800082ad3a88 x6 : 
ffff678740e5b2c0
[   38.893849] x5 : ffff678740e5b2c0 x4 : ffff678740e5b2c0 x3 : 
ffff678740e5b2c0
[   38.901172] x2 : ffff67874a4b1140 x1 : 0000000000000000 x0 : 
0000000000000007
[   38.908496] Call trace:
[   38.911019]  regulator_unregister+0x13c/0x160 (P)
[   38.915856]  regulator_unregister+0xc0/0x160 (L)
[   38.920600]  devm_rdev_release+0x14/0x20
[   38.924634]  devres_release_all+0xa0/0x100
[   38.928845]  device_unbind_cleanup+0x18/0x60
[   38.933239]  device_release_driver_internal+0x1ec/0x228
[   38.938606]  device_release_driver+0x18/0x24
[   38.942998]  bus_remove_device+0xcc/0x10c
[   38.947122]  device_del+0x14c/0x404
[   38.950705]  device_unregister+0x18/0x34
[   38.954738]  of_device_unregister+0x14/0x20
[   38.959041]  pci_remove_bus_device+0x110/0x1b0
[   38.963612]  pci_remove_bus_device+0x38/0x1b0
[   38.968094]  pci_stop_and_remove_bus_device_locked+0x28/0x3c
[   38.973907]  remove_store+0x94/0xa4
[   38.977494]  dev_attr_store+0x18/0x2c
[   38.981263]  sysfs_kf_write+0x44/0x54
[   38.985037]  kernfs_fop_write_iter+0x118/0x1a8
[   38.989607]  vfs_write+0x2b0/0x35c
[   38.993107]  ksys_write+0x68/0xfc
[   38.996519]  __arm64_sys_write+0x1c/0x28
[   39.000552]  invoke_syscall+0x48/0x110
[   39.004411]  el0_svc_common.constprop.0+0x40/0xe8
[   39.009244]  do_el0_svc+0x20/0x2c
[   39.012655]  el0_svc+0x30/0xd0
[   39.015805]  el0t_64_sync_handler+0x144/0x168
[   39.020283]  el0t_64_sync+0x198/0x19c
[   39.024052] ---[ end trace 0000000000000000 ]---
[   39.028897] Unable to handle kernel NULL pointer dereference at 
virtual address 00000000000000c0
[   39.037932] Mem abort info:
[   39.040823]   ESR = 0x0000000096000004
[   39.044691]   EC = 0x25: DABT (current EL), IL = 32 bits
[   39.050161]   SET = 0, FnV = 0
[   39.053316]   EA = 0, S1PTW = 0
[   39.056557]   FSC = 0x04: level 0 translation fault
[   39.061585] Data abort info:
[   39.064554]   ISV = 0, ISS = 0x00000004, ISS2 = 0x00000000
[   39.070190]   CM = 0, WnR = 0, TnD = 0, TagAccess = 0
[   39.075395]   GCS = 0, Overlay = 0, DirtyBit = 0, Xs = 0
[   39.080861] user pgtable: 4k pages, 48-bit VAs, pgdp=0000000889f93000
[   39.087484] [00000000000000c0] pgd=0000000000000000, p4d=0000000000000000
[   39.094467] Internal error: Oops: 0000000096000004 [#1] PREEMPT SMP
[   39.100903] Modules linked in: phy_qcom_qmp_combo aux_bridge 
drm_kms_helper drm nvme backlight pinctrl_sm8550_lpass_lpi 
pci_pwrctl_slot pci_pwrctrl_core nvme_core phy_qcom_edp 
phy_qcom_eusb2_repeater dispcc_x1e80100 pinctrl_lpass_lpi 
phy_qcom_snps_eusb2 lpasscc_sc8280xp typec gpucc_x1e80100 phy_qcom_qmp_pcie
[   39.128882] CPU: 1 UID: 0 PID: 816 Comm: bash Tainted: G W          
6.12.0-next-20241128-00005-g6178bf6ce3c2-dirty #50
[   39.140480] Tainted: [W]=WARN
[   39.143540] Hardware name: Qualcomm IDP, BIOS 
6.0.240607.BOOT.MXF.2.4-00348.1-HAMOA-1.67705.7 06/ 7/2024
[   39.153273] pstate: 21400005 (nzCv daif +PAN -UAO -TCO +DIT -SSBS 
BTYPE=--)
[   39.160421] pc : pci_remove_bus_device+0x120/0x1b0
[   39.165341] lr : pci_remove_bus_device+0x110/0x1b0
[   39.170261] sp : ffff800082ad3c00
[   39.173676] x29: ffff800082ad3c00 x28: ffff67874a4b1140 x27: 
0000000000000000
[   39.181004] x26: 0000000000000000 x25: 0000000000000000 x24: 
ffff67874cb2caa0
[   39.188334] x23: ffff800082ad3d88 x22: 0000000000000000 x21: 
ffff6787458f00c8
[   39.195661] x20: ffff6787458f0000 x19: ffffb72b1c5e88d8 x18: 
ffffffffffffffff
[   39.202984] x17: 74616c703d4d4554 x16: 5359534255530079 x15: 
6d6d75642d676572
[   39.210311] x14: ffffb72b1ca01098 x13: 0000000000000040 x12: 
0000000000000228
[   39.217638] x11: 0000000000000000 x10: 0000000000000000 x9 : 
0000000000000000
[   39.224964] x8 : 0000000000000000 x7 : ffff678740d3ebc8 x6 : 
ffff678745584b40
[   39.232296] x5 : ffff6787411c64e0 x4 : ffff6787411c64e0 x3 : 
ffff678741256679
[   39.239626] x2 : ffff678740e5b048 x1 : 0000000000000008 x0 : 
00000000000000c0
[   39.246951] Call trace:
[   39.249469]  pci_remove_bus_device+0x120/0x1b0 (P)
[   39.254399]  pci_remove_bus_device+0x110/0x1b0 (L)
[   39.259326]  pci_remove_bus_device+0x38/0x1b0
[   39.263801]  pci_stop_and_remove_bus_device_locked+0x28/0x3c
[   39.269620]  remove_store+0x94/0xa4
[   39.273209]  dev_attr_store+0x18/0x2c
[   39.276972]  sysfs_kf_write+0x44/0x54
[   39.280735]  kernfs_fop_write_iter+0x118/0x1a8
[   39.285305]  vfs_write+0x2b0/0x35c
[   39.288808]  ksys_write+0x68/0xfc
[   39.292221]  __arm64_sys_write+0x1c/0x28
[   39.296258]  invoke_syscall+0x48/0x110
[   39.300119]  el0_svc_common.constprop.0+0x40/0xe8
[   39.304952]  do_el0_svc+0x20/0x2c
[   39.308365]  el0_svc+0x30/0xd0
[   39.311515]  el0t_64_sync_handler+0x144/0x168
[   39.316002]  el0t_64_sync+0x198/0x19c
[   39.319766] Code: f9414aa0 d503201f d2800101 91030000 (f821101f)
[   39.326029] ---[ end trace 0000000000000000 ]---

Thanks,
Qiang

Re: [PATCH 0/4] PCI/pwrctrl: Rework pwrctrl driver integration and add driver for PCI slot

[Lukas Wunner]

On Wed, Dec 11, 2024 at 05:55:48PM +0800, Qiang Yu wrote:
> PCIe3 is able to link up after applying your patch. Slot power is turned on
> correctly.
> But see "NULL pointer dereference" when I try to remove device.

There's a WARN splat occurring before the NULL pointer deref.
Was this happening before or is it new?  Probably makes sense
to debug that first before looking into the NULL pointer deref,
which could be a result of it.


> [   38.757726] WARNING: CPU: 1 PID: 816 at drivers/regulator/core.c:5857
> regulator_unregister+0x13c/0x160
> [   38.767288] Modules linked in: phy_qcom_qmp_combo aux_bridge
> drm_kms_helper drm nvme backlight pinctrl_sm8550_lpass_lpi pci_pwrctl_slot
> pci_pwrctrl_core nvme_core phy_qcom_edp phy_qcom_eusb2_repeater
> dispcc_x1e80100 pinctrl_lpass_lpi phy_qcom_snps_eusb2 lpasscc_sc8280xp typec
> gpucc_x1e80100 phy_qcom_qmp_pcie
> [   38.795279] CPU: 1 UID: 0 PID: 816 Comm: bash Not tainted
> 6.12.0-next-20241128-00005-g6178bf6ce3c2-dirty #50
> [   38.805359] Hardware name: Qualcomm IDP, BIOS
> 6.0.240607.BOOT.MXF.2.4-00348.1-HAMOA-1.67705.7 06/ 7/2024
> [   38.815088] pstate: 61400005 (nZCv daif +PAN -UAO -TCO +DIT -SSBS
> BTYPE=--)
> [   38.822239] pc : regulator_unregister+0x13c/0x160
> [   38.827081] lr : regulator_unregister+0xc0/0x160

The WARN splat seems to be caused by:

	WARN_ON(rdev->open_count);

So the regulator is unregistered although it's still in use.
Is there maybe a multifunction PCIe device in your system
so that multiple devices are using the same regulator?

Thanks,

Lukas

Re: [PATCH 0/4] PCI/pwrctrl: Rework pwrctrl driver integration and add driver for PCI slot

[Manivannan Sadhasivam]

On Sun, Dec 15, 2024 at 06:32:02PM +0100, Lukas Wunner wrote:
> On Wed, Dec 11, 2024 at 05:55:48PM +0800, Qiang Yu wrote:
> > PCIe3 is able to link up after applying your patch. Slot power is turned on
> > correctly.
> > But see "NULL pointer dereference" when I try to remove device.
> 
> There's a WARN splat occurring before the NULL pointer deref.
> Was this happening before or is it new?  Probably makes sense
> to debug that first before looking into the NULL pointer deref,
> which could be a result of it.
> 

Precisely.

> 
> > [   38.757726] WARNING: CPU: 1 PID: 816 at drivers/regulator/core.c:5857
> > regulator_unregister+0x13c/0x160
> > [   38.767288] Modules linked in: phy_qcom_qmp_combo aux_bridge
> > drm_kms_helper drm nvme backlight pinctrl_sm8550_lpass_lpi pci_pwrctl_slot
> > pci_pwrctrl_core nvme_core phy_qcom_edp phy_qcom_eusb2_repeater
> > dispcc_x1e80100 pinctrl_lpass_lpi phy_qcom_snps_eusb2 lpasscc_sc8280xp typec
> > gpucc_x1e80100 phy_qcom_qmp_pcie
> > [   38.795279] CPU: 1 UID: 0 PID: 816 Comm: bash Not tainted
> > 6.12.0-next-20241128-00005-g6178bf6ce3c2-dirty #50
> > [   38.805359] Hardware name: Qualcomm IDP, BIOS
> > 6.0.240607.BOOT.MXF.2.4-00348.1-HAMOA-1.67705.7 06/ 7/2024
> > [   38.815088] pstate: 61400005 (nZCv daif +PAN -UAO -TCO +DIT -SSBS
> > BTYPE=--)
> > [   38.822239] pc : regulator_unregister+0x13c/0x160
> > [   38.827081] lr : regulator_unregister+0xc0/0x160
> 
> The WARN splat seems to be caused by:
> 
> 	WARN_ON(rdev->open_count);
> 
> So the regulator is unregistered although it's still in use.
> Is there maybe a multifunction PCIe device in your system
> so that multiple devices are using the same regulator?
> 

Maybe the regulator is shared with other peripherals (not just PCIe) in the
system.

@Qiang: I referred your patch [1] that added the slot regulators, but they were
not used by any peripherals other than PCIe. Could you please post the list of
consumers of the 3 slot regulators?

FYI, I did test root port remove on RB5 board (with dummy fixed regulators) and
it worked fine.

- Mani

[1] https://lore.kernel.org/linux-pci/20240827063631.3932971-8-quic_qianyu@quicinc.com/

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

Re: [PATCH 0/4] PCI/pwrctrl: Rework pwrctrl driver integration and add driver for PCI slot

[Manivannan Sadhasivam]

On Mon, Dec 16, 2024 at 10:51:18AM +0530, Manivannan Sadhasivam wrote:
> On Sun, Dec 15, 2024 at 06:32:02PM +0100, Lukas Wunner wrote:
> > On Wed, Dec 11, 2024 at 05:55:48PM +0800, Qiang Yu wrote:
> > > PCIe3 is able to link up after applying your patch. Slot power is turned on
> > > correctly.
> > > But see "NULL pointer dereference" when I try to remove device.
> > 
> > There's a WARN splat occurring before the NULL pointer deref.
> > Was this happening before or is it new?  Probably makes sense
> > to debug that first before looking into the NULL pointer deref,
> > which could be a result of it.
> > 
> 
> Precisely.
> 
> > 
> > > [   38.757726] WARNING: CPU: 1 PID: 816 at drivers/regulator/core.c:5857
> > > regulator_unregister+0x13c/0x160
> > > [   38.767288] Modules linked in: phy_qcom_qmp_combo aux_bridge
> > > drm_kms_helper drm nvme backlight pinctrl_sm8550_lpass_lpi pci_pwrctl_slot
> > > pci_pwrctrl_core nvme_core phy_qcom_edp phy_qcom_eusb2_repeater
> > > dispcc_x1e80100 pinctrl_lpass_lpi phy_qcom_snps_eusb2 lpasscc_sc8280xp typec
> > > gpucc_x1e80100 phy_qcom_qmp_pcie
> > > [   38.795279] CPU: 1 UID: 0 PID: 816 Comm: bash Not tainted
> > > 6.12.0-next-20241128-00005-g6178bf6ce3c2-dirty #50
> > > [   38.805359] Hardware name: Qualcomm IDP, BIOS
> > > 6.0.240607.BOOT.MXF.2.4-00348.1-HAMOA-1.67705.7 06/ 7/2024
> > > [   38.815088] pstate: 61400005 (nZCv daif +PAN -UAO -TCO +DIT -SSBS
> > > BTYPE=--)
> > > [   38.822239] pc : regulator_unregister+0x13c/0x160
> > > [   38.827081] lr : regulator_unregister+0xc0/0x160
> > 
> > The WARN splat seems to be caused by:
> > 
> > 	WARN_ON(rdev->open_count);
> > 
> > So the regulator is unregistered although it's still in use.
> > Is there maybe a multifunction PCIe device in your system
> > so that multiple devices are using the same regulator?
> > 
> 
> Maybe the regulator is shared with other peripherals (not just PCIe) in the
> system.
> 
> @Qiang: I referred your patch [1] that added the slot regulators, but they were
> not used by any peripherals other than PCIe. Could you please post the list of
> consumers of the 3 slot regulators?
>

Just looked briefly into regulator_unregister() and I can see that it will get
called only when the regulator driver is unbound from the regulator device. Your
previous DT reference suggests that you were probably using fixed regulator for
all 3 slot regulators. In that case, this splat can occur when the regulator
driver is unbound (module unload?) with still one of the consumers holding
reference.

So somehow regulator_put() is never called for that consumer but the regulator
is removed. This looks like a bug somewhere.

- Mani

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