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@xxxxxxxxxxxxxxxx>
> >
> > 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@xxxxxxxxxxxxxxx>
>
> 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@xxxxxxxxxxxxxxxx>
> > Signed-off-by: Krishna Chaitanya Chundru <krishna.chundru@xxxxxxxxxxxxxxxx>
> > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@xxxxxxxxxxxxxxxx>
> > ---
> > 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)
> > {
> >
> >
