Re: [PATCH v1 3/3] drm/bridge: analogix_dp: Add support for optional data-lanes mapping

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Stack buffer overflow when parsing 'data-lanes' property on EXYNOS platforms.
- [High] Incorrect hardware configuration for asymmetric lane mappings due to inverted logical-to-physical mapping.
- [Medium] Inconsistent endpoint node resolution leads to device tree parsing errors.
--

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

drm/bridge: analogix_dp: Add support for optional data-lanes mapping

This commit parses the optional 'data-lanes' device tree property to
support custom physical lane mapping configuration.

> diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> index 8cf6b73bceac0..d53ab25bc57b1 100644
> --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> @@ -1234,6 +1234,59 @@ static const struct drm_bridge_funcs analogix_dp_bridge_funcs = {
>  	.detect = analogix_dp_bridge_detect,
>  };
>  
> +static int analogix_dp_dt_parse_lanes_map(struct analogix_dp_device *dp)
> +{
> +	struct video_info *video_info = &dp->video_info;
> +	struct device_node *endpoint;
> +	u32 tmp[LANE_COUNT4];

[ ... ]

> +	num_lanes = drm_of_get_data_lanes_count_ep(dp->dev->of_node, 1, 0, 1,
> +						   video_info->max_lane_count);
> +	if (num_lanes < 0)
> +		return -EINVAL;
> +
> +	endpoint = of_graph_get_endpoint_by_regs(dp->dev->of_node, 1, -1);

Can this inconsistent endpoint node resolution lead to parsing errors?

The drm_of_get_data_lanes_count_ep() call strictly searches for an endpoint
with reg = 0 on port 1, but the subsequent of_graph_get_endpoint_by_regs()
call uses reg = -1 to match the first available endpoint regardless of its
register value.

If a valid endpoint has a non-zero register, won't the first query fail while
the second would have correctly found it?

> +	if (!endpoint)
> +		return -EINVAL;
> +
> +	ret = of_property_read_u32_array(endpoint, "data-lanes", tmp, num_lanes);

Does this introduce a potential stack buffer overflow for the tmp array on
EXYNOS platforms?

The tmp array is allocated on the stack with a fixed size of LANE_COUNT4 (4),
but num_lanes is bounded by video_info->max_lane_count. Looking at the EXYNOS
platform parsing later in the same file:

	case EXYNOS_DP:
		...
		of_property_read_u32(dp_node, "samsung,lane-count",
				     &video_info->max_lane_count);

The max_lane_count is populated directly from the samsung,lane-count device
tree property without validation. If a malformed device tree specifies a value
greater than 4, num_lanes will exceed the size of the tmp array, causing
of_property_read_u32_array() to write out of bounds and corrupt the stack.

[ ... ]

> diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c b/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c
> index 38fd8d5014d29..45c7652645a13 100644
> --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c
> +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c
> @@ -48,16 +48,15 @@ void analogix_dp_stop_video(struct analogix_dp_device *dp)
>  	writel(reg, dp->reg_base + ANALOGIX_DP_VIDEO_CTL_1);
>  }
>  
> -void analogix_dp_lane_swap(struct analogix_dp_device *dp, bool enable)
> +void analogix_dp_lane_mapping(struct analogix_dp_device *dp)
>  {
> +	u32 *lane_map = dp->video_info.lane_map;
>  	u32 reg;
>  
> -	if (enable)
> -		reg = LANE3_MAP_LOGIC_LANE_0 | LANE2_MAP_LOGIC_LANE_1 |
> -		      LANE1_MAP_LOGIC_LANE_2 | LANE0_MAP_LOGIC_LANE_3;
> -	else
> -		reg = LANE3_MAP_LOGIC_LANE_3 | LANE2_MAP_LOGIC_LANE_2 |
> -		      LANE1_MAP_LOGIC_LANE_1 | LANE0_MAP_LOGIC_LANE_0;
> +	reg = lane_map[0] << LANE0_MAP_SHIFT;
> +	reg |= lane_map[1] << LANE1_MAP_SHIFT;
> +	reg |= lane_map[2] << LANE2_MAP_SHIFT;
> +	reg |= lane_map[3] << LANE3_MAP_SHIFT;

Is the logical-to-physical lane mapping inverted here?

The data-lanes device tree property semantics dictate that the array index is
the logical lane and the array value is the physical lane. However, the
ANALOGIX_DP_LANE_MAP register fields appear to configure which logical lane is
assigned to a specific physical lane.

By assigning reg = lane_map[0] << LANE0_MAP_SHIFT, does this code write the
physical lane assigned to logical lane 0 into the hardware configuration for
physical lane 0? While this works coincidentally for symmetric permutations
like <0 1 2 3> or <3 2 1 0>, it would result in incorrect lane assignments for
asymmetric mappings like <1 2 3 0>.

>  
>  	writel(reg, dp->reg_base + ANALOGIX_DP_LANE_MAP);
>  }

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