Re: [PATCH v2 2/9] phy: stm32: Add support for ST STM32MP25 USB2-FEMTO PHY
From: sashiko-bot@kernel.org
Date: 2026-08-16 21:50:36
Also in:
linux-devicetree
Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider: - [High] Error paths in `stm32_usb2phy_enable()` and `stm32_usb2phy_disable()` fail to unwind the `en_refcnt` atomic counter, leading to state corruption and leaked resources. - [High] Incorrect handling of `devm_regulator_get_optional()` return value unconditionally fails probe on systems without the regulator. - [High] Lockless `en_refcnt` check introduces a TOCTOU race condition allowing concurrent callers to bypass hardware initialization. - [Medium] Unchecked dereferences of `dev->of_node` and `phy_dev->hw_data` during probe allow a local Denial of Service (Oops) if bound without a Device Tree node. --
--- Patch [2]: [PATCH v2 2/9] phy: stm32: Add support for ST STM32MP25 USB2-FEMTO PHY ---commit e1cc42ded5599cd15a5c991b72a87e2e909f244d Author: Pankaj Dev [off-list ref] phy: stm32: Add support for ST STM32MP25 USB2-FEMTO PHY Add USB2 PHY driver for STM32MP25 USB2 controllers, which includes the USB2.0 host-only controller and USB 2.0 part of the DWC3 controller. Two such PHYs in total are present in STM32MP25 SoC.
quoted hunk ↗ jump to hunk
diff --git a/drivers/phy/st/phy-stm32-usb2phy.c b/drivers/phy/st/phy-stm32-usb2phy.c new file mode 100644 index 0000000000000..a5cc7b855c61f --- /dev/null +++ b/drivers/phy/st/phy-stm32-usb2phy.c@@ -0,0 +1,361 @@
[ ... ]
quoted hunk ↗ jump to hunk
+static int stm32_usb2phy_enable(struct stm32_usb2phy *phy_dev) +{ + const struct stm32mp2_usb2phy_hw_data *phy_data = phy_dev->hw_data; + unsigned long rate; + int refsel, ret; + + /* Check if a phy is already init or clk48 in use */ + if (atomic_inc_return(&phy_dev->en_refcnt) > 1) + return 0;
[Severity: High] Is it safe to rely on this lockless check? If two threads call stm32_usb2phy_enable() concurrently (for instance, via phy_init and clk_prepare), the second thread will increment en_refcnt to 2 and return success immediately. Will the second thread then proceed to use the hardware before the first thread has actually finished enabling the regulators and clocks?
quoted hunk ↗ jump to hunk
+ + 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] Does this error path leak the en_refcnt? If we return here, the counter remains incremented. Subsequent calls to stm32_usb2phy_enable() will then see a value > 1 and instantly return success without initializing the hardware. [ ... ]
quoted hunk ↗ jump to hunk
+static int stm32_usb2phy_disable(struct stm32_usb2phy *phy_dev) +{ + int ret; + + /* Check if a phy is still init or clk48 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] Does this error path need to restore the en_refcnt? If reset_control_assert() fails, we return without disabling the clock or regulator, and the counter remains decremented. [ ... ]
+static int stm32_usb2phy_probe(struct platform_device *pdev)
+{[ ... ]
quoted hunk ↗ jump to hunk
+ 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] If this regulator is truly optional, shouldn't we handle the -ENODEV return value gracefully instead of failing the probe unconditionally? If it's missing from the Device Tree, devm_regulator_get_optional() will return -ENODEV and cause the probe to fail on valid hardware configurations.
+ + phy_dev->regmap = syscon_node_to_regmap(dev->of_node->parent);
[Severity: Medium] Can dev->of_node be NULL here? If a user manually instantiates the device via sysfs or software nodes without an associated Device Tree node, this will result in a NULL pointer dereference when accessing parent.
quoted hunk ↗ jump to hunk
+ 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); + + phy_dev->hw_data = device_get_match_data(dev);
[Severity: Medium] Should we check if phy_dev->hw_data is NULL before proceeding? In a scenario without a Device Tree node, device_get_match_data() will return NULL, which is later dereferenced in the enable and disable paths. -- Sashiko AI review · https://sashiko.dev/#/patchset/20260816213849.1044073-1-marex@nabladev.com?part=2 -- linux-phy mailing list linux-phy@lists.infradead.org https://lists.infradead.org/mailman/listinfo/linux-phy