* [RFC PATCH 1/2] PCI: correctly flush workqueue when destroy pcie hotplug controller
@ 2012-04-07 18:18 Jiang Liu
2012-04-07 18:18 ` [RFC PATCH 2/2] PCI: fix four race windows in shpchp driver Jiang Liu
2012-04-10 10:43 ` [RFC PATCH 1/2] PCI: correctly flush workqueue when destroy pcie hotplug controller Kenji Kaneshige
0 siblings, 2 replies; 4+ messages in thread
From: Jiang Liu @ 2012-04-07 18:18 UTC (permalink / raw)
To: Bjorn Helgaas, Yinghai Lu, Kenji Kaneshige
Cc: Jiang Liu, matsumoto.hiroo, Jiang Liu, Keping Chen, linux-kernel,
linux-pci
When destroying a PCIe hotplug controller, all work items associated with
that controller should be flushed. Function pcie_cleanup_slot() calls
cancel_delayed_work() and flush_workqueue() to achieve that.
Function flush_workqueue() will flush all work items already submitted,
but new work items submitted by those already submitted work items may
still be in live state when returning from flush_workqueue().
For the extreme case, pciehp driver may expierence following calling path:
1) pcie_isr() -> pciehp_handle_xxx() -> queue_interrupt_event()->queue_work()
2) interrupt_event_handler() -> handle_button_press_event() ->
queue_delayed_work()
3) pciehp_queue_pushbutton_work() -> queue_work()
So enhance pcie_cleanup_slot() to correctly flush workqueue when destroying
PCIe hotplug controller.
Signed-off-by: Jiang Liu <jiang.liu@huawei.com>
---
drivers/pci/hotplug/pciehp_hpc.c | 13 ++++++++++++-
1 files changed, 12 insertions(+), 1 deletions(-)
diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c
index d4fa705..9a48a51 100644
--- a/drivers/pci/hotplug/pciehp_hpc.c
+++ b/drivers/pci/hotplug/pciehp_hpc.c
@@ -890,8 +890,19 @@ static int pcie_init_slot(struct controller *ctrl)
static void pcie_cleanup_slot(struct controller *ctrl)
{
struct slot *slot = ctrl->slot;
- cancel_delayed_work(&slot->work);
+
+ /*
+ * Following workqueue flushing logic is to deal with the special call path:
+ * 1) pcie_isr() -> pciehp_handle_xxx() ->
+ * queue_interrupt_event(pciehp_wq_event)->queue_work(pciehp_wq)
+ * 2) interrupt_event_handler() -> handle_button_press_event() ->
+ * queue_delayed_work(pciehp_wq)
+ * 3) pciehp_queue_pushbutton_work() -> queue_work(pciehp_wq)
+ */
flush_workqueue(pciehp_wq);
+ cancel_delayed_work_sync(&slot->work);
+ flush_workqueue(pciehp_wq);
+
kfree(slot);
}
--
1.7.5.4
^ permalink raw reply related [flat|nested] 4+ messages in thread
* [RFC PATCH 2/2] PCI: fix four race windows in shpchp driver
2012-04-07 18:18 [RFC PATCH 1/2] PCI: correctly flush workqueue when destroy pcie hotplug controller Jiang Liu
@ 2012-04-07 18:18 ` Jiang Liu
2012-04-10 10:43 ` [RFC PATCH 1/2] PCI: correctly flush workqueue when destroy pcie hotplug controller Kenji Kaneshige
1 sibling, 0 replies; 4+ messages in thread
From: Jiang Liu @ 2012-04-07 18:18 UTC (permalink / raw)
To: Bjorn Helgaas, Yinghai Lu, Kenji Kaneshige
Cc: Jiang Liu, matsumoto.hiroo, Jiang Liu, Keping Chen, linux-kernel,
linux-pci
With current shpchp implementation, interrupt is enabled before corresponding
slot data structures are created. If interrupt happens immediately after
enabling the hotplug interrupt, shpchp_find_slot() may return NULL, thus
causes NULL pointer dereference. So create slot data structures before
enabling interrupt.
The second one is, shpc_isr() may cause invalid memory access if it walks
the slot list when cleanup_slots() is modifying the list. So only call
cleanup_slots() after disabling interrupt/removing the poll timer.
The third one is, del_timer() can't reliabily remove the poll timer, so
use del_timer_sync() instead.
The fourth one is, cleanup_slots() can't reliabily flush all work items.
So tune the workqueue flushing logic to reliabily flush all work items.
Signed-off-by: Jiang Liu <jiang.liu@huawei.com>
---
Hi all,
These issues are identified by code analysis and I have no hardware
platform to verify the patches. Could anybody give me a hand here to test
these patches?
Thanks!
Gerry
---
drivers/pci/hotplug/shpchp.h | 1 +
drivers/pci/hotplug/shpchp_core.c | 8 ++++----
drivers/pci/hotplug/shpchp_hpc.c | 36 +++++++++++++++++++++---------------
3 files changed, 26 insertions(+), 19 deletions(-)
diff --git a/drivers/pci/hotplug/shpchp.h b/drivers/pci/hotplug/shpchp.h
index ca64932..6691dc4 100644
--- a/drivers/pci/hotplug/shpchp.h
+++ b/drivers/pci/hotplug/shpchp.h
@@ -182,6 +182,7 @@ extern int shpchp_unconfigure_device(struct slot *p_slot);
extern void cleanup_slots(struct controller *ctrl);
extern void shpchp_queue_pushbutton_work(struct work_struct *work);
extern int shpc_init( struct controller *ctrl, struct pci_dev *pdev);
+extern void shpc_enable_intr(struct controller *ctrl);
static inline const char *slot_name(struct slot *slot)
{
diff --git a/drivers/pci/hotplug/shpchp_core.c b/drivers/pci/hotplug/shpchp_core.c
index 7414fd9..19762b3 100644
--- a/drivers/pci/hotplug/shpchp_core.c
+++ b/drivers/pci/hotplug/shpchp_core.c
@@ -173,8 +173,8 @@ void cleanup_slots(struct controller *ctrl)
list_for_each_safe(tmp, next, &ctrl->slot_list) {
slot = list_entry(tmp, struct slot, slot_list);
list_del(&slot->slot_list);
- cancel_delayed_work(&slot->work);
flush_workqueue(shpchp_wq);
+ cancel_delayed_work_sync(&slot->work);
flush_workqueue(shpchp_ordered_wq);
pci_hp_deregister(slot->hotplug_slot);
}
@@ -318,14 +318,14 @@ static int shpc_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
goto err_out_release_ctlr;
}
+ shpc_enable_intr(ctrl);
+
rc = shpchp_create_ctrl_files(ctrl);
if (rc)
- goto err_cleanup_slots;
+ goto err_out_release_ctlr;
return 0;
-err_cleanup_slots:
- cleanup_slots(ctrl);
err_out_release_ctlr:
ctrl->hpc_ops->release_ctlr(ctrl);
err_out_free_ctrl:
diff --git a/drivers/pci/hotplug/shpchp_hpc.c b/drivers/pci/hotplug/shpchp_hpc.c
index 75ba231..ff63887 100644
--- a/drivers/pci/hotplug/shpchp_hpc.c
+++ b/drivers/pci/hotplug/shpchp_hpc.c
@@ -592,6 +592,13 @@ static void hpc_release_ctlr(struct controller *ctrl)
shpc_writel(ctrl, SLOT_REG(i), slot_reg);
}
+ if (shpchp_poll_mode)
+ del_timer_sync(&ctrl->poll_timer);
+ else {
+ free_irq(ctrl->pci_dev->irq, ctrl);
+ pci_disable_msi(ctrl->pci_dev);
+ }
+
cleanup_slots(ctrl);
/*
@@ -603,13 +610,6 @@ static void hpc_release_ctlr(struct controller *ctrl)
serr_int &= ~SERR_INTR_RSVDZ_MASK;
shpc_writel(ctrl, SERR_INTR_ENABLE, serr_int);
- if (shpchp_poll_mode)
- del_timer(&ctrl->poll_timer);
- else {
- free_irq(ctrl->pci_dev->irq, ctrl);
- pci_disable_msi(ctrl->pci_dev);
- }
-
iounmap(ctrl->creg);
release_mem_region(ctrl->mmio_base, ctrl->mmio_size);
}
@@ -1081,6 +1081,20 @@ int shpc_init(struct controller *ctrl, struct pci_dev *pdev)
shpc_get_max_bus_speed(ctrl);
shpc_get_cur_bus_speed(ctrl);
+ return 0;
+
+ /* We end up here for the many possible ways to fail this API. */
+abort_iounmap:
+ iounmap(ctrl->creg);
+abort:
+ return rc;
+}
+
+void shpc_enable_intr(struct controller *ctrl)
+{
+ u8 hp_slot;
+ u32 tempdword, slot_reg;
+
/*
* Unmask all event interrupts of all slots
*/
@@ -1102,12 +1116,4 @@ int shpc_init(struct controller *ctrl, struct pci_dev *pdev)
tempdword = shpc_readl(ctrl, SERR_INTR_ENABLE);
ctrl_dbg(ctrl, "SERR_INTR_ENABLE = %x\n", tempdword);
}
-
- return 0;
-
- /* We end up here for the many possible ways to fail this API. */
-abort_iounmap:
- iounmap(ctrl->creg);
-abort:
- return rc;
}
--
1.7.5.4
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [RFC PATCH 1/2] PCI: correctly flush workqueue when destroy pcie hotplug controller
2012-04-07 18:18 [RFC PATCH 1/2] PCI: correctly flush workqueue when destroy pcie hotplug controller Jiang Liu
2012-04-07 18:18 ` [RFC PATCH 2/2] PCI: fix four race windows in shpchp driver Jiang Liu
@ 2012-04-10 10:43 ` Kenji Kaneshige
2012-04-25 17:23 ` Bjorn Helgaas
1 sibling, 1 reply; 4+ messages in thread
From: Kenji Kaneshige @ 2012-04-10 10:43 UTC (permalink / raw)
To: Jiang Liu
Cc: Bjorn Helgaas, Yinghai Lu, Jiang Liu, matsumoto.hiroo,
Keping Chen, linux-kernel, linux-pci
I think you're rignt.
Reviewed-by: Kenji Kaneshige <kaneshige.kenji@jp.fujitsu.com>
(2012/04/08 3:18), Jiang Liu wrote:
> When destroying a PCIe hotplug controller, all work items associated with
> that controller should be flushed. Function pcie_cleanup_slot() calls
> cancel_delayed_work() and flush_workqueue() to achieve that.
> Function flush_workqueue() will flush all work items already submitted,
> but new work items submitted by those already submitted work items may
> still be in live state when returning from flush_workqueue().
>
> For the extreme case, pciehp driver may expierence following calling path:
> 1) pcie_isr() -> pciehp_handle_xxx() -> queue_interrupt_event()->queue_work()
> 2) interrupt_event_handler() -> handle_button_press_event() ->
> queue_delayed_work()
> 3) pciehp_queue_pushbutton_work() -> queue_work()
>
> So enhance pcie_cleanup_slot() to correctly flush workqueue when destroying
> PCIe hotplug controller.
>
> Signed-off-by: Jiang Liu<jiang.liu@huawei.com>
> ---
> drivers/pci/hotplug/pciehp_hpc.c | 13 ++++++++++++-
> 1 files changed, 12 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c
> index d4fa705..9a48a51 100644
> --- a/drivers/pci/hotplug/pciehp_hpc.c
> +++ b/drivers/pci/hotplug/pciehp_hpc.c
> @@ -890,8 +890,19 @@ static int pcie_init_slot(struct controller *ctrl)
> static void pcie_cleanup_slot(struct controller *ctrl)
> {
> struct slot *slot = ctrl->slot;
> - cancel_delayed_work(&slot->work);
> +
> + /*
> + * Following workqueue flushing logic is to deal with the special call path:
> + * 1) pcie_isr() -> pciehp_handle_xxx() ->
> + * queue_interrupt_event(pciehp_wq_event)->queue_work(pciehp_wq)
> + * 2) interrupt_event_handler() -> handle_button_press_event() ->
> + * queue_delayed_work(pciehp_wq)
> + * 3) pciehp_queue_pushbutton_work() -> queue_work(pciehp_wq)
> + */
> flush_workqueue(pciehp_wq);
> + cancel_delayed_work_sync(&slot->work);
> + flush_workqueue(pciehp_wq);
> +
> kfree(slot);
> }
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [RFC PATCH 1/2] PCI: correctly flush workqueue when destroy pcie hotplug controller
2012-04-10 10:43 ` [RFC PATCH 1/2] PCI: correctly flush workqueue when destroy pcie hotplug controller Kenji Kaneshige
@ 2012-04-25 17:23 ` Bjorn Helgaas
0 siblings, 0 replies; 4+ messages in thread
From: Bjorn Helgaas @ 2012-04-25 17:23 UTC (permalink / raw)
To: Kenji Kaneshige
Cc: Jiang Liu, Yinghai Lu, Jiang Liu, matsumoto.hiroo, Keping Chen,
linux-kernel, linux-pci
On Tue, Apr 10, 2012 at 4:43 AM, Kenji Kaneshige
<kaneshige.kenji@jp.fujitsu.com> wrote:
> I think you're rignt.
>
> Reviewed-by: Kenji Kaneshige <kaneshige.kenji@jp.fujitsu.com>
>
> (2012/04/08 3:18), Jiang Liu wrote:
>> When destroying a PCIe hotplug controller, all work items associated with
>> that controller should be flushed. Function pcie_cleanup_slot() calls
>> cancel_delayed_work() and flush_workqueue() to achieve that.
>> Function flush_workqueue() will flush all work items already submitted,
>> but new work items submitted by those already submitted work items may
>> still be in live state when returning from flush_workqueue().
>>
>> For the extreme case, pciehp driver may expierence following calling path:
>> 1) pcie_isr() -> pciehp_handle_xxx() -> queue_interrupt_event()->queue_work()
>> 2) interrupt_event_handler() -> handle_button_press_event() ->
>> queue_delayed_work()
>> 3) pciehp_queue_pushbutton_work() -> queue_work()
>>
>> So enhance pcie_cleanup_slot() to correctly flush workqueue when destroying
>> PCIe hotplug controller.
>>
>> Signed-off-by: Jiang Liu<jiang.liu@huawei.com>
>> ---
>> drivers/pci/hotplug/pciehp_hpc.c | 13 ++++++++++++-
>> 1 files changed, 12 insertions(+), 1 deletions(-)
>>
>> diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c
>> index d4fa705..9a48a51 100644
>> --- a/drivers/pci/hotplug/pciehp_hpc.c
>> +++ b/drivers/pci/hotplug/pciehp_hpc.c
>> @@ -890,8 +890,19 @@ static int pcie_init_slot(struct controller *ctrl)
>> static void pcie_cleanup_slot(struct controller *ctrl)
>> {
>> struct slot *slot = ctrl->slot;
>> - cancel_delayed_work(&slot->work);
>> +
>> + /*
>> + * Following workqueue flushing logic is to deal with the special call path:
>> + * 1) pcie_isr() -> pciehp_handle_xxx() ->
>> + * queue_interrupt_event(pciehp_wq_event)->queue_work(pciehp_wq)
>> + * 2) interrupt_event_handler() -> handle_button_press_event() ->
>> + * queue_delayed_work(pciehp_wq)
>> + * 3) pciehp_queue_pushbutton_work() -> queue_work(pciehp_wq)
>> + */
>> flush_workqueue(pciehp_wq);
>> + cancel_delayed_work_sync(&slot->work);
>> + flush_workqueue(pciehp_wq);
>> +
>> kfree(slot);
>> }
Jiang, I'm ignoring this (and the next [2/2] patch) because I assume
they're superceded by the "[PATCH RFC 00/17] Introduce a global lock
to serialize all PCI hotplug" series of April 16. Let me know if
otherwise.
Bjorn
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2012-04-25 17:23 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-04-07 18:18 [RFC PATCH 1/2] PCI: correctly flush workqueue when destroy pcie hotplug controller Jiang Liu
2012-04-07 18:18 ` [RFC PATCH 2/2] PCI: fix four race windows in shpchp driver Jiang Liu
2012-04-10 10:43 ` [RFC PATCH 1/2] PCI: correctly flush workqueue when destroy pcie hotplug controller Kenji Kaneshige
2012-04-25 17:23 ` Bjorn Helgaas
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.