Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [High] Unconditional do-while loop in `mtk_dp_phy_disable_all_lanes` causes an out-of-bounds bitwise shift and undefined behavior when no lanes are initially enabled. - [High] `mtk_dp_phy_disable_all_lanes` passes a bit index instead of a bitmask to `FIELD_PREP`, failing to disable the correct lanes. --
commit 25cdd51bd1ac4d69a49e77d93a6b1374ba46dd3c Author: AngeloGioacchino Del Regno <[email protected]> phy: phy-mtk-dp: Support set_lanes in configure and properly cleanup Add support for enabling a specifically requested number of lanes in the .configure() callback and disable all lanes in power off and reset callbacks for proper hardware cleanup. > diff --git a/drivers/phy/mediatek/phy-mtk-dp.c > b/drivers/phy/mediatek/phy-mtk-dp.c > index 586e727956332..a2cd22b9da066 100644 > --- a/drivers/phy/mediatek/phy-mtk-dp.c > +++ b/drivers/phy/mediatek/phy-mtk-dp.c [ ... ] > @@ -261,6 +267,35 @@ static int mtk_dp_phy_power_on(struct phy *phy) > return 0; > } > > +static int mtk_dp_phy_disable_all_lanes(struct mtk_dp_phy *dp_phy) > +{ > + const struct mtk_dp_phy_pdata *pdata = dp_phy->pdata; > + const u8 *regs = pdata->regs_dig_glb; > + int ret; > + u32 val; > + > + ret = regmap_read(dp_phy->regmap, pdata->off_dig_glb + > regs[DP_PHYD_TX_CTL_0], &val); > + if (ret) > + return ret; > + > + /* Get mask of currently enabled lane */ > + val = FIELD_GET(PHYD_TX_LN_EN, val); > + > + /* Disable all lanes (needs to be done one by one, from last to first) > */ > + do { > + u32 lane_num = fls(val) - 1; > + val &= ~BIT(lane_num); ^^^^^^^^^^^^^^^^^^^^ [Severity: High] Does this cause undefined behavior if val is initially 0? If the PHY register reports 0 enabled lanes (which can happen after a hardware reset or during initial boot), val will be 0. Since this is an unconditional do-while loop, fls(0) will return 0, leading to lane_num evaluating to UINT_MAX. The subsequent BIT(lane_num) attempt would result in an out-of-bounds bitwise shift. Could this be changed to a while loop, or check if val is 0 before entering the loop? > + > + ret = regmap_clear_bits(dp_phy->regmap, > + pdata->off_dig_glb + > regs[DP_PHYD_TX_CTL_0], > + FIELD_PREP(PHYD_TX_LN_EN, lane_num)); ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ [Severity: High] Does this clear the correct lanes in the register? It looks like the bit index lane_num is passed directly to FIELD_PREP here instead of the bitmask BIT(lane_num), which was correctly used in mtk_dp_phy_configure(). If lane_num is 3, FIELD_PREP(GENMASK(7, 4), 3) will generate 0x30, which clears lanes 0 and 1 instead of 0x80 (which would clear lane 3). This seems like it might repeatedly attempt to clear the wrong lanes and leave higher lanes enabled. > + if (ret) > + return ret; > + } while (val); > + > + return 0; > +} -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=6
