[PATCH v5 0/5] Fix aer_inject bug caused by pci hot-plug

[PATCH v5 0/5] Fix aer_inject bug caused by pci hot-plug

[Yijing Wang]

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(-)

[PATCH v5 1/5] PCI/AER: Fix pci_ops return NULL in pci_read/write_aer

[Yijing Wang]

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.

Signed-off-by: Yijing Wang <wangyijing@huawei.com>
Signed-off-by: Jiang Liu <jiang.liu@huawei.com>
Reviewed-by: Sven Dietrich <Sven.Dietrich@huawei.com>
---
 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

[PATCH v5 2/5] PCI/AER: use list_for_each_entry to avoid a small race condition window

[Yijing Wang]

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 <wangyijing@huawei.com>
---
 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

[PATCH v5 3/5] PCI/AER: clean all untracked pci_ops_aer when rmmod aer_inject

[Yijing Wang]

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.

Signed-off-by: Yijing Wang <wangyijing@huawei.com>
Reviewed-by: Huang Ying <ying.huang@intel.com>
---
 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

[PATCH v5 4/5] PCI/AER: clean pci_bus_ops when related pci bus was removed

[Yijing Wang]

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 <wangyijing@huawei.com>
Reviewed-by: Sven Dietrich <Sven.Dietrich@huawei.com>
---
 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

[PATCH v5 5/5] PCI/AER: free pci_bus_ops_list and remove pci_bus_ops_pop

[Yijing Wang]

Rewrite pci_bus_ops_list release code for simplification, and clean
no used function pci_bus_ops_pop().

Signed-off-by: Yijing Wang <wangyijing@huawei.com>
Reviewed-by: Huang Ying <ying.huang@intel.com>
---
 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

Re: [PATCH v5 1/5] PCI/AER: Fix pci_ops return NULL in pci_read/write_aer

[Huang Ying]

On Fri, 2012-09-21 at 16:43 +0800, Yijing Wang wrote:
> 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.
> 
> Signed-off-by: Yijing Wang <wangyijing@huawei.com>
> Signed-off-by: Jiang Liu <jiang.liu@huawei.com>
> Reviewed-by: Sven Dietrich <Sven.Dietrich@huawei.com>

Reviewed-by: Huang Ying <ying.huang@intel.com>

> ---
>  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);
>  }