Re: [PATCH 2/2] phy: tegra: Add Tegra264 MPHY driver

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Data race on shared `mphy->power_count` and clock state across multiple PHY instances.
- [High] TX PHY lacks power management operations, creating a fragile API contract that risks system crashes.
- [High] Wrong base address pointer used for `MPHY_RX_APB_CAPABILITY` registers.
- [Medium] Missing reset assertion for `mphy->rst_clk_ctl` in the normal `tegra_mphy_rx_power_off` path.
--

commit bceb08380e03847a80ce77db39c3ff271cbccb0c
Author: Kartik Rajput [off-list ref]

phy: tegra: Add Tegra264 MPHY driver

Add a driver for the MIPI M-PHY found on NVIDIA Tegra264 SoCs. The
M-PHY has two lanes, each with a transmit and a receive direction,
exposed as four PHYs to the UFS host controller.

[ ... ]

+static int tegra_mphy_rx_power_on(struct phy *phy)
+{
+	struct tegra_mphy *mphy = dev_get_drvdata(phy->dev.parent);
+	struct tegra_mphy_lane *lane = phy_get_drvdata(phy);
+	u32 value;
+	int err;
+
+	if (mphy->power_count++ == 0) {

            ^^^^^^^^^^^^^^^^^^^^^^^

[Severity: High]
Since l0 and l1 are distinct PHY instances exposed to the consumer, can
concurrent phy_power_on() or phy_power_off() calls race on this shared
mphy->power_count?

The PHY framework's internal mutex only serializes operations on a per-PHY
basis. An unprotected read-modify-write here could lead to corrupted
reference counts, unbalanced clock states, and hardware faults.

+		err = clk_bulk_prepare_enable(mphy->num_clks, mphy->clks);
+		if (err) {
+			mphy->power_count--;
+			dev_err(&phy->dev, "failed to enable clocks: %d\n", err);
+			return err;
+		}
+		reset_control_deassert(mphy->rst_clk_ctl);
+	}
+
+	reset_control_deassert(lane->rst_rx);
+	reset_control_deassert(lane->rst_tx);

                               ^^^^^^^^^^^^^^^^^^^^

[Severity: High]
Is it intentional that the RX power-on path handles the reset logic for the
independently exposed TX PHY?

The TX PHY currently lacks .power_on and .power_off callbacks and seems to
rely implicitly on the RX PHY to enable hardware clocks. If the TX PHY is
accessed via phy_configure() before the RX PHY is fully powered on, could
this result in unclocked memory-mapped I/O and trigger a synchronous
external abort?

+
+	err = tegra_mphy_rx_write_kick_go(lane, MPHY_RX_APB_VENDOR3B, MPHY_EQ_TIMEOUT);
+	if (err) {
+		dev_err(&phy->dev, "eq_timeout programming failed: %d\n", err);
+		goto err_reset;
+	}
+
+	err = tegra_mphy_rx_write_kick_go(lane, MPHY_RX_APB_VENDOR49, MPHY_PWR_CHANGE_CLK_BOOST);
+	if (err) {
+		dev_err(&phy->dev, "pwr_change_clk_boost programming failed: %d\n", err);
+		goto err_reset;
+	}
+
+	value = readl(lane->regs + MPHY_RX_APB_CAPABILITY_88_8B);

                      ^^^^^^^^^^

[Severity: High]
Should this use lane->rx_regs instead of lane->regs for offset application?

Using lane->regs misdirects the read/write operations to offsets 0x88 and
0x94 instead of 0x2088 and 0x2094 (since MPHY_RX_OFFSET is 0x2000), which
may lead to memory corruption and failed configurations.

+	value &= ~RX_HS_G1_SYNC_LENGTH_CAPABILITY(~0);
+	value |= RX_HS_G1_SYNC_LENGTH_CAPABILITY(0xf);
+	writel(value, lane->regs + MPHY_RX_APB_CAPABILITY_88_8B);
+
+	value = readl(lane->regs + MPHY_RX_APB_CAPABILITY_94_97);

                      ^^^^^^^^^^

[Severity: High]
Does this also need to use lane->rx_regs instead of lane->regs?

+	value &= ~RX_HS_G3_SYNC_LENGTH_CAPABILITY(~0);
+	value |= RX_HS_G3_SYNC_LENGTH_CAPABILITY(0xf);
+	value &= ~RX_HS_G2_SYNC_LENGTH_CAPABILITY(~0);
+	value |= RX_HS_G2_SYNC_LENGTH_CAPABILITY(0xf);
+	writel(value, lane->regs + MPHY_RX_APB_CAPABILITY_94_97);

[ ... ]

+static int tegra_mphy_rx_power_off(struct phy *phy)
+{
+	struct tegra_mphy *mphy = dev_get_drvdata(phy->dev.parent);
+	struct tegra_mphy_lane *lane = phy_get_drvdata(phy);
+
+	if (WARN_ON(mphy->power_count == 0))
+		return -EINVAL;
+
+	reset_control_assert(lane->rst_rx);
+	reset_control_assert(lane->rst_tx);
+
+	if (--mphy->power_count == 0)
+		clk_bulk_disable_unprepare(mphy->num_clks, mphy->clks);

                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

[Severity: Medium]
Should mphy->rst_clk_ctl be asserted here before or after disabling the
clocks?

The driver intentionally deasserts mphy->rst_clk_ctl in
tegra_mphy_rx_power_on() when power_count goes from 0 to 1, but omits the
corresponding reset_control_assert() in this successful power-off path,
which leaks the reset state across power cycles.

+
+	return 0;
+}

[ ... ]

+static const struct phy_ops tegra_mphy_tx_ops = {
+	.configure = tegra_mphy_tx_configure,
+	.calibrate = tegra_mphy_tx_calibrate,
+};

        ^^

[Severity: High]
Are .power_on and .power_off callbacks intentionally omitted for the TX PHY
ops?

Leaving the TX PHY exposed to configuration while in an unclocked state
creates a fragile API contract that risks system crashes.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260909-tegra264-mphy-v1-0-ce61a80dc065@nvidia.com?part=2

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