| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20230618141425.GA1588566@nchen-desktop>
Date: Sun, 18 Jun 2023 22:14:25 +0800
From: Peter Chen <peter.chen@...nel.org>
To: Xiaolei Wang <xiaolei.wang@...driver.com>
Cc: pawell@...ence.com, rogerq@...nel.org, a-govindraju@...com,
gregkh@...uxfoundation.org, linux-usb@...r.kernel.org,
linux-kernel@...r.kernel.org
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 <xiaolei.wang@...driver.com>
Acked-by: Peter Chen <peter.chen@...nel.org>
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 <peter.chen@....com>");
> 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
Powered by blists - more mailing lists