Re: [PATCH v12 05/36] phy: rockchip: usbdp: Handle missing clock-names DT property gracefully

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] Missing NULL check for clock IDs when iterating over clocks, leading to a NULL pointer dereference with non-compliant device trees.
- [Medium] Resource leak of clock enable counts on probe error paths and upon driver unbinding due to missing `.remove` callback.
--

--- Patch [5]: [PATCH v12 05/36] phy: rockchip: usbdp: Handle missing clock-names DT property gracefully ---
commit a11924d81c315e2223d2b7de676184666d98b48a
Author: Sebastian Reichel <sebastian.reichel@collabora.com>

phy: rockchip: usbdp: Handle missing clock-names DT property gracefully

This commit fixes a NULL pointer dereference in rk_udphy_clk_init() by
ensuring the clock ID is not NULL before calling strncmp(). This gracefully
handles cases where the device tree specifies 'clocks' but misses the required
'clock-names' property.

Closes: https://sashiko.dev/#/message/20260619154349.071321F000E9%40smtp.kernel.org

> diff --git a/drivers/phy/rockchip/phy-rockchip-usbdp.c b/drivers/phy/rockchip/phy-rockchip-usbdp.c
> index 2845a012eafcc..3fc8222fcaec2 100644
> --- a/drivers/phy/rockchip/phy-rockchip-usbdp.c
> +++ b/drivers/phy/rockchip/phy-rockchip-usbdp.c
> @@ -432,6 +432,9 @@ static int rk_udphy_clk_init(struct rk_udphy *udphy, struct device *dev)
>  
>  	/* used for configure phy reference clock frequency */
>  	for (i = 0; i < udphy->num_clks; i++) {
> +		if (!udphy->clks[i].id)
> +			continue;
> +

[Severity: High]
This is a pre-existing issue, but does the same NULL pointer dereference
exist in other rockchip PHY drivers when 'clock-names' is missing?

For example, looking at rockchip_usb2phy_clk480m_register() in
phy-rockchip-inno-usb2.c:

	for (i = 0; i < rphy->num_clks; i++) {
		if (!strncmp(rphy->clks[i].id, "phyclk", 6)) {
			refclk = rphy->clks[i].clk;

And similarly in rockchip_combphy_parse_dt() in phy-rockchip-naneng-combphy.c:

	for (i = 0; i < priv->num_clks; i++) {
		if (!strncmp(priv->clks[i].id, "ref", 3)) {
			priv->refclk = priv->clks[i].clk;

Do these drivers also need similar checks before calling strncmp() to avoid
crashing with non-compliant device trees?

>  		if (!strncmp(udphy->clks[i].id, "refclk", 6)) {
>  			udphy->refclk = udphy->clks[i].clk;
>  			break;

[Severity: Medium]
This is a pre-existing issue, but does this driver leak clock enable counts
during probe failures or upon unbinding?

Looking at rk_udphy_probe(), if rk_udphy_get_initial_status() detects that
the hardware was already enabled, it leaves the clocks manually enabled.
However, if a subsequent step fails, such as the orientation-switch setup:

	if (device_property_present(dev, "orientation-switch")) {
		ret = rk_udphy_setup_orien_switch(udphy);
		if (ret)
			return ret;
	}

The error path returns directly without disabling the clocks. If this fails
with -EPROBE_DEFER, will the leaked clock enable count increment on each
retry?

Additionally, the driver appears to lack a .remove callback entirely:

static struct platform_driver rk_udphy_driver = {
	.probe		= rk_udphy_probe,
	.driver		= {
		.name	= "rockchip-usbdp-phy",

Does unbinding the driver while the PHY is active leave the clocks
permanently enabled and reset states deasserted?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260710-rockchip-usbdp-cleanup-v12-0-8b41a9a9bef0@collabora.com?part=5