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

> The RX power-on sequence fails to toggle the `apb_rst` reset line,

This is intentional rather than an omission. The vendor BSP toggles
apb_rst only on its TX power-on path and never in its RX bring-up, and
this driver mirrors that split: the APB reset is issued only when the
transmitter powers on first. The TRM start-up sequence (figure 22-8)
does not require it for the receiver, and a CSI-only configuration -
where the receiver is the only direction ever powered - captures
frames on real hardware without the APB reset ever being issued.

> If `samsung_mipi_dphy_rx_lane_enable()` fails [...] leaves the
> `PHY_ENABLE` bits set

Valid - in v2 the already enabled lanes are switched back off before
the error is returned.

> The `lanes` configuration field is shared between TX and RX,

Also valid, and it reproduces on hardware (see my reply on patch 3).
In v2 the lane count becomes per-direction state.

> time-of-check to time-of-use (TOCTOU) vulnerability on the
> unprotected `peer->power_count`.

Agreed - in v2 each direction records its powered state under the
provider mutex instead of reading the peer's power_count, which closes
that window.

Happy to revisit any of these if an assessment looks wrong.

Thanks,
Jason


<[email protected]> 於 2026年7月21日週二 下午6:57寫道：
>
> 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 <[email protected]>
> 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/[email protected]?part=4