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

[Nathan Chancellor]

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/f2ce6ae51c50b0d2e57b905c63b43...@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;