Received-SPF: softfail (google.com: domain of transitioning linux-kernel-owner@vger.kernel.org does not designate 23.128.96.19 as permitted sender) client-ip=23.128.96.19;
Date:   Tue, 12 Apr 2022 14:38:10 +0300
From:   Mika Westerberg <mika.westerberg@linux.intel.com>
To:     "Rafael J. Wysocki" <rafael@kernel.org>
Cc:     "Rafael J. Wysocki" <rjw@rjwysocki.net>,
        Linux PCI <linux-pci@vger.kernel.org>,
        Linux PM <linux-pm@vger.kernel.org>,
        LKML <linux-kernel@vger.kernel.org>,
        Bjorn Helgaas <helgaas@kernel.org>
Subject: Re: [PATCH v2 4/9] PCI/PM: Rework changing power states of PCI
 devices
Message-ID: <YlVkojhMtirzzlFy@lahna>
References: <4419002.LvFx2qVVIh@kreacher>
 <11975904.O9o76ZdvQC@kreacher>
 <13011315.uLZWGnKmhe@kreacher>
 <YlVNzZRHW58QRD3c@lahna>
 <CAJZ5v0iKnzHZnx-96CBaeMvz29ezoCBH=F60gM7uAkoBazUzFA@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <CAJZ5v0iKnzHZnx-96CBaeMvz29ezoCBH=F60gM7uAkoBazUzFA@mail.gmail.com>
Organization: Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo
Precedence: bulk

Hi,

On Tue, Apr 12, 2022 at 01:31:57PM +0200, Rafael J. Wysocki wrote:
> On Tue, Apr 12, 2022 at 1:17 PM Mika Westerberg
> <mika.westerberg@linux.intel.com> wrote:
> >
> > On Mon, Apr 11, 2022 at 04:25:12PM +0200, Rafael J. Wysocki wrote:
> > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > >
> > > There are some issues related to changing power states of PCI
> > > devices, mostly related to carrying out unnecessary actions in some
> > > places, and the code is generally hard to follow.
> > >
> > >  1. pci_power_up() has two callers, pci_set_power_state() and
> > >     pci_pm_default_resume_early().  The latter updates the current
> > >     power state of the device right after calling pci_power_up()
> > >     and it restores the entire config space of the device right
> > >     after that, so pci_power_up() itself need not read the
> > >     PCI_PM_CTRL register or restore the BARs after programming the
> > >     device into D0 in that case.
> > >
> > >  2. It is generally hard to get a clear view of the pci_power_up()
> > >     code flow, especially in some corner cases, due to all of the
> > >     involved PCI_PM_CTRL register reads and writes occurring in
> > >     pci_platform_power_transition() and in pci_raw_set_power_state(),
> > >     some of which are redundant.
> > >
> > >  3. The transitions from low-power states to D0 and the other way
> > >     around are unnecessarily tangled in pci_raw_set_power_state()
> > >     which causes it to use a redundant local variable and makes it
> > >     rather hard to follow.
> > >
> > > To address the above shortcomings, make the following changes:
> > >
> > >  a. Remove the code handling transitions into D0
> >
> > Should this be D3?
> 
> No.  Transitions into D0 will be handled by pci_power_up() directly,
> so they need not be handled by pci_raw_set_power_state().

OK.

> > >     from pci_raw_set_power_state() and rename it as
> > >     pci_set_low_power_state().
> > >
> > >  b. Add the code handling transitions into D0 directly
> > >     to pci_power_up() and to a new wrapper function
> > >     pci_set_full_power_state() calling it internally that is
> > >     only used in pci_set_power_state().
> > >
> > >  c. Make pci_power_up() avoid redundant PCI_PM_CTRL register reads
> > >     and make it work in the same way for transitions from any
> > >     low-power states (transitions from D1 and D2 are handled
> > >     slightly differently before the change).
> > >
> > >  d. Put the restoration of the BARs and the PCI_PM_CTRL
> > >     register read confirming the power state change into
> > >     pci_set_full_power_state() to avoid doing that in
> > >     pci_pm_default_resume_early() unnecessarily.
> > >
> > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > > ---
> > >
> > > v1 -> v2:
> > >    * Do not add a redundant check to pci_set_low_power_state().
> > >
> > > ---
> > >  drivers/pci/pci.c |  154 +++++++++++++++++++++++++++++++++++-------------------
> > >  1 file changed, 101 insertions(+), 53 deletions(-)
> > >
> > > Index: linux-pm/drivers/pci/pci.c
> > > ===================================================================
> > > --- linux-pm.orig/drivers/pci/pci.c
> > > +++ linux-pm/drivers/pci/pci.c
> > > @@ -1068,10 +1068,9 @@ static inline bool platform_pci_bridge_d
> > >  }
> > >
> > >  /**
> > > - * pci_raw_set_power_state - Use PCI PM registers to set the power state of
> > > - *                        given PCI device
> > > + * pci_set_low_power_state - Program the given device into a low-power state
> > >   * @dev: PCI device to handle.
> > > - * @state: PCI power state (D0, D1, D2, D3hot) to put the device into.
> > > + * @state: PCI power state (D1, D2, D3hot) to put the device into.
> > >   *
> > >   * RETURN VALUE:
> > >   * -EINVAL if the requested state is invalid.
> > > @@ -1080,10 +1079,9 @@ static inline bool platform_pci_bridge_d
> > >   * 0 if device already is in the requested state.
> > >   * 0 if device's power state has been successfully changed.
> > >   */
> > > -static int pci_raw_set_power_state(struct pci_dev *dev, pci_power_t state)
> > > +static int pci_set_low_power_state(struct pci_dev *dev, pci_power_t state)
> > >  {
> > >       u16 pmcsr;
> > > -     bool need_restore = false;
> > >
> > >       /* Check if we're already there */
> > >       if (dev->current_state == state)
> > > @@ -1092,7 +1090,7 @@ static int pci_raw_set_power_state(struc
> > >       if (!dev->pm_cap)
> > >               return -EIO;
> > >
> > > -     if (state < PCI_D0 || state > PCI_D3hot)
> > > +     if (state < PCI_D1 || state > PCI_D3hot)
> > >               return -EINVAL;
> > >
> > >       /*
> > > @@ -1101,8 +1099,7 @@ static int pci_raw_set_power_state(struc
> > >        * we can go from D1 to D3, but we can't go directly from D3 to D1;
> > >        * we'd have to go from D3 to D0, then to D1.
> > >        */
> > > -     if (state != PCI_D0 && dev->current_state <= PCI_D3cold
> > > -         && dev->current_state > state) {
> > > +     if (dev->current_state <= PCI_D3cold && dev->current_state > state) {
> > >               pci_err(dev, "invalid power transition (from %s to %s)\n",
> > >                       pci_power_name(dev->current_state),
> > >                       pci_power_name(state));
> > > @@ -1122,29 +1119,8 @@ static int pci_raw_set_power_state(struc
> > >               return -EIO;
> > >       }
> > >
> > > -     /*
> > > -      * If we're (effectively) in D3, force entire word to 0.
> > > -      * This doesn't affect PME_Status, disables PME_En, and
> > > -      * sets PowerState to 0.
> > > -      */
> > > -     switch (dev->current_state) {
> > > -     case PCI_D0:
> > > -     case PCI_D1:
> > > -     case PCI_D2:
> > > -             pmcsr &= ~PCI_PM_CTRL_STATE_MASK;
> > > -             pmcsr |= state;
> > > -             break;
> > > -     case PCI_D3hot:
> > > -     case PCI_D3cold:
> > > -     case PCI_UNKNOWN: /* Boot-up */
> > > -             if ((pmcsr & PCI_PM_CTRL_STATE_MASK) == PCI_D3hot
> > > -              && !(pmcsr & PCI_PM_CTRL_NO_SOFT_RESET))
> > > -                     need_restore = true;
> > > -             fallthrough;    /* force to D0 */
> > > -     default:
> > > -             pmcsr = 0;
> > > -             break;
> > > -     }
> > > +     pmcsr &= ~PCI_PM_CTRL_STATE_MASK;
> > > +     pmcsr |= state;
> > >
> > >       /* Enter specified state */
> > >       pci_write_config_word(dev, dev->pm_cap + PCI_PM_CTRL, pmcsr);
> > > @@ -1153,9 +1129,9 @@ static int pci_raw_set_power_state(struc
> > >        * Mandatory power management transition delays; see PCI PM 1.1
> > >        * 5.6.1 table 18
> > >        */
> > > -     if (state == PCI_D3hot || dev->current_state == PCI_D3hot)
> > > +     if (state == PCI_D3hot)
> > >               pci_dev_d3_sleep(dev);
> > > -     else if (state == PCI_D2 || dev->current_state == PCI_D2)
> > > +     else if (state == PCI_D2)
> > >               udelay(PCI_PM_D2_DELAY);
> > >
> > >       pci_read_config_word(dev, dev->pm_cap + PCI_PM_CTRL, &pmcsr);
> > > @@ -1165,22 +1141,6 @@ static int pci_raw_set_power_state(struc
> > >                        pci_power_name(dev->current_state),
> > >                        pci_power_name(state));
> > >
> > > -     /*
> > > -      * According to section 5.4.1 of the "PCI BUS POWER MANAGEMENT
> > > -      * INTERFACE SPECIFICATION, REV. 1.2", a device transitioning
> > > -      * from D3hot to D0 _may_ perform an internal reset, thereby
> > > -      * going to "D0 Uninitialized" rather than "D0 Initialized".
> > > -      * For example, at least some versions of the 3c905B and the
> > > -      * 3c556B exhibit this behaviour.
> > > -      *
> > > -      * At least some laptop BIOSen (e.g. the Thinkpad T21) leave
> > > -      * devices in a D3hot state at boot.  Consequently, we need to
> > > -      * restore at least the BARs so that the device will be
> > > -      * accessible to its driver.
> > > -      */
> > > -     if (need_restore)
> > > -             pci_restore_bars(dev);
> > > -
> > >       if (dev->bus->self)
> > >               pcie_aspm_pm_state_change(dev->bus->self);
> > >
> > > @@ -1312,8 +1272,54 @@ static int pci_dev_wait(struct pci_dev *
> > >   */
> > >  int pci_power_up(struct pci_dev *dev)
> > >  {
> > > -     pci_platform_power_transition(dev, PCI_D0);
> > > -     return pci_raw_set_power_state(dev, PCI_D0);
> > > +     int ret;
> > > +
> > > +     ret = pci_platform_power_transition(dev, PCI_D0);
> > > +     if (ret) {
> >
> > Here pci_platform_power_transition() returned an error so we go and read
> > back the PM_CTRL to check in which power state the device is in? Perhaps
> > add a comment here explaining why we need to do this?
> 
> That's the comment below, but I gather it is insufficient as is.
> Please let me know if rephrasing it this way would help:
> 
> "Since pci_platform_power_transition() has returned an error, the
> PCI_PM_CTRL register has not been read by it and the current power
> state of the device is unknown. Read the PCI_PM_CTRL register now and
> bail out if that fails."

Yes, that's better, thanks!

> And I've just realized that pm_cap should be checked here, because it
> is not guaranteed to be set.

Good point.