[PATCH 0/5] pci/e1000e: return runtime-pm back to work

[PATCH 0/5] pci/e1000e: return runtime-pm back to work

[Konstantin Khlebnikov]

This patchset contains some fixes for e1000e diver (broken since v2.6.35)
and some related fixes for PCI code (more fresh bugs).
Last patch adds simple debug for catching device-enable-cointer underflows.

All together this fixes my regression report for v3.8-rc1:
https://lkml.org/lkml/2013/1/1/25

---

Konstantin Khlebnikov (5):
      e1000e: fix resuming from runtime-suspend
      e1000e: fix pci device enable counter balance
      PCI: revert preparing for wakeup in runtime-suspend finalization
      PCI: don't touch enable_cnt in pci_device_shutdown()
      PCI: catch enable-counter underflows


 drivers/net/ethernet/intel/e1000e/netdev.c |   20 +++++++++++++++-----
 drivers/pci/pci-driver.c                   |    7 ++++---
 drivers/pci/pci.c                          |    3 +++
 3 files changed, 22 insertions(+), 8 deletions(-)

-- 
Signature

[PATCH 1/5] e1000e: fix resuming from runtime-suspend

[Konstantin Khlebnikov]

Bug was introduced in commit 23606cf5d1192c2b17912cb2ef6e62f9b11de133
("e1000e / PCI / PM: Add basic runtime PM support (rev. 4)") in v2.6.35

Signed-off-by: Konstantin Khlebnikov <khlebnikov@openvz.org>
Cc: e1000-devel@lists.sourceforge.net
Cc: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
Cc: Bruce Allan <bruce.w.allan@intel.com>
---
 drivers/net/ethernet/intel/e1000e/netdev.c |   13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ethernet/intel/e1000e/netdev.c b/drivers/net/ethernet/intel/e1000e/netdev.c
index fbf75fd..2853c11 100644
--- a/drivers/net/ethernet/intel/e1000e/netdev.c
+++ b/drivers/net/ethernet/intel/e1000e/netdev.c
@@ -5691,14 +5691,17 @@ static int e1000_runtime_suspend(struct device *dev)
 	struct pci_dev *pdev = to_pci_dev(dev);
 	struct net_device *netdev = pci_get_drvdata(pdev);
 	struct e1000_adapter *adapter = netdev_priv(netdev);
+	int retval;
+	bool wake;
 
-	if (e1000e_pm_ready(adapter)) {
-		bool wake;
+	if (!e1000e_pm_ready(adapter))
+		return 0;
 
-		__e1000_shutdown(pdev, &wake, true);
-	}
+	retval = __e1000_shutdown(pdev, &wake, true);
+	if (!retval)
+		e1000_power_off(pdev, true, wake);
 
-	return 0;
+	return retval;
 }
 
 static int e1000_idle(struct device *dev)

[PATCH 2/5] e1000e: fix pci device enable counter balance

[Konstantin Khlebnikov]

__e1000_shutdown() calls pci_disable_device() at the end, thus __e1000_resume()
should call pci_enable_device_mem() to keep enable counter in balance.

Bug was introduced in commit 23606cf5d1192c2b17912cb2ef6e62f9b11de133
("e1000e / PCI / PM: Add basic runtime PM support (rev. 4)") in v2.6.35

Signed-off-by: Konstantin Khlebnikov <khlebnikov@openvz.org>
Cc: e1000-devel@lists.sourceforge.net
Cc: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
Cc: Bruce Allan <bruce.w.allan@intel.com>
---
 drivers/net/ethernet/intel/e1000e/netdev.c |    7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/net/ethernet/intel/e1000e/netdev.c b/drivers/net/ethernet/intel/e1000e/netdev.c
index 2853c11..6bce796 100644
--- a/drivers/net/ethernet/intel/e1000e/netdev.c
+++ b/drivers/net/ethernet/intel/e1000e/netdev.c
@@ -5598,6 +5598,13 @@ static int __e1000_resume(struct pci_dev *pdev)
 	pci_restore_state(pdev);
 	pci_save_state(pdev);
 
+	err = pci_enable_device_mem(pdev);
+	if (err) {
+		dev_err(&pdev->dev,
+			"Cannot re-enable PCI device after suspend.\n");
+		return err;
+	}
+
 	e1000e_set_interrupt_capability(adapter);
 	if (netif_running(netdev)) {
 		err = e1000_request_irq(adapter);

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

[Konstantin Khlebnikov]

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(-)

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;
 }

[PATCH 4/5] PCI: don't touch enable_cnt in pci_device_shutdown()

[Konstantin Khlebnikov]

comment in commit b566a22c23327f18ce941ffad0ca907e50a53d41
("PCI: disable Bus Master on PCI device shutdown") says:

| Disable Bus Master bit on the device in pci_device_shutdown() to ensure PCI
| devices do not continue to DMA data after shutdown.  This can cause memory
| corruption in case of a kexec where the current kernel shuts down and
| transfers control to a new kernel while a PCI device continues to DMA to
| memory that does not belong to it any more in the new kernel.

Seems like pci_clear_master() must be used here instead of pci_disable_device(),
because it disables Bus Muster unconditionally and doesn't changes enable_cnt.

Signed-off-by: Konstantin Khlebnikov <khlebnikov@openvz.org>
Cc: linux-pci@vger.kernel.org
Cc: Bjorn Helgaas <bhelgaas@google.com>
Cc: Khalid Aziz <khalid.aziz@hp.com>
---
 drivers/pci/pci-driver.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
index 030dbf0..853d605 100644
--- a/drivers/pci/pci-driver.c
+++ b/drivers/pci/pci-driver.c
@@ -392,7 +392,7 @@ static void pci_device_shutdown(struct device *dev)
 	 * Turn off Bus Master bit on the device to tell it to not
 	 * continue to do DMA
 	 */
-	pci_disable_device(pci_dev);
+	pci_clear_master(pci_dev);
 }
 
 #ifdef CONFIG_PM

[PATCH 5/5] PCI: catch enable-counter underflows

[Konstantin Khlebnikov]

This patch adds single WARN_ONCE() check for catching 'enable_cnt' imbalances.

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

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 5cb5820..ff93f8f 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -1401,6 +1401,9 @@ pci_disable_device(struct pci_dev *dev)
 	if (dr)
 		dr->enabled = 0;
 
+	dev_WARN_ONCE(&dev->dev, atomic_read(&dev->enable_cnt) <= 0,
+			"enable counter underflow");
+
 	if (atomic_sub_return(1, &dev->enable_cnt) != 0)
 		return;
 

Re: [PATCH 4/5] PCI: don't touch enable_cnt in pci_device_shutdown()

[Don Dutile]

On 01/18/2013 06:42 AM, Konstantin Khlebnikov wrote:
> comment in commit b566a22c23327f18ce941ffad0ca907e50a53d41
> ("PCI: disable Bus Master on PCI device shutdown") says:
>
> | Disable Bus Master bit on the device in pci_device_shutdown() to ensure PCI
> | devices do not continue to DMA data after shutdown.  This can cause memory
> | corruption in case of a kexec where the current kernel shuts down and
> | transfers control to a new kernel while a PCI device continues to DMA to
> | memory that does not belong to it any more in the new kernel.
>
> Seems like pci_clear_master() must be used here instead of pci_disable_device(),
> because it disables Bus Muster unconditionally and doesn't changes enable_cnt.
>
> Signed-off-by: Konstantin Khlebnikov<khlebnikov@openvz.org>
> Cc: linux-pci@vger.kernel.org
> Cc: Bjorn Helgaas<bhelgaas@google.com>
> Cc: Khalid Aziz<khalid.aziz@hp.com>

Hmmm.... wondering if this was the problem why kexec folks
said that device_shutdown() didn't work on all systems (when
trying to stop DMA, esp. on IOMMU-enabled systems...).

Bjorn: do you have a list &/or contact in kexec space to try this
patch vs the "reset every PCI bus" strategy that is currently
being pushed for kexec's method to halt DMA from a PCI device ?

> ---
>   drivers/pci/pci-driver.c |    2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
> index 030dbf0..853d605 100644
> --- a/drivers/pci/pci-driver.c
> +++ b/drivers/pci/pci-driver.c
> @@ -392,7 +392,7 @@ static void pci_device_shutdown(struct device *dev)
>   	 * Turn off Bus Master bit on the device to tell it to not
>   	 * continue to do DMA
>   	 */
> -	pci_disable_device(pci_dev);
> +	pci_clear_master(pci_dev);
>   }
>
>   #ifdef CONFIG_PM
>
> --
> 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

Re: [PATCH 1/5] e1000e: fix resuming from runtime-suspend

[Bjorn Helgaas]

[+cc Rafael, author of patch you cited]

On Fri, Jan 18, 2013 at 4:42 AM, Konstantin Khlebnikov
<khlebnikov@openvz.org> wrote:
> Bug was introduced in commit 23606cf5d1192c2b17912cb2ef6e62f9b11de133
> ("e1000e / PCI / PM: Add basic runtime PM support (rev. 4)") in v2.6.35
>
> Signed-off-by: Konstantin Khlebnikov <khlebnikov@openvz.org>
> Cc: e1000-devel@lists.sourceforge.net
> Cc: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
> Cc: Bruce Allan <bruce.w.allan@intel.com>
> ---
>  drivers/net/ethernet/intel/e1000e/netdev.c |   13 ++++++++-----
>  1 file changed, 8 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/net/ethernet/intel/e1000e/netdev.c b/drivers/net/ethernet/intel/e1000e/netdev.c
> index fbf75fd..2853c11 100644
> --- a/drivers/net/ethernet/intel/e1000e/netdev.c
> +++ b/drivers/net/ethernet/intel/e1000e/netdev.c
> @@ -5691,14 +5691,17 @@ static int e1000_runtime_suspend(struct device *dev)
>         struct pci_dev *pdev = to_pci_dev(dev);
>         struct net_device *netdev = pci_get_drvdata(pdev);
>         struct e1000_adapter *adapter = netdev_priv(netdev);
> +       int retval;
> +       bool wake;
>
> -       if (e1000e_pm_ready(adapter)) {
> -               bool wake;
> +       if (!e1000e_pm_ready(adapter))
> +               return 0;
>
> -               __e1000_shutdown(pdev, &wake, true);
> -       }
> +       retval = __e1000_shutdown(pdev, &wake, true);
> +       if (!retval)
> +               e1000_power_off(pdev, true, wake);
>
> -       return 0;
> +       return retval;
>  }
>
>  static int e1000_idle(struct device *dev)
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 2/5] e1000e: fix pci device enable counter balance

[Bjorn Helgaas]

[+cc Rafael]

On Fri, Jan 18, 2013 at 4:42 AM, Konstantin Khlebnikov
<khlebnikov@openvz.org> wrote:
> __e1000_shutdown() calls pci_disable_device() at the end, thus __e1000_resume()
> should call pci_enable_device_mem() to keep enable counter in balance.
>
> Bug was introduced in commit 23606cf5d1192c2b17912cb2ef6e62f9b11de133
> ("e1000e / PCI / PM: Add basic runtime PM support (rev. 4)") in v2.6.35
>
> Signed-off-by: Konstantin Khlebnikov <khlebnikov@openvz.org>
> Cc: e1000-devel@lists.sourceforge.net
> Cc: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
> Cc: Bruce Allan <bruce.w.allan@intel.com>
> ---
>  drivers/net/ethernet/intel/e1000e/netdev.c |    7 +++++++
>  1 file changed, 7 insertions(+)
>
> diff --git a/drivers/net/ethernet/intel/e1000e/netdev.c b/drivers/net/ethernet/intel/e1000e/netdev.c
> index 2853c11..6bce796 100644
> --- a/drivers/net/ethernet/intel/e1000e/netdev.c
> +++ b/drivers/net/ethernet/intel/e1000e/netdev.c
> @@ -5598,6 +5598,13 @@ static int __e1000_resume(struct pci_dev *pdev)
>         pci_restore_state(pdev);
>         pci_save_state(pdev);
>
> +       err = pci_enable_device_mem(pdev);
> +       if (err) {
> +               dev_err(&pdev->dev,
> +                       "Cannot re-enable PCI device after suspend.\n");
> +               return err;
> +       }

Reviewed-by: Bjorn Helgaas <bhelgaas@google.com>

Seems right to me, and the other users I looked at (igb, azx,
virtio_pci) call pci_disable_device() in .suspend() and call
pci_enable_device() in .resume() as you propose to do here.

I assume the e1000 folks will handle this patch (and the previous one).

> +
>         e1000e_set_interrupt_capability(adapter);
>         if (netif_running(netdev)) {
>                 err = e1000_request_irq(adapter);
>
> --
> 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

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

[Bjorn Helgaas]

[+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;
>  }
>

Re: [PATCH 4/5] PCI: don't touch enable_cnt in pci_device_shutdown()

[Bjorn Helgaas]

On Fri, Jan 18, 2013 at 4:42 AM, Konstantin Khlebnikov
<khlebnikov@openvz.org> wrote:
> comment in commit b566a22c23327f18ce941ffad0ca907e50a53d41
> ("PCI: disable Bus Master on PCI device shutdown") says:
>
> | Disable Bus Master bit on the device in pci_device_shutdown() to ensure PCI
> | devices do not continue to DMA data after shutdown.  This can cause memory
> | corruption in case of a kexec where the current kernel shuts down and
> | transfers control to a new kernel while a PCI device continues to DMA to
> | memory that does not belong to it any more in the new kernel.
>
> Seems like pci_clear_master() must be used here instead of pci_disable_device(),
> because it disables Bus Muster unconditionally and doesn't changes enable_cnt.
>
> Signed-off-by: Konstantin Khlebnikov <khlebnikov@openvz.org>
> Cc: linux-pci@vger.kernel.org
> Cc: Bjorn Helgaas <bhelgaas@google.com>
> Cc: Khalid Aziz <khalid.aziz@hp.com>
> ---
>  drivers/pci/pci-driver.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
> index 030dbf0..853d605 100644
> --- a/drivers/pci/pci-driver.c
> +++ b/drivers/pci/pci-driver.c
> @@ -392,7 +392,7 @@ static void pci_device_shutdown(struct device *dev)
>          * Turn off Bus Master bit on the device to tell it to not
>          * continue to do DMA
>          */
> -       pci_disable_device(pci_dev);
> +       pci_clear_master(pci_dev);

We currently only call pci_enable_device() and pci_disable_device()
from drivers, and I think that's a nice division that's worth keeping.
 It keeps the core's mitts off device operation and helps preserve the
enable_cnt integrity.  So I like this change from that perspective.

Any objections to this, Khalid?

>  }
>
>  #ifdef CONFIG_PM
>

Re: [PATCH 4/5] PCI: don't touch enable_cnt in pci_device_shutdown()

[Bjorn Helgaas]

[+cc Khalid new email addr]

On Mon, Jan 28, 2013 at 4:40 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> On Fri, Jan 18, 2013 at 4:42 AM, Konstantin Khlebnikov
> <khlebnikov@openvz.org> wrote:
>> comment in commit b566a22c23327f18ce941ffad0ca907e50a53d41
>> ("PCI: disable Bus Master on PCI device shutdown") says:
>>
>> | Disable Bus Master bit on the device in pci_device_shutdown() to ensure PCI
>> | devices do not continue to DMA data after shutdown.  This can cause memory
>> | corruption in case of a kexec where the current kernel shuts down and
>> | transfers control to a new kernel while a PCI device continues to DMA to
>> | memory that does not belong to it any more in the new kernel.
>>
>> Seems like pci_clear_master() must be used here instead of pci_disable_device(),
>> because it disables Bus Muster unconditionally and doesn't changes enable_cnt.
>>
>> Signed-off-by: Konstantin Khlebnikov <khlebnikov@openvz.org>
>> Cc: linux-pci@vger.kernel.org
>> Cc: Bjorn Helgaas <bhelgaas@google.com>
>> Cc: Khalid Aziz <khalid.aziz@hp.com>
>> ---
>>  drivers/pci/pci-driver.c |    2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
>> index 030dbf0..853d605 100644
>> --- a/drivers/pci/pci-driver.c
>> +++ b/drivers/pci/pci-driver.c
>> @@ -392,7 +392,7 @@ static void pci_device_shutdown(struct device *dev)
>>          * Turn off Bus Master bit on the device to tell it to not
>>          * continue to do DMA
>>          */
>> -       pci_disable_device(pci_dev);
>> +       pci_clear_master(pci_dev);
>
> We currently only call pci_enable_device() and pci_disable_device()
> from drivers, and I think that's a nice division that's worth keeping.
>  It keeps the core's mitts off device operation and helps preserve the
> enable_cnt integrity.  So I like this change from that perspective.
>
> Any objections to this, Khalid?
>
>>  }
>>
>>  #ifdef CONFIG_PM
>>

Re: [PATCH 2/5] e1000e: fix pci device enable counter balance

[Bjorn Helgaas]

[+cc Rafael @sisk.pl]

On Mon, Jan 28, 2013 at 4:09 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> [+cc Rafael]
>
> On Fri, Jan 18, 2013 at 4:42 AM, Konstantin Khlebnikov
> <khlebnikov@openvz.org> wrote:
>> __e1000_shutdown() calls pci_disable_device() at the end, thus __e1000_resume()
>> should call pci_enable_device_mem() to keep enable counter in balance.
>>
>> Bug was introduced in commit 23606cf5d1192c2b17912cb2ef6e62f9b11de133
>> ("e1000e / PCI / PM: Add basic runtime PM support (rev. 4)") in v2.6.35
>>
>> Signed-off-by: Konstantin Khlebnikov <khlebnikov@openvz.org>
>> Cc: e1000-devel@lists.sourceforge.net
>> Cc: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
>> Cc: Bruce Allan <bruce.w.allan@intel.com>
>> ---
>>  drivers/net/ethernet/intel/e1000e/netdev.c |    7 +++++++
>>  1 file changed, 7 insertions(+)
>>
>> diff --git a/drivers/net/ethernet/intel/e1000e/netdev.c b/drivers/net/ethernet/intel/e1000e/netdev.c
>> index 2853c11..6bce796 100644
>> --- a/drivers/net/ethernet/intel/e1000e/netdev.c
>> +++ b/drivers/net/ethernet/intel/e1000e/netdev.c
>> @@ -5598,6 +5598,13 @@ static int __e1000_resume(struct pci_dev *pdev)
>>         pci_restore_state(pdev);
>>         pci_save_state(pdev);
>>
>> +       err = pci_enable_device_mem(pdev);
>> +       if (err) {
>> +               dev_err(&pdev->dev,
>> +                       "Cannot re-enable PCI device after suspend.\n");
>> +               return err;
>> +       }
>
> Reviewed-by: Bjorn Helgaas <bhelgaas@google.com>
>
> Seems right to me, and the other users I looked at (igb, azx,
> virtio_pci) call pci_disable_device() in .suspend() and call
> pci_enable_device() in .resume() as you propose to do here.
>
> I assume the e1000 folks will handle this patch (and the previous one).
>
>> +
>>         e1000e_set_interrupt_capability(adapter);
>>         if (netif_running(netdev)) {
>>                 err = e1000_request_irq(adapter);
>>

I'm still missing something.  In your original report
(https://lkml.org/lkml/2013/1/1/25), you noticed that "enable_cnt ==
0" immediately after boot, after e1000e had claimed the device:

> Right after boot it looks like this:
>
> root@zurg:/sys/bus/pci/devices# lspci
> ...
> 00:19.0 Ethernet controller: Intel Corporation 82579LM Gigabit Network Connection (rev 04)
> ...
> root@zurg:/sys/bus/pci/devices# cat 0000\:00\:19.0/enable
> 0
> here must be '1', not '0'

But these patches only change the e1000e suspend/resume path.  How
could they change the enable_cnt before you've even done a suspend?

Bjorn

Re: [PATCH 1/5] e1000e: fix resuming from runtime-suspend

[Rafael J. Wysocki]

On Monday, January 28, 2013 04:05:33 PM Bjorn Helgaas wrote:
> [+cc Rafael, author of patch you cited]
> 
> On Fri, Jan 18, 2013 at 4:42 AM, Konstantin Khlebnikov
> <khlebnikov@openvz.org> wrote:
> > Bug was introduced in commit 23606cf5d1192c2b17912cb2ef6e62f9b11de133
> > ("e1000e / PCI / PM: Add basic runtime PM support (rev. 4)") in v2.6.35
> >
> > Signed-off-by: Konstantin Khlebnikov <khlebnikov@openvz.org>
> > Cc: e1000-devel@lists.sourceforge.net
> > Cc: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
> > Cc: Bruce Allan <bruce.w.allan@intel.com>
> > ---
> >  drivers/net/ethernet/intel/e1000e/netdev.c |   13 ++++++++-----
> >  1 file changed, 8 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/intel/e1000e/netdev.c b/drivers/net/ethernet/intel/e1000e/netdev.c
> > index fbf75fd..2853c11 100644
> > --- a/drivers/net/ethernet/intel/e1000e/netdev.c
> > +++ b/drivers/net/ethernet/intel/e1000e/netdev.c
> > @@ -5691,14 +5691,17 @@ static int e1000_runtime_suspend(struct device *dev)
> >         struct pci_dev *pdev = to_pci_dev(dev);
> >         struct net_device *netdev = pci_get_drvdata(pdev);
> >         struct e1000_adapter *adapter = netdev_priv(netdev);
> > +       int retval;
> > +       bool wake;
> >
> > -       if (e1000e_pm_ready(adapter)) {
> > -               bool wake;
> > +       if (!e1000e_pm_ready(adapter))
> > +               return 0;
> >
> > -               __e1000_shutdown(pdev, &wake, true);
> > -       }
> > +       retval = __e1000_shutdown(pdev, &wake, true);
> > +       if (!retval)
> > +               e1000_power_off(pdev, true, wake);
> >
> > -       return 0;
> > +       return retval;
> >  }
> >
> >  static int e1000_idle(struct device *dev)
> >

I'd like the changelog to say what the bug is and how it is being fixed in
general terms.

Thanks,
Rafael


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

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

[Rafael J. Wysocki]

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


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH 4/5] PCI: don't touch enable_cnt in pci_device_shutdown()

[Khalid Aziz]

[Sorry about the resend. gmail sent my original reply in html by
default which didn't go through to all mailing lists]

On Mon, Jan 28, 2013 at 4:44 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
>
> [+cc Khalid new email addr]
>
> On Mon, Jan 28, 2013 at 4:40 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> > On Fri, Jan 18, 2013 at 4:42 AM, Konstantin Khlebnikov
> > <khlebnikov@openvz.org> wrote:
> >> comment in commit b566a22c23327f18ce941ffad0ca907e50a53d41
> >> ("PCI: disable Bus Master on PCI device shutdown") says:
> >>
> >> | Disable Bus Master bit on the device in pci_device_shutdown() to ensure PCI
> >> | devices do not continue to DMA data after shutdown.  This can cause memory
> >> | corruption in case of a kexec where the current kernel shuts down and
> >> | transfers control to a new kernel while a PCI device continues to DMA to
> >> | memory that does not belong to it any more in the new kernel.
> >>
> >> Seems like pci_clear_master() must be used here instead of pci_disable_device(),
> >> because it disables Bus Muster unconditionally and doesn't changes enable_cnt.
> >>
> >> Signed-off-by: Konstantin Khlebnikov <khlebnikov@openvz.org>
> >> Cc: linux-pci@vger.kernel.org
> >> Cc: Bjorn Helgaas <bhelgaas@google.com>
> >> Cc: Khalid Aziz <khalid.aziz@hp.com>
> >> ---
> >>  drivers/pci/pci-driver.c |    2 +-
> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
> >> index 030dbf0..853d605 100644
> >> --- a/drivers/pci/pci-driver.c
> >> +++ b/drivers/pci/pci-driver.c
> >> @@ -392,7 +392,7 @@ static void pci_device_shutdown(struct device *dev)
> >>          * Turn off Bus Master bit on the device to tell it to not
> >>          * continue to do DMA
> >>          */
> >> -       pci_disable_device(pci_dev);
> >> +       pci_clear_master(pci_dev);
> >
> > We currently only call pci_enable_device() and pci_disable_device()
> > from drivers, and I think that's a nice division that's worth keeping.
> >  It keeps the core's mitts off device operation and helps preserve the
> > enable_cnt integrity.  So I like this change from that perspective.
> >
> > Any objections to this, Khalid?

Just turning the Bus Master bit off on a device does worry me. For
well behaved PCI devices this should not be an issue at all, but it
has been brought up before that there are misbehaved PCI devices that
do not respond well when Bus Master bit is cleared on them while they
are active. Turning interrupts off as well on them seems to help with
being able to re-initialize them in the new kernel.  As a result, I
wanted to make sure we at least call  pcibios_disable_irq() to turn
off their interrupt and hence I chose to call pci_disable_device()
instead of pci_clear_master(). I understand the concern around messing
up enable_cnt. Would it be better to replace pci_disable_device() with
these two instead:

pci_clear_master(pci_dev);
pcibios_disable_device(pci_dev);

--
Khalid

Re: [PATCH 1/5] e1000e: fix resuming from runtime-suspend

[Konstantin Khlebnikov]

Rafael J. Wysocki wrote:
> On Monday, January 28, 2013 04:05:33 PM Bjorn Helgaas wrote:
>> [+cc Rafael, author of patch you cited]
>>
>> On Fri, Jan 18, 2013 at 4:42 AM, Konstantin Khlebnikov
>> <khlebnikov@openvz.org>  wrote:
>>> Bug was introduced in commit 23606cf5d1192c2b17912cb2ef6e62f9b11de133
>>> ("e1000e / PCI / PM: Add basic runtime PM support (rev. 4)") in v2.6.35
>>>
>>> Signed-off-by: Konstantin Khlebnikov<khlebnikov@openvz.org>
>>> Cc: e1000-devel@lists.sourceforge.net
>>> Cc: Jeff Kirsher<jeffrey.t.kirsher@intel.com>
>>> Cc: Bruce Allan<bruce.w.allan@intel.com>
>>> ---
>>>   drivers/net/ethernet/intel/e1000e/netdev.c |   13 ++++++++-----
>>>   1 file changed, 8 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/drivers/net/ethernet/intel/e1000e/netdev.c b/drivers/net/ethernet/intel/e1000e/netdev.c
>>> index fbf75fd..2853c11 100644
>>> --- a/drivers/net/ethernet/intel/e1000e/netdev.c
>>> +++ b/drivers/net/ethernet/intel/e1000e/netdev.c
>>> @@ -5691,14 +5691,17 @@ static int e1000_runtime_suspend(struct device *dev)
>>>          struct pci_dev *pdev = to_pci_dev(dev);
>>>          struct net_device *netdev = pci_get_drvdata(pdev);
>>>          struct e1000_adapter *adapter = netdev_priv(netdev);
>>> +       int retval;
>>> +       bool wake;
>>>
>>> -       if (e1000e_pm_ready(adapter)) {
>>> -               bool wake;
>>> +       if (!e1000e_pm_ready(adapter))
>>> +               return 0;
>>>
>>> -               __e1000_shutdown(pdev,&wake, true);
>>> -       }
>>> +       retval = __e1000_shutdown(pdev,&wake, true);
>>> +       if (!retval)
>>> +               e1000_power_off(pdev, true, wake);
>>>
>>> -       return 0;
>>> +       return retval;
>>>   }
>>>
>>>   static int e1000_idle(struct device *dev)
>>>
>
> I'd like the changelog to say what the bug is and how it is being fixed in
> general terms.

Ok, my bad.

Problem: ethernet device does not work (no carrier signal).
Right after boot it goes to runtime-suspend and never wake up.

Original code (before your commit) calls pci_prepare_to_sleep() and it
calls pci_enable_wake() and switches device to one of D3 state.

It seems redundant, because pci_pm_runtime_suspend() do the same thing
after calling ->runtime_suspend callback. Or rather it did it before commit
42eca2302146fed51335b95128e949ee6f54478f ("PCI: Don't touch card regs after
runtime suspend D3") and third patch aimed fix this damage.

More over seems like calling pci_enable_wake() from e1000e isn't enough for my case,
because my enthernet cannot wakeup from runtime-suspend without third patch.
Seems like it's because pci_enable_wake() and pci_finish_runtime_suspend()
calls different pratform-pm callbacks -- platform_pci_run_wake() /
platform_pci_sleep_wake().

All this looks messy and I don't know how it should work.

If you prefer to minimize changes -- I can test how it would work without
first (this) patch. Probably fine.

>
> Thanks,
> Rafael
>
>

Re: [PATCH 2/5] e1000e: fix pci device enable counter balance

[Konstantin Khlebnikov]

Bjorn Helgaas wrote:
> [+cc Rafael @sisk.pl]
>
> On Mon, Jan 28, 2013 at 4:09 PM, Bjorn Helgaas<bhelgaas@google.com>  wrote:
>> [+cc Rafael]
>>
>> On Fri, Jan 18, 2013 at 4:42 AM, Konstantin Khlebnikov
>> <khlebnikov@openvz.org>  wrote:
>>> __e1000_shutdown() calls pci_disable_device() at the end, thus __e1000_resume()
>>> should call pci_enable_device_mem() to keep enable counter in balance.
>>>
>>> Bug was introduced in commit 23606cf5d1192c2b17912cb2ef6e62f9b11de133
>>> ("e1000e / PCI / PM: Add basic runtime PM support (rev. 4)") in v2.6.35
>>>
>>> Signed-off-by: Konstantin Khlebnikov<khlebnikov@openvz.org>
>>> Cc: e1000-devel@lists.sourceforge.net
>>> Cc: Jeff Kirsher<jeffrey.t.kirsher@intel.com>
>>> Cc: Bruce Allan<bruce.w.allan@intel.com>
>>> ---
>>>   drivers/net/ethernet/intel/e1000e/netdev.c |    7 +++++++
>>>   1 file changed, 7 insertions(+)
>>>
>>> diff --git a/drivers/net/ethernet/intel/e1000e/netdev.c b/drivers/net/ethernet/intel/e1000e/netdev.c
>>> index 2853c11..6bce796 100644
>>> --- a/drivers/net/ethernet/intel/e1000e/netdev.c
>>> +++ b/drivers/net/ethernet/intel/e1000e/netdev.c
>>> @@ -5598,6 +5598,13 @@ static int __e1000_resume(struct pci_dev *pdev)
>>>          pci_restore_state(pdev);
>>>          pci_save_state(pdev);
>>>
>>> +       err = pci_enable_device_mem(pdev);
>>> +       if (err) {
>>> +               dev_err(&pdev->dev,
>>> +                       "Cannot re-enable PCI device after suspend.\n");
>>> +               return err;
>>> +       }
>>
>> Reviewed-by: Bjorn Helgaas<bhelgaas@google.com>
>>
>> Seems right to me, and the other users I looked at (igb, azx,
>> virtio_pci) call pci_disable_device() in .suspend() and call
>> pci_enable_device() in .resume() as you propose to do here.
>>
>> I assume the e1000 folks will handle this patch (and the previous one).
>>
>>> +
>>>          e1000e_set_interrupt_capability(adapter);
>>>          if (netif_running(netdev)) {
>>>                  err = e1000_request_irq(adapter);
>>>
>
> I'm still missing something.  In your original report
> (https://lkml.org/lkml/2013/1/1/25), you noticed that "enable_cnt ==
> 0" immediately after boot, after e1000e had claimed the device:

Yep, it rise counter from 0 to 1, and runtime-suspend immediately
decrease it back to 0.

>
>> Right after boot it looks like this:
>>
>> root@zurg:/sys/bus/pci/devices# lspci
>> ...
>> 00:19.0 Ethernet controller: Intel Corporation 82579LM Gigabit Network Connection (rev 04)
>> ...
>> root@zurg:/sys/bus/pci/devices# cat 0000\:00\:19.0/enable
>> 0
>> here must be '1', not '0'
>
> But these patches only change the e1000e suspend/resume path.  How
> could they change the enable_cnt before you've even done a suspend?

suspend/resume and runtime_suspend/runtime_resume callbacks calls the one
set of functions: __e1000_shutdown() / __e1000_resume()

Any suspend-resume cycle breaks enable_ent balance.
Thus right after boot and first runtime-suspend device cannot wake up
due to first sort of bugs and after first s2ram suspend-resume cycle
driver breaks it's enable_cnt and device no longer can sleep due to
second sort of bugs.

>
> Bjorn

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

[Konstantin Khlebnikov]

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

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

[Rafael J. Wysocki]

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


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH 1/5] e1000e: fix resuming from runtime-suspend

[Rafael J. Wysocki]

On Tuesday, January 29, 2013 10:32:14 AM Konstantin Khlebnikov wrote:
> Rafael J. Wysocki wrote:
> > On Monday, January 28, 2013 04:05:33 PM Bjorn Helgaas wrote:
> >> [+cc Rafael, author of patch you cited]
> >>
> >> On Fri, Jan 18, 2013 at 4:42 AM, Konstantin Khlebnikov
> >> <khlebnikov@openvz.org>  wrote:
> >>> Bug was introduced in commit 23606cf5d1192c2b17912cb2ef6e62f9b11de133
> >>> ("e1000e / PCI / PM: Add basic runtime PM support (rev. 4)") in v2.6.35
> >>>
> >>> Signed-off-by: Konstantin Khlebnikov<khlebnikov@openvz.org>
> >>> Cc: e1000-devel@lists.sourceforge.net
> >>> Cc: Jeff Kirsher<jeffrey.t.kirsher@intel.com>
> >>> Cc: Bruce Allan<bruce.w.allan@intel.com>
> >>> ---
> >>>   drivers/net/ethernet/intel/e1000e/netdev.c |   13 ++++++++-----
> >>>   1 file changed, 8 insertions(+), 5 deletions(-)
> >>>
> >>> diff --git a/drivers/net/ethernet/intel/e1000e/netdev.c b/drivers/net/ethernet/intel/e1000e/netdev.c
> >>> index fbf75fd..2853c11 100644
> >>> --- a/drivers/net/ethernet/intel/e1000e/netdev.c
> >>> +++ b/drivers/net/ethernet/intel/e1000e/netdev.c
> >>> @@ -5691,14 +5691,17 @@ static int e1000_runtime_suspend(struct device *dev)
> >>>          struct pci_dev *pdev = to_pci_dev(dev);
> >>>          struct net_device *netdev = pci_get_drvdata(pdev);
> >>>          struct e1000_adapter *adapter = netdev_priv(netdev);
> >>> +       int retval;
> >>> +       bool wake;
> >>>
> >>> -       if (e1000e_pm_ready(adapter)) {
> >>> -               bool wake;
> >>> +       if (!e1000e_pm_ready(adapter))
> >>> +               return 0;
> >>>
> >>> -               __e1000_shutdown(pdev,&wake, true);
> >>> -       }
> >>> +       retval = __e1000_shutdown(pdev,&wake, true);
> >>> +       if (!retval)
> >>> +               e1000_power_off(pdev, true, wake);
> >>>
> >>> -       return 0;
> >>> +       return retval;
> >>>   }
> >>>
> >>>   static int e1000_idle(struct device *dev)
> >>>
> >
> > I'd like the changelog to say what the bug is and how it is being fixed in
> > general terms.
> 
> Ok, my bad.
> 
> Problem: ethernet device does not work (no carrier signal).
> Right after boot it goes to runtime-suspend and never wake up.
> 
> Original code (before your commit) calls pci_prepare_to_sleep() and it
> calls pci_enable_wake() and switches device to one of D3 state.
> 
> It seems redundant, because pci_pm_runtime_suspend() do the same thing
> after calling ->runtime_suspend callback. Or rather it did it before commit
> 42eca2302146fed51335b95128e949ee6f54478f ("PCI: Don't touch card regs after
> runtime suspend D3") and third patch aimed fix this damage.
> 
> More over seems like calling pci_enable_wake() from e1000e isn't enough for my case,
> because my enthernet cannot wakeup from runtime-suspend without third patch.
> Seems like it's because pci_enable_wake() and pci_finish_runtime_suspend()
> calls different pratform-pm callbacks -- platform_pci_run_wake() /
> platform_pci_sleep_wake().
> 
> All this looks messy and I don't know how it should work.
> 
> If you prefer to minimize changes -- I can test how it would work without
> first (this) patch. Probably fine.

OK, I need to have a look at that driver, then.  Please give me some time,
though, I'm working on something different at the moment.

And by the way, by sending patches you declare that you know what you're doing.
If you really don't, it's better to just describe the problem. :-)

Thanks,
Rafael


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH 2/5] e1000e: fix pci device enable counter balance

[Rafael J. Wysocki]

On Monday, January 28, 2013 04:09:38 PM Bjorn Helgaas wrote:
> [+cc Rafael]
> 
> On Fri, Jan 18, 2013 at 4:42 AM, Konstantin Khlebnikov
> <khlebnikov@openvz.org> wrote:
> > __e1000_shutdown() calls pci_disable_device() at the end, thus __e1000_resume()
> > should call pci_enable_device_mem() to keep enable counter in balance.
> >
> > Bug was introduced in commit 23606cf5d1192c2b17912cb2ef6e62f9b11de133
> > ("e1000e / PCI / PM: Add basic runtime PM support (rev. 4)") in v2.6.35
> >
> > Signed-off-by: Konstantin Khlebnikov <khlebnikov@openvz.org>
> > Cc: e1000-devel@lists.sourceforge.net
> > Cc: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
> > Cc: Bruce Allan <bruce.w.allan@intel.com>
> > ---
> >  drivers/net/ethernet/intel/e1000e/netdev.c |    7 +++++++
> >  1 file changed, 7 insertions(+)
> >
> > diff --git a/drivers/net/ethernet/intel/e1000e/netdev.c b/drivers/net/ethernet/intel/e1000e/netdev.c
> > index 2853c11..6bce796 100644
> > --- a/drivers/net/ethernet/intel/e1000e/netdev.c
> > +++ b/drivers/net/ethernet/intel/e1000e/netdev.c
> > @@ -5598,6 +5598,13 @@ static int __e1000_resume(struct pci_dev *pdev)
> >         pci_restore_state(pdev);
> >         pci_save_state(pdev);
> >
> > +       err = pci_enable_device_mem(pdev);
> > +       if (err) {
> > +               dev_err(&pdev->dev,
> > +                       "Cannot re-enable PCI device after suspend.\n");
> > +               return err;
> > +       }
> 
> Reviewed-by: Bjorn Helgaas <bhelgaas@google.com>
> 
> Seems right to me, and the other users I looked at (igb, azx,
> virtio_pci) call pci_disable_device() in .suspend() and call
> pci_enable_device() in .resume() as you propose to do here.
> 
> I assume the e1000 folks will handle this patch (and the previous one).
> 

OK, so this one looks like a genuine fix to me.

Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Thanks,
Rafael


> > +
> >         e1000e_set_interrupt_capability(adapter);
> >         if (netif_running(netdev)) {
> >                 err = e1000_request_irq(adapter);
> >
> > --
-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

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

[Rafael J. Wysocki]

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


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH 1/5] e1000e: fix resuming from runtime-suspend

[Rafael J. Wysocki]

On Tuesday, January 29, 2013 10:32:14 AM Konstantin Khlebnikov wrote:
> Rafael J. Wysocki wrote:
> > On Monday, January 28, 2013 04:05:33 PM Bjorn Helgaas wrote:
> >> [+cc Rafael, author of patch you cited]
> >>
> >> On Fri, Jan 18, 2013 at 4:42 AM, Konstantin Khlebnikov
> >> <khlebnikov@openvz.org>  wrote:
> >>> Bug was introduced in commit 23606cf5d1192c2b17912cb2ef6e62f9b11de133
> >>> ("e1000e / PCI / PM: Add basic runtime PM support (rev. 4)") in v2.6.35
> >>>
> >>> Signed-off-by: Konstantin Khlebnikov<khlebnikov@openvz.org>
> >>> Cc: e1000-devel@lists.sourceforge.net
> >>> Cc: Jeff Kirsher<jeffrey.t.kirsher@intel.com>
> >>> Cc: Bruce Allan<bruce.w.allan@intel.com>
> >>> ---
> >>>   drivers/net/ethernet/intel/e1000e/netdev.c |   13 ++++++++-----
> >>>   1 file changed, 8 insertions(+), 5 deletions(-)
> >>>
> >>> diff --git a/drivers/net/ethernet/intel/e1000e/netdev.c b/drivers/net/ethernet/intel/e1000e/netdev.c
> >>> index fbf75fd..2853c11 100644
> >>> --- a/drivers/net/ethernet/intel/e1000e/netdev.c
> >>> +++ b/drivers/net/ethernet/intel/e1000e/netdev.c
> >>> @@ -5691,14 +5691,17 @@ static int e1000_runtime_suspend(struct device *dev)
> >>>          struct pci_dev *pdev = to_pci_dev(dev);
> >>>          struct net_device *netdev = pci_get_drvdata(pdev);
> >>>          struct e1000_adapter *adapter = netdev_priv(netdev);
> >>> +       int retval;
> >>> +       bool wake;
> >>>
> >>> -       if (e1000e_pm_ready(adapter)) {
> >>> -               bool wake;
> >>> +       if (!e1000e_pm_ready(adapter))
> >>> +               return 0;
> >>>
> >>> -               __e1000_shutdown(pdev,&wake, true);
> >>> -       }
> >>> +       retval = __e1000_shutdown(pdev,&wake, true);
> >>> +       if (!retval)
> >>> +               e1000_power_off(pdev, true, wake);
> >>>
> >>> -       return 0;
> >>> +       return retval;
> >>>   }
> >>>
> >>>   static int e1000_idle(struct device *dev)
> >>>
> >
> > I'd like the changelog to say what the bug is and how it is being fixed in
> > general terms.
> 
> Ok, my bad.
> 
> Problem: ethernet device does not work (no carrier signal).
> Right after boot it goes to runtime-suspend and never wake up.
> 
> Original code (before your commit) calls pci_prepare_to_sleep() and it
> calls pci_enable_wake() and switches device to one of D3 state.

The original code in what function?

> It seems redundant, because pci_pm_runtime_suspend() do the same thing
> after calling ->runtime_suspend callback. Or rather it did it before commit
> 42eca2302146fed51335b95128e949ee6f54478f ("PCI: Don't touch card regs after
> runtime suspend D3") and third patch aimed fix this damage.
> 
> More over seems like calling pci_enable_wake() from e1000e isn't enough for my case,
> because my enthernet cannot wakeup from runtime-suspend without third patch.

Which third patch?

> Seems like it's because pci_enable_wake() and pci_finish_runtime_suspend()
> calls different pratform-pm callbacks -- platform_pci_run_wake() /
> platform_pci_sleep_wake().

That's correct, for historical reasons.  We'll need to merge these things, but
for now the are separate (because of the way ACPI handles system suspend and
runtime PM).

Thanks,
Rafael


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH 1/5] e1000e: fix resuming from runtime-suspend

[Rafael J. Wysocki]

On Friday, January 18, 2013 03:42:14 PM Konstantin Khlebnikov wrote:
> Bug was introduced in commit 23606cf5d1192c2b17912cb2ef6e62f9b11de133
> ("e1000e / PCI / PM: Add basic runtime PM support (rev. 4)") in v2.6.35
> 
> Signed-off-by: Konstantin Khlebnikov <khlebnikov@openvz.org>
> Cc: e1000-devel@lists.sourceforge.net
> Cc: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
> Cc: Bruce Allan <bruce.w.allan@intel.com>
> ---
>  drivers/net/ethernet/intel/e1000e/netdev.c |   13 ++++++++-----
>  1 file changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/e1000e/netdev.c b/drivers/net/ethernet/intel/e1000e/netdev.c
> index fbf75fd..2853c11 100644
> --- a/drivers/net/ethernet/intel/e1000e/netdev.c
> +++ b/drivers/net/ethernet/intel/e1000e/netdev.c
> @@ -5691,14 +5691,17 @@ static int e1000_runtime_suspend(struct device *dev)
>  	struct pci_dev *pdev = to_pci_dev(dev);
>  	struct net_device *netdev = pci_get_drvdata(pdev);
>  	struct e1000_adapter *adapter = netdev_priv(netdev);
> +	int retval;
> +	bool wake;
>  
> -	if (e1000e_pm_ready(adapter)) {
> -		bool wake;
> +	if (!e1000e_pm_ready(adapter))
> +		return 0;
>  
> -		__e1000_shutdown(pdev, &wake, true);
> -	}
> +	retval = __e1000_shutdown(pdev, &wake, true);
> +	if (!retval)
> +		e1000_power_off(pdev, true, wake);

That needs to be pci_finish_runtime_suspend() (which needs to be exported
for this purpose) or remove the pci_save_state() from __e1000_shutdown().

If the latter works, I'd very much prefer it.

Thanks,
Rafael


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH 1/5] e1000e: fix resuming from runtime-suspend

[Konstantin Khlebnikov]

Rafael J. Wysocki wrote:
> On Tuesday, January 29, 2013 10:32:14 AM Konstantin Khlebnikov wrote:
>> Rafael J. Wysocki wrote:
>>> On Monday, January 28, 2013 04:05:33 PM Bjorn Helgaas wrote:
>>>> [+cc Rafael, author of patch you cited]
>>>>
>>>> On Fri, Jan 18, 2013 at 4:42 AM, Konstantin Khlebnikov
>>>> <khlebnikov@openvz.org>   wrote:
>>>>> Bug was introduced in commit 23606cf5d1192c2b17912cb2ef6e62f9b11de133
>>>>> ("e1000e / PCI / PM: Add basic runtime PM support (rev. 4)") in v2.6.35
>>>>>
>>>>> Signed-off-by: Konstantin Khlebnikov<khlebnikov@openvz.org>
>>>>> Cc: e1000-devel@lists.sourceforge.net
>>>>> Cc: Jeff Kirsher<jeffrey.t.kirsher@intel.com>
>>>>> Cc: Bruce Allan<bruce.w.allan@intel.com>
>>>>> ---
>>>>>    drivers/net/ethernet/intel/e1000e/netdev.c |   13 ++++++++-----
>>>>>    1 file changed, 8 insertions(+), 5 deletions(-)
>>>>>
>>>>> diff --git a/drivers/net/ethernet/intel/e1000e/netdev.c b/drivers/net/ethernet/intel/e1000e/netdev.c
>>>>> index fbf75fd..2853c11 100644
>>>>> --- a/drivers/net/ethernet/intel/e1000e/netdev.c
>>>>> +++ b/drivers/net/ethernet/intel/e1000e/netdev.c
>>>>> @@ -5691,14 +5691,17 @@ static int e1000_runtime_suspend(struct device *dev)
>>>>>           struct pci_dev *pdev = to_pci_dev(dev);
>>>>>           struct net_device *netdev = pci_get_drvdata(pdev);
>>>>>           struct e1000_adapter *adapter = netdev_priv(netdev);
>>>>> +       int retval;
>>>>> +       bool wake;
>>>>>
>>>>> -       if (e1000e_pm_ready(adapter)) {
>>>>> -               bool wake;
>>>>> +       if (!e1000e_pm_ready(adapter))
>>>>> +               return 0;
>>>>>
>>>>> -               __e1000_shutdown(pdev,&wake, true);
>>>>> -       }
>>>>> +       retval = __e1000_shutdown(pdev,&wake, true);
>>>>> +       if (!retval)
>>>>> +               e1000_power_off(pdev, true, wake);
>>>>>
>>>>> -       return 0;
>>>>> +       return retval;
>>>>>    }
>>>>>
>>>>>    static int e1000_idle(struct device *dev)
>>>>>
>>>
>>> I'd like the changelog to say what the bug is and how it is being fixed in
>>> general terms.
>>
>> Ok, my bad.
>>
>> Problem: ethernet device does not work (no carrier signal).
>> Right after boot it goes to runtime-suspend and never wake up.
>>
>> Original code (before your commit) calls pci_prepare_to_sleep() and it
>> calls pci_enable_wake() and switches device to one of D3 state.
>
> The original code in what function?

Oh, I screwed up again. That commit adds support of runtime-pm,
so there is no original code. But runtime-pm in 'igb' works in this way:
igb_runtime_suspend() -> pci_prepare_to_sleep() -> pci_enable_wake()

>
>> It seems redundant, because pci_pm_runtime_suspend() do the same thing
>> after calling ->runtime_suspend callback. Or rather it did it before commit
>> 42eca2302146fed51335b95128e949ee6f54478f ("PCI: Don't touch card regs after
>> runtime suspend D3") and third patch aimed fix this damage.
>>
>> More over seems like calling pci_enable_wake() from e1000e isn't enough for my case,
>> because my enthernet cannot wakeup from runtime-suspend without third patch.
>
> Which third patch?

third patch in this patchset:
"[PATCH 3/5] PCI: revert preparing for wakeup in runtime-suspend finalization"

>
>> Seems like it's because pci_enable_wake() and pci_finish_runtime_suspend()
>> calls different pratform-pm callbacks -- platform_pci_run_wake() /
>> platform_pci_sleep_wake().
>
> That's correct, for historical reasons.  We'll need to merge these things, but
> for now the are separate (because of the way ACPI handles system suspend and
> runtime PM).

Ok, I have not read yet all code and documentation. But it seems runtime-pm
engine needs some sort of runtime debug-mode which would warn about strange or
ineffective actions in drivers, probably just script for parsing kernel-log
and tracking states for all devices and buses.

>
> Thanks,
> Rafael
>
>

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

[Konstantin Khlebnikov]

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

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

[Rafael J. Wysocki]

On Saturday, February 02, 2013 04:12:03 PM Konstantin Khlebnikov wrote:
> 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

Well, I wonder if we can just do something like the appended patch instead and
address the e1000e runtime suspend by calling pci_finish_runtime_suspend()
directly from e1000_runtime_suspend().

While we can revert commit 42eca2302146, that hardly would be progress,
because then the issue it was supposed to address would still need to be
addressed somehow.

Thanks,
Rafael


---
 drivers/pci/pci-driver.c |    4 ++++
 1 file changed, 4 insertions(+)

Index: linux-pm/drivers/pci/pci-driver.c
===================================================================
--- linux-pm.orig/drivers/pci/pci-driver.c
+++ linux-pm/drivers/pci/pci-driver.c
@@ -628,6 +628,7 @@ static int pci_pm_suspend(struct device
 		goto Fixup;
 	}
 
+	pci_dev->state_saved = false;
 	if (pm->suspend) {
 		pci_power_t prev = pci_dev->current_state;
 		int error;
@@ -774,6 +775,7 @@ static int pci_pm_freeze(struct device *
 		return 0;
 	}
 
+	pci_dev->state_saved = false;
 	if (pm->freeze) {
 		int error;
 
@@ -862,6 +864,7 @@ static int pci_pm_poweroff(struct device
 		goto Fixup;
 	}
 
+	pci_dev->state_saved = false;
 	if (pm->poweroff) {
 		int error;
 
@@ -987,6 +990,7 @@ static int pci_pm_runtime_suspend(struct
 	if (!pm || !pm->runtime_suspend)
 		return -ENOSYS;
 
+	pci_dev->state_saved = false;
 	pci_dev->no_d3cold = false;
 	error = pm->runtime_suspend(dev);
 	suspend_report_result(pm->runtime_suspend, error);

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

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

[Rafael J. Wysocki]

On Saturday, February 02, 2013 09:58:45 PM Rafael J. Wysocki wrote:
> On Saturday, February 02, 2013 04:12:03 PM Konstantin Khlebnikov wrote:
> > 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
> 
> Well, I wonder if we can just do something like the appended patch instead and
> address the e1000e runtime suspend by calling pci_finish_runtime_suspend()
> directly from e1000_runtime_suspend().
> 
> While we can revert commit 42eca2302146, that hardly would be progress,
> because then the issue it was supposed to address would still need to be
> addressed somehow.
> 
> ---
>  drivers/pci/pci-driver.c |    4 ++++
>  1 file changed, 4 insertions(+)
> 
> Index: linux-pm/drivers/pci/pci-driver.c
> ===================================================================
> --- linux-pm.orig/drivers/pci/pci-driver.c
> +++ linux-pm/drivers/pci/pci-driver.c
> @@ -628,6 +628,7 @@ static int pci_pm_suspend(struct device
>  		goto Fixup;
>  	}
>  
> +	pci_dev->state_saved = false;
>  	if (pm->suspend) {
>  		pci_power_t prev = pci_dev->current_state;
>  		int error;
> @@ -774,6 +775,7 @@ static int pci_pm_freeze(struct device *
>  		return 0;
>  	}
>  
> +	pci_dev->state_saved = false;
>  	if (pm->freeze) {
>  		int error;
>  
> @@ -862,6 +864,7 @@ static int pci_pm_poweroff(struct device
>  		goto Fixup;
>  	}
>  
> +	pci_dev->state_saved = false;
>  	if (pm->poweroff) {
>  		int error;
>  
> @@ -987,6 +990,7 @@ static int pci_pm_runtime_suspend(struct
>  	if (!pm || !pm->runtime_suspend)
>  		return -ENOSYS;
>  
> +	pci_dev->state_saved = false;
>  	pci_dev->no_d3cold = false;
>  	error = pm->runtime_suspend(dev);
>  	suspend_report_result(pm->runtime_suspend, error);

For completness, on top of the above one.

---
 drivers/net/ethernet/intel/e1000e/netdev.c |    1 +
 drivers/pci/pci.c                          |    1 +
 drivers/pci/pci.h                          |    1 -
 include/linux/pci.h                        |    1 +
 4 files changed, 3 insertions(+), 1 deletion(-)

Index: linux-pm/drivers/pci/pci.c
===================================================================
--- linux-pm.orig/drivers/pci/pci.c
+++ linux-pm/drivers/pci/pci.c
@@ -1840,6 +1840,7 @@ int pci_finish_runtime_suspend(struct pc
 
 	return error;
 }
+EXPORT_SYMBOL_GPL(pci_finish_runtime_suspend);
 
 /**
  * pci_dev_run_wake - Check if device can generate run-time wake-up events.
Index: linux-pm/drivers/pci/pci.h
===================================================================
--- linux-pm.orig/drivers/pci/pci.h
+++ linux-pm/drivers/pci/pci.h
@@ -64,7 +64,6 @@ extern int pci_set_platform_pm(struct pc
 extern void pci_update_current_state(struct pci_dev *dev, pci_power_t state);
 extern void pci_power_up(struct pci_dev *dev);
 extern void pci_disable_enabled_device(struct pci_dev *dev);
-extern int pci_finish_runtime_suspend(struct pci_dev *dev);
 extern int __pci_pme_wakeup(struct pci_dev *dev, void *ign);
 extern void pci_wakeup_bus(struct pci_bus *bus);
 extern void pci_config_pm_runtime_get(struct pci_dev *dev);
Index: linux-pm/include/linux/pci.h
===================================================================
--- linux-pm.orig/include/linux/pci.h
+++ linux-pm/include/linux/pci.h
@@ -936,6 +936,7 @@ int pci_back_from_sleep(struct pci_dev *
 bool pci_dev_run_wake(struct pci_dev *dev);
 bool pci_check_pme_status(struct pci_dev *dev);
 void pci_pme_wakeup_bus(struct pci_bus *bus);
+int pci_finish_runtime_suspend(struct pci_dev *dev);
 
 static inline int pci_enable_wake(struct pci_dev *dev, pci_power_t state,
 				  bool enable)
Index: linux-pm/drivers/net/ethernet/intel/e1000e/netdev.c
===================================================================
--- linux-pm.orig/drivers/net/ethernet/intel/e1000e/netdev.c
+++ linux-pm/drivers/net/ethernet/intel/e1000e/netdev.c
@@ -5696,6 +5696,7 @@ static int e1000_runtime_suspend(struct
 		bool wake;
 
 		__e1000_shutdown(pdev, &wake, true);
+		pci_finish_runtime_suspend(pdev);
 	}
 
 	return 0;



-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

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

[Konstantin Khlebnikov]

Rafael J. Wysocki wrote:
> On Saturday, February 02, 2013 09:58:45 PM Rafael J. Wysocki wrote:
>> On Saturday, February 02, 2013 04:12:03 PM Konstantin Khlebnikov wrote:
>>> 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
>>
>> Well, I wonder if we can just do something like the appended patch instead and
>> address the e1000e runtime suspend by calling pci_finish_runtime_suspend()
>> directly from e1000_runtime_suspend().
>>
>> While we can revert commit 42eca2302146, that hardly would be progress,
>> because then the issue it was supposed to address would still need to be
>> addressed somehow.
>>
>> ---
>>   drivers/pci/pci-driver.c |    4 ++++
>>   1 file changed, 4 insertions(+)
>>
>> Index: linux-pm/drivers/pci/pci-driver.c
>> ===================================================================
>> --- linux-pm.orig/drivers/pci/pci-driver.c
>> +++ linux-pm/drivers/pci/pci-driver.c
>> @@ -628,6 +628,7 @@ static int pci_pm_suspend(struct device
>>   		goto Fixup;
>>   	}
>>
>> +	pci_dev->state_saved = false;
>>   	if (pm->suspend) {
>>   		pci_power_t prev = pci_dev->current_state;
>>   		int error;
>> @@ -774,6 +775,7 @@ static int pci_pm_freeze(struct device *
>>   		return 0;
>>   	}
>>
>> +	pci_dev->state_saved = false;
>>   	if (pm->freeze) {
>>   		int error;
>>
>> @@ -862,6 +864,7 @@ static int pci_pm_poweroff(struct device
>>   		goto Fixup;
>>   	}
>>
>> +	pci_dev->state_saved = false;
>>   	if (pm->poweroff) {
>>   		int error;
>>
>> @@ -987,6 +990,7 @@ static int pci_pm_runtime_suspend(struct
>>   	if (!pm || !pm->runtime_suspend)
>>   		return -ENOSYS;
>>
>> +	pci_dev->state_saved = false;
>>   	pci_dev->no_d3cold = false;
>>   	error = pm->runtime_suspend(dev);
>>   	suspend_report_result(pm->runtime_suspend, error);
>
> For completness, on top of the above one.

I would prefer to remove pci_save_state() from e1000e_runtime_suspend().

--- a/drivers/net/ethernet/intel/e1000e/netdev.c
+++ b/drivers/net/ethernet/intel/e1000e/netdev.c
@@ -5429,9 +5429,11 @@ static int __e1000_shutdown(struct pci_dev *pdev, bool *enable_wake,
         }
         e1000e_reset_interrupt_capability(adapter);

-       retval = pci_save_state(pdev);
-       if (retval)
-               return retval;
+       if (!runtime) {
+               retval = pci_save_state(pdev);
+               if (retval)
+                       return retval;
+       }

         status = er32(STATUS);
         if (status & E1000_STATUS_LU)

I found another problem in e1000e: it does not calls pci_enable_master()
in 'resume' functions, but it disables 'bus-mastering' on suspending.
Thus if pci_save_state() is called after clearing that bit whole device
wouldn't work after resuming.

--- a/drivers/net/ethernet/intel/e1000e/netdev.c
+++ b/drivers/net/ethernet/intel/e1000e/netdev.c
@@ -5598,6 +5598,7 @@ static int __e1000_resume(struct pci_dev *pdev)

         pci_set_power_state(pdev, PCI_D0);
         pci_restore_state(pdev);
+       pci_set_master(pdev);
         pci_save_state(pdev);

         err = pci_enable_device_mem(pdev);

>
> ---
>   drivers/net/ethernet/intel/e1000e/netdev.c |    1 +
>   drivers/pci/pci.c                          |    1 +
>   drivers/pci/pci.h                          |    1 -
>   include/linux/pci.h                        |    1 +
>   4 files changed, 3 insertions(+), 1 deletion(-)
>
> Index: linux-pm/drivers/pci/pci.c
> ===================================================================
> --- linux-pm.orig/drivers/pci/pci.c
> +++ linux-pm/drivers/pci/pci.c
> @@ -1840,6 +1840,7 @@ int pci_finish_runtime_suspend(struct pc
>
>   	return error;
>   }
> +EXPORT_SYMBOL_GPL(pci_finish_runtime_suspend);
>
>   /**
>    * pci_dev_run_wake - Check if device can generate run-time wake-up events.
> Index: linux-pm/drivers/pci/pci.h
> ===================================================================
> --- linux-pm.orig/drivers/pci/pci.h
> +++ linux-pm/drivers/pci/pci.h
> @@ -64,7 +64,6 @@ extern int pci_set_platform_pm(struct pc
>   extern void pci_update_current_state(struct pci_dev *dev, pci_power_t state);
>   extern void pci_power_up(struct pci_dev *dev);
>   extern void pci_disable_enabled_device(struct pci_dev *dev);
> -extern int pci_finish_runtime_suspend(struct pci_dev *dev);
>   extern int __pci_pme_wakeup(struct pci_dev *dev, void *ign);
>   extern void pci_wakeup_bus(struct pci_bus *bus);
>   extern void pci_config_pm_runtime_get(struct pci_dev *dev);
> Index: linux-pm/include/linux/pci.h
> ===================================================================
> --- linux-pm.orig/include/linux/pci.h
> +++ linux-pm/include/linux/pci.h
> @@ -936,6 +936,7 @@ int pci_back_from_sleep(struct pci_dev *
>   bool pci_dev_run_wake(struct pci_dev *dev);
>   bool pci_check_pme_status(struct pci_dev *dev);
>   void pci_pme_wakeup_bus(struct pci_bus *bus);
> +int pci_finish_runtime_suspend(struct pci_dev *dev);
>
>   static inline int pci_enable_wake(struct pci_dev *dev, pci_power_t state,
>   				  bool enable)
> Index: linux-pm/drivers/net/ethernet/intel/e1000e/netdev.c
> ===================================================================
> --- linux-pm.orig/drivers/net/ethernet/intel/e1000e/netdev.c
> +++ linux-pm/drivers/net/ethernet/intel/e1000e/netdev.c
> @@ -5696,6 +5696,7 @@ static int e1000_runtime_suspend(struct
>   		bool wake;
>
>   		__e1000_shutdown(pdev,&wake, true);
> +		pci_finish_runtime_suspend(pdev);
>   	}
>
>   	return 0;
>
>
>

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

[Rafael J. Wysocki]

On Sunday, February 03, 2013 02:14:46 PM Konstantin Khlebnikov wrote:
> Rafael J. Wysocki wrote:
> > On Saturday, February 02, 2013 09:58:45 PM Rafael J. Wysocki wrote:
> >> On Saturday, February 02, 2013 04:12:03 PM Konstantin Khlebnikov wrote:
> >>> 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
> >>
> >> Well, I wonder if we can just do something like the appended patch instead and
> >> address the e1000e runtime suspend by calling pci_finish_runtime_suspend()
> >> directly from e1000_runtime_suspend().
> >>
> >> While we can revert commit 42eca2302146, that hardly would be progress,
> >> because then the issue it was supposed to address would still need to be
> >> addressed somehow.
> >>
> >> ---
> >>   drivers/pci/pci-driver.c |    4 ++++
> >>   1 file changed, 4 insertions(+)
> >>
> >> Index: linux-pm/drivers/pci/pci-driver.c
> >> ===================================================================
> >> --- linux-pm.orig/drivers/pci/pci-driver.c
> >> +++ linux-pm/drivers/pci/pci-driver.c
> >> @@ -628,6 +628,7 @@ static int pci_pm_suspend(struct device
> >>   		goto Fixup;
> >>   	}
> >>
> >> +	pci_dev->state_saved = false;
> >>   	if (pm->suspend) {
> >>   		pci_power_t prev = pci_dev->current_state;
> >>   		int error;
> >> @@ -774,6 +775,7 @@ static int pci_pm_freeze(struct device *
> >>   		return 0;
> >>   	}
> >>
> >> +	pci_dev->state_saved = false;
> >>   	if (pm->freeze) {
> >>   		int error;
> >>
> >> @@ -862,6 +864,7 @@ static int pci_pm_poweroff(struct device
> >>   		goto Fixup;
> >>   	}
> >>
> >> +	pci_dev->state_saved = false;
> >>   	if (pm->poweroff) {
> >>   		int error;
> >>
> >> @@ -987,6 +990,7 @@ static int pci_pm_runtime_suspend(struct
> >>   	if (!pm || !pm->runtime_suspend)
> >>   		return -ENOSYS;
> >>
> >> +	pci_dev->state_saved = false;
> >>   	pci_dev->no_d3cold = false;
> >>   	error = pm->runtime_suspend(dev);
> >>   	suspend_report_result(pm->runtime_suspend, error);
> >
> > For completness, on top of the above one.
> 
> I would prefer to remove pci_save_state() from e1000e_runtime_suspend().
> 
> --- a/drivers/net/ethernet/intel/e1000e/netdev.c
> +++ b/drivers/net/ethernet/intel/e1000e/netdev.c
> @@ -5429,9 +5429,11 @@ static int __e1000_shutdown(struct pci_dev *pdev, bool *enable_wake,
>          }
>          e1000e_reset_interrupt_capability(adapter);
> 
> -       retval = pci_save_state(pdev);
> -       if (retval)
> -               return retval;
> +       if (!runtime) {
> +               retval = pci_save_state(pdev);
> +               if (retval)
> +                       return retval;
> +       }
> 
>          status = er32(STATUS);
>          if (status & E1000_STATUS_LU)

Well, I'm not sure if it's necessary to do the pci_save_state() for !runtime
here (i.e. why don't we remove it entirely?), but I'm fine with this change. :-)

> I found another problem in e1000e: it does not calls pci_enable_master()
> in 'resume' functions, but it disables 'bus-mastering' on suspending.
> Thus if pci_save_state() is called after clearing that bit whole device
> wouldn't work after resuming.
> 
> --- a/drivers/net/ethernet/intel/e1000e/netdev.c
> +++ b/drivers/net/ethernet/intel/e1000e/netdev.c
> @@ -5598,6 +5598,7 @@ static int __e1000_resume(struct pci_dev *pdev)
> 
>          pci_set_power_state(pdev, PCI_D0);
>          pci_restore_state(pdev);
> +       pci_set_master(pdev);
>          pci_save_state(pdev);
> 
>          err = pci_enable_device_mem(pdev);
> 

Yeah.  Perhaps you can fold this change into your [2/5]?

Rafael


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

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

[David Airlie]

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

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

[David Airlie]

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

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

[Rafael J. Wysocki]

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


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.