@@ -165,7 +165,7 @@ void pciehp_request(struct controller *ctrl, int action);
void pciehp_handle_button_press(struct controller *ctrl);
void pciehp_handle_disable_request(struct controller *ctrl);
void pciehp_handle_presence_or_link_change(struct controller *ctrl, u32 events);
-int pciehp_configure_device(struct controller *ctrl);
+int pciehp_configure_device(struct controller *ctrl, u64 *dsn);
void pciehp_unconfigure_device(struct controller *ctrl, bool presence);
void pciehp_queue_pushbutton_work(struct work_struct *work);
struct controller *pcie_init(struct pcie_device *dev);
@@ -59,6 +59,7 @@ static void set_slot_off(struct controller *ctrl)
static int board_added(struct controller *ctrl)
{
int retval = 0;
+ u64 dsn = 0;
struct pci_bus *parent = ctrl->pcie->port->subordinate;
if (POWER_CTRL(ctrl)) {
@@ -83,13 +84,21 @@ static int board_added(struct controller *ctrl)
goto err_exit;
}
- retval = pciehp_configure_device(ctrl);
+ /*
+ * Release reset_lock during driver binding
+ * to avoid AB-BA deadlock with device_lock.
+ */
+ up_read(&ctrl->reset_lock);
+ retval = pciehp_configure_device(ctrl, &dsn);
+ down_read_nested(&ctrl->reset_lock, ctrl->depth);
if (retval) {
if (retval != -EEXIST) {
ctrl_err(ctrl, "Cannot add device at %04x:%02x:00\n",
pci_domain_nr(parent), parent->number);
goto err_exit;
}
+ } else {
+ ctrl->dsn = dsn;
}
pciehp_set_indicators(ctrl, PCI_EXP_SLTCTL_PWR_IND_ON,
@@ -24,12 +24,14 @@
/**
* pciehp_configure_device() - enumerate PCI devices below a hotplug bridge
* @ctrl: PCIe hotplug controller
+ * @dsn: Device Serial Number of Function 0 in the hotplug slot
*
* Enumerate PCI devices below a hotplug bridge and add them to the system.
+ * On success store the Device Serial Number of Function 0 in @dsn.
* Return 0 on success, %-EEXIST if the devices are already enumerated or
* %-ENODEV if enumeration failed.
*/
-int pciehp_configure_device(struct controller *ctrl)
+int pciehp_configure_device(struct controller *ctrl, u64 *dsn)
{
struct pci_dev *dev;
struct pci_dev *bridge = ctrl->pcie->port;
@@ -64,16 +66,10 @@ int pciehp_configure_device(struct controller *ctrl)
pci_assign_unassigned_bridge_resources(bridge);
pcie_bus_configure_settings(parent);
- /*
- * Release reset_lock during driver binding
- * to avoid AB-BA deadlock with device_lock.
- */
- up_read(&ctrl->reset_lock);
pci_bus_add_devices(parent);
- down_read_nested(&ctrl->reset_lock, ctrl->depth);
dev = pci_get_slot(parent, PCI_DEVFN(0, 0));
- ctrl->dsn = pci_get_dsn(dev);
+ *dsn = pci_get_dsn(dev);
pci_dev_put(dev);
out:
Hej, I stumbled over this lockdep splat during pci hotplug: [ 26.016648] ====================================================== [ 26.019646] WARNING: possible circular locking dependency detected [ 26.022785] 6.12.0-rc6+ #176 Not tainted [ 26.024776] ------------------------------------------------------ [ 26.027909] irq/50-pciehp/57 is trying to acquire lock: [ 26.030559] ffff0000c02ad700 (&ctrl->reset_lock){.+.+}-{3:3}, at: pciehp_configure_device+0xe4/0x1a0 [ 26.035423] [ 26.035423] but task is already holding lock: [ 26.038505] ffff800082f819f8 (pci_rescan_remove_lock){+.+.}-{3:3}, at: pci_lock_rescan_remove+0x24/0x38 [ 26.043512] [ 26.043512] which lock already depends on the new lock. [ 26.043512] [ 26.047863] [ 26.047863] the existing dependency chain (in reverse order) is: [ 26.051823] [ 26.051823] -> #1 (pci_rescan_remove_lock){+.+.}-{3:3}: [ 26.056209] __mutex_lock+0x90/0x3a0 [ 26.057946] mutex_lock_nested+0x2c/0x40 [ 26.059848] pci_lock_rescan_remove+0x24/0x38 [ 26.062560] pciehp_configure_device+0x48/0x1a0 [ 26.065592] pciehp_handle_presence_or_link_change+0x1e0/0x4a0 [ 26.069044] pciehp_ist+0x21c/0x268 [ 26.071186] irq_thread_fn+0x34/0xb8 [ 26.073368] irq_thread+0x154/0x2d0 [ 26.075503] kthread+0x108/0x120 [ 26.077504] ret_from_fork+0x10/0x20 [ 26.079695] [ 26.079695] -> #0 (&ctrl->reset_lock){.+.+}-{3:3}: [ 26.083164] __lock_acquire+0x12bc/0x1eb8 [ 26.085592] lock_acquire+0x1e0/0x358 [ 26.087831] down_read_nested+0x54/0x160 [ 26.090198] pciehp_configure_device+0xe4/0x1a0 [ 26.092895] pciehp_handle_presence_or_link_change+0x1e0/0x4a0 [ 26.096225] pciehp_ist+0x21c/0x268 [ 26.098337] irq_thread_fn+0x34/0xb8 [ 26.100509] irq_thread+0x154/0x2d0 [ 26.102668] kthread+0x108/0x120 [ 26.104660] ret_from_fork+0x10/0x20 [ 26.106790] [ 26.106790] other info that might help us debug this: [ 26.106790] [ 26.111033] Possible unsafe locking scenario: [ 26.111033] [ 26.114184] CPU0 CPU1 [ 26.116607] ---- ---- [ 26.119023] lock(pci_rescan_remove_lock); [ 26.121776] lock(&ctrl->reset_lock); [ 26.123924] lock(pci_rescan_remove_lock); [ 26.126098] rlock(&ctrl->reset_lock); [ 26.127349] [ 26.127349] *** DEADLOCK *** [ 26.127349] [ 26.129274] 1 lock held by irq/50-pciehp/57: [ 26.130664] #0: ffff800082f819f8 (pci_rescan_remove_lock){+.+.}-{3:3}, at: pci_lock_rescan_remove+0x24/0x38 [ 26.135941] [ 26.135941] stack backtrace: [ 26.138240] CPU: 0 UID: 0 PID: 57 Comm: irq/50-pciehp Not tainted 6.12.0-rc6+ #176 [ 26.142223] Hardware name: QEMU KVM Virtual Machine, BIOS edk2-20230524-3.fc37 05/24/2023 [ 26.146514] Call trace: [ 26.147795] dump_backtrace+0xa4/0x130 [ 26.149759] show_stack+0x20/0x38 [ 26.151504] dump_stack_lvl+0x90/0xd0 [ 26.153429] dump_stack+0x18/0x28 [ 26.155209] print_circular_bug+0x28c/0x370 [ 26.157601] check_noncircular+0x140/0x150 [ 26.159830] __lock_acquire+0x12bc/0x1eb8 [ 26.161949] lock_acquire+0x1e0/0x358 [ 26.163990] down_read_nested+0x54/0x160 [ 26.166172] pciehp_configure_device+0xe4/0x1a0 [ 26.168586] pciehp_handle_presence_or_link_change+0x1e0/0x4a0 [ 26.171755] pciehp_ist+0x21c/0x268 [ 26.173672] irq_thread_fn+0x34/0xb8 [ 26.175523] irq_thread+0x154/0x2d0 [ 26.177336] kthread+0x108/0x120 [ 26.179000] ret_from_fork+0x10/0x20 I don't think that this could actually happen since this is only called by a single irq thread but this splat is kinda annoying and pciehp_configure_device() doesn't seem to do much that needs the reset_lock. How about this? ---->8 [PATCH] pciehp: fix lockdep warning Call pciehp_configure_device() without reset_lock being held to fix the following lockdep warning. The only action that seems to require the reset_lock is writing to ctrl->dsn, so move that to the caller that holds the lock. [ 26.019646] WARNING: possible circular locking dependency detected [ 26.022785] 6.12.0-rc6+ #176 Not tainted [ 26.024776] ------------------------------------------------------ [ 26.027909] irq/50-pciehp/57 is trying to acquire lock: [ 26.030559] ffff0000c02ad700 (&ctrl->reset_lock){.+.+}-{3:3}, at: pciehp_configure_device+0xe4/0x1a0 [ 26.035423] [ 26.035423] but task is already holding lock: [ 26.038505] ffff800082f819f8 (pci_rescan_remove_lock){+.+.}-{3:3}, at: pci_lock_rescan_remove+0x24/0x38 [ 26.043512] [ 26.043512] which lock already depends on the new lock. Signed-off-by: Sebastian Ott <sebott@redhat.com> --- drivers/pci/hotplug/pciehp.h | 2 +- drivers/pci/hotplug/pciehp_ctrl.c | 11 ++++++++++- drivers/pci/hotplug/pciehp_pci.c | 12 ++++-------- 3 files changed, 15 insertions(+), 10 deletions(-)