Re: [PATCH 4/4] phy: rockchip-samsung-dcphy: add MIPI D-PHY receiver support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:

New issues:
- [High] The RX power-on sequence fails to toggle the `apb_rst` reset line, potentially leaving the shared APB bus interface uninitialized if the RX direction is powered on before TX.
- [High] If `samsung_mipi_dphy_rx_lane_enable()` fails (e.g., due to a PHY_READY timeout), it returns an error but leaves the `PHY_ENABLE` bits set in the PHY registers and unconditionally deasserts the `s_phy_rst` core reset.
- [High] The `lanes` configuration field is shared between TX and RX, leading to state corruption and incorrect lane cleanup if both directions are configured independently.

Pre-existing issues:
- [High] Concurrent power_on calls for TX and RX can race due to a time-of-check to time-of-use (TOCTOU) vulnerability on the unprotected `peer->power_count`.
--

--- Patch [4]: [PATCH 4/4] phy: rockchip-samsung-dcphy: add MIPI D-PHY receiver support ---
commit 1a7ff981a5098deb181df94fdb6e07f8dc4040a9
Author: Jason Yang <jason98166@gmail.com>
phy: rockchip-samsung-dcphy: add MIPI D-PHY receiver support

This commit implements the receiver (CSI) direction of the D-PHY. It adds the
RX register layout, the HS-frequency settle table, and hooks the receiver
bring-up and teardown into the per-direction power and configuration paths.

> diff --git a/drivers/phy/rockchip/phy-rockchip-samsung-dcphy.c b/drivers/phy/rockchip/phy-rockchip-samsung-dcphy.c
> --- a/drivers/phy/rockchip/phy-rockchip-samsung-dcphy.c
> +++ b/drivers/phy/rockchip/phy-rockchip-samsung-dcphy.c

[ ... ]

> +static int samsung_mipi_dphy_rx_lane_enable(struct samsung_mipi_dcphy *samsung)
> +{
> +	u32 sts;
> +	int ret, i;
> +
> +	regmap_update_bits(samsung->regmap, DPHY_SC_GNR_CON0, PHY_ENABLE, PHY_ENABLE);
> +	for (i = 0; i < samsung->lanes; i++)
> +		regmap_update_bits(samsung->regmap, rx_data_lane_gnr_con0[i],
> +				   PHY_ENABLE, PHY_ENABLE);
> +
> +	ret = regmap_read_poll_timeout(samsung->regmap, DPHY_SC_GNR_CON0,
> +				       sts, sts & PHY_READY, 200, 4000);
> +	if (ret) {
> +		dev_err(samsung->dev, "RX clock lane not ready\n");
> +		return ret;
> +	}

[Severity: High]
If regmap_read_poll_timeout() fails here, do we leak the previously set
PHY_ENABLE bits?

Returning early leaves the PHY_ENABLE bits set in the PHY registers.
The caller catches the error and propagates it, meaning the framework
will not call power_off() to clean up. On subsequent retries, writing
PHY_ENABLE to a register where it is already 1 will fail to produce the
0 -> 1 edge transition required by the PHY state machine to start, breaking
future attempts to initialize the RX lanes.

[ ... ]

> +static int samsung_mipi_dphy_rx_power_on(struct samsung_mipi_dcphy *samsung,
> +					 struct phy *peer)
> +{
> +	bool first = peer->power_count == 0;

[Severity: High]
This is a pre-existing issue, but is it safe to rely on peer->power_count
here to determine if shared initialization is needed?

The PHY core framework increments phy->power_count outside the driver's lock,
only after the power_on() callback returns. If Thread A (RX) and Thread B
(TX) call phy_power_on() concurrently:

Thread A configures RX, drops samsung->lock, but is preempted before
rx_phy->power_count is incremented.
Thread B acquires samsung->lock, sees rx_phy->power_count is still 0, and
executes its 'first' initialization sequence.

For TX, this involves explicitly asserting the shared APB bus reset
(apb_rst), which would wipe out the entire RX configuration Thread A just
applied. Should the driver track the initialization state internally instead
of relying on power_count?

> +	int ret;
> +
> +	if (!samsung->hs_clk_rate)
> +		return -EINVAL;
> +
> +	reset_control_assert(samsung->s_phy_rst);
> +
> +	if (first)
> +		samsung_mipi_dcphy_bias_block_enable(samsung);

[Severity: High]
If the PHY is used in a receive-first or receive-only role, does this sequence
omit the apb_rst hardware initialization for the shared APB bus?

Looking at the TX power-on sequence, it toggles apb_rst when first is true to
ensure the shared APB bus interface is properly initialized. If RX powers on
first, the apb_rst toggle is skipped here, and a subsequent TX power-on will
see first == false and also skip it. Can this leave the shared APB bus
interface uninitialized and potentially cause system hangs during regmap
accesses?

> +	samsung_mipi_dphy_rx_config_common(samsung);
> +
> +	ret = samsung_mipi_dphy_rx_config_settle(samsung);
> +	if (ret)
> +		goto out_deassert;
> +
> +	ret = samsung_mipi_dphy_rx_lane_enable(samsung);
> +
> +out_deassert:
> +	reset_control_deassert(samsung->s_phy_rst);
> +
> +	return ret;
> +}

[Severity: High]
Does this unconditionally commit the leaked PHY_ENABLE state to the hardware?

If samsung_mipi_dphy_rx_lane_enable() fails due to the timeout mentioned
earlier, it drops through to here and deasserts s_phy_rst. Combined with the
missing PHY_ENABLE cleanup, this commits the broken hardware state.

[ ... ]

> @@ -1525,13 +1831,17 @@ static int samsung_mipi_dcphy_configure(struct phy *phy,
>  	struct samsung_mipi_dcphy *samsung = pd->parent;
>  	unsigned long long target_rate = opts->mipi_dphy.hs_clk_rate;
>  
> +	mutex_lock(&samsung->lock);
>  	samsung->lanes = opts->mipi_dphy.lanes > 4 ? 4 : opts->mipi_dphy.lanes;

[Severity: High]
Does sharing the samsung->lanes variable between TX and RX lead to state
corruption?

Since the driver models TX and RX as two independent PHY instances, either
instance will blindly overwrite samsung->lanes here. If a consumer configures
TX with 4 lanes and a second consumer configures RX with 2 lanes,
samsung->lanes becomes 2. A subsequent TX power_off() will read this state
and only disable 2 lanes, leaking the other 2 active TX lanes. Should lanes
be tracked per-direction?

> -	if (pd->dir == RK_DCPHY_DIR_RX)
> -		return -EOPNOTSUPP;
> -
> -	samsung_mipi_dcphy_pll_calc_rate(samsung, target_rate);
> -	opts->mipi_dphy.hs_clk_rate = samsung->pll.rate;
> +	if (pd->dir == RK_DCPHY_DIR_RX) {
> +		/* The sensor supplies the link clock; the PLL stays off. */
> +		samsung->hs_clk_rate = target_rate;
> +	} else {
> +		samsung_mipi_dcphy_pll_calc_rate(samsung, target_rate);
> +		opts->mipi_dphy.hs_clk_rate = samsung->pll.rate;
> +	}
> +	mutex_unlock(&samsung->lock);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260721-dcphy-rx-v1-v1-0-4fc83c0ccac9@gmail.com?part=4

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