[PATCH v3 4/5] usb: cdns3: Add CIX Sky1 glue driver

[sashiko-bot at kernel.org]

Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:
- [High] Double disable of clocks on probe error path causes reference count underflows.
- [High] Incorrect operation order and disable_irq() on a shared IRQ in cdnsp_sky1_shutdown.
- [High] Lockless access to cdns->host_dev during system suspend and shutdown leads to a Use-After-Free.
- [Medium] Incorrect PM wrapper macro used for the dev_pm_ops structure pointer.
- [Medium] Fragile lookup of Device Tree child nodes for register addresses.
- [Medium] Unconditional disabling of AXI and SOF clocks during suspend breaks Wake-on-USB.
- [Low] Missing inclusion of <linux/iopoll.h>.
--

commit 5ad3503fa82899af71645298ce56167ad8a8a82d
Author: Hongliang Yang <hongliang.yang at cixtech.com>
Date:   Thu Sep 3 02:35:20 2026 +0000

    usb: cdns3: Add CIX Sky1 glue driver

This patch adds a glue layer driver for the Cadence USBSSP controller
integrated into the CIX Sky1 SoC. It enables the controller clocks, resets,
and handles mode strap controls via the Sky1 S5 system controller.

> --- /dev/null
> +++ b/drivers/usb/cdns3/cdnsp-sky1.c
[ ... ]
> +static int sky1_handshake(void __iomem *ptr, u32 mask, u32 done, u64 timeout_us)
> +{
> +	u32	result;
> +	int	ret;
> +
> +	ret = readl_poll_timeout(ptr, result,
> +				 (result & mask) == done || result == U32_MAX,
> +				 10, timeout_us);

[Severity: Low]
Could this fail to compile on certain architectures or configurations?

The readl_poll_timeout macro is used here, but its defining header 
<linux/iopoll.h> is not included in the file.

[ ... ]
> +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;

[Severity: High]
Is it safe to read cdns->host_dev here without holding cdns->mutex?

If a concurrent OTG role switch triggers while the system executes this suspend
callback, could this lead to a use-after-free when the host_dev is freed?

[ ... ]
> +static void __iomem *sky1_of_get_addr_by_name(struct device_node *parent, const char *name)
> +{
> +	void __iomem *addr = NULL;
> +	struct device_node *node;
> +	int index;
> +
> +	node = of_get_next_child(parent, NULL);
> +	if (!node)
> +		return NULL;
> +
> +	index = of_property_match_string(node, "reg-names", name);

[Severity: Medium]
Could this lookup be fragile against changes in the device tree ordering?

By passing NULL to of_get_next_child, it blindly assumes the target device node
is always the very first child.

If the structure is ordered differently, this might abort the probe.

[ ... ]
> +err_quiesce:
> +	reset_control_assert(data->reset);
> +	reset_control_assert(data->preset);
> +	cdnsp_sky1_clk_disable_all(dev);

[Severity: High]
Could this cause a reference count underflow on the clocks?

If cdnsp_sky1_clk_enable_all fails mid-initialization, it cleanly unwinds its
own prepared clocks.

Returning the error then propagates up to the probe function which jumps to
err_quiesce and unconditionally calls cdnsp_sky1_clk_disable_all, disabling
the already unwound clocks a second time.

[ ... ]
> +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]
Does unconditionally disabling the AXI and SOF clocks here break Wake-on-USB
functionality?

The comment earlier mentions that closing the axi and sof clock will result in
the inability to generate port status change interrupts, yet the disable
function is called unconditionally, outside the !data->wakeup block.

[ ... ]
> +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 (cdns && cdns->host_dev) {
> +			struct usb_hcd *hcd = platform_get_drvdata(cdns->host_dev);

[Severity: High]
Similar to the suspend callback, is there a risk of a use-after-free here if
an interleaving OTG role switch frees cdns->host_dev right before
platform_get_drvdata is called, since this is accessed without cdns->mutex?

> +
> +			if (hcd && hcd->irq > 0) {
> +				disable_irq(hcd->irq);
> +				synchronize_irq(hcd->irq);
> +				/*
> +				 * 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);
> +			}
> +		}

[Severity: High]
Does calling disable_irq on a potentially shared IRQ line risk breaking other
devices on the same line?

Additionally, the comment explicitly states that HCD_FLAG_HW_ACCESSIBLE must
be cleared before disable_irq to prevent the interrupt handler from reading
USBSTS after clocks are disabled.

However, the implementation executes disable_irq and synchronize_irq before
clear_bit.

Could this out-of-order execution risk interrupt handlers firing
inappropriately during teardown?

[ ... ]
> +static struct platform_driver cdnsp_sky1_driver = {
> +	.probe		= cdnsp_sky1_probe,
> +	.remove		= cdnsp_sky1_remove,
> +	.shutdown	= cdnsp_sky1_shutdown,
> +	.driver		= {
> +		.name	= "cdnsp-sky1",
> +		.of_match_table	= cdns_sky1_of_match,
> +		.pm	= pm_sleep_ptr(&cdnsp_sky1_pm_ops),
> +	},
> +};

[Severity: Medium]
Should the dev_pm_ops structure pointer use the pm_ptr macro instead of
pm_sleep_ptr?

If the kernel is built with CONFIG_PM=y but CONFIG_PM_SLEEP=n, using
pm_sleep_ptr erroneously excludes the structure pointer, leading to dead code
and bypassed power management.

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