FAILED: patch "[PATCH] PCI/ASPM: Fix deadlock when enabling ASPM" failed to apply to 6.1-stable tree
Subsystems (5)
[email protected]
maintainerThe patch below does not apply to the 6.1-stable tree.
If someone wants it applied there, or to any other stable or longterm tree, then please email the backport, including the original git commit id to <[email protected]>.
To reproduce the conflict and resubmit, you may use the following commands:
git fetch https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/ linux-6.1.y
git checkout FETCH_HEAD
git cherry-pick -x 1e560864159d002b453da42bd2c13a1805515a20
# <resolve conflicts, build, test, etc.>
git commit -s
git send-email --to '<[email protected]>' --in-reply-to '2024021334-each-residence-41ce@gregkh' --subject-prefix 'PATCH 6.1.y' HEAD^..
Possible dependencies:
1e560864159d ("PCI/ASPM: Fix deadlock when enabling ASPM")ac865f00af29 ("Merge tag 'pci-v6.7-fixes-2' of git://git.kernel.org/pub/scm/linux/kernel/git/pci/pci")
thanks,
greg k-h
------------------ original commit in Linus's tree ------------------
From 1e560864159d002b453da42bd2c13a1805515a20 Mon Sep 17 00:00:00 2001
From: Johan Hovold <[email protected]>
Date: Tue, 30 Jan 2024 11:02:43 +0100
Subject: [PATCH] PCI/ASPM: Fix deadlock when enabling ASPM
A last minute revert in 6.7-final introduced a potential deadlock when enabling ASPM during probe of Qualcomm PCIe controllers as reported by lockdep:
============================================
WARNING: possible recursive locking detected
6.7.0 #40 Not tainted
--------------------------------------------
kworker/u16:5/90 is trying to acquire lock:
ffffacfa78ced000 (pci_bus_sem){++++}-{3:3}, at: pcie_aspm_pm_state_change+0x58/0xdc
but task is already holding lock:
ffffacfa78ced000 (pci_bus_sem){++++}-{3:3}, at: pci_walk_bus+0x34/0xbc
other info that might help us debug this:
Possible unsafe locking scenario:
CPU0
----
lock(pci_bus_sem);
lock(pci_bus_sem);
*** DEADLOCK ***Call trace: print_deadlock_bug+0x25c/0x348 __lock_acquire+0x10a4/0x2064 lock_acquire+0x1e8/0x318 down_read+0x60/0x184 pcie_aspm_pm_state_change+0x58/0xdc pci_set_full_power_state+0xa8/0x114 pci_set_power_state+0xc4/0x120 qcom_pcie_enable_aspm+0x1c/0x3c [pcie_qcom] pci_walk_bus+0x64/0xbc qcom_pcie_host_post_init_2_7_0+0x28/0x34 [pcie_qcom]
The deadlock can easily be reproduced on machines like the Lenovo ThinkPad X13s by adding a delay to increase the race window during asynchronous probe where another thread can take a write lock.
Add a new pci_set_power_state_locked() and associated helper functions that can be called with the PCI bus semaphore held to avoid taking the read lock twice.
drivers/pci/bus.c +32 -17
@@ -386,21 +386,8 @@ void pci_bus_add_devices(const struct pci_bus *bus) }EXPORT_SYMBOL (pci_bus_add_devices); -/** pci_walk_bus - walk devices on/under bus, calling callback. -* @top bus whose devices should be walked -* @cb callback to be called for each device found -* @userdata arbitrary pointer to be passed to callback. -* -* Walk the given bus, including any bridged devices -* on buses under this bus. Call the provided callback -* on each device found. -* -* We check the return of @cb each time. If it returns anything -* other than 0, we break out. -* -*/ -void pci_walk_bus (struct pci_bus *top,int (*cb)(struct pci_dev *,void *), -void *userdata) +static void __pci_walk_bus (struct pci_bus *top,int (*cb)(struct pci_dev *,void *), +void *userdata,bool locked) {struct pci_dev *dev;struct pci_bus *bus;
@@ -408,7 +395,8 @@ void pci_walk_bus(struct pci_bus *top, int (*cb)(struct pci_dev *, void *),int retval; bus = top; -down_read (&pci_bus_sem); +if (!locked) +down_read (&pci_bus_sem); next = top->devices.next;for (;;) {if (next == &bus->devices) {
@@ -431,10 +419,37 @@ void pci_walk_bus(struct pci_bus *top, int (*cb)(struct pci_dev *, void *),if (retval)break ; } -up_read (&pci_bus_sem); +if (!locked) +up_read (&pci_bus_sem); +} + +/** +* pci_walk_bus - walk devices on/under bus, calling callback. +* @top: bus whose devices should be walked +* @cb: callback to be called for each device found +* @userdata: arbitrary pointer to be passed to callback +* +* Walk the given bus, including any bridged devices +* on buses under this bus. Call the provided callback +* on each device found. +* +* We check the return of @cb each time. If it returns anything +* other than 0, we break out. +*/ +void pci_walk_bus (struct pci_bus *top,int (*cb)(struct pci_dev *,void *),void *userdata) +{ +__pci_walk_bus (top, cb, userdata, false); }EXPORT_SYMBOL_GPL (pci_walk_bus); +void pci_walk_bus_locked (struct pci_bus *top,int (*cb)(struct pci_dev *,void *),void *userdata) +{ +lockdep_assert_held (&pci_bus_sem); + +__pci_walk_bus (top, cb, userdata, true); +} +EXPORT_SYMBOL_GPL (pci_walk_bus_locked); +struct pci_bus *pci_bus_get (struct pci_bus *bus) {if (bus)
drivers/pci/controller/dwc/pcie-qcom.c +1 -1
@@ -972,7 +972,7 @@ static int qcom_pcie_enable_aspm(struct pci_dev *pdev, void *userdata)* Downstream devices need to be in D0 state before enabling PCI PM * substates. */ -pci_set_power_state (pdev,PCI_D0 ); +pci_set_power_state_locked (pdev,PCI_D0 );pci_enable_link_state_locked (pdev,PCIE_LINK_STATE_ALL );return 0 ;
drivers/pci/pci.c +52 -26
149 lines not on this page · raw message
drivers/pci/pci.h +2 -2
@@ -571,12 +571,12 @@ int pcie_retrain_link(struct pci_dev *pdev, bool use_lt);#ifdef CONFIG_PCIEASPM void pcie_aspm_init_link_state (struct pci_dev *pdev);void pcie_aspm_exit_link_state (struct pci_dev *pdev); -void pcie_aspm_pm_state_change (struct pci_dev *pdev); +void pcie_aspm_pm_state_change (struct pci_dev *pdev,bool locked);void pcie_aspm_powersave_config_link (struct pci_dev *pdev);#else static inline void pcie_aspm_init_link_state (struct pci_dev *pdev) { }static inline void pcie_aspm_exit_link_state (struct pci_dev *pdev) { } -static inline void pcie_aspm_pm_state_change (struct pci_dev *pdev) { } +static inline void pcie_aspm_pm_state_change (struct pci_dev *pdev,bool locked) { }static inline void pcie_aspm_powersave_config_link (struct pci_dev *pdev) { }#endif
drivers/pci/pcie/aspm.c +9 -4
@@ -1003,8 +1003,11 @@ void pcie_aspm_exit_link_state(struct pci_dev *pdev)up_read (&pci_bus_sem); } -/* @pdev: the root port or switch downstream port */ -void pcie_aspm_pm_state_change (struct pci_dev *pdev) +/* +* @pdev: the root port or switch downstream port +* @locked: whether pci_bus_sem is held +*/ +void pcie_aspm_pm_state_change (struct pci_dev *pdev,bool locked) {struct pcie_link_state *link = pdev->link_state;
@@ -1014,12 +1017,14 @@ void pcie_aspm_pm_state_change(struct pci_dev *pdev)* Devices changed PM state, we should recheck if latency * meets all functions' requirement */ -down_read (&pci_bus_sem); +if (!locked) +down_read (&pci_bus_sem);mutex_lock (&aspm_lock);pcie_update_aspm_capable (link->root);pcie_config_aspm_path (link);mutex_unlock (&aspm_lock); -up_read (&pci_bus_sem); +if (!locked) +up_read (&pci_bus_sem); }void pcie_aspm_powersave_config_link (struct pci_dev *pdev)
include/linux/pci.h +5 -0
@@ -1422,6 +1422,7 @@ int pci_load_and_free_saved_state(struct pci_dev *dev, struct pci_saved_state **state);int pci_platform_power_transition (struct pci_dev *dev,pci_power_t state);int pci_set_power_state (struct pci_dev *dev,pci_power_t state); +int pci_set_power_state_locked (struct pci_dev *dev,pci_power_t state);pci_power_t pci_choose_state (struct pci_dev *dev,pm_message_t state);bool pci_pme_capable (struct pci_dev *dev,pci_power_t state);void pci_pme_active (struct pci_dev *dev,bool enable);
@@ -1625,6 +1626,8 @@ int pci_scan_bridge(struct pci_bus *bus, struct pci_dev *dev, int max,void pci_walk_bus (struct pci_bus *top,int (*cb)(struct pci_dev *,void *),void *userdata); +void pci_walk_bus_locked (struct pci_bus *top,int (*cb)(struct pci_dev *,void *), +void *userdata);int pci_cfg_space_size (struct pci_dev *dev);unsigned char pci_bus_max_busnr (struct pci_bus *bus);void pci_setup_bridge (struct pci_bus *bus);
@@ -2025,6 +2028,8 @@ static inline int pci_save_state(struct pci_dev *dev) { return 0; }static inline void pci_restore_state (struct pci_dev *dev) { }static inline int pci_set_power_state (struct pci_dev *dev,pci_power_t state) {return 0 ; } +static inline int pci_set_power_state_locked (struct pci_dev *dev,pci_power_t state) +{return 0 ; }static inline int pci_wake_from_d3 (struct pci_dev *dev,bool enable) {return 0 ; } static inline pci_power_t pci_choose_state(struct pci_dev *dev,
PATCH 1/1 PCI/ASPM: Fix deadlock when enabling ASPM
From: Johan Hovold <[email protected]>
commit 1e560864159d002b453da42bd2c13a1805515a20 upstream.
A last minute revert in 6.7-final introduced a potential deadlock when enabling ASPM during probe of Qualcomm PCIe controllers as reported by lockdep:
============================================
WARNING: possible recursive locking detected
6.7.0 #40 Not tainted
--------------------------------------------
kworker/u16:5/90 is trying to acquire lock:
ffffacfa78ced000 (pci_bus_sem){++++}-{3:3}, at: pcie_aspm_pm_state_change+0x58/0xdc
but task is already holding lock:
ffffacfa78ced000 (pci_bus_sem){++++}-{3:3}, at: pci_walk_bus+0x34/0xbc
other info that might help us debug this:
Possible unsafe locking scenario:
CPU0
----
lock(pci_bus_sem);
lock(pci_bus_sem);
*** DEADLOCK ***Call trace: print_deadlock_bug+0x25c/0x348 __lock_acquire+0x10a4/0x2064 lock_acquire+0x1e8/0x318 down_read+0x60/0x184 pcie_aspm_pm_state_change+0x58/0xdc pci_set_full_power_state+0xa8/0x114 pci_set_power_state+0xc4/0x120 qcom_pcie_enable_aspm+0x1c/0x3c [pcie_qcom] pci_walk_bus+0x64/0xbc qcom_pcie_host_post_init_2_7_0+0x28/0x34 [pcie_qcom]
The deadlock can easily be reproduced on machines like the Lenovo ThinkPad X13s by adding a delay to increase the race window during asynchronous probe where another thread can take a write lock.
Add a new pci_set_power_state_locked() and associated helper functions that can be called with the PCI bus semaphore held to avoid taking the read lock twice.
[bhelgaas: backported to v6.1.y, which contains b9c370b61d73 ("Revert "PCI/ASPM: Remove pcie_aspm_pm_state_change()""), a backport of f93e71aea6c6. This omits the drivers/pci/controller/dwc/pcie-qcom.c hunk that updates qcom_pcie_enable_aspm(), which was added by 9f4f3dfad8cf ("PCI: qcom: Enable ASPM for platforms supporting 1.9.0 ops"), which is not present in v6.1.87.]
--- drivers/pci/bus.c | 49 +++++++++++++++++--------- drivers/pci/pci.c | 78 +++++++++++++++++++++++++++-------------- drivers/pci/pci.h | 4 +-- drivers/pci/pcie/aspm.c | 13 ++++--- include/linux/pci.h | 5 +++ 5 files changed, 100 insertions(+), 49 deletions(-)
drivers/pci/bus.c +32 -17
@@ -379,21 +379,8 @@ void pci_bus_add_devices(const struct pci_bus *bus) }EXPORT_SYMBOL (pci_bus_add_devices); -/** pci_walk_bus - walk devices on/under bus, calling callback. -* @top bus whose devices should be walked -* @cb callback to be called for each device found -* @userdata arbitrary pointer to be passed to callback. -* -* Walk the given bus, including any bridged devices -* on buses under this bus. Call the provided callback -* on each device found. -* -* We check the return of @cb each time. If it returns anything -* other than 0, we break out. -* -*/ -void pci_walk_bus (struct pci_bus *top,int (*cb)(struct pci_dev *,void *), -void *userdata) +static void __pci_walk_bus (struct pci_bus *top,int (*cb)(struct pci_dev *,void *), +void *userdata,bool locked) {struct pci_dev *dev;struct pci_bus *bus;
@@ -401,7 +388,8 @@ void pci_walk_bus(struct pci_bus *top, int (*cb)(struct pci_dev *, void *),int retval; bus = top; -down_read (&pci_bus_sem); +if (!locked) +down_read (&pci_bus_sem); next = top->devices.next;for (;;) {if (next == &bus->devices) {
@@ -424,10 +412,37 @@ void pci_walk_bus(struct pci_bus *top, int (*cb)(struct pci_dev *, void *),if (retval)break ; } -up_read (&pci_bus_sem); +if (!locked) +up_read (&pci_bus_sem); +} + +/** +* pci_walk_bus - walk devices on/under bus, calling callback. +* @top: bus whose devices should be walked +* @cb: callback to be called for each device found +* @userdata: arbitrary pointer to be passed to callback +* +* Walk the given bus, including any bridged devices +* on buses under this bus. Call the provided callback +* on each device found. +* +* We check the return of @cb each time. If it returns anything +* other than 0, we break out. +*/ +void pci_walk_bus (struct pci_bus *top,int (*cb)(struct pci_dev *,void *),void *userdata) +{ +__pci_walk_bus (top, cb, userdata, false); }EXPORT_SYMBOL_GPL (pci_walk_bus); +void pci_walk_bus_locked (struct pci_bus *top,int (*cb)(struct pci_dev *,void *),void *userdata) +{ +lockdep_assert_held (&pci_bus_sem); + +__pci_walk_bus (top, cb, userdata, true); +} +EXPORT_SYMBOL_GPL (pci_walk_bus_locked); +struct pci_bus *pci_bus_get (struct pci_bus *bus) {if (bus)
drivers/pci/pci.c +52 -26
149 lines not on this page · raw message
drivers/pci/pci.h +2 -2
@@ -556,12 +556,12 @@ bool pcie_wait_for_link(struct pci_dev *pdev, bool active);#ifdef CONFIG_PCIEASPM void pcie_aspm_init_link_state (struct pci_dev *pdev);void pcie_aspm_exit_link_state (struct pci_dev *pdev); -void pcie_aspm_pm_state_change (struct pci_dev *pdev); +void pcie_aspm_pm_state_change (struct pci_dev *pdev,bool locked);void pcie_aspm_powersave_config_link (struct pci_dev *pdev);#else static inline void pcie_aspm_init_link_state (struct pci_dev *pdev) { }static inline void pcie_aspm_exit_link_state (struct pci_dev *pdev) { } -static inline void pcie_aspm_pm_state_change (struct pci_dev *pdev) { } +static inline void pcie_aspm_pm_state_change (struct pci_dev *pdev,bool locked) { }static inline void pcie_aspm_powersave_config_link (struct pci_dev *pdev) { }#endif
drivers/pci/pcie/aspm.c +9 -4
@@ -1055,8 +1055,11 @@ void pcie_aspm_exit_link_state(struct pci_dev *pdev)up_read (&pci_bus_sem); } -/* @pdev: the root port or switch downstream port */ -void pcie_aspm_pm_state_change (struct pci_dev *pdev) +/* +* @pdev: the root port or switch downstream port +* @locked: whether pci_bus_sem is held +*/ +void pcie_aspm_pm_state_change (struct pci_dev *pdev,bool locked) {struct pcie_link_state *link = pdev->link_state;
@@ -1066,12 +1069,14 @@ void pcie_aspm_pm_state_change(struct pci_dev *pdev)* Devices changed PM state, we should recheck if latency * meets all functions' requirement */ -down_read (&pci_bus_sem); +if (!locked) +down_read (&pci_bus_sem);mutex_lock (&aspm_lock);pcie_update_aspm_capable (link->root);pcie_config_aspm_path (link);mutex_unlock (&aspm_lock); -up_read (&pci_bus_sem); +if (!locked) +up_read (&pci_bus_sem); }void pcie_aspm_powersave_config_link (struct pci_dev *pdev)
include/linux/pci.h +5 -0
@@ -1383,6 +1383,7 @@ int pci_load_and_free_saved_state(struct pci_dev *dev, struct pci_saved_state **state);int pci_platform_power_transition (struct pci_dev *dev,pci_power_t state);int pci_set_power_state (struct pci_dev *dev,pci_power_t state); +int pci_set_power_state_locked (struct pci_dev *dev,pci_power_t state);pci_power_t pci_choose_state (struct pci_dev *dev,pm_message_t state);bool pci_pme_capable (struct pci_dev *dev,pci_power_t state);void pci_pme_active (struct pci_dev *dev,bool enable);
@@ -1553,6 +1554,8 @@ int pci_scan_bridge(struct pci_bus *bus, struct pci_dev *dev, int max,void pci_walk_bus (struct pci_bus *top,int (*cb)(struct pci_dev *,void *),void *userdata); +void pci_walk_bus_locked (struct pci_bus *top,int (*cb)(struct pci_dev *,void *), +void *userdata);int pci_cfg_space_size (struct pci_dev *dev);unsigned char pci_bus_max_busnr (struct pci_bus *bus);void pci_setup_bridge (struct pci_bus *bus);
@@ -1884,6 +1887,8 @@ static inline int pci_save_state(struct pci_dev *dev) { return 0; }static inline void pci_restore_state (struct pci_dev *dev) { }static inline int pci_set_power_state (struct pci_dev *dev,pci_power_t state) {return 0 ; } +static inline int pci_set_power_state_locked (struct pci_dev *dev,pci_power_t state) +{return 0 ; }static inline int pci_wake_from_d3 (struct pci_dev *dev,bool enable) {return 0 ; } static inline pci_power_t pci_choose_state(struct pci_dev *dev,
Signature · 1 line
2.34.1
Greg KH
maintainerRe: [PATCH 6.1.y] PCI/ASPM: Fix deadlock when enabling ASPM
Quoted 8 lines · Bjorn Helgaas
From: Johan Hovold <[email protected]>
commit
1e560864159d002b453da42bd2c13a1805515a20upstream.A last minute revert in 6.7-final introduced a potential deadlock when enabling ASPM during probe of Qualcomm PCIe controllers as reported by lockdep:
This and the 6.6 patch now queued up, thanks.
greg k-h