Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
MIME-Version: 1.0
References: <4198163.ejJDZkT8p0@kreacher> <3623886.MHq7AAxBmi@kreacher> <YkwRjI0KvpmiJjvK@lahna>
In-Reply-To: <YkwRjI0KvpmiJjvK@lahna>
From:   "Rafael J. Wysocki" <rafael@kernel.org>
Date:   Thu, 7 Apr 2022 21:01:59 +0200
Message-ID: <CAJZ5v0go9hLqv6Mcc5Ko770AU7sTYJQvgyjhGJ36AO1kURUnYA@mail.gmail.com>
Subject: Re: [PATCH v1 1/2] PCI: PM: Avoid leaving devices in D0-uninitialized
 in pci_power_up()
To:     Mika Westerberg <mika.westerberg@linux.intel.com>
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>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Tue, Apr 5, 2022 at 1:45 PM Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:
>
> On Mon, Apr 04, 2022 at 05:41:13PM +0200, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >
> > In theory, pci_power_up() may leave a device in D0-uninitialized
> > during a transition from D3cold to D0.
> >
> > Say, a PCIe device depending on some ACPI power resources is put into
> > D3cold, so the power resources in question are all turned off.  Then,
> > pci_power_up() is called to put it into D0.
> >
> > It first calls pci_platform_power_transition() which invokes
> > platform_pci_set_power_state() to turn on the ACPI power resources
> > depended on by the device and, if that is successful, it calls
> > pci_update_current_state() to update the current_state field of
> > the PCI device object.  If the device's configuration space is
> > accessible at this point, which is the case if
> > platform_pci_set_power_state() leaves it in D0-uninitialized (and
> > there's nothing to prevent it from doing so), current_state will be
> > set to PCI_D0 and the pci_raw_set_power_state() called subsequently
> > will notice that the device is in D0 already and do nothing.
> > However, that is not correct, because it may be still necessary to
> > restore the device's BARs at this point.
> >
> > To address this issue, set current_state temporarily to PCI_D3hot
> > in the cases in which pci_raw_set_power_state() may need to do more
> > than just changing the power state of the device.
> >
> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>
> Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>

Thanks, but on second thought, I'm not sure if this is the best way to
address the issue.

Basically, pci_power_up() is called in two places, in
pci_set_power_state() (for the transitions to D0) and in
pci_pm_default_resume_early().  In the latter case,
pci_restore_state() is called right after it and that covers BARs
restoration, so nothing more needs to be done in that case.

This means that pci_set_power_state() is the only place needing to
restore the BARs when going into D0 from D3hot or deeper and it is
better to move BARs restoration directly into it.  I'll update the
series accordingly and resend.

I also think that the mandatory delay is not needed at all when
pci_raw_set_power_state() is called for transitions D3cold -> D0,
because in that case either the device has been powered up via
platform_pci_set_power_state(), or via the bridge resume which takes
the delay into account.