Re: [PATCH v4 2/3] drm/bridge: analogix_dp: Add validation for samsung,lane-count property

[Damon Ding]

Hi Luca,

On 5/30/2026 9:38 PM, Luca Ceresoli wrote:
> On Sat May 30, 2026 at 3:33 PM CEST, Luca Ceresoli wrote:
> > On Fri, 29 May 2026 12:05:29 +0800, Damon Ding <damon.ding@xxxxxxxxxxxxxx> wrote:
> >
> > Hello Damon,
> >
> > > diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> > > index 8cf6b73bceac..699a7f380c56 100644
> > > --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> > > +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> > > @@ -1260,8 +1261,16 @@ static int analogix_dp_dt_parse_pdata(struct analogix_dp_device *dp)
> > >   		 */
> > >   		of_property_read_u32(dp_node, "samsung,link-rate",
> > >   				     &video_info->max_link_rate);
> > > -		of_property_read_u32(dp_node, "samsung,lane-count",
> > > -				     &video_info->max_lane_count);
> > > +		ret = of_property_read_u32(dp_node, "samsung,lane-count",
> > > +					   &video_info->max_lane_count);
> > > +		if (!ret) {
> > > +			if (video_info->max_lane_count == 0 ||
> > > +			    video_info->max_lane_count > LANE_COUNT4) {
> >
> > This sashiko report seems to me valid.
>
> Meh, messed up with 'b4 review' :-/ Apologies
>
> "This sashiko report" [0] was about an enum being signed, so '== 0' could miss
> negative numbers coming from bogus DT values higher than 1^31.

Ah, I agree. It would be better to add a new inline function to validate 
that the lane count passed from DT is exactly 1, 2, or 4, just as 
Sashiko suggested.

> > But I'n no DP expert, I have no idea whether this ther one is valid.
>
> And this was about "Additionally, does this check inadvertently allow 3,
> which is an invalid DisplayPort lane count?"

Yes, the DisplayPort specification only allows lane counts of 1, 2, or 
4. I did miss the check for the invalid value 3 in the current code.

(BTW: I did not find a common helper function in drm_dp_helper.h to 
validate valid DP lane counts (1, 2, 4). I'm not sure if it would be 
better to add a generic lane count validation function to the DP helper 
library instead, to avoid duplicating the same functionality across 
individual DP drivers. Perhaps other DP experts could provide some 
advice on this.)

> > > +				dev_err(dp->dev, "samsung,lane-count = %d is out of range\n",
> > > +					video_info->max_lane_count);
> > > +				return -EINVAL;
> > > +			}
> > > +		}
> >
> > As reported by sashiko, 'count == 0' should be 'count <= 0', being an enum.
> >
> > Additionally I'd avoid the nested if, and I think using dev_err_probe() is
> > correct here (we are only called by probe functions), so it all could
> > become:
> >
> >     if (ret || count <= 0 || count > LANE_COUNT0)
> >          return dev_err_probe(...);
> >
> > There are other sashiko reports to patch 3, and at least one seems valid to
> > me. Can you either fix them in the next iteration or elaborate on why the
> > code is correct there?

Yes, I will add a helper function to validate the valid lane counts (1, 
2, 4) and use dev_err_probe() instead in next version.

> [0] https://sashiko.dev/#/patchset/20260529040530.741336-1-damon.ding%40rock-chips.com

Best regards,
Damon