Shivaprasad G Bhat <[email protected]> writes:
hey, sorry looks like this fall through the cracks.
> The commit 1010b4c012b0 ("powerpc/eeh: Make EEH driver device hotplug
> safe") refactored the EEH code such that the pci_rescan_remove_lock is
> held at the beginning of eeh_handle_normal_event() and the
> eeh_reset_device() is called with that lock being held. Looks like the
> commit missed to remove the existing lock/unlock inside eeh_rmv_device()
> which is no longer necessary. This is causing the eehd to hang on the
> lock which it actually holds when that code path is taken.
>
> [<0>] 0xc00000011c78f870
> [<0>] __switch_to+0xfc/0x1a0
> [<0>] pci_lock_rescan_remove+0x30/0x44
> [<0>] eeh_rmv_device+0x290/0x2e0
> [<0>] eeh_pe_dev_traverse+0x80/0x130
> [<0>] eeh_reset_device+0xcc/0x23c
> [<0>] eeh_handle_normal_event+0x830/0xa80
> [<0>] eeh_event_handler+0xf8/0x190
> [<0>] kthread+0x194/0x1b0
> [<0>] start_kernel_thread+0x14/0x18
>
yup. I see eeh_handle_normal_event(), already holds this lock,
pci_lock_rescan_remove().
And eeh_rmv_device() only ever gets called from
eeh_handle_normal_event()
eeh_reset_device()
eeh_pe_dev_traverse(pe, eeh_rmv_device,...)
OR
eeh_handle_normal_event()),
eeh_pe_dev_traverse(pe, eeh_rmv_device,...)
In both the paths, eeh_handle_normal_event() holds this lock. Although I
am not an expert in eeh area - but looking at the relevant code paths,
the race looks real and this patch fixes that. So feel free to add:
Reviewed-by: Ritesh Harjani (IBM) <[email protected]>
I wonder whether clang context analyzer [1] could catch this. This is a
good candidate to try that out.
[1]: https://lore.kernel.org/lkml/[email protected]/
> The issue is seen for cases where the errors are detected on the PHB
> directly AND|OR for devices where the driver error_detected() returns
> PCI_ERS_RESULT_NEED_RESET, and driver being not EEH sensitive(i.e no
> error handlers like slot_reset(), resume() etc defined).
>
I am just wondering how did we catch this issue and whether we have some
ways to test eeh scenarios. Although the patch looks good to me.
> Fixes: 1010b4c012b0 ("powerpc/eeh: Make EEH driver device hotplug safe")
Cc: [email protected]
Let's add the above tag so that this also goes to stable trees. Since
it's been sometime since this was last posted - maybe we should rebase
and resend this with the above tag.
-ritesh
> Signed-off-by: Shivaprasad G Bhat <[email protected]>
> ---
> arch/powerpc/kernel/eeh_driver.c | 2 --
> 1 file changed, 2 deletions(-)
>
> diff --git a/arch/powerpc/kernel/eeh_driver.c
> b/arch/powerpc/kernel/eeh_driver.c
> index 028f69158532..d64cce17a4e0 100644
> --- a/arch/powerpc/kernel/eeh_driver.c
> +++ b/arch/powerpc/kernel/eeh_driver.c
> @@ -533,9 +533,7 @@ static void eeh_rmv_device(struct eeh_dev *edev, void
> *userdata)
> if (rmv_data)
> list_add(&edev->rmv_entry, &rmv_data->removed_vf_list);
> } else {
> - pci_lock_rescan_remove();
> pci_stop_and_remove_bus_device(dev);
> - pci_unlock_rescan_remove();
> }
> }
>
