Re: [PATCH v2 4/6] phy: rockchip: Add support for Naneng eDP phy

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] phy: rockchip: missing set_lanes handling in rockchip_edp_phy_configure()
- [Medium] phy: rockchip: uninitialized val read in rockchip_edp_phy_enabled()
--

commit d07a3a0e3533b84cdc67b6de45db340e9d9898c9
Author: Damon Ding <damon.ding@rock-chips.com>

phy: rockchip: Add support for Naneng eDP phy

This commit adds a driver for the Rockchip eDP transmitter PHY with the
NaNeng IP block found on the RK3568 SoC, controlled via the eDP PHY GRF
syscon regmap.

> diff --git a/drivers/phy/rockchip/phy-rockchip-naneng-edp.c b/drivers/phy/rockchip/phy-rockchip-naneng-edp.c
> new file mode 100644
> index 0000000000000..a407714f639a7
> --- /dev/null
> +++ b/drivers/phy/rockchip/phy-rockchip-naneng-edp.c

[ ... ]

> +static int rockchip_edp_phy_set_voltages(struct rockchip_edp_phy *edpphy,
> +					 struct phy_configure_opts_dp *dp)
> +{
> +	u8 lane;
> +
> +	for (lane = 0; lane < dp->lanes; lane++)
> +		rockchip_edp_phy_set_voltage(edpphy, dp, lane);
> +
> +	return 0;
> +}
> +
> +static int rockchip_edp_phy_set_rate(struct rockchip_edp_phy *edpphy,
> +				     struct phy_configure_opts_dp *dp)
> +{
> +	u32 value;
> +	int ret;

[ ... ]

> +	regmap_write(edpphy->grf, EDP_PHY_GRF_CON0, FIELD_PREP_WM16(EDP_PHY_PD_PLL_MASK, 0));
> +	regmap_write(edpphy->grf, EDP_PHY_GRF_CON0,
> +		     FIELD_PREP_WM16(EDP_PHY_TX_PD_MASK, ~GENMASK(dp->lanes - 1, 0)));
> +	ret = regmap_read_poll_timeout(edpphy->grf, EDP_PHY_GRF_STATUS0,
> +				       value, value & PLL_RDY, 100, 1000);
> +	if (ret) {
> +		dev_err(edpphy->dev, "Failed to wait for PHY PLL ready: %d\n", ret);
> +		return ret;
> +	}
> +
> +	regmap_write(edpphy->grf, EDP_PHY_GRF_CON5, FIELD_PREP_WM16(EDP_PHY_TX_MODE_MASK, 0x0));
> +	regmap_write(edpphy->grf, EDP_PHY_GRF_CON0,
> +		     FIELD_PREP_WM16(EDP_PHY_TX_IDLE_MASK, ~GENMASK(dp->lanes - 1, 0)));
> +
> +	return 0;
> +}
> +
> +static int rockchip_edp_phy_verify_config(struct rockchip_edp_phy *edpphy,
> +					  struct phy_configure_opts_dp *dp)
> +{
> +	int i;
> +
> +	/* If changing link rate was required, verify it's supported. */
> +	if (dp->set_rate) {
> +		switch (dp->link_rate) {
> +		case 1620:
> +		case 2700:
> +			/* valid bit rate */
> +			break;
> +		default:
> +			return -EINVAL;
> +		}
> +	}
> +
> +	if (dp->set_lanes) {
> +		/* Verify lane count. */
> +		switch (dp->lanes) {
> +		case 1:
> +		case 2:
> +		case 4:
> +			/* valid lane count. */
> +			break;
> +		default:
> +			return -EINVAL;
> +		}
> +	}
> +
> +	/*
> +	 * If changing voltages is required, check swing and pre-emphasis
> +	 * levels, per-lane.
> +	 */
> +	if (dp->set_voltages) {
> +		/* Lane count verified previously. */
> +		for (i = 0; i < dp->lanes; i++) {
> +			if (dp->voltage[i] > 3 || dp->pre[i] > 3)
> +				return -EINVAL;
> +
> +			/*
> +			 * Sum of voltage swing and pre-emphasis levels cannot
> +			 * exceed 3.
> +			 */
> +			if (dp->voltage[i] + dp->pre[i] > 3)
> +				return -EINVAL;
> +		}
> +	}
> +
> +	return 0;
> +}
> +
> +static int rockchip_edp_phy_configure(struct phy *phy, union phy_configure_opts *opts)
> +{
> +	struct rockchip_edp_phy *edpphy = phy_get_drvdata(phy);
> +	int ret;
> +
> +	ret = rockchip_edp_phy_verify_config(edpphy, &opts->dp);
> +	if (ret) {
> +		dev_err(edpphy->dev, "Invalid params for phy configure\n");
> +		return ret;
> +	}
> +
> +	if (opts->dp.set_rate) {
> +		ret = rockchip_edp_phy_set_rate(edpphy, &opts->dp);
> +		if (ret) {
> +			dev_err(edpphy->dev, "Failed to set rate\n");
> +			return ret;
> +		}
> +	}
> +
> +	if (opts->dp.set_voltages) {
> +		ret = rockchip_edp_phy_set_voltages(edpphy, &opts->dp);
> +		if (ret) {
> +			dev_err(edpphy->dev, "Failed to set voltages\n");
> +			return ret;
> +		}
> +	}
> +
> +	return 0;
> +}

[Severity: High]
How does rockchip_edp_phy_configure() handle callers that issue separate
phy_configure() calls for rate, lane count, and voltage levels?

Looking at drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c, the Analogix DP
bridge issues three separate phy_configure() calls, each with a
zero-initialized union phy_configure_opts:

1. analogix_dp_set_link_bandwidth() sets only phy_cfg.dp.link_rate and
phy_cfg.dp.set_rate = true, leaving phy_cfg.dp.lanes = 0:

    union phy_configure_opts phy_cfg = {0};

    phy_cfg.dp.link_rate =
        drm_dp_bw_code_to_link_rate(dp->link_train.link_rate) / 100;
    phy_cfg.dp.set_rate = true;
    ret = phy_configure(dp->phy, &phy_cfg);

When rockchip_edp_phy_set_rate() runs, dp->lanes is 0 (and unvalidated since
dp->set_lanes is false), so GENMASK(dp->lanes - 1, 0) evaluates
GENMASK(UINT_MAX, 0) when programming EDP_PHY_TX_PD_MASK and
EDP_PHY_TX_IDLE_MASK.

2. analogix_dp_set_lane_count() sets phy_cfg.dp.lanes and
phy_cfg.dp.set_lanes = true, but rockchip_edp_phy_configure() never checks
opts->dp.set_lanes, so the lane-dependent TX configuration is not applied and
the active lane count is not stored in struct rockchip_edp_phy.

3. analogix_dp_set_lane_link_training() sets phy_cfg.dp.voltage[],
phy_cfg.dp.pre[], and phy_cfg.dp.set_voltages = true, with phy_cfg.dp.lanes
left at 0. Both rockchip_edp_phy_verify_config() and
rockchip_edp_phy_set_voltages() loop up to dp->lanes, so 0 iterations run and
no voltage swing or pre-emphasis registers are written (or if a caller passes
set_voltages = true with set_lanes = false and an unvalidated dp->lanes > 4,
the loops read past the end of dp->voltage[4] and dp->pre[4]).

Should the lane-dependent TX configuration be applied in a set_lanes handler
that also stores the lane count in struct rockchip_edp_phy for set_voltages
to use?

> +static bool rockchip_edp_phy_enabled(struct rockchip_edp_phy *edpphy)
> +{
> +	u32 val;
> +
> +	regmap_read(edpphy->grf, EDP_PHY_GRF_STATUS0, &val);
> +
> +	return FIELD_GET(PLL_RDY, val);
> +}

[Severity: Medium]
What happens in rockchip_edp_phy_enabled() if regmap_read() fails?

Because val is uninitialized and the return value of regmap_read() is not
checked, FIELD_GET(PLL_RDY, val) will read an uninitialized stack variable,
which can cause rockchip_edp_phy_power_on() to return 0 early without
powering on or configuring the PHY AUX channel.

Should val be initialized to 0 (or the regmap_read() error checked)?

[ ... ]

> -- 
> 2.34.1

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261019102824.3157044-1-damon.ding@rock-chips.com?part=4