* [PATCH v4 1/5] PCI/AER: Fix pci_ops return NULL in pci_read/write_aer
2012-09-21 3:44 [PATCH v4 0/5] Fix aer_inject bug caused by pci hot-plug Yijing Wang
@ 2012-09-21 3:44 ` Yijing Wang
2012-09-21 3:44 ` [PATCH v4 2/5] PCI/AER: introduce pci_bus_ops_get() to avoid a small race condition window Yijing Wang
` (3 subsequent siblings)
4 siblings, 0 replies; 12+ messages in thread
From: Yijing Wang @ 2012-09-21 3:44 UTC (permalink / raw)
To: Bjorn Helgaas, Huang Ying, Chen Gong
Cc: jiang.liu, Hanjun Guo, linux-pci, 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
^ permalink raw reply related [flat|nested] 12+ messages in thread* [PATCH v4 2/5] PCI/AER: introduce pci_bus_ops_get() to avoid a small race condition window
2012-09-21 3:44 [PATCH v4 0/5] Fix aer_inject bug caused by pci hot-plug Yijing Wang
2012-09-21 3:44 ` [PATCH v4 1/5] PCI/AER: Fix pci_ops return NULL in pci_read/write_aer Yijing Wang
@ 2012-09-21 3:44 ` Yijing Wang
2012-09-21 5:08 ` Huang Ying
2012-09-21 3:44 ` [PATCH v4 3/5] PCI/AER: clean all untracked pci_ops_aer when rmmod aer_inject Yijing Wang
` (2 subsequent siblings)
4 siblings, 1 reply; 12+ messages in thread
From: Yijing Wang @ 2012-09-21 3:44 UTC (permalink / raw)
To: Bjorn Helgaas, Huang Ying, Chen Gong
Cc: jiang.liu, Hanjun Guo, linux-pci, 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
^ permalink raw reply related [flat|nested] 12+ messages in thread* Re: [PATCH v4 2/5] PCI/AER: introduce pci_bus_ops_get() to avoid a small race condition window
2012-09-21 3:44 ` [PATCH v4 2/5] PCI/AER: introduce pci_bus_ops_get() to avoid a small race condition window Yijing Wang
@ 2012-09-21 5:08 ` Huang Ying
2012-09-21 6:01 ` Yijing Wang
0 siblings, 1 reply; 12+ messages in thread
From: Huang Ying @ 2012-09-21 5:08 UTC (permalink / raw)
To: Yijing Wang; +Cc: Bjorn Helgaas, Chen Gong, jiang.liu, Hanjun Guo, linux-pci
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) {
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH v4 2/5] PCI/AER: introduce pci_bus_ops_get() to avoid a small race condition window
2012-09-21 5:08 ` Huang Ying
@ 2012-09-21 6:01 ` Yijing Wang
0 siblings, 0 replies; 12+ messages in thread
From: Yijing Wang @ 2012-09-21 6:01 UTC (permalink / raw)
To: Huang Ying; +Cc: Bjorn Helgaas, Chen Gong, jiang.liu, Hanjun Guo, linux-pci
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH v4 3/5] PCI/AER: clean all untracked pci_ops_aer when rmmod aer_inject
2012-09-21 3:44 [PATCH v4 0/5] Fix aer_inject bug caused by pci hot-plug Yijing Wang
2012-09-21 3:44 ` [PATCH v4 1/5] PCI/AER: Fix pci_ops return NULL in pci_read/write_aer Yijing Wang
2012-09-21 3:44 ` [PATCH v4 2/5] PCI/AER: introduce pci_bus_ops_get() to avoid a small race condition window Yijing Wang
@ 2012-09-21 3:44 ` Yijing Wang
2012-09-21 5:13 ` Huang Ying
2012-09-21 3:44 ` [PATCH v4 4/5] PCI/AER: clean pci_bus_ops when related pci bus was removed Yijing Wang
2012-09-21 3:44 ` [PATCH v4 5/5] PCI/AER: free pci_bus_ops_list and remove pci_bus_ops_pop Yijing Wang
4 siblings, 1 reply; 12+ messages in thread
From: Yijing Wang @ 2012-09-21 3:44 UTC (permalink / raw)
To: Bjorn Helgaas, Huang Ying, Chen Gong
Cc: jiang.liu, Hanjun Guo, linux-pci, 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
^ permalink raw reply related [flat|nested] 12+ messages in thread* Re: [PATCH v4 3/5] PCI/AER: clean all untracked pci_ops_aer when rmmod aer_inject
2012-09-21 3:44 ` [PATCH v4 3/5] PCI/AER: clean all untracked pci_ops_aer when rmmod aer_inject Yijing Wang
@ 2012-09-21 5:13 ` Huang Ying
2012-09-21 5:47 ` Yijing Wang
0 siblings, 1 reply; 12+ messages in thread
From: Huang Ying @ 2012-09-21 5:13 UTC (permalink / raw)
To: Yijing Wang; +Cc: Bjorn Helgaas, Chen Gong, jiang.liu, Hanjun Guo, linux-pci
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);
>
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH v4 3/5] PCI/AER: clean all untracked pci_ops_aer when rmmod aer_inject
2012-09-21 5:13 ` Huang Ying
@ 2012-09-21 5:47 ` Yijing Wang
2012-09-21 6:15 ` Huang Ying
0 siblings, 1 reply; 12+ messages in thread
From: Yijing Wang @ 2012-09-21 5:47 UTC (permalink / raw)
To: Huang Ying; +Cc: Bjorn Helgaas, Chen Gong, jiang.liu, Hanjun Guo, linux-pci
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
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH v4 3/5] PCI/AER: clean all untracked pci_ops_aer when rmmod aer_inject
2012-09-21 5:47 ` Yijing Wang
@ 2012-09-21 6:15 ` Huang Ying
0 siblings, 0 replies; 12+ messages in thread
From: Huang Ying @ 2012-09-21 6:15 UTC (permalink / raw)
To: Yijing Wang; +Cc: Bjorn Helgaas, Chen Gong, jiang.liu, Hanjun Guo, linux-pci
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
> >
> > .
> >
>
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH v4 4/5] PCI/AER: clean pci_bus_ops when related pci bus was removed
2012-09-21 3:44 [PATCH v4 0/5] Fix aer_inject bug caused by pci hot-plug Yijing Wang
` (2 preceding siblings ...)
2012-09-21 3:44 ` [PATCH v4 3/5] PCI/AER: clean all untracked pci_ops_aer when rmmod aer_inject Yijing Wang
@ 2012-09-21 3:44 ` Yijing Wang
2012-09-21 3:44 ` [PATCH v4 5/5] PCI/AER: free pci_bus_ops_list and remove pci_bus_ops_pop Yijing Wang
4 siblings, 0 replies; 12+ messages in thread
From: Yijing Wang @ 2012-09-21 3:44 UTC (permalink / raw)
To: Bjorn Helgaas, Huang Ying, Chen Gong
Cc: jiang.liu, Hanjun Guo, linux-pci, 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
^ permalink raw reply related [flat|nested] 12+ messages in thread* [PATCH v4 5/5] PCI/AER: free pci_bus_ops_list and remove pci_bus_ops_pop
2012-09-21 3:44 [PATCH v4 0/5] Fix aer_inject bug caused by pci hot-plug Yijing Wang
` (3 preceding siblings ...)
2012-09-21 3:44 ` [PATCH v4 4/5] PCI/AER: clean pci_bus_ops when related pci bus was removed Yijing Wang
@ 2012-09-21 3:44 ` Yijing Wang
2012-09-21 5:15 ` Huang Ying
4 siblings, 1 reply; 12+ messages in thread
From: Yijing Wang @ 2012-09-21 3:44 UTC (permalink / raw)
To: Bjorn Helgaas, Huang Ying, Chen Gong
Cc: jiang.liu, Hanjun Guo, linux-pci, 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
^ permalink raw reply related [flat|nested] 12+ messages in thread* Re: [PATCH v4 5/5] PCI/AER: free pci_bus_ops_list and remove pci_bus_ops_pop
2012-09-21 3:44 ` [PATCH v4 5/5] PCI/AER: free pci_bus_ops_list and remove pci_bus_ops_pop Yijing Wang
@ 2012-09-21 5:15 ` Huang Ying
0 siblings, 0 replies; 12+ messages in thread
From: Huang Ying @ 2012-09-21 5:15 UTC (permalink / raw)
To: Yijing Wang; +Cc: Bjorn Helgaas, Chen Gong, jiang.liu, Hanjun Guo, linux-pci
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) {
^ permalink raw reply [flat|nested] 12+ messages in thread