Re: [PATCH 3/4] phy: rockchip-samsung-dcphy: model TX and RX as separate PHYs

> Does this code modify the shared samsung->lanes state before checking if
> the direction is supported?

Yes, this is a valid finding - and the shared lane count turned out to
be a real problem beyond the ordering. It reproduces on hardware: with
the receiver configured for one data lane, powering the transmitter
off afterwards disables only one of its two lanes and leaves the
second still enabled, which the per-lane GNR_CON0 registers confirm.

In v2 both the lane count and the receiver link rate become
per-direction state, so a configure() call can no longer clobber the
other direction, and the peer power_count check is replaced by a
powered flag kept under the provider mutex. With those changes the
same sequence disables both lanes cleanly on the same hardware.

Thanks,
Jason

<[email protected]> 於 2026年7月21日週二 下午6:44寫道：
>
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [High] Concurrent or unsupported PHY configuration clobbers the shared `samsung->lanes` driver state before verifying the direction is supported.
> --
>
> --- Patch [3]: [PATCH 3/4] phy: rockchip-samsung-dcphy: model TX and RX as separate PHYs ---
> commit 242b9435d09e0133de4658156f3cc224aa2f84c9
> Author: Jason Yang <[email protected]>
>
> phy: rockchip-samsung-dcphy: model TX and RX as separate PHYs
>
> This commit models the MIPI DSI transmitter and MIPI CSI receiver as separate
> PHYs that share the same hardware block. It registers one struct phy per
> direction and adds a per-provider mutex to serialize access to the shared
> state.
>
> > 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
>
> [ ... ]
>
> > @@ -1488,11 +1521,15 @@ samsung_mipi_dcphy_pll_calc_rate(struct samsung_mipi_dcphy *samsung,
> >  static int samsung_mipi_dcphy_configure(struct phy *phy,
> >                                       union phy_configure_opts *opts)
> >  {
> > -     struct samsung_mipi_dcphy *samsung = phy_get_drvdata(phy);
> > +     struct samsung_mipi_dcphy_dir *pd = phy_get_drvdata(phy);
> > +     struct samsung_mipi_dcphy *samsung = pd->parent;
> >       unsigned long long target_rate = opts->mipi_dphy.hs_clk_rate;
> >
> >       samsung->lanes = opts->mipi_dphy.lanes > 4 ? 4 : opts->mipi_dphy.lanes;
> >
> > +     if (pd->dir == RK_DCPHY_DIR_RX)
> > +             return -EOPNOTSUPP;
> > +
>
> [Severity: High]
> Does this code modify the shared samsung->lanes state before checking if
> the direction is supported?
>
> Since this is done without holding samsung->lock, can a consumer
> configuring the RX PHY clobber the active TX PHY's lane count?
>
> If the lane count is corrupted, it looks like it could lead to incorrect lane
> power-down sequences when the TX PHY is disabled later, potentially leaving
> MIPI lanes actively driven.
>
> Should the pd->dir check and the lock acquisition happen before updating
> samsung->lanes?
>
> [ ... ]
>
> --
> Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=3