2022-03-04 21:05:05

by Tom Rix

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

From: Tom Rix <[email protected]>

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 <[email protected]>
---
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

2022-03-04 21:14:02

by Ville Syrjälä

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

On Fri, Mar 04, 2022 at 01:03:55PM -0800, [email protected] wrote:
> From: Tom Rix <[email protected]>
>
> 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 <[email protected]>
> ---
> 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

2022-03-04 21:26:48

by Ville Syrjälä

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

On Fri, Mar 04, 2022 at 11:11:54PM +0200, Ville Syrj?l? wrote:
> On Fri, Mar 04, 2022 at 01:03:55PM -0800, [email protected] wrote:
> > From: Tom Rix <[email protected]>
> >
> > 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 <[email protected]>
> > ---
> > 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