Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider: - [High] The patch introduces a forbidden sleeping function (`msleep`) inside atomic commit paths. - [High] `mtk_dp_bridge_atomic_disable()` incorrectly forces the encoder bias power ON rather than OFF, likely due to a copy-paste error. - [High] Conflicting register programming in `mtk_dp_encoder_reset` overwrites PHY enable bits and potentially leaves the PHY in permanent reset. - [High] Double unregister of the PHY platform device (`mtk_dp->phy_dev`) on the error path. - [High] `mtk_dp_setup_encoder` constructs an update mask using raw bit values instead of proper field masks, failing to clear bits in multi-bit fields. --
--- Patch [10]: [PATCH v2 10/11] drm/mediatek: mtk_dp: Add support for eDP1.5 IPs and MT8196 SoC --- commit a9e93bd64557c3ad0ae0387b6e0c8b87e41baeda Author: AngeloGioacchino Del Regno <[email protected]> Subject: drm/mediatek: mtk_dp: Add support for eDP1.5 IPs and MT8196 SoC This patch adds support for MediaTek's Embedded DisplayPort (eDP) v1.5 IP found in the MT8196 SoC and other variants. It introduces conditional hardware initialization sequences, platform data switches for newer features, and includes partial support paths for the DisplayPort 1.4 IP on the same SoC. > diff --git a/drivers/gpu/drm/mediatek/mtk_dp.c > b/drivers/gpu/drm/mediatek/mtk_dp.c > index fb216811c80c7..868d918a0b161 100644 > --- a/drivers/gpu/drm/mediatek/mtk_dp.c > +++ b/drivers/gpu/drm/mediatek/mtk_dp.c [ ... ] > @@ -648,7 +669,18 @@ static void mtk_dp_setup_encoder(struct mtk_dp *mtk_dp) > mtk_dp_update_bits(mtk_dp, MTK_DP_ENC1_P0_3364, > FIFO_READ_START_POINT_DP_ENC1_P0_VAL << 12, > FIFO_READ_START_POINT_DP_ENC1_P0_MASK); > - mtk_dp_write(mtk_dp, MTK_DP_ENC1_P0_3368, DP_ENC1_P0_3368_VAL); > + > + mask = VIDEO_SRAM_FIFO_CNT_RESET_SEL_DP_ENC1_P0 | > VIDEO_STABLE_CNT_THRD_DP_ENC1_P0; > + mask |= BS2BS_MODE_DP_ENC1_P0; > + > + val = mask; > + > + mask |= SDP_DP13_EN_DP_ENC1_P0; > + > + if (mtk_dp->data->sdp_encoder1_enable) > + val |= SDP_DP13_EN_DP_ENC1_P0; > + > + mtk_dp_update_bits(mtk_dp, MTK_DP_ENC1_P0_3368, val, mask); [Severity: High] Does this code construct the update mask using single-bit values rather than multi-bit field masks? The previous code wrote directly to the register, but the update here seems to use bit values (like BS2BS_MODE_DP_ENC1_P0) as the mask itself. Since BS2BS_MODE_DP_ENC1_P0 is just one bit but the field might be larger, will this leave uncleared bits in the multi-bit fields like VIDEO_SRAM_FIFO_CNT_RESET_SEL_MASK and BS2BS_MODE_DP_ENC1_P0_MASK? [ ... ] > @@ -1116,6 +1148,72 @@ static void mtk_dp_hwirq_enable(struct mtk_dp *mtk_dp, > bool enable) > IRQ_MASK_DP_TRANS_P0_MASK); > } > > +static void mtk_dp_encoder_force_bias_power_on(struct mtk_dp *mtk_dp) > +{ > + /* Force MCU to enable BIAS */ > + mtk_dp_update_bits(mtk_dp, REG_3F44_DP_ENC_P0_3, > + ENC_PHY_PWR_OVERWRITE_EN, ENC_PHY_PWR_OVERWRITE_EN); > + mtk_dp_update_bits(mtk_dp, REG_3F44_DP_ENC_P0_3, > + FIELD_PREP_CONST(ENC_PHY_PWR_STATE_OVERWRITE_MASK, > + ENC_PHY_BIAS_PWR_ON), > + ENC_PHY_PWR_STATE_OVERWRITE_MASK); > + > + /* Wait for the MCU to get LDO ready in PHYD */ > + msleep(50); [Severity: High] Can this sleep in an atomic context? This function is called by mtk_dp_set_lanes() during display enabling, and by mtk_dp_bridge_atomic_disable(), both of which are executed in atomic commit paths where sleeping functions are forbidden. Could this msleep(50) cause kernel warnings or deadlocks during non-blocking commits? > + > + mtk_dp_update_bits(mtk_dp, REG_3F44_DP_ENC_P0_3, > + 0, ENC_PHY_PWR_OVERWRITE_EN); > +} [ ... ] > +static void mtk_dp_encoder_reset(struct mtk_dp *mtk_dp) > +{ > /* dp tx encoder reset all sw */ > mtk_dp_update_bits(mtk_dp, MTK_DP_ENC0_P0_3004, > DP_TX_ENCODER_4P_RESET_SW_DP_ENC0_P0, > DP_TX_ENCODER_4P_RESET_SW_DP_ENC0_P0); > > + if (mtk_dp->data->hwip_v1p4) { > + mtk_dp_update_bits(mtk_dp, MTK_DP_ENC0_P0_3004, 0, > + DP_TX_ENCODER_4P_RESET_SW_DP_ENC0_P0); > + mtk_dp_update_bits(mtk_dp, REG_3FF8_DP_ENC_P0_3, > + ENC_PHY_STATE_RESET_ALL_MASK, > + ENC_PHY_STATE_RESET_ALL_MASK); [Severity: High] Does this write to ENC_PHY_STATE_RESET_ALL_MASK permanently hold the PHY in reset and overwrite unrelated fields? Looking at the definition of ENC_PHY_STATE_RESET_ALL_MASK, it is GENMASK(7, 0). Writing 1s to this entire mask overwrites the enable bits DP_ENCODER_DVO_ON_W_1 and PHY_STATE_W_1_DP_ENC_P0_3 (bits 5 and 6) that were just configured. Since the reset is never cleared, will the PHY remain stuck in this reset state? > + } [ ... ] > @@ -2580,6 +2795,9 @@ static void mtk_dp_bridge_atomic_disable(struct > drm_bridge *bridge, > /* SDP path reset sw*/ > mtk_dp_sdp_path_reset(mtk_dp); > > + if (mtk_dp->data->force_pwr_states) > + mtk_dp_encoder_force_bias_power_on(mtk_dp); > + [Severity: High] Is mtk_dp_encoder_force_bias_power_on() the correct function to call during the disable path? This turns the encoder bias power ON during bridge disable. Given that mtk_dp_power_disable() correctly calls mtk_dp_encoder_force_bias_power_off(), should this be calling the power_off function instead to properly power down the display path? > /* Ensure the sink is muted */ > msleep(20); > } [ ... ] > @@ -2860,7 +3078,8 @@ static int mtk_dp_register_phy(struct mtk_dp *mtk_dp) > "Failed to add phy unregister devm > action"); > > /* PHY calibration data is in mtk_dp only for legacy devicetree */ > - mtk_dp_get_calibration_data(mtk_dp); > + if (mtk_dp->data->efuse_fmt) > + mtk_dp_get_calibration_data(mtk_dp); > > mtk_dp->phy = devm_phy_get(&mtk_dp->phy_dev->dev, "dp"); > if (IS_ERR(mtk_dp->phy)) { [Severity: High] Could the error path for devm_phy_get cause a double free of the PHY device? The patch registers a devm action earlier in this function: ret = devm_add_action_or_reset(dev, mtk_dp_unregister_pdevs, mtk_dp->phy_dev); However, the existing error path for devm_phy_get manually unregisters the device: if (IS_ERR(mtk_dp->phy)) { platform_device_unregister(mtk_dp->phy_dev); return dev_err_probe(...); } Since returning an error will trigger the devres cleanup (and run mtk_dp_unregister_pdevs), will this result in platform_device_unregister() being called a second time on the same pointer? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=10
