Re: [RFC PATCH v1 4/4] PCI/ASPM: Remove struct pcie_link_state.clkpm_disable

From: Saheed Bolarinwa <refactormyself@gmail.com>
To: Bjorn Helgaas <helgaas@kernel.org>
Cc: linux-pci@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [RFC PATCH v1 4/4] PCI/ASPM: Remove struct pcie_link_state.clkpm_disable
Date: Mon, 11 Oct 2021 08:06:09 +0200	[thread overview]
Message-ID: <CABGxfO6MTXX-NmrU+N55-c-F4nrbwkg8YD9zF7NjFvuhEXbMzA@mail.gmail.com> (raw)
In-Reply-To: <20210930202000.GA906085@bhelgaas>

Thank you for the review.

On Thu, Sep 30, 2021 at 10:20 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
>
> On Wed, Sep 29, 2021 at 02:44:00AM +0200, Saheed O. Bolarinwa wrote:
> > From: "Bolarinwa O. Saheed" <refactormyself@gmail.com>
> >
> > 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@gmail.com>
> > ---
> >  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
> >