Hi Ritesh,
Thanks for the review.
On 7/8/26 8:36 PM, Ritesh Harjani (IBM) wrote:
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]/
I did give it a try.
https://gist.github.com/shivaprasadbhat/473ea8bcb63afda669bcb5de24ac22f8
It couldnt catch it. May need some more work.
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.
Two ways to recreate, Mostly with drivers not having eeh_handlers like
slot_reset(), resume defined.
- On Baremetal, eeh on multi-function device bound to vfio.
- On PowerVM, eeh on a device behind a switch or IO drawer.
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.
Sure.
Thanks,
Shivaprasad
-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();
}
}
