Re: [PATCH v2 3/5] phy: cix: Add CIX Sky1 USB3 PHY driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Hardware resets asserted after exposing the PHY provider, risking a race condition with consumers.
- [Medium] Teardown asymmetry and inverted unroll order for hardware clocks and resets.
- [Medium] Unsafe string comparison using `fwnode_get_name()` without NULL check.
--

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

phy: cix: Add CIX Sky1 USB3 PHY driver

This commit adds a PHY driver for the USB3 PHY on CIX Sky1 SoCs, handling
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
> --- /dev/null
> +++ b/drivers/phy/cix/phy-cix-usb3.c
[ ... ]
> +static int sky1_u3phy_exit(struct cix_u3phy *u3phy)
> +{
> +	u3phy->init_count--;
> +	if (u3phy->init && u3phy->init_count == 0) {
> +		reset_control_assert(u3phy->reset);
> +		reset_control_assert(u3phy->preset);
> +		clk_disable_unprepare(u3phy->apb_clk);
> +		clk_disable_unprepare(u3phy->ref_clk);

[Severity: Medium]
Does this disable the clocks in the correct order? The initialization path
enables apb_clk before ref_clk. To ensure a proper LIFO teardown sequence,
should ref_clk be disabled before apb_clk here?

[ ... ]
> +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_assert(u3phy->preset);
> +	reset_control_assert(u3phy->reset);

[Severity: Medium]
Will this error unroll sequence leave the hardware in an unrecoverable state?

This path disables the clocks before asserting the hardware resets. This
contradicts the normal teardown sequence in sky1_u3phy_exit(), which asserts
the resets before disabling the clocks.

[ ... ]
> +static int cix_u3phy_probe(struct platform_device *pdev)
> +{
[ ... ]
> +	device_for_each_child_node_scoped(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))

[Severity: Medium]
Can this trigger a NULL pointer dereference? fwnode_get_name() can return
NULL for certain software nodes.

Additionally, does using strncmp() with a fixed length incorrectly match
arbitrary suffixes like "usb-port-foo" while failing to properly handle
unit addresses like "usb-port@0"? Could fwnode_name_eq() be used here
instead?

[ ... ]
> +	phy_provider = devm_of_phy_provider_register(dev, of_phy_simple_xlate);
> +	if (IS_ERR(phy_provider)) {
> +		ret = PTR_ERR(phy_provider);
> +		dev_err(dev, "failed to register phy provider\n");
> +		return ret;
> +	}
> +	reset_control_assert(u3phy->reset);
> +	reset_control_assert(u3phy->preset);

[Severity: High]
Does asserting these resets after exposing the PHY provider create a race
condition with consumers?

Once devm_of_phy_provider_register() completes, a consumer driver could
concurrently probe and call phy_init(). If the consumer initializes the
PHY and deasserts the resets, this probe thread will then continue and
unconditionally assert the resets without holding u3phy->mutex. This would
destroy the initialized state while the consumer is actively using the PHY.

> +	return 0;
> +}

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