On Wed, Sep 29, 2021 at 02:44:00AM +0200, Saheed O. Bolarinwa wrote:
> From: "Bolarinwa O. Saheed" <refactormyself@xxxxxxxxx>
>
> The clkpm_disable member of the struct pcie_link_state indicates
> if the Clock PM state of the device is disabled. There are two
> situations which can cause the Clock PM state disabled.
> 1. If the device fails sanity check as in pcie_aspm_sanity_check()
> 2. By calling __pci_disable_link_state()
And, 3. clkpm_store(), when the user writes to the "clkpm" sysfs file,
right?
IIUC, clkpm_disable really tells us whether we can enable clkpm. The
only place we test clkpm_disable is in pcie_set_clkpm():
pcie_set_clkpm(struct pcie_link_state *link, int enable)
{
if (!link->clkpm_capable || link->clkpm_disable)
enable = 0;
pcie_set_clkpm_nocheck(link, enable);
}
So in other words, if clkpm_disable is set, we will never call
pcie_set_clkpm_nocheck() to *enable* clkpm. We will only call it to
*disable* clkpm.
Tangent: I think the usefulness of pcie_set_clkpm_nocheck() being a
separate function is gone. I think things will be a little simpler if
we integrate it into pcie_set_clkpm(). Separate preliminary patch, of
course.
> It is possible to set the Clock PM state of a device ON or OFF by
> calling pcie_set_clkpm(). The state can be retieved by calling
> pcie_get_clkpm_state().
s/retieved/retrieved/
> pcie_link_state.clkpm_disable is only accessed in pcie_set_clkpm()
> to ensure that Clock PM state can be reenabled after being disabled.
>
> This patch:
> - add pm_disable to the struct pcie_link_state, to indicate that
> the kernel has marked the device's AS and Clock PM states disabled
> - removes clkpm_disable from the struct pcie_link_state
> - removes all instance where clkpm_disable is set
> - ensure that the Clock PM is always disabled if it is part of the
> states passed into __pci_disable_link_state(), regardless of the
> global policy
>
> Signed-off-by: Bolarinwa O. Saheed <refactormyself@xxxxxxxxx>
> ---
> drivers/pci/pcie/aspm.c | 18 +++++-------------
> 1 file changed, 5 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
> index 368828cd427d..e6ae00daa7ae 100644
> --- a/drivers/pci/pcie/aspm.c
> +++ b/drivers/pci/pcie/aspm.c
> @@ -60,8 +60,7 @@ struct pcie_link_state {
> u32 aspm_default:7; /* Default ASPM state by BIOS */
> u32 aspm_disable:7; /* Disabled ASPM state */
>
> - /* Clock PM state */
> - u32 clkpm_disable:1; /* Clock PM disabled */
> + u32 pm_disabled:1; /* Disabled AS and Clock PM ? */
What did we gain by renaming this? AFAICT this only affects clkpm
(the only test of pm_disabled is in pcie_set_clkpm()).
> /* Exit latencies */
> struct aspm_latency latency_up; /* Upstream direction exit latency */
> @@ -198,7 +197,7 @@ static void pcie_set_clkpm(struct pcie_link_state *link, int enable)
> * Don't enable Clock PM if the link is not Clock PM capable
> * or Clock PM is disabled
> */
> - if (!capable || link->clkpm_disable)
> + if (enable && (!capable || link->pm_disabled))
> enable = 0;
> /* Need nothing if the specified equals to current state */
> if (pcie_get_clkpm_state(link->pdev) == enable)
> @@ -206,11 +205,6 @@ static void pcie_set_clkpm(struct pcie_link_state *link, int enable)
> pcie_set_clkpm_nocheck(link, enable);
> }
>
> -static void pcie_clkpm_cap_init(struct pcie_link_state *link, int blacklist)
> -{
> - link->clkpm_disable = blacklist ? 1 : 0;
> -}
> -
> static bool pcie_retrain_link(struct pcie_link_state *link)
> {
> struct pci_dev *parent = link->pdev;
> @@ -952,8 +946,7 @@ void pcie_aspm_init_link_state(struct pci_dev *pdev)
> */
> pcie_aspm_cap_init(link, blacklist);
>
> - /* Setup initial Clock PM state */
> - pcie_clkpm_cap_init(link, blacklist);
> + link->pm_disabled = blacklist;
>
> /*
> * At this stage drivers haven't had an opportunity to change the
> @@ -1129,8 +1122,8 @@ static int __pci_disable_link_state(struct pci_dev *pdev, int state, bool sem)
> pcie_config_aspm_link(link, policy_to_aspm_state(link));
>
> if (state & PCIE_LINK_STATE_CLKPM)
> - link->clkpm_disable = 1;
> - pcie_set_clkpm(link, policy_to_clkpm_state(link));
> + pcie_set_clkpm(link, 0);
> +
> mutex_unlock(&aspm_lock);
> if (sem)
> up_read(&pci_bus_sem);
> @@ -1301,7 +1294,6 @@ static ssize_t clkpm_store(struct device *dev,
> down_read(&pci_bus_sem);
> mutex_lock(&aspm_lock);
>
> - link->clkpm_disable = !state_enable;
Something is seriously wrong here because clkpm_store() no longer does
anything with "state_enable", the value we got from the user.
> pcie_set_clkpm(link, policy_to_clkpm_state(link));
>
> mutex_unlock(&aspm_lock);
> --
> 2.20.1
>
