[PATCH v1 0/2] PCI: hotplug: Add checks to avoid doing hotplug on PCIe Upstream Ports

[PATCH v1 0/2] PCI: hotplug: Add checks to avoid doing hotplug on PCIe Upstream Ports

[Rafael J. Wysocki]

Hi All,

PCIe Upstream Ports are not hotplug-capable by definition, but it turns out
that in some cases, if the system is configured in a particularly interesting
way, the kernel may be made attempt to operate an Upstream Port as a hotplug
one which causes functional issues to appear.

The following 2 patches amend the code to prevent this behavior from occurring.

Thanks!

[PATCH v1 1/2] PCI/portdrv: Set PCIE_PORT_SERVICE_HP for Root and Downstream Ports only

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

It is reported that on some systems pciehp binds to an Upstream Port and
attempts to operate it which causes devices below the Port to disappear
from the bus.

This happens because acpiphp sets is_hotplug_bridge for that Port (after
receiving a Device Check notification on it from the platform firmware
via ACPI) during the enumeration of PCI devices and so when
get_port_device_capability() runs, it sees that is_hotplug_bridge is
set and adds PCIE_PORT_SERVICE_HP to Port services (which allows pciehp
to bind to the Port in question) without consulting the PCIe type which
should be either Root Port or Downstream Port for the hotplug capability
to be present.

Make get_port_device_capability() more robust by adding a PCIe type
check to it before adding PCIE_PORT_SERVICE_HP to Port services which
helps to avoid the problem.

Reported-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
Suggested-by: Lukas Wunner <lukas@wunner.de>
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/pci/pcie/portdrv_core.c |    2 ++
 1 file changed, 2 insertions(+)

Index: linux-pm/drivers/pci/pcie/portdrv_core.c
===================================================================
--- linux-pm.orig/drivers/pci/pcie/portdrv_core.c
+++ linux-pm/drivers/pci/pcie/portdrv_core.c
@@ -209,6 +209,8 @@ static int get_port_device_capability(st
 	int services = 0;
 
 	if (dev->is_hotplug_bridge &&
+	    (pci_pcie_type(dev) == PCI_EXP_TYPE_ROOT_PORT ||
+	     pci_pcie_type(dev) == PCI_EXP_TYPE_DOWNSTREAM) &&
 	    (pcie_ports_native || host->native_pcie_hotplug)) {
 		services |= PCIE_PORT_SERVICE_HP;
 

[PATCH v1 2/2] ACPI: PCI: hotplug: Avoid setting is_hotplug_bridge for PCIe Upstream Ports

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

It is reported that on some systems pciehp binds to an Upstream Port and
attempts to operate it which causes devices below the Port to disappear
from the bus.

This happens because acpiphp sets is_hotplug_bridge for that Port (after
receiving a Device Check notification on it from the platform firmware
via ACPI) during the enumeration of PCI devices and so when
get_port_device_capability() runs, it sees that is_hotplug_bridge is
set and adds PCIE_PORT_SERVICE_HP to Port services (which allows pciehp
to bind to the Port in question).

Even though this particular problem can be addressed by making the
portdrv_core checks more robust, it also causes power management to
work differently on the affected systems which generally is not
desirable (PCIe Ports with is_hotplug_bridge set have to pass
additional tests to be allowed to go into the D3hot/cold power
states which affects runtime PM of devices below these Ports).

For this reason, amend check_hotplug_bridge() with a PCIe type check
to prevent it from setting is_hotplug_bridge for Upstream Ports.

Reported-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
Suggested-by: Lukas Wunner <lukas@wunner.de>
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/pci/hotplug/acpiphp_glue.c |    8 ++++++++
 1 file changed, 8 insertions(+)

Index: linux-pm/drivers/pci/hotplug/acpiphp_glue.c
===================================================================
--- linux-pm.orig/drivers/pci/hotplug/acpiphp_glue.c
+++ linux-pm/drivers/pci/hotplug/acpiphp_glue.c
@@ -411,6 +411,14 @@ static void check_hotplug_bridge(struct
 	if (dev->is_hotplug_bridge)
 		return;
 
+	/*
+	 * In the PCIe case, only Root Ports and Downstream Ports are capable of
+	 * accommodating hotplug devices, so avoid marking Upstream Ports as
+	 * "hotplug bridges".
+	 */
+	if (pci_is_pcie(dev) && pci_pcie_type(dev) == PCI_EXP_TYPE_UPSTREAM)
+		return;
+
 	list_for_each_entry(func, &slot->funcs, sibling) {
 		if (PCI_FUNC(dev->devfn) == func->function) {
 			dev->is_hotplug_bridge = 1;

Re: [PATCH v1 2/2] ACPI: PCI: hotplug: Avoid setting is_hotplug_bridge for PCIe Upstream Ports

[Rodrigo Vivi]

On Mon, Nov 21, 2022 at 07:16:57PM +0100, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> It is reported that on some systems pciehp binds to an Upstream Port and
> attempts to operate it which causes devices below the Port to disappear
> from the bus.
> 
> This happens because acpiphp sets is_hotplug_bridge for that Port (after
> receiving a Device Check notification on it from the platform firmware
> via ACPI) during the enumeration of PCI devices and so when
> get_port_device_capability() runs, it sees that is_hotplug_bridge is
> set and adds PCIE_PORT_SERVICE_HP to Port services (which allows pciehp
> to bind to the Port in question).
> 
> Even though this particular problem can be addressed by making the
> portdrv_core checks more robust, it also causes power management to
> work differently on the affected systems which generally is not
> desirable (PCIe Ports with is_hotplug_bridge set have to pass
> additional tests to be allowed to go into the D3hot/cold power
> states which affects runtime PM of devices below these Ports).
> 
> For this reason, amend check_hotplug_bridge() with a PCIe type check
> to prevent it from setting is_hotplug_bridge for Upstream Ports.
> 
> Reported-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

for the series:

Tested-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

Based on all the explanations you gave and docs you showed to me
recently this makes total sense and the double protection seems
good to me.

Let's see if Lukas agree, but feel free to also use if needed:

Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

> Suggested-by: Lukas Wunner <lukas@wunner.de>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>  drivers/pci/hotplug/acpiphp_glue.c |    8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> Index: linux-pm/drivers/pci/hotplug/acpiphp_glue.c
> ===================================================================
> --- linux-pm.orig/drivers/pci/hotplug/acpiphp_glue.c
> +++ linux-pm/drivers/pci/hotplug/acpiphp_glue.c
> @@ -411,6 +411,14 @@ static void check_hotplug_bridge(struct
>  	if (dev->is_hotplug_bridge)
>  		return;
>  
> +	/*
> +	 * In the PCIe case, only Root Ports and Downstream Ports are capable of
> +	 * accommodating hotplug devices, so avoid marking Upstream Ports as
> +	 * "hotplug bridges".
> +	 */
> +	if (pci_is_pcie(dev) && pci_pcie_type(dev) == PCI_EXP_TYPE_UPSTREAM)
> +		return;
> +
>  	list_for_each_entry(func, &slot->funcs, sibling) {
>  		if (PCI_FUNC(dev->devfn) == func->function) {
>  			dev->is_hotplug_bridge = 1;
> 
> 
> 

Re: [PATCH v1 0/2] PCI: hotplug: Add checks to avoid doing hotplug on PCIe Upstream Ports

[Lukas Wunner]

On Mon, Nov 21, 2022 at 07:13:15PM +0100, Rafael J. Wysocki wrote:
> PCIe Upstream Ports are not hotplug-capable by definition, but it turns out
> that in some cases, if the system is configured in a particularly interesting
> way, the kernel may be made attempt to operate an Upstream Port as a hotplug
> one which causes functional issues to appear.
> 
> The following 2 patches amend the code to prevent this behavior from occurring.

Both patches LGTM.

The spec reference for this change is PCIe r6.0.1 sec 7.5.3.2:

The Slot Implemented bit in the PCI Express Capabilities register
is only valid for Downstream Ports and undefined on Upstream Ports.

The Slot Capabilities / Control / Status registers are only operable
if the Slot Implemented bit is valid and set.  PCIe hotplug depends
on those registers.

(pcie_capability_reg_implemented() in drivers/pci/access.c returns false
for the Slot Capabilities / Control / Status registers unless the port
is a Root or Downstream Port.  Reads of those registers thus always
return 0.)

(Root Ports are Downstream Ports per the definition of "Downstream"
on page 94 of the PCIe r6.0.1 Base Spec.)

Thanks,

Lukas

Re: [PATCH v1 0/2] PCI: hotplug: Add checks to avoid doing hotplug on PCIe Upstream Ports

[Bjorn Helgaas]

On Mon, Nov 21, 2022 at 07:13:15PM +0100, Rafael J. Wysocki wrote:
> Hi All,
> 
> PCIe Upstream Ports are not hotplug-capable by definition, but it turns out
> that in some cases, if the system is configured in a particularly interesting
> way, the kernel may be made attempt to operate an Upstream Port as a hotplug
> one which causes functional issues to appear.
> 
> The following 2 patches amend the code to prevent this behavior from occurring.

Thanks, applied to pci/hotplug for v6.2.  Lukas, I didn't presume to
convert your LGTM to Reviewed-by, but would be happy add it.

Bjorn

Re: [PATCH v1 0/2] PCI: hotplug: Add checks to avoid doing hotplug on PCIe Upstream Ports

[Lukas Wunner]

On Tue, Nov 22, 2022 at 12:06:03PM -0600, Bjorn Helgaas wrote:
> On Mon, Nov 21, 2022 at 07:13:15PM +0100, Rafael J. Wysocki wrote:
> > PCIe Upstream Ports are not hotplug-capable by definition, but it turns out
> > that in some cases, if the system is configured in a particularly interesting
> > way, the kernel may be made attempt to operate an Upstream Port as a hotplug
> > one which causes functional issues to appear.
> > 
> > The following 2 patches amend the code to prevent this behavior from occurring.
> 
> Thanks, applied to pci/hotplug for v6.2.  Lukas, I didn't presume to
> convert your LGTM to Reviewed-by, but would be happy add it.

I figured that having both a Suggested-by and a Reviewed-by might
look odd, hence went with the more neutral LGTM.  But I see it was
ambiguous.  Either way is fine for me. :)

Thanks,

Lukas