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

[sashiko-bot@kernel.org]

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 <kkartik@nvidia.com>

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.

> diff --git a/drivers/phy/tegra/phy-tegra-mphy.c b/drivers/phy/tegra/phy-tegra-mphy.c
> index 0000000000000..01cafe1c7477f
> --- /dev/null
> +++ b/drivers/phy/tegra/phy-tegra-mphy.c

[ ... ]

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