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;
Date:   Fri, 8 Apr 2022 15:43:18 +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 v1 1/2] PCI: PM: Avoid leaving devices in
 D0-uninitialized in pci_power_up()
Message-ID: <YlAt5he5B1SlORMh@lahna>
References: <4198163.ejJDZkT8p0@kreacher>
 <3623886.MHq7AAxBmi@kreacher>
 <YkwRjI0KvpmiJjvK@lahna>
 <CAJZ5v0go9hLqv6Mcc5Ko770AU7sTYJQvgyjhGJ36AO1kURUnYA@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <CAJZ5v0go9hLqv6Mcc5Ko770AU7sTYJQvgyjhGJ36AO1kURUnYA@mail.gmail.com>
Organization: Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo
Precedence: bulk

Hi Rafael,

On Thu, Apr 07, 2022 at 09:01:59PM +0200, Rafael J. Wysocki wrote:
> 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.

I see.

> 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.

Okay sounds good.

> 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.

I agree.