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

[PATCH v4 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: introduce pci_bus_ops_get() 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 |  127 ++++++++++++++++++++++++++++++++-----
 1 files changed, 112 insertions(+), 15 deletions(-)

[PATCH v4 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 v4 2/5] PCI/AER: introduce pci_bus_ops_get() 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 introduce pci_bus_ops_get() to avoid this.

Signed-off-by: Yijing Wang <wangyijing@huawei.com>
Signed-off-by: Huang Ying <ying.huang@intel.com>
---
 drivers/pci/pcie/aer/aer_inject.c |   21 ++++++++++++++++++---
 1 files changed, 18 insertions(+), 3 deletions(-)

diff --git a/drivers/pci/pcie/aer/aer_inject.c b/drivers/pci/pcie/aer/aer_inject.c
index fdab3bb..0123120 100644
--- a/drivers/pci/pcie/aer/aer_inject.c
+++ b/drivers/pci/pcie/aer/aer_inject.c
@@ -67,6 +67,8 @@ struct pci_bus_ops {
 	struct pci_ops *ops;
 };
 
+#define to_pci_bus_ops(n) container_of(n, struct pci_bus_ops, list)
+
 static LIST_HEAD(einjected);
 
 static LIST_HEAD(pci_bus_ops_list);
@@ -160,6 +162,18 @@ static struct pci_bus_ops *pci_bus_ops_pop(void)
 	return bus_ops;
 }
 
+static struct pci_bus_ops *pci_bus_ops_get(struct pci_bus_ops *from)
+{
+	struct pci_bus_ops *bus_ops = NULL;
+	struct list_head *n;
+
+	n = from ? from->list.next : pci_bus_ops_list.next;
+	if (n != &pci_bus_ops_list)
+		bus_ops = to_pci_bus_ops(n);
+
+	return bus_ops;
+}
+
 static u32 *find_pci_config_dword(struct aer_error *err, int where,
 				  int *prw1cs)
 {
@@ -539,14 +553,15 @@ 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 = NULL;
 
 	misc_deregister(&aer_inject_device);
 
-	while ((bus_ops = pci_bus_ops_pop())) {
+	while ((bus_ops = pci_bus_ops_get(bus_ops)))
 		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 v4 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>
Signed-off-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 0123120..e04cf24 100644
--- a/drivers/pci/pcie/aer/aer_inject.c
+++ b/drivers/pci/pcie/aer/aer_inject.c
@@ -299,6 +299,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;
@@ -560,6 +583,7 @@ static void __exit aer_inject_exit(void)
 	while ((bus_ops = pci_bus_ops_get(bus_ops)))
 		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 v4 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 e04cf24..a5fd393 100644
--- a/drivers/pci/pcie/aer/aer_inject.c
+++ b/drivers/pci/pcie/aer/aer_inject.c
@@ -567,9 +567,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)
@@ -578,6 +624,7 @@ static void __exit aer_inject_exit(void)
 	unsigned long flags;
 	struct pci_bus_ops *bus_ops = NULL;
 
+	bus_unregister_notifier(&pci_bus_type, &aerinj_hp_notifier);
 	misc_deregister(&aer_inject_device);
 
 	while ((bus_ops = pci_bus_ops_get(bus_ops)))
-- 
1.7.1

[PATCH v4 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>
Signed-off-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 a5fd393..4c9268a 100644
--- a/drivers/pci/pcie/aer/aer_inject.c
+++ b/drivers/pci/pcie/aer/aer_inject.c
@@ -145,23 +145,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 struct pci_bus_ops *pci_bus_ops_get(struct pci_bus_ops *from)
 {
 	struct pci_bus_ops *bus_ops = NULL;
@@ -622,7 +605,7 @@ static void __exit aer_inject_exit(void)
 {
 	struct aer_error *err, *err_next;
 	unsigned long flags;
-	struct pci_bus_ops *bus_ops = NULL;
+	struct pci_bus_ops *bus_ops = NULL, *tmp_ops;
 
 	bus_unregister_notifier(&pci_bus_type, &aerinj_hp_notifier);
 	misc_deregister(&aer_inject_device);
@@ -631,8 +614,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 v4 2/5] PCI/AER: introduce pci_bus_ops_get() to avoid a small race condition window

[Huang Ying]

On Fri, 2012-09-21 at 11:44 +0800, Yijing Wang wrote:
> 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 introduce pci_bus_ops_get() to avoid this.
> 
> Signed-off-by: Yijing Wang <wangyijing@huawei.com>
> Signed-off-by: Huang Ying <ying.huang@intel.com>

Please don't add my signed-off.

> ---
>  drivers/pci/pcie/aer/aer_inject.c |   21 ++++++++++++++++++---
>  1 files changed, 18 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/pci/pcie/aer/aer_inject.c b/drivers/pci/pcie/aer/aer_inject.c
> index fdab3bb..0123120 100644
> --- a/drivers/pci/pcie/aer/aer_inject.c
> +++ b/drivers/pci/pcie/aer/aer_inject.c
> @@ -67,6 +67,8 @@ struct pci_bus_ops {
>  	struct pci_ops *ops;
>  };
>  
> +#define to_pci_bus_ops(n) container_of(n, struct pci_bus_ops, list)
> +
>  static LIST_HEAD(einjected);
>  
>  static LIST_HEAD(pci_bus_ops_list);
> @@ -160,6 +162,18 @@ static struct pci_bus_ops *pci_bus_ops_pop(void)
>  	return bus_ops;
>  }
>  
> +static struct pci_bus_ops *pci_bus_ops_get(struct pci_bus_ops *from)
> +{
> +	struct pci_bus_ops *bus_ops = NULL;
> +	struct list_head *n;
> +
> +	n = from ? from->list.next : pci_bus_ops_list.next;
> +	if (n != &pci_bus_ops_list)
> +		bus_ops = to_pci_bus_ops(n);
> +
> +	return bus_ops;
> +}
> +
>  static u32 *find_pci_config_dword(struct aer_error *err, int where,
>  				  int *prw1cs)
>  {
> @@ -539,14 +553,15 @@ 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 = NULL;
>  
>  	misc_deregister(&aer_inject_device);
>  
> -	while ((bus_ops = pci_bus_ops_pop())) {
> +	while ((bus_ops = pci_bus_ops_get(bus_ops)))
>  		pci_bus_set_ops(bus_ops->bus, bus_ops->ops);

Why not just use

	list_for_each_entry(&pci_bus_ops_list) {
		pci_bus_set_ops();
	}

Best Regards,
Huang Ying

> +
> +	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) {

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

[Huang Ying]

On Fri, 2012-09-21 at 11:44 +0800, Yijing Wang wrote:
> 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>
> Signed-off-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 0123120..e04cf24 100644
> --- a/drivers/pci/pcie/aer/aer_inject.c
> +++ b/drivers/pci/pcie/aer/aer_inject.c
> @@ -299,6 +299,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))

Why do cleanup only for root bus?

Best Regards,
Huang Ying

> +			pci_clean_child_aer_ops(bus_ops->bus);
> +	}
> +}
> +
>  static int pci_bus_set_aer_ops(struct pci_bus *bus)
>  {
>  	struct pci_ops *ops;
> @@ -560,6 +583,7 @@ static void __exit aer_inject_exit(void)
>  	while ((bus_ops = pci_bus_ops_get(bus_ops)))
>  		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);
>  

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

[Huang Ying]

On Fri, 2012-09-21 at 11:44 +0800, Yijing Wang wrote:
> 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>
> Signed-off-by: Huang Ying <ying.huang@intel.com>

Please remove my signed-off.

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 a5fd393..4c9268a 100644
> --- a/drivers/pci/pcie/aer/aer_inject.c
> +++ b/drivers/pci/pcie/aer/aer_inject.c
> @@ -145,23 +145,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 struct pci_bus_ops *pci_bus_ops_get(struct pci_bus_ops *from)
>  {
>  	struct pci_bus_ops *bus_ops = NULL;
> @@ -622,7 +605,7 @@ static void __exit aer_inject_exit(void)
>  {
>  	struct aer_error *err, *err_next;
>  	unsigned long flags;
> -	struct pci_bus_ops *bus_ops = NULL;
> +	struct pci_bus_ops *bus_ops = NULL, *tmp_ops;
>  
>  	bus_unregister_notifier(&pci_bus_type, &aerinj_hp_notifier);
>  	misc_deregister(&aer_inject_device);
> @@ -631,8 +614,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) {

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

[Yijing Wang]

On 2012/9/21 13:13, Huang Ying wrote:
> On Fri, 2012-09-21 at 11:44 +0800, Yijing Wang wrote:
>> 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>
>> Signed-off-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 0123120..e04cf24 100644
>> --- a/drivers/pci/pcie/aer/aer_inject.c
>> +++ b/drivers/pci/pcie/aer/aer_inject.c
>> @@ -299,6 +299,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))
> 
> Why do cleanup only for root bus?
> 

Because the bus with untracked pci_ops_aer always is the child bus of the root bus
whose pci_ops_aer is tracked in pci_bus_ops_list. So here only do cleanup for root bus to
avoid unnecessary pci_clean_child_aer_ops() called.
eg.
If inject aer errors into 0000:46:00.0, so the pci_ops of bus 0000:40 and bus 0000:46 will be assign to pci_ops_aer
hot remove and hot add 0000:46:00.0, so newly created bus 0000:47 0000:48 and 0000:49 have untracked pci_ops_aer;
In this case 0000:40 and 0000:46 are tracked in pci_bus_ops_list, but 0000:47 0000:48 and 0000:49 are not.
So when we clean untracked pci_ops_aer, I think only cleanup root bus's child bus is enough. cleanup bus 0000:46's child bus
is unnecessary.

 |           +-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

Thanks
Yijing


> Best Regards,
> Huang Ying
> 
>> +			pci_clean_child_aer_ops(bus_ops->bus);
>> +	}
>> +}
>> +
>>  static int pci_bus_set_aer_ops(struct pci_bus *bus)
>>  {
>>  	struct pci_ops *ops;
>> @@ -560,6 +583,7 @@ static void __exit aer_inject_exit(void)
>>  	while ((bus_ops = pci_bus_ops_get(bus_ops)))
>>  		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);
>>  
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> .
> 


-- 
Thanks!
Yijing

Re: [PATCH v4 2/5] PCI/AER: introduce pci_bus_ops_get() to avoid a small race condition window

[Yijing Wang]

On 2012/9/21 13:08, Huang Ying wrote:
> On Fri, 2012-09-21 at 11:44 +0800, Yijing Wang wrote:
>> 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 introduce pci_bus_ops_get() to avoid this.
>>
>> Signed-off-by: Yijing Wang <wangyijing@huawei.com>
>> Signed-off-by: Huang Ying <ying.huang@intel.com>
> 
> Please don't add my signed-off.
> 
>> ---
>>  drivers/pci/pcie/aer/aer_inject.c |   21 ++++++++++++++++++---
>>  1 files changed, 18 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/pci/pcie/aer/aer_inject.c b/drivers/pci/pcie/aer/aer_inject.c
>> index fdab3bb..0123120 100644
>> --- a/drivers/pci/pcie/aer/aer_inject.c
>> +++ b/drivers/pci/pcie/aer/aer_inject.c
>> @@ -67,6 +67,8 @@ struct pci_bus_ops {
>>  	struct pci_ops *ops;
>>  };
>>  
>> +#define to_pci_bus_ops(n) container_of(n, struct pci_bus_ops, list)
>> +
>>  static LIST_HEAD(einjected);
>>  
>>  static LIST_HEAD(pci_bus_ops_list);
>> @@ -160,6 +162,18 @@ static struct pci_bus_ops *pci_bus_ops_pop(void)
>>  	return bus_ops;
>>  }
>>  
>> +static struct pci_bus_ops *pci_bus_ops_get(struct pci_bus_ops *from)
>> +{
>> +	struct pci_bus_ops *bus_ops = NULL;
>> +	struct list_head *n;
>> +
>> +	n = from ? from->list.next : pci_bus_ops_list.next;
>> +	if (n != &pci_bus_ops_list)
>> +		bus_ops = to_pci_bus_ops(n);
>> +
>> +	return bus_ops;
>> +}
>> +
>>  static u32 *find_pci_config_dword(struct aer_error *err, int where,
>>  				  int *prw1cs)
>>  {
>> @@ -539,14 +553,15 @@ 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 = NULL;
>>  
>>  	misc_deregister(&aer_inject_device);
>>  
>> -	while ((bus_ops = pci_bus_ops_pop())) {
>> +	while ((bus_ops = pci_bus_ops_get(bus_ops)))
>>  		pci_bus_set_ops(bus_ops->bus, bus_ops->ops);
> 
> Why not just use
> 
> 	list_for_each_entry(&pci_bus_ops_list) {
> 		pci_bus_set_ops();
> 	}
> 

Yes, you are right, use list_for_each_entry(&pci_bus_ops_list) is cleaner.

Thanks
Yijing

> Best Regards,
> Huang Ying
> 
>> +
>> +	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) {
> 
> 
> 
> .
> 


-- 
Thanks!
Yijing

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

[Huang Ying]

On Fri, 2012-09-21 at 13:47 +0800, Yijing Wang wrote:
> On 2012/9/21 13:13, Huang Ying wrote:
> > On Fri, 2012-09-21 at 11:44 +0800, Yijing Wang wrote:
> >> 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>
> >> Signed-off-by: Huang Ying <ying.huang@intel.com>

Please remove my signed-off.

> >> ---
> >>  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 0123120..e04cf24 100644
> >> --- a/drivers/pci/pcie/aer/aer_inject.c
> >> +++ b/drivers/pci/pcie/aer/aer_inject.c
> >> @@ -299,6 +299,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))
> > 
> > Why do cleanup only for root bus?
> > 
> 
> Because the bus with untracked pci_ops_aer always is the child bus of the root bus
> whose pci_ops_aer is tracked in pci_bus_ops_list. So here only do cleanup for root bus to
> avoid unnecessary pci_clean_child_aer_ops() called.
> eg.
> If inject aer errors into 0000:46:00.0, so the pci_ops of bus 0000:40 and bus 0000:46 will be assign to pci_ops_aer
> hot remove and hot add 0000:46:00.0, so newly created bus 0000:47 0000:48 and 0000:49 have untracked pci_ops_aer;
> In this case 0000:40 and 0000:46 are tracked in pci_bus_ops_list, but 0000:47 0000:48 and 0000:49 are not.
> So when we clean untracked pci_ops_aer, I think only cleanup root bus's child bus is enough. cleanup bus 0000:46's child bus
> is unnecessary.
> 
>  |           +-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

I see.

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

Best Regards,
Huang Ying

> Thanks
> Yijing
> 
> 
> > Best Regards,
> > Huang Ying
> > 
> >> +			pci_clean_child_aer_ops(bus_ops->bus);
> >> +	}
> >> +}
> >> +
> >>  static int pci_bus_set_aer_ops(struct pci_bus *bus)
> >>  {
> >>  	struct pci_ops *ops;
> >> @@ -560,6 +583,7 @@ static void __exit aer_inject_exit(void)
> >>  	while ((bus_ops = pci_bus_ops_get(bus_ops)))
> >>  		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);
> >>  
> > 
> > 
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > 
> > .
> > 
> 
>