[PATCH] drm/i915: rework the error handling in *_dpll_params

[PATCH] drm/i915: rework the error handling in *_dpll_params

[trix]

From: Tom Rix <trix@redhat.com>

Clang static analysis reports this issue
intel_dpll.c:472:31: warning: The left operand of '-'
  is a garbage value [core.UndefinedBinaryOperatorResult]
  this_err = abs(clock.dot - target);
                 ~~~~~~~~~ ^

In a loop clock.dot is set on successful call to
i9xx_calc_dpll_params().  If the call fails, the later
*is_valid() will use the previous loop's clock.dot.

The *_dpll_params functions return an arithmetic statement
with the clock.dot as the variable.  Change the error handler
to set clock.dot to 0 and jump to the return statement.

Fixes: dccbea3b0704 ("drm/i915: calculate the port clock rate along with other PLL params")
Signed-off-by: Tom Rix <trix@redhat.com>
---
 drivers/gpu/drm/i915/display/intel_dpll.c | 32 ++++++++++++++---------
 1 file changed, 20 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_dpll.c b/drivers/gpu/drm/i915/display/intel_dpll.c
index 0ae37fdbf2a5b..ba7cada704288 100644
--- a/drivers/gpu/drm/i915/display/intel_dpll.c
+++ b/drivers/gpu/drm/i915/display/intel_dpll.c
@@ -309,11 +309,13 @@ int pnv_calc_dpll_params(int refclk, struct dpll *clock)
 {
 	clock->m = clock->m2 + 2;
 	clock->p = clock->p1 * clock->p2;
-	if (WARN_ON(clock->n == 0 || clock->p == 0))
-		return 0;
+	if (WARN_ON(clock->n == 0 || clock->p == 0)) {
+		clock->dot = 0;
+		goto end;
+	}
 	clock->vco = DIV_ROUND_CLOSEST(refclk * clock->m, clock->n);
 	clock->dot = DIV_ROUND_CLOSEST(clock->vco, clock->p);
-
+end:
 	return clock->dot;
 }
 
@@ -326,11 +328,13 @@ int i9xx_calc_dpll_params(int refclk, struct dpll *clock)
 {
 	clock->m = i9xx_dpll_compute_m(clock);
 	clock->p = clock->p1 * clock->p2;
-	if (WARN_ON(clock->n + 2 == 0 || clock->p == 0))
-		return 0;
+	if (WARN_ON(clock->n + 2 == 0 || clock->p == 0)) {
+		clock->dot = 0;
+		goto end;
+	}
 	clock->vco = DIV_ROUND_CLOSEST(refclk * clock->m, clock->n + 2);
 	clock->dot = DIV_ROUND_CLOSEST(clock->vco, clock->p);
-
+end:
 	return clock->dot;
 }
 
@@ -338,11 +342,13 @@ int vlv_calc_dpll_params(int refclk, struct dpll *clock)
 {
 	clock->m = clock->m1 * clock->m2;
 	clock->p = clock->p1 * clock->p2;
-	if (WARN_ON(clock->n == 0 || clock->p == 0))
-		return 0;
+	if (WARN_ON(clock->n == 0 || clock->p == 0)) {
+		clock->dot = 0;
+		goto end;
+	}
 	clock->vco = DIV_ROUND_CLOSEST(refclk * clock->m, clock->n);
 	clock->dot = DIV_ROUND_CLOSEST(clock->vco, clock->p);
-
+end:
 	return clock->dot / 5;
 }
 
@@ -350,12 +356,14 @@ int chv_calc_dpll_params(int refclk, struct dpll *clock)
 {
 	clock->m = clock->m1 * clock->m2;
 	clock->p = clock->p1 * clock->p2;
-	if (WARN_ON(clock->n == 0 || clock->p == 0))
-		return 0;
+	if (WARN_ON(clock->n == 0 || clock->p == 0)) {
+		clock->dot = 0;
+		goto end;
+	}
 	clock->vco = DIV_ROUND_CLOSEST_ULL(mul_u32_u32(refclk, clock->m),
 					   clock->n << 22);
 	clock->dot = DIV_ROUND_CLOSEST(clock->vco, clock->p);
-
+end:
 	return clock->dot / 5;
 }
 
-- 
2.26.3

Re: [PATCH] drm/i915: rework the error handling in *_dpll_params

[Ville Syrjälä]

On Fri, Mar 04, 2022 at 01:03:55PM -0800, trix@redhat.com wrote:
> From: Tom Rix <trix@redhat.com>
> 
> Clang static analysis reports this issue
> intel_dpll.c:472:31: warning: The left operand of '-'
>   is a garbage value [core.UndefinedBinaryOperatorResult]
>   this_err = abs(clock.dot - target);
>                  ~~~~~~~~~ ^
> 
> In a loop clock.dot is set on successful call to
> i9xx_calc_dpll_params().  If the call fails, the later
> *is_valid() will use the previous loop's clock.dot.

I don't think this can happen. intel_pll_is_valid() validates
all the dividers first and bails out if they are junk.

> 
> The *_dpll_params functions return an arithmetic statement
> with the clock.dot as the variable.  Change the error handler
> to set clock.dot to 0 and jump to the return statement.
> 
> Fixes: dccbea3b0704 ("drm/i915: calculate the port clock rate along with other PLL params")
> Signed-off-by: Tom Rix <trix@redhat.com>
> ---
>  drivers/gpu/drm/i915/display/intel_dpll.c | 32 ++++++++++++++---------
>  1 file changed, 20 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_dpll.c b/drivers/gpu/drm/i915/display/intel_dpll.c
> index 0ae37fdbf2a5b..ba7cada704288 100644
> --- a/drivers/gpu/drm/i915/display/intel_dpll.c
> +++ b/drivers/gpu/drm/i915/display/intel_dpll.c
> @@ -309,11 +309,13 @@ int pnv_calc_dpll_params(int refclk, struct dpll *clock)
>  {
>  	clock->m = clock->m2 + 2;
>  	clock->p = clock->p1 * clock->p2;
> -	if (WARN_ON(clock->n == 0 || clock->p == 0))
> -		return 0;
> +	if (WARN_ON(clock->n == 0 || clock->p == 0)) {
> +		clock->dot = 0;
> +		goto end;
> +	}
>  	clock->vco = DIV_ROUND_CLOSEST(refclk * clock->m, clock->n);
>  	clock->dot = DIV_ROUND_CLOSEST(clock->vco, clock->p);
> -
> +end:
>  	return clock->dot;
>  }
>  
> @@ -326,11 +328,13 @@ int i9xx_calc_dpll_params(int refclk, struct dpll *clock)
>  {
>  	clock->m = i9xx_dpll_compute_m(clock);
>  	clock->p = clock->p1 * clock->p2;
> -	if (WARN_ON(clock->n + 2 == 0 || clock->p == 0))
> -		return 0;
> +	if (WARN_ON(clock->n + 2 == 0 || clock->p == 0)) {
> +		clock->dot = 0;
> +		goto end;
> +	}
>  	clock->vco = DIV_ROUND_CLOSEST(refclk * clock->m, clock->n + 2);
>  	clock->dot = DIV_ROUND_CLOSEST(clock->vco, clock->p);
> -
> +end:
>  	return clock->dot;
>  }
>  
> @@ -338,11 +342,13 @@ int vlv_calc_dpll_params(int refclk, struct dpll *clock)
>  {
>  	clock->m = clock->m1 * clock->m2;
>  	clock->p = clock->p1 * clock->p2;
> -	if (WARN_ON(clock->n == 0 || clock->p == 0))
> -		return 0;
> +	if (WARN_ON(clock->n == 0 || clock->p == 0)) {
> +		clock->dot = 0;
> +		goto end;
> +	}
>  	clock->vco = DIV_ROUND_CLOSEST(refclk * clock->m, clock->n);
>  	clock->dot = DIV_ROUND_CLOSEST(clock->vco, clock->p);
> -
> +end:
>  	return clock->dot / 5;
>  }
>  
> @@ -350,12 +356,14 @@ int chv_calc_dpll_params(int refclk, struct dpll *clock)
>  {
>  	clock->m = clock->m1 * clock->m2;
>  	clock->p = clock->p1 * clock->p2;
> -	if (WARN_ON(clock->n == 0 || clock->p == 0))
> -		return 0;
> +	if (WARN_ON(clock->n == 0 || clock->p == 0)) {
> +		clock->dot = 0;
> +		goto end;
> +	}
>  	clock->vco = DIV_ROUND_CLOSEST_ULL(mul_u32_u32(refclk, clock->m),
>  					   clock->n << 22);
>  	clock->dot = DIV_ROUND_CLOSEST(clock->vco, clock->p);
> -
> +end:
>  	return clock->dot / 5;
>  }
>  
> -- 
> 2.26.3

-- 
Ville Syrjälä
Intel

Re: [PATCH] drm/i915: rework the error handling in *_dpll_params

[Ville Syrjälä]

On Fri, Mar 04, 2022 at 11:11:54PM +0200, Ville Syrjälä wrote:
> On Fri, Mar 04, 2022 at 01:03:55PM -0800, trix@redhat.com wrote:
> > From: Tom Rix <trix@redhat.com>
> > 
> > Clang static analysis reports this issue
> > intel_dpll.c:472:31: warning: The left operand of '-'
> >   is a garbage value [core.UndefinedBinaryOperatorResult]
> >   this_err = abs(clock.dot - target);
> >                  ~~~~~~~~~ ^
> > 
> > In a loop clock.dot is set on successful call to
> > i9xx_calc_dpll_params().  If the call fails, the later
> > *is_valid() will use the previous loop's clock.dot.
> 
> I don't think this can happen. intel_pll_is_valid() validates
> all the dividers first and bails out if they are junk.

Hmm. That said, there is actually a case to be made for fully
initializing the struct, and even removing the WARN. If the
BIOS (or whatever was doing stuff before i915 takes over)
has misprogrammed the DPLL then we could potentially have
garbage dividers on our hands, and during readout we'd just
blindly call *_calc_dpll_params() on them.

So I'm thinking something along the lines of
 clock->vco = <divisor> ? DIV_ROUND_CLOSEST(...) : 0;
 clock->dot = <divisor> ? DIV_ROUND_CLOSEST(...) : 0;
might be what we should do here.

To make it a bit less ugly a small helper function might
be in order. intel_pll_div() perhaps?

> 
> > 
> > The *_dpll_params functions return an arithmetic statement
> > with the clock.dot as the variable.  Change the error handler
> > to set clock.dot to 0 and jump to the return statement.
> > 
> > Fixes: dccbea3b0704 ("drm/i915: calculate the port clock rate along with other PLL params")
> > Signed-off-by: Tom Rix <trix@redhat.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_dpll.c | 32 ++++++++++++++---------
> >  1 file changed, 20 insertions(+), 12 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_dpll.c b/drivers/gpu/drm/i915/display/intel_dpll.c
> > index 0ae37fdbf2a5b..ba7cada704288 100644
> > --- a/drivers/gpu/drm/i915/display/intel_dpll.c
> > +++ b/drivers/gpu/drm/i915/display/intel_dpll.c
> > @@ -309,11 +309,13 @@ int pnv_calc_dpll_params(int refclk, struct dpll *clock)
> >  {
> >  	clock->m = clock->m2 + 2;
> >  	clock->p = clock->p1 * clock->p2;
> > -	if (WARN_ON(clock->n == 0 || clock->p == 0))
> > -		return 0;
> > +	if (WARN_ON(clock->n == 0 || clock->p == 0)) {
> > +		clock->dot = 0;
> > +		goto end;
> > +	}
> >  	clock->vco = DIV_ROUND_CLOSEST(refclk * clock->m, clock->n);
> >  	clock->dot = DIV_ROUND_CLOSEST(clock->vco, clock->p);
> > -
> > +end:
> >  	return clock->dot;
> >  }
> >  
> > @@ -326,11 +328,13 @@ int i9xx_calc_dpll_params(int refclk, struct dpll *clock)
> >  {
> >  	clock->m = i9xx_dpll_compute_m(clock);
> >  	clock->p = clock->p1 * clock->p2;
> > -	if (WARN_ON(clock->n + 2 == 0 || clock->p == 0))
> > -		return 0;
> > +	if (WARN_ON(clock->n + 2 == 0 || clock->p == 0)) {
> > +		clock->dot = 0;
> > +		goto end;
> > +	}
> >  	clock->vco = DIV_ROUND_CLOSEST(refclk * clock->m, clock->n + 2);
> >  	clock->dot = DIV_ROUND_CLOSEST(clock->vco, clock->p);
> > -
> > +end:
> >  	return clock->dot;
> >  }
> >  
> > @@ -338,11 +342,13 @@ int vlv_calc_dpll_params(int refclk, struct dpll *clock)
> >  {
> >  	clock->m = clock->m1 * clock->m2;
> >  	clock->p = clock->p1 * clock->p2;
> > -	if (WARN_ON(clock->n == 0 || clock->p == 0))
> > -		return 0;
> > +	if (WARN_ON(clock->n == 0 || clock->p == 0)) {
> > +		clock->dot = 0;
> > +		goto end;
> > +	}
> >  	clock->vco = DIV_ROUND_CLOSEST(refclk * clock->m, clock->n);
> >  	clock->dot = DIV_ROUND_CLOSEST(clock->vco, clock->p);
> > -
> > +end:
> >  	return clock->dot / 5;
> >  }
> >  
> > @@ -350,12 +356,14 @@ int chv_calc_dpll_params(int refclk, struct dpll *clock)
> >  {
> >  	clock->m = clock->m1 * clock->m2;
> >  	clock->p = clock->p1 * clock->p2;
> > -	if (WARN_ON(clock->n == 0 || clock->p == 0))
> > -		return 0;
> > +	if (WARN_ON(clock->n == 0 || clock->p == 0)) {
> > +		clock->dot = 0;
> > +		goto end;
> > +	}
> >  	clock->vco = DIV_ROUND_CLOSEST_ULL(mul_u32_u32(refclk, clock->m),
> >  					   clock->n << 22);
> >  	clock->dot = DIV_ROUND_CLOSEST(clock->vco, clock->p);
> > -
> > +end:
> >  	return clock->dot / 5;
> >  }
> >  
> > -- 
> > 2.26.3
> 
> -- 
> Ville Syrjälä
> Intel

-- 
Ville Syrjälä
Intel