Thank you for the review.
On Thu, Sep 30, 2021 at 10:20 PM Bjorn Helgaas <helgaas@xxxxxxxxxx> wrote:
>
> 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?
That's right and something went wrong truly. I intend to propose that
instead of setting
link->clkpm_disable = !state_enable;
we can :
if (!state_enable)
pci_disable_link_state_locked(pdev, PCIE_LINK_STATE_CLKPM);
>
> 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.
>
For case 1 (as stated above), this is fine. However, I am concerned that
the intention of cases 2 and 3 may require that it should be possible to
re-enable clkpm after disabling it. Is this the correct perspective?
I think the essence of this patch should have been to show that cases 2 and 3
have the same goal which is different from case 1. Hence, three and not two
checks are needed within pcie_set_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
> >
