PCI: pcie hotplug related patch

75e4615: pciehp: Disable/enable link during slot power off/on 92bdfaf: pciehp: Add Disable/enable link functions
47442c1: pciehp: Add pcie_wait_link_not_active()
40b6c9b: pciehp: print out link status when dlla get active.
dcc66b6: pciehp: Checking pci conf reading to new added device instead of sleep 1s
aadd74c: PCI: Separate pci_bus_read_dev_vendor_id from pci_scan_device
e7457be: PCI: Make sriov work with hotplug remove

 drivers/pci/hotplug/pciehp_hpc.c |  133 +++++++++++++++++++++++++++++++-------
 drivers/pci/pci.h                |    2 +
 drivers/pci/probe.c              |   48 +++++++++-----
 drivers/pci/remove.c             |   10 +++-
 4 files changed, 151 insertions(+), 42 deletions(-)

First one is for hotplug with sriov under bridge. following two are for hotplug probing with pci conf reading. Last four are for pcie link disable when power off slots.

-v2: update first one according to reviewing from linus.
other like pci_dev_read_config return checking is not changed.

Resending whole set according to Jesse.

Thanks

Yinghai Lu

When hot remove pci express module that have pcie switch and support SRIOV, got

[ 5918.610127] pciehp 0000:80:02.2:pcie04: pcie_isr: intr_loc 1
[ 5918.615779] pciehp 0000:80:02.2:pcie04: Attention button interrupt received
[ 5918.622730] pciehp 0000:80:02.2:pcie04: Button pressed on Slot(3)
[ 5918.629002] pciehp 0000:80:02.2:pcie04: pciehp_get_power_status: SLOTCTRL a8 value read 1f9
[ 5918.637416] pciehp 0000:80:02.2:pcie04: PCI slot #3 - powering off due to button press.
[ 5918.647125] pciehp 0000:80:02.2:pcie04: pcie_isr: intr_loc 10
[ 5918.653039] pciehp 0000:80:02.2:pcie04: pciehp_green_led_blink: SLOTCTRL a8 write cmd 200
[ 5918.661229] pciehp 0000:80:02.2:pcie04: pciehp_set_attention_status: SLOTCTRL a8 write cmd c0
[ 5924.667627] pciehp 0000:80:02.2:pcie04: Disabling domain:bus:device=0000:b0:00
[ 5924.674909] pciehp 0000:80:02.2:pcie04: pciehp_get_power_status: SLOTCTRL a8 value read 2f9
[ 5924.683262] pciehp 0000:80:02.2:pcie04: pciehp_unconfigure_device: domain:bus:dev = 0000:b0:00
[ 5924.693976] libfcoe_device_notification: NETDEV_UNREGISTER eth6
[ 5924.764979] libfcoe_device_notification: NETDEV_UNREGISTER eth14
[ 5924.873539] libfcoe_device_notification: NETDEV_UNREGISTER eth15
[ 5924.995209] libfcoe_device_notification: NETDEV_UNREGISTER eth16
[ 5926.114407] sxge 0000:b2:00.0: PCI INT A disabled
[ 5926.119342] BUG: unable to handle kernel NULL pointer dereference at (null)
[ 5926.127189] IP: [<ffffffff81353a3b>] pci_stop_bus_device+0x33/0x83
[ 5926.133377] PGD 0
[ 5926.135402] Oops: 0000 [#1] SMP
[ 5926.138659] CPU 2
[ 5926.140499] Modules linked in:
...
[ 5926.143754]
[ 5926.275823] Call Trace:
[ 5926.278267]  [<ffffffff81353a38>] pci_stop_bus_device+0x30/0x83
[ 5926.284180]  [<ffffffff81353af4>] pci_remove_bus_device+0x1a/0xba
[ 5926.290264]  [<ffffffff81366311>] pciehp_unconfigure_device+0x110/0x17b
[ 5926.296866]  [<ffffffff81365dd9>] ? pciehp_disable_slot+0x188/0x188
[ 5926.303123]  [<ffffffff81365d6f>] pciehp_disable_slot+0x11e/0x188
[ 5926.309206]  [<ffffffff81365e68>] pciehp_power_thread+0x8f/0xe0

...

 +-[0000:80]-+-00.0-[81-8f]--
 |           +-01.0-[90-9f]--
 |           +-02.0-[a0-af]--
 |           +-02.2-[b0-bf]----00.0-[b1-b3]--+-02.0-[b2]--+-00.0 Device
 |           |                               |            +-00.1 Device
 |           |                               |            +-00.2 Device
 |           |                               |            \-00.3 Device
 |           |                               \-03.0-[b3]--+-00.0 Device
 |           |                                            +-00.1 Device
 |           |                                            +-00.2 Device
 |           |                                            \-00.3 Device

root complex: 80:02.2
pci express modules: have pcie switch and are listed as b0:00.0, b1:02.0 and b1:03.0.

                end devices  are b2:00.0 and b3.00.0.
                VFs are: b2:00.1,... b2:00.3, and b3:00.1,...,b3:00.3

Root cause: when doing pci_stop_bus_device() with phys fn, it will stop virt fn and remove the fn, so

	list_for_each_safe(l, n, &bus->devices)

will have problem to refer freed n that is pointed to vf entry.

Solution is just replacing list_for_each_safe() with list_for_each_prev_safe().

	it will make sure we can get valid n pointer to PF insteaf of freed VF.
	because new added device is inserted to bus->devices list tail.

During reviewing the patch, Bjorn said:

|   The PCI hot-remove path calls pci_stop_bus_devices() via
|   pci_remove_bus_device().
|
|   pci_stop_bus_devices() traverses the bus->devices list (point A below),
|   stopping each device in turn, which calls the driver remove() method.  When
|   the device is an SR-IOV PF, the driver calls pci_disable_sriov(), which
|   also uses pci_remove_bus_device() to remove the VF devices from the
|   bus->devices list (point B).
|
|       pci_remove_bus_device
|         pci_stop_bus_device
|           pci_stop_bus_devices(subordinate)
|             list_for_each(bus->devices)             <-- A
|               pci_stop_bus_device(PF)
|                 ...
|                   driver->remove
|                     pci_disable_sriov
|                       ...
|                         pci_remove_bus_device(VF)
|                             <remove from bus_list>  <-- B
|
|   At B, we're changing the same list we're iterating through at A, so when
|   the driver remove() method returns, the pci_stop_bus_devices() iterator has
|   a pointer to a list entry that has already been freed.

Discussion thread can be found : https://lkml.org/lkml/2011/10/15/141

				 https://lkml.org/lkml/2012/1/23/360

-v5: According to Linus to make remove more robust, Change to list_for_each_prev_safe
instead. That is more reasonable, because those devices are added to tail of the list before.

---
 drivers/pci/remove.c |   10 +++++++++-
 1 files changed, 9 insertions(+), 1 deletions(-)

index 7f87bee..e03c234 100644
@@ -124,7 +124,15 @@ static void pci_stop_bus_devices(struct pci_bus *bus)
 {
 	struct list_head *l, *n;
 
-	list_for_each_safe(l, n, &bus->devices) {
+	/*
+	 * VFs could be removed by pci_remove_bus_device() in the
+	 *  pci_stop_bus_devices() code path for PF.
+	 *  aka, bus->devices get updated in the process.
+	 * but VFs are inserted after PFs when SRIOV is enabled for PF,
+	 * We can iterate the list backwards to get prev valid PF instead
+	 *  of removed VF.
+	 */
+	list_for_each_prev_safe(l, n, &bus->devices) {
 		struct pci_dev *dev = pci_dev_b(l);
 		pci_stop_bus_device(dev);
 	}

1.7.7

Ok applied this series, thanks Yinghai.

Jesse Barnes, Intel Open Source Technology Center

Will reuse that function for pciehp probing.

-v2: according to Kenji, fix crs timeout checking, and export the function
for later using when pciehp is compiled as module.

---
 drivers/pci/pci.h   |    2 ++
 drivers/pci/probe.c |   48 +++++++++++++++++++++++++++++++-----------------
 2 files changed, 33 insertions(+), 17 deletions(-)

index b74084e..8dadbd3 100644
@@ -205,6 +205,8 @@ enum pci_bar_type {
 	pci_bar_mem64,		/* A 64-bit memory BAR */
 };
 
+bool pci_bus_read_dev_vendor_id(struct pci_bus *bus, int devfn, u32 *pl,
+				int crs_timeout);
 extern int pci_setup_device(struct pci_dev *dev);
 extern int __pci_read_base(struct pci_dev *dev, enum pci_bar_type type,
 				struct resource *res, unsigned int reg);

1.7.7

During reviewing
| PCI: pciehp: wait 1000 ms before Link Training check

Try to access the pci conf of pci device that is supposed to show up in 1s, if could read back valid vender/device id, could bail out early.

Related discussion could be found:

	https://lkml.org/lkml/2011/12/6/339

-v2: seperate code to pci_bus_read_dev_vendor_id() from pci_scan_device()
and reuse it from pciehp code. Suggested by Matthew Wilcox.
-v3: According to Kenj, don't use array in stack, and don't wait too long
for crs, also return fail status if not found.
Also separate pci_bus_dev_read_vendor_id() change to another patch.

---
 drivers/pci/hotplug/pciehp_hpc.c |   49 ++++++++++++++++++++++++++-----------
 1 files changed, 34 insertions(+), 15 deletions(-)

index 7b14148..ad8c1be 100644
@@ -265,10 +265,37 @@ static void pcie_wait_link_active(struct controller *ctrl)
 	ctrl_dbg(ctrl, "Data Link Layer Link Active not set in 1000 msec\n");
 }
 
+static bool pci_bus_check_dev(struct pci_bus *bus, int devfn)
+{
+	u32 l;
+	int count = 0;
+	int delay = 1000, step = 20;
+	bool found = false;
+
+	do {
+		found = pci_bus_read_dev_vendor_id(bus, devfn, &l, 0);
+		count++;
+
+		if (found)
+			break;
+
+		msleep(step);
+		delay -= step;
+	} while (delay > 0);
+
+	if (count > 1 && pciehp_debug)
+		printk(KERN_DEBUG "pci %04x:%02x:%02x.%d id reading try %d times with interval %d ms to get %08x\n",
+			pci_domain_nr(bus), bus->number, PCI_SLOT(devfn),
+			PCI_FUNC(devfn), count, step, l);
+
+	return found;
+}
+
 int pciehp_check_link_status(struct controller *ctrl)
 {
 	u16 lnk_status;
 	int retval = 0;
+	bool found = false;
 
         /*
          * Data Link Layer Link Active Reporting must be capable for

@@ -280,13 +307,10 @@ int pciehp_check_link_status(struct controller *ctrl)
         else
                 msleep(1000);
 
-	/*
-	 * Need to wait for 1000 ms after Data Link Layer Link Active
-	 * (DLLLA) bit reads 1b before sending configuration request.
-	 * We need it before checking Link Training (LT) bit becuase
-	 * LT is still set even after DLLLA bit is set on some platform.
-	 */
-	msleep(1000);
+	/* wait 100ms before read pci conf, and try in 1s */
+	msleep(100);
+	found = pci_bus_check_dev(ctrl->pcie->port->subordinate,
+					PCI_DEVFN(0, 0));
 
 	retval = pciehp_readw(ctrl, PCI_EXP_LNKSTA, &lnk_status);
 	if (retval) {

@@ -302,16 +326,11 @@ int pciehp_check_link_status(struct controller *ctrl)
 		return retval;
 	}
 
-	/*
-	 * If the port supports Link speeds greater than 5.0 GT/s, we
-	 * must wait for 100 ms after Link training completes before
-	 * sending configuration request.
-	 */
-	if (ctrl->pcie->port->subordinate->max_bus_speed > PCIE_SPEED_5_0GT)
-		msleep(100);
-
 	pcie_update_link_speed(ctrl->pcie->port->subordinate, lnk_status);
 
+	if (!found && !retval)
+		retval = -1;
+
 	return retval;
 }
 

1.7.7

Use it get timestamp to calculate time for the pci conf access readness.

and let it return bool instead of int.

also remove inline attribute, let compiler decide it.

---
 drivers/pci/hotplug/pciehp_hpc.c |   17 ++++++++++++-----
 1 files changed, 12 insertions(+), 5 deletions(-)

index ad8c1be..a1a9879 100644
@@ -241,13 +241,20 @@ static int pcie_write_cmd(struct controller *ctrl, u16 cmd, u16 mask)
 	return retval;
 }
 
-static inline int check_link_active(struct controller *ctrl)
+static bool check_link_active(struct controller *ctrl)
 {
-	u16 link_status;
+	bool ret = false;
+	u16 lnk_status;
 
-	if (pciehp_readw(ctrl, PCI_EXP_LNKSTA, &link_status))
-		return 0;
-	return !!(link_status & PCI_EXP_LNKSTA_DLLLA);
+	if (pciehp_readw(ctrl, PCI_EXP_LNKSTA, &lnk_status))
+		return ret;
+
+	ret = !!(lnk_status & PCI_EXP_LNKSTA_DLLLA);
+
+	if (ret)
+		ctrl_dbg(ctrl, "%s: lnk_status = %x\n", __func__, lnk_status);
+
+	return ret;
 }
 
 static void pcie_wait_link_active(struct controller *ctrl)

1.7.7

Will use for link disable status checking

---
 drivers/pci/hotplug/pciehp_hpc.c |   19 +++++++++++++++----
 1 files changed, 15 insertions(+), 4 deletions(-)

index a1a9879..ea54aef 100644
@@ -257,19 +257,30 @@ static bool check_link_active(struct controller *ctrl)
 	return ret;
 }
 
-static void pcie_wait_link_active(struct controller *ctrl)
+static void __pcie_wait_link_active(struct controller *ctrl, bool active)
 {
 	int timeout = 1000;
 
-	if (check_link_active(ctrl))
+	if (check_link_active(ctrl) == active)
 		return;
 	while (timeout > 0) {
 		msleep(10);
 		timeout -= 10;
-		if (check_link_active(ctrl))
+		if (check_link_active(ctrl) == active)
 			return;
 	}
-	ctrl_dbg(ctrl, "Data Link Layer Link Active not set in 1000 msec\n");
+	ctrl_dbg(ctrl, "Data Link Layer Link Active not %s in 1000 msec\n",
+			active ? "set" : "cleared");
+}
+
+static void pcie_wait_link_active(struct controller *ctrl)
+{
+	__pcie_wait_link_active(ctrl, true);
+}
+
+static void pcie_wait_link_not_active(struct controller *ctrl)
+{
+	__pcie_wait_link_active(ctrl, false);
 }
 
 static bool pci_bus_check_dev(struct pci_bus *bus, int devfn)

1.7.7

Will use it during power off/on slots

---
 drivers/pci/hotplug/pciehp_hpc.c |   36 ++++++++++++++++++++++++++++++++++++
 1 files changed, 36 insertions(+), 0 deletions(-)

index ea54aef..fdab8a6 100644
@@ -352,6 +352,42 @@ int pciehp_check_link_status(struct controller *ctrl)
 	return retval;
 }
 
+static int __pciehp_link_set(struct controller *ctrl, bool enable)
+{
+	u16 lnk_ctrl;
+	int retval = 0;
+
+	retval = pciehp_readw(ctrl, PCI_EXP_LNKCTL, &lnk_ctrl);
+	if (retval) {
+		ctrl_err(ctrl, "Cannot read LNKCTRL register\n");
+		return retval;
+	}
+
+	if (enable)
+		lnk_ctrl &= ~PCI_EXP_LNKCTL_LD;
+	else
+		lnk_ctrl |= PCI_EXP_LNKCTL_LD;
+
+	retval = pciehp_writew(ctrl, PCI_EXP_LNKCTL, lnk_ctrl);
+	if (retval) {
+		ctrl_err(ctrl, "Cannot write LNKCTRL register\n");
+		return retval;
+	}
+	ctrl_dbg(ctrl, "%s: lnk_ctrl = %x\n", __func__, lnk_ctrl);
+
+	return retval;
+}
+
+static int pciehp_link_enable(struct controller *ctrl)
+{
+	return __pciehp_link_set(ctrl, true);
+}
+
+static int pciehp_link_disable(struct controller *ctrl)
+{
+	return __pciehp_link_set(ctrl, false);
+}
+
 int pciehp_get_attention_status(struct slot *slot, u8 *status)
 {
 	struct controller *ctrl = slot->ctrl;

1.7.7

One system have repeater in system board to support gen2 hotplug.

Found when EM is removed from some slots, /var/log/message will be full of "card present/not present" warning.

It turns out root complex still try to train the link to repeater because repeater is not reset.

This patch will disable link to make repeater could reset properly. Also could kill AER during EM removal.

Recently when testing hotplug on one system under development, found if boot the system without EM, and later hotplug does not work with Linux. But other OS is ok.
The root cause is that bios left link disabled when slot is empty, and other OS is playing link disable bit in link ctrl during power on/off.

So We could do the same thing to disable/enable link during power off/on.

-v2: check link DLLA bit instead of 100ms waiting.
Separate link disable/enable functions to another patch.

---
 drivers/pci/hotplug/pciehp_hpc.c |   12 ++++++++++++
 1 files changed, 12 insertions(+), 0 deletions(-)

index fdab8a6..0e35d70 100644
@@ -606,6 +606,10 @@ int pciehp_power_on_slot(struct slot * slot)
 	ctrl_dbg(ctrl, "%s: SLOTCTRL %x write cmd %x\n", __func__,
 		 pci_pcie_cap(ctrl->pcie->port) + PCI_EXP_SLTCTL, slot_cmd);
 
+	retval = pciehp_link_enable(ctrl);
+	if (retval)
+		ctrl_err(ctrl, "%s: Can not enable the link!\n", __func__);
+
 	return retval;
 }
 

@@ -616,6 +620,14 @@ int pciehp_power_off_slot(struct slot * slot)
 	u16 cmd_mask;
 	int retval;
 
+	/* Disable the link at first */
+	pciehp_link_disable(ctrl);
+	/* wait the link is down */
+	if (ctrl->link_active_reporting)
+		pcie_wait_link_not_active(ctrl);
+	else
+		msleep(1000);
+
 	slot_cmd = POWER_OFF;
 	cmd_mask = PCI_EXP_SLTCTL_PCC;
 	retval = pcie_write_cmd(ctrl, slot_cmd, cmd_mask);

1.7.7

Yinghai, Jesse,

I tested pciehp with your set of patches. I have some comments below.

(1) I got a following warning message on compiling the patch [5/7].

drivers/pci/hotplug/pciehp_hpc.c:281: warning: 'pcie_wait_link_not_active' defined but not used

(2) I got following warning messages on compiling the patch [6/7]

drivers/pci/hotplug/pciehp_hpc.c:381: warning: 'pciehp_link_enable' defined but not used drivers/pci/hotplug/pciehp_hpc.c:386: warning: 'pciehp_link_disable' defined but not used

(3) I've asked Naoki Yanagimoto, who reported that configuration read on some hot-added PCIe device returns invalid value, to test the patch. Unfortunately, the problem happens with your patch. But after some discussion and testing, it turned out that problem doesn't happen when the same card with updated bios is used. So it seems the problem is in PCIe card side.

As a result, problems I found are (1) and (2). Please fix those. Other than that, pciehp seems to work well.

Regards,
Kenji Kaneshige

Great. thanks for confirmation.

for (1) and (2), patch 5, and 6 will add some helper functions and they will be used by patch 7.

so when patch 7 is applied, there will be no compiling warning anymore.

Thanks

Yinghai

I know that.
But I think each patch should be compiled without warnings. Patch 5/7 and 6/7 are useless without 7/7. How about merging them?

Regards,
Kenji Kaneshige

just want to keep patch small and easy to be reviewed.

and I split it to three intentionally.

Thanks

Yinghai