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

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [High] Error paths in `stm32_usb2phy_enable` and `stm32_usb2phy_disable` fail to correctly rollback the `en_refcnt` atomic counter, leading to permanent state lockup or leaked resources.
- [High] Lack of serialization in hardware initialization allows concurrent callers to bypass initialization and access unready hardware.
- [High] `stm32_usb2phy_set_mode()` fails to clear `SYSCFG_USB2PHY2CR_VBUSVLDEXTSEL` when switching from device mode to host mode.
- [High] The driver inappropriately treats an optional regulator as mandatory by unconditionally failing probe on `-ENODEV`.
- [Medium] `stm32_usb2phy2_init` hardcodes `USB_ROLE_NONE`, erasing submode state across re-init cycles.
- [Medium] Uncoordinated access to the `SYSCFG_USB2PHY2CR_USB2PHY2CMN` bit between the PHY mode setting and the Clock prepare/unprepare operations.
--

--- Patch [2]: [PATCH v3 2/9] phy: stm32: Add support for ST STM32MP25 USB2-FEMTO PHY ---
commit 6743b66f90992ce83291ec2073d75b9d6849c462
Author: Pankaj Dev <[email protected]>
Subject: phy: stm32: Add support for ST STM32MP25 USB2-FEMTO PHY

This commit adds the USB2 PHY driver for STM32MP25 USB2 controllers. It
supports the USB 2.0 host-only controller and the USB 2.0 part of the DWC3
controller.

> diff --git a/drivers/phy/st/phy-stm32-usb2phy.c b/drivers/phy/st/phy-stm32-usb2phy.c
> new file mode 100644
> --- /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]
Can concurrent callers bypass hardware initialization here? If two threads
call stm32_usb2phy_enable() at the same time, it seems the second caller
will observe the incremented counter and return immediately, while the first
thread is still executing the setup sequence. Would it be safer to use a
driver-level mutex to serialize initialization instead of an atomic check?

> +
> +	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]
If stm32_usb2phy_enable() returns -EINVAL for an unsupported clock rate,
does en_refcnt remain incremented? It looks like subsequent calls might see
an inflated count and return success without doing any hardware initialization.

> +
> +	ret = regmap_update_bits(phy_dev->regmap,
[ ... ]
> +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;
> +
> +	ret = reset_control_assert(phy_dev->rstc);
> +	if (ret)
> +		return ret;

[Severity: High]
If reset_control_assert() fails, does the early return leave en_refcnt
decremented while resources are still enabled? This might lead to a state
mismatch and resource leaks if the hardware is out of sync with the reference
count.

> +
> +	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;

[Severity: High]
When switching back to host mode, is the SYSCFG_USB2PHY2CR_VBUSVLDEXTSEL bit
left set? It appears omitted from this default mask.

If it was set during a switch to PHY_MODE_USB_DEVICE, it looks like it won't
be cleared when returning to PHY_MODE_USB_HOST, which might leave the
hardware multiplexer stuck on external VBUS sensing.

> +	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: Medium]
Can toggling the CMN clock corrupt the mode configuration? The PHY framework
sets the SYSCFG_USB2PHY2CR_USB2PHY2CMN bit here, but the clock framework
clears this exact same bit in stm32_usb2phy_clkcmn_prepare(). This
uncoordinated access might step on the PHY's expected state.

> +		if (submode != USB_ROLE_NONE)
> +			val |= SYSCFG_USB2PHY2CR_VBUSVLDEXT;
> +		mask |= SYSCFG_USB2PHY2CR_VBUSVLDEXTSEL;
> +	} else {
> +		return -EINVAL;
> +	}
[ ... ]
> +static int stm32_usb2phy2_init(struct phy *phy)
> +{
> +	struct stm32_usb2phy *phy_dev = phy_get_drvdata(phy);
> +	int ret;
> +
> +	ret = stm32_usb2phy_enable(phy_dev);
> +	if (ret)
> +		return ret;
> +
> +	if (phy_dev->mode != PHY_MODE_INVALID) {
> +		ret = stm32_usb2phy_set_mode(phy, phy_dev->mode, USB_ROLE_NONE);

[Severity: Medium]
Does hardcoding USB_ROLE_NONE erase the previously configured submode state?
If the PHY was previously configured with a valid submode, calling phy_init()
again might explicitly clear the VBUSVALID and VBUSVLDEXT bits, leading to
a silent loss of VBUS detection state.

> +		if (ret) {
> +			stm32_usb2phy_disable(phy_dev);
> +			return ret;
> +		}
> +	}
> +
> +	phy_dev->is_init = true;
> +
> +	return 0;
> +}
[ ... ]
> +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: High]
Does returning PTR_ERR() unconditionally here defeat the purpose of using
an optional regulator? devm_regulator_get_optional() returns -ENODEV if the
regulator is absent in the device tree, which would cause the probe to fail
unconditionally on platforms without it.

> +
> +	phy_dev->regmap = syscon_node_to_regmap(dev->of_node->parent);
> +	if (IS_ERR(phy_dev->regmap))
> +		return dev_err_probe(dev, PTR_ERR(phy_dev->regmap), "Failed to get regmap\n");
[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2