Re: [PATCH v6 2/3] usb: cdns3: Add CIX Sky1 glue driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:
- [High] usb: cdns3: unclocked MMIO access and host_dev race in cdnsp_sky1_shutdown()
- [High] usb: cdns3: controller reset in cdnsp_sky1_system_suspend() breaks resume
- [Medium] usb: cdns3: ignored D3 errors and bad D0 poll in cdns_sky1_platform_suspend()
- [Medium] usb: cdns3: wakeup clocks disabled on suspend in cdnsp_sky1_system_suspend()
- [Medium] usb: cdns3: inconsistent wakeup checks between parent and child in cdnsp-sky1
- [Medium] usb: cdns3: clock disable underflow after failed cdnsp_sky1_system_resume()
- [Low] usb: cdns3: missing Return sections in cdnsp-sky1 kernel-doc comments
--

commit 0410eea3b97afaa677cd0ee5ca265bc4f7a11f31
Author: Hongliang Yang (杨宏亮) <hongliang.yang@cixtech.com>

usb: cdns3: Add CIX Sky1 glue driver

This commit adds a glue driver for the Cadence USBSSP dual-role controller
on the CIX Sky1 SoC, managing clocks, resets, mode strap controls in the
S5 system controller, and per-controller glue registers.

> diff --git a/drivers/usb/cdns3/cdnsp-sky1.c b/drivers/usb/cdns3/cdnsp-sky1.c
> new file mode 100644
> index 0000000000000..ca1a37c61baa6
> --- /dev/null
> +++ b/drivers/usb/cdns3/cdnsp-sky1.c
> @@ -0,0 +1,680 @@

[ ... ]

> +/**
> + * cdnsp_sky1_clk_enable_all() - enable all clocks for usb controller
> + * @dev:	Pointer to the device of platform_device
> + *
> + */
> +
> +static int cdnsp_sky1_clk_enable_all(struct device *dev)
> +{

[ ... ]

> +};
> +
> +/**
> + * cdnsp_sky1_clk_disable_all() - disable all clocks for usb controller
> + * @dev:	Pointer to the device of platform_device
> + *
> + */
> +
> +static void cdnsp_sky1_clk_disable_all(struct device *dev)
> +{
> +	int i;
> +	struct cdnsp_sky1 *data = dev_get_drvdata(dev);
> +	struct clk **cix_usb_clks = data->cix_usb_clks;
> +
> +	for (i = 0; i < CIX_USB_CLK_NUM; i++)
> +		clk_disable_unprepare(cix_usb_clks[i]);
> +};
> +
> +/**
> + * cdnsp_sky1_clk_enable_resume() - enable the clocks that are turned
> + * off while suspend
> + * @dev:	Pointer to the device of platform_device
> + *
> + */
> +
> +static int cdnsp_sky1_clk_enable_resume(struct device *dev)

[Severity: Low]
This isn't a bug, but both cdnsp_sky1_clk_enable_all() and
cdnsp_sky1_clk_enable_resume() have kernel-doc comments without a Return:
section. Will scripts/kernel-doc emit warnings here when building with
W=1?

[ ... ]

> +static int cdns_sky1_platform_suspend(struct device *dev,
> +				      bool suspend, bool wakeup)
> +{
> +	struct cdns *cdns = dev_get_drvdata(dev);
> +	struct platform_device *xhci_dev = cdns->host_dev;
> +	struct usb_hcd  *hcd;
> +	struct device *parent = cdns->dev->parent;
> +	struct cdnsp_sky1 *data = dev_get_drvdata(parent);
> +	u32 value;
> +	int poll;
> +	int ret = 0;
> +	int count = 3;
> +
> +	data->wakeup = wakeup;
> +
> +	if (cdns->role != USB_ROLE_HOST)
> +		return 0;
> +
> +	if (!xhci_dev) {
> +		dev_dbg(dev, "host controller has not been registered\n");
> +		return 0;
> +	}
> +
> +	hcd = dev_get_drvdata(&xhci_dev->dev);
> +	if (!hcd) {
> +		dev_dbg(dev, "host controller have not registered\n");
> +		return 0;
> +	}
> +
> +	if (suspend) {
> +		while (count--) {
> +			value = readl(hcd->regs + XECP_PM_PMCSR);
> +			value &= ~PS_MASK;
> +			value |= PS_D3 | PS_PME_En;
> +			writel(value, hcd->regs + XECP_PM_PMCSR);
> +			/* After controller enters D3, disable AXI and SOF
> +			 * until AXI valid flag changes to 0.
> +			 */
> +			if (sky1_rcsu_handshake(data, AXI_CLOCK_VALID, 0,
> +						100ULL * 1000)) {
> +				u32 ctst;
> +
> +				ctst = readl(data->rcsu_ctst_base);
> +				dev_dbg(dev, "enter D3 failed, register value:%x\n",
> +					ctst);
> +			} else {
> +				break;
> +			}
> +		}
> +		if (count < 0) {
> +			u32 ctst;
> +
> +			ctst = readl(data->rcsu_ctst_base);
> +			dev_err(dev, "enter D3 failed after retries, register value:%x\n",
> +				ctst);
> +		}
> +	} else {
> +		while (count--) {
> +			value = readl(hcd->regs + XECP_PM_PMCSR);
> +			value &= ~PS_MASK;
> +			value |= PS_D0;
> +			value &= ~PS_PME_En;
> +			writel(value, hcd->regs + XECP_PM_PMCSR);
> +			/* Wait power state back to D0 */
> +			poll = readl_poll_timeout(hcd->regs + XECP_PM_PMCSR,
> +						  value,
> +						  (value & PS_MASK) == 0 ||
> +						  value == U32_MAX,
> +						  10, 100ULL * 1000);
> +			if (poll) {
> +				dev_dbg(dev, "exit D3 timeout, power state=0x%lx\n",
> +					value & PS_MASK);
> +			} else {
> +				break;
> +			}
> +		}
> +		if (count < 0) {
> +			dev_err(dev, "exit D3 timeout after retries, power state=0x%lx\n",
> +				value & PS_MASK);
> +		}
> +	}
> +
> +	return ret;
> +}

[Severity: Medium]
As reported by Sashiko on v5 [1], are there several issues in
cdns_sky1_platform_suspend()?

First, ret is initialized to 0 and never updated on failure. When
sky1_rcsu_handshake() fails all retries on suspend (count < 0) or
readl_poll_timeout() fails all retries on resume (count < 0),
cdns_sky1_platform_suspend() logs an error via dev_err() but still returns
0. On suspend, won't callers then proceed to power off PHYs and disable the
AXI and SOF clocks in cdnsp_sky1_system_suspend() while AXI_CLOCK_VALID is
still asserted?

Second, in the resume (!suspend) branch, readl_poll_timeout() polls
hcd->regs + XECP_PM_PMCSR with the condition (value & PS_MASK) == 0 ||
value == U32_MAX. Unlike sky1_rcsu_handshake(), which checks if
result == U32_MAX and returns -ENODEV, this loop only checks if (poll) and
breaks out when poll == 0. If reading XECP_PM_PMCSR returns U32_MAX (where
value & PS_MASK is PS_D3), won't readl_poll_timeout() return 0 and cause
the loop to exit immediately as if the controller had reached D0?

Third, in the suspend branch, value |= PS_D3 | PS_PME_En sets PS_PME_En
unconditionally even when wakeup is false.

Fourth, data->wakeup = wakeup is assigned before returning early when
cdns->role != USB_ROLE_HOST. In peripheral or idle OTG role, the
controller is not placed into D3 and AXI_CLOCK_VALID is not cleared, yet
if wakeup is true, cdnsp_sky1_system_suspend() skips asserting resets while
disabling the sof and axi clocks with the controller still active in D0.

[ ... ]

> +static void cdnsp_sky1_remove(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct cdnsp_sky1 *data = dev_get_drvdata(dev);
> +
> +	pm_runtime_get_sync(dev);
> +	of_platform_depopulate(dev);
> +	sky1_put_addr(data->xhci_base);
> +	sky1_put_addr(data->device_base);
> +	reset_control_assert(data->reset);
> +	reset_control_assert(data->preset);
> +	cdnsp_sky1_clk_disable_all(dev);
> +	pm_runtime_disable(dev);
> +	pm_runtime_set_suspended(dev);
> +	pm_runtime_put_noidle(dev);
> +}
> +
> +/*
> + * Because the wake-up interrupt and host interrupt are the same interrupt,
> + * closing the axi and sof clock will result in the inability to generate
> + * port status change interrupt.
> + */
> +static int cdnsp_sky1_system_suspend(struct device *dev)
> +{
> +	struct cdnsp_sky1 *data = dev_get_drvdata(dev);
> +
> +	if (!data->wakeup) {
> +		reset_control_assert(data->reset);
> +		reset_control_assert(data->preset);
> +	}
> +	cdnsp_sky1_clk_disable_suspend(dev);
> +	return 0;
> +}

[Severity: Medium]
As reported by Sashiko on v5 [1], the comment above
cdnsp_sky1_system_suspend() notes that closing the axi and sof clocks
prevents generating a port status change wakeup interrupt. However,
cdnsp_sky1_system_suspend() only guards reset_control_assert() with
if (!data->wakeup) and then unconditionally calls
cdnsp_sky1_clk_disable_suspend(dev), which disables the first
CIX_USB_CLK_OFF_NUM clocks (sof and axi).

Doesn't this disable the sof and axi clocks even when data->wakeup is
true, preventing wakeup interrupts during system sleep?

> +
> +static int cdnsp_sky1_system_resume(struct device *dev)
> +{
> +	int ret = 0;
> +	struct cdnsp_sky1 *data = dev_get_drvdata(dev);
> +
> +	ret = cdnsp_sky1_clk_enable_resume(dev);
> +	if (ret)
> +		return ret;
> +	if (!data->wakeup) {
> +		writel(CIX_USB_AXI_WR_CACHE_VALUE, data->rcsu_axi_base);
> +		cdnsp_sky1_set_mode(data->dev, MODE_STRAP_OTG);
> +		reset_control_deassert(data->preset);
> +		cdnsp_sky1_configure_controller(data);
> +		reset_control_deassert(data->reset);
> +	}
> +	return 0;
> +}

[Severity: High]
As reported by Sashiko on v5 [1], how does system resume recover after
cdnsp_sky1_system_suspend() asserts data->reset and data->preset when
!data->wakeup?

Asserting data->reset and data->preset resets the entire Cadence USBSSP
controller, clearing all OTG/DRD, xHCI, and gadget registers. On system
resume, cdnsp_sky1_system_resume() deasserts preset, runs
cdnsp_sky1_configure_controller(), and deasserts reset, leaving the DRD
block in its post-reset state (OTGCMD_HOST_BUS_REQ is not set,
OTGSTS_CDNSP_XHCI_READY is 0, OVERRIDE_IDPULLUP is cleared, and OTG
interrupts in otg_cdnsp_regs->ien are disabled).

When the child device then resumes in
cdns3_plat_resume() -> cdns3_controller_resume():

drivers/usb/cdns3/cdns3-plat.c:cdns3_controller_resume() {
	...
	cdns3_set_platform_suspend(cdns->dev, false, false);

	spin_lock_irqsave(&cdns->lock, flags);
	cdns_resume(cdns);
	cdns->in_lpm = false;
	spin_unlock_irqrestore(&cdns->lock, flags);
	...
}

First, cdns3_set_platform_suspend() invokes cdns_sky1_platform_suspend()
before cdns_resume() runs, reading and writing hcd->regs + XECP_PM_PMCSR
before cdns_drd_host_on() has re-enabled the host controller in the DRD
block.

Second, cdns3_controller_resume() calls cdns_resume(cdns) while holding
spin_lock_irqsave(&cdns->lock, flags). In cdns_resume(),
cdns_power_is_lost(cdns) is checked (which for CDNSP_CONTROLLER_V2 reads
&cdns->otg_v1_regs->simulate at offset 0x40 rather than simulate at 0x28
in struct cdnsp_otg_regs, and simulate is never written in cdns_drd_init()
for CDNSP_CONTROLLER_V2). If cdns_power_is_lost() returns true (after the
reset or during runtime resume enabled by CDNS3_DEFAULT_PM_RUNTIME_ALLOW),
cdns_resume() calls cdns_hw_role_switch() (which calls
pm_runtime_get_sync() and mutex_lock(&cdns->mutex)) or cdns_drd_host_on() /
cdns_drd_gadget_on() (which call phy_set_mode() -> mutex_lock(&phy->mutex))
under the spinlock, sleeping in atomic context. If cdns_power_is_lost()
returns false, cdns_drd_host_on() and xHCI restore are skipped, and
neither cdnsp_sky1_system_resume() nor cdns_resume() calls
cdns_drd_update_mode() to restore OVERRIDE_IDPULLUP and
otg_irq_regs->ien.

[Severity: Medium]
As reported by Sashiko on v5 [1], what happens if
cdnsp_sky1_clk_enable_resume() fails in cdnsp_sky1_system_resume()?

During system suspend, cdnsp_sky1_clk_disable_suspend() disables and
unprepares the first CIX_USB_CLK_OFF_NUM clocks (sof and axi). If
cdnsp_sky1_clk_enable_resume() fails during resume, it unwinds any clock
it just enabled and returns the error, leaving sof and axi disabled
without tracking that those clocks remain off. When cdnsp_sky1_remove() or
cdnsp_sky1_shutdown() later runs, it unconditionally calls
cdnsp_sky1_clk_disable_all(dev) across all CIX_USB_CLK_NUM clocks. Won't
that underflow the enable and prepare counts for sof and axi?

[ ... ]

> +static void cdnsp_sky1_shutdown(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct cdnsp_sky1 *data = dev_get_drvdata(dev);
> +	struct device *cdns_dev = NULL;
> +	struct device *gadget_dev = NULL;
> +	struct cdns *cdns = NULL;
> +
> +	if (!device_may_wakeup(dev)) {

[Severity: Medium]
As reported by Sashiko on v5 [1], is the wakeup state checked consistently
between the parent glue device and the child cdns,usb3 device?

cdnsp_sky1_probe() calls device_set_wakeup_capable(dev, true) on the
parent glue device, while cdns3_plat_probe() also marks the child
cdns,usb3 device wakeup-capable.

During system suspend and resume, cdnsp_sky1_system_suspend() and
cdnsp_sky1_system_resume() check data->wakeup (populated from the child
device in cdns_sky1_platform_suspend()), ignoring device_may_wakeup(dev) on
the parent. Conversely, cdnsp_sky1_shutdown() checks
if (!device_may_wakeup(dev)) on the parent glue device, ignoring the child
device's wakeup setting.

Also, during runtime suspend (PMSG_AUTO_SUSPEND),
cdns3_controller_suspend() passes wakeup = true to
cdns_sky1_platform_suspend(), setting data->wakeup = true; if the child
device remains in cdns->in_lpm when system suspend runs,
cdns3_controller_suspend() returns early without updating data->wakeup to
device_may_wakeup(cdns->dev).

> +		/*
> +		 * Find the cdns3 child device, then find its gadget device
> +		 * and release the function driver before disabling clocks.
> +		 * This ensures all register accesses in gadget_unbind_driver
> +		 * complete before clocks are turned off.
> +		 */
> +		device_for_each_child(dev, &cdns_dev, cdnsp_sky1_find_cdns_dev);
> +		if (cdns_dev)
> +			cdns = dev_get_drvdata(cdns_dev);
> +		if (cdns)
> +			device_for_each_child(cdns_dev, &gadget_dev,
> +					      cdnsp_sky1_find_gadget_match);
> +		if (gadget_dev)
> +			device_release_driver(gadget_dev);
> +		if (cdns && cdns->host_dev) {
> +			struct usb_hcd *hcd = platform_get_drvdata(cdns->host_dev);
> +
> +			if (hcd && hcd->irq > 0) {
> +				/*
> +				 * Clear HCD_FLAG_HW_ACCESSIBLE before disable_irq.
> +				 * This prevents usb_hcd_irq from calling xhci_irq
> +				 * (which reads USBSTS) after clocks are disabled.
> +				 */
> +				clear_bit(HCD_FLAG_HW_ACCESSIBLE, &hcd->flags);
> +				disable_irq(hcd->irq);
> +				synchronize_irq(hcd->irq);
> +			}
> +		}
> +		reset_control_assert(data->reset);
> +		reset_control_assert(data->preset);
> +		cdnsp_sky1_clk_disable_all(dev);
> +		put_device(gadget_dev);
> +		put_device(cdns_dev);
> +	}
> +}

[Severity: High]
As reported by Sashiko on v5 [1], can cdnsp_sky1_shutdown() trigger
unclocked MMIO accesses or race with role switching?

1. Because cdnsp_sky1_probe() sets CDNS3_DEFAULT_PM_RUNTIME_ALLOW, the
child cdns,usb3 device autosuspends when idle or in peripheral mode.
During device_shutdown(), the driver core only calls
pm_runtime_get_noresume(), not pm_runtime_get_sync(). When cdns->host_dev
is not active, usb_hcd_platform_shutdown() does not run to runtime-resume
cdns_dev. Calling device_release_driver(gadget_dev) without first calling
pm_runtime_get_sync(cdns_dev) causes
gadget_unbind_driver() -> cdnsp_gadget_udc_stop() -> cdnsp_stop() to access
controller MMIO registers while the controller is in low-power state.

2. cdnsp_sky1_shutdown() reads cdns->host_dev and accesses its drvdata
struct usb_hcd without holding cdns->mutex or disabling cdns->otg_irq. A
concurrent OTG role switch via cdns_drd_thread_irq() or cdns_role_set()
calls cdns_host_exit() under cdns->mutex:

drivers/usb/cdns3/host.c:cdns_host_exit() {
	kfree(cdns->xhci_plat_data);
	platform_device_unregister(cdns->host_dev);
	cdns->host_dev = NULL;
	cdns_drd_host_off(cdns);
}

which unregisters and frees cdns->host_dev and struct usb_hcd before
setting cdns->host_dev = NULL, racing with cdnsp_sky1_shutdown().

3. Calling device_release_driver(gadget_dev) only unbinds the gadget
function driver on the gadget bus and does not tear down the gadget or DRD
drivers. Both cdns->otg_irq (requested with IRQF_SHARED for
cdns_drd_irq()) and cdns->dev_irq (requested with IRQF_SHARED for
cdnsp_irq_handler(), and not disabled via hcd->irq when cdns->host_dev is
NULL in device mode) remain enabled when cdnsp_sky1_shutdown() asserts
data->reset and data->preset and disables all clocks via
cdnsp_sky1_clk_disable_all(dev). If a shared or pending interrupt fires on
otg_irq or dev_irq afterward, cdns_drd_irq() or cdnsp_irq_handler() will
read controller MMIO registers while resets are asserted and clocks are
disabled.

[1] https://lore.kernel.org/r/20261103074929.1971195-3-hongliang.yang@cixtech.com

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261105011803.4058557-1-hongliang.yang@cixtech.com?part=2