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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential
issues to consider:
- [Medium] usb: cdns3: cdns_sky1_pdata leak in cdnsp_sky1_probe()
- [Medium] usb: cdns3: unconditional clock gating in
  cdnsp_sky1_system_suspend()
- [Medium] usb: cdns3: cdnsp-sky1: clock refcount underflow after resume
  failure
- [Low] usb: cdns3: PS_PME_En not cleared in
  cdns_sky1_platform_suspend()
- [Low] usb: cdns3: missing MFD_SYSCON dependency for USB_CDNSP_SKY1
--

commit 7c5b7619382aea292ee4e64d01413bfd07b37097
Author: Hongliang Yang <hongliang.yang@cixtech.com>

usb: cdns3: Add CIX Sky1 glue driver

This commit adds a glue driver for the Cadence USBSSP DRD controller on
CIX Sky1 SoCs, managing clocks, resets, S5 syscon mode straps, and RCSU
registers.

> diff --git a/drivers/usb/cdns3/Kconfig b/drivers/usb/cdns3/Kconfig
> index 39ad23d1ada89..678cedd8de97b 100644
> --- a/drivers/usb/cdns3/Kconfig
> +++ b/drivers/usb/cdns3/Kconfig
> @@ -77,6 +77,19 @@ config USB_CDNSP_PCI
>  	  If you choose to build this driver as module it will
>  	  be dynamically linked and module will be called cdnsp-pci.ko
>  
> +config USB_CDNSP_SKY1
> +	tristate "Cadence USB3 support on CIX Sky1 SoC platforms"
> +	depends on ARCH_CIX || COMPILE_TEST
> +	default USB_CDNS3

[Severity: Low]
Should USB_CDNSP_SKY1 select or depend on MFD_SYSCON?  Without
CONFIG_MFD_SYSCON, syscon_regmap_lookup_by_phandle_args() is a stub that
returns ERR_PTR(-ENOTSUPP), so cdnsp_sky1_probe() always fails.

[ ... ]

> diff --git a/drivers/usb/cdns3/cdnsp-sky1.c b/drivers/usb/cdns3/cdnsp-sky1.c
> new file mode 100644
> index 0000000000000..c5d0ff9141cc9
> --- /dev/null
> +++ b/drivers/usb/cdns3/cdnsp-sky1.c

[ ... ]

> +static int cdnsp_sky1_clk_enable_resume(struct device *dev)
> +{
> +	int i, ret;
> +	struct cdnsp_sky1 *data = dev_get_drvdata(dev);
> +	struct clk **cix_usb_clks = data->cix_usb_clks;
> +
> +	for (i = 0; i < CIX_USB_CLK_OFF_NUM; i++) {
> +		ret = clk_prepare_enable(cix_usb_clks[i]);
> +		if (ret) {
> +			dev_err(dev, "failed to enable clock %s: %d\n",
> +				cix_usb_clk_names[i], ret);
> +			goto err_usb_clks;
> +		}
> +	}
> +	return 0;
> +
> +err_usb_clks:
> +	while (--i >= 0)
> +		clk_disable_unprepare(cix_usb_clks[i]);
> +	return ret;
> +};

[Severity: Medium]
Can a failure here underflow the enable and prepare counts of the sof and
axi clocks?

If clk_prepare_enable() fails during system resume, those two clocks are
left disabled and unprepared, but a later cdnsp_sky1_remove(),
cdnsp_sky1_shutdown(), or cdnsp_sky1_system_suspend() still calls
clk_disable_unprepare() on them unconditionally.

[ ... ]

> +	if (suspend) {
> +		while (count--) {
> +			value = readl(hcd->regs + XECP_PM_PMCSR);
> +			value &= ~PS_MASK;
> +			value |= PS_D3;
> +			if (wakeup)
> +				value |= PS_PME_En;
> +			writel(value, hcd->regs + XECP_PM_PMCSR);

[Severity: Low]
Should this clear PS_PME_En when wakeup is false?  If PS_PME_En is already
set in XECP_PM_PMCSR (for example from initial state or after a prior
resume timeout), suspending with wakeup disabled leaves PME wakeup
signaling enabled in hardware.  Also, cdns3_controller_suspend() and
cdns3_controller_resume() ignore the return value of platform_suspend(),
so the -ETIMEDOUT returned by cdns_sky1_platform_suspend() is discarded.

[ ... ]

> +	cdns_sky1_pdata = kzalloc(sizeof(*cdns_sky1_pdata), GFP_KERNEL);
> +	if (!cdns_sky1_pdata) {
> +		ret = -ENOMEM;
> +		goto err_quiesce;
> +	}
> +	cdns_sky1_pdata->platform_suspend = cdns_sky1_platform_suspend;
> +	cdns_sky1_pdata->quirks = CDNS3_DEFAULT_PM_RUNTIME_ALLOW;
> +	cdns_sky1_auxdata[0].platform_data = cdns_sky1_pdata;
> +	ret = of_platform_populate(node, NULL, cdns_sky1_auxdata, dev);
> +	if (ret) {
> +		dev_err(dev, "failed to create children: %d\n", ret);
> +		kfree(cdns_sky1_pdata);
> +		goto err_quiesce;
> +	}

[Severity: Medium]
Can this leak cdns_sky1_pdata when no child device is created?

of_platform_populate() calls of_platform_bus_create() with matches set to
NULL:

drivers/of/platform.c:of_platform_bus_create() {
    ...
	dev = of_platform_device_create_pdata(bus, bus_id, platform_data, parent);
	if (!dev || !of_match_node(matches, bus))
		return 0;
    ...
}

Because of_platform_bus_create() returns 0 even when
of_platform_device_create_pdata() fails or when no child node matches
"cdns,usb3", of_platform_populate() returns 0 whenever node is non-NULL.
The if (ret) branch is unreachable and cdns_sky1_pdata is leaked if no
child takes ownership of it (or double freed in platform_device_release()
if more than one child matches "cdns,usb3").

[ ... ]

> +/*
> + * 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.
> + *
> + * The resets are left alone here. Asserting them throws away the OTG
> + * registers programmed by cdns_drd_init(), and nothing programs them again
> + * after the child device resumes, so the controller keeps its state across
> + * system suspend and only the clocks are gated.
> + */
> +static int cdnsp_sky1_system_suspend(struct device *dev)
> +{
> +	cdnsp_sky1_clk_disable_suspend(dev);
> +
> +	return 0;
> +}

[Severity: Medium]
Should cdnsp_sky1_system_suspend() skip gating the sof and axi clocks when
wakeup is enabled or the controller has not entered D3?

As the comment notes, disabling sof and axi prevents port status change
wakeup interrupts, yet cdnsp_sky1_system_suspend() gates both clocks even
when device_may_wakeup() is true on the child device.  In addition,
cdns_sky1_platform_suspend() only puts the controller into D3 and waits for
AXI_CLOCK_VALID to clear when cdns->role == USB_ROLE_HOST (and its
-ETIMEDOUT return is ignored by cdns3_controller_suspend()), so in device
or idle OTG role, or after a D3 entry timeout, this gates the AXI and SOF
clocks while AXI_CLOCK_VALID is still asserted.

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