[PATCH v2 0/2] PCI: Fix runtime PME generation from D3hot

[PATCH v2 0/2] PCI: Fix runtime PME generation from D3hot

[Mika Westerberg]

Hi all,

Heiner reported [1] that runtime PME generation of his network card does
not work after commit 0e157e528604 ("PCI/PME: Implement runtime PM
callbacks") that landed in v4.20. Reverting the commit helps but it has
another drawback, which I originally tried to solve with the commit, that
the PCIe hierarchy wakes up immediately after being put into D3cold.

This series of two patches tries to fix both issues so that PME wakes up
from D3hot and that the hierarchy does not wake up immediately from D3cold.

The previous version of the series can be found here:

  https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1892241.html

Changes from the previous version:

  * Add tags from Heiner and Rafael
  * Update changelog to mention relevent PCIe spec sections
  * Add comment to pcie_disable_interrupt() explaining why and what is
    masked

[1] https://www.spinics.net/lists/linux-pci/msg79051.html

Mika Westerberg (2):
  Revert "PCI/PME: Implement runtime PM callbacks"
  PCI: pciehp: Disable Data Link Layer State Changed event on suspend

 drivers/pci/hotplug/pciehp_hpc.c | 17 +++++++++++++++--
 drivers/pci/pcie/pme.c           | 27 ---------------------------
 2 files changed, 15 insertions(+), 29 deletions(-)

-- 
2.20.1

[PATCH v2 1/2] Revert "PCI/PME: Implement runtime PM callbacks"

[Mika Westerberg]

This reverts commit 0e157e52860441cb26051f131dd0b5ae3187a07b.

Heiner reported that the commit in question prevents his network adapter
from triggering PME and waking up when network cable is plugged.

The commit tried to prevent root port waking up from D3cold immediately
but looks like disabing root port PME interrupt is not the right way to
fix that issue so revert it now. The patch following proposes an
alternative solution to that issue.

Link: https://bugzilla.kernel.org/show_bug.cgi?id=202103
Fixes: 0e157e528604 ("PCI/PME: Implement runtime PM callbacks")
Reported-by: Heiner Kallweit <hkallweit1@gmail.com>
Tested-by: Heiner Kallweit <hkallweit1@gmail.com>
Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/pci/pcie/pme.c | 27 ---------------------------
 1 file changed, 27 deletions(-)

diff --git a/drivers/pci/pcie/pme.c b/drivers/pci/pcie/pme.c
index 0dbcf429089f..1a8b85051b1b 100644
--- a/drivers/pci/pcie/pme.c
+++ b/drivers/pci/pcie/pme.c
@@ -432,31 +432,6 @@ static void pcie_pme_remove(struct pcie_device *srv)
 	kfree(get_service_data(srv));
 }
 
-static int pcie_pme_runtime_suspend(struct pcie_device *srv)
-{
-	struct pcie_pme_service_data *data = get_service_data(srv);
-
-	spin_lock_irq(&data->lock);
-	pcie_pme_interrupt_enable(srv->port, false);
-	pcie_clear_root_pme_status(srv->port);
-	data->noirq = true;
-	spin_unlock_irq(&data->lock);
-
-	return 0;
-}
-
-static int pcie_pme_runtime_resume(struct pcie_device *srv)
-{
-	struct pcie_pme_service_data *data = get_service_data(srv);
-
-	spin_lock_irq(&data->lock);
-	pcie_pme_interrupt_enable(srv->port, true);
-	data->noirq = false;
-	spin_unlock_irq(&data->lock);
-
-	return 0;
-}
-
 static struct pcie_port_service_driver pcie_pme_driver = {
 	.name		= "pcie_pme",
 	.port_type	= PCI_EXP_TYPE_ROOT_PORT,
@@ -464,8 +439,6 @@ static struct pcie_port_service_driver pcie_pme_driver = {
 
 	.probe		= pcie_pme_probe,
 	.suspend	= pcie_pme_suspend,
-	.runtime_suspend = pcie_pme_runtime_suspend,
-	.runtime_resume	= pcie_pme_runtime_resume,
 	.resume		= pcie_pme_resume,
 	.remove		= pcie_pme_remove,
 };
-- 
2.20.1

[PATCH v2 2/2] PCI: pciehp: Disable Data Link Layer State Changed event on suspend

[Mika Westerberg]

Commit 0e157e528604 ("PCI/PME: Implement runtime PM callbacks") tried to
solve an issue where the hierarchy immediately wakes up when it is
transitioned into D3cold. However, it turns out to prevent PME
propagation on some systems that do not support D3cold.

I looked more closely what might cause the immediate wakeup. It happens
when the ACPI power resource of the root port is turned off. The AML
code associated with the _OFF() method of the ACPI power resource
executes PCIe L2/3 ready transition and waits for it to complete. Right
after the L2/3 ready transition is started the root port receives PME
from the downstream port.

The simplest hierarchy where this happens looks like this:

  00:1d.0 PCIe Root port
    ^
    |
    v
    05:00.0 PCIe switch #1 upstream port
      06:01.0 PCIe switch #1 downstream hotplug port
        ^
        |
        v
        08:00.0 Pcie switch #2 upstream port

It seems that the PCIe link between the two switches, before
PME_Turn_Off/PME_TO_Ack is complete for the whole hierarchy, goes
inactive and triggers PME towards the root port bringing it back to D0.
The L2/3 ready sequence is described in PCIe r4.0 spec sections 5.2 and
5.3.3 but unfortunately they do not state what happens if DLLSCE is
enabled during the sequence.

Disabling Data Link Layer State Changed event (DLLSCE) seems to prevent
the issue and still allows the downstream hotplug port to notice when a
device is plugged/unplugged.

Link: https://bugzilla.kernel.org/show_bug.cgi?id=202103
Fixes: 0e157e528604 ("PCI/PME: Implement runtime PM callbacks")
Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
Reviewed-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/pci/hotplug/pciehp_hpc.c | 17 +++++++++++++++--
 1 file changed, 15 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c
index cd9eae650aa5..8bfcb8cd0900 100644
--- a/drivers/pci/hotplug/pciehp_hpc.c
+++ b/drivers/pci/hotplug/pciehp_hpc.c
@@ -736,12 +736,25 @@ void pcie_clear_hotplug_events(struct controller *ctrl)
 
 void pcie_enable_interrupt(struct controller *ctrl)
 {
-	pcie_write_cmd(ctrl, PCI_EXP_SLTCTL_HPIE, PCI_EXP_SLTCTL_HPIE);
+	u16 mask;
+
+	mask = PCI_EXP_SLTCTL_HPIE | PCI_EXP_SLTCTL_DLLSCE;
+	pcie_write_cmd(ctrl, mask, mask);
 }
 
 void pcie_disable_interrupt(struct controller *ctrl)
 {
-	pcie_write_cmd(ctrl, 0, PCI_EXP_SLTCTL_HPIE);
+	u16 mask;
+
+	/*
+	 * Mask hot-plug interrupt to prevent it triggering immediately
+	 * when the link goes inactive (we still get PME when any of the
+	 * enabled events is detected). Same goes with Link Layer State
+	 * changed event which generates PME immediately when the link goes
+	 * inactive so mask it as well.
+	 */
+	mask = PCI_EXP_SLTCTL_HPIE | PCI_EXP_SLTCTL_DLLSCE;
+	pcie_write_cmd(ctrl, 0, mask);
 }
 
 /*
-- 
2.20.1

Re: [PATCH v2 2/2] PCI: pciehp: Disable Data Link Layer State Changed event on suspend

[Bjorn Helgaas]

On Thu, Jan 31, 2019 at 08:07:46PM +0300, Mika Westerberg wrote:
> Commit 0e157e528604 ("PCI/PME: Implement runtime PM callbacks") tried to
> solve an issue where the hierarchy immediately wakes up when it is
> transitioned into D3cold. However, it turns out to prevent PME
> propagation on some systems that do not support D3cold.
> 
> I looked more closely what might cause the immediate wakeup. It happens
> when the ACPI power resource of the root port is turned off. The AML
> code associated with the _OFF() method of the ACPI power resource
> executes PCIe L2/3 ready transition and waits for it to complete. Right
> after the L2/3 ready transition is started the root port receives PME
> from the downstream port.
> 
> The simplest hierarchy where this happens looks like this:
> 
>   00:1d.0 PCIe Root port
>     ^
>     |
>     v
>     05:00.0 PCIe switch #1 upstream port
>       06:01.0 PCIe switch #1 downstream hotplug port
>         ^
>         |
>         v
>         08:00.0 Pcie switch #2 upstream port
> 
> It seems that the PCIe link between the two switches, before
> PME_Turn_Off/PME_TO_Ack is complete for the whole hierarchy, goes
> inactive and triggers PME towards the root port bringing it back to D0.
> The L2/3 ready sequence is described in PCIe r4.0 spec sections 5.2 and
> 5.3.3 but unfortunately they do not state what happens if DLLSCE is
> enabled during the sequence.

The hotplug (and, I guess, the DLLSCE interrupt) and PME interrupts
are the same INTx wire or MSI vector.  But I guess we know the
interrupt here must really be a PME because pcie_pme_irq() reads
PCI_EXP_RTSTA_PME and does nothing unless it is set.

> Disabling Data Link Layer State Changed event (DLLSCE) seems to prevent
> the issue and still allows the downstream hotplug port to notice when a
> device is plugged/unplugged.

Interesting that Hot-Plug Interrupt Enable by itself doesn't seem to
gate the DLLSCE notification.  Sec 6.7.3.4 says Hot-Plug Interrupt
Enable is a master enable/disable bit for "all hot-plug events".  It's
not completely explicit about what "all hot-plug events" includes, but
DLLSCE is definitely included in the list in sec 6.7.3.

I don't think the bugzilla from Heiner below is actually related to
*this* patch.  Heiner's system doesn't have the topology above.

If you have a report for a system where the immediate wakeup happens,
i.e., something with the topology above, I'd be interested in
including that report here.  Ideally it would have complete lspci
info.  I'm just wondering if this is actually a defect in the switch.

> Link: https://bugzilla.kernel.org/show_bug.cgi?id=202103
> Fixes: 0e157e528604 ("PCI/PME: Implement runtime PM callbacks")
> Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> Reviewed-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>  drivers/pci/hotplug/pciehp_hpc.c | 17 +++++++++++++++--
>  1 file changed, 15 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c
> index cd9eae650aa5..8bfcb8cd0900 100644
> --- a/drivers/pci/hotplug/pciehp_hpc.c
> +++ b/drivers/pci/hotplug/pciehp_hpc.c
> @@ -736,12 +736,25 @@ void pcie_clear_hotplug_events(struct controller *ctrl)
>  
>  void pcie_enable_interrupt(struct controller *ctrl)
>  {
> -	pcie_write_cmd(ctrl, PCI_EXP_SLTCTL_HPIE, PCI_EXP_SLTCTL_HPIE);
> +	u16 mask;
> +
> +	mask = PCI_EXP_SLTCTL_HPIE | PCI_EXP_SLTCTL_DLLSCE;
> +	pcie_write_cmd(ctrl, mask, mask);
>  }
>  
>  void pcie_disable_interrupt(struct controller *ctrl)
>  {
> -	pcie_write_cmd(ctrl, 0, PCI_EXP_SLTCTL_HPIE);
> +	u16 mask;
> +
> +	/*
> +	 * Mask hot-plug interrupt to prevent it triggering immediately
> +	 * when the link goes inactive (we still get PME when any of the
> +	 * enabled events is detected). Same goes with Link Layer State
> +	 * changed event which generates PME immediately when the link goes
> +	 * inactive so mask it as well.
> +	 */
> +	mask = PCI_EXP_SLTCTL_HPIE | PCI_EXP_SLTCTL_DLLSCE;
> +	pcie_write_cmd(ctrl, 0, mask);
>  }
>  
>  /*
> -- 
> 2.20.1
> 

Re: [PATCH v2 2/2] PCI: pciehp: Disable Data Link Layer State Changed event on suspend

[Mika Westerberg]

On Thu, Feb 14, 2019 at 03:23:38PM -0600, Bjorn Helgaas wrote:
> On Thu, Jan 31, 2019 at 08:07:46PM +0300, Mika Westerberg wrote:
> > Commit 0e157e528604 ("PCI/PME: Implement runtime PM callbacks") tried to
> > solve an issue where the hierarchy immediately wakes up when it is
> > transitioned into D3cold. However, it turns out to prevent PME
> > propagation on some systems that do not support D3cold.
> > 
> > I looked more closely what might cause the immediate wakeup. It happens
> > when the ACPI power resource of the root port is turned off. The AML
> > code associated with the _OFF() method of the ACPI power resource
> > executes PCIe L2/3 ready transition and waits for it to complete. Right
> > after the L2/3 ready transition is started the root port receives PME
> > from the downstream port.
> > 
> > The simplest hierarchy where this happens looks like this:
> > 
> >   00:1d.0 PCIe Root port
> >     ^
> >     |
> >     v
> >     05:00.0 PCIe switch #1 upstream port
> >       06:01.0 PCIe switch #1 downstream hotplug port
> >         ^
> >         |
> >         v
> >         08:00.0 Pcie switch #2 upstream port
> > 
> > It seems that the PCIe link between the two switches, before
> > PME_Turn_Off/PME_TO_Ack is complete for the whole hierarchy, goes
> > inactive and triggers PME towards the root port bringing it back to D0.
> > The L2/3 ready sequence is described in PCIe r4.0 spec sections 5.2 and
> > 5.3.3 but unfortunately they do not state what happens if DLLSCE is
> > enabled during the sequence.
> 
> The hotplug (and, I guess, the DLLSCE interrupt) and PME interrupts
> are the same INTx wire or MSI vector.  But I guess we know the
> interrupt here must really be a PME because pcie_pme_irq() reads
> PCI_EXP_RTSTA_PME and does nothing unless it is set.
> 
> > Disabling Data Link Layer State Changed event (DLLSCE) seems to prevent
> > the issue and still allows the downstream hotplug port to notice when a
> > device is plugged/unplugged.
> 
> Interesting that Hot-Plug Interrupt Enable by itself doesn't seem to
> gate the DLLSCE notification.  Sec 6.7.3.4 says Hot-Plug Interrupt
> Enable is a master enable/disable bit for "all hot-plug events".  It's
> not completely explicit about what "all hot-plug events" includes, but
> DLLSCE is definitely included in the list in sec 6.7.3.

Indeed the spec is quite vague at places and leaves too much for the
imagination of the reader.

Sec 6.7.3.4 says:

  Software enables a hot-plug event to generate a wakeup event by
  enabling software notification of the event as described in Section
  6.7.3.1. Note that in order for software to disable interrupt
  generation while keeping wakeup generation enabled, the Hot-Plug
  Interrupt Enable bit must be cleared. For form factors that support
  wake generation, a wakeup event must be generated if all
  three of the following conditions occur:
  * The status register for an enabled event transitions from Clearnot
    set to Set
  * The Port is in device state D1, D2, or D3 Hot , and
  * The PME_En bit in the Port’s Power Management Control/Status register is Set

Which to me means that PMEs are generated based on which events are
enabled regardless whether Hot-Plug Interrupt is enabled or not.

> I don't think the bugzilla from Heiner below is actually related to
> *this* patch.  Heiner's system doesn't have the topology above.
> 
> If you have a report for a system where the immediate wakeup happens,
> i.e., something with the topology above, I'd be interested in
> including that report here.  Ideally it would have complete lspci
> info.  I'm just wondering if this is actually a defect in the switch.

I filed a new bugzilla about this here:

  https://bugzilla.kernel.org/show_bug.cgi?id=202593

Let me know if I should re-send with the link corrected.

Re: [PATCH v2 0/2] PCI: Fix runtime PME generation from D3hot

[Bjorn Helgaas]

On Thu, Jan 31, 2019 at 08:07:44PM +0300, Mika Westerberg wrote:
> Hi all,
> 
> Heiner reported [1] that runtime PME generation of his network card does
> not work after commit 0e157e528604 ("PCI/PME: Implement runtime PM
> callbacks") that landed in v4.20. Reverting the commit helps but it has
> another drawback, which I originally tried to solve with the commit, that
> the PCIe hierarchy wakes up immediately after being put into D3cold.
> 
> This series of two patches tries to fix both issues so that PME wakes up
> from D3hot and that the hierarchy does not wake up immediately from D3cold.
> 
> The previous version of the series can be found here:
> 
>   https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1892241.html
> 
> Changes from the previous version:
> 
>   * Add tags from Heiner and Rafael
>   * Update changelog to mention relevent PCIe spec sections
>   * Add comment to pcie_disable_interrupt() explaining why and what is
>     masked
> 
> [1] https://www.spinics.net/lists/linux-pci/msg79051.html
> 
> Mika Westerberg (2):
>   Revert "PCI/PME: Implement runtime PM callbacks"
>   PCI: pciehp: Disable Data Link Layer State Changed event on suspend

I tentatively applied these to pci/pm for v5.1.

I suspect these should be marked for stable (v4.20+)?

I don't think the bugzilla in the second patch is directly relevant to
that patch.  I'd like to update the patch with a more relevant
bugzilla if you have or can open one?

>  drivers/pci/hotplug/pciehp_hpc.c | 17 +++++++++++++++--
>  drivers/pci/pcie/pme.c           | 27 ---------------------------
>  2 files changed, 15 insertions(+), 29 deletions(-)
> 
> -- 
> 2.20.1
> 

Re: [PATCH v2 0/2] PCI: Fix runtime PME generation from D3hot

[Mika Westerberg]

On Thu, Feb 14, 2019 at 03:26:19PM -0600, Bjorn Helgaas wrote:
> On Thu, Jan 31, 2019 at 08:07:44PM +0300, Mika Westerberg wrote:
> > Hi all,
> > 
> > Heiner reported [1] that runtime PME generation of his network card does
> > not work after commit 0e157e528604 ("PCI/PME: Implement runtime PM
> > callbacks") that landed in v4.20. Reverting the commit helps but it has
> > another drawback, which I originally tried to solve with the commit, that
> > the PCIe hierarchy wakes up immediately after being put into D3cold.
> > 
> > This series of two patches tries to fix both issues so that PME wakes up
> > from D3hot and that the hierarchy does not wake up immediately from D3cold.
> > 
> > The previous version of the series can be found here:
> > 
> >   https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1892241.html
> > 
> > Changes from the previous version:
> > 
> >   * Add tags from Heiner and Rafael
> >   * Update changelog to mention relevent PCIe spec sections
> >   * Add comment to pcie_disable_interrupt() explaining why and what is
> >     masked
> > 
> > [1] https://www.spinics.net/lists/linux-pci/msg79051.html
> > 
> > Mika Westerberg (2):
> >   Revert "PCI/PME: Implement runtime PM callbacks"
> >   PCI: pciehp: Disable Data Link Layer State Changed event on suspend
> 
> I tentatively applied these to pci/pm for v5.1.

Thanks!

> I suspect these should be marked for stable (v4.20+)?

Yes, I think it makes sense.

> I don't think the bugzilla in the second patch is directly relevant to
> that patch.  I'd like to update the patch with a more relevant
> bugzilla if you have or can open one?

I created a new one and included the link in my reply to the other
patch.

Re: [PATCH v2 0/2] PCI: Fix runtime PME generation from D3hot

[Bjorn Helgaas]

On Fri, Feb 15, 2019 at 11:38:20AM +0200, Mika Westerberg wrote:
> On Thu, Feb 14, 2019 at 03:26:19PM -0600, Bjorn Helgaas wrote:
> > On Thu, Jan 31, 2019 at 08:07:44PM +0300, Mika Westerberg wrote:
> > > Hi all,
> > > 
> > > Heiner reported [1] that runtime PME generation of his network card does
> > > not work after commit 0e157e528604 ("PCI/PME: Implement runtime PM
> > > callbacks") that landed in v4.20. Reverting the commit helps but it has
> > > another drawback, which I originally tried to solve with the commit, that
> > > the PCIe hierarchy wakes up immediately after being put into D3cold.
> > > 
> > > This series of two patches tries to fix both issues so that PME wakes up
> > > from D3hot and that the hierarchy does not wake up immediately from D3cold.
> > > 
> > > The previous version of the series can be found here:
> > > 
> > >   https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1892241.html
> > > 
> > > Changes from the previous version:
> > > 
> > >   * Add tags from Heiner and Rafael
> > >   * Update changelog to mention relevent PCIe spec sections
> > >   * Add comment to pcie_disable_interrupt() explaining why and what is
> > >     masked
> > > 
> > > [1] https://www.spinics.net/lists/linux-pci/msg79051.html
> > > 
> > > Mika Westerberg (2):
> > >   Revert "PCI/PME: Implement runtime PM callbacks"
> > >   PCI: pciehp: Disable Data Link Layer State Changed event on suspend
> > 
> > I tentatively applied these to pci/pm for v5.1.
> 
> Thanks!
> 
> > I suspect these should be marked for stable (v4.20+)?
> 
> Yes, I think it makes sense.
> 
> > I don't think the bugzilla in the second patch is directly relevant to
> > that patch.  I'd like to update the patch with a more relevant
> > bugzilla if you have or can open one?
> 
> I created a new one and included the link in my reply to the other
> patch.

I marked both for stable and updated the bugzilla link.  Thanks!

Bjorn