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

From: Peter Chen <peter.chen@kernel.org>
To: Xiaolei Wang <xiaolei.wang@windriver.com>
Cc: pawell@cadence.com, rogerq@kernel.org, a-govindraju@ti.com,
	gregkh@linuxfoundation.org, linux-usb@vger.kernel.org,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH v3] usb: cdns3: Put the cdns set active part outside the spin lock
Date: Sun, 18 Jun 2023 22:14:25 +0800	[thread overview]
Message-ID: <20230618141425.GA1588566@nchen-desktop> (raw)
In-Reply-To: <20230616021952.1025854-1-xiaolei.wang@windriver.com>

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@windriver.com>

Acked-by: Peter Chen <peter.chen@kernel.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@nxp.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