[PATCH v5 2/2] phy: stm32: Add support for ST STM32MP25 USB2-FEMTO PHY

[sashiko-bot at kernel.org]

Thank you for your contribution! Sashiko AI review found 11 potential issue(s) to consider:
- [High] Lockless atomic counter allows concurrent callers to access uninitialized hardware.
- [High] Missing decrement of `en_refcnt` on error paths permanently bricks hardware initialization.
- [High] Inconsistent PM suspend/resume logic fails to power down hardware if the clock provider is actively used.
- [High] Logical conflict between Clock Framework and PHY set_mode over the CMN register bit causes state desynchronization.
- [High] Incorrect parsing of `reg` property using `of_get_address()` potentially corrupts syscon register 0 on 64-bit platforms.
- [Medium] Failure to assert reset in `stm32_usb2phy_disable` leaves clock and regulator permanently enabled.
- [Medium] Driver strictly fails probe if the ostensibly 'optional' vdd33 regulator is missing, breaking the `_optional` contract.
- [Medium] Inconsistent masking in `stm32_usb2phy_set_mode` prevents clearing `VBUSVLDEXTSEL` on transition to HOST mode.
- [Medium] Blind dereferences of `dev->of_node` and `device_get_match_data()` will crash if bound manually.
- [Low] Build failure due to passing a non-constant mask to `field_prep()`.
- [Low] Commit message contradicts code regarding PHY2 set_mode support.
--

commit 291cd4f88294f76f9e6908ba105bb434b48f3830
Author: Pankaj Dev <pankaj.dev at st.com>

phy: stm32: Add support for ST STM32MP25 USB2-FEMTO PHY

This commit adds a USB2 PHY driver for STM32MP25 USB2 controllers. It
supports two slightly different PHYs on the SoC, each requiring a
different compatible string. The driver handles initialization, clocks,
resets, and regulators for these PHYs.

> V3: - Update copyright years
>     - Register two sets of clock, one 480 MHz PLL and one CMN
>     - Split PHY ops per PHY, since the PHY2 does not have set mode ops

[Severity: Low]
This isn't a bug, but does PHY2 actually support set mode ops?
The commit message indicates PHY2 does not have set mode ops, but they are
assigned in the code for stm32_usb2phy2_data.

> diff --git a/drivers/phy/st/phy-stm32-usb2phy.c b/drivers/phy/st/phy-stm32-usb2phy.c
> index 0000000000000..e173557a3cb59
> --- /dev/null
> +++ b/drivers/phy/st/phy-stm32-usb2phy.c

[ ... ]

> +static int stm32_usb2phy_enable(struct stm32_usb2phy *phy_dev)
> +{
> +	const struct stm32mp2_usb2phy_hw_data *phy_data = phy_dev->hw_data;
> +	unsigned int refsel;
> +	unsigned long rate;
> +	int ret;
> +
> +	/* Check if a phy is already init or clkpll in use */
> +	if (atomic_inc_return(&phy_dev->en_refcnt) > 1)
> +		return 0;

[Severity: High]
Does this lockless early return allow a concurrent caller to proceed with
hardware access while initialization is still in flight?

If the PHY and Clock frameworks concurrently invoke stm32_usb2phy_enable(),
such as phy_init() from one driver and clk_prepare() from another, could
thread A increment the counter to 1 and begin enabling regulators, while
thread B increments to 2 and returns success immediately, using the
uninitialized hardware?

> +
> +	rate = clk_get_rate(phy_dev->phyref);
> +	if (rate == 19200000)
> +		refsel = 0;
> +	else if (rate == 20000000)
> +		refsel = 1;
> +	else if (rate == 24000000)
> +		refsel = 2;
> +	else
> +		return -EINVAL;

[Severity: High]
Is it possible this error path permanently bricks hardware initialization?

By returning an error without decrementing the en_refcnt, the reference count
becomes artificially elevated. Subsequent initialization attempts might bypass
initialization entirely while the hardware remains broken.

> +
> +	ret = regmap_update_bits(phy_dev->regmap,
> +				 phy_dev->cr_offset,
> +				 phy_data->phyrefsel_mask,
> +				 field_prep(phy_data->phyrefsel_mask, refsel));

[Severity: Low]
Will this cause a build failure? 

field_prep() strictly requires a compile-time constant mask, but
phy_data->phyrefsel_mask is resolved at runtime via a pointer dereference.

> +	if (ret)
> +		return ret;

[Severity: High]
Could this error return leave the en_refcnt artificially elevated, preventing
subsequent initialization attempts from ever succeeding?

> +
> +	ret = regulator_enable(phy_dev->vdd33);
> +	if (ret)
> +		return ret;
> +
> +	ret = clk_prepare_enable(phy_dev->phyref);
> +	if (ret)
> +		goto error_regdis;
> +
> +	ret = reset_control_deassert(phy_dev->rstc);
> +	if (ret)
> +		goto error_clkdis;
> +
> +	return 0;
> +
> +error_clkdis:
> +	clk_disable_unprepare(phy_dev->phyref);
> +error_regdis:
> +	regulator_disable(phy_dev->vdd33);
> +
> +	return ret;

[Severity: High]
Does this final cleanup error path also leave the en_refcnt incremented?

> +}
> +
> +static int stm32_usb2phy_disable(struct stm32_usb2phy *phy_dev)
> +{
> +	int ret;
> +
> +	/* Check if a phy is still init or clkpll in use */
> +	if (atomic_dec_return(&phy_dev->en_refcnt) > 0)
> +		return 0;

[Severity: High]
Does this lockless early return also allow teardown races?

> +
> +	ret = reset_control_assert(phy_dev->rstc);
> +	if (ret)
> +		return ret;

[Severity: Medium]
Will failing to assert the reset here leak the clock and regulator resources,
leaving them permanently enabled?

> +
> +	clk_disable_unprepare(phy_dev->phyref);
> +
> +	return regulator_disable(phy_dev->vdd33);
> +}
> +
> +static int stm32_usb2phy_set_mode(struct phy *phy, enum phy_mode mode, int submode)
> +{
> +	struct stm32_usb2phy *phy_dev = phy_get_drvdata(phy);
> +	u32 mask = SYSCFG_USB2PHY2CR_USB2PHY2CMN |
> +		   SYSCFG_USB2PHY2CR_VBUSVALID |
> +		   SYSCFG_USB2PHY2CR_VBUSVLDEXT;
> +	u32 val = 0;
> +	int ret;
> +
> +	if (mode == PHY_MODE_USB_HOST) {
> +		if (submode != USB_ROLE_NONE)
> +			val = SYSCFG_USB2PHY2CR_VBUSVALID;
> +	} else if (mode == PHY_MODE_USB_DEVICE) {
> +		val = SYSCFG_USB2PHY2CR_USB2PHY2CMN |
> +		      SYSCFG_USB2PHY2CR_VBUSVLDEXTSEL;

[Severity: High]
Is there a logical conflict over the SYSCFG_USB2PHY2CR_USB2PHY2CMN bit?

The Clock Framework's internal state could become desynchronized if
stm32_usb2phy_clkcmn_prepare() clears this bit, but stm32_usb2phy_set_mode()
is concurrently called to switch to DEVICE mode and directly sets this exact
same bit via regmap_update_bits().

> +		if (submode != USB_ROLE_NONE)
> +			val |= SYSCFG_USB2PHY2CR_VBUSVLDEXT;
> +		mask |= SYSCFG_USB2PHY2CR_VBUSVLDEXTSEL;

[Severity: Medium]
Does adding the VBUSVLDEXTSEL bit to the mask dynamically here instead of in
the base mask prevent clearing it on subsequent transitions to HOST mode?

If it is omitted from the mask during a transition to PHY_MODE_USB_HOST,
regmap_update_bits() won't be able to clear it.

> +	} else {
> +		return -EINVAL;
> +	}

[ ... ]

> +static int stm32_usb2phy_probe(struct platform_device *pdev)
> +{
> +	struct phy_provider *phy_provider;
> +	struct device *dev = &pdev->dev;
> +	struct stm32_usb2phy *phy_dev;
> +	char *pllname, *cmnname;
> +	const __be32 *offset;
> +	struct phy *phy;
> +	int ret;

[ ... ]

> +	phy_dev->phyref = devm_clk_get(dev, NULL);
> +	if (IS_ERR(phy_dev->phyref))
> +		return dev_err_probe(dev, PTR_ERR(phy_dev->phyref), "Failed to get phyref clk\n");
> +
> +	phy_dev->vdd33 = devm_regulator_get_optional(dev, "vdd33");
> +	if (IS_ERR(phy_dev->vdd33))
> +		return dev_err_probe(dev, PTR_ERR(phy_dev->vdd33), "Failed to get vdd3v3 supply\n");

[Severity: Medium]
Since this propagates -ENODEV when the regulator is absent in the DT, does
this strictly fail probe for platforms legitimately lacking the optional
vdd33 regulator, breaking the _optional contract?

> +
> +	phy_dev->regmap = syscon_node_to_regmap(dev->of_node->parent);

[Severity: Medium]
Will this unconditionally dereference dev->of_node? 

If the device is instantiated manually, such as via sysfs bind without a
backing Device Tree node, dev->of_node will be NULL, leading to a kernel
panic.

> +	if (IS_ERR(phy_dev->regmap))
> +		return dev_err_probe(dev, PTR_ERR(phy_dev->regmap), "Failed to get regmap\n");
> +
> +	offset = of_get_address(dev->of_node, 0, NULL, NULL);
> +	if (!offset)
> +		return dev_err_probe(dev, -EINVAL, "Failed to get regmap offset\n");
> +
> +	phy_dev->cr_offset = be32_to_cpu(*offset);

[Severity: High]
Does this logic incorrectly parse the reg property on 64-bit platforms?

If the parent node uses 2 address cells, which is common on STM32MP25,
*offset contains the high 32 bits of the address, which is typically 0.
This could cause the driver to incorrectly extract 0 and overwrite syscon
register 0 instead of the intended PHY control register.

> +
> +	phy_dev->hw_data = device_get_match_data(dev);
> +
> +	phy = devm_phy_create(dev, NULL, phy_dev->hw_data->phy_ops);

[Severity: Medium]
Could this unconditionally dereference phy_dev->hw_data if
device_get_match_data(dev) returns NULL when the device is bound manually?

> +	if (IS_ERR(phy))
> +		return dev_err_probe(dev, PTR_ERR(phy), "Failed to create PHY\n");

[ ... ]

> +static int stm32_usb2phy_suspend(struct device *dev)
> +{
> +	struct stm32_usb2phy *phy_dev = dev_get_drvdata(dev);
> +
> +	if (phy_dev->is_init)
> +		return stm32_usb2phy_disable(phy_dev);

[Severity: High]
Does this conditional logic incorrectly assume is_init and disable()
correctly account for all framework consumers?

If the hardware was enabled via the Clock API, en_refcnt is 1 but is_init
is false, so suspend does nothing. If enabled via both, calling
stm32_usb2phy_disable() merely decrements en_refcnt to 1 and returns early
without powering off the hardware. Could this fail to shut down the hardware
if the CCF clock is in use?

> +
> +	return 0;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260902141312.649406-1-marex@nabladev.com?part=2