Re: [PATCH 2/4] usb: cdns3: sky1: Add cdnsp-sky1 glue driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [High] Probe returns success (0) when memory resource retrieval fails, leading to a NULL pointer dereference.
- [High] Probe forcefully overwrites the mode strap configuration for all USB controllers on the SoC.
- [Medium] Resource leak of clocks on early probe error paths.
- [Medium] Unconditional assignment of dev_pm_ops violates Power Management Subsystem rules.
- [Medium] Commit message and implementation mismatch regarding hardware resets.
- [Low] Using of_match_ptr() with a statically allocated array causes compiler warnings.
--

commit a626bc93f86adbca6a4dce0b669265133a32a8cb
Author: Peter Chen <peter.chen@cixtech.com>

usb: cdns3: sky1: Add cdnsp-sky1 glue driver

Add a CIX sky1 platform glue driver with Kconfig and Makefile entry.
It calls APIs exported from cdns3-plat.c for probe/remote/suspend/resume
routines.

> 	  Glue driver for the Cadence USB dual-role controllers on CIX Sky1
> 	  (device tree compatible cix,sky1-usb3). It enables clocks and resets
> 	  from the SoC, then uses the shared cdns3 platform core (cdns.ko).

The commit message mentions it enables clocks and resets from the SoC, but is
there missing code to handle the resets? The implementation includes linux/reset.h
but doesn't appear to request or interact with any reset lines.

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

[ ... ]

> +static int cdnsp_sky1_set_all_controllers_otg(struct regmap *syscon)
> +{
> +	int id, ret;
> +
> +	for (id = 0; id < SKY1_USB_S5_NUM; id++) {
> +		ret = cdnsp_sky1_set_mode_by_id(syscon, id, MODE_STRAP_OTG);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	return 0;
> +}

Does this code unintentionally alter the state of other independent USB
controllers? It appears to iterate through all controllers on the SoC and
updates them to MODE_STRAP_OTG, which might affect concurrent controllers
that are explicitly configured as host-only (like U2_HOST0_ID).

[ ... ]

> +static int cdnsp_sky1_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct cdnsp_sky1 *priv;
> +	struct cdns *cdns;
> +	struct cdns3_probe_data probe_data;
> +	struct resource *res;
> +	int ret;
> +
> +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	priv->dev = dev;
> +	priv->num_clks = ARRAY_SIZE(cdnsp_sky1_cdns_core_clks);
> +	priv->clks = devm_kmemdup(dev, cdnsp_sky1_cdns_core_clks,
> +				   sizeof(cdnsp_sky1_cdns_core_clks), GFP_KERNEL);
> +	if (!priv->clks)
> +		return -ENOMEM;
> +
> +	ret = devm_clk_bulk_get(dev, priv->num_clks, priv->clks);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "failed to get clocks\n");
> +
> +	ret = clk_bulk_prepare_enable(priv->num_clks, priv->clks);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "failed to enable clocks\n");
> +
> +	priv->usb_syscon = syscon_regmap_lookup_by_phandle(dev->of_node,
> +							   "cix,syscon-usb");
> +	if (IS_ERR(priv->usb_syscon))
> +		return dev_err_probe(dev, PTR_ERR(priv->usb_syscon),
> +				     "failed to get cix,syscon-usb regmap\n");
> +
> +	ret = cdnsp_sky1_set_all_controllers_otg(priv->usb_syscon);
> +	if (ret)
> +		return dev_err_probe(dev, ret,
> +				     "failed to set USB controllers to OTG strap\n");

Can these error paths cause a resource leak? The clocks were previously enabled
via clk_bulk_prepare_enable(), but these direct returns bypass the err_clk
label where clk_bulk_disable_unprepare() is called.

> +
> +	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "glue");
> +	if (!res)
> +		goto err_clk;

If this resource retrieval fails, does the probe function return success?

The ret variable was previously set to 0 by the successful call to
cdnsp_sky1_set_all_controllers_otg(), so jumping to err_clk without updating
ret to an error code like -ENODEV would result in returning 0.

Could this lead to a NULL pointer dereference during subsequent power
management operations? Since the device would be considered bound without
calling cdns3_core_probe(), dev_get_drvdata() might return NULL in
callbacks like cdnsp_sky1_runtime_suspend().

[ ... ]

> +static const struct of_device_id cdnsp_sky1_of_match[] = {
> +	{ .compatible = "cix,sky1-usb3" },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(of, cdnsp_sky1_of_match);
> +
> +static struct platform_driver cdnsp_sky1_driver = {
> +	.probe		= cdnsp_sky1_probe,
> +	.remove		= cdnsp_sky1_remove,
> +	.driver		= {
> +		.name		= "cdnsp-sky1",
> +		.of_match_table	= of_match_ptr(cdnsp_sky1_of_match),

If the kernel is built with CONFIG_OF disabled, of_match_ptr() evaluates to
NULL. Since the cdnsp_sky1_of_match array is declared statically and without
__maybe_unused annotations, will this result in an unused variable compiler
warning?

> +		.pm		= &cdnsp_sky1_pm_ops,

Does unconditionally assigning the pm callback structure produce dead code when
CONFIG_PM is disabled? According to the power management subsystem guidelines,
this should likely be wrapped with the pm_ptr() macro so it evaluates to NULL
without PM support.

> +	},
> +};

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260511024244.981941-1-peter.chen@cixtech.com?part=2