On Wed, Jul 16, 2025 at 06:26:21PM +0530, Manivannan Sadhasivam via B4 Relay
wrote:
> From: Manivannan Sadhasivam <manivannan.sadhasi...@oss.qualcomm.com>
>
> Both of the current callers of the pci_enable_link_state_locked() API
> transition the device to D0 before calling. This aligns with the PCIe spec
> r6.0, sec 5.5.4:
>
> "If setting either or both of the enable bits for PCI-PM L1 PM Substates,
> both ports must be configured as described in this section while in D0."
>
> But it looks redundant to let the callers transition the device to D0. So
> move the logic inside the API and perform D0 transition only if the PCI-PM
> L1 Substates are getting enabled.
> +++ b/drivers/pci/pcie/aspm.c
> @@ -1474,13 +1474,20 @@ static int __pci_enable_link_state(struct pci_dev
> *pdev, int state, bool locked)
> * 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.
> + * Note: The device will be transitioned to D0 state if the PCI-PM L1
> Substates
> + * are getting enabled.
> *
> * Return: 0 on success, a negative errno otherwise.
> */
> int pci_enable_link_state(struct pci_dev *pdev, int state)
> {
> + /*
> + * Ensure the device is in D0 before enabling PCI-PM L1 PM Substates,
> per
> + * PCIe r6.0, sec 5.5.4.
> + */
> + if (FIELD_GET(PCIE_LINK_STATE_L1_SS_PCIPM, state))
> + pci_set_power_state(pdev, PCI_D0);
This is really just a move, not new code, but this niggles at me a
little bit because my impression is that pci_set_power_state() doesn't
guarantee that the device *stays* in the given state.
Rafael, is there a get/put interface we should be wrapping this with
instead?
I'm also not sure it's worth the FIELD_GET(). This should be a
low-frequency operation and making the power state dependent on the
exact "state" makes more paths to worry about.
> return __pci_enable_link_state(pdev, state, false);
> }
