[PATCH -v4 0/6] Use PCI Serial Number to identify device change

[PATCH -v4 0/6] Use PCI Serial Number to identify device change

[Yijing Wang]

v3->v4: introudce PCI VPD Sn support suggested by Bjorn, and other rework things.
		
v2->v3: add a wrap function pci_dsn_init, fix other typo error and git am error.
		Thanks for Don Dutile and Paul Bolle 's review,comments and test.
v1->v2: Modify pci_get_dsn to pci_device_serial_number,
	    power off slot before remove the old device during resume to avoid
		old .remove() method to touch new hardware.
		Fix other typo and fail check problems.
		split the list_empty() guard into new patch.
	    Thanks for Bjorn's review and comments.

This series applied to Bjorn's pci-next branch.

Yijing Wang (6):
  PCI,pciehp: avoid add a device already exist before suspend during
    resume
  PCI: introduce PCIe Device Serial Number Capability support
  PCI: Introduce Vital Product Data Serial Number capability support
  PCI: add pci_serial_number_changed() for device change identification
  PCI: add inspection of device change in pci_scan_single_device
  PCI,pciehp: identify device change during suspend

 drivers/pci/access.c              |   13 +-----
 drivers/pci/hotplug/pciehp_core.c |   40 +++++++++++++++++--
 drivers/pci/pci.c                 |   77 +++++++++++++++++++++++++++++++++++++
 drivers/pci/pci.h                 |   14 ++++++-
 drivers/pci/probe.c               |    4 ++
 drivers/pci/vpd.c                 |   70 +++++++++++++++++++++++++++++++++
 include/linux/pci.h               |    4 ++
 7 files changed, 207 insertions(+), 15 deletions(-)

[PATCH -v4 1/6] PCI,pciehp: avoid add a device already exist before suspend during resume

[Yijing Wang]

Currently, pciehp_resume() try to hot add device if the slot adapter
status return true. But if there are already some devices exist,
namely list_empty(bus->devices) return false. We should not add the device
again, because the device add action will fail. Also print some uncomfortable
messages like this:
	pciehp 0000:00:1c.1:pcie04: Device 0000:03:00.0 already exists at 0000:03:00, cannot hot-add
	pciehp 0000:00:1c.1:pcie04: Cannot add device at 0000:03:00

Tested-by: Paul Bolle <pebolle@tiscali.nl>
Signed-off-by: Yijing Wang <wangyijing@huawei.com>
Cc: Paul Bolle <pebolle@tiscali.nl>
Cc: "Rafael J. Wysocki" <rjw@sisk.pl>
Cc: Oliver Neukum <oneukum@suse.de>
Cc: Gu Zheng <guz.fnst@cn.fujitsu.com>
Cc: linux-pci@vger.kernel.org
---
 drivers/pci/hotplug/pciehp_core.c |   10 +++++++---
 1 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/pci/hotplug/pciehp_core.c b/drivers/pci/hotplug/pciehp_core.c
index 7d72c5e..7fe9dbd 100644
--- a/drivers/pci/hotplug/pciehp_core.c
+++ b/drivers/pci/hotplug/pciehp_core.c
@@ -300,6 +300,7 @@ static int pciehp_resume (struct pcie_device *dev)
 {
 	struct controller *ctrl;
 	struct slot *slot;
+	struct pci_bus *pbus = dev->port->subordinate;
 	u8 status;
 
 	ctrl = get_service_data(dev);
@@ -311,10 +312,13 @@ static int pciehp_resume (struct pcie_device *dev)
 
 	/* Check if slot is occupied */
 	pciehp_get_adapter_status(slot, &status);
-	if (status)
-		pciehp_enable_slot(slot);
-	else
+	if (status) {
+		if (list_empty(&pbus->devices))
+			pciehp_enable_slot(slot);
+	} else if (!list_empty(&pbus->devices)) {
 		pciehp_disable_slot(slot);
+	}
+
 	return 0;
 }
 #endif /* PM */
-- 
1.7.1

[PATCH -v4 2/6] PCI: introduce PCIe Device Serial Number Capability support

[Yijing Wang]

Introduce PCIe Ext Capability Device Serial Number support,
so we can use the unique device serial number to identify
the physical device. During system suspend, if the PCIe
device was removed and reinserted a new same device, during
system resume there is no good way to identify it, maybe
Device Serial Number is a good choice if device support.

Signed-off-by: Yijing Wang <wangyijing@huawei.com>
Cc: Paul Bolle <pebolle@tiscali.nl>
Cc: "Rafael J. Wysocki" <rjw@sisk.pl>
Cc: Oliver Neukum <oneukum@suse.de>
Cc: Gu Zheng <guz.fnst@cn.fujitsu.com>
Cc: linux-pci@vger.kernel.org
---
 drivers/pci/pci.c   |   29 +++++++++++++++++++++++++++++
 drivers/pci/pci.h   |    2 +-
 drivers/pci/probe.c |    2 ++
 include/linux/pci.h |    1 +
 4 files changed, 33 insertions(+), 1 deletions(-)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index a8d5fd0..4de8468 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -2048,6 +2048,35 @@ void pci_free_cap_save_buffers(struct pci_dev *dev)
 }
 
 /**
+ * pci_device_serial_number - get device serial number
+ * @dev: the PCI device
+ *
+ * return the device serial number if device support,
+ * otherwise return 0.
+ */
+static u64 pci_device_serial_number(struct pci_dev *dev)
+{
+	int pos;
+	u32 lo, hi;
+
+	if (!pci_is_pcie(dev))
+		return 0;
+
+	pos = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_DSN);
+	if (!pos)
+		return 0;
+
+	pci_read_config_dword(dev, pos + 4, &lo);
+	pci_read_config_dword(dev, pos + 8, &hi);
+	return ((u64)hi << 32) | lo;
+}
+
+void pci_dsn_init(struct pci_dev *dev)
+{
+	dev->sn = pci_device_serial_number(dev);
+}
+
+/**
  * pci_configure_ari - enable or disable ARI forwarding
  * @dev: the PCI device
  *
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index d1182c4..5f398a5 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -207,7 +207,7 @@ void __ref __pci_bus_size_bridges(struct pci_bus *bus,
 void __ref __pci_bus_assign_resources(const struct pci_bus *bus,
 				      struct list_head *realloc_head,
 				      struct list_head *fail_head);
-
+void pci_dsn_init(struct pci_dev *dev);
 /**
  * pci_ari_enabled - query ARI forwarding status
  * @bus: the PCI bus
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 46ada5c..edb138c 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -1325,6 +1325,8 @@ static void pci_init_capabilities(struct pci_dev *dev)
 	/* Vital Product Data */
 	pci_vpd_pci22_init(dev);
 
+	pci_dsn_init(dev);
+
 	/* Alternative Routing-ID Forwarding */
 	pci_configure_ari(dev);
 
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 0fd1f15..4354eaf 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -342,6 +342,7 @@ struct pci_dev {
 	struct list_head msi_list;
 	struct kset *msi_kset;
 #endif
+	u64 sn;		/* device serial number, 0 if not support */
 	struct pci_vpd *vpd;
 #ifdef CONFIG_PCI_ATS
 	union {
-- 
1.7.1

[PATCH -v4 3/6] PCI: Introduce Vital Product Data Serial Number capability support

[Yijing Wang]

Vital Product Data Serial Number is another capability to support
device serial number, some devices may implement this cap. So we
introduce VPD SN support here to enhance the device change
identification.

Suggested-by: Bjorn Helgaas <bhelgaas@google.com>
Signed-off-by: Yijing Wang <wangyijing@huawei.com>
Cc: Paul Bolle <pebolle@tiscali.nl>
Cc: "Rafael J. Wysocki" <rjw@sisk.pl>
Cc: Oliver Neukum <oneukum@suse.de>
Cc: Gu Zheng <guz.fnst@cn.fujitsu.com>
Cc: linux-pci@vger.kernel.org
---
 drivers/pci/access.c |    4 ++-
 drivers/pci/pci.h    |    4 +++
 drivers/pci/vpd.c    |   70 ++++++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/pci.h  |    1 +
 4 files changed, 78 insertions(+), 1 deletions(-)

diff --git a/drivers/pci/access.c b/drivers/pci/access.c
index 1cc2366..0069981 100644
--- a/drivers/pci/access.c
+++ b/drivers/pci/access.c
@@ -194,7 +194,6 @@ PCI_USER_WRITE_CONFIG(dword, u32)
 
 /* VPD access through PCI 2.2+ VPD capability */
 
-#define PCI_VPD_PCI22_SIZE (PCI_VPD_ADDR_MASK + 1)
 
 struct pci_vpd_pci22 {
 	struct pci_vpd base;
@@ -350,6 +349,7 @@ out:
 
 static void pci_vpd_pci22_release(struct pci_dev *dev)
 {
+	kfree(dev->vpd->sn);
 	kfree(container_of(dev->vpd, struct pci_vpd_pci22, base));
 }
 
@@ -377,6 +377,8 @@ int pci_vpd_pci22_init(struct pci_dev *dev)
 	vpd->cap = cap;
 	vpd->busy = false;
 	dev->vpd = &vpd->base;
+
+	pci_vpd_serial_number_init(dev, dev->vpd);
 	return 0;
 }
 
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index 5f398a5..aed8751 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -83,6 +83,8 @@ static inline bool pci_is_bridge(struct pci_dev *pci_dev)
 	return !!(pci_dev->subordinate);
 }
 
+#define PCI_VPD_PCI22_SIZE (PCI_VPD_ADDR_MASK + 1)
+
 struct pci_vpd_ops {
 	ssize_t (*read)(struct pci_dev *dev, loff_t pos, size_t count, void *buf);
 	ssize_t (*write)(struct pci_dev *dev, loff_t pos, size_t count, const void *buf);
@@ -91,6 +93,7 @@ struct pci_vpd_ops {
 
 struct pci_vpd {
 	unsigned int len;
+	char *sn; /* serial number */
 	const struct pci_vpd_ops *ops;
 	struct bin_attribute *attr; /* descriptor for sysfs VPD entry */
 };
@@ -208,6 +211,7 @@ void __ref __pci_bus_assign_resources(const struct pci_bus *bus,
 				      struct list_head *realloc_head,
 				      struct list_head *fail_head);
 void pci_dsn_init(struct pci_dev *dev);
+struct pci_vpd *pci_vpd_serial_number_init(struct pci_dev *dev, struct pci_vpd *vpd);
 /**
  * pci_ari_enabled - query ARI forwarding status
  * @bus: the PCI bus
diff --git a/drivers/pci/vpd.c b/drivers/pci/vpd.c
index 39b7907..e5a4853 100644
--- a/drivers/pci/vpd.c
+++ b/drivers/pci/vpd.c
@@ -7,6 +7,7 @@
 
 #include <linux/pci.h>
 #include <linux/export.h>
+#include "pci.h"
 
 int pci_vpd_find_tag(const u8 *buf, unsigned int off, unsigned int len, u8 rdt)
 {
@@ -60,3 +61,71 @@ int pci_vpd_find_info_keyword(const u8 *buf, unsigned int off,
 	return -ENOENT;
 }
 EXPORT_SYMBOL_GPL(pci_vpd_find_info_keyword);
+
+/**
+ * pci_vpd_serial_number_init - initialize the device VPD SN
+ * @dev: the PCI device
+ * @vpd: the VPD used to initialize SN
+ *
+ * Initialize the device VPD Serial Number, if vpd passed as NULL,
+ * allocate a new VPD and initialize its SN.
+ */
+struct pci_vpd *pci_vpd_serial_number_init(struct pci_dev *dev,
+				struct pci_vpd *vpd)
+{
+	char *buf = NULL;
+	struct pci_vpd *old = dev->vpd;
+	struct pci_vpd *new = NULL;
+	int cnt, i, end, j, len;
+
+	/* To detect whether the device is changed, we should
+	 * allocate a new VPD to initialize serial number,
+	 * because if the device has been changed, the cap
+	 * info is stale.
+	 */
+	if (!vpd) {
+		dev->vpd = NULL;
+		pci_vpd_pci22_init(dev);
+		if (!dev->vpd)
+			goto fail;
+	}
+
+	new = dev->vpd;
+	buf = kzalloc(PCI_VPD_PCI22_SIZE, GFP_KERNEL);
+	if (!buf)
+		goto fail;
+
+	cnt = pci_read_vpd(dev, 0, PCI_VPD_PCI22_SIZE, buf);
+	if (cnt < 0)
+		goto fail;
+
+	i = pci_vpd_find_tag(buf, 0, PCI_VPD_PCI22_SIZE,
+			PCI_VPD_LRDT_RO_DATA);
+	if (i < 0)
+		goto fail;
+
+	end = i + PCI_VPD_LRDT_TAG_SIZE + pci_vpd_lrdt_size(&buf[i]);
+	i += PCI_VPD_LRDT_TAG_SIZE;
+
+	j = pci_vpd_find_info_keyword(buf, i, end,
+			PCI_VPD_RO_KEYWORD_SN);
+	if (j < 0)
+		goto fail;
+
+	len = pci_vpd_info_field_size(&buf[j]);
+	new->sn = kzalloc(len + 1, GFP_KERNEL);
+	if (!new->sn)
+		goto fail;
+
+	j += PCI_VPD_INFO_FLD_HDR_SIZE;
+	memcpy(new->sn, &buf[j], len);
+	kfree(buf);
+	dev->vpd = old;
+	return new;
+fail:
+	kfree(buf);
+	if (new != old)
+		pci_vpd_release(dev);
+	dev->vpd = old;
+	return NULL;
+}
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 4354eaf..5c38b55 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -1781,6 +1781,7 @@ bool pci_acs_path_enabled(struct pci_dev *start,
 
 #define PCI_VPD_RO_KEYWORD_PARTNO	"PN"
 #define PCI_VPD_RO_KEYWORD_MFR_ID	"MN"
+#define PCI_VPD_RO_KEYWORD_SN		"SN"
 #define PCI_VPD_RO_KEYWORD_VENDOR0	"V0"
 #define PCI_VPD_RO_KEYWORD_CHKSUM	"RV"
 
-- 
1.7.1

[PATCH -v4 4/6] PCI: add pci_serial_number_changed() for device change identification

[Yijing Wang]

Sometimes OS do not know the physical device swap,
for instance, some device hotplug during system suspend.
Interrupt can not deliver to OS in some platform.
So we can use pci serial number capability to detect this
issue if device supports serial number.

Signed-off-by: Yijing Wang <wangyijing@huawei.com>
Cc: Paul Bolle <pebolle@tiscali.nl>
Cc: "Rafael J. Wysocki" <rjw@sisk.pl>
Cc: Oliver Neukum <oneukum@suse.de>
Cc: Gu Zheng <guz.fnst@cn.fujitsu.com>
Cc: linux-pci@vger.kernel.org
---
 drivers/pci/access.c |    9 ---------
 drivers/pci/pci.c    |   48 ++++++++++++++++++++++++++++++++++++++++++++++++
 drivers/pci/pci.h    |    8 ++++++++
 include/linux/pci.h  |    2 ++
 4 files changed, 58 insertions(+), 9 deletions(-)

diff --git a/drivers/pci/access.c b/drivers/pci/access.c
index 0069981..7f8df11 100644
--- a/drivers/pci/access.c
+++ b/drivers/pci/access.c
@@ -194,15 +194,6 @@ PCI_USER_WRITE_CONFIG(dword, u32)
 
 /* VPD access through PCI 2.2+ VPD capability */
 
-
-struct pci_vpd_pci22 {
-	struct pci_vpd base;
-	struct mutex lock;
-	u16	flag;
-	bool	busy;
-	u8	cap;
-};
-
 /*
  * Wait for last operation to complete.
  * This code has to spin since there is no other notification from the PCI
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 4de8468..55b1069 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -2077,6 +2077,54 @@ void pci_dsn_init(struct pci_dev *dev)
 }
 
 /**
+ * pci_serial_number_changed - check the device SN is changed
+ * @pdev: the PCI device
+ *
+ * check the device serial number is changed.
+ * if device does not support device serial number,
+ * return false.
+ */
+bool pci_serial_number_changed(struct pci_dev *pdev)
+{
+	struct pci_vpd *vpd;
+	u64 old_dsn, new_dsn;
+	int ret;
+
+	/* first check PCIe DSN */
+	old_dsn = pdev->sn;
+	new_dsn = pci_device_serial_number(pdev);
+
+	if (old_dsn != new_dsn)
+		return true;
+	else if (old_dsn)
+		return false;
+
+	/* PCIe DSN does not support, check VPD SN */
+	vpd = pci_vpd_serial_number_init(pdev, NULL);
+	if (!pdev->vpd && !vpd) {
+		/* VPD SN does not support */
+		return false;
+	} else if (pdev->vpd && pdev->vpd->sn && vpd) {
+		ret = strcmp(pdev->vpd->sn, vpd->sn);
+		kfree(vpd->sn);
+		kfree(container_of(vpd, struct pci_vpd_pci22, base));
+		if (!ret)
+			return false;
+		else
+			return true;
+	} else if ((pdev->vpd && pdev->vpd->sn) || (vpd && vpd->sn)) {
+		if (vpd) {
+			kfree(vpd->sn);
+			kfree(container_of(vpd, struct pci_vpd_pci22, base));
+		}
+		return true;
+	}
+
+	return false;
+}
+EXPORT_SYMBOL(pci_serial_number_changed);
+
+/**
  * pci_configure_ari - enable or disable ARI forwarding
  * @dev: the PCI device
  *
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index aed8751..eb86cf8 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -98,6 +98,14 @@ struct pci_vpd {
 	struct bin_attribute *attr; /* descriptor for sysfs VPD entry */
 };
 
+struct pci_vpd_pci22 {
+	struct pci_vpd base;
+	struct mutex lock;
+	u16	flag;
+	bool	busy;
+	u8	cap;
+};
+
 int pci_vpd_pci22_init(struct pci_dev *dev);
 static inline void pci_vpd_release(struct pci_dev *dev)
 {
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 5c38b55..9580fa5 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -996,6 +996,8 @@ ssize_t pci_read_vpd(struct pci_dev *dev, loff_t pos, size_t count, void *buf);
 ssize_t pci_write_vpd(struct pci_dev *dev, loff_t pos, size_t count, const void *buf);
 int pci_vpd_truncate(struct pci_dev *dev, size_t size);
 
+bool pci_serial_number_changed(struct pci_dev *pdev);
+
 /* Helper functions for low-level code (drivers/pci/setup-[bus,res].c) */
 resource_size_t pcibios_retrieve_fw_addr(struct pci_dev *dev, int idx);
 void pci_bus_assign_resources(const struct pci_bus *bus);
-- 
1.7.1

[PATCH -v4 5/6] PCI: add inspection of device change in pci_scan_single_device

[Yijing Wang]

Add inspection of device change in pci_scan_single_device.
If device change found, print warning. Because the new device
bind the old device driver, so for safety, we do not remove this
device.

Signed-off-by: Yijing Wang <wangyijing@huawei.com>
Cc: Paul Bolle <pebolle@tiscali.nl>
Cc: "Rafael J. Wysocki" <rjw@sisk.pl>
Cc: Oliver Neukum <oneukum@suse.de>
Cc: Gu Zheng <guz.fnst@cn.fujitsu.com>
Cc: linux-pci@vger.kernel.org
---
 drivers/pci/probe.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index edb138c..3ac65aa 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -1389,6 +1389,8 @@ struct pci_dev *__ref pci_scan_single_device(struct pci_bus *bus, int devfn)
 
 	dev = pci_get_slot(bus, devfn);
 	if (dev) {
+		if (pci_serial_number_changed(dev))
+			dev_warn(&dev->dev, "device serial number changed!");
 		pci_dev_put(dev);
 		return dev;
 	}
-- 
1.7.1

[PATCH -v4 6/6] PCI,pciehp: identify device change during suspend

[Yijing Wang]

If device was removed from slot and reinsert a new device during
suspend, pciehp can not identify the physical device change now.
So the old driver .resume() method will be called for the new device,
this is bad. If device support device serial number capability,
we can identify this by DSN. So the reasonable way is first remove
the old device, then enable the new device.

Signed-off-by: Yijing Wang <wangyijing@huawei.com>
Cc: Paul Bolle <pebolle@tiscali.nl>
Cc: "Rafael J. Wysocki" <rjw@sisk.pl>
Cc: Oliver Neukum <oneukum@suse.de>
Cc: Gu Zheng <guz.fnst@cn.fujitsu.com>
Cc: linux-pci@vger.kernel.org
---
 drivers/pci/hotplug/pciehp_core.c |   34 +++++++++++++++++++++++++++++++---
 1 files changed, 31 insertions(+), 3 deletions(-)

diff --git a/drivers/pci/hotplug/pciehp_core.c b/drivers/pci/hotplug/pciehp_core.c
index 7fe9dbd..db5f597 100644
--- a/drivers/pci/hotplug/pciehp_core.c
+++ b/drivers/pci/hotplug/pciehp_core.c
@@ -301,6 +301,8 @@ static int pciehp_resume (struct pcie_device *dev)
 	struct controller *ctrl;
 	struct slot *slot;
 	struct pci_bus *pbus = dev->port->subordinate;
+	struct pci_dev *pdev;
+	int ret = 0;
 	u8 status;
 
 	ctrl = get_service_data(dev);
@@ -315,10 +317,36 @@ static int pciehp_resume (struct pcie_device *dev)
 	if (status) {
 		if (list_empty(&pbus->devices))
 			pciehp_enable_slot(slot);
-	} else if (!list_empty(&pbus->devices)) {
-		pciehp_disable_slot(slot);
+
+		pdev = pci_get_slot(pbus, PCI_DEVFN(0, 0));
+		if (!pdev)
+			return 0;
+		ret = pci_serial_number_changed(pdev);
+		pci_dev_put(pdev);
+		if (ret) {
+			/*
+			 * first power off slot, avoid the old driver
+			 * .remove() method touch the new hardware
+			 */
+			if (POWER_CTRL(ctrl)) {
+				ret = pciehp_power_off_slot(slot);
+				if (ret) {
+					ctrl_err(ctrl,
+						"Issue of Slot Disable command failed\n");
+					return 0;
+				}
+				msleep(1000);
+				pciehp_unconfigure_device(slot);
+				/* call child devices driver->suspend() for symmetry */
+				list_for_each_entry(pdev, &pbus->devices, bus_list)
+					if (pdev->driver)
+						pdev->driver->driver.pm->suspend(&pdev->dev);
+				pciehp_enable_slot(slot);
+			}
+		} else if (!list_empty(&pbus->devices)) {
+			pciehp_disable_slot(slot);
+		}
 	}
-	
 	return 0;
 }
 #endif /* PM */
-- 
1.7.1

Re: [PATCH -v4 6/6] PCI,pciehp: identify device change during suspend

[Yijing Wang]

Hi Bjorn,
   I still leave this code in pciehp modeule, because I do not find
a better place to move. Only in pciehp module, we can know the device whether support
hotplug, if we can not power off device, although we find the device change, but we
can not remove the device for safety. What do you think?

Thanks!
Yijing.

On 2013/8/1 21:06, Yijing Wang wrote:
> If device was removed from slot and reinsert a new device during
> suspend, pciehp can not identify the physical device change now.
> So the old driver .resume() method will be called for the new device,
> this is bad. If device support device serial number capability,
> we can identify this by DSN. So the reasonable way is first remove
> the old device, then enable the new device.
> 
> Signed-off-by: Yijing Wang <wangyijing@huawei.com>
> Cc: Paul Bolle <pebolle@tiscali.nl>
> Cc: "Rafael J. Wysocki" <rjw@sisk.pl>
> Cc: Oliver Neukum <oneukum@suse.de>
> Cc: Gu Zheng <guz.fnst@cn.fujitsu.com>
> Cc: linux-pci@vger.kernel.org
> ---
>  drivers/pci/hotplug/pciehp_core.c |   34 +++++++++++++++++++++++++++++++---
>  1 files changed, 31 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/pci/hotplug/pciehp_core.c b/drivers/pci/hotplug/pciehp_core.c
> index 7fe9dbd..db5f597 100644
> --- a/drivers/pci/hotplug/pciehp_core.c
> +++ b/drivers/pci/hotplug/pciehp_core.c
> @@ -301,6 +301,8 @@ static int pciehp_resume (struct pcie_device *dev)
>  	struct controller *ctrl;
>  	struct slot *slot;
>  	struct pci_bus *pbus = dev->port->subordinate;
> +	struct pci_dev *pdev;
> +	int ret = 0;
>  	u8 status;
>  
>  	ctrl = get_service_data(dev);
> @@ -315,10 +317,36 @@ static int pciehp_resume (struct pcie_device *dev)
>  	if (status) {
>  		if (list_empty(&pbus->devices))
>  			pciehp_enable_slot(slot);
> -	} else if (!list_empty(&pbus->devices)) {
> -		pciehp_disable_slot(slot);
> +
> +		pdev = pci_get_slot(pbus, PCI_DEVFN(0, 0));
> +		if (!pdev)
> +			return 0;
> +		ret = pci_serial_number_changed(pdev);
> +		pci_dev_put(pdev);
> +		if (ret) {
> +			/*
> +			 * first power off slot, avoid the old driver
> +			 * .remove() method touch the new hardware
> +			 */
> +			if (POWER_CTRL(ctrl)) {
> +				ret = pciehp_power_off_slot(slot);
> +				if (ret) {
> +					ctrl_err(ctrl,
> +						"Issue of Slot Disable command failed\n");
> +					return 0;
> +				}
> +				msleep(1000);
> +				pciehp_unconfigure_device(slot);
> +				/* call child devices driver->suspend() for symmetry */
> +				list_for_each_entry(pdev, &pbus->devices, bus_list)
> +					if (pdev->driver)
> +						pdev->driver->driver.pm->suspend(&pdev->dev);
> +				pciehp_enable_slot(slot);
> +			}
> +		} else if (!list_empty(&pbus->devices)) {
> +			pciehp_disable_slot(slot);
> +		}
>  	}
> -	
>  	return 0;
>  }
>  #endif /* PM */
> 


-- 
Thanks!
Yijing

Re: [PATCH -v4 6/6] PCI,pciehp: identify device change during suspend

[Paul Bolle]

Yijing,

On Thu, 2013-08-01 at 21:06 +0800, Yijing Wang wrote:
> diff --git a/drivers/pci/hotplug/pciehp_core.c b/drivers/pci/hotplug/pciehp_core.c
> index 7fe9dbd..db5f597 100644
> --- a/drivers/pci/hotplug/pciehp_core.c
> +++ b/drivers/pci/hotplug/pciehp_core.c
> [...]
> @@ -315,10 +317,36 @@ static int pciehp_resume (struct pcie_device *dev)
>  	if (status) {
>  		if (list_empty(&pbus->devices))
>  			pciehp_enable_slot(slot);
> -	} else if (!list_empty(&pbus->devices)) {
> -		pciehp_disable_slot(slot);
> +
> +		pdev = pci_get_slot(pbus, PCI_DEVFN(0, 0));
> +		if (!pdev)
> +			return 0;
> +		ret = pci_serial_number_changed(pdev);
> +		pci_dev_put(pdev);
> +		if (ret) {
> +			/*
> +			 * first power off slot, avoid the old driver
> +			 * .remove() method touch the new hardware
> +			 */
> +			if (POWER_CTRL(ctrl)) {
> +				ret = pciehp_power_off_slot(slot);
> +				if (ret) {
> +					ctrl_err(ctrl,
> +						"Issue of Slot Disable command failed\n");
> +					return 0;
> +				}
> +				msleep(1000);
> +				pciehp_unconfigure_device(slot);
> +				/* call child devices driver->suspend() for symmetry */
> +				list_for_each_entry(pdev, &pbus->devices, bus_list)
> +					if (pdev->driver)
> +						pdev->driver->driver.pm->suspend(&pdev->dev);
> +				pciehp_enable_slot(slot);
> +			}
> +		} else if (!list_empty(&pbus->devices)) {
> +			pciehp_disable_slot(slot);
> +		}
>  	}
> -	

I ran into a problem with this change. This line, removing a line
consisting of only a single tab, appears to conflict with a patch
earlier in this series. I had to manually remove just the change that
removed that line to get this patch to apply. Any idea what's going on?

>  	return 0;
>  }
>  #endif /* PM */


Paul Bolle

Re: [PATCH -v4 1/6] PCI,pciehp: avoid add a device already exist before suspend during resume

[Paul Bolle]

Yijing,

On Thu, 2013-08-01 at 21:06 +0800, Yijing Wang wrote:
> Currently, pciehp_resume() try to hot add device if the slot adapter
> status return true. But if there are already some devices exist,
> namely list_empty(bus->devices) return false. We should not add the device
> again, because the device add action will fail. Also print some uncomfortable
> messages like this:
> 	pciehp 0000:00:1c.1:pcie04: Device 0000:03:00.0 already exists at 0000:03:00, cannot hot-add
> 	pciehp 0000:00:1c.1:pcie04: Cannot add device at 0000:03:00
> 
> Tested-by: Paul Bolle <pebolle@tiscali.nl>
> Signed-off-by: Yijing Wang <wangyijing@huawei.com>
> Cc: Paul Bolle <pebolle@tiscali.nl>
> Cc: "Rafael J. Wysocki" <rjw@sisk.pl>
> Cc: Oliver Neukum <oneukum@suse.de>
> Cc: Gu Zheng <guz.fnst@cn.fujitsu.com>
> Cc: linux-pci@vger.kernel.org

0) There's no 0/6 in my mailbox, so I reply to this message.

1) This series applies cleanly on top of v3.10.5-rc1 (except for some
odd problem in 6/6, which I mentioned in my reply to 6/6).

2) However, this series doesn't appear to work on v3.10.5-rc1. After the
first resume the wireless connection appears to be gone (in Gnome 3).
iwl4965 is still loaded, but I can't reconnect. That should happen
automagically.

3) And the second resume apparently hangs the system. Not sure how it
hangs exactly, but I have to manually shutdown the laptop by pressing
the power button for a few seconds.

4) I guess I'll have to respin v3.10.5-rc1 without this series to
determine if v3.10.5-rc1 or this series causes these issues. But feel
free to prod me for further tests.


Paul Bolle

Re: [PATCH -v4 1/6] PCI,pciehp: avoid add a device already exist before suspend during resume

[Paul Bolle]

On Fri, 2013-08-02 at 20:52 +0200, Paul Bolle wrote:
> 2) However, this series doesn't appear to work on v3.10.5-rc1. After the
> first resume the wireless connection appears to be gone (in Gnome 3).
> iwl4965 is still loaded, but I can't reconnect. That should happen
> automagically.
> 
> 3) And the second resume apparently hangs the system. Not sure how it
> hangs exactly, but I have to manually shutdown the laptop by pressing
> the power button for a few seconds.
> 
> 4) I guess I'll have to respin v3.10.5-rc1 without this series to
> determine if v3.10.5-rc1 or this series causes these issues. But feel
> free to prod me for further tests.

Well, v3.10.5-rc1 without this series resumes fine. (But, of course, I
now again see the pciehp errors at resume.) Is this series actually
expected to work on v3.10.y?


Paul Bolle

Re: [PATCH -v4 6/6] PCI,pciehp: identify device change during suspend

[Yijing Wang]

> I ran into a problem with this change. This line, removing a line
> consisting of only a single tab, appears to conflict with a patch
> earlier in this series. I had to manually remove just the change that
> removed that line to get this patch to apply. Any idea what's going on?
> 
>>  	return 0;
>>  }
>>  #endif /* PM */

Hi Paul,
   Thanks for your review and test very much!
Because this series was based Bjorn's pci-next branch, so maybe this is
the cause, although I don't find any difference between Bjorn's pci-next tree and Linus master tree.

I will refresh this patch.

Thanks!
Yijing.



-- 
Thanks!
Yijing

Re: [PATCH -v4 1/6] PCI,pciehp: avoid add a device already exist before suspend during resume

[Yijing Wang]

> 
> 0) There's no 0/6 in my mailbox, so I reply to this message.
>

Sorry, I forgot adding your cc into patch 0.

> 1) This series applies cleanly on top of v3.10.5-rc1 (except for some
> odd problem in 6/6, which I mentioned in my reply to 6/6).
> 
> 2) However, this series doesn't appear to work on v3.10.5-rc1. After the
> first resume the wireless connection appears to be gone (in Gnome 3).
> iwl4965 is still loaded, but I can't reconnect. That should happen
> automagically.

I found some mistakes in my patch 6/6, I'm very sorry about that.
Will be more careful next time, I will refresh this patch, can your help
to test it again? I will update this patch and resend it soon.

> 
> 3) And the second resume apparently hangs the system. Not sure how it
> hangs exactly, but I have to manually shutdown the laptop by pressing
> the power button for a few seconds.
> 
> 4) I guess I'll have to respin v3.10.5-rc1 without this series to
> determine if v3.10.5-rc1 or this series causes these issues. But feel
> free to prod me for further tests.

Thanks!
Yijing.




-- 
Thanks!
Yijing