Re: [GIT PULL] PCI fixes for v4.10
From: Yinghai Lu
Date: Sat Feb 11 2017 - 02:14:00 EST
On Fri, Feb 10, 2017 at 6:39 PM, Yinghai Lu <yinghai@xxxxxxxxxx> wrote:
> Ashok,
>
> Can ask your QA guys check only attached patch and commit 68db9bc ?
more clean patches: split that into two small patches.
Thanks
Yinghai
From 68db9bc814362e7f24371c27d12a4f34477d9356 Mon Sep 17 00:00:00 2001
From: Lukas Wunner <lukas@xxxxxxxxx>
Date: Fri, 28 Oct 2016 10:52:06 +0200
Subject: PCI: pciehp: Add runtime PM support for PCIe hotplug ports
Linux 4.8 added support for runtime suspending PCIe ports to D3hot with
commit 006d44e49a25 ("PCI: Add runtime PM support for PCIe ports"), but
excluded hotplug ports. Those are now afforded runtime PM by the present
commit.
Hotplug ports require a few extra considerations:
- The configuration space of the port remains accessible in D3hot, so all
the functions to read or modify the Slot Status and Slot Control
registers need not be modified. Even turning on slot power doesn't seem
to require the port to be in D0, at least the PCIe spec doesn't say so
and I confirmed that by testing with a Thunderbolt controller.
- However D0 is required to access devices on the secondary bus. This
happens in pciehp_check_link_status() and pciehp_configure_device() (both
called from board_added()) and in pciehp_unconfigure_device() (called
from remove_board()), so acquire a runtime PM ref for their invocation.
- The hotplug port stays active as long as it has active children. If all
hotplugged devices below the port runtime suspend, the port is allowed to
runtime suspend as well. Plug and unplug detection continues to work in
D3hot.
- Hotplug interrupts are delivered in-band, so while the hotplug port
itself is allowed to go to D3hot, its parent ports must stay in D0 for
interrupts to come through. Add a corresponding restriction to
pci_dev_check_d3cold().
- Runtime PM may only be allowed if the hotplug port is handled natively by
the OS. On ACPI systems, the port may alternatively be handled by the
firmware and things break if the OS puts the port into D3 behind the
firmware's back: E.g. Thunderbolt hotplug ports on non-Macs are handled
by Intel's firmware in System Management Mode and the firmware is known
to access devices on the port's secondary bus without checking first if
the port is in D0: https://bugzilla.kernel.org/show_bug.cgi?id=53811
Signed-off-by: Lukas Wunner <lukas@xxxxxxxxx>
Signed-off-by: Bjorn Helgaas <bhelgaas@xxxxxxxxxx>
Reviewed-by: Rafael J. Wysocki <rafael.j.wysocki@xxxxxxxxx>
CC: Mika Westerberg <mika.westerberg@xxxxxxxxxxxxxxx>
diff --git a/drivers/pci/hotplug/pciehp_ctrl.c b/drivers/pci/hotplug/pciehp_ctrl.c
index efe69e8..ffd3fe6 100644
--- a/drivers/pci/hotplug/pciehp_ctrl.c
+++ b/drivers/pci/hotplug/pciehp_ctrl.c
@@ -31,6 +31,7 @@
#include <linux/kernel.h>
#include <linux/types.h>
#include <linux/slab.h>
+#include <linux/pm_runtime.h>
#include <linux/pci.h>
#include "../pci.h"
#include "pciehp.h"
@@ -98,6 +99,7 @@ static int board_added(struct slot *p_slot)
pciehp_green_led_blink(p_slot);
/* Check link training status */
+ pm_runtime_get_sync(&ctrl->pcie->port->dev);
retval = pciehp_check_link_status(ctrl);
if (retval) {
ctrl_err(ctrl, "Failed to check link status\n");
@@ -118,12 +120,14 @@ static int board_added(struct slot *p_slot)
if (retval != -EEXIST)
goto err_exit;
}
+ pm_runtime_put(&ctrl->pcie->port->dev);
pciehp_green_led_on(p_slot);
pciehp_set_attention_status(p_slot, 0);
return 0;
err_exit:
+ pm_runtime_put(&ctrl->pcie->port->dev);
set_slot_off(ctrl, p_slot);
return retval;
}
@@ -137,7 +141,9 @@ static int remove_board(struct slot *p_slot)
int retval;
struct controller *ctrl = p_slot->ctrl;
+ pm_runtime_get_sync(&ctrl->pcie->port->dev);
retval = pciehp_unconfigure_device(p_slot);
+ pm_runtime_put(&ctrl->pcie->port->dev);
if (retval)
return retval;
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index d86351a..1eb622c 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -2245,13 +2245,10 @@ bool pci_bridge_d3_possible(struct pci_dev *bridge)
return false;
/*
- * Hotplug interrupts cannot be delivered if the link is down,
- * so parents of a hotplug port must stay awake. In addition,
- * hotplug ports handled by firmware in System Management Mode
+ * Hotplug ports handled by firmware in System Management Mode
* may not be put into D3 by the OS (Thunderbolt on non-Macs).
- * For simplicity, disallow in general for now.
*/
- if (bridge->is_hotplug_bridge)
+ if (bridge->is_hotplug_bridge && !pciehp_is_native(bridge))
return false;
if (pci_bridge_d3_force)
@@ -2283,7 +2280,10 @@ static int pci_dev_check_d3cold(struct pci_dev *dev, void *data)
!pci_pme_capable(dev, PCI_D3cold)) ||
/* If it is a bridge it must be allowed to go to D3. */
- !pci_power_manageable(dev))
+ !pci_power_manageable(dev) ||
+
+ /* Hotplug interrupts cannot be delivered if the link is down. */
+ dev->is_hotplug_bridge)
*d3cold_ok = false;
Subject: [PATCH] PCI, pciehp: clean and reuse set_slot_off
Move out led setting, and reuse it in remove_board.
Signed-off-by: Yinghai Lu <yinghai@xxxxxxxxxx>
---
drivers/pci/hotplug/pciehp_ctrl.c | 19 +++++--------------
1 file changed, 5 insertions(+), 14 deletions(-)
Index: linux-2.6/drivers/pci/hotplug/pciehp_ctrl.c
===================================================================
--- linux-2.6.orig/drivers/pci/hotplug/pciehp_ctrl.c
+++ linux-2.6/drivers/pci/hotplug/pciehp_ctrl.c
@@ -60,7 +60,7 @@ void pciehp_queue_interrupt_event(struct
static void set_slot_off(struct controller *ctrl, struct slot *pslot)
{
- /* turn off slot, turn on Amber LED, turn off Green LED if supported*/
+ /* turn off slot if supported*/
if (POWER_CTRL(ctrl)) {
pciehp_power_off_slot(pslot);
@@ -71,9 +71,6 @@ static void set_slot_off(struct controll
*/
msleep(1000);
}
-
- pciehp_green_led_off(pslot);
- pciehp_set_attention_status(pslot, 1);
}
/**
@@ -129,6 +126,9 @@ static int board_added(struct slot *p_sl
err_exit:
pm_runtime_put(&ctrl->pcie->port->dev);
set_slot_off(ctrl, p_slot);
+ /* turn on Amber LED, turn off Green LED */
+ pciehp_green_led_off(p_slot);
+ pciehp_set_attention_status(p_slot, 1);
return retval;
}
@@ -147,16 +147,7 @@ static int remove_board(struct slot *p_s
if (retval)
return retval;
- if (POWER_CTRL(ctrl)) {
- pciehp_power_off_slot(p_slot);
-
- /*
- * After turning power off, we must wait for at least 1 second
- * before taking any action that relies on power having been
- * removed from the slot/adapter.
- */
- msleep(1000);
- }
+ set_slot_off(ctrl, p_slot);
/* turn off Green LED */
pciehp_green_led_off(p_slot);
Subject:[PATCH v2] PCI, pciechp: power on/off slots after change to D0
Found power on via /sys has problem.
sca05-0a81fd7f:~ # echo 1 > /sys/bus/pci/slots/7/power
[ 300.949937] pci_hotplug: power_write_file: power = 1
[ 300.955502] pciehp 0000:73:00.0:pcie004: pciehp_get_power_status: SLOTCTRL a8 value read 17f1
[ 300.982557] pciehp 0000:73:00.0:pcie004: pending interrupts 0x0010 from Slot Status
[ 300.991171] pciehp 0000:73:00.0:pcie004: pciehp_power_on_slot: SLOTCTRL a8 write cmd 0
[ 301.000033] pciehp 0000:73:00.0:pcie004: pciehp_green_led_blink: SLOTCTRL a8 write cmd 200
[ 301.009274] pciehp 0000:73:00.0:pcie004: pending interrupts 0x0010 from Slot Status
[ 301.662172] pciehp 0000:73:00.0:pcie004: pciehp_check_link_active: lnk_status = f083
[ 301.670827] pciehp 0000:73:00.0:pcie004: pending interrupts 0x0108 from Slot Status
[ 301.679376] pciehp 0000:73:00.0:pcie004: Slot(7): Link Up
[ 301.685463] pciehp 0000:73:00.0:pcie004: Slot(7): Link Up event ignored; already powering on
[ 301.685508] pciehp 0000:73:00.0:pcie004: pciehp_check_link_active: lnk_status = f083
[ 302.005967] pciehp 0000:73:00.0:pcie004: pciehp_check_link_status: lnk_status = f083
[ 302.014859] pci 0000:74:00.0: [15b3:1003] type 00 class 0x0c0600
also find other slot with other card still have extra link up problem on power off.
That mean commit 68db9bc assumpation about power on/off on D3 is not right.
>
>- The configuration space of the port remains accessible in D3hot, so all
> the functions to read or modify the Slot Status and Slot Control
> registers need not be modified. Even turning on slot power doesn't seem
> to require the port to be in D0, at least the PCIe spec doesn't say so
> and I confirmed that by testing with a Thunderbolt controller.
This patch put back D0 when trying to power on/off the slots.
Signed-off-by: Yinghai Lu <yinghai@xxxxxxxxxx>
---
drivers/pci/hotplug/pciehp_ctrl.c | 13 ++++++++-----
1 file changed, 8 insertions(+), 5 deletions(-)
Index: linux-2.6/drivers/pci/hotplug/pciehp_ctrl.c
===================================================================
--- linux-2.6.orig/drivers/pci/hotplug/pciehp_ctrl.c
+++ linux-2.6/drivers/pci/hotplug/pciehp_ctrl.c
@@ -86,17 +86,17 @@ static int board_added(struct slot *p_sl
struct controller *ctrl = p_slot->ctrl;
struct pci_bus *parent = ctrl->pcie->port->subordinate;
+ pm_runtime_get_sync(&ctrl->pcie->port->dev);
if (POWER_CTRL(ctrl)) {
/* Power on slot */
retval = pciehp_power_on_slot(p_slot);
if (retval)
- return retval;
+ goto err_exit;
}
pciehp_green_led_blink(p_slot);
/* Check link training status */
- pm_runtime_get_sync(&ctrl->pcie->port->dev);
retval = pciehp_check_link_status(ctrl);
if (retval) {
ctrl_err(ctrl, "Failed to check link status\n");
@@ -124,8 +124,9 @@ static int board_added(struct slot *p_sl
return 0;
err_exit:
- pm_runtime_put(&ctrl->pcie->port->dev);
set_slot_off(ctrl, p_slot);
+ pm_runtime_put(&ctrl->pcie->port->dev);
+
/* turn on Amber LED, turn off Green LED */
pciehp_green_led_off(p_slot);
pciehp_set_attention_status(p_slot, 1);
@@ -143,11 +144,13 @@ static int remove_board(struct slot *p_s
pm_runtime_get_sync(&ctrl->pcie->port->dev);
retval = pciehp_unconfigure_device(p_slot);
- pm_runtime_put(&ctrl->pcie->port->dev);
- if (retval)
+ if (retval) {
+ pm_runtime_put(&ctrl->pcie->port->dev);
return retval;
+ }
set_slot_off(ctrl, p_slot);
+ pm_runtime_put(&ctrl->pcie->port->dev);
/* turn off Green LED */
pciehp_green_led_off(p_slot);