Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20220830104913.1620539-1-rajvi.jingar@linux.intel.com>
In-Reply-To: <20220830104913.1620539-1-rajvi.jingar@linux.intel.com>
From:   "Rafael J. Wysocki" <rafael@kernel.org>
Date:   Tue, 30 Aug 2022 13:44:43 +0200
Message-ID: <CAJZ5v0hhc+n+WbLCxbJ2VPSWj0RAy4AM+pvqy8eiAJVtBecVWg@mail.gmail.com>
Subject: Re: [RESEND PATCH v3 1/2] PCI/PM: refactor pci_pm_suspend_noirq()
To:     Rajvi Jingar <rajvi.jingar@linux.intel.com>,
        Bjorn Helgaas <bhelgaas@google.com>
Cc:     Rafael Wysocki <rafael.j.wysocki@intel.com>,
        David Box <david.e.box@linux.intel.com>,
        Linux PCI <linux-pci@vger.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        Linux PM <linux-pm@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

Hi Bjorn,

On Tue, Aug 30, 2022 at 12:49 PM Rajvi Jingar
<rajvi.jingar@linux.intel.com> wrote:
>
> The state of the device is saved during pci_pm_suspend_noirq(), if it
> has not already been saved, regardless of the skip_bus_pm flag value. So
> skip_bus_pm check is removed before saving the device state.
>
> Signed-off-by: Rajvi Jingar <rajvi.jingar@linux.intel.com>
> Reviewed-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

I have reviewed this and the [2/2] already and they are clear
improvements to me.

Do you have any concerns regarding any of them?

If not, do you want me to pick them up or do you plan to take care of
them yourself?

> ---
>  v1->v2: no change
>  v2->v3: no change
> ---
>  drivers/pci/pci-driver.c | 18 ++++++------------
>  1 file changed, 6 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
> index 49238ddd39ee..1f64de3e5280 100644
> --- a/drivers/pci/pci-driver.c
> +++ b/drivers/pci/pci-driver.c
> @@ -867,20 +867,14 @@ static int pci_pm_suspend_noirq(struct device *dev)
>                 }
>         }
>
> -       if (pci_dev->skip_bus_pm) {
> +       if (!pci_dev->state_saved) {
> +               pci_save_state(pci_dev);
>                 /*
> -                * Either the device is a bridge with a child in D0 below it, or
> -                * the function is running for the second time in a row without
> -                * going through full resume, which is possible only during
> -                * suspend-to-idle in a spurious wakeup case.  The device should
> -                * be in D0 at this point, but if it is a bridge, it may be
> -                * necessary to save its state.
> +                * If the device is a bridge with a child in D0 below it, it needs to
> +                * stay in D0, so check skip_bus_pm to avoid putting it into a
> +                * low-power state in that case.
>                  */
> -               if (!pci_dev->state_saved)
> -                       pci_save_state(pci_dev);
> -       } else if (!pci_dev->state_saved) {
> -               pci_save_state(pci_dev);
> -               if (pci_power_manageable(pci_dev))
> +               if (!pci_dev->skip_bus_pm && pci_power_manageable(pci_dev))
>                         pci_prepare_to_sleep(pci_dev);
>         }
>
> --
> 2.25.1
>