* [PATCH -v2 0/3] Use PCIe DSN to improve pciehp_resume
@ 2013-07-11 9:43 Yijing Wang
2013-07-11 9:43 ` [PATCH -v2 1/3] PCI: introduce PCIe Device Serial Number Capability support Yijing Wang
` (3 more replies)
0 siblings, 4 replies; 23+ messages in thread
From: Yijing Wang @ 2013-07-11 9:43 UTC (permalink / raw)
To: Bjorn Helgaas
Cc: linux-kernel, linux-pci, Rafael, Hanjun Guo, jiang.liu, Yijing Wang
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.
Yijing Wang (3):
PCI: introduce PCIe Device Serial Number Capability support
PCI,pciehp: avoid add a device already exist before suspend during
resume
PCI,pciehp: use PCIe DSN to identify device change during suspend
drivers/pci/hotplug/pciehp_core.c | 54 ++++++++++++++++++++++++++++++++++--
drivers/pci/pci.c | 27 ++++++++++++++++++
drivers/pci/probe.c | 2 +
include/linux/pci.h | 3 ++
4 files changed, 83 insertions(+), 3 deletions(-)
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH -v2 1/3] PCI: introduce PCIe Device Serial Number Capability support
2013-07-11 9:43 [PATCH -v2 0/3] Use PCIe DSN to improve pciehp_resume Yijing Wang
@ 2013-07-11 9:43 ` Yijing Wang
2013-07-11 9:51 ` Don Dutile
2013-07-11 14:22 ` Paul Bolle
2013-07-11 9:43 ` [PATCH -v2 2/3] PCI,pciehp: avoid add a device already exist before suspend during resume Yijing Wang
` (2 subsequent siblings)
3 siblings, 2 replies; 23+ messages in thread
From: Yijing Wang @ 2013-07-11 9:43 UTC (permalink / raw)
To: Bjorn Helgaas
Cc: linux-kernel, linux-pci, Rafael, Hanjun Guo, jiang.liu, 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 inserted a new same device, after
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>
---
drivers/pci/pci.c | 27 +++++++++++++++++++++++++++
drivers/pci/probe.c | 2 ++
include/linux/pci.h | 3 +++
3 files changed, 32 insertions(+), 0 deletions(-)
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index e37fea6..2e855b5 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -2048,6 +2048,33 @@ 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.
+ */
+u64 pci_device_serial_number(struct pci_dev *dev)
+{
+ int pos;
+ u64 sn;
+ 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);
+ sn = ((u64)hi << 32) | lo;
+ return sn;
+}
+EXPORT_SYMBOL(pci_device_serial_number);
+
+/**
* pci_configure_ari - enable or disable ARI forwarding
* @dev: the PCI device
*
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 46ada5c..c4c1a2b 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -1322,6 +1322,8 @@ static void pci_init_capabilities(struct pci_dev *dev)
/* Power Management */
pci_pm_init(dev);
+ dev->sn = pci_device_serial_number(dev);
+
/* Vital Product Data */
pci_vpd_pci22_init(dev);
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 0fd1f15..10d190b 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 serieal number, 0 if not support */
struct pci_vpd *vpd;
#ifdef CONFIG_PCI_ATS
union {
@@ -995,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);
+u64 pci_device_serial_number(struct pci_dev *dev);
+
/* 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
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [PATCH -v2 2/3] PCI,pciehp: avoid add a device already exist before suspend during resume
2013-07-11 9:43 [PATCH -v2 0/3] Use PCIe DSN to improve pciehp_resume Yijing Wang
2013-07-11 9:43 ` [PATCH -v2 1/3] PCI: introduce PCIe Device Serial Number Capability support Yijing Wang
@ 2013-07-11 9:43 ` Yijing Wang
2013-07-11 14:27 ` Paul Bolle
2013-07-11 9:43 ` [PATCH -v2 3/3] PCI,pciehp: use PCIe DSN to identify device change during suspend Yijing Wang
2013-07-11 14:19 ` [PATCH -v2 0/3] Use PCIe DSN to improve pciehp_resume Paul Bolle
3 siblings, 1 reply; 23+ messages in thread
From: Yijing Wang @ 2013-07-11 9:43 UTC (permalink / raw)
To: Bjorn Helgaas
Cc: linux-kernel, linux-pci, Rafael, Hanjun Guo, jiang.liu,
Yijing Wang, Paul Bolle, Oliver Neukum, Gu Zheng
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
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 | 9 ++++++---
1 files changed, 6 insertions(+), 3 deletions(-)
diff --git a/drivers/pci/hotplug/pciehp_core.c b/drivers/pci/hotplug/pciehp_core.c
index 7d72c5e..1542735 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,12 @@ 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
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [PATCH -v2 3/3] PCI,pciehp: use PCIe DSN to identify device change during suspend
2013-07-11 9:43 [PATCH -v2 0/3] Use PCIe DSN to improve pciehp_resume Yijing Wang
2013-07-11 9:43 ` [PATCH -v2 1/3] PCI: introduce PCIe Device Serial Number Capability support Yijing Wang
2013-07-11 9:43 ` [PATCH -v2 2/3] PCI,pciehp: avoid add a device already exist before suspend during resume Yijing Wang
@ 2013-07-11 9:43 ` Yijing Wang
2013-07-11 10:04 ` Don Dutile
2013-07-11 14:33 ` Paul Bolle
2013-07-11 14:19 ` [PATCH -v2 0/3] Use PCIe DSN to improve pciehp_resume Paul Bolle
3 siblings, 2 replies; 23+ messages in thread
From: Yijing Wang @ 2013-07-11 9:43 UTC (permalink / raw)
To: Bjorn Helgaas
Cc: linux-kernel, linux-pci, Rafael, Hanjun Guo, jiang.liu,
Yijing Wang, Paul Bolle, Oliver Neukum, Gu Zheng
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 | 45 +++++++++++++++++++++++++++++++++++++
1 files changed, 45 insertions(+), 0 deletions(-)
diff --git a/drivers/pci/hotplug/pciehp_core.c b/drivers/pci/hotplug/pciehp_core.c
index 1542735..f2eb214 100644
--- a/drivers/pci/hotplug/pciehp_core.c
+++ b/drivers/pci/hotplug/pciehp_core.c
@@ -296,11 +296,38 @@ static int pciehp_suspend (struct pcie_device *dev)
return 0;
}
+/*
+ * check the func 0 device serial number is changed,
+ * if device does not support device serial number,
+ * return false.
+ */
+static bool device_serial_number_changed(struct pci_bus *pbus)
+{
+ u64 old_dsn, new_dsn;
+ struct pci_dev *pdev;
+
+ pdev = pci_get_slot(pbus, PCI_DEVFN(0, 0));
+ if (!pdev)
+ return false;
+
+ old_dsn = pdev->sn;
+
+ /* get func 0 device serial number */
+ new_dsn = pci_device_serial_number(pdev);
+ pci_dev_put(pdev);
+
+ if (old_dsn != new_dsn)
+ return true;
+
+ return false;
+}
+
static int pciehp_resume (struct pcie_device *dev)
{
struct controller *ctrl;
struct slot *slot;
struct pci_bus *pbus = dev->port->subordinate;
+ int retval = 0;
u8 status;
ctrl = get_service_data(dev);
@@ -315,6 +342,24 @@ static int pciehp_resume (struct pcie_device *dev)
if (status) {
if (list_empty(&pbus->devices))
pciehp_enable_slot(slot);
+
+ if (device_serial_number_changed(pbus)) {
+ /*
+ * first power off slot, avoid the old driver
+ * .remove() method touch the new hardware
+ */
+ if (POWER_CTRL(ctrl)) {
+ retval = pciehp_power_off_slot(slot);
+ if (retval) {
+ ctrl_err(ctrl,
+ "Issue of Slot Disable command failed\n");
+ return 0;
+ }
+ msleep(1000);
+ pciehp_unconfigure_device(slot);
+ pciehp_enable_slot(slot);
+ }
+ }
} else if (!list_empty(&pbus->devices))
pciehp_disable_slot(slot);
--
1.7.1
^ permalink raw reply related [flat|nested] 23+ messages in thread
* Re: [PATCH -v2 1/3] PCI: introduce PCIe Device Serial Number Capability support
2013-07-11 9:43 ` [PATCH -v2 1/3] PCI: introduce PCIe Device Serial Number Capability support Yijing Wang
@ 2013-07-11 9:51 ` Don Dutile
2013-07-11 20:09 ` Bjorn Helgaas
2013-07-12 1:38 ` Yijing Wang
2013-07-11 14:22 ` Paul Bolle
1 sibling, 2 replies; 23+ messages in thread
From: Don Dutile @ 2013-07-11 9:51 UTC (permalink / raw)
To: Yijing Wang
Cc: Bjorn Helgaas, linux-kernel, linux-pci, Rafael, Hanjun Guo, jiang.liu
On 07/11/2013 05:43 AM, Yijing Wang wrote:
> 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 inserted a new same device, after
> 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>
> ---
> drivers/pci/pci.c | 27 +++++++++++++++++++++++++++
> drivers/pci/probe.c | 2 ++
> include/linux/pci.h | 3 +++
> 3 files changed, 32 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index e37fea6..2e855b5 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -2048,6 +2048,33 @@ 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.
> + */
> +u64 pci_device_serial_number(struct pci_dev *dev)
See comment below:
void pci_device_serial_number(struct pci_dev *dev)
> +{
> + int pos;
> + u64 sn;
> + u32 lo, hi;
> +
> + if (!pci_is_pcie(dev))
> + return 0;
> +
See comment below:
if (!pci_is_pcie(dev)) {
dev->sn = 0;
return;
}
> + 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);
> + sn = ((u64)hi<< 32) | lo;
See comment below:
dev->sn = ((u64)hi<< 32) | lo;
return;
> + return sn;
> +}
> +EXPORT_SYMBOL(pci_device_serial_number);
> +
> +/**
> * pci_configure_ari - enable or disable ARI forwarding
> * @dev: the PCI device
> *
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index 46ada5c..c4c1a2b 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -1322,6 +1322,8 @@ static void pci_init_capabilities(struct pci_dev *dev)
> /* Power Management */
> pci_pm_init(dev);
>
> + dev->sn = pci_device_serial_number(dev);
> +
Finally, 'the comment below':
I know you were following Bjorn's suggestion, which I thought
was an improvement, but why not do above assignment in pci_device_serial_number() ?
See above....
- Don
> /* Vital Product Data */
> pci_vpd_pci22_init(dev);
>
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 0fd1f15..10d190b 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 serieal number, 0 if not support */
> struct pci_vpd *vpd;
> #ifdef CONFIG_PCI_ATS
> union {
> @@ -995,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);
>
> +u64 pci_device_serial_number(struct pci_dev *dev);
> +
> /* 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);
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH -v2 3/3] PCI,pciehp: use PCIe DSN to identify device change during suspend
2013-07-11 9:43 ` [PATCH -v2 3/3] PCI,pciehp: use PCIe DSN to identify device change during suspend Yijing Wang
@ 2013-07-11 10:04 ` Don Dutile
2013-07-11 14:33 ` Paul Bolle
1 sibling, 0 replies; 23+ messages in thread
From: Don Dutile @ 2013-07-11 10:04 UTC (permalink / raw)
To: Yijing Wang
Cc: Bjorn Helgaas, linux-kernel, linux-pci, Rafael, Hanjun Guo,
jiang.liu, Paul Bolle, Oliver Neukum, Gu Zheng
Sorry, did a 'reply' instead of 'reply all' on first reply to this patch...
excuse any repeat...
- Don
On 07/11/2013 05:43 AM, 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 | 45 +++++++++++++++++++++++++++++++++++++
> 1 files changed, 45 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/pci/hotplug/pciehp_core.c b/drivers/pci/hotplug/pciehp_core.c
> index 1542735..f2eb214 100644
> --- a/drivers/pci/hotplug/pciehp_core.c
> +++ b/drivers/pci/hotplug/pciehp_core.c
> @@ -296,11 +296,38 @@ static int pciehp_suspend (struct pcie_device *dev)
> return 0;
> }
>
> +/*
> + * check the func 0 device serial number is changed,
> + * if device does not support device serial number,
> + * return false.
> + */
> +static bool device_serial_number_changed(struct pci_bus *pbus)
> +{
> + u64 old_dsn, new_dsn;
> + struct pci_dev *pdev;
> +
> + pdev = pci_get_slot(pbus, PCI_DEVFN(0, 0));
> + if (!pdev)
> + return false;
> +
> + old_dsn = pdev->sn;
> +
> + /* get func 0 device serial number */
> + new_dsn = pci_device_serial_number(pdev);
Due to previous recommended change to 1/3 patch, above would be:
pci_device_serial_number(pdev);
> + pci_dev_put(pdev);
> +
> + if (old_dsn != new_dsn)
> + return true;
> +
and above would be:
new_dsn = pdev->dsn;
pci_dev_put(pdev);
if(pdev->dsn != old_dsn)
return true;
> + return false;
> +}
> +
> static int pciehp_resume (struct pcie_device *dev)
> {
> struct controller *ctrl;
> struct slot *slot;
> struct pci_bus *pbus = dev->port->subordinate;
> + int retval = 0;
> u8 status;
>
> ctrl = get_service_data(dev);
> @@ -315,6 +342,24 @@ static int pciehp_resume (struct pcie_device *dev)
> if (status) {
> if (list_empty(&pbus->devices))
> pciehp_enable_slot(slot);
> +
> + if (device_serial_number_changed(pbus)) {
> + /*
> + * first power off slot, avoid the old driver
> + * .remove() method touch the new hardware
> + */
> + if (POWER_CTRL(ctrl)) {
> + retval = pciehp_power_off_slot(slot);
> + if (retval) {
> + ctrl_err(ctrl,
> + "Issue of Slot Disable command failed\n");
> + return 0;
> + }
> + msleep(1000);
> + pciehp_unconfigure_device(slot);
> + pciehp_enable_slot(slot);
> + }
> + }
> } else if (!list_empty(&pbus->devices))
> pciehp_disable_slot(slot);
>
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH -v2 0/3] Use PCIe DSN to improve pciehp_resume
2013-07-11 9:43 [PATCH -v2 0/3] Use PCIe DSN to improve pciehp_resume Yijing Wang
` (2 preceding siblings ...)
2013-07-11 9:43 ` [PATCH -v2 3/3] PCI,pciehp: use PCIe DSN to identify device change during suspend Yijing Wang
@ 2013-07-11 14:19 ` Paul Bolle
2013-07-12 1:52 ` Yijing Wang
3 siblings, 1 reply; 23+ messages in thread
From: Paul Bolle @ 2013-07-11 14:19 UTC (permalink / raw)
To: Yijing Wang
Cc: Bjorn Helgaas, linux-kernel, linux-pci, Rafael, Hanjun Guo, jiang.liu
On Thu, 2013-07-11 at 17:43 +0800, Yijing Wang wrote:
> 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.
>
>
> Yijing Wang (3):
> PCI: introduce PCIe Device Serial Number Capability support
> PCI,pciehp: avoid add a device already exist before suspend during
> resume
> PCI,pciehp: use PCIe DSN to identify device change during suspend
>
> drivers/pci/hotplug/pciehp_core.c | 54 ++++++++++++++++++++++++++++++++++--
> drivers/pci/pci.c | 27 ++++++++++++++++++
> drivers/pci/probe.c | 2 +
> include/linux/pci.h | 3 ++
> 4 files changed, 83 insertions(+), 3 deletions(-)
Series applies cleanly to v3.10 (but there was a small problem with 3/3,
which I'll mention in a reply to that patch). Compiles without warning.
Those two errors on every resume are now gone!
Thanks.
Paul Bolle
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH -v2 1/3] PCI: introduce PCIe Device Serial Number Capability support
2013-07-11 9:43 ` [PATCH -v2 1/3] PCI: introduce PCIe Device Serial Number Capability support Yijing Wang
2013-07-11 9:51 ` Don Dutile
@ 2013-07-11 14:22 ` Paul Bolle
2013-07-12 1:55 ` Yijing Wang
1 sibling, 1 reply; 23+ messages in thread
From: Paul Bolle @ 2013-07-11 14:22 UTC (permalink / raw)
To: Yijing Wang
Cc: Bjorn Helgaas, linux-kernel, linux-pci, Rafael, Hanjun Guo, jiang.liu
On Thu, 2013-07-11 at 17:43 +0800, Yijing Wang wrote:
> 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 inserted a new same device, after
> 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>
> ---
> drivers/pci/pci.c | 27 +++++++++++++++++++++++++++
> drivers/pci/probe.c | 2 ++
> include/linux/pci.h | 3 +++
> 3 files changed, 32 insertions(+), 0 deletions(-)
>
[...]
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 0fd1f15..10d190b 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 serieal number, 0 if not support */
Typo: serieal
> struct pci_vpd *vpd;
> #ifdef CONFIG_PCI_ATS
> union {
[...]
Paul Bolle
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH -v2 2/3] PCI,pciehp: avoid add a device already exist before suspend during resume
2013-07-11 9:43 ` [PATCH -v2 2/3] PCI,pciehp: avoid add a device already exist before suspend during resume Yijing Wang
@ 2013-07-11 14:27 ` Paul Bolle
2013-07-12 2:19 ` Yijing Wang
0 siblings, 1 reply; 23+ messages in thread
From: Paul Bolle @ 2013-07-11 14:27 UTC (permalink / raw)
To: Yijing Wang
Cc: Bjorn Helgaas, linux-kernel, linux-pci, Rafael, Hanjun Guo,
jiang.liu, Oliver Neukum, Gu Zheng
On Thu, 2013-07-11 at 17:43 +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
>
> 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 | 9 ++++++---
> 1 files changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/pci/hotplug/pciehp_core.c b/drivers/pci/hotplug/pciehp_core.c
> index 7d72c5e..1542735 100644
> --- a/drivers/pci/hotplug/pciehp_core.c
> +++ b/drivers/pci/hotplug/pciehp_core.c
[...]
> @@ -311,10 +312,12 @@ 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);
> +
Coding style: braces for the "else if" branch too? Or change the first
test to "if (status && list_empty([...]))" and drop the braces?
> return 0;
> }
> #endif /* PM */
Paul Bolle
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH -v2 3/3] PCI,pciehp: use PCIe DSN to identify device change during suspend
2013-07-11 9:43 ` [PATCH -v2 3/3] PCI,pciehp: use PCIe DSN to identify device change during suspend Yijing Wang
2013-07-11 10:04 ` Don Dutile
@ 2013-07-11 14:33 ` Paul Bolle
2013-07-12 2:19 ` Yijing Wang
1 sibling, 1 reply; 23+ messages in thread
From: Paul Bolle @ 2013-07-11 14:33 UTC (permalink / raw)
To: Yijing Wang
Cc: Bjorn Helgaas, linux-kernel, linux-pci, Rafael, Hanjun Guo,
jiang.liu, Oliver Neukum, Gu Zheng
On Thu, 2013-07-11 at 17:43 +0800, Yijing Wang wrote:
[...]
> diff --git a/drivers/pci/hotplug/pciehp_core.c b/drivers/pci/hotplug/pciehp_core.c
> index 1542735..f2eb214 100644
> --- a/drivers/pci/hotplug/pciehp_core.c
> +++ b/drivers/pci/hotplug/pciehp_core.c
[...]
> @@ -315,6 +342,24 @@ static int pciehp_resume (struct pcie_device *dev)
> if (status) {
> if (list_empty(&pbus->devices))
> pciehp_enable_slot(slot);
> +
> + if (device_serial_number_changed(pbus)) {
> + /*
> + * first power off slot, avoid the old driver
> + * .remove() method touch the new hardware
> + */
> + if (POWER_CTRL(ctrl)) {
> + retval = pciehp_power_off_slot(slot);
> + if (retval) {
> + ctrl_err(ctrl,
> + "Issue of Slot Disable command failed\n");
> + return 0;
> + }
> + msleep(1000);
> + pciehp_unconfigure_device(slot);
> + pciehp_enable_slot(slot);
> + }
> + }
> } else if (!list_empty(&pbus->devices))
> pciehp_disable_slot(slot);
>
It was surprisingly hard to see why the patch wouldn't apply to v3.10.
It turns out the very last line of context is a line consisting of just
a single tab. And in v3.10 it is an empty line.
Is that lone tab perhaps an editing mistake on your side?
Paul Bolle
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH -v2 1/3] PCI: introduce PCIe Device Serial Number Capability support
2013-07-11 20:09 ` Bjorn Helgaas
@ 2013-07-11 18:18 ` Don Dutile
2013-07-12 2:39 ` Yijing Wang
2013-07-12 3:37 ` Bjorn Helgaas
2013-07-12 2:30 ` Yijing Wang
1 sibling, 2 replies; 23+ messages in thread
From: Don Dutile @ 2013-07-11 18:18 UTC (permalink / raw)
To: Bjorn Helgaas
Cc: Yijing Wang, linux-kernel, linux-pci, Rafael, Hanjun Guo, Jiang Liu
On 07/11/2013 04:09 PM, Bjorn Helgaas wrote:
> On Thu, Jul 11, 2013 at 3:51 AM, Don Dutile<ddutile@redhat.com> wrote:
>> On 07/11/2013 05:43 AM, Yijing Wang wrote:
>>>
>>> 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 inserted a new same device, after
>>> 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>
>>> ---
>>> drivers/pci/pci.c | 27 +++++++++++++++++++++++++++
>>> drivers/pci/probe.c | 2 ++
>>> include/linux/pci.h | 3 +++
>>> 3 files changed, 32 insertions(+), 0 deletions(-)
>>>
>>> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
>>> index e37fea6..2e855b5 100644
>>> --- a/drivers/pci/pci.c
>>> +++ b/drivers/pci/pci.c
>>> @@ -2048,6 +2048,33 @@ 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.
>>> + */
>>> +u64 pci_device_serial_number(struct pci_dev *dev)
>>
>> See comment below:
>> void pci_device_serial_number(struct pci_dev *dev)
>>
>>
>>> +{
>>> + int pos;
>>> + u64 sn;
>>> + u32 lo, hi;
>>> +
>>> + if (!pci_is_pcie(dev))
>>> + return 0;
>>> +
>>
>> See comment below:
>> if (!pci_is_pcie(dev)) {
>> dev->sn = 0;
>> return;
>>
>> }
>>>
>>> + 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);
>>> + sn = ((u64)hi<< 32) | lo;
>>
>> See comment below:
>> dev->sn = ((u64)hi<< 32) | lo;
>> return;
>>
>>> + return sn;
>>> +}
>>> +EXPORT_SYMBOL(pci_device_serial_number);
>>> +
>>> +/**
>>> * pci_configure_ari - enable or disable ARI forwarding
>>> * @dev: the PCI device
>>> *
>>> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
>>> index 46ada5c..c4c1a2b 100644
>>> --- a/drivers/pci/probe.c
>>> +++ b/drivers/pci/probe.c
>>> @@ -1322,6 +1322,8 @@ static void pci_init_capabilities(struct pci_dev
>>> *dev)
>>> /* Power Management */
>>> pci_pm_init(dev);
>>>
>>> + dev->sn = pci_device_serial_number(dev);
>>> +
>>
>> Finally, 'the comment below':
>> I know you were following Bjorn's suggestion, which I thought
>> was an improvement, but why not do above assignment in
>> pci_device_serial_number() ?
>> See above....
>
> pci_device_serial_number() would then have the side-effect of saving
> the result somewhere, and callers would have to know where to look.
Ah, like so many other features of a PCI device? -- what are all those
flags/status bits in pdev for ??? ;-)
> Personally, I think it's simpler to return the serial number directly
> and avoid the side-effect, but maybe this is just bike-shedding.
>
What struck me about rtning a value is in pci_init_capabilities(),
it was the only function with a rtn value that had to be put into a pci_dev
struct element,
while all the others stored their related capabilities in the pdev, or other struct
related to it.
So, maybe there should be a "pci_device_serial_number_init()" function
which does the storage in the pdev struct, and another (wrapper,
dare I shoot myself, inline) that is "pci_device_serial_number()" that returns
the value of the pdev's sn element, if another module or other core component
want to get it. .... and do that in a future patch... :-/
> As long as we are bike-shedding, I'd just drop "sn" and do:
>
> return ((u64)hi<< 32) | lo;
>
> And of course, you need spaces before "&hi" and "&lo" in the
> pci_read_config_dword() calls.
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH -v2 1/3] PCI: introduce PCIe Device Serial Number Capability support
2013-07-11 9:51 ` Don Dutile
@ 2013-07-11 20:09 ` Bjorn Helgaas
2013-07-11 18:18 ` Don Dutile
2013-07-12 2:30 ` Yijing Wang
2013-07-12 1:38 ` Yijing Wang
1 sibling, 2 replies; 23+ messages in thread
From: Bjorn Helgaas @ 2013-07-11 20:09 UTC (permalink / raw)
To: Don Dutile
Cc: Yijing Wang, linux-kernel, linux-pci, Rafael, Hanjun Guo, Jiang Liu
On Thu, Jul 11, 2013 at 3:51 AM, Don Dutile <ddutile@redhat.com> wrote:
> On 07/11/2013 05:43 AM, Yijing Wang wrote:
>>
>> 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 inserted a new same device, after
>> 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>
>> ---
>> drivers/pci/pci.c | 27 +++++++++++++++++++++++++++
>> drivers/pci/probe.c | 2 ++
>> include/linux/pci.h | 3 +++
>> 3 files changed, 32 insertions(+), 0 deletions(-)
>>
>> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
>> index e37fea6..2e855b5 100644
>> --- a/drivers/pci/pci.c
>> +++ b/drivers/pci/pci.c
>> @@ -2048,6 +2048,33 @@ 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.
>> + */
>> +u64 pci_device_serial_number(struct pci_dev *dev)
>
> See comment below:
> void pci_device_serial_number(struct pci_dev *dev)
>
>
>> +{
>> + int pos;
>> + u64 sn;
>> + u32 lo, hi;
>> +
>> + if (!pci_is_pcie(dev))
>> + return 0;
>> +
>
> See comment below:
> if (!pci_is_pcie(dev)) {
> dev->sn = 0;
> return;
>
> }
>>
>> + 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);
>> + sn = ((u64)hi<< 32) | lo;
>
> See comment below:
> dev->sn = ((u64)hi<< 32) | lo;
> return;
>
>> + return sn;
>> +}
>> +EXPORT_SYMBOL(pci_device_serial_number);
>> +
>> +/**
>> * pci_configure_ari - enable or disable ARI forwarding
>> * @dev: the PCI device
>> *
>> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
>> index 46ada5c..c4c1a2b 100644
>> --- a/drivers/pci/probe.c
>> +++ b/drivers/pci/probe.c
>> @@ -1322,6 +1322,8 @@ static void pci_init_capabilities(struct pci_dev
>> *dev)
>> /* Power Management */
>> pci_pm_init(dev);
>>
>> + dev->sn = pci_device_serial_number(dev);
>> +
>
> Finally, 'the comment below':
> I know you were following Bjorn's suggestion, which I thought
> was an improvement, but why not do above assignment in
> pci_device_serial_number() ?
> See above....
pci_device_serial_number() would then have the side-effect of saving
the result somewhere, and callers would have to know where to look.
Personally, I think it's simpler to return the serial number directly
and avoid the side-effect, but maybe this is just bike-shedding.
As long as we are bike-shedding, I'd just drop "sn" and do:
return ((u64)hi << 32) | lo;
And of course, you need spaces before "&hi" and "&lo" in the
pci_read_config_dword() calls.
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH -v2 1/3] PCI: introduce PCIe Device Serial Number Capability support
2013-07-11 9:51 ` Don Dutile
2013-07-11 20:09 ` Bjorn Helgaas
@ 2013-07-12 1:38 ` Yijing Wang
2013-07-12 2:43 ` Don Dutile
1 sibling, 1 reply; 23+ messages in thread
From: Yijing Wang @ 2013-07-12 1:38 UTC (permalink / raw)
To: Don Dutile
Cc: Bjorn Helgaas, linux-kernel, linux-pci, Rafael, Hanjun Guo, jiang.liu
Hi Don,
Thanks for your review and comments very much!
>> + dev->sn = pci_device_serial_number(dev);
>> +
> Finally, 'the comment below':
> I know you were following Bjorn's suggestion, which I thought
> was an improvement, but why not do above assignment in pci_device_serial_number() ?
I don't do assignment in pci_device_serial_number() because pci_device_serial_number()
is an EXPORT_SYMBOL, and will be used to get dsn for non-scaned device, for example:
1. we have pcie device in slot before suspend.
2. we removed the device during suspend.
3. we reinserted a new device during suspend.
4. we should check the device change during resume, so we
try to use pcie device serial number to identify changes.
But the pci_dev structure maybe the old stale data. So
I prefer to get dsn by return.
what about this:
static void pci_init_capabilities(struct pci_dev *dev)
........
pci_dsn_init(dev);
..........
void pci_dsn_init(dev)
{
dev->sn = pci_device_serial_number(dev);
}
> See above....
> - Don
>
>> /* Vital Product Data */
>> pci_vpd_pci22_init(dev);
>>
>> diff --git a/include/linux/pci.h b/include/linux/pci.h
>> index 0fd1f15..10d190b 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 serieal number, 0 if not support */
>> struct pci_vpd *vpd;
>> #ifdef CONFIG_PCI_ATS
>> union {
>> @@ -995,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);
>>
>> +u64 pci_device_serial_number(struct pci_dev *dev);
>> +
>> /* 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);
>
>
> .
>
--
Thanks!
Yijing
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH -v2 0/3] Use PCIe DSN to improve pciehp_resume
2013-07-11 14:19 ` [PATCH -v2 0/3] Use PCIe DSN to improve pciehp_resume Paul Bolle
@ 2013-07-12 1:52 ` Yijing Wang
2013-07-13 10:20 ` Paul Bolle
0 siblings, 1 reply; 23+ messages in thread
From: Yijing Wang @ 2013-07-12 1:52 UTC (permalink / raw)
To: Paul Bolle
Cc: Bjorn Helgaas, linux-kernel, linux-pci, Rafael, Hanjun Guo, jiang.liu
On 2013/7/11 22:19, Paul Bolle wrote:
> On Thu, 2013-07-11 at 17:43 +0800, Yijing Wang wrote:
>> 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.
>>
>>
>> Yijing Wang (3):
>> PCI: introduce PCIe Device Serial Number Capability support
>> PCI,pciehp: avoid add a device already exist before suspend during
>> resume
>> PCI,pciehp: use PCIe DSN to identify device change during suspend
>>
>> drivers/pci/hotplug/pciehp_core.c | 54 ++++++++++++++++++++++++++++++++++--
>> drivers/pci/pci.c | 27 ++++++++++++++++++
>> drivers/pci/probe.c | 2 +
>> include/linux/pci.h | 3 ++
>> 4 files changed, 83 insertions(+), 3 deletions(-)
>
> Series applies cleanly to v3.10 (but there was a small problem with 3/3,
> which I'll mention in a reply to that patch). Compiles without warning.
> Those two errors on every resume are now gone!
I will check patch 3/3, Paul, Do you wireless card support Device Serial Number ?
You can confirm it by lspci -vvv.
>
> Thanks.
>
>
> Paul Bolle
>
>
> .
>
--
Thanks!
Yijing
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH -v2 1/3] PCI: introduce PCIe Device Serial Number Capability support
2013-07-11 14:22 ` Paul Bolle
@ 2013-07-12 1:55 ` Yijing Wang
0 siblings, 0 replies; 23+ messages in thread
From: Yijing Wang @ 2013-07-12 1:55 UTC (permalink / raw)
To: Paul Bolle
Cc: Bjorn Helgaas, linux-kernel, linux-pci, Rafael, Hanjun Guo, jiang.liu
>>
> [...]
>> diff --git a/include/linux/pci.h b/include/linux/pci.h
>> index 0fd1f15..10d190b 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 serieal number, 0 if not support */
>
> Typo: serieal
Thanks very much! Will update.
>
>> struct pci_vpd *vpd;
>> #ifdef CONFIG_PCI_ATS
>> union {
>
> [...]
>
>
> Paul Bolle
>
>
> .
>
--
Thanks!
Yijing
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH -v2 2/3] PCI,pciehp: avoid add a device already exist before suspend during resume
2013-07-11 14:27 ` Paul Bolle
@ 2013-07-12 2:19 ` Yijing Wang
0 siblings, 0 replies; 23+ messages in thread
From: Yijing Wang @ 2013-07-12 2:19 UTC (permalink / raw)
To: Paul Bolle
Cc: Bjorn Helgaas, linux-kernel, linux-pci, Rafael, Hanjun Guo,
jiang.liu, Oliver Neukum, Gu Zheng
>> ---
>> drivers/pci/hotplug/pciehp_core.c | 9 ++++++---
>> 1 files changed, 6 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/pci/hotplug/pciehp_core.c b/drivers/pci/hotplug/pciehp_core.c
>> index 7d72c5e..1542735 100644
>> --- a/drivers/pci/hotplug/pciehp_core.c
>> +++ b/drivers/pci/hotplug/pciehp_core.c
> [...]
>> @@ -311,10 +312,12 @@ 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);
>> +
>
> Coding style: braces for the "else if" branch too? Or change the first
> test to "if (status && list_empty([...]))" and drop the braces?
Hmmm, I will add the braces for "else if "
Change the first test "if (status && list_empty([...]))" is not a good idea,
because this change will modify the code logic, for example
if a device was present before suspend and still there during resume, we should do nothing,
but after the logic change, we may disable it.
>
>> return 0;
>> }
>> #endif /* PM */
>
>
> Paul Bolle
>
>
> .
>
--
Thanks!
Yijing
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH -v2 3/3] PCI,pciehp: use PCIe DSN to identify device change during suspend
2013-07-11 14:33 ` Paul Bolle
@ 2013-07-12 2:19 ` Yijing Wang
0 siblings, 0 replies; 23+ messages in thread
From: Yijing Wang @ 2013-07-12 2:19 UTC (permalink / raw)
To: Paul Bolle
Cc: Bjorn Helgaas, linux-kernel, linux-pci, Rafael, Hanjun Guo,
jiang.liu, Oliver Neukum, Gu Zheng
On 2013/7/11 22:33, Paul Bolle wrote:
> On Thu, 2013-07-11 at 17:43 +0800, Yijing Wang wrote:
> [...]
>> diff --git a/drivers/pci/hotplug/pciehp_core.c b/drivers/pci/hotplug/pciehp_core.c
>> index 1542735..f2eb214 100644
>> --- a/drivers/pci/hotplug/pciehp_core.c
>> +++ b/drivers/pci/hotplug/pciehp_core.c
> [...]
>> @@ -315,6 +342,24 @@ static int pciehp_resume (struct pcie_device *dev)
>> if (status) {
>> if (list_empty(&pbus->devices))
>> pciehp_enable_slot(slot);
>> +
>> + if (device_serial_number_changed(pbus)) {
>> + /*
>> + * first power off slot, avoid the old driver
>> + * .remove() method touch the new hardware
>> + */
>> + if (POWER_CTRL(ctrl)) {
>> + retval = pciehp_power_off_slot(slot);
>> + if (retval) {
>> + ctrl_err(ctrl,
>> + "Issue of Slot Disable command failed\n");
>> + return 0;
>> + }
>> + msleep(1000);
>> + pciehp_unconfigure_device(slot);
>> + pciehp_enable_slot(slot);
>> + }
>> + }
>> } else if (!list_empty(&pbus->devices))
>> pciehp_disable_slot(slot);
>>
>
> It was surprisingly hard to see why the patch wouldn't apply to v3.10.
> It turns out the very last line of context is a line consisting of just
> a single tab. And in v3.10 it is an empty line.
>
> Is that lone tab perhaps an editing mistake on your side?
I will check this, thanks!
>
>
> Paul Bolle
>
>
> .
>
--
Thanks!
Yijing
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH -v2 1/3] PCI: introduce PCIe Device Serial Number Capability support
2013-07-11 20:09 ` Bjorn Helgaas
2013-07-11 18:18 ` Don Dutile
@ 2013-07-12 2:30 ` Yijing Wang
1 sibling, 0 replies; 23+ messages in thread
From: Yijing Wang @ 2013-07-12 2:30 UTC (permalink / raw)
To: Bjorn Helgaas
Cc: Don Dutile, linux-kernel, linux-pci, Rafael, Hanjun Guo, Jiang Liu
>> }
>>>
>>> + 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);
>>> + sn = ((u64)hi<< 32) | lo;
>>
>> See comment below:
>> dev->sn = ((u64)hi<< 32) | lo;
>> return;
>>
>>> + return sn;
>>> +}
>>> +EXPORT_SYMBOL(pci_device_serial_number);
>>> +
>>> +/**
>>> * pci_configure_ari - enable or disable ARI forwarding
>>> * @dev: the PCI device
>>> *
>>> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
>>> index 46ada5c..c4c1a2b 100644
>>> --- a/drivers/pci/probe.c
>>> +++ b/drivers/pci/probe.c
>>> @@ -1322,6 +1322,8 @@ static void pci_init_capabilities(struct pci_dev
>>> *dev)
>>> /* Power Management */
>>> pci_pm_init(dev);
>>>
>>> + dev->sn = pci_device_serial_number(dev);
>>> +
>>
>> Finally, 'the comment below':
>> I know you were following Bjorn's suggestion, which I thought
>> was an improvement, but why not do above assignment in
>> pci_device_serial_number() ?
>> See above....
>
> pci_device_serial_number() would then have the side-effect of saving
> the result somewhere, and callers would have to know where to look.
> Personally, I think it's simpler to return the serial number directly
> and avoid the side-effect, but maybe this is just bike-shedding.
>
> As long as we are bike-shedding, I'd just drop "sn" and do:
>
> return ((u64)hi << 32) | lo;
>
> And of course, you need spaces before "&hi" and "&lo" in the
> pci_read_config_dword() calls.
OK, will drop "sn", strangely my original patch has spaces before "&hi" and "&lo",
but in reply the spaces disappeared.
Thanks!
Yijing.
>
> .
>
--
Thanks!
Yijing
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH -v2 1/3] PCI: introduce PCIe Device Serial Number Capability support
2013-07-11 18:18 ` Don Dutile
@ 2013-07-12 2:39 ` Yijing Wang
2013-07-12 3:37 ` Bjorn Helgaas
1 sibling, 0 replies; 23+ messages in thread
From: Yijing Wang @ 2013-07-12 2:39 UTC (permalink / raw)
To: Don Dutile
Cc: Bjorn Helgaas, linux-kernel, linux-pci, Rafael, Hanjun Guo, Jiang Liu
On 2013/7/12 2:18, Don Dutile wrote:
> On 07/11/2013 04:09 PM, Bjorn Helgaas wrote:
>> On Thu, Jul 11, 2013 at 3:51 AM, Don Dutile<ddutile@redhat.com> wrote:
>>> On 07/11/2013 05:43 AM, Yijing Wang wrote:
>>>>
>>>> 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 inserted a new same device, after
>>>> 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>
>>>> ---
>>>> drivers/pci/pci.c | 27 +++++++++++++++++++++++++++
>>>> drivers/pci/probe.c | 2 ++
>>>> include/linux/pci.h | 3 +++
>>>> 3 files changed, 32 insertions(+), 0 deletions(-)
>>>>
>>>> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
>>>> index e37fea6..2e855b5 100644
>>>> --- a/drivers/pci/pci.c
>>>> +++ b/drivers/pci/pci.c
>>>> @@ -2048,6 +2048,33 @@ 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.
>>>> + */
>>>> +u64 pci_device_serial_number(struct pci_dev *dev)
>>>
>>> See comment below:
>>> void pci_device_serial_number(struct pci_dev *dev)
>>>
>>>
>>>> +{
>>>> + int pos;
>>>> + u64 sn;
>>>> + u32 lo, hi;
>>>> +
>>>> + if (!pci_is_pcie(dev))
>>>> + return 0;
>>>> +
>>>
>>> See comment below:
>>> if (!pci_is_pcie(dev)) {
>>> dev->sn = 0;
>>> return;
>>>
>>> }
>>>>
>>>> + 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);
>>>> + sn = ((u64)hi<< 32) | lo;
>>>
>>> See comment below:
>>> dev->sn = ((u64)hi<< 32) | lo;
>>> return;
>>>
>>>> + return sn;
>>>> +}
>>>> +EXPORT_SYMBOL(pci_device_serial_number);
>>>> +
>>>> +/**
>>>> * pci_configure_ari - enable or disable ARI forwarding
>>>> * @dev: the PCI device
>>>> *
>>>> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
>>>> index 46ada5c..c4c1a2b 100644
>>>> --- a/drivers/pci/probe.c
>>>> +++ b/drivers/pci/probe.c
>>>> @@ -1322,6 +1322,8 @@ static void pci_init_capabilities(struct pci_dev
>>>> *dev)
>>>> /* Power Management */
>>>> pci_pm_init(dev);
>>>>
>>>> + dev->sn = pci_device_serial_number(dev);
>>>> +
>>>
>>> Finally, 'the comment below':
>>> I know you were following Bjorn's suggestion, which I thought
>>> was an improvement, but why not do above assignment in
>>> pci_device_serial_number() ?
>>> See above....
>>
>> pci_device_serial_number() would then have the side-effect of saving
>> the result somewhere, and callers would have to know where to look.
> Ah, like so many other features of a PCI device? -- what are all those
> flags/status bits in pdev for ??? ;-)
>
>> Personally, I think it's simpler to return the serial number directly
>> and avoid the side-effect, but maybe this is just bike-shedding.
>>
> What struck me about rtning a value is in pci_init_capabilities(),
> it was the only function with a rtn value that had to be put into a pci_dev struct element,
> while all the others stored their related capabilities in the pdev, or other struct
> related to it.
>
> So, maybe there should be a "pci_device_serial_number_init()" function
> which does the storage in the pdev struct, and another (wrapper,
> dare I shoot myself, inline) that is "pci_device_serial_number()" that returns
add an wrapper function may be ok, as I mentioned in pre-reply, what about,
static void pci_init_capabilities(struct pci_dev *dev)
...............
pci_dsn_init(dev);
.................
void pci_dsn_init(dev)
{
dev->sn = pci_device_serial_number(dev);
}
> the value of the pdev's sn element, if another module or other core component
> want to get it. .... and do that in a future patch... :-/
>
>> As long as we are bike-shedding, I'd just drop "sn" and do:
>>
>> return ((u64)hi<< 32) | lo;
>>
>> And of course, you need spaces before "&hi" and "&lo" in the
>> pci_read_config_dword() calls.
>
>
> .
>
--
Thanks!
Yijing
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH -v2 1/3] PCI: introduce PCIe Device Serial Number Capability support
2013-07-12 1:38 ` Yijing Wang
@ 2013-07-12 2:43 ` Don Dutile
0 siblings, 0 replies; 23+ messages in thread
From: Don Dutile @ 2013-07-12 2:43 UTC (permalink / raw)
To: Yijing Wang
Cc: Bjorn Helgaas, linux-kernel, linux-pci, Rafael, Hanjun Guo, jiang.liu
On 07/11/2013 09:38 PM, Yijing Wang wrote:
> Hi Don,
> Thanks for your review and comments very much!
>
>>> + dev->sn = pci_device_serial_number(dev);
>>> +
>> Finally, 'the comment below':
>> I know you were following Bjorn's suggestion, which I thought
>> was an improvement, but why not do above assignment in pci_device_serial_number() ?
>
> I don't do assignment in pci_device_serial_number() because pci_device_serial_number()
> is an EXPORT_SYMBOL, and will be used to get dsn for non-scaned device, for example:
>
> 1. we have pcie device in slot before suspend.
> 2. we removed the device during suspend.
> 3. we reinserted a new device during suspend.
> 4. we should check the device change during resume, so we
> try to use pcie device serial number to identify changes.
> But the pci_dev structure maybe the old stale data. So
> I prefer to get dsn by return.
>
> what about this:
>
> static void pci_init_capabilities(struct pci_dev *dev)
> ........
> pci_dsn_init(dev);
> ..........
>
>
> void pci_dsn_init(dev)
> {
> dev->sn = pci_device_serial_number(dev);
> }
>
>
>
Looks good! I think that meets all the uses (use in init, post-suspend).
- Don
>> See above....
>> - Don
>>
>>> /* Vital Product Data */
>>> pci_vpd_pci22_init(dev);
>>>
>>> diff --git a/include/linux/pci.h b/include/linux/pci.h
>>> index 0fd1f15..10d190b 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 serieal number, 0 if not support */
>>> struct pci_vpd *vpd;
>>> #ifdef CONFIG_PCI_ATS
>>> union {
>>> @@ -995,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);
>>>
>>> +u64 pci_device_serial_number(struct pci_dev *dev);
>>> +
>>> /* 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);
>>
>>
>> .
>>
>
>
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH -v2 1/3] PCI: introduce PCIe Device Serial Number Capability support
2013-07-11 18:18 ` Don Dutile
2013-07-12 2:39 ` Yijing Wang
@ 2013-07-12 3:37 ` Bjorn Helgaas
1 sibling, 0 replies; 23+ messages in thread
From: Bjorn Helgaas @ 2013-07-12 3:37 UTC (permalink / raw)
To: Don Dutile
Cc: Yijing Wang, linux-kernel, linux-pci, Rafael, Hanjun Guo, Jiang Liu
On Thu, Jul 11, 2013 at 12:18 PM, Don Dutile <ddutile@redhat.com> wrote:
> On 07/11/2013 04:09 PM, Bjorn Helgaas wrote:
>>
>> On Thu, Jul 11, 2013 at 3:51 AM, Don Dutile<ddutile@redhat.com> wrote:
>>>
>>> On 07/11/2013 05:43 AM, Yijing Wang wrote:
>>>>
>>>>
>>>> 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 inserted a new same device, after
>>>> 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>
>>>> ---
>>>> drivers/pci/pci.c | 27 +++++++++++++++++++++++++++
>>>> drivers/pci/probe.c | 2 ++
>>>> include/linux/pci.h | 3 +++
>>>> 3 files changed, 32 insertions(+), 0 deletions(-)
>>>>
>>>> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
>>>> index e37fea6..2e855b5 100644
>>>> --- a/drivers/pci/pci.c
>>>> +++ b/drivers/pci/pci.c
>>>> @@ -2048,6 +2048,33 @@ 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.
>>>> + */
>>>> +u64 pci_device_serial_number(struct pci_dev *dev)
>>>
>>>
>>> See comment below:
>>> void pci_device_serial_number(struct pci_dev *dev)
>>>
>>>
>>>> +{
>>>> + int pos;
>>>> + u64 sn;
>>>> + u32 lo, hi;
>>>> +
>>>> + if (!pci_is_pcie(dev))
>>>> + return 0;
>>>> +
>>>
>>>
>>> See comment below:
>>> if (!pci_is_pcie(dev)) {
>>> dev->sn = 0;
>>> return;
>>>
>>> }
>>>>
>>>>
>>>> + 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);
>>>> + sn = ((u64)hi<< 32) | lo;
>>>
>>>
>>> See comment below:
>>> dev->sn = ((u64)hi<< 32) | lo;
>>> return;
>>>
>>>> + return sn;
>>>> +}
>>>> +EXPORT_SYMBOL(pci_device_serial_number);
>>>> +
>>>> +/**
>>>> * pci_configure_ari - enable or disable ARI forwarding
>>>> * @dev: the PCI device
>>>> *
>>>> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
>>>> index 46ada5c..c4c1a2b 100644
>>>> --- a/drivers/pci/probe.c
>>>> +++ b/drivers/pci/probe.c
>>>> @@ -1322,6 +1322,8 @@ static void pci_init_capabilities(struct pci_dev
>>>> *dev)
>>>> /* Power Management */
>>>> pci_pm_init(dev);
>>>>
>>>> + dev->sn = pci_device_serial_number(dev);
>>>> +
>>>
>>>
>>> Finally, 'the comment below':
>>> I know you were following Bjorn's suggestion, which I thought
>>> was an improvement, but why not do above assignment in
>>> pci_device_serial_number() ?
>>> See above....
>>
>>
>> pci_device_serial_number() would then have the side-effect of saving
>> the result somewhere, and callers would have to know where to look.
>
> Ah, like so many other features of a PCI device? -- what are all those
> flags/status bits in pdev for ??? ;-)
>
>
>> Personally, I think it's simpler to return the serial number directly
>> and avoid the side-effect, but maybe this is just bike-shedding.
>>
> What struck me about rtning a value is in pci_init_capabilities(),
> it was the only function with a rtn value that had to be put into a pci_dev
> struct element,
> while all the others stored their related capabilities in the pdev, or other
> struct
> related to it.
Good point, BTW. I hadn't thought about it from that angle.
> So, maybe there should be a "pci_device_serial_number_init()" function
> which does the storage in the pdev struct, and another (wrapper,
> dare I shoot myself, inline) that is "pci_device_serial_number()" that
> returns
> the value of the pdev's sn element, if another module or other core
> component
> want to get it. .... and do that in a future patch... :-/
Of course, for Yijing's use, it's important that we're able to get the
*non-cached* serial number, because the cached one belongs to the old,
pre-suspend device, and we want to look at the post-suspend device to
see if it's the same.
Sigh. I dunno, I'll have to finish sorting through this in a week or
so... (I'll be on vacation for the next few days.)
Bjorn
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH -v2 0/3] Use PCIe DSN to improve pciehp_resume
2013-07-12 1:52 ` Yijing Wang
@ 2013-07-13 10:20 ` Paul Bolle
2013-07-15 0:51 ` Yijing Wang
0 siblings, 1 reply; 23+ messages in thread
From: Paul Bolle @ 2013-07-13 10:20 UTC (permalink / raw)
To: Yijing Wang
Cc: Bjorn Helgaas, linux-kernel, linux-pci, Rafael, Hanjun Guo, jiang.liu
Yijing,
On Fri, 2013-07-12 at 09:52 +0800, Yijing Wang wrote:
> Do you wireless card support Device Serial Number ?
> You can confirm it by lspci -vvv.
Yes, it does:
lspci -vvv -s 03:00 | grep Serial
Capabilities: [140 v1] Device Serial Number [...]
Paul Bolle
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH -v2 0/3] Use PCIe DSN to improve pciehp_resume
2013-07-13 10:20 ` Paul Bolle
@ 2013-07-15 0:51 ` Yijing Wang
0 siblings, 0 replies; 23+ messages in thread
From: Yijing Wang @ 2013-07-15 0:51 UTC (permalink / raw)
To: Paul Bolle
Cc: Bjorn Helgaas, linux-kernel, linux-pci, Rafael, Hanjun Guo, jiang.liu
On 2013/7/13 18:20, Paul Bolle wrote:
> Yijing,
>
> On Fri, 2013-07-12 at 09:52 +0800, Yijing Wang wrote:
>> Do you wireless card support Device Serial Number ?
>> You can confirm it by lspci -vvv.
>
> Yes, it does:
> lspci -vvv -s 03:00 | grep Serial
> Capabilities: [140 v1] Device Serial Number [...]
>
OK, I got it, thanks!
>
> Paul Bolle
>
>
>
--
Thanks!
Yijing
^ permalink raw reply [flat|nested] 23+ messages in thread
end of thread, other threads:[~2013-07-15 0:51 UTC | newest]
Thread overview: 23+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-07-11 9:43 [PATCH -v2 0/3] Use PCIe DSN to improve pciehp_resume Yijing Wang
2013-07-11 9:43 ` [PATCH -v2 1/3] PCI: introduce PCIe Device Serial Number Capability support Yijing Wang
2013-07-11 9:51 ` Don Dutile
2013-07-11 20:09 ` Bjorn Helgaas
2013-07-11 18:18 ` Don Dutile
2013-07-12 2:39 ` Yijing Wang
2013-07-12 3:37 ` Bjorn Helgaas
2013-07-12 2:30 ` Yijing Wang
2013-07-12 1:38 ` Yijing Wang
2013-07-12 2:43 ` Don Dutile
2013-07-11 14:22 ` Paul Bolle
2013-07-12 1:55 ` Yijing Wang
2013-07-11 9:43 ` [PATCH -v2 2/3] PCI,pciehp: avoid add a device already exist before suspend during resume Yijing Wang
2013-07-11 14:27 ` Paul Bolle
2013-07-12 2:19 ` Yijing Wang
2013-07-11 9:43 ` [PATCH -v2 3/3] PCI,pciehp: use PCIe DSN to identify device change during suspend Yijing Wang
2013-07-11 10:04 ` Don Dutile
2013-07-11 14:33 ` Paul Bolle
2013-07-12 2:19 ` Yijing Wang
2013-07-11 14:19 ` [PATCH -v2 0/3] Use PCIe DSN to improve pciehp_resume Paul Bolle
2013-07-12 1:52 ` Yijing Wang
2013-07-13 10:20 ` Paul Bolle
2013-07-15 0:51 ` Yijing Wang
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.