Hi Tony,
Can you pick up these bug fix patches into your ras tree? I sent them to PCI subsystem maillist, but Bjorn does not seem
very interested in these aer_inject tool bug fix patches. Huang Ying <[email protected]> is the author of aer_inject tool.
And I had a long discussion with Huang Ying about these patches, and got his reviewed-by. I do not know what people are really
interested in these patches, So I try to send them to you. Thank you very much!
This series of patch mainly to fix the aer_inject bug described as below:
-+-[0000:40]-+-00.0-[0000:41]--
| +-01.0-[0000:42]--+-00.0 Intel Corporation 82576 Gigabit Network Connection
| | \-00.1 Intel Corporation 82576 Gigabit Network Connection
| +-03.0-[0000:43]----00.0 LSI Logic / Symbios Logic SAS1064ET PCI-Express Fusion-MPT SAS
| +-04.0-[0000:44]--
| +-05.0-[0000:45]--
| +-07.0-[0000:46-49]----00.0-[0000:47-49]--+-02.0-[0000:48]--+-00.0 Intel Corporation 82576 Gigabit Network Connection
| | | \-00.1 Intel Corporation 82576 Gigabit Network Connection
| | \-04.0-[0000:49]--+-00.0 Intel Corporation 82576 Gigabit Network Connection
| | \-00.1 Intel Corporation 82576 Gigabit Network Connection
my steps:
1)modprobe aer_inject
2)inject aer errors to pcie device 0000:48:00.0
3)modprobe pciehp
4)hot remove Network Card in slot(port 0000:40:07.0)
5)hot add Network Card in slot(port 0000:40:07.0)
6)system panic
in step 2) the pci_ops of bus 0000:48 and bus 0000:40 will be assigned to pci_ops_aer
in step 5) the pci_ops of the newly created bus 0000:46 will be assigned to pci_ops_aer(inherited by parent pci_ops),
but this pci_ops(0000:46) is not tracked in pci_bus_ops_list in aer_inject module. So every access to pci_config space
by pci_ops of 0000:46 will cause system panic, Since pci_ops_aer cannot find its original pci_ops, thus , a NULL pci_ops return;
The first patch fix this bug by finding parent pci_ops(tracked in pci_ops_list) instead of returning NULL in step 5);
The second patch fix a small race condition window in aer_inject_exit;
The Third patch to find and clean all untracked pci_ops_aer in system when aer_inject module exit
The rest two patch mainly about to clean bus_ops;
Yijing Wang (5):
PCI/AER: Fix pci_ops return NULL in pci_read/write_aer
PCI/AER: use list_for_each_entry to avoid a small race condition
window
PCI/AER: clean all untracked pci_ops_aer when rmmod aer_inject
PCI/AER: clean pci_bus_ops when related pci bus was removed
PCI/AER: free pci_bus_ops_list and remove pci_bus_ops_pop
drivers/pci/pcie/aer/aer_inject.c | 123 +++++++++++++++++++++++++++++++------
1 files changed, 103 insertions(+), 20 deletions(-)
When Inject aer errors to the target pci device, a pci_bus_ops will
be allocated for the pci device's pci bus.When the pci bus was removed,
we should also release the pci_bus_ops.
Signed-off-by: Yijing Wang <[email protected]>
Reviewed-by: Sven Dietrich <[email protected]>
---
drivers/pci/pcie/aer/aer_inject.c | 49 ++++++++++++++++++++++++++++++++++++-
1 files changed, 48 insertions(+), 1 deletions(-)
diff --git a/drivers/pci/pcie/aer/aer_inject.c b/drivers/pci/pcie/aer/aer_inject.c
index 4777c44..c5372dd 100644
--- a/drivers/pci/pcie/aer/aer_inject.c
+++ b/drivers/pci/pcie/aer/aer_inject.c
@@ -553,9 +553,55 @@ static struct miscdevice aer_inject_device = {
.fops = &aer_inject_fops,
};
+static void aer_clean_pci_bus_ops(struct pci_dev *dev)
+{
+ unsigned long flags;
+ struct pci_bus_ops *bus_ops, *tmp_ops;
+ struct pci_bus *bus;
+ bus = dev->subordinate;
+ if (!bus)
+ return;
+
+ spin_lock_irqsave(&inject_lock, flags);
+ list_for_each_entry_safe(bus_ops, tmp_ops, &pci_bus_ops_list, list)
+ if (bus_ops->bus == bus) {
+ list_del(&bus_ops->list);
+ kfree(bus_ops);
+ break;
+ }
+ spin_unlock_irqrestore(&inject_lock, flags);
+}
+
+static int aer_hp_notify_fn(struct notifier_block *nb,
+ unsigned long event, void *data)
+{
+ switch (event) {
+ case BUS_NOTIFY_DEL_DEVICE:
+ aer_clean_pci_bus_ops(to_pci_dev(data));
+ break;
+ default:
+ return NOTIFY_DONE;
+ }
+
+ return NOTIFY_OK;
+}
+
+static struct notifier_block aerinj_hp_notifier = {
+ .notifier_call = &aer_hp_notify_fn,
+};
+
static int __init aer_inject_init(void)
{
- return misc_register(&aer_inject_device);
+ int ret;
+ ret = misc_register(&aer_inject_device);
+ if (ret)
+ goto out;
+
+ ret = bus_register_notifier(&pci_bus_type, &aerinj_hp_notifier);
+ if (ret)
+ misc_deregister(&aer_inject_device);
+out:
+ return ret;
}
static void __exit aer_inject_exit(void)
@@ -564,6 +610,7 @@ static void __exit aer_inject_exit(void)
unsigned long flags;
struct pci_bus_ops *bus_ops;
+ bus_unregister_notifier(&pci_bus_type, &aerinj_hp_notifier);
misc_deregister(&aer_inject_device);
list_for_each_entry(bus_ops, &pci_bus_ops_list, list)
--
1.7.1
When we rmmod aer_inject module, there is a small race condition window between pci_bus_ops_pop()
and pci_bus_set_ops() in aer_inject_exit, eg. pci_read_aer/pci_write_aer was called between
them. So keep pci_bus_ops_list until pci_bus_set_ops complete, use list_for_each_entry() instead of
pci_bus_ops_pop to get bus_ops.
Signed-off-by: Yijing Wang <[email protected]>
---
drivers/pci/pcie/aer/aer_inject.c | 5 +++--
1 files changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/pci/pcie/aer/aer_inject.c b/drivers/pci/pcie/aer/aer_inject.c
index fdab3bb..f499f01 100644
--- a/drivers/pci/pcie/aer/aer_inject.c
+++ b/drivers/pci/pcie/aer/aer_inject.c
@@ -543,10 +543,11 @@ static void __exit aer_inject_exit(void)
misc_deregister(&aer_inject_device);
- while ((bus_ops = pci_bus_ops_pop())) {
+ list_for_each_entry(bus_ops, &pci_bus_ops_list, list)
pci_bus_set_ops(bus_ops->bus, bus_ops->ops);
+
+ while ((bus_ops = pci_bus_ops_pop()))
kfree(bus_ops);
- }
spin_lock_irqsave(&inject_lock, flags);
list_for_each_entry_safe(err, err_next, &einjected, list) {
--
1.7.1
When we do hot plug for pci devices that were injected aer errors, some newly created child buses'
pci_ops will be assigned to pci_ops_aer. Aer_inject module will not track these pci_ops_aer(not
list in pci_bus_ops_list),so we should clean all of these when rmmod aer_inject module.
Reviewed-by: Huang Ying <[email protected]>
Signed-off-by: Yijing Wang <[email protected]>
---
drivers/pci/pcie/aer/aer_inject.c | 24 ++++++++++++++++++++++++
1 files changed, 24 insertions(+), 0 deletions(-)
diff --git a/drivers/pci/pcie/aer/aer_inject.c b/drivers/pci/pcie/aer/aer_inject.c
index f499f01..4777c44 100644
--- a/drivers/pci/pcie/aer/aer_inject.c
+++ b/drivers/pci/pcie/aer/aer_inject.c
@@ -285,6 +285,29 @@ static void pci_bus_ops_init(struct pci_bus_ops *bus_ops,
bus_ops->ops = ops;
}
+static void pci_clean_child_aer_ops(struct pci_bus *bus)
+{
+ struct pci_bus *child;
+
+ list_for_each_entry(child, &bus->children, node) {
+ if (child->ops == &pci_ops_aer)
+ pci_bus_set_ops(child, bus->ops);
+ pci_clean_child_aer_ops(child);
+ }
+}
+
+/* find pci_ops_aer from root bus, and replace it by parent bus's pci_ops.
+ * pci_ops of root bus won't be pci_ops_aer here*/
+static void clean_untracked_pci_ops_aer(void)
+{
+ struct pci_bus_ops *bus_ops;
+
+ list_for_each_entry(bus_ops, &pci_bus_ops_list, list) {
+ if (pci_is_root_bus(bus_ops->bus))
+ pci_clean_child_aer_ops(bus_ops->bus);
+ }
+}
+
static int pci_bus_set_aer_ops(struct pci_bus *bus)
{
struct pci_ops *ops;
@@ -546,6 +569,7 @@ static void __exit aer_inject_exit(void)
list_for_each_entry(bus_ops, &pci_bus_ops_list, list)
pci_bus_set_ops(bus_ops->bus, bus_ops->ops);
+ clean_untracked_pci_ops_aer();
while ((bus_ops = pci_bus_ops_pop()))
kfree(bus_ops);
--
1.7.1
When we injected aer errors to the pcie device by aer_inject module, pci_ops of the pci
bus the device on will be assigned to pci_ops_aer.So if the target pci device
is a bridge, once we hot-remove and hot-add the bridge, the newly created child bus's pci_ops
will be assigned to pci_ops_aer too.Now every access to the child bus's device will cause the
system panic, because it will get a NULL pci_ops in pci_read_aer/pci_write_aer.
Reviewed-by: Huang Ying <[email protected]>
Signed-off-by: Yijing Wang <[email protected]>
Signed-off-by: Jiang Liu <[email protected]>
Reviewed-by: Sven Dietrich <[email protected]>
---
drivers/pci/pcie/aer/aer_inject.c | 26 +++++++++++++++++++++++++-
1 files changed, 25 insertions(+), 1 deletions(-)
diff --git a/drivers/pci/pcie/aer/aer_inject.c b/drivers/pci/pcie/aer/aer_inject.c
index 4e24cb8..fdab3bb 100644
--- a/drivers/pci/pcie/aer/aer_inject.c
+++ b/drivers/pci/pcie/aer/aer_inject.c
@@ -109,6 +109,26 @@ static struct aer_error *__find_aer_error_by_dev(struct pci_dev *dev)
return __find_aer_error((u16)domain, dev->bus->number, dev->devfn);
}
+/* find pci_ops of the nearest parent bus */
+static struct pci_ops *__find_pci_bus_ops_parent(struct pci_bus *bus)
+{
+ struct pci_bus_ops *bus_ops;
+ struct pci_bus *pbus = bus->parent;
+
+ if (!pbus)
+ return NULL;
+
+ while (pbus) {
+ list_for_each_entry(bus_ops, &pci_bus_ops_list, list)
+ if (bus_ops->bus == pbus)
+ return bus_ops->ops;
+
+ pbus = pbus->parent;
+ }
+
+ return NULL;
+}
+
/* inject_lock must be held before calling */
static struct pci_ops *__find_pci_bus_ops(struct pci_bus *bus)
{
@@ -118,7 +138,9 @@ static struct pci_ops *__find_pci_bus_ops(struct pci_bus *bus)
if (bus_ops->bus == bus)
return bus_ops->ops;
}
- return NULL;
+
+ /* can't find bus_ops, fall back to get bus_ops of parent bus */
+ return __find_pci_bus_ops_parent(bus);
}
static struct pci_bus_ops *pci_bus_ops_pop(void)
@@ -208,6 +230,7 @@ static int pci_read_aer(struct pci_bus *bus, unsigned int devfn, int where,
}
out:
ops = __find_pci_bus_ops(bus);
+ BUG_ON(!ops);
spin_unlock_irqrestore(&inject_lock, flags);
return ops->read(bus, devfn, where, size, val);
}
@@ -243,6 +266,7 @@ int pci_write_aer(struct pci_bus *bus, unsigned int devfn, int where, int size,
}
out:
ops = __find_pci_bus_ops(bus);
+ BUG_ON(!ops);
spin_unlock_irqrestore(&inject_lock, flags);
return ops->write(bus, devfn, where, size, val);
}
--
1.7.1
Rewrite pci_bus_ops_list release code for simplification, and clean
no used function pci_bus_ops_pop().
Reviewed-by: Huang Ying <[email protected]>
Signed-off-by: Yijing Wang <[email protected]>
---
drivers/pci/pcie/aer/aer_inject.c | 25 ++++++-------------------
1 files changed, 6 insertions(+), 19 deletions(-)
diff --git a/drivers/pci/pcie/aer/aer_inject.c b/drivers/pci/pcie/aer/aer_inject.c
index c5372dd..14d2c92 100644
--- a/drivers/pci/pcie/aer/aer_inject.c
+++ b/drivers/pci/pcie/aer/aer_inject.c
@@ -143,23 +143,6 @@ static struct pci_ops *__find_pci_bus_ops(struct pci_bus *bus)
return __find_pci_bus_ops_parent(bus);
}
-static struct pci_bus_ops *pci_bus_ops_pop(void)
-{
- unsigned long flags;
- struct pci_bus_ops *bus_ops = NULL;
-
- spin_lock_irqsave(&inject_lock, flags);
- if (list_empty(&pci_bus_ops_list))
- bus_ops = NULL;
- else {
- struct list_head *lh = pci_bus_ops_list.next;
- list_del(lh);
- bus_ops = list_entry(lh, struct pci_bus_ops, list);
- }
- spin_unlock_irqrestore(&inject_lock, flags);
- return bus_ops;
-}
-
static u32 *find_pci_config_dword(struct aer_error *err, int where,
int *prw1cs)
{
@@ -608,7 +591,7 @@ static void __exit aer_inject_exit(void)
{
struct aer_error *err, *err_next;
unsigned long flags;
- struct pci_bus_ops *bus_ops;
+ struct pci_bus_ops *bus_ops, *tmp_ops;
bus_unregister_notifier(&pci_bus_type, &aerinj_hp_notifier);
misc_deregister(&aer_inject_device);
@@ -617,8 +600,12 @@ static void __exit aer_inject_exit(void)
pci_bus_set_ops(bus_ops->bus, bus_ops->ops);
clean_untracked_pci_ops_aer();
- while ((bus_ops = pci_bus_ops_pop()))
+
+ /* free pci_bus_ops_list */
+ list_for_each_entry_safe(bus_ops, tmp_ops, &pci_bus_ops_list, list) {
+ list_del(&bus_ops->list);
kfree(bus_ops);
+ }
spin_lock_irqsave(&inject_lock, flags);
list_for_each_entry_safe(err, err_next, &einjected, list) {
--
1.7.1