Re: [PATCH v1 2/4] phy: cix: Add CIX Sky1 USB3 PHY driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [High] Uninitialized pointer dereference and double-free of OF node reference on probe error path.
- [High] The probe function fails to propagate error codes and always returns 0 (success) on failure paths.
- [High] Missing cleanup for dynamically allocated `phy_lookup` entries causes a memory leak and Use-After-Free.
- [High] Hardware resets are unconditionally asserted without locking after the PHY provider is registered, creating a race condition with consumer initialization.
- [Medium] The error path in `sky1_u3phy_init` mistakenly deasserts the hardware resets instead of asserting them.
- [Medium] Missing `MODULE_DEVICE_TABLE(of, ...)` macro prevents automatic module loading.
--

commit 2c8f0e43d1973dd21cdeec9d0d419e0f7d1939aa
Author: Hongliang Yang <hongliang.yang@cixtech.com>

    phy: cix: Add CIX Sky1 USB3 PHY driver
    
This patch adds a new PHY driver for the USB3 PHY found on CIX Sky1 SoCs.
The driver handles initialization, reset, and clock management for the
multi-port USB3 PHY.

> diff --git a/drivers/phy/cix/phy-cix-usb3.c b/drivers/phy/cix/phy-cix-usb3.c
> new file mode 100644
> index 0000000000000..3b6e2669b09d8
> --- /dev/null
> +++ b/drivers/phy/cix/phy-cix-usb3.c
[ ... ]
> +static int sky1_u3phy_init(struct cix_u3phy *u3phy)
> +{
[ ... ]
> +disable_ref_clk:
> +	clk_disable_unprepare(u3phy->ref_clk);
> +disable_apb_clk:
> +	clk_disable_unprepare(u3phy->apb_clk);
> +assert_reset_preset:
> +	reset_control_deassert(u3phy->preset);
> +	reset_control_deassert(u3phy->reset);

[Severity: Medium]
Does this error path correctly assert the reset lines? 

The code calls reset_control_deassert() instead of reset_control_assert(),
which leaves the PHY in a deasserted reset state while its clocks are
disabled. This might violate the intended quiescent hardware state and
could potentially lead to hardware lockups or excessive power consumption.

> +	u3phy->init = false;
> +	return ret;
> +}
[ ... ]
> +static int cix_u3phy_probe(struct platform_device *pdev)
> +{
[ ... ]
> +	device_for_each_child_node(dev, child_fn) {
> +		child_np = to_of_node(child_fn);
> +		if (!strncmp(fwnode_get_name(child_fn), "usb-port", 8) ||
> +		    !strncmp(fwnode_get_name(child_fn), "USB", 3))
> +			phy = devm_phy_create(dev, child_np, &cix_u3phy_ops);
> +		else
> +			continue;
> +		if (IS_ERR(phy)) {
> +			dev_err(dev, "failed to create phy: %s\n",
> +				fwnode_get_name(child_fn));
> +			goto put_child;

[Severity: High]
Does this error path intentionally return 0 instead of propagating the
error code?

The variable 'ret' is initialized to 0 and is not updated with the error
from devm_phy_create() before jumping to the cleanup label. This causes
the kernel framework to consider the driver successfully probed despite
the initialization failure.

> +		}
> +		phy_set_drvdata(phy, u3phy);
> +		phy_create_lookup(phy, fwnode_get_name(child_fn), dev_name(dev));

[Severity: High]
Is there a corresponding cleanup mechanism for this dynamically allocated
phy_lookup entry? 

The driver creates lookup entries using phy_create_lookup() but does not
seem to provide a .remove callback or devres action to call
phy_remove_lookup(). When the driver is unbound, devres frees the struct
phy, leaving dangling pointers in the global lookup list which could cause
a use-after-free for subsequent lookups by consumers.

> +	}
> +	phy_provider = devm_of_phy_provider_register(dev, of_phy_simple_xlate);
> +	if (IS_ERR(phy_provider)) {
> +		dev_err(dev, "failed to register phy provider\n");
> +		goto put_child;

[Severity: High]
Similar to the devm_phy_create() error path, does this drop the error code
from devm_of_phy_provider_register()?

Additionally, does jumping to put_child here cause a double-free or a
dereference of an uninitialized pointer? 

If the device_for_each_child_node loop executed zero times, child_np is
completely uninitialized. If the loop executed at least once and completed
normally, child_np holds a pointer to the last child whose reference was
already released by the iterator's final condition check.

> +	}
> +	reset_control_assert(u3phy->reset);
> +	reset_control_assert(u3phy->preset);

[Severity: High]
Is it safe to unconditionally assert these hardware resets here without
locking after the PHY provider is registered? 

A deferred consumer could immediately acquire the exposed PHY and call
phy_init(), which initializes the PHY and deasserts resets while holding a
mutex. Concurrently, this probe function unconditionally executes
reset_control_assert() without holding the mutex, which might forcefully
reset the active PHY hardware.

> +	return 0;
> +
> +put_child:
> +	of_node_put(child_np);
> +	return ret;
> +}
> +
> +static const struct of_device_id cix_u3phy_dt_match[] = {
> +	{
> +		.compatible = "cix,sky1-usb3-phy",
> +		.data = &sky1_u3phy_cfg
> +	},
> +	{ /* sentinel */ }
> +};

[Severity: Medium]
Is the MODULE_DEVICE_TABLE(of, ...) macro missing here? 

Without it, the driver module cannot be automatically loaded by udev or kmod
based on the Device Tree compatible string.

> +
> +static struct platform_driver cix_usb3_phy_driver = {
> +	.probe		= cix_u3phy_probe,
> +	.driver		= {
> +		.name	= "cix-usb3-phy",
> +		.of_match_table = cix_u3phy_dt_match,
> +	},
> +};

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

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy