2023-06-16 02:40:07

by Xiaolei Wang

[permalink] [raw]
Subject: [PATCH v3] usb: cdns3: Put the cdns set active part outside the spin lock

The device may be scheduled during the resume process,
so this cannot appear in atomic operations. Since
pm_runtime_set_active will resume suppliers, put set
active outside the spin lock, which is only used to
protect the struct cdns data structure, otherwise the
kernel will report the following warning:

BUG: sleeping function called from invalid context at drivers/base/power/runtime.c:1163
in_atomic(): 1, irqs_disabled(): 0, non_block: 0, pid: 651, name: sh
preempt_count: 1, expected: 0
RCU nest depth: 0, expected: 0
CPU: 0 PID: 651 Comm: sh Tainted: G WC 6.1.20 #1
Hardware name: Freescale i.MX8QM MEK (DT)
Call trace:
dump_backtrace.part.0+0xe0/0xf0
show_stack+0x18/0x30
dump_stack_lvl+0x64/0x80
dump_stack+0x1c/0x38
__might_resched+0x1fc/0x240
__might_sleep+0x68/0xc0
__pm_runtime_resume+0x9c/0xe0
rpm_get_suppliers+0x68/0x1b0
__pm_runtime_set_status+0x298/0x560
cdns_resume+0xb0/0x1c0
cdns3_controller_resume.isra.0+0x1e0/0x250
cdns3_plat_resume+0x28/0x40

Signed-off-by: Xiaolei Wang <[email protected]>
---
v3:
* Fix build error:
cdns3-plat.c:258:18: error: too few arguments to function call, expected 2, have 1
258 | cdns_resume(cdns);
* use 'cdns_resume(struct cdns *cdns) { return 0; }' instead of
'cdns_resume(struct cdns *cdns, u8 set_active) { return 0; }'
and add 'static inline int cdns_set_active(struct cdns *cdns, u8 set_active)
{ return 0; }'
v2:
* Fix build error: unused variable 'dev'
* delete unused 'struct device *dev = cdns->dev;' in cdns_resume()

drivers/usb/cdns3/cdns3-plat.c | 3 ++-
drivers/usb/cdns3/cdnsp-pci.c | 3 ++-
drivers/usb/cdns3/core.c | 15 +++++++++++----
drivers/usb/cdns3/core.h | 7 +++++--
4 files changed, 20 insertions(+), 8 deletions(-)

diff --git a/drivers/usb/cdns3/cdns3-plat.c b/drivers/usb/cdns3/cdns3-plat.c
index 884e2301237f..1168dbeed2ce 100644
--- a/drivers/usb/cdns3/cdns3-plat.c
+++ b/drivers/usb/cdns3/cdns3-plat.c
@@ -255,9 +255,10 @@ static int cdns3_controller_resume(struct device *dev, pm_message_t msg)
cdns3_set_platform_suspend(cdns->dev, false, false);

spin_lock_irqsave(&cdns->lock, flags);
- cdns_resume(cdns, !PMSG_IS_AUTO(msg));
+ cdns_resume(cdns);
cdns->in_lpm = false;
spin_unlock_irqrestore(&cdns->lock, flags);
+ cdns_set_active(cdns, !PMSG_IS_AUTO(msg));
if (cdns->wakeup_pending) {
cdns->wakeup_pending = false;
enable_irq(cdns->wakeup_irq);
diff --git a/drivers/usb/cdns3/cdnsp-pci.c b/drivers/usb/cdns3/cdnsp-pci.c
index 7b151f5af3cc..0725668ffea4 100644
--- a/drivers/usb/cdns3/cdnsp-pci.c
+++ b/drivers/usb/cdns3/cdnsp-pci.c
@@ -208,8 +208,9 @@ static int __maybe_unused cdnsp_pci_resume(struct device *dev)
int ret;

spin_lock_irqsave(&cdns->lock, flags);
- ret = cdns_resume(cdns, 1);
+ ret = cdns_resume(cdns);
spin_unlock_irqrestore(&cdns->lock, flags);
+ cdns_set_active(cdns, 1);

return ret;
}
diff --git a/drivers/usb/cdns3/core.c b/drivers/usb/cdns3/core.c
index dbcdf3b24b47..7b20d2d5c262 100644
--- a/drivers/usb/cdns3/core.c
+++ b/drivers/usb/cdns3/core.c
@@ -522,9 +522,8 @@ int cdns_suspend(struct cdns *cdns)
}
EXPORT_SYMBOL_GPL(cdns_suspend);

-int cdns_resume(struct cdns *cdns, u8 set_active)
+int cdns_resume(struct cdns *cdns)
{
- struct device *dev = cdns->dev;
enum usb_role real_role;
bool role_changed = false;
int ret = 0;
@@ -556,15 +555,23 @@ int cdns_resume(struct cdns *cdns, u8 set_active)
if (cdns->roles[cdns->role]->resume)
cdns->roles[cdns->role]->resume(cdns, cdns_power_is_lost(cdns));

+ return 0;
+}
+EXPORT_SYMBOL_GPL(cdns_resume);
+
+void cdns_set_active(struct cdns *cdns, u8 set_active)
+{
+ struct device *dev = cdns->dev;
+
if (set_active) {
pm_runtime_disable(dev);
pm_runtime_set_active(dev);
pm_runtime_enable(dev);
}

- return 0;
+ return;
}
-EXPORT_SYMBOL_GPL(cdns_resume);
+EXPORT_SYMBOL_GPL(cdns_set_active);
#endif /* CONFIG_PM_SLEEP */

MODULE_AUTHOR("Peter Chen <[email protected]>");
diff --git a/drivers/usb/cdns3/core.h b/drivers/usb/cdns3/core.h
index 2d332a788871..4a4dbc2c1561 100644
--- a/drivers/usb/cdns3/core.h
+++ b/drivers/usb/cdns3/core.h
@@ -125,10 +125,13 @@ int cdns_init(struct cdns *cdns);
int cdns_remove(struct cdns *cdns);

#ifdef CONFIG_PM_SLEEP
-int cdns_resume(struct cdns *cdns, u8 set_active);
+int cdns_resume(struct cdns *cdns);
int cdns_suspend(struct cdns *cdns);
+void cdns_set_active(struct cdns *cdns, u8 set_active);
#else /* CONFIG_PM_SLEEP */
-static inline int cdns_resume(struct cdns *cdns, u8 set_active)
+static inline int cdns_resume(struct cdns *cdns)
+{ return 0; }
+static inline int cdns_set_active(struct cdns *cdns, u8 set_active)
{ return 0; }
static inline int cdns_suspend(struct cdns *cdns)
{ return 0; }
--
2.25.1



2023-06-18 14:18:24

by Peter Chen

[permalink] [raw]
Subject: Re: [PATCH v3] usb: cdns3: Put the cdns set active part outside the spin lock

On 23-06-16 10:19:51, Xiaolei Wang wrote:
> The device may be scheduled during the resume process,
> so this cannot appear in atomic operations. Since
> pm_runtime_set_active will resume suppliers, put set
> active outside the spin lock, which is only used to
> protect the struct cdns data structure, otherwise the
> kernel will report the following warning:
>
> BUG: sleeping function called from invalid context at drivers/base/power/runtime.c:1163
> in_atomic(): 1, irqs_disabled(): 0, non_block: 0, pid: 651, name: sh
> preempt_count: 1, expected: 0
> RCU nest depth: 0, expected: 0
> CPU: 0 PID: 651 Comm: sh Tainted: G WC 6.1.20 #1
> Hardware name: Freescale i.MX8QM MEK (DT)
> Call trace:
> dump_backtrace.part.0+0xe0/0xf0
> show_stack+0x18/0x30
> dump_stack_lvl+0x64/0x80
> dump_stack+0x1c/0x38
> __might_resched+0x1fc/0x240
> __might_sleep+0x68/0xc0
> __pm_runtime_resume+0x9c/0xe0
> rpm_get_suppliers+0x68/0x1b0
> __pm_runtime_set_status+0x298/0x560
> cdns_resume+0xb0/0x1c0
> cdns3_controller_resume.isra.0+0x1e0/0x250
> cdns3_plat_resume+0x28/0x40
>
> Signed-off-by: Xiaolei Wang <[email protected]>

Acked-by: Peter Chen <[email protected]>

Peter
> ---
> v3:
> * Fix build error:
> cdns3-plat.c:258:18: error: too few arguments to function call, expected 2, have 1
> 258 | cdns_resume(cdns);
> * use 'cdns_resume(struct cdns *cdns) { return 0; }' instead of
> 'cdns_resume(struct cdns *cdns, u8 set_active) { return 0; }'
> and add 'static inline int cdns_set_active(struct cdns *cdns, u8 set_active)
> { return 0; }'
> v2:
> * Fix build error: unused variable 'dev'
> * delete unused 'struct device *dev = cdns->dev;' in cdns_resume()
>
> drivers/usb/cdns3/cdns3-plat.c | 3 ++-
> drivers/usb/cdns3/cdnsp-pci.c | 3 ++-
> drivers/usb/cdns3/core.c | 15 +++++++++++----
> drivers/usb/cdns3/core.h | 7 +++++--
> 4 files changed, 20 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/usb/cdns3/cdns3-plat.c b/drivers/usb/cdns3/cdns3-plat.c
> index 884e2301237f..1168dbeed2ce 100644
> --- a/drivers/usb/cdns3/cdns3-plat.c
> +++ b/drivers/usb/cdns3/cdns3-plat.c
> @@ -255,9 +255,10 @@ static int cdns3_controller_resume(struct device *dev, pm_message_t msg)
> cdns3_set_platform_suspend(cdns->dev, false, false);
>
> spin_lock_irqsave(&cdns->lock, flags);
> - cdns_resume(cdns, !PMSG_IS_AUTO(msg));
> + cdns_resume(cdns);
> cdns->in_lpm = false;
> spin_unlock_irqrestore(&cdns->lock, flags);
> + cdns_set_active(cdns, !PMSG_IS_AUTO(msg));
> if (cdns->wakeup_pending) {
> cdns->wakeup_pending = false;
> enable_irq(cdns->wakeup_irq);
> diff --git a/drivers/usb/cdns3/cdnsp-pci.c b/drivers/usb/cdns3/cdnsp-pci.c
> index 7b151f5af3cc..0725668ffea4 100644
> --- a/drivers/usb/cdns3/cdnsp-pci.c
> +++ b/drivers/usb/cdns3/cdnsp-pci.c
> @@ -208,8 +208,9 @@ static int __maybe_unused cdnsp_pci_resume(struct device *dev)
> int ret;
>
> spin_lock_irqsave(&cdns->lock, flags);
> - ret = cdns_resume(cdns, 1);
> + ret = cdns_resume(cdns);
> spin_unlock_irqrestore(&cdns->lock, flags);
> + cdns_set_active(cdns, 1);
>
> return ret;
> }
> diff --git a/drivers/usb/cdns3/core.c b/drivers/usb/cdns3/core.c
> index dbcdf3b24b47..7b20d2d5c262 100644
> --- a/drivers/usb/cdns3/core.c
> +++ b/drivers/usb/cdns3/core.c
> @@ -522,9 +522,8 @@ int cdns_suspend(struct cdns *cdns)
> }
> EXPORT_SYMBOL_GPL(cdns_suspend);
>
> -int cdns_resume(struct cdns *cdns, u8 set_active)
> +int cdns_resume(struct cdns *cdns)
> {
> - struct device *dev = cdns->dev;
> enum usb_role real_role;
> bool role_changed = false;
> int ret = 0;
> @@ -556,15 +555,23 @@ int cdns_resume(struct cdns *cdns, u8 set_active)
> if (cdns->roles[cdns->role]->resume)
> cdns->roles[cdns->role]->resume(cdns, cdns_power_is_lost(cdns));
>
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(cdns_resume);
> +
> +void cdns_set_active(struct cdns *cdns, u8 set_active)
> +{
> + struct device *dev = cdns->dev;
> +
> if (set_active) {
> pm_runtime_disable(dev);
> pm_runtime_set_active(dev);
> pm_runtime_enable(dev);
> }
>
> - return 0;
> + return;
> }
> -EXPORT_SYMBOL_GPL(cdns_resume);
> +EXPORT_SYMBOL_GPL(cdns_set_active);
> #endif /* CONFIG_PM_SLEEP */
>
> MODULE_AUTHOR("Peter Chen <[email protected]>");
> diff --git a/drivers/usb/cdns3/core.h b/drivers/usb/cdns3/core.h
> index 2d332a788871..4a4dbc2c1561 100644
> --- a/drivers/usb/cdns3/core.h
> +++ b/drivers/usb/cdns3/core.h
> @@ -125,10 +125,13 @@ int cdns_init(struct cdns *cdns);
> int cdns_remove(struct cdns *cdns);
>
> #ifdef CONFIG_PM_SLEEP
> -int cdns_resume(struct cdns *cdns, u8 set_active);
> +int cdns_resume(struct cdns *cdns);
> int cdns_suspend(struct cdns *cdns);
> +void cdns_set_active(struct cdns *cdns, u8 set_active);
> #else /* CONFIG_PM_SLEEP */
> -static inline int cdns_resume(struct cdns *cdns, u8 set_active)
> +static inline int cdns_resume(struct cdns *cdns)
> +{ return 0; }
> +static inline int cdns_set_active(struct cdns *cdns, u8 set_active)
> { return 0; }
> static inline int cdns_suspend(struct cdns *cdns)
> { return 0; }
> --
> 2.25.1
>

--

Thanks,
Peter Chen