This series is based on Bjorn's pci-next branch.
Currently, more and more PCIe devices support PCIe DSN(Device Serial Number)
capability, we can use this cap to identify device. In some platform,
when we hotplug PCIe device, no interrupts will be received in system.
So we can not identify the device whether is changed during suspend.
PCIe DSN can help to identify this.
Legacy PCI device can achieve it by PCI Vital Product Data capability.
We can use its SN keyword to report the unique number.
But in my platform, PCI devices which support VPD SN cap report the
meaningless same string "0123456789".
Rework PCI VPD code to support device identification is not an easy work.
Plan to do it in part2.
Yijing Wang (7):
PCI: rework pci_find_next_ext_capability()
PCI: introduce pci_bus_find_ext_capability()
PCI: Add support for Device Serial Number capability
PCI: Introduce pci_serial_number_changed()
PCI: Add pci_dummy_ops to isolate pci device temporarily
PCI: Check pci device serial number when scan device
PCI: pciehp: Don't enable/disable slot on resume unless status
changed
drivers/pci/hotplug/pciehp_core.c | 10 ++-
drivers/pci/pci.c | 160 +++++++++++++++++++++++++++++++++++--
drivers/pci/pci.h | 2 +-
drivers/pci/probe.c | 15 +++-
include/linux/pci.h | 9 ++-
5 files changed, 182 insertions(+), 14 deletions(-)
Sometimes we need to find PCI EXT CAP before
pci_dev set up. So introduce pci_bus_find_ext_capability(),
it will be used to get PCI EXT DSN before pci_dev set up.
Signed-off-by: Yijing Wang <[email protected]>
---
drivers/pci/pci.c | 31 +++++++++++++++++++++++++++++++
include/linux/pci.h | 1 +
2 files changed, 32 insertions(+), 0 deletions(-)
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index a263c0a..432ac86 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -327,6 +327,37 @@ int pci_find_ext_capability(struct pci_dev *dev, int cap)
}
EXPORT_SYMBOL_GPL(pci_find_ext_capability);
+/**
+ * pci_bus_find_ext_capability - Find an extended capability
+ * @bus: the PCI bus to query
+ * @devfn: PCI device to query
+ * @cap: capability code
+ *
+ * Like pci_find_ext_capability() but works for pci devices that do not have a
+ * pci_dev structure set up yet.
+ *
+ * Returns the address of the requested capability structure within the
+ * device's PCI configuration space or 0 in case the device does not
+ * support it.
+ *
+ * */
+int pci_bus_find_ext_capability(struct pci_bus *bus, int devfn, int cap)
+{
+ int pos;
+ u32 status;
+
+ pos = pci_bus_find_capability(bus, devfn, PCI_CAP_ID_EXP);
+ if (!pos)
+ return 0;
+
+ if (pci_bus_read_config_dword(bus, devfn, PCI_CFG_SPACE_SIZE, &status) !=
+ PCIBIOS_SUCCESSFUL || status == 0xffffffff)
+ return 0;
+
+ return pci_find_next_ext_capability(bus, devfn, 0, cap);
+}
+
+
static int __pci_find_next_ht_cap(struct pci_dev *dev, int pos, int ht_cap)
{
int rc, ttl = PCI_FIND_CAP_TTL;
diff --git a/include/linux/pci.h b/include/linux/pci.h
index df495e9..470de02 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -797,6 +797,7 @@ int pci_find_capability(struct pci_dev *dev, int cap);
int pci_find_next_capability(struct pci_dev *dev, u8 pos, int cap);
int pci_find_ext_capability(struct pci_dev *dev, int cap);
int pci_find_next_ext_capability(struct pci_bus *bus, int devfn, int pos, int cap);
+int pci_bus_find_ext_capability(struct pci_bus *bus, int devfn, int cap);
int pci_find_ht_capability(struct pci_dev *dev, int ht_cap);
int pci_find_next_ht_capability(struct pci_dev *dev, int pos, int ht_cap);
struct pci_bus *pci_find_next_bus(const struct pci_bus *from);
--
1.7.1
Add support for the Device Serial Number capability, so we can use the
unique device serial number to identify the physical device. This helps
determine whether a device was replaced while the system was suspended.
[bhelgaas: changelog, drop pci_dsn_init(), spell out "serial_number"]
Reviewed-by: Gu Zheng <[email protected]>
Signed-off-by: Yijing Wang <[email protected]>
Signed-off-by: Bjorn Helgaas <[email protected]>
Cc: Paul Bolle <[email protected]>
Cc: "Rafael J. Wysocki" <[email protected]>
Cc: Oliver Neukum <[email protected]>
Cc: Gu Zheng <[email protected]>
---
drivers/pci/pci.c | 30 ++++++++++++++++++++++++++++++
drivers/pci/pci.h | 2 +-
drivers/pci/probe.c | 2 ++
include/linux/pci.h | 1 +
4 files changed, 34 insertions(+), 1 deletions(-)
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 432ac86..af06064 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -2168,6 +2168,36 @@ void pci_free_cap_save_buffers(struct pci_dev *dev)
}
/**
+ * pci_device_serial_number - get device serial number
+ * @bus: PCI bus the device on
+ * @devfn: the PCI device
+ *
+ * return the device serial number if device support,
+ * otherwise return 0.
+ */
+static u64 pci_device_serial_number(struct pci_bus *bus, int devfn)
+{
+ int pos;
+ u32 lo, hi;
+
+ if (!pci_bus_find_capability(bus, devfn, PCI_CAP_ID_EXP))
+ return 0;
+
+ pos = pci_bus_find_ext_capability(bus, devfn, PCI_EXT_CAP_ID_DSN);
+ if (!pos)
+ return 0;
+
+ pci_bus_read_config_dword(bus, devfn, pos + 4, &lo);
+ pci_bus_read_config_dword(bus, devfn, pos + 8, &hi);
+ return ((u64)hi << 32) | lo;
+}
+
+void pci_dsn_init(struct pci_dev *dev)
+{
+ dev->sn = pci_device_serial_number(dev->bus, dev->devfn);
+}
+
+/**
* 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 4df38df..685301a 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -208,7 +208,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 6e34498..27d3e6f 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -1324,6 +1324,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 470de02..3631859 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -354,6 +354,7 @@ struct pci_dev {
struct list_head msi_list;
const struct attribute_group **msi_irq_groups;
#endif
+ u64 sn; /* device serial number, 0 if not support */
struct pci_vpd *vpd;
#ifdef CONFIG_PCI_ATS
union {
--
1.7.1
Currently pciehp_resume() always enables the slot if it is occupied. But
often the slot was already occupied before the suspend, so we complain 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
This patch only enables the slot if it was empty before the suspend and is
now occupied, i.e., a card was inserted while suspended.
Similarly, we only disable the slot if a card was removed while suspended.
If it was already empty before the suspend, we don't need to do anything.
[bhelgaas: changelog]
Tested-by: Paul Bolle <[email protected]>
Signed-off-by: Yijing Wang <[email protected]>
Signed-off-by: Bjorn Helgaas <[email protected]>
Cc: "Rafael J. Wysocki" <[email protected]>
Cc: Oliver Neukum <[email protected]>
Cc: Gu Zheng <[email protected]>
---
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 53b58de..551137f 100644
--- a/drivers/pci/hotplug/pciehp_core.c
+++ b/drivers/pci/hotplug/pciehp_core.c
@@ -317,6 +317,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);
@@ -328,10 +329,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
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 <[email protected]>
Cc: Paul Bolle <[email protected]>
Cc: "Rafael J. Wysocki" <[email protected]>
Cc: Oliver Neukum <[email protected]>
Cc: Gu Zheng <[email protected]>
Cc: [email protected]
---
drivers/pci/pci.c | 28 ++++++++++++++++++++++++++++
include/linux/pci.h | 2 ++
2 files changed, 30 insertions(+), 0 deletions(-)
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index af06064..8f31ab3 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -2198,6 +2198,34 @@ 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)
+{
+ u64 old, new;
+ old = pdev->sn;
+
+ if (!pci_is_pcie(pdev))
+ return false;
+
+ new = pci_device_serial_number(pdev->bus,
+ pdev->devfn);
+
+ if (old != new) {
+ pr_info("%s: Device Serial Number Changed!\n",
+ pci_name(pdev));
+ return true;
+ } else
+ return false;
+}
+EXPORT_SYMBOL(pci_serial_number_changed);
+
+/**
* pci_configure_ari - enable or disable ARI forwarding
* @dev: the PCI device
*
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 3631859..d60c0b6 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -1025,6 +1025,8 @@ void pci_unlock_rescan_remove(void);
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);
+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
Pci_dummy_ops does nothing when we use it to read/write
pci_device. So we can isolate pci device by replace its
bus pci_ops by pci_dummy_ops. This is preparation for
the later patch.
Signed-off-by: Yijing Wang <[email protected]>
---
drivers/pci/pci.c | 54 +++++++++++++++++++++++++++++++++++++++++++++++++++
include/linux/pci.h | 3 ++
2 files changed, 57 insertions(+), 0 deletions(-)
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 8f31ab3..bbef95e 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -2225,6 +2225,60 @@ bool pci_serial_number_changed(struct pci_dev *pdev)
}
EXPORT_SYMBOL(pci_serial_number_changed);
+static int pci_dummy_read(struct pci_bus *bus, unsigned int devfn,
+ int where, int size, u32 *val)
+{
+ return PCIBIOS_DEVICE_NOT_FOUND;
+}
+
+static int pci_dummy_write(struct pci_bus *bus, unsigned int devfn,
+ int reg, int size, u32 val)
+{
+ return PCIBIOS_DEVICE_NOT_FOUND;
+}
+
+static struct pci_ops pci_dummy_ops = {
+ .read = pci_dummy_read,
+ .write = pci_dummy_write,
+};
+
+static DEFINE_SPINLOCK(pci_freeze_lock);
+/**
+ * pci_bus_freeze_device - freeze pci bus to access pci device
+ * @bus: the pci bus to freeze
+ *
+ * Replace pci bus ops by pci_dummy_ops, protect system from
+ * accessing pci devices.
+ */
+void pci_bus_freeze_device(struct pci_bus *bus)
+{
+ struct pci_ops *ops;
+ unsigned long flags;
+
+ spin_lock_irqsave(&pci_freeze_lock, flags);
+ ops = pci_bus_set_ops(bus, &pci_dummy_ops);
+ bus->save_ops = ops;
+ spin_unlock_irqrestore(&pci_freeze_lock, flags);
+}
+
+/**
+ * pci_bus_unfreeze_device - unfreeze pci bus to acess pci devices
+ * @bus: the pci bus to unfreeze
+ *
+ * Restore pci bus original ops, so pci bus can access
+ * pci devices normally.
+ */
+void pci_bus_unfreeze_device(struct pci_bus *bus)
+{
+ unsigned long flags;
+
+ BUG_ON(!bus->save_ops);
+ spin_lock_irqsave(&pci_freeze_lock, flags);
+ pci_bus_set_ops(bus, bus->save_ops);
+ bus->save_ops = NULL;
+ spin_unlock_irqrestore(&pci_freeze_lock, flags);
+}
+
/**
* pci_configure_ari - enable or disable ARI forwarding
* @dev: the PCI device
diff --git a/include/linux/pci.h b/include/linux/pci.h
index d60c0b6..74dd968 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -442,6 +442,7 @@ struct pci_bus {
struct resource busn_res; /* bus numbers routed to this bus */
struct pci_ops *ops; /* configuration access functions */
+ struct pci_ops *save_ops; /* save configuration access functions */
struct msi_chip *msi; /* MSI controller */
void *sysdata; /* hook for sys-specific extension */
struct proc_dir_entry *procdir; /* directory entry in /proc/bus/pci */
@@ -1026,6 +1027,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);
bool pci_serial_number_changed(struct pci_dev *pdev);
+void pci_bus_freeze_device(struct pci_bus *bus);
+void pci_bus_unfreeze_device(struct pci_bus *bus);
/* 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);
--
1.7.1
Sometimes pci device will be removed and reinsert
while suspended. In this case system can not identify
the device has been changed. Now PCIe support Device
Serial Number Capability which has the unique number.
So make system check pci device DSN during scanning
device if support.
Signed-off-by: Yijing Wang <[email protected]>
---
drivers/pci/probe.c | 13 ++++++++++++-
1 files changed, 12 insertions(+), 1 deletions(-)
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 27d3e6f..370b25c 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -1383,11 +1383,22 @@ void pci_device_add(struct pci_dev *dev, struct pci_bus *bus)
struct pci_dev *__ref pci_scan_single_device(struct pci_bus *bus, int devfn)
{
struct pci_dev *dev;
+ int rescan = 0;
dev = pci_get_slot(bus, devfn);
if (dev) {
+ /* only check func 0 device */
+ if (PCI_FUNC(devfn) == 0) {
+ if (pci_serial_number_changed(dev)) {
+ pci_bus_freeze_device(bus);
+ pci_stop_and_remove_bus_device(dev);
+ pci_bus_unfreeze_device(bus);
+ rescan = 1;
+ }
+ }
pci_dev_put(dev);
- return dev;
+ if (!rescan)
+ return dev;
}
dev = pci_scan_device(bus, devfn);
--
1.7.1
Rework pci_find_next_ext_capability(), use
pci_bus_read_config_xxx() instead of
pci_read_config_xxx(). So we can use this
function before pci_dev setup.
Signed-off-by: Yijing Wang <[email protected]>
---
drivers/pci/pci.c | 17 +++++++++--------
include/linux/pci.h | 2 +-
2 files changed, 10 insertions(+), 9 deletions(-)
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 1febe90..a263c0a 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -256,7 +256,8 @@ int pci_bus_find_capability(struct pci_bus *bus, unsigned int devfn, int cap)
/**
* pci_find_next_ext_capability - Find an extended capability
- * @dev: PCI device to query
+ * @bus: the PCI bus to query
+ * @devfn: PCI device to query
* @start: address at which to start looking (0 to start at beginning of list)
* @cap: capability code
*
@@ -265,7 +266,7 @@ int pci_bus_find_capability(struct pci_bus *bus, unsigned int devfn, int cap)
* not support it. Some capabilities can occur several times, e.g., the
* vendor-specific capability, and this provides a way to find them all.
*/
-int pci_find_next_ext_capability(struct pci_dev *dev, int start, int cap)
+int pci_find_next_ext_capability(struct pci_bus *bus, int devfn, int start, int cap)
{
u32 header;
int ttl;
@@ -274,13 +275,10 @@ int pci_find_next_ext_capability(struct pci_dev *dev, int start, int cap)
/* minimum 8 bytes per capability */
ttl = (PCI_CFG_SPACE_EXP_SIZE - PCI_CFG_SPACE_SIZE) / 8;
- if (dev->cfg_size <= PCI_CFG_SPACE_SIZE)
- return 0;
-
if (start)
pos = start;
- if (pci_read_config_dword(dev, pos, &header) != PCIBIOS_SUCCESSFUL)
+ if (pci_bus_read_config_dword(bus, devfn, pos, &header) != PCIBIOS_SUCCESSFUL)
return 0;
/*
@@ -298,7 +296,7 @@ int pci_find_next_ext_capability(struct pci_dev *dev, int start, int cap)
if (pos < PCI_CFG_SPACE_SIZE)
break;
- if (pci_read_config_dword(dev, pos, &header) != PCIBIOS_SUCCESSFUL)
+ if (pci_bus_read_config_dword(bus, devfn, pos, &header) != PCIBIOS_SUCCESSFUL)
break;
}
@@ -322,7 +320,10 @@ EXPORT_SYMBOL_GPL(pci_find_next_ext_capability);
*/
int pci_find_ext_capability(struct pci_dev *dev, int cap)
{
- return pci_find_next_ext_capability(dev, 0, cap);
+ if (dev->cfg_size <= PCI_CFG_SPACE_SIZE)
+ return 0;
+
+ return pci_find_next_ext_capability(dev->bus, dev->devfn, 0, cap);
}
EXPORT_SYMBOL_GPL(pci_find_ext_capability);
diff --git a/include/linux/pci.h b/include/linux/pci.h
index fb57c89..df495e9 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -796,7 +796,7 @@ enum pci_lost_interrupt_reason pci_lost_interrupt(struct pci_dev *dev);
int pci_find_capability(struct pci_dev *dev, int cap);
int pci_find_next_capability(struct pci_dev *dev, u8 pos, int cap);
int pci_find_ext_capability(struct pci_dev *dev, int cap);
-int pci_find_next_ext_capability(struct pci_dev *dev, int pos, int cap);
+int pci_find_next_ext_capability(struct pci_bus *bus, int devfn, int pos, int cap);
int pci_find_ht_capability(struct pci_dev *dev, int ht_cap);
int pci_find_next_ht_capability(struct pci_dev *dev, int pos, int ht_cap);
struct pci_bus *pci_find_next_bus(const struct pci_bus *from);
--
1.7.1
On Mon, 2014-02-10 at 12:04 +0800, Yijing Wang wrote:
> +static DEFINE_SPINLOCK(pci_freeze_lock);
The lock is used only here.
> +/**
> + * pci_bus_freeze_device - freeze pci bus to access pci device
> + * @bus: the pci bus to freeze
> + *
> + * Replace pci bus ops by pci_dummy_ops, protect system from
> + * accessing pci devices.
> + */
> +void pci_bus_freeze_device(struct pci_bus *bus)
> +{
> + struct pci_ops *ops;
> + unsigned long flags;
> +
> + spin_lock_irqsave(&pci_freeze_lock, flags);
> + ops = pci_bus_set_ops(bus, &pci_dummy_ops);
> + bus->save_ops = ops;
> + spin_unlock_irqrestore(&pci_freeze_lock, flags);
Against what exactly are you locking here?
Regards
Oliver
Hi Oliver,
Thanks for your review and comments!
>> +static DEFINE_SPINLOCK(pci_freeze_lock);
>
> The lock is used only here.
Also be used in pci_bus_unfreeze_device();
>
>> +/**
>> + * pci_bus_freeze_device - freeze pci bus to access pci device
>> + * @bus: the pci bus to freeze
>> + *
>> + * Replace pci bus ops by pci_dummy_ops, protect system from
>> + * accessing pci devices.
>> + */
>> +void pci_bus_freeze_device(struct pci_bus *bus)
>> +{
>> + struct pci_ops *ops;
>> + unsigned long flags;
>> +
>> + spin_lock_irqsave(&pci_freeze_lock, flags);
>> + ops = pci_bus_set_ops(bus, &pci_dummy_ops);
>> + bus->save_ops = ops;
>> + spin_unlock_irqrestore(&pci_freeze_lock, flags);
>
> Against what exactly are you locking here?
I want to use this spin lock to serialize freeze device and unfreeze device.
Thanks!
Yijing.
>
>
>
>
--
Thanks!
Yijing
On Mon, 2014-02-10 at 15:59 +0800, Yijing Wang wrote:
> Hi Oliver,
> Thanks for your review and comments!
>
> >> +static DEFINE_SPINLOCK(pci_freeze_lock);
> >
> > The lock is used only here.
>
> Also be used in pci_bus_unfreeze_device();
Sorry, I meant only in this patch.
>
> >
> >> +/**
> >> + * pci_bus_freeze_device - freeze pci bus to access pci device
> >> + * @bus: the pci bus to freeze
> >> + *
> >> + * Replace pci bus ops by pci_dummy_ops, protect system from
> >> + * accessing pci devices.
> >> + */
> >> +void pci_bus_freeze_device(struct pci_bus *bus)
> >> +{
> >> + struct pci_ops *ops;
> >> + unsigned long flags;
> >> +
> >> + spin_lock_irqsave(&pci_freeze_lock, flags);
> >> + ops = pci_bus_set_ops(bus, &pci_dummy_ops);
> >> + bus->save_ops = ops;
> >> + spin_unlock_irqrestore(&pci_freeze_lock, flags);
> >
> > Against what exactly are you locking here?
>
> I want to use this spin lock to serialize freeze device and unfreeze device.
Yes, but against what? I am sorry I should have been more explicit.
You are using these functions only in pci_scan_single_device()
CPU A CPU B
pci_bus_freeze_device() wait
bus->save_ops = ops {valid} wait
... pci_bus_freeze_device()
wait bus->save_ops = ops
{pci_dummy_ops !}
pci_bus_unfreeze_device() wait
pci_bus_set_ops(bus, bus->save_ops)
You see the problem?
If this function ever races with itself, the locking is useless.
If it doesn't race with itself, the locking is not needed.
If this function can really race with itself, you need a refcount
for freezing.
Regards
Oliver
On Mon, 2014-02-10 at 12:04 +0800, Yijing Wang wrote:
> +static u64 pci_device_serial_number(struct pci_bus *bus, int devfn)
> +{
> + int pos;
> + u32 lo, hi;
> +
> + if (!pci_bus_find_capability(bus, devfn, PCI_CAP_ID_EXP))
> + return 0;
> +
> + pos = pci_bus_find_ext_capability(bus, devfn,
> PCI_EXT_CAP_ID_DSN);
> + if (!pos)
> + return 0;
> +
> + pci_bus_read_config_dword(bus, devfn, pos + 4, &lo);
> + pci_bus_read_config_dword(bus, devfn, pos + 8, &hi);
We have no macro for that?
> + return ((u64)hi << 32) | lo;
> +}
Regards
Oliver
>>>> + spin_lock_irqsave(&pci_freeze_lock, flags);
>>>> + ops = pci_bus_set_ops(bus, &pci_dummy_ops);
>>>> + bus->save_ops = ops;
>>>> + spin_unlock_irqrestore(&pci_freeze_lock, flags);
>>>
>>> Against what exactly are you locking here?
>>
>> I want to use this spin lock to serialize freeze device and unfreeze device.
>
> Yes, but against what? I am sorry I should have been more explicit.
> You are using these functions only in pci_scan_single_device()
Hi Oliver, thanks very much for your detailed analysis. My original intention to use
pci_freeze_lock to serialize pci_bus_freeze_device() and pci_bus_unfreeze_device(),
because I think these two functions maybe used in other places, although currently
only used in pci_scan_single_device().
Like:
CPU A CPU B
pci_bus_freeze_device() pci_bus_unfreeze_device()
pci_bus_set_ops(bus, &pci_dummy_ops);
pci_bus_set_ops(bus, bus->save_ops); ---> here, save_ops is NULL, it's bad.
bus->save_ops = ops;
>
>
> CPU A CPU B
> pci_bus_freeze_device() wait
> bus->save_ops = ops {valid} wait
> ... pci_bus_freeze_device()
> wait bus->save_ops = ops
> {pci_dummy_ops !}
> pci_bus_unfreeze_device() wait
> pci_bus_set_ops(bus, bus->save_ops)
>
> You see the problem?
>
Yes, this is a issue, good catch. I should add a refcount to avoid this situation.
> If this function ever races with itself, the locking is useless.
> If it doesn't race with itself, the locking is not needed.
> If this function can really race with itself, you need a refcount
> for freezing.
Thanks again!
>
>
>
> .
>
--
Thanks!
Yijing
On 2014/2/10 18:21, Oliver Neukum wrote:
>
> On Mon, 2014-02-10 at 12:04 +0800, Yijing Wang wrote:
>> +static u64 pci_device_serial_number(struct pci_bus *bus, int devfn)
>> +{
>> + int pos;
>> + u32 lo, hi;
>> +
>> + if (!pci_bus_find_capability(bus, devfn, PCI_CAP_ID_EXP))
>> + return 0;
>> +
>> + pos = pci_bus_find_ext_capability(bus, devfn,
>> PCI_EXT_CAP_ID_DSN);
>> + if (!pos)
>> + return 0;
>> +
>> + pci_bus_read_config_dword(bus, devfn, pos + 4, &lo);
>> + pci_bus_read_config_dword(bus, devfn, pos + 8, &hi);
>
> We have no macro for that?
Yes, I will try to define macros for them, thanks!
>
>> + return ((u64)hi << 32) | lo;
>> +}
>
> Regards
> Oliver
>
>
>
>
> .
>
--
Thanks!
Yijing