Re: [PATCH 05/19] usb: cdns3: switch the role unlocked on resume

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] usb: cdns3: role switched in cdns3_controller_resume() while in_lpm is true
--

commit 4cc15afc73e105403285c3f1673e0d0dcaa8187a
Author: Josef Bacik <josef@toxicpanda.com>

usb: cdns3: switch the role unlocked on resume

This commit moves the OTG role switch during system resume out of
cdns_resume() into a new cdns_resume_role() helper called before acquiring
cdns->lock. This avoids calling sleeping functions from atomic context when
the controller loses power during suspend and the role changes while asleep.

> diff --git a/drivers/usb/cdns3/cdns3-plat.c b/drivers/usb/cdns3/cdns3-plat.c
> index e3f32c3e95352..674625cdd4d48 100644
> --- a/drivers/usb/cdns3/cdns3-plat.c
> +++ b/drivers/usb/cdns3/cdns3-plat.c
> @@ -268,7 +268,7 @@ static int cdns3_controller_suspend(struct device *dev, pm_message_t msg)
>  static int cdns3_controller_resume(struct device *dev, pm_message_t msg)
>  {
>  	struct cdns *cdns = dev_get_drvdata(dev);
> -	int ret;
> +	int ret, role_changed;
>  	unsigned long flags;
>  
>  	if (!cdns->in_lpm)
> @@ -292,8 +292,12 @@ static int cdns3_controller_resume(struct device *dev, pm_message_t msg)
>  
>  	cdns3_set_platform_suspend(cdns->dev, false, false);
>  
> +	/* It resumes the device, which cannot be done from runtime resume */
> +	role_changed = PMSG_IS_AUTO(msg) ? 0 : cdns_resume_role(cdns);
> +
>  	spin_lock_irqsave(&cdns->lock, flags);
> -	cdns_resume(cdns);
> +	if (!role_changed)
> +		cdns_resume(cdns);
>  	cdns->in_lpm = false;
>  	spin_unlock_irqrestore(&cdns->lock, flags);
>  	cdns_set_active(cdns, !PMSG_IS_AUTO(msg));

[Severity: High]
Can calling cdns_resume_role() with interrupts enabled while cdns->in_lpm is
still true cause an unhandled interrupt storm?

When the controller loses power during system suspend and transitions to
USB_ROLE_DEVICE on resume (for example, because a peripheral cable was
connected while asleep), cdns_resume_role() invokes the following call chain
before cdns->in_lpm is cleared:

  cdns_resume_role()
    cdns_hw_role_switch()
      cdns_role_start()
        __cdns3_gadget_init()

In drivers/usb/cdns3/cdns3-gadget.c, __cdns3_gadget_init() enables the gadget
block, starts the gadget (which binds the gadget driver and enables
USB_IEN_INIT and USB_CONF_DEVEN), and requests cdns->dev_irq:

	cdns_drd_gadget_on(cdns);
	pm_runtime_get_sync(cdns->dev);

	ret = cdns3_gadget_start(cdns);
	...
	ret = devm_request_threaded_irq(cdns->dev, cdns->dev_irq,
					cdns3_device_irq_handler,
					cdns3_device_thread_irq_handler,
					IRQF_SHARED, dev_name(cdns->dev),
					cdns->gadget_dev);

Because VBUS is present and device interrupts are enabled in hardware, the
level-triggered cdns->dev_irq can fire immediately while cdns_resume_role()
is still running.

However, cdns3_device_irq_handler() in drivers/usb/cdns3/cdns3-gadget.c (as
well as cdns_drd_irq() in drivers/usb/cdns3/drd.c) checks cdns->in_lpm at the
entry of the handler:

	if (cdns->in_lpm)
		return ret;

Since cdns->in_lpm is not set to false until after cdns_resume_role() returns
and cdns->lock is acquired, the handler returns IRQ_NONE without masking or
acknowledging the pending hardware interrupt, which can cause the kernel to
disable the IRQ line or trigger spurious wakeup handling on shared IRQ
platforms.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261027-llm-usb-v1-0-b9751ddb9c7c@linuxfoundation.org?part=5