diff mbox

[3/5] PCI: revert preparing for wakeup in runtime-suspend finalization

Message ID 20130118114222.6698.7103.stgit@zurg (mailing list archive)
State New, archived
Delegated to: Bjorn Helgaas
Headers show

Commit Message

Konstantin Khlebnikov Jan. 18, 2013, 11:42 a.m. UTC
This patch effectively reverts commit 42eca2302146fed51335b95128e949ee6f54478f
("PCI: Don't touch card regs after runtime suspend D3")

| This patch checks whether the pci state is saved and doesn't attempt to hit
| any registers after that point if it is.

This seems completely wrong. Yes, PCI configuration space has been saved by
driver, but this doesn't means that all job is done and device has been
suspended and ready for waking up in the future.

For example driver e1000e for ethernet in my thinkpad x220 saves pci-state
but device cannot wakeup after that, because it needs some ACPI callbacks
which usually called from pci_finish_runtime_suspend().

| Optimus (dual-gpu) laptops seem to have their own form of D3cold, but
| unfortunately enter it on normal D3 transitions via the ACPI callback.

Hardware which disappears from the bus unexpectedly is exception, so let's
handle it as an exception. Its driver should set device state to D3cold and
the rest code will handle it properly.

Signed-off-by: Konstantin Khlebnikov <khlebnikov@openvz.org>
Cc: linux-pci@vger.kernel.org
Cc: Bjorn Helgaas <bhelgaas@google.com>
Cc: Dave Airlie <airlied@redhat.com>
---
 drivers/pci/pci-driver.c |    5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)


--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Bjorn Helgaas Jan. 28, 2013, 11:17 p.m. UTC | #1
[+cc Rafael]

On Fri, Jan 18, 2013 at 4:42 AM, Konstantin Khlebnikov
<khlebnikov@openvz.org> wrote:
> This patch effectively reverts commit 42eca2302146fed51335b95128e949ee6f54478f
> ("PCI: Don't touch card regs after runtime suspend D3")
>
> | This patch checks whether the pci state is saved and doesn't attempt to hit
> | any registers after that point if it is.
>
> This seems completely wrong. Yes, PCI configuration space has been saved by
> driver, but this doesn't means that all job is done and device has been
> suspended and ready for waking up in the future.
>
> For example driver e1000e for ethernet in my thinkpad x220 saves pci-state
> but device cannot wakeup after that, because it needs some ACPI callbacks
> which usually called from pci_finish_runtime_suspend().
>
> | Optimus (dual-gpu) laptops seem to have their own form of D3cold, but
> | unfortunately enter it on normal D3 transitions via the ACPI callback.
>
> Hardware which disappears from the bus unexpectedly is exception, so let's
> handle it as an exception. Its driver should set device state to D3cold and
> the rest code will handle it properly.

Functions in D3cold don't have power, so it's completely expected that
they would disappear from the bus and not respond to config accesses.
Maybe Dave was referring to D3hot, where functions *should* respond to
config accesses.  I dunno.

Just to be clear, it sounds like 42eca230 caused a regression on your
e1000e device?  If so, I guess we should revert it unless you and Dave
can figure out a better patch that fixes both your e1000e device and
the Optimus issue.

> Signed-off-by: Konstantin Khlebnikov <khlebnikov@openvz.org>
> Cc: linux-pci@vger.kernel.org
> Cc: Bjorn Helgaas <bhelgaas@google.com>
> Cc: Dave Airlie <airlied@redhat.com>
> ---
>  drivers/pci/pci-driver.c |    5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
> index f79cbcd..030dbf0 100644
> --- a/drivers/pci/pci-driver.c
> +++ b/drivers/pci/pci-driver.c
> @@ -1005,10 +1005,11 @@ static int pci_pm_runtime_suspend(struct device *dev)
>                 return 0;
>         }
>
> -       if (!pci_dev->state_saved) {
> +       if (!pci_dev->state_saved)
>                 pci_save_state(pci_dev);
> +
> +       if (pci_dev->current_state != PCI_D3cold)
>                 pci_finish_runtime_suspend(pci_dev);
> -       }
>
>         return 0;
>  }
>
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Rafael Wysocki Jan. 29, 2013, 1:15 a.m. UTC | #2
On Monday, January 28, 2013 04:17:42 PM Bjorn Helgaas wrote:
> [+cc Rafael]
> 
> On Fri, Jan 18, 2013 at 4:42 AM, Konstantin Khlebnikov
> <khlebnikov@openvz.org> wrote:
> > This patch effectively reverts commit 42eca2302146fed51335b95128e949ee6f54478f
> > ("PCI: Don't touch card regs after runtime suspend D3")
> >
> > | This patch checks whether the pci state is saved and doesn't attempt to hit
> > | any registers after that point if it is.
> >
> > This seems completely wrong. Yes, PCI configuration space has been saved by
> > driver, but this doesn't means that all job is done and device has been
> > suspended and ready for waking up in the future.
> >
> > For example driver e1000e for ethernet in my thinkpad x220 saves pci-state
> > but device cannot wakeup after that, because it needs some ACPI callbacks
> > which usually called from pci_finish_runtime_suspend().
> >
> > | Optimus (dual-gpu) laptops seem to have their own form of D3cold, but
> > | unfortunately enter it on normal D3 transitions via the ACPI callback.
> >
> > Hardware which disappears from the bus unexpectedly is exception, so let's
> > handle it as an exception. Its driver should set device state to D3cold and
> > the rest code will handle it properly.
> 
> Functions in D3cold don't have power, so it's completely expected that
> they would disappear from the bus and not respond to config accesses.
> Maybe Dave was referring to D3hot, where functions *should* respond to
> config accesses.  I dunno.
> 
> Just to be clear, it sounds like 42eca230 caused a regression on your
> e1000e device?  If so, I guess we should revert it unless you and Dave
> can figure out a better patch that fixes both your e1000e device and
> the Optimus issue.

Yes, if there's a regression, let's revert it, but I'd like the regression
to be described clearly.

Thanks,
Rafael
Konstantin Khlebnikov Jan. 29, 2013, 7:04 a.m. UTC | #3
Rafael J. Wysocki wrote:
> On Monday, January 28, 2013 04:17:42 PM Bjorn Helgaas wrote:
>> [+cc Rafael]
>>
>> On Fri, Jan 18, 2013 at 4:42 AM, Konstantin Khlebnikov
>> <khlebnikov@openvz.org>  wrote:
>>> This patch effectively reverts commit 42eca2302146fed51335b95128e949ee6f54478f
>>> ("PCI: Don't touch card regs after runtime suspend D3")
>>>
>>> | This patch checks whether the pci state is saved and doesn't attempt to hit
>>> | any registers after that point if it is.
>>>
>>> This seems completely wrong. Yes, PCI configuration space has been saved by
>>> driver, but this doesn't means that all job is done and device has been
>>> suspended and ready for waking up in the future.
>>>
>>> For example driver e1000e for ethernet in my thinkpad x220 saves pci-state
>>> but device cannot wakeup after that, because it needs some ACPI callbacks
>>> which usually called from pci_finish_runtime_suspend().
>>>
>>> | Optimus (dual-gpu) laptops seem to have their own form of D3cold, but
>>> | unfortunately enter it on normal D3 transitions via the ACPI callback.
>>>
>>> Hardware which disappears from the bus unexpectedly is exception, so let's
>>> handle it as an exception. Its driver should set device state to D3cold and
>>> the rest code will handle it properly.
>>
>> Functions in D3cold don't have power, so it's completely expected that
>> they would disappear from the bus and not respond to config accesses.
>> Maybe Dave was referring to D3hot, where functions *should* respond to
>> config accesses.  I dunno.
>>
>> Just to be clear, it sounds like 42eca230 caused a regression on your
>> e1000e device?  If so, I guess we should revert it unless you and Dave
>> can figure out a better patch that fixes both your e1000e device and
>> the Optimus issue.
>
> Yes, if there's a regression, let's revert it, but I'd like the regression
> to be described clearly.

Yep, this is regression.

commit 42eca2302146fed51335b95128e949ee6f54478f ("PCI: Don't touch
card regs after runtime suspend D3") changes state convention during
runtime-suspend transaction too much. If PCI configuration space
has been saved by driver that does not means that all job is done
and device has been suspended and ready for waking up in the future.

e1000e saves pci-config space itself, but it requires operations which
pci_finish_runtime_suspend() does: preparing for wake (calling particular
platform pm-callbacks) and switching to proper sleep state.

>
> Thanks,
> Rafael
>
>

--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Rafael Wysocki Jan. 29, 2013, 11:55 a.m. UTC | #4
On Tuesday, January 29, 2013 11:04:57 AM Konstantin Khlebnikov wrote:
> Rafael J. Wysocki wrote:
> > On Monday, January 28, 2013 04:17:42 PM Bjorn Helgaas wrote:
> >> [+cc Rafael]
> >>
> >> On Fri, Jan 18, 2013 at 4:42 AM, Konstantin Khlebnikov
> >> <khlebnikov@openvz.org>  wrote:
> >>> This patch effectively reverts commit 42eca2302146fed51335b95128e949ee6f54478f
> >>> ("PCI: Don't touch card regs after runtime suspend D3")
> >>>
> >>> | This patch checks whether the pci state is saved and doesn't attempt to hit
> >>> | any registers after that point if it is.
> >>>
> >>> This seems completely wrong. Yes, PCI configuration space has been saved by
> >>> driver, but this doesn't means that all job is done and device has been
> >>> suspended and ready for waking up in the future.
> >>>
> >>> For example driver e1000e for ethernet in my thinkpad x220 saves pci-state
> >>> but device cannot wakeup after that, because it needs some ACPI callbacks
> >>> which usually called from pci_finish_runtime_suspend().
> >>>
> >>> | Optimus (dual-gpu) laptops seem to have their own form of D3cold, but
> >>> | unfortunately enter it on normal D3 transitions via the ACPI callback.
> >>>
> >>> Hardware which disappears from the bus unexpectedly is exception, so let's
> >>> handle it as an exception. Its driver should set device state to D3cold and
> >>> the rest code will handle it properly.
> >>
> >> Functions in D3cold don't have power, so it's completely expected that
> >> they would disappear from the bus and not respond to config accesses.
> >> Maybe Dave was referring to D3hot, where functions *should* respond to
> >> config accesses.  I dunno.
> >>
> >> Just to be clear, it sounds like 42eca230 caused a regression on your
> >> e1000e device?  If so, I guess we should revert it unless you and Dave
> >> can figure out a better patch that fixes both your e1000e device and
> >> the Optimus issue.
> >
> > Yes, if there's a regression, let's revert it, but I'd like the regression
> > to be described clearly.
> 
> Yep, this is regression.
> 
> commit 42eca2302146fed51335b95128e949ee6f54478f ("PCI: Don't touch
> card regs after runtime suspend D3") changes state convention during
> runtime-suspend transaction too much. If PCI configuration space
> has been saved by driver that does not means that all job is done
> and device has been suspended and ready for waking up in the future.
> 
> e1000e saves pci-config space itself, but it requires operations which
> pci_finish_runtime_suspend() does: preparing for wake (calling particular
> platform pm-callbacks) and switching to proper sleep state.

Well, I'd argue this is a bug in e1000e.  Why does it need to save the PCI
config space even though pci_pm_runtime_suspend() will do that anyway?

Rafael
Rafael Wysocki Jan. 31, 2013, 1:13 a.m. UTC | #5
On Tuesday, January 29, 2013 12:55:15 PM Rafael J. Wysocki wrote:
> On Tuesday, January 29, 2013 11:04:57 AM Konstantin Khlebnikov wrote:
> > Rafael J. Wysocki wrote:
> > > On Monday, January 28, 2013 04:17:42 PM Bjorn Helgaas wrote:
> > >> [+cc Rafael]
> > >>
> > >> On Fri, Jan 18, 2013 at 4:42 AM, Konstantin Khlebnikov
> > >> <khlebnikov@openvz.org>  wrote:
> > >>> This patch effectively reverts commit 42eca2302146fed51335b95128e949ee6f54478f
> > >>> ("PCI: Don't touch card regs after runtime suspend D3")
> > >>>
> > >>> | This patch checks whether the pci state is saved and doesn't attempt to hit
> > >>> | any registers after that point if it is.
> > >>>
> > >>> This seems completely wrong. Yes, PCI configuration space has been saved by
> > >>> driver, but this doesn't means that all job is done and device has been
> > >>> suspended and ready for waking up in the future.
> > >>>
> > >>> For example driver e1000e for ethernet in my thinkpad x220 saves pci-state
> > >>> but device cannot wakeup after that, because it needs some ACPI callbacks
> > >>> which usually called from pci_finish_runtime_suspend().
> > >>>
> > >>> | Optimus (dual-gpu) laptops seem to have their own form of D3cold, but
> > >>> | unfortunately enter it on normal D3 transitions via the ACPI callback.
> > >>>
> > >>> Hardware which disappears from the bus unexpectedly is exception, so let's
> > >>> handle it as an exception. Its driver should set device state to D3cold and
> > >>> the rest code will handle it properly.
> > >>
> > >> Functions in D3cold don't have power, so it's completely expected that
> > >> they would disappear from the bus and not respond to config accesses.
> > >> Maybe Dave was referring to D3hot, where functions *should* respond to
> > >> config accesses.  I dunno.
> > >>
> > >> Just to be clear, it sounds like 42eca230 caused a regression on your
> > >> e1000e device?  If so, I guess we should revert it unless you and Dave
> > >> can figure out a better patch that fixes both your e1000e device and
> > >> the Optimus issue.
> > >
> > > Yes, if there's a regression, let's revert it, but I'd like the regression
> > > to be described clearly.
> > 
> > Yep, this is regression.
> > 
> > commit 42eca2302146fed51335b95128e949ee6f54478f ("PCI: Don't touch
> > card regs after runtime suspend D3") changes state convention during
> > runtime-suspend transaction too much. If PCI configuration space
> > has been saved by driver that does not means that all job is done
> > and device has been suspended and ready for waking up in the future.
> > 
> > e1000e saves pci-config space itself, but it requires operations which
> > pci_finish_runtime_suspend() does: preparing for wake (calling particular
> > platform pm-callbacks) and switching to proper sleep state.
> 
> Well, I'd argue this is a bug in e1000e.  Why does it need to save the PCI
> config space even though pci_pm_runtime_suspend() will do that anyway?

I honestly don't think we should revert 42eca2302146 because of this.

Yes, there is a requirement that drivers not save the PCI config space by
themselves unless they want to do the whole power management by themselves too
and e1000e is not following that.  So either we need to drop the
pci_save_state() from __e1000_shutdown() which I would prefer (I'm not really
sure why it is there), or e1000_runtime_suspend() needs to call
pci_finish_runtime_suspend() by itself.

Thanks,
Rafael
Konstantin Khlebnikov Feb. 2, 2013, 12:12 p.m. UTC | #6
Rafael J. Wysocki wrote:
> On Tuesday, January 29, 2013 12:55:15 PM Rafael J. Wysocki wrote:
>> On Tuesday, January 29, 2013 11:04:57 AM Konstantin Khlebnikov wrote:
>>> Rafael J. Wysocki wrote:
>>>> On Monday, January 28, 2013 04:17:42 PM Bjorn Helgaas wrote:
>>>>> [+cc Rafael]
>>>>>
>>>>> On Fri, Jan 18, 2013 at 4:42 AM, Konstantin Khlebnikov
>>>>> <khlebnikov@openvz.org>   wrote:
>>>>>> This patch effectively reverts commit 42eca2302146fed51335b95128e949ee6f54478f
>>>>>> ("PCI: Don't touch card regs after runtime suspend D3")
>>>>>>
>>>>>> | This patch checks whether the pci state is saved and doesn't attempt to hit
>>>>>> | any registers after that point if it is.
>>>>>>
>>>>>> This seems completely wrong. Yes, PCI configuration space has been saved by
>>>>>> driver, but this doesn't means that all job is done and device has been
>>>>>> suspended and ready for waking up in the future.
>>>>>>
>>>>>> For example driver e1000e for ethernet in my thinkpad x220 saves pci-state
>>>>>> but device cannot wakeup after that, because it needs some ACPI callbacks
>>>>>> which usually called from pci_finish_runtime_suspend().
>>>>>>
>>>>>> | Optimus (dual-gpu) laptops seem to have their own form of D3cold, but
>>>>>> | unfortunately enter it on normal D3 transitions via the ACPI callback.
>>>>>>
>>>>>> Hardware which disappears from the bus unexpectedly is exception, so let's
>>>>>> handle it as an exception. Its driver should set device state to D3cold and
>>>>>> the rest code will handle it properly.
>>>>>
>>>>> Functions in D3cold don't have power, so it's completely expected that
>>>>> they would disappear from the bus and not respond to config accesses.
>>>>> Maybe Dave was referring to D3hot, where functions *should* respond to
>>>>> config accesses.  I dunno.
>>>>>
>>>>> Just to be clear, it sounds like 42eca230 caused a regression on your
>>>>> e1000e device?  If so, I guess we should revert it unless you and Dave
>>>>> can figure out a better patch that fixes both your e1000e device and
>>>>> the Optimus issue.
>>>>
>>>> Yes, if there's a regression, let's revert it, but I'd like the regression
>>>> to be described clearly.
>>>
>>> Yep, this is regression.
>>>
>>> commit 42eca2302146fed51335b95128e949ee6f54478f ("PCI: Don't touch
>>> card regs after runtime suspend D3") changes state convention during
>>> runtime-suspend transaction too much. If PCI configuration space
>>> has been saved by driver that does not means that all job is done
>>> and device has been suspended and ready for waking up in the future.
>>>
>>> e1000e saves pci-config space itself, but it requires operations which
>>> pci_finish_runtime_suspend() does: preparing for wake (calling particular
>>> platform pm-callbacks) and switching to proper sleep state.
>>
>> Well, I'd argue this is a bug in e1000e.  Why does it need to save the PCI
>> config space even though pci_pm_runtime_suspend() will do that anyway?
>
> I honestly don't think we should revert 42eca2302146 because of this.
>
> Yes, there is a requirement that drivers not save the PCI config space by
> themselves unless they want to do the whole power management by themselves too
> and e1000e is not following that.  So either we need to drop the
> pci_save_state() from __e1000_shutdown() which I would prefer (I'm not really
> sure why it is there), or e1000_runtime_suspend() needs to call
> pci_finish_runtime_suspend() by itself.

Yet another problem: some drivers calls pci_save_state() from ->probe() callback
to use this saved state in pci_error_handlers->slot_reset().
As result pdev->state_saved is true mostly all time.
At least e1000e and drivers/pci/pcie/portdrv_pci.c are doing this.

I think it will be safer to revert 42eca2302146 in v3.8
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Airlie Feb. 3, 2013, 11:03 p.m. UTC | #7
> Rafael J. Wysocki wrote:
> > On Tuesday, January 29, 2013 12:55:15 PM Rafael J. Wysocki wrote:
> >> On Tuesday, January 29, 2013 11:04:57 AM Konstantin Khlebnikov
> >> wrote:
> >>> Rafael J. Wysocki wrote:
> >>>> On Monday, January 28, 2013 04:17:42 PM Bjorn Helgaas wrote:
> >>>>> [+cc Rafael]
> >>>>>
> >>>>> On Fri, Jan 18, 2013 at 4:42 AM, Konstantin Khlebnikov
> >>>>> <khlebnikov@openvz.org>   wrote:
> >>>>>> This patch effectively reverts commit
> >>>>>> 42eca2302146fed51335b95128e949ee6f54478f
> >>>>>> ("PCI: Don't touch card regs after runtime suspend D3")
> >>>>>>
> >>>>>> | This patch checks whether the pci state is saved and doesn't
> >>>>>> | attempt to hit
> >>>>>> | any registers after that point if it is.
> >>>>>>
> >>>>>> This seems completely wrong. Yes, PCI configuration space has
> >>>>>> been saved by
> >>>>>> driver, but this doesn't means that all job is done and device
> >>>>>> has been
> >>>>>> suspended and ready for waking up in the future.
> >>>>>>
> >>>>>> For example driver e1000e for ethernet in my thinkpad x220
> >>>>>> saves pci-state
> >>>>>> but device cannot wakeup after that, because it needs some
> >>>>>> ACPI callbacks
> >>>>>> which usually called from pci_finish_runtime_suspend().
> >>>>>>
> >>>>>> | Optimus (dual-gpu) laptops seem to have their own form of
> >>>>>> | D3cold, but
> >>>>>> | unfortunately enter it on normal D3 transitions via the ACPI
> >>>>>> | callback.
> >>>>>>
> >>>>>> Hardware which disappears from the bus unexpectedly is
> >>>>>> exception, so let's
> >>>>>> handle it as an exception. Its driver should set device state
> >>>>>> to D3cold and
> >>>>>> the rest code will handle it properly.
> >>>>>
> >>>>> Functions in D3cold don't have power, so it's completely
> >>>>> expected that
> >>>>> they would disappear from the bus and not respond to config
> >>>>> accesses.
> >>>>> Maybe Dave was referring to D3hot, where functions *should*
> >>>>> respond to
> >>>>> config accesses.  I dunno.

Just to clarify what is going on with optimus laptops:

D3cold didn't exist as a real thing, so when you call the ACPI method
to enter D3, it actually powers off the card completely, I've actually
in my future work fixed it so that the driver then reports it being in
D3cold when that happens.

The problem here was originally that the kernel didn't care if we hit
D3hot or D3cold, it wanted to finish the dynamic suspend, however
in this case there was no way it could.

So I'm happy with any solution that means cards reporting being in D3cold,
won't get touched again after they return.

Dave.
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Airlie Feb. 3, 2013, 11:19 p.m. UTC | #8
----- Original Message -----
> From: "Konstantin Khlebnikov" <khlebnikov@openvz.org>
> To: "Rafael J. Wysocki" <rjw@sisk.pl>
> Cc: "Bjorn Helgaas" <bhelgaas@google.com>, linux-kernel@vger.kernel.org, e1000-devel@lists.sourceforge.net,
> linux-pci@vger.kernel.org, "Dave Airlie" <airlied@redhat.com>, "Rafael J. Wysocki" <rafael.j.wysocki@intel.com>
> Sent: Saturday, 2 February, 2013 10:12:03 PM
> Subject: Re: [PATCH 3/5] PCI: revert preparing for wakeup in runtime-suspend finalization
> 
> Rafael J. Wysocki wrote:
> > On Tuesday, January 29, 2013 12:55:15 PM Rafael J. Wysocki wrote:
> >> On Tuesday, January 29, 2013 11:04:57 AM Konstantin Khlebnikov
> >> wrote:
> >>> Rafael J. Wysocki wrote:
> >>>> On Monday, January 28, 2013 04:17:42 PM Bjorn Helgaas wrote:
> >>>>> [+cc Rafael]
> >>>>>
> >>>>> On Fri, Jan 18, 2013 at 4:42 AM, Konstantin Khlebnikov
> >>>>> <khlebnikov@openvz.org>   wrote:
> >>>>>> This patch effectively reverts commit
> >>>>>> 42eca2302146fed51335b95128e949ee6f54478f
> >>>>>> ("PCI: Don't touch card regs after runtime suspend D3")
> >>>>>>
> >>>>>> | This patch checks whether the pci state is saved and doesn't
> >>>>>> | attempt to hit
> >>>>>> | any registers after that point if it is.
> >>>>>>
> >>>>>> This seems completely wrong. Yes, PCI configuration space has
> >>>>>> been saved by
> >>>>>> driver, but this doesn't means that all job is done and device
> >>>>>> has been
> >>>>>> suspended and ready for waking up in the future.
> >>>>>>
> >>>>>> For example driver e1000e for ethernet in my thinkpad x220
> >>>>>> saves pci-state
> >>>>>> but device cannot wakeup after that, because it needs some
> >>>>>> ACPI callbacks
> >>>>>> which usually called from pci_finish_runtime_suspend().
> >>>>>>
> >>>>>> | Optimus (dual-gpu) laptops seem to have their own form of
> >>>>>> | D3cold, but
> >>>>>> | unfortunately enter it on normal D3 transitions via the ACPI
> >>>>>> | callback.
> >>>>>>
> >>>>>> Hardware which disappears from the bus unexpectedly is
> >>>>>> exception, so let's
> >>>>>> handle it as an exception. Its driver should set device state
> >>>>>> to D3cold and
> >>>>>> the rest code will handle it properly.
> >>>>>
> >>>>> Functions in D3cold don't have power, so it's completely
> >>>>> expected that
> >>>>> they would disappear from the bus and not respond to config
> >>>>> accesses.
> >>>>> Maybe Dave was referring to D3hot, where functions *should*
> >>>>> respond to
> >>>>> config accesses.  I dunno.
> >>>>>
> >>>>> Just to be clear, it sounds like 42eca230 caused a regression
> >>>>> on your
> >>>>> e1000e device?  If so, I guess we should revert it unless you
> >>>>> and Dave
> >>>>> can figure out a better patch that fixes both your e1000e
> >>>>> device and
> >>>>> the Optimus issue.
> >>>>
> >>>> Yes, if there's a regression, let's revert it, but I'd like the
> >>>> regression
> >>>> to be described clearly.
> >>>
> >>> Yep, this is regression.
> >>>
> >>> commit 42eca2302146fed51335b95128e949ee6f54478f ("PCI: Don't
> >>> touch
> >>> card regs after runtime suspend D3") changes state convention
> >>> during
> >>> runtime-suspend transaction too much. If PCI configuration space
> >>> has been saved by driver that does not means that all job is done
> >>> and device has been suspended and ready for waking up in the
> >>> future.
> >>>
> >>> e1000e saves pci-config space itself, but it requires operations
> >>> which
> >>> pci_finish_runtime_suspend() does: preparing for wake (calling
> >>> particular
> >>> platform pm-callbacks) and switching to proper sleep state.
> >>
> >> Well, I'd argue this is a bug in e1000e.  Why does it need to save
> >> the PCI
> >> config space even though pci_pm_runtime_suspend() will do that
> >> anyway?
> >
> > I honestly don't think we should revert 42eca2302146 because of
> > this.
> >
> > Yes, there is a requirement that drivers not save the PCI config
> > space by
> > themselves unless they want to do the whole power management by
> > themselves too
> > and e1000e is not following that.  So either we need to drop the
> > pci_save_state() from __e1000_shutdown() which I would prefer (I'm
> > not really
> > sure why it is there), or e1000_runtime_suspend() needs to call
> > pci_finish_runtime_suspend() by itself.
> 
> Yet another problem: some drivers calls pci_save_state() from
> ->probe() callback
> to use this saved state in pci_error_handlers->slot_reset().
> As result pdev->state_saved is true mostly all time.
> At least e1000e and drivers/pci/pcie/portdrv_pci.c are doing this.
> 
> I think it will be safer to revert 42eca2302146 in v3.8
> 

btw I've no problem reverting this for 3.8, though I'd like to get a fix in for 3.9 then,
the code relying on this change is still not completed, so a revert shouldn't break anything.

but definitely if a card goes into D3cold, we need to not poke any registers on it after it
returns.

Dave.
Dave.
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Rafael Wysocki Feb. 3, 2013, 11:31 p.m. UTC | #9
On Sunday, February 03, 2013 06:19:55 PM David Airlie wrote:
> 
> ----- Original Message -----
> > From: "Konstantin Khlebnikov" <khlebnikov@openvz.org>
> > To: "Rafael J. Wysocki" <rjw@sisk.pl>
> > Cc: "Bjorn Helgaas" <bhelgaas@google.com>, linux-kernel@vger.kernel.org, e1000-devel@lists.sourceforge.net,
> > linux-pci@vger.kernel.org, "Dave Airlie" <airlied@redhat.com>, "Rafael J. Wysocki" <rafael.j.wysocki@intel.com>
> > Sent: Saturday, 2 February, 2013 10:12:03 PM
> > Subject: Re: [PATCH 3/5] PCI: revert preparing for wakeup in runtime-suspend finalization
> > 
> > Rafael J. Wysocki wrote:
> > > On Tuesday, January 29, 2013 12:55:15 PM Rafael J. Wysocki wrote:
> > >> On Tuesday, January 29, 2013 11:04:57 AM Konstantin Khlebnikov
> > >> wrote:
> > >>> Rafael J. Wysocki wrote:
> > >>>> On Monday, January 28, 2013 04:17:42 PM Bjorn Helgaas wrote:
> > >>>>> [+cc Rafael]
> > >>>>>
> > >>>>> On Fri, Jan 18, 2013 at 4:42 AM, Konstantin Khlebnikov
> > >>>>> <khlebnikov@openvz.org>   wrote:
> > >>>>>> This patch effectively reverts commit
> > >>>>>> 42eca2302146fed51335b95128e949ee6f54478f
> > >>>>>> ("PCI: Don't touch card regs after runtime suspend D3")
> > >>>>>>
> > >>>>>> | This patch checks whether the pci state is saved and doesn't
> > >>>>>> | attempt to hit
> > >>>>>> | any registers after that point if it is.
> > >>>>>>
> > >>>>>> This seems completely wrong. Yes, PCI configuration space has
> > >>>>>> been saved by
> > >>>>>> driver, but this doesn't means that all job is done and device
> > >>>>>> has been
> > >>>>>> suspended and ready for waking up in the future.
> > >>>>>>
> > >>>>>> For example driver e1000e for ethernet in my thinkpad x220
> > >>>>>> saves pci-state
> > >>>>>> but device cannot wakeup after that, because it needs some
> > >>>>>> ACPI callbacks
> > >>>>>> which usually called from pci_finish_runtime_suspend().
> > >>>>>>
> > >>>>>> | Optimus (dual-gpu) laptops seem to have their own form of
> > >>>>>> | D3cold, but
> > >>>>>> | unfortunately enter it on normal D3 transitions via the ACPI
> > >>>>>> | callback.
> > >>>>>>
> > >>>>>> Hardware which disappears from the bus unexpectedly is
> > >>>>>> exception, so let's
> > >>>>>> handle it as an exception. Its driver should set device state
> > >>>>>> to D3cold and
> > >>>>>> the rest code will handle it properly.
> > >>>>>
> > >>>>> Functions in D3cold don't have power, so it's completely
> > >>>>> expected that
> > >>>>> they would disappear from the bus and not respond to config
> > >>>>> accesses.
> > >>>>> Maybe Dave was referring to D3hot, where functions *should*
> > >>>>> respond to
> > >>>>> config accesses.  I dunno.
> > >>>>>
> > >>>>> Just to be clear, it sounds like 42eca230 caused a regression
> > >>>>> on your
> > >>>>> e1000e device?  If so, I guess we should revert it unless you
> > >>>>> and Dave
> > >>>>> can figure out a better patch that fixes both your e1000e
> > >>>>> device and
> > >>>>> the Optimus issue.
> > >>>>
> > >>>> Yes, if there's a regression, let's revert it, but I'd like the
> > >>>> regression
> > >>>> to be described clearly.
> > >>>
> > >>> Yep, this is regression.
> > >>>
> > >>> commit 42eca2302146fed51335b95128e949ee6f54478f ("PCI: Don't
> > >>> touch
> > >>> card regs after runtime suspend D3") changes state convention
> > >>> during
> > >>> runtime-suspend transaction too much. If PCI configuration space
> > >>> has been saved by driver that does not means that all job is done
> > >>> and device has been suspended and ready for waking up in the
> > >>> future.
> > >>>
> > >>> e1000e saves pci-config space itself, but it requires operations
> > >>> which
> > >>> pci_finish_runtime_suspend() does: preparing for wake (calling
> > >>> particular
> > >>> platform pm-callbacks) and switching to proper sleep state.
> > >>
> > >> Well, I'd argue this is a bug in e1000e.  Why does it need to save
> > >> the PCI
> > >> config space even though pci_pm_runtime_suspend() will do that
> > >> anyway?
> > >
> > > I honestly don't think we should revert 42eca2302146 because of
> > > this.
> > >
> > > Yes, there is a requirement that drivers not save the PCI config
> > > space by
> > > themselves unless they want to do the whole power management by
> > > themselves too
> > > and e1000e is not following that.  So either we need to drop the
> > > pci_save_state() from __e1000_shutdown() which I would prefer (I'm
> > > not really
> > > sure why it is there), or e1000_runtime_suspend() needs to call
> > > pci_finish_runtime_suspend() by itself.
> > 
> > Yet another problem: some drivers calls pci_save_state() from
> > ->probe() callback
> > to use this saved state in pci_error_handlers->slot_reset().
> > As result pdev->state_saved is true mostly all time.
> > At least e1000e and drivers/pci/pcie/portdrv_pci.c are doing this.
> > 
> > I think it will be safer to revert 42eca2302146 in v3.8
> > 
> 
> btw I've no problem reverting this for 3.8, though I'd like to get a fix in for 3.9 then,
> the code relying on this change is still not completed, so a revert shouldn't break anything.
> 
> but definitely if a card goes into D3cold, we need to not poke any registers on it after it
> returns.

Sure.

As I said, I don't think it is necessary to revert 42eca2302146 and I've got a
confirmation from Boris that the approach proposed so far works, so now it only
is a matter of cutting final patches and testing them.

I'm not quite sure what Konstantin's plans in that respect are, though.
Konstantin, care to tell us?

Rafael
diff mbox

Patch

diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
index f79cbcd..030dbf0 100644
--- a/drivers/pci/pci-driver.c
+++ b/drivers/pci/pci-driver.c
@@ -1005,10 +1005,11 @@  static int pci_pm_runtime_suspend(struct device *dev)
 		return 0;
 	}
 
-	if (!pci_dev->state_saved) {
+	if (!pci_dev->state_saved)
 		pci_save_state(pci_dev);
+
+	if (pci_dev->current_state != PCI_D3cold)
 		pci_finish_runtime_suspend(pci_dev);
-	}
 
 	return 0;
 }