[v7,03/12] PCI: Request control of native PCIe hotplug only if supported

Message ID 20180517092903.43701-4-mika.westerberg@linux.intel.com
State Accepted
Delegated to: Bjorn Helgaas
Headers show
Series
  • PCI: Fixes and cleanups for native PCIe, SHPC and ACPI hotplug
Related show

Commit Message

Mika Westerberg May 17, 2018, 9:28 a.m.
Currently we request control of native PCIe hotplug unconditionally.
That may cause problems because native PCIe hotplug events are handled
by pciehp driver and if it is not enabled those events will be lost.
Make this bit more robust and request control of native PCIe hotplug
only if we are actually prepared to do so (pciehp driver is enabled).

While there rename host->native_hotplug to host->native_pcie_hotplug
because we do the same for SHPC hotplug in subsequent patches.

Suggested-by: Bjorn Helgaas <bhelgaas@google.com>
Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
Reviewed-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/acpi/pci_root.c         | 6 ++++--
 drivers/pci/pcie/portdrv_core.c | 2 +-
 drivers/pci/probe.c             | 2 +-
 include/linux/pci.h             | 2 +-
 4 files changed, 7 insertions(+), 5 deletions(-)

Comments

Bjorn Helgaas May 17, 2018, 2:13 p.m. | #1
On Thu, May 17, 2018 at 12:28:54PM +0300, Mika Westerberg wrote:
> Currently we request control of native PCIe hotplug unconditionally.
> That may cause problems because native PCIe hotplug events are handled
> by pciehp driver and if it is not enabled those events will be lost.
> Make this bit more robust and request control of native PCIe hotplug
> only if we are actually prepared to do so (pciehp driver is enabled).
> 
> While there rename host->native_hotplug to host->native_pcie_hotplug
> because we do the same for SHPC hotplug in subsequent patches.
> 
> Suggested-by: Bjorn Helgaas <bhelgaas@google.com>
> Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> Reviewed-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>  drivers/acpi/pci_root.c         | 6 ++++--
>  drivers/pci/pcie/portdrv_core.c | 2 +-
>  drivers/pci/probe.c             | 2 +-
>  include/linux/pci.h             | 2 +-
>  4 files changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
> index 0da18bde6a16..02ab96f00952 100644
> --- a/drivers/acpi/pci_root.c
> +++ b/drivers/acpi/pci_root.c
> @@ -29,6 +29,7 @@
>  #include <linux/pci.h>
>  #include <linux/pci-acpi.h>
>  #include <linux/pci-aspm.h>
> +#include <linux/pci_hotplug.h>
>  #include <linux/dmar.h>
>  #include <linux/acpi.h>
>  #include <linux/slab.h>
> @@ -472,9 +473,10 @@ static void negotiate_os_control(struct acpi_pci_root *root, int *no_aspm)
>  	}
>  
>  	control = OSC_PCI_EXPRESS_CAPABILITY_CONTROL
> -		| OSC_PCI_EXPRESS_NATIVE_HP_CONTROL
>  		| OSC_PCI_EXPRESS_PME_CONTROL;
>  
> +	if (IS_ENABLED(CONFIG_HOTPLUG_PCI_PCIE))
> +		control |= OSC_PCI_EXPRESS_NATIVE_HP_CONTROL;
>  	if (pci_aer_available()) {
>  		if (aer_acpi_firmware_first())
>  			dev_info(&device->dev,
> @@ -900,7 +902,7 @@ struct pci_bus *acpi_pci_root_create(struct acpi_pci_root *root,
>  
>  	host_bridge = to_pci_host_bridge(bus->bridge);
>  	if (!(root->osc_control_set & OSC_PCI_EXPRESS_NATIVE_HP_CONTROL))
> -		host_bridge->native_hotplug = 0;
> +		host_bridge->native_pcie_hotplug = 0;
>  	if (!(root->osc_control_set & OSC_PCI_EXPRESS_AER_CONTROL))
>  		host_bridge->native_aer = 0;
>  	if (!(root->osc_control_set & OSC_PCI_EXPRESS_PME_CONTROL))

Rafael, I noticed that the PCI Firmware Spec, r3.2, sec 4.5.2.2, says
we're required to evaluate _OSC when resuming from S4.  I don't see a
path where we do that today.

Am I missing it?  If not, what resume hook do we need to use to do
this?
Rafael J. Wysocki May 18, 2018, 7:48 a.m. | #2
On Thursday, May 17, 2018 4:13:24 PM CEST Bjorn Helgaas wrote:
> On Thu, May 17, 2018 at 12:28:54PM +0300, Mika Westerberg wrote:
> > Currently we request control of native PCIe hotplug unconditionally.
> > That may cause problems because native PCIe hotplug events are handled
> > by pciehp driver and if it is not enabled those events will be lost.
> > Make this bit more robust and request control of native PCIe hotplug
> > only if we are actually prepared to do so (pciehp driver is enabled).
> > 
> > While there rename host->native_hotplug to host->native_pcie_hotplug
> > because we do the same for SHPC hotplug in subsequent patches.
> > 
> > Suggested-by: Bjorn Helgaas <bhelgaas@google.com>
> > Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> > Reviewed-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > ---
> >  drivers/acpi/pci_root.c         | 6 ++++--
> >  drivers/pci/pcie/portdrv_core.c | 2 +-
> >  drivers/pci/probe.c             | 2 +-
> >  include/linux/pci.h             | 2 +-
> >  4 files changed, 7 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
> > index 0da18bde6a16..02ab96f00952 100644
> > --- a/drivers/acpi/pci_root.c
> > +++ b/drivers/acpi/pci_root.c
> > @@ -29,6 +29,7 @@
> >  #include <linux/pci.h>
> >  #include <linux/pci-acpi.h>
> >  #include <linux/pci-aspm.h>
> > +#include <linux/pci_hotplug.h>
> >  #include <linux/dmar.h>
> >  #include <linux/acpi.h>
> >  #include <linux/slab.h>
> > @@ -472,9 +473,10 @@ static void negotiate_os_control(struct acpi_pci_root *root, int *no_aspm)
> >  	}
> >  
> >  	control = OSC_PCI_EXPRESS_CAPABILITY_CONTROL
> > -		| OSC_PCI_EXPRESS_NATIVE_HP_CONTROL
> >  		| OSC_PCI_EXPRESS_PME_CONTROL;
> >  
> > +	if (IS_ENABLED(CONFIG_HOTPLUG_PCI_PCIE))
> > +		control |= OSC_PCI_EXPRESS_NATIVE_HP_CONTROL;
> >  	if (pci_aer_available()) {
> >  		if (aer_acpi_firmware_first())
> >  			dev_info(&device->dev,
> > @@ -900,7 +902,7 @@ struct pci_bus *acpi_pci_root_create(struct acpi_pci_root *root,
> >  
> >  	host_bridge = to_pci_host_bridge(bus->bridge);
> >  	if (!(root->osc_control_set & OSC_PCI_EXPRESS_NATIVE_HP_CONTROL))
> > -		host_bridge->native_hotplug = 0;
> > +		host_bridge->native_pcie_hotplug = 0;
> >  	if (!(root->osc_control_set & OSC_PCI_EXPRESS_AER_CONTROL))
> >  		host_bridge->native_aer = 0;
> >  	if (!(root->osc_control_set & OSC_PCI_EXPRESS_PME_CONTROL))
> 
> Rafael, I noticed that the PCI Firmware Spec, r3.2, sec 4.5.2.2, says
> we're required to evaluate _OSC when resuming from S4.  I don't see a
> path where we do that today.

No, we don't do that today.

> Am I missing it?  If not, what resume hook do we need to use to do
> this?

We could do that in the ->restore_noirq callback of the host bridge
device I suppose.  Basically, before resuming all of the PCI devices
on the bus.

However, question is on what conditions.  Do the previous versions of the
spec have this requirement or is it just a new thing?
Bjorn Helgaas May 18, 2018, 12:57 p.m. | #3
On Fri, May 18, 2018 at 09:48:54AM +0200, Rafael J. Wysocki wrote:
> On Thursday, May 17, 2018 4:13:24 PM CEST Bjorn Helgaas wrote:
> > On Thu, May 17, 2018 at 12:28:54PM +0300, Mika Westerberg wrote:

> > >  	host_bridge = to_pci_host_bridge(bus->bridge);
> > >  	if (!(root->osc_control_set & OSC_PCI_EXPRESS_NATIVE_HP_CONTROL))
> > > -		host_bridge->native_hotplug = 0;
> > > +		host_bridge->native_pcie_hotplug = 0;
> > >  	if (!(root->osc_control_set & OSC_PCI_EXPRESS_AER_CONTROL))
> > >  		host_bridge->native_aer = 0;
> > >  	if (!(root->osc_control_set & OSC_PCI_EXPRESS_PME_CONTROL))
> > 
> > Rafael, I noticed that the PCI Firmware Spec, r3.2, sec 4.5.2.2, says
> > we're required to evaluate _OSC when resuming from S4.  I don't see a
> > path where we do that today.
> 
> No, we don't do that today.
> 
> > Am I missing it?  If not, what resume hook do we need to use to do
> > this?
> 
> We could do that in the ->restore_noirq callback of the host bridge
> device I suppose.  Basically, before resuming all of the PCI devices
> on the bus.
> 
> However, question is on what conditions.  Do the previous versions of the
> spec have this requirement or is it just a new thing?

I think the requirement has been there since the beginning of _OSC.
The first reference I can find is ACPI 3.0, sec 6.2.9, which says we
should evaluate _OSC on wake from S4 and when a Notify(<device>, 8) is
delivered.  AFAICT, this applies to all _OSC methods, not just those
for PCI host bridges.

The OSHP description (PCI Firmware Spec r3.2, sec 4.8) talks about
interrupt reconfiguration that may be needed when the OS takes over
control of SHPC.  I assume this kind of reconfiguration needs to be
done on wake from S4.
Bjorn Helgaas May 18, 2018, 1:07 p.m. | #4
On Fri, May 18, 2018 at 07:57:07AM -0500, Bjorn Helgaas wrote:
> On Fri, May 18, 2018 at 09:48:54AM +0200, Rafael J. Wysocki wrote:
> > On Thursday, May 17, 2018 4:13:24 PM CEST Bjorn Helgaas wrote:
> > > On Thu, May 17, 2018 at 12:28:54PM +0300, Mika Westerberg wrote:
> 
> > > >  	host_bridge = to_pci_host_bridge(bus->bridge);
> > > >  	if (!(root->osc_control_set & OSC_PCI_EXPRESS_NATIVE_HP_CONTROL))
> > > > -		host_bridge->native_hotplug = 0;
> > > > +		host_bridge->native_pcie_hotplug = 0;
> > > >  	if (!(root->osc_control_set & OSC_PCI_EXPRESS_AER_CONTROL))
> > > >  		host_bridge->native_aer = 0;
> > > >  	if (!(root->osc_control_set & OSC_PCI_EXPRESS_PME_CONTROL))
> > > 
> > > Rafael, I noticed that the PCI Firmware Spec, r3.2, sec 4.5.2.2, says
> > > we're required to evaluate _OSC when resuming from S4.  I don't see a
> > > path where we do that today.
> > 
> > No, we don't do that today.
> > 
> > > Am I missing it?  If not, what resume hook do we need to use to do
> > > this?
> > 
> > We could do that in the ->restore_noirq callback of the host bridge
> > device I suppose.  Basically, before resuming all of the PCI devices
> > on the bus.
> > 
> > However, question is on what conditions.  Do the previous versions of the
> > spec have this requirement or is it just a new thing?
> 
> I think the requirement has been there since the beginning of _OSC.
> The first reference I can find is ACPI 3.0, sec 6.2.9, which says we
> should evaluate _OSC on wake from S4 and when a Notify(<device>, 8) is
> delivered.  AFAICT, this applies to all _OSC methods, not just those
> for PCI host bridges.
> 
> The OSHP description (PCI Firmware Spec r3.2, sec 4.8) talks about
> interrupt reconfiguration that may be needed when the OS takes over
> control of SHPC.  I assume this kind of reconfiguration needs to be
> done on wake from S4.

Forgot to mention: I wonder if this omission could be related to mysterious
resume issues like https://bugzilla.kernel.org/show_bug.cgi?id=99751
Rafael J. Wysocki May 18, 2018, 8:28 p.m. | #5
On Friday, May 18, 2018 2:57:07 PM CEST Bjorn Helgaas wrote:
> On Fri, May 18, 2018 at 09:48:54AM +0200, Rafael J. Wysocki wrote:
> > On Thursday, May 17, 2018 4:13:24 PM CEST Bjorn Helgaas wrote:
> > > On Thu, May 17, 2018 at 12:28:54PM +0300, Mika Westerberg wrote:
> 
> > > >  	host_bridge = to_pci_host_bridge(bus->bridge);
> > > >  	if (!(root->osc_control_set & OSC_PCI_EXPRESS_NATIVE_HP_CONTROL))
> > > > -		host_bridge->native_hotplug = 0;
> > > > +		host_bridge->native_pcie_hotplug = 0;
> > > >  	if (!(root->osc_control_set & OSC_PCI_EXPRESS_AER_CONTROL))
> > > >  		host_bridge->native_aer = 0;
> > > >  	if (!(root->osc_control_set & OSC_PCI_EXPRESS_PME_CONTROL))
> > > 
> > > Rafael, I noticed that the PCI Firmware Spec, r3.2, sec 4.5.2.2, says
> > > we're required to evaluate _OSC when resuming from S4.  I don't see a
> > > path where we do that today.
> > 
> > No, we don't do that today.

In fact, I was not exactly right when I said that.

We don't do that in the image kernel, but the restore kernel started
in order to load the image, actually evaluates _OSC (as part of its
initialization, which is the same as on a fresh boot).  So we do
evaluate _OSC on *wakeup* from S4 which is what the specifications
require IMO.

What may be problematic is that we silently assume that the responses from
the firmware received in the process will always be the same last time.

> > > Am I missing it?  If not, what resume hook do we need to use to do
> > > this?
> > 
> > We could do that in the ->restore_noirq callback of the host bridge
> > device I suppose.  Basically, before resuming all of the PCI devices
> > on the bus.
> > 
> > However, question is on what conditions.  Do the previous versions of the
> > spec have this requirement or is it just a new thing?
> 
> I think the requirement has been there since the beginning of _OSC.
> The first reference I can find is ACPI 3.0, sec 6.2.9, which says we
> should evaluate _OSC on wake from S4 and when a Notify(<device>, 8) is
> delivered.  AFAICT, this applies to all _OSC methods, not just those
> for PCI host bridges.
> 
> The OSHP description (PCI Firmware Spec r3.2, sec 4.8) talks about
> interrupt reconfiguration that may be needed when the OS takes over
> control of SHPC.  I assume this kind of reconfiguration needs to be
> done on wake from S4.

The fact that in our case resume from hibernation involves two instances of
the kernel doesn't make things more straightforward in that respect, so to
speak.

We just need to assume that whatever the restore kernel sees will match
what the image kernel saw before hibernation - or we need a special
protocol to pass that information between the two kernels which is
not there ATM (and I would like to see clear evidence that it really is
necessary before attempting to invent one).
Rafael J. Wysocki May 18, 2018, 8:30 p.m. | #6
On Friday, May 18, 2018 3:07:26 PM CEST Bjorn Helgaas wrote:
> On Fri, May 18, 2018 at 07:57:07AM -0500, Bjorn Helgaas wrote:
> > On Fri, May 18, 2018 at 09:48:54AM +0200, Rafael J. Wysocki wrote:
> > > On Thursday, May 17, 2018 4:13:24 PM CEST Bjorn Helgaas wrote:
> > > > On Thu, May 17, 2018 at 12:28:54PM +0300, Mika Westerberg wrote:
> > 
> > > > >  	host_bridge = to_pci_host_bridge(bus->bridge);
> > > > >  	if (!(root->osc_control_set & OSC_PCI_EXPRESS_NATIVE_HP_CONTROL))
> > > > > -		host_bridge->native_hotplug = 0;
> > > > > +		host_bridge->native_pcie_hotplug = 0;
> > > > >  	if (!(root->osc_control_set & OSC_PCI_EXPRESS_AER_CONTROL))
> > > > >  		host_bridge->native_aer = 0;
> > > > >  	if (!(root->osc_control_set & OSC_PCI_EXPRESS_PME_CONTROL))
> > > > 
> > > > Rafael, I noticed that the PCI Firmware Spec, r3.2, sec 4.5.2.2, says
> > > > we're required to evaluate _OSC when resuming from S4.  I don't see a
> > > > path where we do that today.
> > > 
> > > No, we don't do that today.
> > > 
> > > > Am I missing it?  If not, what resume hook do we need to use to do
> > > > this?
> > > 
> > > We could do that in the ->restore_noirq callback of the host bridge
> > > device I suppose.  Basically, before resuming all of the PCI devices
> > > on the bus.
> > > 
> > > However, question is on what conditions.  Do the previous versions of the
> > > spec have this requirement or is it just a new thing?
> > 
> > I think the requirement has been there since the beginning of _OSC.
> > The first reference I can find is ACPI 3.0, sec 6.2.9, which says we
> > should evaluate _OSC on wake from S4 and when a Notify(<device>, 8) is
> > delivered.  AFAICT, this applies to all _OSC methods, not just those
> > for PCI host bridges.
> > 
> > The OSHP description (PCI Firmware Spec r3.2, sec 4.8) talks about
> > interrupt reconfiguration that may be needed when the OS takes over
> > control of SHPC.  I assume this kind of reconfiguration needs to be
> > done on wake from S4.
> 
> Forgot to mention: I wonder if this omission could be related to mysterious
> resume issues like https://bugzilla.kernel.org/show_bug.cgi?id=99751

I don't quite think so.  For this particular machine the _OSC done by the
restore kernel (see my previous reply in this thread) should be sufficient.

Besides, I don't think this one is about hibernation.

Patch

diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
index 0da18bde6a16..02ab96f00952 100644
--- a/drivers/acpi/pci_root.c
+++ b/drivers/acpi/pci_root.c
@@ -29,6 +29,7 @@ 
 #include <linux/pci.h>
 #include <linux/pci-acpi.h>
 #include <linux/pci-aspm.h>
+#include <linux/pci_hotplug.h>
 #include <linux/dmar.h>
 #include <linux/acpi.h>
 #include <linux/slab.h>
@@ -472,9 +473,10 @@  static void negotiate_os_control(struct acpi_pci_root *root, int *no_aspm)
 	}
 
 	control = OSC_PCI_EXPRESS_CAPABILITY_CONTROL
-		| OSC_PCI_EXPRESS_NATIVE_HP_CONTROL
 		| OSC_PCI_EXPRESS_PME_CONTROL;
 
+	if (IS_ENABLED(CONFIG_HOTPLUG_PCI_PCIE))
+		control |= OSC_PCI_EXPRESS_NATIVE_HP_CONTROL;
 	if (pci_aer_available()) {
 		if (aer_acpi_firmware_first())
 			dev_info(&device->dev,
@@ -900,7 +902,7 @@  struct pci_bus *acpi_pci_root_create(struct acpi_pci_root *root,
 
 	host_bridge = to_pci_host_bridge(bus->bridge);
 	if (!(root->osc_control_set & OSC_PCI_EXPRESS_NATIVE_HP_CONTROL))
-		host_bridge->native_hotplug = 0;
+		host_bridge->native_pcie_hotplug = 0;
 	if (!(root->osc_control_set & OSC_PCI_EXPRESS_AER_CONTROL))
 		host_bridge->native_aer = 0;
 	if (!(root->osc_control_set & OSC_PCI_EXPRESS_PME_CONTROL))
diff --git a/drivers/pci/pcie/portdrv_core.c b/drivers/pci/pcie/portdrv_core.c
index c9c0663db282..6cb30aec2452 100644
--- a/drivers/pci/pcie/portdrv_core.c
+++ b/drivers/pci/pcie/portdrv_core.c
@@ -199,7 +199,7 @@  static int get_port_device_capability(struct pci_dev *dev)
 	int services = 0;
 
 	if (dev->is_hotplug_bridge &&
-	    (pcie_ports_native || host->native_hotplug)) {
+	    (pcie_ports_native || host->native_pcie_hotplug)) {
 		services |= PCIE_PORT_SERVICE_HP;
 
 		/*
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 7c34a2ccb514..a6c3b8d30f8f 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -552,7 +552,7 @@  struct pci_host_bridge *pci_alloc_host_bridge(size_t priv)
 	 * OS from interfering.
 	 */
 	bridge->native_aer = 1;
-	bridge->native_hotplug = 1;
+	bridge->native_pcie_hotplug = 1;
 	bridge->native_pme = 1;
 
 	return bridge;
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 73178a2fcee0..359a197d0310 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -471,7 +471,7 @@  struct pci_host_bridge {
 	unsigned int	ignore_reset_delay:1;	/* For entire hierarchy */
 	unsigned int	no_ext_tags:1;		/* No Extended Tags */
 	unsigned int	native_aer:1;		/* OS may use PCIe AER */
-	unsigned int	native_hotplug:1;	/* OS may use PCIe hotplug */
+	unsigned int	native_pcie_hotplug:1;	/* OS may use PCIe hotplug */
 	unsigned int	native_pme:1;		/* OS may use PCIe PME */
 	/* Resource alignment requirements */
 	resource_size_t (*align_resource)(struct pci_dev *dev,