From: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>
pci_enable_link_state() and pci_enable_link_state_locked() APIs are
supposed to be symmectric with pci_disable_link_state() and
pci_disable_link_state_locked() APIs.
But unfortunately, they are not symmetric. This behavior was mentioned in
the kernel-doc of these APIs:
" Clear and set the default device link state..."
and
"Also note that this does not enable states disabled by
pci_disable_link_state()"
These APIs won't enable all the states specified by the 'state' parameter,
but only enable the ones not previously disabled by the
pci_disable_link_state*() APIs. But this behavior doesn't align with the
naming of these APIs, as they give the impression that these APIs will
enable all the specified states.
To resolve this ambiguity, allow these APIs to enable the specified states,
regardeless of whether they were previously disabled or not. This is
accomplished by clearing the previously disabled states from the
'link::aspm_disable' parameter in __pci_enable_link_state() helper. Also,
reword the kernel-doc to reflect this behavior.
The current callers of pci_enable_link_state_locked() APIs (vmd and
pcie-qcom) did not disable the ASPM states before calling this API. So it
is evident that they do not depend on the previous behavior of this API and
intend to enable all the specified states.
And the other API, pci_enable_link_state() doesn't have a caller for now,
but will be used by the 'atheros' WLAN drivers in the subsequent commits.
Suggested-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
Co-developed-by: Krishna Chaitanya Chundru <krishna.chundru@oss.qualcomm.com>
Signed-off-by: Krishna Chaitanya Chundru <krishna.chundru@oss.qualcomm.com>
Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>
---
drivers/pci/pcie/aspm.c | 33 ++++++++++++++++++---------------
1 file changed, 18 insertions(+), 15 deletions(-)
diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
index be9bd272057c3472f3e31dc9568340b19d52012a..fac46113a90c7fac6c97125e6a7e385045780005 100644
--- a/drivers/pci/pcie/aspm.c
+++ b/drivers/pci/pcie/aspm.c
@@ -1459,6 +1459,7 @@ static int __pci_enable_link_state(struct pci_dev *pdev, int state, bool locked)
down_read(&pci_bus_sem);
mutex_lock(&aspm_lock);
link->aspm_default = pci_calc_aspm_enable_mask(state);
+ link->aspm_disable &= ~state;
pcie_config_aspm_link(link, policy_to_aspm_state(link));
link->clkpm_default = (state & PCIE_LINK_STATE_CLKPM) ? 1 : 0;
@@ -1471,17 +1472,18 @@ static int __pci_enable_link_state(struct pci_dev *pdev, int state, bool locked)
}
/**
- * pci_enable_link_state - Clear and set the default device link state so that
- * the link may be allowed to enter the specified states. Note that if the
- * BIOS didn't grant ASPM control to the OS, this does nothing because we can't
- * touch the LNKCTL register. Also note that this does not enable states
- * disabled by pci_disable_link_state(). Return 0 or a negative errno.
+ * pci_enable_link_state - Enable device's link state
+ * @pdev: PCI device
+ * @state: Mask of ASPM link states to enable
+ *
+ * Enable device's link state, so the link will enter the specified states.
+ * Note that if the BIOS didn't grant ASPM control to the OS, this does
+ * nothing because we can't touch the LNKCTL register.
*
* Note: Ensure devices are in D0 before enabling PCI-PM L1 PM Substates, per
* PCIe r6.0, sec 5.5.4.
*
- * @pdev: PCI device
- * @state: Mask of ASPM link states to enable
+ * Return: 0 on success, a negative errno otherwise.
*/
int pci_enable_link_state(struct pci_dev *pdev, int state)
{
@@ -1490,19 +1492,20 @@ int pci_enable_link_state(struct pci_dev *pdev, int state)
EXPORT_SYMBOL(pci_enable_link_state);
/**
- * pci_enable_link_state_locked - Clear and set the default device link state
- * so that the link may be allowed to enter the specified states. Note that if
- * the BIOS didn't grant ASPM control to the OS, this does nothing because we
- * can't touch the LNKCTL register. Also note that this does not enable states
- * disabled by pci_disable_link_state(). Return 0 or a negative errno.
+ * pci_enable_link_state_locked - Enable device's link state
+ * @pdev: PCI device
+ * @state: Mask of ASPM link states to enable
+ *
+ * Enable device's link state, so the link will enter the specified states.
+ * Note that if the BIOS didn't grant ASPM control to the OS, this does
+ * nothing because we can't touch the LNKCTL register.
*
* Note: Ensure devices are in D0 before enabling PCI-PM L1 PM Substates, per
* PCIe r6.0, sec 5.5.4.
*
- * @pdev: PCI device
- * @state: Mask of ASPM link states to enable
- *
* Context: Caller holds pci_bus_sem read lock.
+ *
+ * Return: 0 on success, a negative errno otherwise.
*/
int pci_enable_link_state_locked(struct pci_dev *pdev, int state)
{
--
2.45.2
+David
On Mon, 25 Aug 2025, Manivannan Sadhasivam via B4 Relay wrote:
> From: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>
>
> pci_enable_link_state() and pci_enable_link_state_locked() APIs are
> supposed to be symmectric with pci_disable_link_state() and
> pci_disable_link_state_locked() APIs.
>
> But unfortunately, they are not symmetric. This behavior was mentioned in
> the kernel-doc of these APIs:
>
> " Clear and set the default device link state..."
>
> and
>
> "Also note that this does not enable states disabled by
> pci_disable_link_state()"
>
> These APIs won't enable all the states specified by the 'state' parameter,
> but only enable the ones not previously disabled by the
> pci_disable_link_state*() APIs. But this behavior doesn't align with the
> naming of these APIs, as they give the impression that these APIs will
> enable all the specified states.
>
> To resolve this ambiguity, allow these APIs to enable the specified states,
> regardeless of whether they were previously disabled or not. This is
> accomplished by clearing the previously disabled states from the
> 'link::aspm_disable' parameter in __pci_enable_link_state() helper. Also,
> reword the kernel-doc to reflect this behavior.
>
> The current callers of pci_enable_link_state_locked() APIs (vmd and
> pcie-qcom) did not disable the ASPM states before calling this API. So it
> is evident that they do not depend on the previous behavior of this API and
> intend to enable all the specified states.
While it might be "safe" in the sense that ->aspm_disable is not set by
anything, I'm still not sure if overloading this function for two
different use cases is a good idea.
I'd like to hear David's opinion on this as he grasps the ->aspm_default
vs ->aspm_disable thing much better than I do.
> And the other API, pci_enable_link_state() doesn't have a caller for now,
> but will be used by the 'atheros' WLAN drivers in the subsequent commits.
>
> Suggested-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
This tag sound like I'm endorsing this approach which is not the case. I'd
prefer separate functions for each use case, setting aspm_default and
another for the enable state.
--
i.
> Co-developed-by: Krishna Chaitanya Chundru <krishna.chundru@oss.qualcomm.com>
> Signed-off-by: Krishna Chaitanya Chundru <krishna.chundru@oss.qualcomm.com>
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>
> ---
> drivers/pci/pcie/aspm.c | 33 ++++++++++++++++++---------------
> 1 file changed, 18 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
> index be9bd272057c3472f3e31dc9568340b19d52012a..fac46113a90c7fac6c97125e6a7e385045780005 100644
> --- a/drivers/pci/pcie/aspm.c
> +++ b/drivers/pci/pcie/aspm.c
> @@ -1459,6 +1459,7 @@ static int __pci_enable_link_state(struct pci_dev *pdev, int state, bool locked)
> down_read(&pci_bus_sem);
> mutex_lock(&aspm_lock);
> link->aspm_default = pci_calc_aspm_enable_mask(state);
> + link->aspm_disable &= ~state;
> pcie_config_aspm_link(link, policy_to_aspm_state(link));
>
> link->clkpm_default = (state & PCIE_LINK_STATE_CLKPM) ? 1 : 0;
> @@ -1471,17 +1472,18 @@ static int __pci_enable_link_state(struct pci_dev *pdev, int state, bool locked)
> }
>
> /**
> - * pci_enable_link_state - Clear and set the default device link state so that
> - * the link may be allowed to enter the specified states. Note that if the
> - * BIOS didn't grant ASPM control to the OS, this does nothing because we can't
> - * touch the LNKCTL register. Also note that this does not enable states
> - * disabled by pci_disable_link_state(). Return 0 or a negative errno.
> + * pci_enable_link_state - Enable device's link state
> + * @pdev: PCI device
> + * @state: Mask of ASPM link states to enable
> + *
> + * Enable device's link state, so the link will enter the specified states.
> + * Note that if the BIOS didn't grant ASPM control to the OS, this does
> + * nothing because we can't touch the LNKCTL register.
> *
> * Note: Ensure devices are in D0 before enabling PCI-PM L1 PM Substates, per
> * PCIe r6.0, sec 5.5.4.
> *
> - * @pdev: PCI device
> - * @state: Mask of ASPM link states to enable
> + * Return: 0 on success, a negative errno otherwise.
> */
> int pci_enable_link_state(struct pci_dev *pdev, int state)
> {
> @@ -1490,19 +1492,20 @@ int pci_enable_link_state(struct pci_dev *pdev, int state)
> EXPORT_SYMBOL(pci_enable_link_state);
>
> /**
> - * pci_enable_link_state_locked - Clear and set the default device link state
> - * so that the link may be allowed to enter the specified states. Note that if
> - * the BIOS didn't grant ASPM control to the OS, this does nothing because we
> - * can't touch the LNKCTL register. Also note that this does not enable states
> - * disabled by pci_disable_link_state(). Return 0 or a negative errno.
> + * pci_enable_link_state_locked - Enable device's link state
> + * @pdev: PCI device
> + * @state: Mask of ASPM link states to enable
> + *
> + * Enable device's link state, so the link will enter the specified states.
> + * Note that if the BIOS didn't grant ASPM control to the OS, this does
> + * nothing because we can't touch the LNKCTL register.
> *
> * Note: Ensure devices are in D0 before enabling PCI-PM L1 PM Substates, per
> * PCIe r6.0, sec 5.5.4.
> *
> - * @pdev: PCI device
> - * @state: Mask of ASPM link states to enable
> - *
> * Context: Caller holds pci_bus_sem read lock.
> + *
> + * Return: 0 on success, a negative errno otherwise.
> */
> int pci_enable_link_state_locked(struct pci_dev *pdev, int state)
> {
>
>
On Tue, Aug 26, 2025 at 03:55:42PM GMT, Ilpo Järvinen wrote: > +David > > On Mon, 25 Aug 2025, Manivannan Sadhasivam via B4 Relay wrote: > > > From: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com> > > > > pci_enable_link_state() and pci_enable_link_state_locked() APIs are > > supposed to be symmectric with pci_disable_link_state() and > > pci_disable_link_state_locked() APIs. > > > > But unfortunately, they are not symmetric. This behavior was mentioned in > > the kernel-doc of these APIs: > > > > " Clear and set the default device link state..." > > > > and > > > > "Also note that this does not enable states disabled by > > pci_disable_link_state()" > > > > These APIs won't enable all the states specified by the 'state' parameter, > > but only enable the ones not previously disabled by the > > pci_disable_link_state*() APIs. But this behavior doesn't align with the > > naming of these APIs, as they give the impression that these APIs will > > enable all the specified states. > > > > To resolve this ambiguity, allow these APIs to enable the specified states, > > regardeless of whether they were previously disabled or not. This is > > accomplished by clearing the previously disabled states from the > > 'link::aspm_disable' parameter in __pci_enable_link_state() helper. Also, > > reword the kernel-doc to reflect this behavior. > > > > The current callers of pci_enable_link_state_locked() APIs (vmd and > > pcie-qcom) did not disable the ASPM states before calling this API. So it > > is evident that they do not depend on the previous behavior of this API and > > intend to enable all the specified states. > > While it might be "safe" in the sense that ->aspm_disable is not set by > anything, I'm still not sure if overloading this function for two > different use cases is a good idea. > Why? I thought your concern was with the callers of this API. Since that is taken care, do you have any other concerns? > I'd like to hear David's opinion on this as he grasps the ->aspm_default > vs ->aspm_disable thing much better than I do. > > > And the other API, pci_enable_link_state() doesn't have a caller for now, > > but will be used by the 'atheros' WLAN drivers in the subsequent commits. > > > > Suggested-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> > > This tag sound like I'm endorsing this approach which is not the case. I'd > prefer separate functions for each use case, setting aspm_default and > another for the enable state. > Sorry, I misunderstood then. I'll drop this tag. - Mani -- மணிவண்ணன் சதாசிவம்
On Sat, 6 Sep 2025, Manivannan Sadhasivam wrote: > On Tue, Aug 26, 2025 at 03:55:42PM GMT, Ilpo Järvinen wrote: > > +David > > > > On Mon, 25 Aug 2025, Manivannan Sadhasivam via B4 Relay wrote: > > > > > From: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com> > > > > > > pci_enable_link_state() and pci_enable_link_state_locked() APIs are > > > supposed to be symmectric with pci_disable_link_state() and > > > pci_disable_link_state_locked() APIs. > > > > > > But unfortunately, they are not symmetric. This behavior was mentioned in > > > the kernel-doc of these APIs: > > > > > > " Clear and set the default device link state..." > > > > > > and > > > > > > "Also note that this does not enable states disabled by > > > pci_disable_link_state()" > > > > > > These APIs won't enable all the states specified by the 'state' parameter, > > > but only enable the ones not previously disabled by the > > > pci_disable_link_state*() APIs. But this behavior doesn't align with the > > > naming of these APIs, as they give the impression that these APIs will > > > enable all the specified states. > > > > > > To resolve this ambiguity, allow these APIs to enable the specified states, > > > regardeless of whether they were previously disabled or not. This is > > > accomplished by clearing the previously disabled states from the > > > 'link::aspm_disable' parameter in __pci_enable_link_state() helper. Also, > > > reword the kernel-doc to reflect this behavior. > > > > > > The current callers of pci_enable_link_state_locked() APIs (vmd and > > > pcie-qcom) did not disable the ASPM states before calling this API. So it > > > is evident that they do not depend on the previous behavior of this API and > > > intend to enable all the specified states. > > > > While it might be "safe" in the sense that ->aspm_disable is not set by > > anything, I'm still not sure if overloading this function for two > > different use cases is a good idea. > > > > Why? I thought your concern was with the callers of this API. Since that is > taken care, do you have any other concerns? I don't think it really matters anymore as it looks the vmd one is going to be removed by the David's patch and the qcom one is removed by your patch so no users remain. > > I'd like to hear David's opinion on this as he grasps the ->aspm_default > > vs ->aspm_disable thing much better than I do. > > > > > And the other API, pci_enable_link_state() doesn't have a caller for now, > > > but will be used by the 'atheros' WLAN drivers in the subsequent commits. > > > > > > Suggested-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> > > > > This tag sound like I'm endorsing this approach which is not the case. I'd > > prefer separate functions for each use case, setting aspm_default and > > another for the enable state. > > > > Sorry, I misunderstood then. I'll drop this tag. > > - Mani > > -- i.
On Tue, Aug 26, 2025 at 03:55:42PM +0300, Ilpo Järvinen wrote:
> +David
>
> On Mon, 25 Aug 2025, Manivannan Sadhasivam via B4 Relay wrote:
>
> > From: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>
> >
> > pci_enable_link_state() and pci_enable_link_state_locked() APIs are
> > supposed to be symmectric with pci_disable_link_state() and
> > pci_disable_link_state_locked() APIs.
> >
> > But unfortunately, they are not symmetric. This behavior was mentioned in
> > the kernel-doc of these APIs:
> >
> > " Clear and set the default device link state..."
> >
> > and
> >
> > "Also note that this does not enable states disabled by
> > pci_disable_link_state()"
> >
> > These APIs won't enable all the states specified by the 'state' parameter,
> > but only enable the ones not previously disabled by the
> > pci_disable_link_state*() APIs. But this behavior doesn't align with the
> > naming of these APIs, as they give the impression that these APIs will
> > enable all the specified states.
> >
> > To resolve this ambiguity, allow these APIs to enable the specified states,
> > regardeless of whether they were previously disabled or not. This is
> > accomplished by clearing the previously disabled states from the
> > 'link::aspm_disable' parameter in __pci_enable_link_state() helper. Also,
> > reword the kernel-doc to reflect this behavior.
> >
> > The current callers of pci_enable_link_state_locked() APIs (vmd and
> > pcie-qcom) did not disable the ASPM states before calling this API. So it
> > is evident that they do not depend on the previous behavior of this API and
> > intend to enable all the specified states.
>
> While it might be "safe" in the sense that ->aspm_disable is not set by
> anything, I'm still not sure if overloading this function for two
> different use cases is a good idea.
>
> I'd like to hear David's opinion on this as he grasps the ->aspm_default
> vs ->aspm_disable thing much better than I do.
The concern I see is that this would override the init-time blacklist which is
set in pcie_aspm_sanity_check() and only consulted during initialization.
__pci_disable_link_state() doesn't do this. It ORs in bits to aspm_disable. By
contrast, this change would clear bits from aspm_disable in the enable path,
which allows ASPM to be enabled on links that pcie_aspm_sanity_check()
determined should be disabled.
But I noticed the sysfs path, aspm_attr_store_common(), already permits this
override. That may be unintentional though since the comment in
pcie_aspm_sanity_check() implies the blacklist can only be overridden with
pcie_aspm=force. At minimum, that needs to be clarified.
David
>
> > And the other API, pci_enable_link_state() doesn't have a caller for now,
> > but will be used by the 'atheros' WLAN drivers in the subsequent commits.
> >
> > Suggested-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
>
> This tag sound like I'm endorsing this approach which is not the case. I'd
> prefer separate functions for each use case, setting aspm_default and
> another for the enable state.
>
> --
> i.
>
> > Co-developed-by: Krishna Chaitanya Chundru <krishna.chundru@oss.qualcomm.com>
> > Signed-off-by: Krishna Chaitanya Chundru <krishna.chundru@oss.qualcomm.com>
> > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>
> > ---
> > drivers/pci/pcie/aspm.c | 33 ++++++++++++++++++---------------
> > 1 file changed, 18 insertions(+), 15 deletions(-)
> >
> > diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
> > index be9bd272057c3472f3e31dc9568340b19d52012a..fac46113a90c7fac6c97125e6a7e385045780005 100644
> > --- a/drivers/pci/pcie/aspm.c
> > +++ b/drivers/pci/pcie/aspm.c
> > @@ -1459,6 +1459,7 @@ static int __pci_enable_link_state(struct pci_dev *pdev, int state, bool locked)
> > down_read(&pci_bus_sem);
> > mutex_lock(&aspm_lock);
> > link->aspm_default = pci_calc_aspm_enable_mask(state);
> > + link->aspm_disable &= ~state;
> > pcie_config_aspm_link(link, policy_to_aspm_state(link));
> >
> > link->clkpm_default = (state & PCIE_LINK_STATE_CLKPM) ? 1 : 0;
> > @@ -1471,17 +1472,18 @@ static int __pci_enable_link_state(struct pci_dev *pdev, int state, bool locked)
> > }
> >
> > /**
> > - * pci_enable_link_state - Clear and set the default device link state so that
> > - * the link may be allowed to enter the specified states. Note that if the
> > - * BIOS didn't grant ASPM control to the OS, this does nothing because we can't
> > - * touch the LNKCTL register. Also note that this does not enable states
> > - * disabled by pci_disable_link_state(). Return 0 or a negative errno.
> > + * pci_enable_link_state - Enable device's link state
> > + * @pdev: PCI device
> > + * @state: Mask of ASPM link states to enable
> > + *
> > + * Enable device's link state, so the link will enter the specified states.
> > + * Note that if the BIOS didn't grant ASPM control to the OS, this does
> > + * nothing because we can't touch the LNKCTL register.
> > *
> > * Note: Ensure devices are in D0 before enabling PCI-PM L1 PM Substates, per
> > * PCIe r6.0, sec 5.5.4.
> > *
> > - * @pdev: PCI device
> > - * @state: Mask of ASPM link states to enable
> > + * Return: 0 on success, a negative errno otherwise.
> > */
> > int pci_enable_link_state(struct pci_dev *pdev, int state)
> > {
> > @@ -1490,19 +1492,20 @@ int pci_enable_link_state(struct pci_dev *pdev, int state)
> > EXPORT_SYMBOL(pci_enable_link_state);
> >
> > /**
> > - * pci_enable_link_state_locked - Clear and set the default device link state
> > - * so that the link may be allowed to enter the specified states. Note that if
> > - * the BIOS didn't grant ASPM control to the OS, this does nothing because we
> > - * can't touch the LNKCTL register. Also note that this does not enable states
> > - * disabled by pci_disable_link_state(). Return 0 or a negative errno.
> > + * pci_enable_link_state_locked - Enable device's link state
> > + * @pdev: PCI device
> > + * @state: Mask of ASPM link states to enable
> > + *
> > + * Enable device's link state, so the link will enter the specified states.
> > + * Note that if the BIOS didn't grant ASPM control to the OS, this does
> > + * nothing because we can't touch the LNKCTL register.
> > *
> > * Note: Ensure devices are in D0 before enabling PCI-PM L1 PM Substates, per
> > * PCIe r6.0, sec 5.5.4.
> > *
> > - * @pdev: PCI device
> > - * @state: Mask of ASPM link states to enable
> > - *
> > * Context: Caller holds pci_bus_sem read lock.
> > + *
> > + * Return: 0 on success, a negative errno otherwise.
> > */
> > int pci_enable_link_state_locked(struct pci_dev *pdev, int state)
> > {
> >
> >
On Tue, Aug 26, 2025 at 02:24:05PM GMT, David Box wrote: > On Tue, Aug 26, 2025 at 03:55:42PM +0300, Ilpo Järvinen wrote: > > +David > > > > On Mon, 25 Aug 2025, Manivannan Sadhasivam via B4 Relay wrote: > > > > > From: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com> > > > > > > pci_enable_link_state() and pci_enable_link_state_locked() APIs are > > > supposed to be symmectric with pci_disable_link_state() and > > > pci_disable_link_state_locked() APIs. > > > > > > But unfortunately, they are not symmetric. This behavior was mentioned in > > > the kernel-doc of these APIs: > > > > > > " Clear and set the default device link state..." > > > > > > and > > > > > > "Also note that this does not enable states disabled by > > > pci_disable_link_state()" > > > > > > These APIs won't enable all the states specified by the 'state' parameter, > > > but only enable the ones not previously disabled by the > > > pci_disable_link_state*() APIs. But this behavior doesn't align with the > > > naming of these APIs, as they give the impression that these APIs will > > > enable all the specified states. > > > > > > To resolve this ambiguity, allow these APIs to enable the specified states, > > > regardeless of whether they were previously disabled or not. This is > > > accomplished by clearing the previously disabled states from the > > > 'link::aspm_disable' parameter in __pci_enable_link_state() helper. Also, > > > reword the kernel-doc to reflect this behavior. > > > > > > The current callers of pci_enable_link_state_locked() APIs (vmd and > > > pcie-qcom) did not disable the ASPM states before calling this API. So it > > > is evident that they do not depend on the previous behavior of this API and > > > intend to enable all the specified states. > > > > While it might be "safe" in the sense that ->aspm_disable is not set by > > anything, I'm still not sure if overloading this function for two > > different use cases is a good idea. > > > > I'd like to hear David's opinion on this as he grasps the ->aspm_default > > vs ->aspm_disable thing much better than I do. > > The concern I see is that this would override the init-time blacklist which is > set in pcie_aspm_sanity_check() and only consulted during initialization. > __pci_disable_link_state() doesn't do this. It ORs in bits to aspm_disable. By > contrast, this change would clear bits from aspm_disable in the enable path, > which allows ASPM to be enabled on links that pcie_aspm_sanity_check() > determined should be disabled. > > But I noticed the sysfs path, aspm_attr_store_common(), already permits this > override. That may be unintentional though since the comment in > pcie_aspm_sanity_check() implies the blacklist can only be overridden with > pcie_aspm=force. At minimum, that needs to be clarified. > Thanks for pointing out the blacklist devices issue. I have no concerns with pcie-qcom as we are going to drop the pci_enable_link_state_locked() API anyway from it. But I'm not sure about VMD as one may still connect pre 1.1 device to it and observe issues. So I'll create a separate API for this new behavior and use it with ath drivers only since they know what kind of devices they are dealing with and since they were changing the LNKCTL manually, there shouldn't be any issue. - Mani -- மணிவண்ணன் சதாசிவம்
© 2016 - 2025 Red Hat, Inc.