[PATCH 0/6] fix aer_inject bug while doing pci hot-plug

[PATCH 0/6] fix aer_inject bug while doing pci hot-plug

[Yijing Wang]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 852 bytes --]

v2--->v3
1¡¢find the pci_ops of nearest parent bus in patch 1;
2¡¢introduce pci_bus_ops_get to avoid race condition window in aer_inject_exit;
3¡¢scan the root bus for cleaning untracked pci_ops_aer instead of scan all pci bus as Huang Ying suggestion;
4¡¢remove unused code pci_bus_ops_pop;

Yijing Wang (6):
  PCI/AER: fix pci_ops return NULL when hotplug a pci bus doing aer
    error inject
  PCI/AER: introduce pci_bus_ops_get() function 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: introduce pci_bus_ops_free to free pci_bus_ops
  PCI/AER: clean unused code pci_bus_ops_pop

 drivers/pci/pcie/aer/aer_inject.c |  149 ++++++++++++++++++++++++++++++++-----
 1 files changed, 131 insertions(+), 18 deletions(-)

[PATCH 1/6] PCI/AER: fix pci_ops return NULL when hotplug a pci bus doing aer error inject

[Yijing Wang]

When we inject aer errors to the target pcie device by aer_inject module, the pci_ops of pci
bus which the target device is 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 devices will result to
system panic, because it 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 |   27 ++++++++++++++++++++++++++-
 1 files changed, 26 insertions(+), 1 deletions(-)

diff --git a/drivers/pci/pcie/aer/aer_inject.c b/drivers/pci/pcie/aer/aer_inject.c
index 4e24cb8..0f00a27 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 upstream 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);
 }
@@ -506,6 +530,7 @@ static struct miscdevice aer_inject_device = {
 	.fops = &aer_inject_fops,
 };
 
+
 static int __init aer_inject_init(void)
 {
 	return misc_register(&aer_inject_device);
-- 
1.7.1

[PATCH 2/6] PCI/AER: introduce pci_bus_ops_get() function to avoid a small race condition window

[Yijing Wang]

When we rmmod aer_inject module, there is a 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>
---
 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 0f00a27..442147b 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)
 {
@@ -540,14 +554,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 3/6] PCI/AER: clean all untracked pci_ops_aer when rmmod aer_inject

[Yijing Wang]

Since hot plug for pci devices while doing aer inject, some newly created child buses'
pci_ops will be assigned to pci_ops_aer. Aer_inject does not track these pci_ops_aer(not
list in pci_bus_ops_list),we should clean all of these when rmmod aer_inject module.

Signed-off-by: Yijing Wang <wangyijing@huawei.com>
---
 drivers/pci/pcie/aer/aer_inject.c |   35 +++++++++++++++++++++++++++++++++++
 1 files changed, 35 insertions(+), 0 deletions(-)

diff --git a/drivers/pci/pcie/aer/aer_inject.c b/drivers/pci/pcie/aer/aer_inject.c
index 442147b..e2400b5 100644
--- a/drivers/pci/pcie/aer/aer_inject.c
+++ b/drivers/pci/pcie/aer/aer_inject.c
@@ -299,6 +299,32 @@ 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) {
+		/* the bus with untracked pci_ops_aer always the
+		 * child bus of root bus tracked in pci_bus_ops_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;
@@ -558,9 +584,18 @@ static void __exit aer_inject_exit(void)
 
 	misc_deregister(&aer_inject_device);
 
+	/* clean pci_bus_ops tracked in pci_bus_ops_list */
 	while ((bus_ops = pci_bus_ops_get(bus_ops)))
 		pci_bus_set_ops(bus_ops->bus, bus_ops->ops);
 
+	/* Inject aer errors and hotplug the same pcie device
+	 * maybe assign some newly created buses' pci_ops pci_ops_aer.
+	 * Since these pci_ops_aer are not tracked in pci_bus_ops_list,
+	 * we need to find and clean untracked pci_ops_aer before aer_inject
+	 * module exit
+	 */
+	clean_untracked_pci_ops_aer();
+
 	while ((bus_ops = pci_bus_ops_pop()))
 		kfree(bus_ops);
 
-- 
1.7.1

[PATCH 4/6] 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 |   48 ++++++++++++++++++++++++++++++++++++-
 1 files changed, 47 insertions(+), 1 deletions(-)

diff --git a/drivers/pci/pcie/aer/aer_inject.c b/drivers/pci/pcie/aer/aer_inject.c
index e2400b5..0a12ac7 100644
--- a/drivers/pci/pcie/aer/aer_inject.c
+++ b/drivers/pci/pcie/aer/aer_inject.c
@@ -570,10 +570,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)
@@ -582,6 +627,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);
 
 	/* clean pci_bus_ops tracked in pci_bus_ops_list */
-- 
1.7.1

[PATCH 5/6] PCI/AER: introduce pci_bus_ops_free to free pci_bus_ops

[Yijing Wang]

Signed-off-by: Yijing Wang <wangyijing@huawei.com>
---
 drivers/pci/pcie/aer/aer_inject.c |   14 +++++++++++---
 1 files changed, 11 insertions(+), 3 deletions(-)

diff --git a/drivers/pci/pcie/aer/aer_inject.c b/drivers/pci/pcie/aer/aer_inject.c
index 0a12ac7..79b611d 100644
--- a/drivers/pci/pcie/aer/aer_inject.c
+++ b/drivers/pci/pcie/aer/aer_inject.c
@@ -162,6 +162,16 @@ static struct pci_bus_ops *pci_bus_ops_pop(void)
 	return bus_ops;
 }
 
+static void pci_bus_ops_free(void)
+{
+	struct pci_bus_ops *bus_ops, *tmp_ops;
+
+	list_for_each_entry_safe(bus_ops, tmp_ops, &pci_bus_ops_list, list) {
+		list_del(&bus_ops->list);
+		kfree(bus_ops);
+	}
+}
+
 static struct pci_bus_ops *pci_bus_ops_get(struct pci_bus_ops *from)
 {
 	struct pci_bus_ops *bus_ops = NULL;
@@ -641,9 +651,7 @@ static void __exit aer_inject_exit(void)
 	 * module exit
 	 */
 	clean_untracked_pci_ops_aer();
-
-	while ((bus_ops = pci_bus_ops_pop()))
-		kfree(bus_ops);
+	pci_bus_ops_free();
 
 	spin_lock_irqsave(&inject_lock, flags);
 	list_for_each_entry_safe(err, err_next, &einjected, list) {
-- 
1.7.1

[PATCH 6/6] PCI/AER: clean unused code pci_bus_ops_pop

[Yijing Wang]

Signed-off-by: Yijing Wang <wangyijing@huawei.com>
---
 drivers/pci/pcie/aer/aer_inject.c |   16 ----------------
 1 files changed, 0 insertions(+), 16 deletions(-)

diff --git a/drivers/pci/pcie/aer/aer_inject.c b/drivers/pci/pcie/aer/aer_inject.c
index 79b611d..9a24af4 100644
--- a/drivers/pci/pcie/aer/aer_inject.c
+++ b/drivers/pci/pcie/aer/aer_inject.c
@@ -145,22 +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 void pci_bus_ops_free(void)
 {
-- 
1.7.1

Re: [PATCH 1/6] PCI/AER: fix pci_ops return NULL when hotplug a pci bus doing aer error inject

[Huang Ying]

On Wed, 2012-09-19 at 10:40 +0800, Yijing Wang wrote:
> When we inject aer errors to the target pcie device by aer_inject module, the pci_ops of pci
> bus which the target device is 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 devices will result to
> system panic, because it 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 |   27 ++++++++++++++++++++++++++-
>  1 files changed, 26 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/pci/pcie/aer/aer_inject.c b/drivers/pci/pcie/aer/aer_inject.c
> index 4e24cb8..0f00a27 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 upstream 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);
>  }
> @@ -506,6 +530,7 @@ static struct miscdevice aer_inject_device = {
>  	.fops = &aer_inject_fops,
>  };
>  
> +

unnecessary new line?

Best Regards,
Huang Ying

>  static int __init aer_inject_init(void)
>  {
>  	return misc_register(&aer_inject_device);

Re: [PATCH 1/6] PCI/AER: fix pci_ops return NULL when hotplug a pci bus doing aer error inject

[Yijing Wang]

On 2012/9/19 13:13, Huang Ying wrote:
> On Wed, 2012-09-19 at 10:40 +0800, Yijing Wang wrote:
>> When we inject aer errors to the target pcie device by aer_inject module, the pci_ops of pci
>> bus which the target device is 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 devices will result to
>> system panic, because it 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 |   27 ++++++++++++++++++++++++++-
>>  1 files changed, 26 insertions(+), 1 deletions(-)
>>
>> diff --git a/drivers/pci/pcie/aer/aer_inject.c b/drivers/pci/pcie/aer/aer_inject.c
>> index 4e24cb8..0f00a27 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 upstream 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);
>>  }
>> @@ -506,6 +530,7 @@ static struct miscdevice aer_inject_device = {
>>  	.fops = &aer_inject_fops,
>>  };
>>  
>> +
> 
> unnecessary new line?
> 

Yes, it's unnecessary, I will clean this later.

Thanks
Yijing

> Best Regards,
> Huang Ying
> 
>>  static int __init aer_inject_init(void)
>>  {
>>  	return misc_register(&aer_inject_device);
> 
> 
> 
> .
> 


-- 
Thanks!
Yijing

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

[Huang Ying]

On Wed, 2012-09-19 at 10:40 +0800, Yijing Wang wrote:
> When we rmmod aer_inject module, there is a 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>
> ---
>  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 0f00a27..442147b 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)
>  {
> @@ -540,14 +554,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);

In fact, this is

list_for_each_entry(&pci_bus_ops_list)
	pci_bus_set_ops()

Because we are in module exit path, there will be no new user of
pci_bus_ops_list, it appears safe to do that without lock.

But the bus_ops may be deleted from the list when accessed via
pci_ops_aer.  So It may be better to wait for all pci_ops_aer functions
return before delete them.  synchronize_rcu() should be sufficient for
that, because all pci_ops_aer functions are called with spinlock held.

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 3/6] PCI/AER: clean all untracked pci_ops_aer when rmmod aer_inject

[Huang Ying]

On Wed, 2012-09-19 at 10:40 +0800, Yijing Wang wrote:
> Since hot plug for pci devices while doing aer inject, some newly created child buses'
> pci_ops will be assigned to pci_ops_aer. Aer_inject does not track these pci_ops_aer(not
> list in pci_bus_ops_list),we should clean all of these when rmmod aer_inject module.
> 
> Signed-off-by: Yijing Wang <wangyijing@huawei.com>
> ---
>  drivers/pci/pcie/aer/aer_inject.c |   35 +++++++++++++++++++++++++++++++++++
>  1 files changed, 35 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/pci/pcie/aer/aer_inject.c b/drivers/pci/pcie/aer/aer_inject.c
> index 442147b..e2400b5 100644
> --- a/drivers/pci/pcie/aer/aer_inject.c
> +++ b/drivers/pci/pcie/aer/aer_inject.c
> @@ -299,6 +299,32 @@ 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);
> +	}

Why not use pci_walk_bus?

> +}
> +
> +/* 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) {
> +		/* the bus with untracked pci_ops_aer always the
> +		 * child bus of root bus tracked in pci_bus_ops_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;
> @@ -558,9 +584,18 @@ static void __exit aer_inject_exit(void)
>  
>  	misc_deregister(&aer_inject_device);
>  
> +	/* clean pci_bus_ops tracked in pci_bus_ops_list */
>  	while ((bus_ops = pci_bus_ops_get(bus_ops)))
>  		pci_bus_set_ops(bus_ops->bus, bus_ops->ops);

Why not do clean here inside loop?

Best Regards,
Huang Ying

>  
> +	/* Inject aer errors and hotplug the same pcie device
> +	 * maybe assign some newly created buses' pci_ops pci_ops_aer.
> +	 * Since these pci_ops_aer are not tracked in pci_bus_ops_list,
> +	 * we need to find and clean untracked pci_ops_aer before aer_inject
> +	 * module exit
> +	 */
> +	clean_untracked_pci_ops_aer();
> +
>  	while ((bus_ops = pci_bus_ops_pop()))
>  		kfree(bus_ops);
>  

Re: [PATCH 5/6] PCI/AER: introduce pci_bus_ops_free to free pci_bus_ops

[Huang Ying]

On Wed, 2012-09-19 at 10:40 +0800, Yijing Wang wrote:
> Signed-off-by: Yijing Wang <wangyijing@huawei.com>
> ---
>  drivers/pci/pcie/aer/aer_inject.c |   14 +++++++++++---
>  1 files changed, 11 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/pci/pcie/aer/aer_inject.c b/drivers/pci/pcie/aer/aer_inject.c
> index 0a12ac7..79b611d 100644
> --- a/drivers/pci/pcie/aer/aer_inject.c
> +++ b/drivers/pci/pcie/aer/aer_inject.c
> @@ -162,6 +162,16 @@ static struct pci_bus_ops *pci_bus_ops_pop(void)
>  	return bus_ops;
>  }
>  
> +static void pci_bus_ops_free(void)
> +{
> +	struct pci_bus_ops *bus_ops, *tmp_ops;
> +
> +	list_for_each_entry_safe(bus_ops, tmp_ops, &pci_bus_ops_list, list) {
> +		list_del(&bus_ops->list);
> +		kfree(bus_ops);
> +	}
> +}

IMHO, this can be inlined into aer_inject_exit

>  static struct pci_bus_ops *pci_bus_ops_get(struct pci_bus_ops *from)
>  {
>  	struct pci_bus_ops *bus_ops = NULL;
> @@ -641,9 +651,7 @@ static void __exit aer_inject_exit(void)
>  	 * module exit
>  	 */
>  	clean_untracked_pci_ops_aer();
> -
> -	while ((bus_ops = pci_bus_ops_pop()))

You can remove pci_bus_ops_pop now.

Best Regards,
Huang Ying

> -		kfree(bus_ops);
> +	pci_bus_ops_free();
>  
>  	spin_lock_irqsave(&inject_lock, flags);
>  	list_for_each_entry_safe(err, err_next, &einjected, list) {

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

[Yijing Wang]

On 2012/9/19 13:57, Huang Ying wrote:
> On Wed, 2012-09-19 at 10:40 +0800, Yijing Wang wrote:
>> Since hot plug for pci devices while doing aer inject, some newly created child buses'
>> pci_ops will be assigned to pci_ops_aer. Aer_inject does not track these pci_ops_aer(not
>> list in pci_bus_ops_list),we should clean all of these when rmmod aer_inject module.
>>
>> Signed-off-by: Yijing Wang <wangyijing@huawei.com>
>> ---
>>  drivers/pci/pcie/aer/aer_inject.c |   35 +++++++++++++++++++++++++++++++++++
>>  1 files changed, 35 insertions(+), 0 deletions(-)
>>
>> diff --git a/drivers/pci/pcie/aer/aer_inject.c b/drivers/pci/pcie/aer/aer_inject.c
>> index 442147b..e2400b5 100644
>> --- a/drivers/pci/pcie/aer/aer_inject.c
>> +++ b/drivers/pci/pcie/aer/aer_inject.c
>> @@ -299,6 +299,32 @@ 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);
>> +	}
> 
> Why not use pci_walk_bus?
> 

I think here walk all child buses is enough, pci_walk_bus will walk all child devices under this bus.

>> +}
>> +
>> +/* 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) {
>> +		/* the bus with untracked pci_ops_aer always the
>> +		 * child bus of root bus tracked in pci_bus_ops_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;
>> @@ -558,9 +584,18 @@ static void __exit aer_inject_exit(void)
>>  
>>  	misc_deregister(&aer_inject_device);
>>  
>> +	/* clean pci_bus_ops tracked in pci_bus_ops_list */
>>  	while ((bus_ops = pci_bus_ops_get(bus_ops)))
>>  		pci_bus_set_ops(bus_ops->bus, bus_ops->ops);
> 
> Why not do clean here inside loop?
> 

Because I also need these bus_ops to scan untracked pci_ops_aer.

> Best Regards,
> Huang Ying
> 
>>  
>> +	/* Inject aer errors and hotplug the same pcie device
>> +	 * maybe assign some newly created buses' pci_ops pci_ops_aer.
>> +	 * Since these pci_ops_aer are not tracked in pci_bus_ops_list,
>> +	 * we need to find and clean untracked pci_ops_aer before aer_inject
>> +	 * module exit
>> +	 */
>> +	clean_untracked_pci_ops_aer();
>> +
>>  	while ((bus_ops = pci_bus_ops_pop()))
>>  		kfree(bus_ops);
>>  
> 
> 
> 
> .
> 


-- 
Thanks!
Yijing

Re: [PATCH 5/6] PCI/AER: introduce pci_bus_ops_free to free pci_bus_ops

[Yijing Wang]

On 2012/9/19 14:03, Huang Ying wrote:
> On Wed, 2012-09-19 at 10:40 +0800, Yijing Wang wrote:
>> Signed-off-by: Yijing Wang <wangyijing@huawei.com>
>> ---
>>  drivers/pci/pcie/aer/aer_inject.c |   14 +++++++++++---
>>  1 files changed, 11 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/pci/pcie/aer/aer_inject.c b/drivers/pci/pcie/aer/aer_inject.c
>> index 0a12ac7..79b611d 100644
>> --- a/drivers/pci/pcie/aer/aer_inject.c
>> +++ b/drivers/pci/pcie/aer/aer_inject.c
>> @@ -162,6 +162,16 @@ static struct pci_bus_ops *pci_bus_ops_pop(void)
>>  	return bus_ops;
>>  }
>>  
>> +static void pci_bus_ops_free(void)
>> +{
>> +	struct pci_bus_ops *bus_ops, *tmp_ops;
>> +
>> +	list_for_each_entry_safe(bus_ops, tmp_ops, &pci_bus_ops_list, list) {
>> +		list_del(&bus_ops->list);
>> +		kfree(bus_ops);
>> +	}
>> +}
> 
> IMHO, this can be inlined into aer_inject_exit
> 
>>  static struct pci_bus_ops *pci_bus_ops_get(struct pci_bus_ops *from)
>>  {
>>  	struct pci_bus_ops *bus_ops = NULL;
>> @@ -641,9 +651,7 @@ static void __exit aer_inject_exit(void)
>>  	 * module exit
>>  	 */
>>  	clean_untracked_pci_ops_aer();
>> -
>> -	while ((bus_ops = pci_bus_ops_pop()))
> 
> You can remove pci_bus_ops_pop now.
> 

OK, I will move pci_bus_ops_free inlined into aer_inject_exit and remove pci_bus_ops_pop in this patch.

> Best Regards,
> Huang Ying
> 
>> -		kfree(bus_ops);
>> +	pci_bus_ops_free();
>>  
>>  	spin_lock_irqsave(&inject_lock, flags);
>>  	list_for_each_entry_safe(err, err_next, &einjected, list) {
> 
> 
> 
> .
> 


-- 
Thanks!
Yijing

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

[Huang Ying]

On Wed, 2012-09-19 at 14:09 +0800, Yijing Wang wrote:
> On 2012/9/19 13:57, Huang Ying wrote:
> > On Wed, 2012-09-19 at 10:40 +0800, Yijing Wang wrote:
> >> Since hot plug for pci devices while doing aer inject, some newly created child buses'
> >> pci_ops will be assigned to pci_ops_aer. Aer_inject does not track these pci_ops_aer(not
> >> list in pci_bus_ops_list),we should clean all of these when rmmod aer_inject module.
> >>
> >> Signed-off-by: Yijing Wang <wangyijing@huawei.com>
> >> ---
> >>  drivers/pci/pcie/aer/aer_inject.c |   35 +++++++++++++++++++++++++++++++++++
> >>  1 files changed, 35 insertions(+), 0 deletions(-)
> >>
> >> diff --git a/drivers/pci/pcie/aer/aer_inject.c b/drivers/pci/pcie/aer/aer_inject.c
> >> index 442147b..e2400b5 100644
> >> --- a/drivers/pci/pcie/aer/aer_inject.c
> >> +++ b/drivers/pci/pcie/aer/aer_inject.c
> >> @@ -299,6 +299,32 @@ 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);
> >> +	}
> > 
> > Why not use pci_walk_bus?
> > 
> 
> I think here walk all child buses is enough, pci_walk_bus will walk all child devices under this bus.
> 
> >> +}
> >> +
> >> +/* 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) {
> >> +		/* the bus with untracked pci_ops_aer always the
> >> +		 * child bus of root bus tracked in pci_bus_ops_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;
> >> @@ -558,9 +584,18 @@ static void __exit aer_inject_exit(void)
> >>  
> >>  	misc_deregister(&aer_inject_device);
> >>  
> >> +	/* clean pci_bus_ops tracked in pci_bus_ops_list */
> >>  	while ((bus_ops = pci_bus_ops_get(bus_ops)))
> >>  		pci_bus_set_ops(bus_ops->bus, bus_ops->ops);
> > 
> > Why not do clean here inside loop?
> > 
> 
> Because I also need these bus_ops to scan untracked pci_ops_aer.

I mean why not call pci_clean_child_aer_ops() inside loop.

Best Regards,
Huang Ying

> > 
> >>  
> >> +	/* Inject aer errors and hotplug the same pcie device
> >> +	 * maybe assign some newly created buses' pci_ops pci_ops_aer.
> >> +	 * Since these pci_ops_aer are not tracked in pci_bus_ops_list,
> >> +	 * we need to find and clean untracked pci_ops_aer before aer_inject
> >> +	 * module exit
> >> +	 */
> >> +	clean_untracked_pci_ops_aer();
> >> +
> >>  	while ((bus_ops = pci_bus_ops_pop()))
> >>  		kfree(bus_ops);
> >>  
> > 
> > 
> > 
> > .
> > 
> 
> 

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

[Yijing Wang]

On 2012/9/19 14:18, Huang Ying wrote:
> On Wed, 2012-09-19 at 14:09 +0800, Yijing Wang wrote:
>> On 2012/9/19 13:57, Huang Ying wrote:
>>> On Wed, 2012-09-19 at 10:40 +0800, Yijing Wang wrote:
>>>> Since hot plug for pci devices while doing aer inject, some newly created child buses'
>>>> pci_ops will be assigned to pci_ops_aer. Aer_inject does not track these pci_ops_aer(not
>>>> list in pci_bus_ops_list),we should clean all of these when rmmod aer_inject module.
>>>>
>>>> Signed-off-by: Yijing Wang <wangyijing@huawei.com>
>>>> ---
>>>>  drivers/pci/pcie/aer/aer_inject.c |   35 +++++++++++++++++++++++++++++++++++
>>>>  1 files changed, 35 insertions(+), 0 deletions(-)
>>>>
>>>> diff --git a/drivers/pci/pcie/aer/aer_inject.c b/drivers/pci/pcie/aer/aer_inject.c
>>>> index 442147b..e2400b5 100644
>>>> --- a/drivers/pci/pcie/aer/aer_inject.c
>>>> +++ b/drivers/pci/pcie/aer/aer_inject.c
>>>> @@ -299,6 +299,32 @@ 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);
>>>> +	}
>>>
>>> Why not use pci_walk_bus?
>>>
>>
>> I think here walk all child buses is enough, pci_walk_bus will walk all child devices under this bus.
>>
>>>> +}
>>>> +
>>>> +/* 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) {
>>>> +		/* the bus with untracked pci_ops_aer always the
>>>> +		 * child bus of root bus tracked in pci_bus_ops_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;
>>>> @@ -558,9 +584,18 @@ static void __exit aer_inject_exit(void)
>>>>  
>>>>  	misc_deregister(&aer_inject_device);
>>>>  
>>>> +	/* clean pci_bus_ops tracked in pci_bus_ops_list */
>>>>  	while ((bus_ops = pci_bus_ops_get(bus_ops)))
>>>>  		pci_bus_set_ops(bus_ops->bus, bus_ops->ops);
>>>
>>> Why not do clean here inside loop?
>>>
>>
>> Because I also need these bus_ops to scan untracked pci_ops_aer.
> 
> I mean why not call pci_clean_child_aer_ops() inside loop.
> 

My idea is first clean all pci_ops_aer tracked in pci_bus_ops_list, second clean all untracked pci_ops_aer.
If put pci_clean_child_aer_ops inside loop, when I clean tracked pci_ops_aer for root bus 0, next I will use
bus 0 's pci_ops to clean bus 1 's pci_ops_aer. Actually maybe bus 1 have its own pci_ops_aer tracked in pci_bus_ops_list.
So I think use every bus's own bus_ops to clean pci_ops_aer first is better.

 \-[0000:00]-+-00.0  Intel Corporation 5500 I/O Hub to ESI Port
             +-01.0-[01]--+-00.0  Broadcom Corporation NetXtreme II BCM5709 Gigabit Ethernet
             |            \-00.1  Broadcom Corporation NetXtreme II BCM5709 Gigabit Ethernet


> Best Regards,
> Huang Ying
> 
>>>
>>>>  
>>>> +	/* Inject aer errors and hotplug the same pcie device
>>>> +	 * maybe assign some newly created buses' pci_ops pci_ops_aer.
>>>> +	 * Since these pci_ops_aer are not tracked in pci_bus_ops_list,
>>>> +	 * we need to find and clean untracked pci_ops_aer before aer_inject
>>>> +	 * module exit
>>>> +	 */
>>>> +	clean_untracked_pci_ops_aer();
>>>> +
>>>>  	while ((bus_ops = pci_bus_ops_pop()))
>>>>  		kfree(bus_ops);
>>>>  
>>>
>>>
>>>
>>> .
>>>
>>
>>
> 
> 
> 
> .
> 


-- 
Thanks!
Yijing

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

[Yijing Wang]

On 2012/9/19 13:52, Huang Ying wrote:
> On Wed, 2012-09-19 at 10:40 +0800, Yijing Wang wrote:
>> When we rmmod aer_inject module, there is a 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>
>> ---
>>  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 0f00a27..442147b 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)
>>  {
>> @@ -540,14 +554,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);
> 
> In fact, this is
> 
> list_for_each_entry(&pci_bus_ops_list)
> 	pci_bus_set_ops()
> 
> Because we are in module exit path, there will be no new user of
> pci_bus_ops_list, it appears safe to do that without lock.
> 
> But the bus_ops may be deleted from the list when accessed via
> pci_ops_aer.  So It may be better to wait for all pci_ops_aer functions

Hi Huang Ying,
   I have some confusions about this, can you explain this? Thanks very much!
In my idea, if pci_ops_aer be called, it hold the pci_lock, so pci_bus_set_ops will wait for
pci_ops_aer functions to exit.So in my idea, after pci_bus_set_ops loop completed. pci_ops_aer functions
have been exit, and will never be called again(because all pci_ops_aer).

> return before delete them.  synchronize_rcu() should be sufficient for
> that, because all pci_ops_aer functions are called with spinlock held.
> 


> 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 2/6] PCI/AER: introduce pci_bus_ops_get() function to avoid a small race condition window

[Huang Ying]

On Wed, 2012-09-19 at 14:42 +0800, Yijing Wang wrote:
> On 2012/9/19 13:52, Huang Ying wrote:
> > On Wed, 2012-09-19 at 10:40 +0800, Yijing Wang wrote:
> >> When we rmmod aer_inject module, there is a 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>
> >> ---
> >>  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 0f00a27..442147b 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)
> >>  {
> >> @@ -540,14 +554,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);
> > 
> > In fact, this is
> > 
> > list_for_each_entry(&pci_bus_ops_list)
> > 	pci_bus_set_ops()
> > 
> > Because we are in module exit path, there will be no new user of
> > pci_bus_ops_list, it appears safe to do that without lock.
> > 
> > But the bus_ops may be deleted from the list when accessed via
> > pci_ops_aer.  So It may be better to wait for all pci_ops_aer functions
> 
> Hi Huang Ying,
>    I have some confusions about this, can you explain this? Thanks very much!
> In my idea, if pci_ops_aer be called, it hold the pci_lock, so pci_bus_set_ops will wait for
> pci_ops_aer functions to exit.So in my idea, after pci_bus_set_ops loop completed. pci_ops_aer functions
> have been exit, and will never be called again(because all pci_ops_aer).

Yes. You are right,  waiting is not necessary here.

Best Regards,
Huang Ying

> > return before delete them.  synchronize_rcu() should be sufficient for
> > that, because all pci_ops_aer functions are called with spinlock held.
> > 
> 
> 
> > 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 5/6] PCI/AER: introduce pci_bus_ops_free to free pci_bus_ops

[Chen Gong]

[-- Attachment #1: Type: text/plain, Size: 617 bytes --]

On Wed, Sep 19, 2012 at 10:40:41AM +0800, Yijing Wang wrote:
> Date: Wed, 19 Sep 2012 10:40:41 +0800
> From: Yijing Wang <wangyijing@huawei.com>
> To: Bjorn Helgaas <bhelgaas@google.com>, Huang Ying <ying.huang@intel.com>,
>  Chen Gong <gong.chen@linux.intel.com>
> CC: jiang.liu@huawei.com, Hanjun Guo <guohanjun@huawei.com>,
>  linux-pci@vger.kernel.org, Yijing Wang <wangyijing@huawei.com>
> Subject: [PATCH 5/6] PCI/AER: introduce pci_bus_ops_free to free pci_bus_ops
> X-Mailer: git-send-email 1.7.11.msysgit.1
> 
> Signed-off-by: Yijing Wang <wangyijing@huawei.com>

Your patch hasn't description.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 0/6] fix aer_inject bug while doing pci hot-plug

[Chen Gong]

[-- Attachment #1: Type: text/plain, Size: 1806 bytes --]

On Wed, Sep 19, 2012 at 10:40:36AM +0800, Yijing Wang wrote:
> Date: Wed, 19 Sep 2012 10:40:36 +0800
> From: Yijing Wang <wangyijing@huawei.com>
> To: Bjorn Helgaas <bhelgaas@google.com>, Huang Ying <ying.huang@intel.com>,
>  Chen Gong <gong.chen@linux.intel.com>
> CC: jiang.liu@huawei.com, Hanjun Guo <guohanjun@huawei.com>,
>  linux-pci@vger.kernel.org, Yijing Wang <wangyijing@huawei.com>
> Subject: [PATCH 0/6] fix aer_inject bug while doing pci hot-plug
> X-Mailer: git-send-email 1.7.11.msysgit.1
> 
> v2--->v3
> 1??find the pci_ops of nearest parent bus in patch 1;
> 2??introduce pci_bus_ops_get to avoid race condition window in aer_inject_exit;
> 3??scan the root bus for cleaning untracked pci_ops_aer instead of scan all pci bus as Huang Ying suggestion;
> 4??remove unused code pci_bus_ops_pop;
> 
> Yijing Wang (6):
>   PCI/AER: fix pci_ops return NULL when hotplug a pci bus doing aer
>     error inject
>   PCI/AER: introduce pci_bus_ops_get() function 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: introduce pci_bus_ops_free to free pci_bus_ops
>   PCI/AER: clean unused code pci_bus_ops_pop
> 
>  drivers/pci/pcie/aer/aer_inject.c |  149 ++++++++++++++++++++++++++++++++-----
>  1 files changed, 131 insertions(+), 18 deletions(-)
> 
After reviewing the whole patch series, I hava a feeling that you don't
desrcibe the whole usage model very clearly. I suggest you update the
description and express it more clearly and in detail, so other guys never touch
aer-inject can understand it well. You patch is good but it doesn't mean
others will like it because it is obscure to others never touching these codes.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 5/6] PCI/AER: introduce pci_bus_ops_free to free pci_bus_ops

[Yijing Wang]

On 2012/9/19 15:11, Chen Gong wrote:
> On Wed, Sep 19, 2012 at 10:40:41AM +0800, Yijing Wang wrote:
>> Date: Wed, 19 Sep 2012 10:40:41 +0800
>> From: Yijing Wang <wangyijing@huawei.com>
>> To: Bjorn Helgaas <bhelgaas@google.com>, Huang Ying <ying.huang@intel.com>,
>>  Chen Gong <gong.chen@linux.intel.com>
>> CC: jiang.liu@huawei.com, Hanjun Guo <guohanjun@huawei.com>,
>>  linux-pci@vger.kernel.org, Yijing Wang <wangyijing@huawei.com>
>> Subject: [PATCH 5/6] PCI/AER: introduce pci_bus_ops_free to free pci_bus_ops
>> X-Mailer: git-send-email 1.7.11.msysgit.1
>>
>> Signed-off-by: Yijing Wang <wangyijing@huawei.com>
> 
> Your patch hasn't description.
> 
I will add description later, Thanks!

-- 
Thanks!
Yijing

Re: [PATCH 0/6] fix aer_inject bug while doing pci hot-plug

[Yijing Wang]

On 2012/9/19 15:24, Chen Gong wrote:
> On Wed, Sep 19, 2012 at 10:40:36AM +0800, Yijing Wang wrote:
>> Date: Wed, 19 Sep 2012 10:40:36 +0800
>> From: Yijing Wang <wangyijing@huawei.com>
>> To: Bjorn Helgaas <bhelgaas@google.com>, Huang Ying <ying.huang@intel.com>,
>>  Chen Gong <gong.chen@linux.intel.com>
>> CC: jiang.liu@huawei.com, Hanjun Guo <guohanjun@huawei.com>,
>>  linux-pci@vger.kernel.org, Yijing Wang <wangyijing@huawei.com>
>> Subject: [PATCH 0/6] fix aer_inject bug while doing pci hot-plug
>> X-Mailer: git-send-email 1.7.11.msysgit.1
>>
>> v2--->v3
>> 1??find the pci_ops of nearest parent bus in patch 1;
>> 2??introduce pci_bus_ops_get to avoid race condition window in aer_inject_exit;
>> 3??scan the root bus for cleaning untracked pci_ops_aer instead of scan all pci bus as Huang Ying suggestion;
>> 4??remove unused code pci_bus_ops_pop;
>>
>> Yijing Wang (6):
>>   PCI/AER: fix pci_ops return NULL when hotplug a pci bus doing aer
>>     error inject
>>   PCI/AER: introduce pci_bus_ops_get() function 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: introduce pci_bus_ops_free to free pci_bus_ops
>>   PCI/AER: clean unused code pci_bus_ops_pop
>>
>>  drivers/pci/pcie/aer/aer_inject.c |  149 ++++++++++++++++++++++++++++++++-----
>>  1 files changed, 131 insertions(+), 18 deletions(-)
>>
> After reviewing the whole patch series, I hava a feeling that you don't
> desrcibe the whole usage model very clearly. I suggest you update the
> description and express it more clearly and in detail, so other guys never touch
> aer-inject can understand it well. You patch is good but it doesn't mean
> others will like it because it is obscure to others never touching these codes.
> 
Sure, I will update the description of these patches.

-- 
Thanks!
Yijing

Re: [PATCH 1/6] PCI/AER: fix pci_ops return NULL when hotplug a pci bus doing aer error inject

[Jiang Liu]

On 2012-9-19 10:40, Yijing Wang wrote:
> When we inject aer errors to the target pcie device by aer_inject module, the pci_ops of pci
> bus which the target device is 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 devices will result to
> system panic, because it 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 |   27 ++++++++++++++++++++++++++-
>  1 files changed, 26 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/pci/pcie/aer/aer_inject.c b/drivers/pci/pcie/aer/aer_inject.c
> index 4e24cb8..0f00a27 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 upstream 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);
How about move BUG_ON(!ops)into __find_pci_bus_ops?

>  	spin_unlock_irqrestore(&inject_lock, flags);
>  	return ops->write(bus, devfn, where, size, val);
>  }
> @@ -506,6 +530,7 @@ static struct miscdevice aer_inject_device = {
>  	.fops = &aer_inject_fops,
>  };
>  
> +
Please remove this above line.

>  static int __init aer_inject_init(void)
>  {
>  	return misc_register(&aer_inject_device);