* [PATCH -v3 1/3] PCI: introduce PCIe Device Serial Number Capability support
2013-07-12 9:32 [PATCH -v3 0/3] Use PCIe DSN to improve pciehp_resume Yijing Wang
@ 2013-07-12 9:32 ` Yijing Wang
2013-07-12 9:32 ` [PATCH -v3 2/3] PCI,pciehp: avoid add a device already exist before suspend during resume Yijing Wang
2013-07-12 9:32 ` [PATCH -v3 3/3] PCI,pciehp: use PCIe DSN to identify device change during suspend Yijing Wang
2 siblings, 0 replies; 9+ messages in thread
From: Yijing Wang @ 2013-07-12 9:32 UTC (permalink / raw)
To: Bjorn Helgaas
Cc: linux-kernel, Don Dutile, Paul Bolle, linux-pci, Rafael,
Hanjun Guo, jiang.liu, Yijing Wang, Oliver Neukum, Gu Zheng
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 | 30 ++++++++++++++++++++++++++++++
drivers/pci/pci.h | 1 +
drivers/pci/probe.c | 2 ++
include/linux/pci.h | 3 +++
4 files changed, 36 insertions(+), 0 deletions(-)
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index e37fea6..156e251 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -1985,6 +1985,11 @@ void pci_pm_init(struct pci_dev *dev)
}
}
+void pci_dsn_init(struct pci_dev *dev)
+{
+ dev->sn = pci_device_serial_number(dev);
+}
+
static void pci_add_saved_cap(struct pci_dev *pci_dev,
struct pci_cap_saved_state *new_cap)
{
@@ -2048,6 +2053,31 @@ 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;
+ 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;
+}
+EXPORT_SYMBOL(pci_device_serial_number);
+
+/**
* 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 68678ed..f626006 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -202,6 +202,7 @@ int __pci_read_base(struct pci_dev *dev, enum pci_bar_type type,
struct resource *res, unsigned int reg);
int pci_resource_bar(struct pci_dev *dev, int resno, enum pci_bar_type *type);
void pci_configure_ari(struct pci_dev *dev);
+void pci_dsn_init(struct pci_dev *dev);
/**
* pci_ari_enabled - query ARI forwarding status
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 46ada5c..b37534f 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);
+ pci_dsn_init(dev);
+
/* Vital Product Data */
pci_vpd_pci22_init(dev);
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 0fd1f15..f2c6ed9 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 {
@@ -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] 9+ messages in thread* [PATCH -v3 2/3] PCI,pciehp: avoid add a device already exist before suspend during resume
2013-07-12 9:32 [PATCH -v3 0/3] Use PCIe DSN to improve pciehp_resume Yijing Wang
2013-07-12 9:32 ` [PATCH -v3 1/3] PCI: introduce PCIe Device Serial Number Capability support Yijing Wang
@ 2013-07-12 9:32 ` Yijing Wang
2013-07-12 9:32 ` [PATCH -v3 3/3] PCI,pciehp: use PCIe DSN to identify device change during suspend Yijing Wang
2 siblings, 0 replies; 9+ messages in thread
From: Yijing Wang @ 2013-07-12 9:32 UTC (permalink / raw)
To: Bjorn Helgaas
Cc: linux-kernel, Don Dutile, Paul Bolle, linux-pci, Rafael,
Hanjun Guo, jiang.liu, Yijing Wang, 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
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
^ permalink raw reply related [flat|nested] 9+ messages in thread* [PATCH -v3 3/3] PCI,pciehp: use PCIe DSN to identify device change during suspend
2013-07-12 9:32 [PATCH -v3 0/3] Use PCIe DSN to improve pciehp_resume Yijing Wang
2013-07-12 9:32 ` [PATCH -v3 1/3] PCI: introduce PCIe Device Serial Number Capability support Yijing Wang
2013-07-12 9:32 ` [PATCH -v3 2/3] PCI,pciehp: avoid add a device already exist before suspend during resume Yijing Wang
@ 2013-07-12 9:32 ` Yijing Wang
2013-07-26 0:17 ` Bjorn Helgaas
2 siblings, 1 reply; 9+ messages in thread
From: Yijing Wang @ 2013-07-12 9:32 UTC (permalink / raw)
To: Bjorn Helgaas
Cc: linux-kernel, Don Dutile, Paul Bolle, linux-pci, Rafael,
Hanjun Guo, jiang.liu, Yijing Wang, 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 7fe9dbd..bc47f8e 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] 9+ messages in thread* Re: [PATCH -v3 3/3] PCI,pciehp: use PCIe DSN to identify device change during suspend
2013-07-12 9:32 ` [PATCH -v3 3/3] PCI,pciehp: use PCIe DSN to identify device change during suspend Yijing Wang
@ 2013-07-26 0:17 ` Bjorn Helgaas
2013-07-26 8:25 ` Yijing Wang
2013-07-30 3:46 ` Yijing Wang
0 siblings, 2 replies; 9+ messages in thread
From: Bjorn Helgaas @ 2013-07-26 0:17 UTC (permalink / raw)
To: Yijing Wang
Cc: linux-kernel@vger.kernel.org, Don Dutile, Paul Bolle,
linux-pci@vger.kernel.org, Rafael, Hanjun Guo, Jiang Liu,
Oliver Neukum, Gu Zheng
On Fri, Jul 12, 2013 at 3:32 AM, Yijing Wang <wangyijing@huawei.com> 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 7fe9dbd..bc47f8e 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);
> + }
> + }
I'm not sure this is implemented at the correct place. The idea of
using the serial number to detect card swaps is not really specific to
pciehp. I know the Device Serial Number capability is defined in the
PCIe spec, but it doesn't *require* any PCIe functionality, and
there's no reason a similar capability couldn't be defined for
conventional PCI.
I don't know much about it, but conventional PCI *does* in fact
support the VPD capability, which can contain a serial number. I
wonder if we should enhance pci_device_serial_number() to look for a
VPD serial number if there's no PCIe DSN. Then we would want this
check for card swap in a more generic place, e.g., somewhere in
pci_scan_slot(), so all forms of hotplug would benefit from it.
Also, I think it's possible to use acpiphp for ExpressCard slots, and
this patch doesn't help acpiphp detect card swaps. I don't see any
mention of suspend/resume in acpiphp, so I don't know if it does
anything at all to detect card changes while suspended. Maybe Rafael
can shed some light?
I put the first two patches on a pci/yijing-dsn-v3 branch while we
work out the details of this one.
Bjorn
> } else if (!list_empty(&pbus->devices)) {
> pciehp_disable_slot(slot);
> }
> --
> 1.7.1
>
>
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH -v3 3/3] PCI,pciehp: use PCIe DSN to identify device change during suspend
2013-07-26 0:17 ` Bjorn Helgaas
@ 2013-07-26 8:25 ` Yijing Wang
2013-07-30 3:46 ` Yijing Wang
1 sibling, 0 replies; 9+ messages in thread
From: Yijing Wang @ 2013-07-26 8:25 UTC (permalink / raw)
To: Bjorn Helgaas
Cc: linux-kernel@vger.kernel.org, Don Dutile, Paul Bolle,
linux-pci@vger.kernel.org, Rafael, Hanjun Guo, Jiang Liu,
Oliver Neukum, Gu Zheng, Rafael J. Wysocki
Hi Bjorn,
Thanks for your review and comments!
>
> I'm not sure this is implemented at the correct place. The idea of
> using the serial number to detect card swaps is not really specific to
> pciehp. I know the Device Serial Number capability is defined in the
> PCIe spec, but it doesn't *require* any PCIe functionality, and
> there's no reason a similar capability couldn't be defined for
> conventional PCI.
Yes, maybe implement this interface in PCI core is better.
>
> I don't know much about it, but conventional PCI *does* in fact
> support the VPD capability, which can contain a serial number. I
> wonder if we should enhance pci_device_serial_number() to look for a
> VPD serial number if there's no PCIe DSN. Then we would want this
> check for card swap in a more generic place, e.g., somewhere in
> pci_scan_slot(), so all forms of hotplug would benefit from it.
This is a good idea, I will try to enhance pci_device_serial_number to
support legacy PCI device VPD capability. Then I will try to move this to
a more generic place.
>
> Also, I think it's possible to use acpiphp for ExpressCard slots, and
> this patch doesn't help acpiphp detect card swaps. I don't see any
> mention of suspend/resume in acpiphp, so I don't know if it does
> anything at all to detect card changes while suspended. Maybe Rafael
> can shed some light?
Acpiphp driver is not attached to a specific pci/pcie device, so i think
there is no point to call driver->suspend/resume interface.
Add cc Rafael J. Wysocki.
>
> I put the first two patches on a pci/yijing-dsn-v3 branch while we
> work out the details of this one.
Thanks!
>
> Bjorn
>
>> } else if (!list_empty(&pbus->devices)) {
>> pciehp_disable_slot(slot);
>> }
>> --
>> 1.7.1
>>
>>
>
> .
>
--
Thanks!
Yijing
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH -v3 3/3] PCI,pciehp: use PCIe DSN to identify device change during suspend
2013-07-26 0:17 ` Bjorn Helgaas
2013-07-26 8:25 ` Yijing Wang
@ 2013-07-30 3:46 ` Yijing Wang
2013-07-30 3:58 ` Bjorn Helgaas
1 sibling, 1 reply; 9+ messages in thread
From: Yijing Wang @ 2013-07-30 3:46 UTC (permalink / raw)
To: Bjorn Helgaas
Cc: linux-kernel@vger.kernel.org, Don Dutile, Paul Bolle,
linux-pci@vger.kernel.org, Rafael, Hanjun Guo, Jiang Liu,
Oliver Neukum, Gu Zheng
> I'm not sure this is implemented at the correct place. The idea of
> using the serial number to detect card swaps is not really specific to
> pciehp. I know the Device Serial Number capability is defined in the
> PCIe spec, but it doesn't *require* any PCIe functionality, and
> there's no reason a similar capability couldn't be defined for
> conventional PCI.
>
> I don't know much about it, but conventional PCI *does* in fact
> support the VPD capability, which can contain a serial number. I
> wonder if we should enhance pci_device_serial_number() to look for a
> VPD serial number if there's no PCIe DSN. Then we would want this
> check for card swap in a more generic place, e.g., somewhere in
> pci_scan_slot(), so all forms of hotplug would benefit from it.
Hi Bjorn,
I'm reworking this patch, but found some strange info about vpd serial number.
I have two x86 machines, they almost have the same hardware topology. But by lspci,
I found two different Broadcom BCM5709 NIC in different machine have the same vpd serial
number. If this is normal, vpd serial number seems to be useless for identify device change.
The first machine:
linux:/home/yijing # lspci -vvv -s 0000:01:00.0
01:00.0 Ethernet controller: Broadcom Corporation NetXtreme II BCM5709 Gigabit Ethernet (rev 20)
Subsystem: Broadcom Corporation NetXtreme II BCM5709 Gigabit Ethernet
Control: I/O- Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop- ParErr- Stepping- SERR- FastB2B- DisINTx-
Status: Cap+ 66MHz- UDF- FastB2B- ParErr- DEVSEL=fast >TAbort- <TAbort- <MAbort- >SERR- <PERR- INTx-
Latency: 0, Cache Line Size: 256 bytes
Interrupt: pin A routed to IRQ 28
Region 0: Memory at c0000000 (64-bit, non-prefetchable) [size=32M]
Capabilities: [48] Power Management version 3
Flags: PMEClk- DSI- D1- D2- AuxCurrent=0mA PME(D0+,D1-,D2-,D3hot+,D3cold+)
Status: D0 NoSoftRst+ PME-Enable- DSel=0 DScale=1 PME-
Capabilities: [50] Vital Product Data
Product Name: Broadcom NetXtreme II Ethernet Controller
Read-only fields:
[PN] Part number: BCM95706A0
[EC] Engineering changes: 220197-2
[SN] Serial number: 0123456789
[MN] Manufacture ID: 31 34 65 34
[RV] Reserved: checksum good, 31 byte(s) reserved
End
Capabilities: [58] MSI: Enable- Count=1/16 Maskable- 64bit+
Address: 0000000000000000 Data: 0000
..............[snip]...................
The second machine:
linux-suse-vmdq:~/pciutils-3.2.0 # ./lspci -vvv -s 01:00.0
01:00.0 Ethernet controller: Broadcom Corporation NetXtreme II BCM5709 Gigabit Ethernet (rev 20)
Subsystem: Broadcom Corporation NetXtreme II BCM5709 Gigabit Ethernet
Control: I/O- Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop- ParErr- Stepping- SERR- FastB2B- DisINTx-
Status: Cap+ 66MHz- UDF- FastB2B- ParErr- DEVSEL=fast >TAbort- <TAbort- <MAbort- >SERR- <PERR- INTx-
Latency: 0, Cache Line Size: 256 bytes
Interrupt: pin A routed to IRQ 28
Region 0: Memory at f6000000 (64-bit, non-prefetchable) [size=32M]
Capabilities: [48] Power Management version 3
Flags: PMEClk- DSI- D1- D2- AuxCurrent=0mA PME(D0+,D1-,D2-,D3hot+,D3cold+)
Status: D0 NoSoftRst+ PME-Enable- DSel=0 DScale=1 PME-
Capabilities: [50] Vital Product Data
Product Name: Broadcom NetXtreme II Ethernet Controller
Read-only fields:
[PN] Part number: BCM95706A0
[EC] Engineering changes: 220197-2
[SN] Serial number: 0123456789
[MN] Manufacture ID: 31 34 65 34
[RV] Reserved: checksum good, 31 byte(s) reserved
End
.......[snip]........
>
> Also, I think it's possible to use acpiphp for ExpressCard slots, and
> this patch doesn't help acpiphp detect card swaps. I don't see any
> mention of suspend/resume in acpiphp, so I don't know if it does
> anything at all to detect card changes while suspended. Maybe Rafael
> can shed some light?
>
> I put the first two patches on a pci/yijing-dsn-v3 branch while we
> work out the details of this one.
>
> Bjorn
>
>> } else if (!list_empty(&pbus->devices)) {
>> pciehp_disable_slot(slot);
>> }
>> --
>> 1.7.1
>>
>>
>
> .
>
--
Thanks!
Yijing
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH -v3 3/3] PCI,pciehp: use PCIe DSN to identify device change during suspend
2013-07-30 3:46 ` Yijing Wang
@ 2013-07-30 3:58 ` Bjorn Helgaas
2013-07-30 4:06 ` Yijing Wang
0 siblings, 1 reply; 9+ messages in thread
From: Bjorn Helgaas @ 2013-07-30 3:58 UTC (permalink / raw)
To: Yijing Wang
Cc: linux-kernel@vger.kernel.org, Don Dutile, Paul Bolle,
linux-pci@vger.kernel.org, Rafael, Hanjun Guo, Jiang Liu,
Oliver Neukum, Gu Zheng
On Mon, Jul 29, 2013 at 9:46 PM, Yijing Wang <wangyijing@huawei.com> wrote:
>> I'm not sure this is implemented at the correct place. The idea of
>> using the serial number to detect card swaps is not really specific to
>> pciehp. I know the Device Serial Number capability is defined in the
>> PCIe spec, but it doesn't *require* any PCIe functionality, and
>> there's no reason a similar capability couldn't be defined for
>> conventional PCI.
>>
>> I don't know much about it, but conventional PCI *does* in fact
>> support the VPD capability, which can contain a serial number. I
>> wonder if we should enhance pci_device_serial_number() to look for a
>> VPD serial number if there's no PCIe DSN. Then we would want this
>> check for card swap in a more generic place, e.g., somewhere in
>> pci_scan_slot(), so all forms of hotplug would benefit from it.
>
> Hi Bjorn,
> I'm reworking this patch, but found some strange info about vpd serial number.
> I have two x86 machines, they almost have the same hardware topology. But by lspci,
> I found two different Broadcom BCM5709 NIC in different machine have the same vpd serial
> number. If this is normal, vpd serial number seems to be useless for identify device change.
I wouldn't say it's completely useless. If the VPD serial number
changes, we can be pretty confident that the card has changed. It's
just that if the VPD serial number is unchanged, we might incorrectly
assume it's the same device.
But we currently *always* assume it's the same device, since we don't
look at serial numbers at all. If we can detect some changes, that
should be an improvement over what we have today even though it's not
perfect.
> The first machine:
> linux:/home/yijing # lspci -vvv -s 0000:01:00.0
> 01:00.0 Ethernet controller: Broadcom Corporation NetXtreme II BCM5709 Gigabit Ethernet (rev 20)
> Subsystem: Broadcom Corporation NetXtreme II BCM5709 Gigabit Ethernet
> Control: I/O- Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop- ParErr- Stepping- SERR- FastB2B- DisINTx-
> Status: Cap+ 66MHz- UDF- FastB2B- ParErr- DEVSEL=fast >TAbort- <TAbort- <MAbort- >SERR- <PERR- INTx-
> Latency: 0, Cache Line Size: 256 bytes
> Interrupt: pin A routed to IRQ 28
> Region 0: Memory at c0000000 (64-bit, non-prefetchable) [size=32M]
> Capabilities: [48] Power Management version 3
> Flags: PMEClk- DSI- D1- D2- AuxCurrent=0mA PME(D0+,D1-,D2-,D3hot+,D3cold+)
> Status: D0 NoSoftRst+ PME-Enable- DSel=0 DScale=1 PME-
> Capabilities: [50] Vital Product Data
> Product Name: Broadcom NetXtreme II Ethernet Controller
> Read-only fields:
> [PN] Part number: BCM95706A0
> [EC] Engineering changes: 220197-2
> [SN] Serial number: 0123456789
> [MN] Manufacture ID: 31 34 65 34
> [RV] Reserved: checksum good, 31 byte(s) reserved
> End
> Capabilities: [58] MSI: Enable- Count=1/16 Maskable- 64bit+
> Address: 0000000000000000 Data: 0000
> ..............[snip]...................
>
> The second machine:
> linux-suse-vmdq:~/pciutils-3.2.0 # ./lspci -vvv -s 01:00.0
> 01:00.0 Ethernet controller: Broadcom Corporation NetXtreme II BCM5709 Gigabit Ethernet (rev 20)
> Subsystem: Broadcom Corporation NetXtreme II BCM5709 Gigabit Ethernet
> Control: I/O- Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop- ParErr- Stepping- SERR- FastB2B- DisINTx-
> Status: Cap+ 66MHz- UDF- FastB2B- ParErr- DEVSEL=fast >TAbort- <TAbort- <MAbort- >SERR- <PERR- INTx-
> Latency: 0, Cache Line Size: 256 bytes
> Interrupt: pin A routed to IRQ 28
> Region 0: Memory at f6000000 (64-bit, non-prefetchable) [size=32M]
> Capabilities: [48] Power Management version 3
> Flags: PMEClk- DSI- D1- D2- AuxCurrent=0mA PME(D0+,D1-,D2-,D3hot+,D3cold+)
> Status: D0 NoSoftRst+ PME-Enable- DSel=0 DScale=1 PME-
> Capabilities: [50] Vital Product Data
> Product Name: Broadcom NetXtreme II Ethernet Controller
> Read-only fields:
> [PN] Part number: BCM95706A0
> [EC] Engineering changes: 220197-2
> [SN] Serial number: 0123456789
> [MN] Manufacture ID: 31 34 65 34
> [RV] Reserved: checksum good, 31 byte(s) reserved
> End
> .......[snip]........
>
>
>>
>> Also, I think it's possible to use acpiphp for ExpressCard slots, and
>> this patch doesn't help acpiphp detect card swaps. I don't see any
>> mention of suspend/resume in acpiphp, so I don't know if it does
>> anything at all to detect card changes while suspended. Maybe Rafael
>> can shed some light?
>>
>> I put the first two patches on a pci/yijing-dsn-v3 branch while we
>> work out the details of this one.
>>
>> Bjorn
>>
>>> } else if (!list_empty(&pbus->devices)) {
>>> pciehp_disable_slot(slot);
>>> }
>>> --
>>> 1.7.1
>>>
>>>
>>
>> .
>>
>
>
> --
> Thanks!
> Yijing
>
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH -v3 3/3] PCI,pciehp: use PCIe DSN to identify device change during suspend
2013-07-30 3:58 ` Bjorn Helgaas
@ 2013-07-30 4:06 ` Yijing Wang
0 siblings, 0 replies; 9+ messages in thread
From: Yijing Wang @ 2013-07-30 4:06 UTC (permalink / raw)
To: Bjorn Helgaas
Cc: linux-kernel@vger.kernel.org, Don Dutile, Paul Bolle,
linux-pci@vger.kernel.org, Rafael, Hanjun Guo, Jiang Liu,
Oliver Neukum, Gu Zheng
>>
>> Hi Bjorn,
>> I'm reworking this patch, but found some strange info about vpd serial number.
>> I have two x86 machines, they almost have the same hardware topology. But by lspci,
>> I found two different Broadcom BCM5709 NIC in different machine have the same vpd serial
>> number. If this is normal, vpd serial number seems to be useless for identify device change.
>
> I wouldn't say it's completely useless. If the VPD serial number
> changes, we can be pretty confident that the card has changed. It's
> just that if the VPD serial number is unchanged, we might incorrectly
> assume it's the same device.
>
> But we currently *always* assume it's the same device, since we don't
> look at serial numbers at all. If we can detect some changes, that
> should be an improvement over what we have today even though it's not
> perfect.
Hmmm, Yes, different VPD serial number must comes from different device.
Although the same serial number maybe not the same device.
I will send out the new version patchset soon.
Thanks!
Yijing.
>
>> The first machine:
>> linux:/home/yijing # lspci -vvv -s 0000:01:00.0
>> 01:00.0 Ethernet controller: Broadcom Corporation NetXtreme II BCM5709 Gigabit Ethernet (rev 20)
>> Subsystem: Broadcom Corporation NetXtreme II BCM5709 Gigabit Ethernet
>> Control: I/O- Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop- ParErr- Stepping- SERR- FastB2B- DisINTx-
>> Status: Cap+ 66MHz- UDF- FastB2B- ParErr- DEVSEL=fast >TAbort- <TAbort- <MAbort- >SERR- <PERR- INTx-
>> Latency: 0, Cache Line Size: 256 bytes
>> Interrupt: pin A routed to IRQ 28
>> Region 0: Memory at c0000000 (64-bit, non-prefetchable) [size=32M]
>> Capabilities: [48] Power Management version 3
>> Flags: PMEClk- DSI- D1- D2- AuxCurrent=0mA PME(D0+,D1-,D2-,D3hot+,D3cold+)
>> Status: D0 NoSoftRst+ PME-Enable- DSel=0 DScale=1 PME-
>> Capabilities: [50] Vital Product Data
>> Product Name: Broadcom NetXtreme II Ethernet Controller
>> Read-only fields:
>> [PN] Part number: BCM95706A0
>> [EC] Engineering changes: 220197-2
>> [SN] Serial number: 0123456789
>> [MN] Manufacture ID: 31 34 65 34
>> [RV] Reserved: checksum good, 31 byte(s) reserved
>> End
>> Capabilities: [58] MSI: Enable- Count=1/16 Maskable- 64bit+
>> Address: 0000000000000000 Data: 0000
>> ..............[snip]...................
>>
>> The second machine:
>> linux-suse-vmdq:~/pciutils-3.2.0 # ./lspci -vvv -s 01:00.0
>> 01:00.0 Ethernet controller: Broadcom Corporation NetXtreme II BCM5709 Gigabit Ethernet (rev 20)
>> Subsystem: Broadcom Corporation NetXtreme II BCM5709 Gigabit Ethernet
>> Control: I/O- Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop- ParErr- Stepping- SERR- FastB2B- DisINTx-
>> Status: Cap+ 66MHz- UDF- FastB2B- ParErr- DEVSEL=fast >TAbort- <TAbort- <MAbort- >SERR- <PERR- INTx-
>> Latency: 0, Cache Line Size: 256 bytes
>> Interrupt: pin A routed to IRQ 28
>> Region 0: Memory at f6000000 (64-bit, non-prefetchable) [size=32M]
>> Capabilities: [48] Power Management version 3
>> Flags: PMEClk- DSI- D1- D2- AuxCurrent=0mA PME(D0+,D1-,D2-,D3hot+,D3cold+)
>> Status: D0 NoSoftRst+ PME-Enable- DSel=0 DScale=1 PME-
>> Capabilities: [50] Vital Product Data
>> Product Name: Broadcom NetXtreme II Ethernet Controller
>> Read-only fields:
>> [PN] Part number: BCM95706A0
>> [EC] Engineering changes: 220197-2
>> [SN] Serial number: 0123456789
>> [MN] Manufacture ID: 31 34 65 34
>> [RV] Reserved: checksum good, 31 byte(s) reserved
>> End
>> .......[snip]........
>>
>>
>>>
>>> Also, I think it's possible to use acpiphp for ExpressCard slots, and
>>> this patch doesn't help acpiphp detect card swaps. I don't see any
>>> mention of suspend/resume in acpiphp, so I don't know if it does
>>> anything at all to detect card changes while suspended. Maybe Rafael
>>> can shed some light?
>>>
>>> I put the first two patches on a pci/yijing-dsn-v3 branch while we
>>> work out the details of this one.
>>>
>>> Bjorn
>>>
>>>> } else if (!list_empty(&pbus->devices)) {
>>>> pciehp_disable_slot(slot);
>>>> }
>>>> --
>>>> 1.7.1
>>>>
>>>>
>>>
>>> .
>>>
>>
>>
>> --
>> Thanks!
>> Yijing
>>
>
> .
>
--
Thanks!
Yijing
^ permalink raw reply [flat|nested] 9+ messages in thread