| Message ID | 20171222122556.28384-5-michal.wajdeczko@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Quoting Michal Wajdeczko (2017-12-22 12:25:55) > If someone provides too large number for fixed16 type > we will WARN but we will not correctly clamp values > and that may lead to fully wrong calculations. > > Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com> > Cc: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> > --- > drivers/gpu/drm/i915/fixed16_16.h | 13 +++++++++---- > 1 file changed, 9 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/i915/fixed16_16.h b/drivers/gpu/drm/i915/fixed16_16.h > index af23997..43fe0037 100644 > --- a/drivers/gpu/drm/i915/fixed16_16.h > +++ b/drivers/gpu/drm/i915/fixed16_16.h > @@ -57,7 +57,8 @@ static inline fixed16_16_t u32_to_fixed16(u32 val) > { > fixed16_16_t fp; > > - WARN_ON(val > U16_MAX); > + if (WARN_ON(val > U16_MAX)) > + val = U16_MAX; return FIXED16_16_MAX; If val is too large, the closest we can get to val in our representation is 16_16_MAX. > > fp.val = val << 16; > return fp; return TO_FIXED16_16(val << 16); > @@ -93,7 +94,9 @@ static inline fixed16_16_t clamp_u64_to_fixed16(u64 val) > { > fixed16_16_t fp; > > - WARN_ON(val > U32_MAX); > + if (WARN_ON(val > U32_MAX)) > + val = U32_MAX; I would do the early return. Perhaps check with gcc if prefers returning a constant than the jump back? -Chris
On Fri, 22 Dec 2017 13:39:11 +0100, Chris Wilson <chris@chris-wilson.co.uk> wrote: > Quoting Michal Wajdeczko (2017-12-22 12:25:55) >> If someone provides too large number for fixed16 type >> we will WARN but we will not correctly clamp values >> and that may lead to fully wrong calculations. >> >> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com> >> Cc: Chris Wilson <chris@chris-wilson.co.uk> >> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> >> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> >> --- >> drivers/gpu/drm/i915/fixed16_16.h | 13 +++++++++---- >> 1 file changed, 9 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/fixed16_16.h >> b/drivers/gpu/drm/i915/fixed16_16.h >> index af23997..43fe0037 100644 >> --- a/drivers/gpu/drm/i915/fixed16_16.h >> +++ b/drivers/gpu/drm/i915/fixed16_16.h >> @@ -57,7 +57,8 @@ static inline fixed16_16_t u32_to_fixed16(u32 val) >> { >> fixed16_16_t fp; >> >> - WARN_ON(val > U16_MAX); >> + if (WARN_ON(val > U16_MAX)) >> + val = U16_MAX; > > return FIXED16_16_MAX; > > If val is too large, the closest we can get to val in our representation > is 16_16_MAX. hmm, true, but then our fixed representation will include fractional part... but I guess it is still ok > >> >> fp.val = val << 16; >> return fp; > > return TO_FIXED16_16(val << 16); > >> @@ -93,7 +94,9 @@ static inline fixed16_16_t clamp_u64_to_fixed16(u64 >> val) >> { >> fixed16_16_t fp; >> >> - WARN_ON(val > U32_MAX); >> + if (WARN_ON(val > U32_MAX)) >> + val = U32_MAX; > > I would do the early return. Perhaps check with gcc if prefers returning > a constant than the jump back? > -Chris
diff --git a/drivers/gpu/drm/i915/fixed16_16.h b/drivers/gpu/drm/i915/fixed16_16.h index af23997..43fe0037 100644 --- a/drivers/gpu/drm/i915/fixed16_16.h +++ b/drivers/gpu/drm/i915/fixed16_16.h @@ -57,7 +57,8 @@ static inline fixed16_16_t u32_to_fixed16(u32 val) { fixed16_16_t fp; - WARN_ON(val > U16_MAX); + if (WARN_ON(val > U16_MAX)) + val = U16_MAX; fp.val = val << 16; return fp; @@ -93,7 +94,9 @@ static inline fixed16_16_t clamp_u64_to_fixed16(u64 val) { fixed16_16_t fp; - WARN_ON(val > U32_MAX); + if (WARN_ON(val > U32_MAX)) + val = U32_MAX; + fp.val = (u32) val; return fp; } @@ -109,7 +112,8 @@ static inline u32 mul_round_up_u32_fixed16(u32 val, fixed16_16_t mul) intermediate_val = (u64) val * mul.val; intermediate_val = DIV_ROUND_UP_ULL(intermediate_val, 1 << 16); - WARN_ON(intermediate_val > U32_MAX); + if (WARN_ON(intermediate_val > U32_MAX)) + intermediate_val = U32_MAX; return (u32) intermediate_val; } @@ -137,7 +141,8 @@ static inline u32 div_round_up_u32_fixed16(u32 val, fixed16_16_t d) interm_val = (u64)val << 16; interm_val = DIV_ROUND_UP_ULL(interm_val, d.val); - WARN_ON(interm_val > U32_MAX); + if (WARN_ON(interm_val > U32_MAX)) + interm_val = U32_MAX; return (u32) interm_val; }
If someone provides too large number for fixed16 type we will WARN but we will not correctly clamp values and that may lead to fully wrong calculations. Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com> Cc: Chris Wilson <chris@chris-wilson.co.uk> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> --- drivers/gpu/drm/i915/fixed16_16.h | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-)