Re: [PATCH] drm/msm/dp: Avoid division by zero in msm_dp_ctrl_config_msa()

[Nathan Chancellor <nathan@kernel.org>]

On Mon, Jan 12, 2026 at 04:37:46AM +0200, Dmitry Baryshkov wrote:
> On Fri, Jan 09, 2026 at 10:06:29AM +0100, Konrad Dybcio wrote:
> > Dmitry, would it be beneficial to throw an actual error when the rate is
> > is mangled? i.e.
> > 
> > diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.c b/drivers/gpu/drm/msm/dp/dp_ctrl.c
> > index aa2303d0e148..4f710b8e6bc6 100644
> > --- a/drivers/gpu/drm/msm/dp/dp_ctrl.c
> > +++ b/drivers/gpu/drm/msm/dp/dp_ctrl.c
> > @@ -2404,9 +2404,9 @@ static int msm_dp_ctrl_link_retrain(struct msm_dp_ctrl_private *ctrl)
> >         return msm_dp_ctrl_setup_main_link(ctrl, &training_step);
> >  }
> >  
> > -static void msm_dp_ctrl_config_msa(struct msm_dp_ctrl_private *ctrl,
> > -                              u32 rate, u32 stream_rate_khz,
> > -                              bool is_ycbcr_420)
> > +static int msm_dp_ctrl_config_msa(struct msm_dp_ctrl_private *ctrl,
> > +                                 u32 rate, u32 stream_rate_khz,
> > +                                 bool is_ycbcr_420)
> >  {
> >         u32 pixel_m, pixel_n;
> >         u32 mvid, nvid, pixel_div = 0, dispcc_input_rate;
> > @@ -2415,14 +2415,21 @@ static void msm_dp_ctrl_config_msa(struct msm_dp_ctrl_private *ctrl,
> >         u32 const link_rate_hbr3 = 810000;
> >         unsigned long den, num;
> >  
> > -       if (rate == link_rate_hbr3)
> > +       switch (rate) {
> > +       case link_rate_hbr3:
> >                 pixel_div = 6;
> > -       else if (rate == 162000 || rate == 270000)
> > -               pixel_div = 2;
> > -       else if (rate == link_rate_hbr2)
> > +               break;
> > +       case link_rate_hbr2:
> >                 pixel_div = 4;
> > -       else
> > +               break;
> > +       case 270000:
> > +       case 162000:
> > +               pixel_div = 2;
> > +               break;
> > +       default:
> >                 DRM_ERROR("Invalid pixel mux divider\n");
> > +               return -EINVAL;
> 
> Well... In the ideal world, we can't end up here, PHY's
> configure_dp_clocks (or qcom_edp_set_vco_div()) will fail if the link
> rate is is invalid here. I think, we should return an error here, but
> there is no need to propagate it further.
> 
> See the discussion at https://lore.kernel.org/dri-devel/f2ce6ae51c50b0d2e57b905c63b43413@codeaurora.org/

I interpret that as approving of the above hunk but omitting the hunk
that modifies msm_dp_ctrl_on_stream(). In that case, what is the point
of changing the return type of msm_dp_ctrl_config_msa()? Wouldn't the
below diff have the same exact effect as a smaller change? I don't mind
rolling this up as a v2.

Cheers,
Nathan

diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.c b/drivers/gpu/drm/msm/dp/dp_ctrl.c
index aa2303d0e148..d8ea73b89f7c 100644
--- a/drivers/gpu/drm/msm/dp/dp_ctrl.c
+++ b/drivers/gpu/drm/msm/dp/dp_ctrl.c
@@ -2409,20 +2409,27 @@ static void msm_dp_ctrl_config_msa(struct msm_dp_ctrl_private *ctrl,
 			       bool is_ycbcr_420)
 {
 	u32 pixel_m, pixel_n;
-	u32 mvid, nvid, pixel_div = 0, dispcc_input_rate;
+	u32 mvid, nvid, pixel_div, dispcc_input_rate;
 	u32 const nvid_fixed = DP_LINK_CONSTANT_N_VALUE;
 	u32 const link_rate_hbr2 = 540000;
 	u32 const link_rate_hbr3 = 810000;
 	unsigned long den, num;
 
-	if (rate == link_rate_hbr3)
+	switch (rate) {
+	case link_rate_hbr3:
 		pixel_div = 6;
-	else if (rate == 162000 || rate == 270000)
-		pixel_div = 2;
-	else if (rate == link_rate_hbr2)
+		break;
+	case link_rate_hbr2:
 		pixel_div = 4;
-	else
+		break;
+	case 162000:
+	case 270000:
+		pixel_div = 2;
+		break;
+	default:
 		DRM_ERROR("Invalid pixel mux divider\n");
+		return;
+	}
 
 	dispcc_input_rate = (rate * 10) / pixel_div;