[PATCH 0/2] PCI: qcom: Switch to bus notifier for enabling ASPM of PCI devices

[PATCH 0/2] PCI: qcom: Switch to bus notifier for enabling ASPM of PCI devices

[Manivannan Sadhasivam]

Hi,

This series switches the Qcom PCIe controller driver to bus notifier for
enabling ASPM (and updating OPP) for PCI devices. This series is intented
to fix the ASPM regression reported (offlist) on the Qcom compute platforms
running Linux. It turned out that the ASPM enablement logic in the Qcom
controller driver had a flaw that got triggered by the recent changes to the
pwrctrl framework (more details in patch 1/1).

Testing
-------

I've tested this series on Thinkpad T14s laptop and able to observe ASPM state
changes (through controller debugfs entry and lspci) for the WLAN device.

Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>
---
Manivannan Sadhasivam (2):
      PCI: qcom: Switch to bus notifier for enabling ASPM of PCI devices
      PCI: qcom: Move qcom_pcie_icc_opp_update() to notifier callback

 drivers/pci/controller/dwc/pcie-qcom.c | 73 ++++++++++++++++++----------------
 1 file changed, 38 insertions(+), 35 deletions(-)
---
base-commit: 00f0defc332be94b7f1fdc56ce7dcb6528cdf002
change-id: 20250714-aspm_fix-eed392631c8f

Best regards,
-- 
Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>

[PATCH 1/2] PCI: qcom: Switch to bus notifier for enabling ASPM of PCI devices

[Manivannan Sadhasivam]

Commit 9f4f3dfad8cf ("PCI: qcom: Enable ASPM for platforms supporting 1.9.0
ops") allowed the Qcom controller driver to enable ASPM for all PCI devices
enumerated at the time of the controller driver probe. It proved to be
useful for devices already powered on by the bootloader as it allowed
devices to enter ASPM without user intervention.

However, it could not enable ASPM for the hotplug capable devices i.e.,
devices enumerated *after* the controller driver probe. This limitation
mostly went unnoticed as the Qcom PCI controllers are not hotplug capable
and also the bootloader has been enabling the PCI devices before Linux
Kernel boots (mostly on the Qcom compute platforms which users use on a
daily basis).

But with the advent of the commit b458ff7e8176 ("PCI/pwrctl: Ensure that
pwrctl drivers are probed before PCI client drivers"), the pwrctrl driver
started to block the PCI device enumeration until it had been probed.
Though, the intention of the commit was to avoid race between the pwrctrl
driver and PCI client driver, it also meant that the pwrctrl controlled PCI
devices may get probed after the controller driver and will no longer have
ASPM enabled. So users started noticing high runtime power consumption with
WLAN chipsets on Qcom compute platforms like Thinkpad X13s, and Thinkpad
T14s, etc...

Obviously, it is the pwrctrl change that caused regression, but it
ultimately uncovered a flaw in the ASPM enablement logic of the controller
driver. So to address the actual issue, switch to the bus notifier for
enabling ASPM of the PCI devices. The notifier will notify the controller
driver when a PCI device is attached to the bus, thereby allowing it to
enable ASPM more reliably. It should be noted that the
'pci_dev::link_state', which is required for enabling ASPM by the
pci_enable_link_state_locked() API, is only set by the time of
BUS_NOTIFY_BIND_DRIVER stage of the notification. So we cannot enable ASPM
during BUS_NOTIFY_ADD_DEVICE stage.

So with this, we can also get rid of the controller driver specific
'qcom_pcie_ops::host_post_init' callback.

Cc: stable@vger.kernel.org # v6.7
Fixes: 9f4f3dfad8cf ("PCI: qcom: Enable ASPM for platforms supporting 1.9.0 ops")
Reported-by: Johan Hovold <johan@kernel.org>
Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>
---
 drivers/pci/controller/dwc/pcie-qcom.c | 70 ++++++++++++++++++----------------
 1 file changed, 37 insertions(+), 33 deletions(-)

diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
index 620ac7cf09472b84c37e83ee3ce40e94a1d9d878..b4993642ed90915299e825e47d282b8175a78346 100644
--- a/drivers/pci/controller/dwc/pcie-qcom.c
+++ b/drivers/pci/controller/dwc/pcie-qcom.c
@@ -20,6 +20,7 @@
 #include <linux/kernel.h>
 #include <linux/limits.h>
 #include <linux/init.h>
+#include <linux/notifier.h>
 #include <linux/of.h>
 #include <linux/of_pci.h>
 #include <linux/pci.h>
@@ -247,7 +248,6 @@ struct qcom_pcie_ops {
 	int (*get_resources)(struct qcom_pcie *pcie);
 	int (*init)(struct qcom_pcie *pcie);
 	int (*post_init)(struct qcom_pcie *pcie);
-	void (*host_post_init)(struct qcom_pcie *pcie);
 	void (*deinit)(struct qcom_pcie *pcie);
 	void (*ltssm_enable)(struct qcom_pcie *pcie);
 	int (*config_sid)(struct qcom_pcie *pcie);
@@ -286,6 +286,7 @@ struct qcom_pcie {
 	const struct qcom_pcie_cfg *cfg;
 	struct dentry *debugfs;
 	struct list_head ports;
+	struct notifier_block nb;
 	bool suspended;
 	bool use_pm_opp;
 };
@@ -1040,25 +1041,6 @@ static int qcom_pcie_post_init_2_7_0(struct qcom_pcie *pcie)
 	return 0;
 }
 
-static int qcom_pcie_enable_aspm(struct pci_dev *pdev, void *userdata)
-{
-	/*
-	 * Downstream devices need to be in D0 state before enabling PCI PM
-	 * substates.
-	 */
-	pci_set_power_state_locked(pdev, PCI_D0);
-	pci_enable_link_state_locked(pdev, PCIE_LINK_STATE_ALL);
-
-	return 0;
-}
-
-static void qcom_pcie_host_post_init_2_7_0(struct qcom_pcie *pcie)
-{
-	struct dw_pcie_rp *pp = &pcie->pci->pp;
-
-	pci_walk_bus(pp->bridge->bus, qcom_pcie_enable_aspm, NULL);
-}
-
 static void qcom_pcie_deinit_2_7_0(struct qcom_pcie *pcie)
 {
 	struct qcom_pcie_resources_2_7_0 *res = &pcie->res.v2_7_0;
@@ -1358,19 +1340,9 @@ static void qcom_pcie_host_deinit(struct dw_pcie_rp *pp)
 	pcie->cfg->ops->deinit(pcie);
 }
 
-static void qcom_pcie_host_post_init(struct dw_pcie_rp *pp)
-{
-	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
-	struct qcom_pcie *pcie = to_qcom_pcie(pci);
-
-	if (pcie->cfg->ops->host_post_init)
-		pcie->cfg->ops->host_post_init(pcie);
-}
-
 static const struct dw_pcie_host_ops qcom_pcie_dw_ops = {
 	.init		= qcom_pcie_host_init,
 	.deinit		= qcom_pcie_host_deinit,
-	.post_init	= qcom_pcie_host_post_init,
 };
 
 /* Qcom IP rev.: 2.1.0	Synopsys IP rev.: 4.01a */
@@ -1432,7 +1404,6 @@ static const struct qcom_pcie_ops ops_1_9_0 = {
 	.get_resources = qcom_pcie_get_resources_2_7_0,
 	.init = qcom_pcie_init_2_7_0,
 	.post_init = qcom_pcie_post_init_2_7_0,
-	.host_post_init = qcom_pcie_host_post_init_2_7_0,
 	.deinit = qcom_pcie_deinit_2_7_0,
 	.ltssm_enable = qcom_pcie_2_3_2_ltssm_enable,
 	.config_sid = qcom_pcie_config_sid_1_9_0,
@@ -1443,7 +1414,6 @@ static const struct qcom_pcie_ops ops_1_21_0 = {
 	.get_resources = qcom_pcie_get_resources_2_7_0,
 	.init = qcom_pcie_init_2_7_0,
 	.post_init = qcom_pcie_post_init_2_7_0,
-	.host_post_init = qcom_pcie_host_post_init_2_7_0,
 	.deinit = qcom_pcie_deinit_2_7_0,
 	.ltssm_enable = qcom_pcie_2_3_2_ltssm_enable,
 };
@@ -1773,6 +1743,33 @@ static int qcom_pcie_parse_legacy_binding(struct qcom_pcie *pcie)
 	return 0;
 }
 
+static int qcom_pcie_enable_aspm(struct pci_dev *pdev)
+{
+	/*
+	 * Downstream devices need to be in D0 state before enabling PCI PM
+	 * substates.
+	 */
+	pci_set_power_state_locked(pdev, PCI_D0);
+	pci_enable_link_state_locked(pdev, PCIE_LINK_STATE_ALL);
+
+	return 0;
+}
+
+static int pcie_qcom_notify(struct notifier_block *nb, unsigned long action,
+			      void *data)
+{
+	struct qcom_pcie *pcie = container_of(nb, struct qcom_pcie, nb);
+	struct device *dev = data;
+	struct pci_dev *pdev = to_pci_dev(dev);
+
+	switch (action) {
+	case BUS_NOTIFY_BIND_DRIVER:
+		qcom_pcie_enable_aspm(pdev);
+		break;
+	}
+
+	return NOTIFY_DONE;
+}
 static int qcom_pcie_probe(struct platform_device *pdev)
 {
 	const struct qcom_pcie_cfg *pcie_cfg;
@@ -1946,10 +1943,15 @@ static int qcom_pcie_probe(struct platform_device *pdev)
 	if (irq > 0)
 		pp->use_linkup_irq = true;
 
+	pcie->nb.notifier_call = pcie_qcom_notify;
+	ret = bus_register_notifier(&pci_bus_type, &pcie->nb);
+	if (ret)
+		goto err_phy_exit;
+
 	ret = dw_pcie_host_init(pp);
 	if (ret) {
 		dev_err(dev, "cannot initialize host\n");
-		goto err_phy_exit;
+		goto err_unregister_notifier;
 	}
 
 	name = devm_kasprintf(dev, GFP_KERNEL, "qcom_pcie_global_irq%d",
@@ -1982,6 +1984,8 @@ static int qcom_pcie_probe(struct platform_device *pdev)
 
 err_host_deinit:
 	dw_pcie_host_deinit(pp);
+err_unregister_notifier:
+	bus_unregister_notifier(&pci_bus_type, &pcie->nb);
 err_phy_exit:
 	qcom_pcie_phy_exit(pcie);
 	list_for_each_entry_safe(port, tmp, &pcie->ports, list)

-- 
2.45.2

[PATCH 2/2] PCI: qcom: Move qcom_pcie_icc_opp_update() to notifier callback

[Manivannan Sadhasivam]

It allows us to group all the settings that need to be done when a PCI
device is attached to the bus in a single place.

Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>
---
 drivers/pci/controller/dwc/pcie-qcom.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
index b4993642ed90915299e825e47d282b8175a78346..b364977d78a2c659f65f0f12ce4274601d20eaa6 100644
--- a/drivers/pci/controller/dwc/pcie-qcom.c
+++ b/drivers/pci/controller/dwc/pcie-qcom.c
@@ -1616,8 +1616,6 @@ static irqreturn_t qcom_pcie_global_irq_thread(int irq, void *data)
 		pci_lock_rescan_remove();
 		pci_rescan_bus(pp->bridge->bus);
 		pci_unlock_rescan_remove();
-
-		qcom_pcie_icc_opp_update(pcie);
 	} else {
 		dev_WARN_ONCE(dev, 1, "Received unknown event. INT_STATUS: 0x%08x\n",
 			      status);
@@ -1765,6 +1763,7 @@ static int pcie_qcom_notify(struct notifier_block *nb, unsigned long action,
 	switch (action) {
 	case BUS_NOTIFY_BIND_DRIVER:
 		qcom_pcie_enable_aspm(pdev);
+		qcom_pcie_icc_opp_update(pcie);
 		break;
 	}
 

-- 
2.45.2

Re: [PATCH 1/2] PCI: qcom: Switch to bus notifier for enabling ASPM of PCI devices

[Johan Hovold]

On Mon, Jul 14, 2025 at 11:31:04PM +0530, Manivannan Sadhasivam wrote:
> Commit 9f4f3dfad8cf ("PCI: qcom: Enable ASPM for platforms supporting 1.9.0
> ops") allowed the Qcom controller driver to enable ASPM for all PCI devices
> enumerated at the time of the controller driver probe. It proved to be
> useful for devices already powered on by the bootloader as it allowed
> devices to enter ASPM without user intervention.
> 
> However, it could not enable ASPM for the hotplug capable devices i.e.,
> devices enumerated *after* the controller driver probe. This limitation
> mostly went unnoticed as the Qcom PCI controllers are not hotplug capable
> and also the bootloader has been enabling the PCI devices before Linux
> Kernel boots (mostly on the Qcom compute platforms which users use on a
> daily basis).
> 
> But with the advent of the commit b458ff7e8176 ("PCI/pwrctl: Ensure that
> pwrctl drivers are probed before PCI client drivers"), the pwrctrl driver
> started to block the PCI device enumeration until it had been probed.
> Though, the intention of the commit was to avoid race between the pwrctrl
> driver and PCI client driver, it also meant that the pwrctrl controlled PCI
> devices may get probed after the controller driver and will no longer have
> ASPM enabled. So users started noticing high runtime power consumption with
> WLAN chipsets on Qcom compute platforms like Thinkpad X13s, and Thinkpad
> T14s, etc...
> 
> Obviously, it is the pwrctrl change that caused regression, but it
> ultimately uncovered a flaw in the ASPM enablement logic of the controller
> driver. So to address the actual issue, switch to the bus notifier for
> enabling ASPM of the PCI devices. The notifier will notify the controller
> driver when a PCI device is attached to the bus, thereby allowing it to
> enable ASPM more reliably. It should be noted that the
> 'pci_dev::link_state', which is required for enabling ASPM by the
> pci_enable_link_state_locked() API, is only set by the time of
> BUS_NOTIFY_BIND_DRIVER stage of the notification. So we cannot enable ASPM
> during BUS_NOTIFY_ADD_DEVICE stage.

A problem with this approach is that ASPM will never be enabled (and
power consumption will be higher) in case an endpoint driver is missing.

I think that's something we should try to avoid.

> So with this, we can also get rid of the controller driver specific
> 'qcom_pcie_ops::host_post_init' callback.
> 
> Cc: stable@vger.kernel.org # v6.7
> Fixes: 9f4f3dfad8cf ("PCI: qcom: Enable ASPM for platforms supporting 1.9.0 ops")
> Reported-by: Johan Hovold <johan@kernel.org>
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>

Note that the patch fails to apply to 6.16-rc6 due to changes in
linux-next. Depending on how fast we can come up with a fix it may be
better to target 6.16.

> -static int qcom_pcie_enable_aspm(struct pci_dev *pdev, void *userdata)
> -{
> -	/*
> -	 * Downstream devices need to be in D0 state before enabling PCI PM
> -	 * substates.
> -	 */
> -	pci_set_power_state_locked(pdev, PCI_D0);
> -	pci_enable_link_state_locked(pdev, PCIE_LINK_STATE_ALL);
> -
> -	return 0;
> -}

I think you should consider leaving this helper in place here to keep
the size of the diff down (e.g. as you intend to backport this).

> +static int qcom_pcie_enable_aspm(struct pci_dev *pdev)
> +{
> +	/*
> +	 * Downstream devices need to be in D0 state before enabling PCI PM
> +	 * substates.
> +	 */
> +	pci_set_power_state_locked(pdev, PCI_D0);
> +	pci_enable_link_state_locked(pdev, PCIE_LINK_STATE_ALL);

You need to use the non-locked helpers here since you no longer hold the
bus semaphore (e.g. as reported by lockdep).

Maybe this makes the previous comment about not moving the helper moot.

> +
> +	return 0;
> +}
> +
> +static int pcie_qcom_notify(struct notifier_block *nb, unsigned long action,
> +			      void *data)
> +{
> +	struct qcom_pcie *pcie = container_of(nb, struct qcom_pcie, nb);

This results in an unused variable warning (presumably until the next
patch in the series is applied).

> +	struct device *dev = data;
> +	struct pci_dev *pdev = to_pci_dev(dev);
> +
> +	switch (action) {
> +	case BUS_NOTIFY_BIND_DRIVER:
> +		qcom_pcie_enable_aspm(pdev);
> +		break;
> +	}
> +
> +	return NOTIFY_DONE;
> +}

Missing newline.

>  static int qcom_pcie_probe(struct platform_device *pdev)
>  {
>  	const struct qcom_pcie_cfg *pcie_cfg;

Johan

Re: [PATCH 2/2] PCI: qcom: Move qcom_pcie_icc_opp_update() to notifier callback

[Johan Hovold]

On Mon, Jul 14, 2025 at 11:31:05PM +0530, Manivannan Sadhasivam wrote:
> It allows us to group all the settings that need to be done when a PCI
> device is attached to the bus in a single place.

This commit message should be amended so that it makes sense on its own
(e.g. without Subject).

> @@ -1616,8 +1616,6 @@ static irqreturn_t qcom_pcie_global_irq_thread(int irq, void *data)
>  		pci_lock_rescan_remove();
>  		pci_rescan_bus(pp->bridge->bus);
>  		pci_unlock_rescan_remove();
> -
> -		qcom_pcie_icc_opp_update(pcie);
>  	} else {
>  		dev_WARN_ONCE(dev, 1, "Received unknown event. INT_STATUS: 0x%08x\n",
>  			      status);
> @@ -1765,6 +1763,7 @@ static int pcie_qcom_notify(struct notifier_block *nb, unsigned long action,
>  	switch (action) {
>  	case BUS_NOTIFY_BIND_DRIVER:
>  		qcom_pcie_enable_aspm(pdev);
> +		qcom_pcie_icc_opp_update(pcie);

I guess you should also drop the now redundant
qcom_pcie_icc_opp_update() call from probe()?

>  		break;
>  	}

Johan

Re: [PATCH 1/2] PCI: qcom: Switch to bus notifier for enabling ASPM of PCI devices

[Manivannan Sadhasivam]

On Tue, Jul 15, 2025 at 09:48:30AM GMT, Johan Hovold wrote:
> On Mon, Jul 14, 2025 at 11:31:04PM +0530, Manivannan Sadhasivam wrote:
> > Commit 9f4f3dfad8cf ("PCI: qcom: Enable ASPM for platforms supporting 1.9.0
> > ops") allowed the Qcom controller driver to enable ASPM for all PCI devices
> > enumerated at the time of the controller driver probe. It proved to be
> > useful for devices already powered on by the bootloader as it allowed
> > devices to enter ASPM without user intervention.
> > 
> > However, it could not enable ASPM for the hotplug capable devices i.e.,
> > devices enumerated *after* the controller driver probe. This limitation
> > mostly went unnoticed as the Qcom PCI controllers are not hotplug capable
> > and also the bootloader has been enabling the PCI devices before Linux
> > Kernel boots (mostly on the Qcom compute platforms which users use on a
> > daily basis).
> > 
> > But with the advent of the commit b458ff7e8176 ("PCI/pwrctl: Ensure that
> > pwrctl drivers are probed before PCI client drivers"), the pwrctrl driver
> > started to block the PCI device enumeration until it had been probed.
> > Though, the intention of the commit was to avoid race between the pwrctrl
> > driver and PCI client driver, it also meant that the pwrctrl controlled PCI
> > devices may get probed after the controller driver and will no longer have
> > ASPM enabled. So users started noticing high runtime power consumption with
> > WLAN chipsets on Qcom compute platforms like Thinkpad X13s, and Thinkpad
> > T14s, etc...
> > 
> > Obviously, it is the pwrctrl change that caused regression, but it
> > ultimately uncovered a flaw in the ASPM enablement logic of the controller
> > driver. So to address the actual issue, switch to the bus notifier for
> > enabling ASPM of the PCI devices. The notifier will notify the controller
> > driver when a PCI device is attached to the bus, thereby allowing it to
> > enable ASPM more reliably. It should be noted that the
> > 'pci_dev::link_state', which is required for enabling ASPM by the
> > pci_enable_link_state_locked() API, is only set by the time of
> > BUS_NOTIFY_BIND_DRIVER stage of the notification. So we cannot enable ASPM
> > during BUS_NOTIFY_ADD_DEVICE stage.
> 
> A problem with this approach is that ASPM will never be enabled (and
> power consumption will be higher) in case an endpoint driver is missing.
> 

I'm aware of this limiation. But I don't think we should really worry about that
scenario. No one is going to run an OS intentionally with a PCI device and
without the relevant driver. If that happens, it might be due to some issue in
driver loading or the user is doing it intentionally. Such scenarios are short
lived IMO.

> I think that's something we should try to avoid.
> 

I would've fancied a bus notifier post device addition, but there is none
available and I don't see a real incentive in adding one. The other option
would be to add an ops to 'struct pci_host_bridge', but I really try not to
introduce such thing unless really manadatory.

> > So with this, we can also get rid of the controller driver specific
> > 'qcom_pcie_ops::host_post_init' callback.
> > 
> > Cc: stable@vger.kernel.org # v6.7
> > Fixes: 9f4f3dfad8cf ("PCI: qcom: Enable ASPM for platforms supporting 1.9.0 ops")
> > Reported-by: Johan Hovold <johan@kernel.org>
> > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>
> 
> Note that the patch fails to apply to 6.16-rc6 due to changes in
> linux-next. Depending on how fast we can come up with a fix it may be
> better to target 6.16.
> 

I rebased this series on top of pci/controller/qcom branch, where we have some
dependency. But I could spin an independent fix if Bjorn is OK to take it for
the 6.16-rcS.

> > -static int qcom_pcie_enable_aspm(struct pci_dev *pdev, void *userdata)
> > -{
> > -	/*
> > -	 * Downstream devices need to be in D0 state before enabling PCI PM
> > -	 * substates.
> > -	 */
> > -	pci_set_power_state_locked(pdev, PCI_D0);
> > -	pci_enable_link_state_locked(pdev, PCIE_LINK_STATE_ALL);
> > -
> > -	return 0;
> > -}
> 
> I think you should consider leaving this helper in place here to keep
> the size of the diff down (e.g. as you intend to backport this).
> 

Ok.

> > +static int qcom_pcie_enable_aspm(struct pci_dev *pdev)
> > +{
> > +	/*
> > +	 * Downstream devices need to be in D0 state before enabling PCI PM
> > +	 * substates.
> > +	 */
> > +	pci_set_power_state_locked(pdev, PCI_D0);
> > +	pci_enable_link_state_locked(pdev, PCIE_LINK_STATE_ALL);
> 
> You need to use the non-locked helpers here since you no longer hold the
> bus semaphore (e.g. as reported by lockdep).
> 

Good catch!

- Mani

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

Re: [PATCH 2/2] PCI: qcom: Move qcom_pcie_icc_opp_update() to notifier callback

[Manivannan Sadhasivam]

On Tue, Jul 15, 2025 at 09:51:40AM GMT, Johan Hovold wrote:
> On Mon, Jul 14, 2025 at 11:31:05PM +0530, Manivannan Sadhasivam wrote:
> > It allows us to group all the settings that need to be done when a PCI
> > device is attached to the bus in a single place.
> 
> This commit message should be amended so that it makes sense on its own
> (e.g. without Subject).
> 
> > @@ -1616,8 +1616,6 @@ static irqreturn_t qcom_pcie_global_irq_thread(int irq, void *data)
> >  		pci_lock_rescan_remove();
> >  		pci_rescan_bus(pp->bridge->bus);
> >  		pci_unlock_rescan_remove();
> > -
> > -		qcom_pcie_icc_opp_update(pcie);
> >  	} else {
> >  		dev_WARN_ONCE(dev, 1, "Received unknown event. INT_STATUS: 0x%08x\n",
> >  			      status);
> > @@ -1765,6 +1763,7 @@ static int pcie_qcom_notify(struct notifier_block *nb, unsigned long action,
> >  	switch (action) {
> >  	case BUS_NOTIFY_BIND_DRIVER:
> >  		qcom_pcie_enable_aspm(pdev);
> > +		qcom_pcie_icc_opp_update(pcie);
> 
> I guess you should also drop the now redundant
> qcom_pcie_icc_opp_update() call from probe()?
> 

Oops. This got sneaked in. I removed it locally but eventually lost the change
while rebasing. Will remove it in next version. This API just bails out if the
link is not up. So no reason to call it here also now.

- Mani

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

Re: [PATCH 1/2] PCI: qcom: Switch to bus notifier for enabling ASPM of PCI devices

[Johan Hovold]

On Tue, Jul 15, 2025 at 02:41:23PM +0530, Manivannan Sadhasivam wrote:
> On Tue, Jul 15, 2025 at 09:48:30AM GMT, Johan Hovold wrote:
> > On Mon, Jul 14, 2025 at 11:31:04PM +0530, Manivannan Sadhasivam wrote:

> > > Obviously, it is the pwrctrl change that caused regression, but it
> > > ultimately uncovered a flaw in the ASPM enablement logic of the controller
> > > driver. So to address the actual issue, switch to the bus notifier for
> > > enabling ASPM of the PCI devices. The notifier will notify the controller
> > > driver when a PCI device is attached to the bus, thereby allowing it to
> > > enable ASPM more reliably. It should be noted that the
> > > 'pci_dev::link_state', which is required for enabling ASPM by the
> > > pci_enable_link_state_locked() API, is only set by the time of
> > > BUS_NOTIFY_BIND_DRIVER stage of the notification. So we cannot enable ASPM
> > > during BUS_NOTIFY_ADD_DEVICE stage.
> > 
> > A problem with this approach is that ASPM will never be enabled (and
> > power consumption will be higher) in case an endpoint driver is missing.
> 
> I'm aware of this limiation. But I don't think we should really worry about that
> scenario. No one is going to run an OS intentionally with a PCI device and
> without the relevant driver. If that happens, it might be due to some issue in
> driver loading or the user is doing it intentionally. Such scenarios are short
> lived IMO.

There may not even be a driver (yet). A user could plug in whatever
device in a free slot. I can also imagine someone wanting to blacklist
a driver temporarily for whatever reason.

How would this work on x86? Would the BIOS typically enable ASPM for
each EP? Then that's what we should do here too, even if the EP driver
happens to be disabled.

> > Note that the patch fails to apply to 6.16-rc6 due to changes in
> > linux-next. Depending on how fast we can come up with a fix it may be
> > better to target 6.16.
> 
> I rebased this series on top of pci/controller/qcom branch, where we have some
> dependency. But I could spin an independent fix if Bjorn is OK to take it for
> the 6.16-rcS.

Or we can just backport manually as we are indeed already at rc6.

Johan

Re: [PATCH 1/2] PCI: qcom: Switch to bus notifier for enabling ASPM of PCI devices

[Konrad Dybcio]

On 7/15/25 11:33 AM, Johan Hovold wrote:
> On Tue, Jul 15, 2025 at 02:41:23PM +0530, Manivannan Sadhasivam wrote:
>> On Tue, Jul 15, 2025 at 09:48:30AM GMT, Johan Hovold wrote:
>>> On Mon, Jul 14, 2025 at 11:31:04PM +0530, Manivannan Sadhasivam wrote:
> 
>>>> Obviously, it is the pwrctrl change that caused regression, but it
>>>> ultimately uncovered a flaw in the ASPM enablement logic of the controller
>>>> driver. So to address the actual issue, switch to the bus notifier for
>>>> enabling ASPM of the PCI devices. The notifier will notify the controller
>>>> driver when a PCI device is attached to the bus, thereby allowing it to
>>>> enable ASPM more reliably. It should be noted that the
>>>> 'pci_dev::link_state', which is required for enabling ASPM by the
>>>> pci_enable_link_state_locked() API, is only set by the time of
>>>> BUS_NOTIFY_BIND_DRIVER stage of the notification. So we cannot enable ASPM
>>>> during BUS_NOTIFY_ADD_DEVICE stage.
>>>
>>> A problem with this approach is that ASPM will never be enabled (and
>>> power consumption will be higher) in case an endpoint driver is missing.
>>
>> I'm aware of this limiation. But I don't think we should really worry about that
>> scenario. No one is going to run an OS intentionally with a PCI device and
>> without the relevant driver. If that happens, it might be due to some issue in
>> driver loading or the user is doing it intentionally. Such scenarios are short
>> lived IMO.
> 
> There may not even be a driver (yet). A user could plug in whatever
> device in a free slot. I can also imagine someone wanting to blacklist
> a driver temporarily for whatever reason.
> 
> How would this work on x86? Would the BIOS typically enable ASPM for
> each EP? Then that's what we should do here too, even if the EP driver
> happens to be disabled.

Not sure about all x86, but the Intel VMD controller driver surely doesn't
care what's on the other end:

drivers/pci/controller/vmd.c : vmd_pm_enable_quirk()

Konrad

Re: [PATCH 2/2] PCI: qcom: Move qcom_pcie_icc_opp_update() to notifier callback

[Konrad Dybcio]

On 7/14/25 8:01 PM, Manivannan Sadhasivam wrote:
> It allows us to group all the settings that need to be done when a PCI
> device is attached to the bus in a single place.
> 
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>
> ---
>  drivers/pci/controller/dwc/pcie-qcom.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> index b4993642ed90915299e825e47d282b8175a78346..b364977d78a2c659f65f0f12ce4274601d20eaa6 100644
> --- a/drivers/pci/controller/dwc/pcie-qcom.c
> +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> @@ -1616,8 +1616,6 @@ static irqreturn_t qcom_pcie_global_irq_thread(int irq, void *data)
>  		pci_lock_rescan_remove();
>  		pci_rescan_bus(pp->bridge->bus);
>  		pci_unlock_rescan_remove();
> -
> -		qcom_pcie_icc_opp_update(pcie);
>  	} else {
>  		dev_WARN_ONCE(dev, 1, "Received unknown event. INT_STATUS: 0x%08x\n",
>  			      status);
> @@ -1765,6 +1763,7 @@ static int pcie_qcom_notify(struct notifier_block *nb, unsigned long action,
>  	switch (action) {
>  	case BUS_NOTIFY_BIND_DRIVER:
>  		qcom_pcie_enable_aspm(pdev);
> +		qcom_pcie_icc_opp_update(pcie);

So I assume that we're not exactly going to do much with the device if
there isn't a driver for it, but I have concerns that since the link
would already be established(?), the icc vote may be too low, especially
if the user uses something funky like UIO

Konrad

Re: [PATCH 1/2] PCI: qcom: Switch to bus notifier for enabling ASPM of PCI devices

[Manivannan Sadhasivam]

On Tue, Jul 15, 2025 at 11:33:16AM GMT, Johan Hovold wrote:
> On Tue, Jul 15, 2025 at 02:41:23PM +0530, Manivannan Sadhasivam wrote:
> > On Tue, Jul 15, 2025 at 09:48:30AM GMT, Johan Hovold wrote:
> > > On Mon, Jul 14, 2025 at 11:31:04PM +0530, Manivannan Sadhasivam wrote:
> 
> > > > Obviously, it is the pwrctrl change that caused regression, but it
> > > > ultimately uncovered a flaw in the ASPM enablement logic of the controller
> > > > driver. So to address the actual issue, switch to the bus notifier for
> > > > enabling ASPM of the PCI devices. The notifier will notify the controller
> > > > driver when a PCI device is attached to the bus, thereby allowing it to
> > > > enable ASPM more reliably. It should be noted that the
> > > > 'pci_dev::link_state', which is required for enabling ASPM by the
> > > > pci_enable_link_state_locked() API, is only set by the time of
> > > > BUS_NOTIFY_BIND_DRIVER stage of the notification. So we cannot enable ASPM
> > > > during BUS_NOTIFY_ADD_DEVICE stage.
> > > 
> > > A problem with this approach is that ASPM will never be enabled (and
> > > power consumption will be higher) in case an endpoint driver is missing.
> > 
> > I'm aware of this limiation. But I don't think we should really worry about that
> > scenario. No one is going to run an OS intentionally with a PCI device and
> > without the relevant driver. If that happens, it might be due to some issue in
> > driver loading or the user is doing it intentionally. Such scenarios are short
> > lived IMO.
> 
> There may not even be a driver (yet). A user could plug in whatever
> device in a free slot. I can also imagine someone wanting to blacklist
> a driver temporarily for whatever reason.
> 

Yes, that's why I said these scenarios are 'shortlived'.

> How would this work on x86? Would the BIOS typically enable ASPM for
> each EP? Then that's what we should do here too, even if the EP driver
> happens to be disabled.
> 

There is no guarantee that BIOS would enable ASPM for all the devices in x86
world. Usually, BIOS would enable ASPM for devices that it makes use of (like
NVMe SSD or WLAN) and don't touch the rest, AFAIK.

> > > Note that the patch fails to apply to 6.16-rc6 due to changes in
> > > linux-next. Depending on how fast we can come up with a fix it may be
> > > better to target 6.16.
> > 
> > I rebased this series on top of pci/controller/qcom branch, where we have some
> > dependency. But I could spin an independent fix if Bjorn is OK to take it for
> > the 6.16-rcS.
> 
> Or we can just backport manually as we are indeed already at rc6.
> 

I'll leave it up to Bjorn to decide.

- Mani

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

Re: [PATCH 2/2] PCI: qcom: Move qcom_pcie_icc_opp_update() to notifier callback

[Manivannan Sadhasivam]

On Tue, Jul 15, 2025 at 11:54:48AM GMT, Konrad Dybcio wrote:
> On 7/14/25 8:01 PM, Manivannan Sadhasivam wrote:
> > It allows us to group all the settings that need to be done when a PCI
> > device is attached to the bus in a single place.
> > 
> > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>
> > ---
> >  drivers/pci/controller/dwc/pcie-qcom.c | 3 +--
> >  1 file changed, 1 insertion(+), 2 deletions(-)
> > 
> > diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> > index b4993642ed90915299e825e47d282b8175a78346..b364977d78a2c659f65f0f12ce4274601d20eaa6 100644
> > --- a/drivers/pci/controller/dwc/pcie-qcom.c
> > +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> > @@ -1616,8 +1616,6 @@ static irqreturn_t qcom_pcie_global_irq_thread(int irq, void *data)
> >  		pci_lock_rescan_remove();
> >  		pci_rescan_bus(pp->bridge->bus);
> >  		pci_unlock_rescan_remove();
> > -
> > -		qcom_pcie_icc_opp_update(pcie);
> >  	} else {
> >  		dev_WARN_ONCE(dev, 1, "Received unknown event. INT_STATUS: 0x%08x\n",
> >  			      status);
> > @@ -1765,6 +1763,7 @@ static int pcie_qcom_notify(struct notifier_block *nb, unsigned long action,
> >  	switch (action) {
> >  	case BUS_NOTIFY_BIND_DRIVER:
> >  		qcom_pcie_enable_aspm(pdev);
> > +		qcom_pcie_icc_opp_update(pcie);
> 
> So I assume that we're not exactly going to do much with the device if
> there isn't a driver for it, but I have concerns that since the link
> would already be established(?), the icc vote may be too low, especially
> if the user uses something funky like UIO
> 

Hmm, that's a good point. Not enabling ASPM wouldn't have much consequence, but
not updating OPP would be.

Let me think of other ways to call these two APIs during the device addition. If
there are no sane ways, I'll drop *this* patch.

- Mani

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

Re: [PATCH 2/2] PCI: qcom: Move qcom_pcie_icc_opp_update() to notifier callback

[Konrad Dybcio]

On 7/15/25 12:36 PM, Manivannan Sadhasivam wrote:
> On Tue, Jul 15, 2025 at 11:54:48AM GMT, Konrad Dybcio wrote:
>> On 7/14/25 8:01 PM, Manivannan Sadhasivam wrote:
>>> It allows us to group all the settings that need to be done when a PCI
>>> device is attached to the bus in a single place.
>>>
>>> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>
>>> ---
>>>  drivers/pci/controller/dwc/pcie-qcom.c | 3 +--
>>>  1 file changed, 1 insertion(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
>>> index b4993642ed90915299e825e47d282b8175a78346..b364977d78a2c659f65f0f12ce4274601d20eaa6 100644
>>> --- a/drivers/pci/controller/dwc/pcie-qcom.c
>>> +++ b/drivers/pci/controller/dwc/pcie-qcom.c
>>> @@ -1616,8 +1616,6 @@ static irqreturn_t qcom_pcie_global_irq_thread(int irq, void *data)
>>>  		pci_lock_rescan_remove();
>>>  		pci_rescan_bus(pp->bridge->bus);
>>>  		pci_unlock_rescan_remove();
>>> -
>>> -		qcom_pcie_icc_opp_update(pcie);
>>>  	} else {
>>>  		dev_WARN_ONCE(dev, 1, "Received unknown event. INT_STATUS: 0x%08x\n",
>>>  			      status);
>>> @@ -1765,6 +1763,7 @@ static int pcie_qcom_notify(struct notifier_block *nb, unsigned long action,
>>>  	switch (action) {
>>>  	case BUS_NOTIFY_BIND_DRIVER:
>>>  		qcom_pcie_enable_aspm(pdev);
>>> +		qcom_pcie_icc_opp_update(pcie);
>>
>> So I assume that we're not exactly going to do much with the device if
>> there isn't a driver for it, but I have concerns that since the link
>> would already be established(?), the icc vote may be too low, especially
>> if the user uses something funky like UIO
>>
> 
> Hmm, that's a good point. Not enabling ASPM wouldn't have much consequence, but
> not updating OPP would be.
> 
> Let me think of other ways to call these two APIs during the device addition. If
> there are no sane ways, I'll drop *this* patch.

Would it be too naive to assume BUS_NOTIFY_ADD_DEVICE is a good fit? Do
ASPM setting need to be reapplied after the PCIe device is reset? (well
I would assume there are probably multiple levels of "reset" :/)

Konrad

Re: [PATCH 1/2] PCI: qcom: Switch to bus notifier for enabling ASPM of PCI devices

[Johan Hovold]

On Tue, Jul 15, 2025 at 03:57:12PM +0530, Manivannan Sadhasivam wrote:
> On Tue, Jul 15, 2025 at 11:33:16AM GMT, Johan Hovold wrote:
> > On Tue, Jul 15, 2025 at 02:41:23PM +0530, Manivannan Sadhasivam wrote:
> > > On Tue, Jul 15, 2025 at 09:48:30AM GMT, Johan Hovold wrote:

> > > > A problem with this approach is that ASPM will never be enabled (and
> > > > power consumption will be higher) in case an endpoint driver is missing.
> > > 
> > > I'm aware of this limiation. But I don't think we should really worry about that
> > > scenario. No one is going to run an OS intentionally with a PCI device and
> > > without the relevant driver. If that happens, it might be due to some issue in
> > > driver loading or the user is doing it intentionally. Such scenarios are short
> > > lived IMO.
> > 
> > There may not even be a driver (yet). A user could plug in whatever
> > device in a free slot. I can also imagine someone wanting to blacklist
> > a driver temporarily for whatever reason.
> 
> Yes, that's why I said these scenarios are 'shortlived'.

My point is the opposite; that you should not make such assumptions
(e.g. hardware not supported by linux or drivers disabled due to
stability or security concerns).

Johan

Re: [PATCH 2/2] PCI: qcom: Move qcom_pcie_icc_opp_update() to notifier callback

[Krishna Chaitanya Chundru]

On 7/15/2025 4:06 PM, Manivannan Sadhasivam wrote:
> On Tue, Jul 15, 2025 at 11:54:48AM GMT, Konrad Dybcio wrote:
>> On 7/14/25 8:01 PM, Manivannan Sadhasivam wrote:
>>> It allows us to group all the settings that need to be done when a PCI
>>> device is attached to the bus in a single place.
>>>
>>> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>
>>> ---
>>>   drivers/pci/controller/dwc/pcie-qcom.c | 3 +--
>>>   1 file changed, 1 insertion(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
>>> index b4993642ed90915299e825e47d282b8175a78346..b364977d78a2c659f65f0f12ce4274601d20eaa6 100644
>>> --- a/drivers/pci/controller/dwc/pcie-qcom.c
>>> +++ b/drivers/pci/controller/dwc/pcie-qcom.c
>>> @@ -1616,8 +1616,6 @@ static irqreturn_t qcom_pcie_global_irq_thread(int irq, void *data)
>>>   		pci_lock_rescan_remove();
>>>   		pci_rescan_bus(pp->bridge->bus);
>>>   		pci_unlock_rescan_remove();
>>> -
>>> -		qcom_pcie_icc_opp_update(pcie);
>>>   	} else {
>>>   		dev_WARN_ONCE(dev, 1, "Received unknown event. INT_STATUS: 0x%08x\n",
>>>   			      status);
>>> @@ -1765,6 +1763,7 @@ static int pcie_qcom_notify(struct notifier_block *nb, unsigned long action,
>>>   	switch (action) {
>>>   	case BUS_NOTIFY_BIND_DRIVER:
>>>   		qcom_pcie_enable_aspm(pdev);
>>> +		qcom_pcie_icc_opp_update(pcie);
>>
>> So I assume that we're not exactly going to do much with the device if
>> there isn't a driver for it, but I have concerns that since the link
>> would already be established(?), the icc vote may be too low, especially
>> if the user uses something funky like UIO
>>
> 
> Hmm, that's a good point. Not enabling ASPM wouldn't have much consequence, but
> not updating OPP would be.
> 
> Let me think of other ways to call these two APIs during the device addition. If
> there are no sane ways, I'll drop *this* patch.
> 
How about using enable_device in host bridge, without pci_enable_device
call the endpoints can't start the transfers. May be we can use that.

- Krishna Chaitanya.
> - Mani
> 

Re: [PATCH 2/2] PCI: qcom: Move qcom_pcie_icc_opp_update() to notifier callback

[Manivannan Sadhasivam]

On Tue, Jul 15, 2025 at 12:45:36PM GMT, Konrad Dybcio wrote:
> On 7/15/25 12:36 PM, Manivannan Sadhasivam wrote:
> > On Tue, Jul 15, 2025 at 11:54:48AM GMT, Konrad Dybcio wrote:
> >> On 7/14/25 8:01 PM, Manivannan Sadhasivam wrote:
> >>> It allows us to group all the settings that need to be done when a PCI
> >>> device is attached to the bus in a single place.
> >>>
> >>> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>
> >>> ---
> >>>  drivers/pci/controller/dwc/pcie-qcom.c | 3 +--
> >>>  1 file changed, 1 insertion(+), 2 deletions(-)
> >>>
> >>> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> >>> index b4993642ed90915299e825e47d282b8175a78346..b364977d78a2c659f65f0f12ce4274601d20eaa6 100644
> >>> --- a/drivers/pci/controller/dwc/pcie-qcom.c
> >>> +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> >>> @@ -1616,8 +1616,6 @@ static irqreturn_t qcom_pcie_global_irq_thread(int irq, void *data)
> >>>  		pci_lock_rescan_remove();
> >>>  		pci_rescan_bus(pp->bridge->bus);
> >>>  		pci_unlock_rescan_remove();
> >>> -
> >>> -		qcom_pcie_icc_opp_update(pcie);
> >>>  	} else {
> >>>  		dev_WARN_ONCE(dev, 1, "Received unknown event. INT_STATUS: 0x%08x\n",
> >>>  			      status);
> >>> @@ -1765,6 +1763,7 @@ static int pcie_qcom_notify(struct notifier_block *nb, unsigned long action,
> >>>  	switch (action) {
> >>>  	case BUS_NOTIFY_BIND_DRIVER:
> >>>  		qcom_pcie_enable_aspm(pdev);
> >>> +		qcom_pcie_icc_opp_update(pcie);
> >>
> >> So I assume that we're not exactly going to do much with the device if
> >> there isn't a driver for it, but I have concerns that since the link
> >> would already be established(?), the icc vote may be too low, especially
> >> if the user uses something funky like UIO
> >>
> > 
> > Hmm, that's a good point. Not enabling ASPM wouldn't have much consequence, but
> > not updating OPP would be.
> > 
> > Let me think of other ways to call these two APIs during the device addition. If
> > there are no sane ways, I'll drop *this* patch.
> 
> Would it be too naive to assume BUS_NOTIFY_ADD_DEVICE is a good fit?

BUS_NOTIFY_ADD_DEVICE is not currently a good fit as ASPM link state
initialization happen after all the devices are enumerated for the slot. This is
something to be fixed in the PCI core and would allow us to use
BUS_NOTIFY_ADD_DEVICE.

I talked to Bjorn H and we both agreed that this needs to be revisited. But I'm
just worrried that until this happens, we cannot upstream the ASPM fix and not
even backport it to 6.16/16.

So maybe we need to resort to this patch as an interim fix if everyone agrees.

> Do
> ASPM setting need to be reapplied after the PCIe device is reset? (well
> I would assume there are probably multiple levels of "reset" :/)
> 

I'm assuming that you are referring to link down reset here. PCI core takes care
of saving both the endpoint as well as Root Port config space when that happens
and restores them afterwards.

- Mani

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

Re: [PATCH 2/2] PCI: qcom: Move qcom_pcie_icc_opp_update() to notifier callback

[Manivannan Sadhasivam]

On Wed, Jul 16, 2025 at 10:24:23AM GMT, Krishna Chaitanya Chundru wrote:
> 
> 
> On 7/15/2025 4:06 PM, Manivannan Sadhasivam wrote:
> > On Tue, Jul 15, 2025 at 11:54:48AM GMT, Konrad Dybcio wrote:
> > > On 7/14/25 8:01 PM, Manivannan Sadhasivam wrote:
> > > > It allows us to group all the settings that need to be done when a PCI
> > > > device is attached to the bus in a single place.
> > > > 
> > > > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>
> > > > ---
> > > >   drivers/pci/controller/dwc/pcie-qcom.c | 3 +--
> > > >   1 file changed, 1 insertion(+), 2 deletions(-)
> > > > 
> > > > diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> > > > index b4993642ed90915299e825e47d282b8175a78346..b364977d78a2c659f65f0f12ce4274601d20eaa6 100644
> > > > --- a/drivers/pci/controller/dwc/pcie-qcom.c
> > > > +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> > > > @@ -1616,8 +1616,6 @@ static irqreturn_t qcom_pcie_global_irq_thread(int irq, void *data)
> > > >   		pci_lock_rescan_remove();
> > > >   		pci_rescan_bus(pp->bridge->bus);
> > > >   		pci_unlock_rescan_remove();
> > > > -
> > > > -		qcom_pcie_icc_opp_update(pcie);
> > > >   	} else {
> > > >   		dev_WARN_ONCE(dev, 1, "Received unknown event. INT_STATUS: 0x%08x\n",
> > > >   			      status);
> > > > @@ -1765,6 +1763,7 @@ static int pcie_qcom_notify(struct notifier_block *nb, unsigned long action,
> > > >   	switch (action) {
> > > >   	case BUS_NOTIFY_BIND_DRIVER:
> > > >   		qcom_pcie_enable_aspm(pdev);
> > > > +		qcom_pcie_icc_opp_update(pcie);
> > > 
> > > So I assume that we're not exactly going to do much with the device if
> > > there isn't a driver for it, but I have concerns that since the link
> > > would already be established(?), the icc vote may be too low, especially
> > > if the user uses something funky like UIO
> > > 
> > 
> > Hmm, that's a good point. Not enabling ASPM wouldn't have much consequence, but
> > not updating OPP would be.
> > 
> > Let me think of other ways to call these two APIs during the device addition. If
> > there are no sane ways, I'll drop *this* patch.
> > 
> How about using enable_device in host bridge, without pci_enable_device
> call the endpoints can't start the transfers. May be we can use that.
> 

Q: Who is going to call pci_enable_device()?
A: The PCI client driver

This is same as relying on BUS_NOTIFY_BIND_DRIVER notifier.

- Mani

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

Re: [PATCH 2/2] PCI: qcom: Move qcom_pcie_icc_opp_update() to notifier callback

[Krishna Chaitanya Chundru]

On 7/16/2025 12:16 PM, Manivannan Sadhasivam wrote:
> On Wed, Jul 16, 2025 at 10:24:23AM GMT, Krishna Chaitanya Chundru wrote:
>>
>>
>> On 7/15/2025 4:06 PM, Manivannan Sadhasivam wrote:
>>> On Tue, Jul 15, 2025 at 11:54:48AM GMT, Konrad Dybcio wrote:
>>>> On 7/14/25 8:01 PM, Manivannan Sadhasivam wrote:
>>>>> It allows us to group all the settings that need to be done when a PCI
>>>>> device is attached to the bus in a single place.
>>>>>
>>>>> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>
>>>>> ---
>>>>>    drivers/pci/controller/dwc/pcie-qcom.c | 3 +--
>>>>>    1 file changed, 1 insertion(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
>>>>> index b4993642ed90915299e825e47d282b8175a78346..b364977d78a2c659f65f0f12ce4274601d20eaa6 100644
>>>>> --- a/drivers/pci/controller/dwc/pcie-qcom.c
>>>>> +++ b/drivers/pci/controller/dwc/pcie-qcom.c
>>>>> @@ -1616,8 +1616,6 @@ static irqreturn_t qcom_pcie_global_irq_thread(int irq, void *data)
>>>>>    		pci_lock_rescan_remove();
>>>>>    		pci_rescan_bus(pp->bridge->bus);
>>>>>    		pci_unlock_rescan_remove();
>>>>> -
>>>>> -		qcom_pcie_icc_opp_update(pcie);
>>>>>    	} else {
>>>>>    		dev_WARN_ONCE(dev, 1, "Received unknown event. INT_STATUS: 0x%08x\n",
>>>>>    			      status);
>>>>> @@ -1765,6 +1763,7 @@ static int pcie_qcom_notify(struct notifier_block *nb, unsigned long action,
>>>>>    	switch (action) {
>>>>>    	case BUS_NOTIFY_BIND_DRIVER:
>>>>>    		qcom_pcie_enable_aspm(pdev);
>>>>> +		qcom_pcie_icc_opp_update(pcie);
>>>>
>>>> So I assume that we're not exactly going to do much with the device if
>>>> there isn't a driver for it, but I have concerns that since the link
>>>> would already be established(?), the icc vote may be too low, especially
>>>> if the user uses something funky like UIO
>>>>
>>>
>>> Hmm, that's a good point. Not enabling ASPM wouldn't have much consequence, but
>>> not updating OPP would be.
>>>
>>> Let me think of other ways to call these two APIs during the device addition. If
>>> there are no sane ways, I'll drop *this* patch.
>>>
>> How about using enable_device in host bridge, without pci_enable_device
>> call the endpoints can't start the transfers. May be we can use that.
>>
> 
> Q: Who is going to call pci_enable_device()?
> A: The PCI client driver
> 
> This is same as relying on BUS_NOTIFY_BIND_DRIVER notifier.
>
userspace can enable device using sysfs[1] without attaching
any kernel drivers.

[1] 
https://elixir.bootlin.com/linux/v6.16-rc6/source/drivers/pci/pci-sysfs.c#L315
- Krishna Chaitanya.

> - Mani
> 

Re: [PATCH 2/2] PCI: qcom: Move qcom_pcie_icc_opp_update() to notifier callback

[Manivannan Sadhasivam]

On Wed, Jul 16, 2025 at 12:23:54PM GMT, Krishna Chaitanya Chundru wrote:
> 
> 
> On 7/16/2025 12:16 PM, Manivannan Sadhasivam wrote:
> > On Wed, Jul 16, 2025 at 10:24:23AM GMT, Krishna Chaitanya Chundru wrote:
> > > 
> > > 
> > > On 7/15/2025 4:06 PM, Manivannan Sadhasivam wrote:
> > > > On Tue, Jul 15, 2025 at 11:54:48AM GMT, Konrad Dybcio wrote:
> > > > > On 7/14/25 8:01 PM, Manivannan Sadhasivam wrote:
> > > > > > It allows us to group all the settings that need to be done when a PCI
> > > > > > device is attached to the bus in a single place.
> > > > > > 
> > > > > > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>
> > > > > > ---
> > > > > >    drivers/pci/controller/dwc/pcie-qcom.c | 3 +--
> > > > > >    1 file changed, 1 insertion(+), 2 deletions(-)
> > > > > > 
> > > > > > diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> > > > > > index b4993642ed90915299e825e47d282b8175a78346..b364977d78a2c659f65f0f12ce4274601d20eaa6 100644
> > > > > > --- a/drivers/pci/controller/dwc/pcie-qcom.c
> > > > > > +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> > > > > > @@ -1616,8 +1616,6 @@ static irqreturn_t qcom_pcie_global_irq_thread(int irq, void *data)
> > > > > >    		pci_lock_rescan_remove();
> > > > > >    		pci_rescan_bus(pp->bridge->bus);
> > > > > >    		pci_unlock_rescan_remove();
> > > > > > -
> > > > > > -		qcom_pcie_icc_opp_update(pcie);
> > > > > >    	} else {
> > > > > >    		dev_WARN_ONCE(dev, 1, "Received unknown event. INT_STATUS: 0x%08x\n",
> > > > > >    			      status);
> > > > > > @@ -1765,6 +1763,7 @@ static int pcie_qcom_notify(struct notifier_block *nb, unsigned long action,
> > > > > >    	switch (action) {
> > > > > >    	case BUS_NOTIFY_BIND_DRIVER:
> > > > > >    		qcom_pcie_enable_aspm(pdev);
> > > > > > +		qcom_pcie_icc_opp_update(pcie);
> > > > > 
> > > > > So I assume that we're not exactly going to do much with the device if
> > > > > there isn't a driver for it, but I have concerns that since the link
> > > > > would already be established(?), the icc vote may be too low, especially
> > > > > if the user uses something funky like UIO
> > > > > 
> > > > 
> > > > Hmm, that's a good point. Not enabling ASPM wouldn't have much consequence, but
> > > > not updating OPP would be.
> > > > 
> > > > Let me think of other ways to call these two APIs during the device addition. If
> > > > there are no sane ways, I'll drop *this* patch.
> > > > 
> > > How about using enable_device in host bridge, without pci_enable_device
> > > call the endpoints can't start the transfers. May be we can use that.
> > > 
> > 
> > Q: Who is going to call pci_enable_device()?
> > A: The PCI client driver
> > 
> > This is same as relying on BUS_NOTIFY_BIND_DRIVER notifier.
> > 
> userspace can enable device using sysfs[1] without attaching
> any kernel drivers.
> 

But that's not a common usecase. Even so, we cannot insist users to write to the
sysfs knob to let ASPM/OPP work without a driver.

- Mani

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

Re: [PATCH 2/2] PCI: qcom: Move qcom_pcie_icc_opp_update() to notifier callback

[Konrad Dybcio]

On 7/16/25 7:28 AM, Manivannan Sadhasivam wrote:
> On Tue, Jul 15, 2025 at 12:45:36PM GMT, Konrad Dybcio wrote:
>> On 7/15/25 12:36 PM, Manivannan Sadhasivam wrote:
>>> On Tue, Jul 15, 2025 at 11:54:48AM GMT, Konrad Dybcio wrote:
>>>> On 7/14/25 8:01 PM, Manivannan Sadhasivam wrote:
>>>>> It allows us to group all the settings that need to be done when a PCI
>>>>> device is attached to the bus in a single place.
>>>>>
>>>>> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>
>>>>> ---

[...]

>>> Let me think of other ways to call these two APIs during the device addition. If
>>> there are no sane ways, I'll drop *this* patch.
>>
>> Would it be too naive to assume BUS_NOTIFY_ADD_DEVICE is a good fit?
> 
> BUS_NOTIFY_ADD_DEVICE is not currently a good fit as ASPM link state
> initialization happen after all the devices are enumerated for the slot. This is
> something to be fixed in the PCI core and would allow us to use
> BUS_NOTIFY_ADD_DEVICE.
> 
> I talked to Bjorn H and we both agreed that this needs to be revisited. But I'm
> just worrried that until this happens, we cannot upstream the ASPM fix and not
> even backport it to 6.16/16.
> 
> So maybe we need to resort to this patch as an interim fix if everyone agrees.

I'm not opposed if there's going to be an improved solution next cycle.
Having ASPM 99.9% of the time is much better than not having it at all

> 
>> Do
>> ASPM setting need to be reapplied after the PCIe device is reset? (well
>> I would assume there are probably multiple levels of "reset" :/)
>>
> 
> I'm assuming that you are referring to link down reset here. PCI core takes care
> of saving both the endpoint as well as Root Port config space when that happens
> and restores them afterwards.

Nice, thanks for confirming

Konrad

Re: [PATCH 2/2] PCI: qcom: Move qcom_pcie_icc_opp_update() to notifier callback

[Johan Hovold]

On Wed, Jul 16, 2025 at 12:16:42PM +0530, Manivannan Sadhasivam wrote:
> On Wed, Jul 16, 2025 at 10:24:23AM GMT, Krishna Chaitanya Chundru wrote:

> > How about using enable_device in host bridge, without pci_enable_device
> > call the endpoints can't start the transfers. May be we can use that.
> 
> Q: Who is going to call pci_enable_device()?
> A: The PCI client driver
> 
> This is same as relying on BUS_NOTIFY_BIND_DRIVER notifier.

It seems to me that enable_device() may be a better fit if we're only
going to enable ASPM for devices with a driver (or when enabled through
sysfs).

PCI core will already have placed the device in D0, and this avoids
dealing with notifiers.

Johan

Re: [PATCH 2/2] PCI: qcom: Move qcom_pcie_icc_opp_update() to notifier callback

[Johan Hovold]

On Wed, Jul 16, 2025 at 12:33:48PM +0200, Konrad Dybcio wrote:
> On 7/16/25 7:28 AM, Manivannan Sadhasivam wrote:
> > On Tue, Jul 15, 2025 at 12:45:36PM GMT, Konrad Dybcio wrote:
> >> On 7/15/25 12:36 PM, Manivannan Sadhasivam wrote:
> >>> On Tue, Jul 15, 2025 at 11:54:48AM GMT, Konrad Dybcio wrote:
> >>>> On 7/14/25 8:01 PM, Manivannan Sadhasivam wrote:
> >>>>> It allows us to group all the settings that need to be done when a PCI
> >>>>> device is attached to the bus in a single place.
> >>>>>
> >>>>> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>
> >>>>> ---
> 
> [...]
> 
> >>> Let me think of other ways to call these two APIs during the device addition. If
> >>> there are no sane ways, I'll drop *this* patch.
> >>
> >> Would it be too naive to assume BUS_NOTIFY_ADD_DEVICE is a good fit?
> > 
> > BUS_NOTIFY_ADD_DEVICE is not currently a good fit as ASPM link state
> > initialization happen after all the devices are enumerated for the slot. This is
> > something to be fixed in the PCI core and would allow us to use
> > BUS_NOTIFY_ADD_DEVICE.
> > 
> > I talked to Bjorn H and we both agreed that this needs to be revisited. But I'm
> > just worrried that until this happens, we cannot upstream the ASPM fix and not
> > even backport it to 6.16/16.
> > 
> > So maybe we need to resort to this patch as an interim fix if everyone agrees.
> 
> I'm not opposed if there's going to be an improved solution next cycle.
> Having ASPM 99.9% of the time is much better than not having it at all

This has been broken since 6.15 (not 6.13 as the commit message of patch
1/2 suggests) so there's no rush to get it sorted in 6.16.

The current approach also works for everything but devices using pwrctrl
(read: anything but ath11k/ath12k).

It seems like adding an enable_device() callback can be used as minimal,
backportable fix for the ath11k/ath12k regression on Qualcomm platforms,
while working on something more general (e.g. also handling the OPPs) if
that turns out to be more invasive.

Johan

Re: [PATCH 1/2] PCI: qcom: Switch to bus notifier for enabling ASPM of PCI devices

[Johan Hovold]

On Mon, Jul 14, 2025 at 11:31:04PM +0530, Manivannan Sadhasivam wrote:
> Commit 9f4f3dfad8cf ("PCI: qcom: Enable ASPM for platforms supporting 1.9.0
> ops") allowed the Qcom controller driver to enable ASPM for all PCI devices
> enumerated at the time of the controller driver probe. It proved to be
> useful for devices already powered on by the bootloader as it allowed
> devices to enter ASPM without user intervention.
> 
> However, it could not enable ASPM for the hotplug capable devices i.e.,
> devices enumerated *after* the controller driver probe. This limitation
> mostly went unnoticed as the Qcom PCI controllers are not hotplug capable
> and also the bootloader has been enabling the PCI devices before Linux
> Kernel boots (mostly on the Qcom compute platforms which users use on a
> daily basis).
> 
> But with the advent of the commit b458ff7e8176 ("PCI/pwrctl: Ensure that
> pwrctl drivers are probed before PCI client drivers"), the pwrctrl driver
> started to block the PCI device enumeration until it had been probed.
> Though, the intention of the commit was to avoid race between the pwrctrl
> driver and PCI client driver, it also meant that the pwrctrl controlled PCI
> devices may get probed after the controller driver and will no longer have
> ASPM enabled. So users started noticing high runtime power consumption with
> WLAN chipsets on Qcom compute platforms like Thinkpad X13s, and Thinkpad
> T14s, etc...

Note the ASPM regression for ath11k/ath12k only happened in 6.15, so
commit b458ff7e8176 ("PCI/pwrctl: Ensure that pwrctl drivers are probed
before PCI client drivers") in 6.13 does not seem to be the immediate
culprit here.

Candidates from 6.15 include commits like

	957f40d039a9 ("PCI/pwrctrl: Move creation of pwrctrl devices to pci_scan_device()")
	2489eeb777af ("PCI/pwrctrl: Skip scanning for the device further if pwrctrl device is created")

This is probably related to the reports of these drivers sometimes
failing to probe with

	ath12k_pci 0004:01:00.0: of_irq_parse_pci: failed with rc=134

after pwrctrl was merged, and which since 6.15 should instead result in
the drivers not probing at all (as we've discussed off list).

> Obviously, it is the pwrctrl change that caused regression, but it
> ultimately uncovered a flaw in the ASPM enablement logic of the controller
> driver. So to address the actual issue, switch to the bus notifier for
> enabling ASPM of the PCI devices. The notifier will notify the controller
> driver when a PCI device is attached to the bus, thereby allowing it to
> enable ASPM more reliably. It should be noted that the
> 'pci_dev::link_state', which is required for enabling ASPM by the
> pci_enable_link_state_locked() API, is only set by the time of
> BUS_NOTIFY_BIND_DRIVER stage of the notification. So we cannot enable ASPM
> during BUS_NOTIFY_ADD_DEVICE stage.
> 
> So with this, we can also get rid of the controller driver specific
> 'qcom_pcie_ops::host_post_init' callback.
> 
> Cc: stable@vger.kernel.org # v6.7
> Fixes: 9f4f3dfad8cf ("PCI: qcom: Enable ASPM for platforms supporting 1.9.0 ops")

So whatever form this fix ends up taking it only needs to be backported
to 6.15.

As you mention above these platforms do not support hotplug, but even if
they were, enabling ASPM for hotplugged devices is arguably more of a
new features than a bug fix.

Johan

Re: [PATCH 1/2] PCI: qcom: Switch to bus notifier for enabling ASPM of PCI devices

[Manivannan Sadhasivam]

On Mon, Jul 21, 2025 at 11:32:44AM GMT, Johan Hovold wrote:
> On Mon, Jul 14, 2025 at 11:31:04PM +0530, Manivannan Sadhasivam wrote:
> > Commit 9f4f3dfad8cf ("PCI: qcom: Enable ASPM for platforms supporting 1.9.0
> > ops") allowed the Qcom controller driver to enable ASPM for all PCI devices
> > enumerated at the time of the controller driver probe. It proved to be
> > useful for devices already powered on by the bootloader as it allowed
> > devices to enter ASPM without user intervention.
> > 
> > However, it could not enable ASPM for the hotplug capable devices i.e.,
> > devices enumerated *after* the controller driver probe. This limitation
> > mostly went unnoticed as the Qcom PCI controllers are not hotplug capable
> > and also the bootloader has been enabling the PCI devices before Linux
> > Kernel boots (mostly on the Qcom compute platforms which users use on a
> > daily basis).
> > 
> > But with the advent of the commit b458ff7e8176 ("PCI/pwrctl: Ensure that
> > pwrctl drivers are probed before PCI client drivers"), the pwrctrl driver
> > started to block the PCI device enumeration until it had been probed.
> > Though, the intention of the commit was to avoid race between the pwrctrl
> > driver and PCI client driver, it also meant that the pwrctrl controlled PCI
> > devices may get probed after the controller driver and will no longer have
> > ASPM enabled. So users started noticing high runtime power consumption with
> > WLAN chipsets on Qcom compute platforms like Thinkpad X13s, and Thinkpad
> > T14s, etc...
> 
> Note the ASPM regression for ath11k/ath12k only happened in 6.15, so
> commit b458ff7e8176 ("PCI/pwrctl: Ensure that pwrctl drivers are probed
> before PCI client drivers") in 6.13 does not seem to be the immediate
> culprit here.
> 

This series was intented to fix the ASPM issue which exist even before the
introduction of pwrctrl framework. But I also agree that the below commits made
the issue more visible and caused regression on platforms where WLAN used to
work.

> Candidates from 6.15 include commits like
> 
> 	957f40d039a9 ("PCI/pwrctrl: Move creation of pwrctrl devices to pci_scan_device()")
> 	2489eeb777af ("PCI/pwrctrl: Skip scanning for the device further if pwrctrl device is created")
> 
> This is probably related to the reports of these drivers sometimes
> failing to probe with
> 
> 	ath12k_pci 0004:01:00.0: of_irq_parse_pci: failed with rc=134
> 
> after pwrctrl was merged, and which since 6.15 should instead result in
> the drivers not probing at all (as we've discussed off list).
> 

We discussed about the ASPM issue IIRC. The above mentioned of_irq_parse_pci
could also be related to the fact that we are turning off the supplies after
pci_dev destruction. For this issue, I guess the patch from Brian could be the
fix:

https://lore.kernel.org/linux-pci/20250711174332.1.I623f788178c1e4c5b1a41dbfc8c7fa55966373c0@changeid/

> > Obviously, it is the pwrctrl change that caused regression, but it
> > ultimately uncovered a flaw in the ASPM enablement logic of the controller
> > driver. So to address the actual issue, switch to the bus notifier for
> > enabling ASPM of the PCI devices. The notifier will notify the controller
> > driver when a PCI device is attached to the bus, thereby allowing it to
> > enable ASPM more reliably. It should be noted that the
> > 'pci_dev::link_state', which is required for enabling ASPM by the
> > pci_enable_link_state_locked() API, is only set by the time of
> > BUS_NOTIFY_BIND_DRIVER stage of the notification. So we cannot enable ASPM
> > during BUS_NOTIFY_ADD_DEVICE stage.
> > 
> > So with this, we can also get rid of the controller driver specific
> > 'qcom_pcie_ops::host_post_init' callback.
> > 
> > Cc: stable@vger.kernel.org # v6.7
> > Fixes: 9f4f3dfad8cf ("PCI: qcom: Enable ASPM for platforms supporting 1.9.0 ops")
> 
> So whatever form this fix ends up taking it only needs to be backported
> to 6.15.
> 
> As you mention above these platforms do not support hotplug, but even if
> they were, enabling ASPM for hotplugged devices is arguably more of a
> new features than a bug fix.
> 

FYI, I'm going to drop this series in favor this (with one yet-to-be-submitted
patch on top):
https://lore.kernel.org/linux-pci/20250720190140.2639200-1-david.e.box@linux.intel.com/

- Mani

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

Re: [PATCH 2/2] PCI: qcom: Move qcom_pcie_icc_opp_update() to notifier callback

[Manivannan Sadhasivam]

On Mon, Jul 21, 2025 at 11:02:01AM GMT, Johan Hovold wrote:
> On Wed, Jul 16, 2025 at 12:16:42PM +0530, Manivannan Sadhasivam wrote:
> > On Wed, Jul 16, 2025 at 10:24:23AM GMT, Krishna Chaitanya Chundru wrote:
> 
> > > How about using enable_device in host bridge, without pci_enable_device
> > > call the endpoints can't start the transfers. May be we can use that.
> > 
> > Q: Who is going to call pci_enable_device()?
> > A: The PCI client driver
> > 
> > This is same as relying on BUS_NOTIFY_BIND_DRIVER notifier.
> 
> It seems to me that enable_device() may be a better fit if we're only
> going to enable ASPM for devices with a driver (or when enabled through
> sysfs).
> 
> PCI core will already have placed the device in D0, and this avoids
> dealing with notifiers.
> 

I'm planning to drop this series in favor of this patch (with one
yet-to-be-submitted patch for pcie-qcom on top):

https://lore.kernel.org/linux-pci/20250720190140.2639200-1-david.e.box@linux.intel.com/

This patch is more elegant compared to this series and also avoids the issue
we are discussing.

- Mani

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

Re: [PATCH 1/2] PCI: qcom: Switch to bus notifier for enabling ASPM of PCI devices

[Johan Hovold]

On Mon, Jul 21, 2025 at 04:26:41PM +0530, Manivannan Sadhasivam wrote:
> On Mon, Jul 21, 2025 at 11:32:44AM GMT, Johan Hovold wrote:
> > On Mon, Jul 14, 2025 at 11:31:04PM +0530, Manivannan Sadhasivam wrote:
> > > Commit 9f4f3dfad8cf ("PCI: qcom: Enable ASPM for platforms supporting 1.9.0
> > > ops") allowed the Qcom controller driver to enable ASPM for all PCI devices
> > > enumerated at the time of the controller driver probe. It proved to be
> > > useful for devices already powered on by the bootloader as it allowed
> > > devices to enter ASPM without user intervention.
> > > 
> > > However, it could not enable ASPM for the hotplug capable devices i.e.,
> > > devices enumerated *after* the controller driver probe. This limitation
> > > mostly went unnoticed as the Qcom PCI controllers are not hotplug capable
> > > and also the bootloader has been enabling the PCI devices before Linux
> > > Kernel boots (mostly on the Qcom compute platforms which users use on a
> > > daily basis).
> > > 
> > > But with the advent of the commit b458ff7e8176 ("PCI/pwrctl: Ensure that
> > > pwrctl drivers are probed before PCI client drivers"), the pwrctrl driver
> > > started to block the PCI device enumeration until it had been probed.
> > > Though, the intention of the commit was to avoid race between the pwrctrl
> > > driver and PCI client driver, it also meant that the pwrctrl controlled PCI
> > > devices may get probed after the controller driver and will no longer have
> > > ASPM enabled. So users started noticing high runtime power consumption with
> > > WLAN chipsets on Qcom compute platforms like Thinkpad X13s, and Thinkpad
> > > T14s, etc...
> > 
> > Note the ASPM regression for ath11k/ath12k only happened in 6.15, so
> > commit b458ff7e8176 ("PCI/pwrctl: Ensure that pwrctl drivers are probed
> > before PCI client drivers") in 6.13 does not seem to be the immediate
> > culprit here.
> 
> This series was intented to fix the ASPM issue which exist even before the
> introduction of pwrctrl framework.

But this limitation of the ASPM enable implementation wasn't really an
issue before pwrctrl since, as you point out above, these controllers
are not hotplug capable.

> But I also agree that the below commits made
> the issue more visible and caused regression on platforms where WLAN used to
> work.
> 
> > Candidates from 6.15 include commits like
> > 
> > 	957f40d039a9 ("PCI/pwrctrl: Move creation of pwrctrl devices to pci_scan_device()")
> > 	2489eeb777af ("PCI/pwrctrl: Skip scanning for the device further if pwrctrl device is created")
> > 
> > This is probably related to the reports of these drivers sometimes
> > failing to probe with
> > 
> > 	ath12k_pci 0004:01:00.0: of_irq_parse_pci: failed with rc=134
> > 
> > after pwrctrl was merged, and which since 6.15 should instead result in
> > the drivers not probing at all (as we've discussed off list).
> 
> We discussed about the ASPM issue IIRC. The above mentioned of_irq_parse_pci
> could also be related to the fact that we are turning off the supplies after
> pci_dev destruction. For this issue, I guess the patch from Brian could be the
> fix:
> 
> https://lore.kernel.org/linux-pci/20250711174332.1.I623f788178c1e4c5b1a41dbfc8c7fa55966373c0@changeid/

We've also discussed the rc=134 issue, which appears to be due to some
pwrctrl race. IIRC, you thought it may be the bluetooth driver powering
down the bt/wlan controller before the wlan bit has had a chance to
(complete its) probe. Not sure if that was fully confirmed, but I
remember you saying that the rc=134 symptom would no longer be visible
since 6.15 and instead wlan would never even probe at all if we hit this
issue...

The patch you link to above only appears to relate to drivers being
manually unbound. I hope we're not also hitting such issues during
regular boot?

> > > Obviously, it is the pwrctrl change that caused regression, but it
> > > ultimately uncovered a flaw in the ASPM enablement logic of the controller
> > > driver. So to address the actual issue, switch to the bus notifier for
> > > enabling ASPM of the PCI devices. The notifier will notify the controller
> > > driver when a PCI device is attached to the bus, thereby allowing it to
> > > enable ASPM more reliably. It should be noted that the
> > > 'pci_dev::link_state', which is required for enabling ASPM by the
> > > pci_enable_link_state_locked() API, is only set by the time of
> > > BUS_NOTIFY_BIND_DRIVER stage of the notification. So we cannot enable ASPM
> > > during BUS_NOTIFY_ADD_DEVICE stage.
> > > 
> > > So with this, we can also get rid of the controller driver specific
> > > 'qcom_pcie_ops::host_post_init' callback.
> > > 
> > > Cc: stable@vger.kernel.org # v6.7
> > > Fixes: 9f4f3dfad8cf ("PCI: qcom: Enable ASPM for platforms supporting 1.9.0 ops")
> > 
> > So whatever form this fix ends up taking it only needs to be backported
> > to 6.15.
> > 
> > As you mention above these platforms do not support hotplug, but even if
> > they were, enabling ASPM for hotplugged devices is arguably more of a
> > new features than a bug fix.
> 
> FYI, I'm going to drop this series in favor this (with one yet-to-be-submitted
> patch on top):
> https://lore.kernel.org/linux-pci/20250720190140.2639200-1-david.e.box@linux.intel.com/

Sounds good. Thanks.

Johan

Re: [PATCH 1/2] PCI: qcom: Switch to bus notifier for enabling ASPM of PCI devices

[Manivannan Sadhasivam]

On Mon, Jul 21, 2025 at 02:49:14PM GMT, Johan Hovold wrote:
> On Mon, Jul 21, 2025 at 04:26:41PM +0530, Manivannan Sadhasivam wrote:
> > On Mon, Jul 21, 2025 at 11:32:44AM GMT, Johan Hovold wrote:
> > > On Mon, Jul 14, 2025 at 11:31:04PM +0530, Manivannan Sadhasivam wrote:
> > > > Commit 9f4f3dfad8cf ("PCI: qcom: Enable ASPM for platforms supporting 1.9.0
> > > > ops") allowed the Qcom controller driver to enable ASPM for all PCI devices
> > > > enumerated at the time of the controller driver probe. It proved to be
> > > > useful for devices already powered on by the bootloader as it allowed
> > > > devices to enter ASPM without user intervention.
> > > > 
> > > > However, it could not enable ASPM for the hotplug capable devices i.e.,
> > > > devices enumerated *after* the controller driver probe. This limitation
> > > > mostly went unnoticed as the Qcom PCI controllers are not hotplug capable
> > > > and also the bootloader has been enabling the PCI devices before Linux
> > > > Kernel boots (mostly on the Qcom compute platforms which users use on a
> > > > daily basis).
> > > > 
> > > > But with the advent of the commit b458ff7e8176 ("PCI/pwrctl: Ensure that
> > > > pwrctl drivers are probed before PCI client drivers"), the pwrctrl driver
> > > > started to block the PCI device enumeration until it had been probed.
> > > > Though, the intention of the commit was to avoid race between the pwrctrl
> > > > driver and PCI client driver, it also meant that the pwrctrl controlled PCI
> > > > devices may get probed after the controller driver and will no longer have
> > > > ASPM enabled. So users started noticing high runtime power consumption with
> > > > WLAN chipsets on Qcom compute platforms like Thinkpad X13s, and Thinkpad
> > > > T14s, etc...
> > > 
> > > Note the ASPM regression for ath11k/ath12k only happened in 6.15, so
> > > commit b458ff7e8176 ("PCI/pwrctl: Ensure that pwrctl drivers are probed
> > > before PCI client drivers") in 6.13 does not seem to be the immediate
> > > culprit here.
> > 
> > This series was intented to fix the ASPM issue which exist even before the
> > introduction of pwrctrl framework.
> 
> But this limitation of the ASPM enable implementation wasn't really an
> issue before pwrctrl since, as you point out above, these controllers
> are not hotplug capable.
> 

Yeah, but nothing prevented an user from powering on the endpoint later and
doing manual rescan.

> > But I also agree that the below commits made
> > the issue more visible and caused regression on platforms where WLAN used to
> > work.
> > 
> > > Candidates from 6.15 include commits like
> > > 
> > > 	957f40d039a9 ("PCI/pwrctrl: Move creation of pwrctrl devices to pci_scan_device()")
> > > 	2489eeb777af ("PCI/pwrctrl: Skip scanning for the device further if pwrctrl device is created")
> > > 
> > > This is probably related to the reports of these drivers sometimes
> > > failing to probe with
> > > 
> > > 	ath12k_pci 0004:01:00.0: of_irq_parse_pci: failed with rc=134
> > > 
> > > after pwrctrl was merged, and which since 6.15 should instead result in
> > > the drivers not probing at all (as we've discussed off list).
> > 
> > We discussed about the ASPM issue IIRC. The above mentioned of_irq_parse_pci
> > could also be related to the fact that we are turning off the supplies after
> > pci_dev destruction. For this issue, I guess the patch from Brian could be the
> > fix:
> > 
> > https://lore.kernel.org/linux-pci/20250711174332.1.I623f788178c1e4c5b1a41dbfc8c7fa55966373c0@changeid/
> 
> We've also discussed the rc=134 issue, which appears to be due to some
> pwrctrl race. IIRC, you thought it may be the bluetooth driver powering
> down the bt/wlan controller before the wlan bit has had a chance to
> (complete its) probe. Not sure if that was fully confirmed, but I
> remember you saying that the rc=134 symptom would no longer be visible
> since 6.15 and instead wlan would never even probe at all if we hit this
> issue...
> 

Ah yes, this one was *before* the ASPM discussion we had.

> The patch you link to above only appears to relate to drivers being
> manually unbound. I hope we're not also hitting such issues during
> regular boot?
> 

The patch makes sure that the pwrctrl doesn't power off the endpoint when
'struct pci_dev' gets destroyed. But thinking more, I'm not sure if that's
what happenning during the 'of_irq_parse_pci' issue.

I need to dig more at some point.

- Mani

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