All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] drm/i915: Fix erroneous conversion to u8
@ 2014-08-08 17:34 Damien Lespiau
  2014-08-08 17:56 ` Ville Syrjälä
  0 siblings, 1 reply; 5+ messages in thread
From: Damien Lespiau @ 2014-08-08 17:34 UTC (permalink / raw)
  To: intel-gfx

adj was defined as u8. The issue is last_adj can be negative and adj is
initialized with:

  adj = dev_priv->rps.last_adj;

and we were also happily doing things like:

  if (adj < 0)

(thank static analysers!)

Cc: Deepak S <deepak.s@linux.intel.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
---
 drivers/gpu/drm/i915/i915_irq.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 9fdf738..89e633f 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -1330,7 +1330,8 @@ static u32 vlv_c0_residency(struct drm_i915_private *dev_priv,
 static u32 vlv_calc_delay_from_C0_counters(struct drm_i915_private *dev_priv)
 {
 	u32 residency_C0_up = 0, residency_C0_down = 0;
-	u8 new_delay, adj;
+	u8 new_delay;
+	int adj;
 
 	dev_priv->rps.ei_interrupt_count++;
 
-- 
1.8.3.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply related	[flat|nested] 5+ messages in thread
* Re: [PATCH] drm/i915: Fix erroneous conversion to u8
  2014-08-08 17:34 [PATCH] drm/i915: Fix erroneous conversion to u8 Damien Lespiau
@ 2014-08-08 17:56 ` Ville Syrjälä
  2014-08-08 18:25   ` Damien Lespiau
  0 siblings, 1 reply; 5+ messages in thread
From: Ville Syrjälä @ 2014-08-08 17:56 UTC (permalink / raw)
  To: Damien Lespiau; +Cc: intel-gfx

On Fri, Aug 08, 2014 at 06:34:48PM +0100, Damien Lespiau wrote:
> adj was defined as u8. The issue is last_adj can be negative and adj is
> initialized with:
> 
>   adj = dev_priv->rps.last_adj;
> 
> and we were also happily doing things like:
> 
>   if (adj < 0)
> 
> (thank static analysers!)
> 
> Cc: Deepak S <deepak.s@linux.intel.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_irq.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 9fdf738..89e633f 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -1330,7 +1330,8 @@ static u32 vlv_c0_residency(struct drm_i915_private *dev_priv,
>  static u32 vlv_calc_delay_from_C0_counters(struct drm_i915_private *dev_priv)
>  {
>  	u32 residency_C0_up = 0, residency_C0_down = 0;
> -	u8 new_delay, adj;
> +	u8 new_delay;
> +	int adj;

Might be better to make new_delay int too, so that we don't accidentally
under/overflow it due to the adj acceleration thing. And the return
value too. I'm feeling too lazy to think what kind of conditions that
would required, so just making it all int seems easier.

>  
>  	dev_priv->rps.ei_interrupt_count++;
>  
> -- 
> 1.8.3.1

-- 
Ville Syrjälä
Intel OTC

^ permalink raw reply	[flat|nested] 5+ messages in thread
* [PATCH] drm/i915: Fix erroneous conversion to u8
  2014-08-08 17:56 ` Ville Syrjälä
@ 2014-08-08 18:25   ` Damien Lespiau
  2014-08-08 18:32     ` Ville Syrjälä
  0 siblings, 1 reply; 5+ messages in thread
From: Damien Lespiau @ 2014-08-08 18:25 UTC (permalink / raw)
  To: intel-gfx

adj was defined as u8. The issue is last_adj can be negative and adj is
initialized with:

  adj = dev_priv->rps.last_adj;

and we were also happily doing things like:

  if (adj < 0)

(thank static analysers!)

v2: Make new_delay an int in case we overflow the u8 in the intermediate
    computations. new_delay will get clamped at the end anyway. (Ville)

Cc: Deepak S <deepak.s@linux.intel.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
---
 drivers/gpu/drm/i915/i915_irq.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 9fdf738..8e6729e 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -1327,10 +1327,10 @@ static u32 vlv_c0_residency(struct drm_i915_private *dev_priv,
  * @dev_priv: DRM device private
  *
  */
-static u32 vlv_calc_delay_from_C0_counters(struct drm_i915_private *dev_priv)
+static int vlv_calc_delay_from_C0_counters(struct drm_i915_private *dev_priv)
 {
 	u32 residency_C0_up = 0, residency_C0_down = 0;
-	u8 new_delay, adj;
+	int new_delay, adj;
 
 	dev_priv->rps.ei_interrupt_count++;
 
-- 
1.8.3.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply related	[flat|nested] 5+ messages in thread
* Re: [PATCH] drm/i915: Fix erroneous conversion to u8
  2014-08-08 18:25   ` Damien Lespiau
@ 2014-08-08 18:32     ` Ville Syrjälä
  2014-08-08 18:52       ` Daniel Vetter
  0 siblings, 1 reply; 5+ messages in thread
From: Ville Syrjälä @ 2014-08-08 18:32 UTC (permalink / raw)
  To: Damien Lespiau; +Cc: intel-gfx

On Fri, Aug 08, 2014 at 07:25:57PM +0100, Damien Lespiau wrote:
> adj was defined as u8. The issue is last_adj can be negative and adj is
> initialized with:
> 
>   adj = dev_priv->rps.last_adj;
> 
> and we were also happily doing things like:
> 
>   if (adj < 0)
> 
> (thank static analysers!)
> 
> v2: Make new_delay an int in case we overflow the u8 in the intermediate
>     computations. new_delay will get clamped at the end anyway. (Ville)
> 
> Cc: Deepak S <deepak.s@linux.intel.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>

Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> ---
>  drivers/gpu/drm/i915/i915_irq.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 9fdf738..8e6729e 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -1327,10 +1327,10 @@ static u32 vlv_c0_residency(struct drm_i915_private *dev_priv,
>   * @dev_priv: DRM device private
>   *
>   */
> -static u32 vlv_calc_delay_from_C0_counters(struct drm_i915_private *dev_priv)
> +static int vlv_calc_delay_from_C0_counters(struct drm_i915_private *dev_priv)
>  {
>  	u32 residency_C0_up = 0, residency_C0_down = 0;
> -	u8 new_delay, adj;
> +	int new_delay, adj;
>  
>  	dev_priv->rps.ei_interrupt_count++;
>  
> -- 
> 1.8.3.1

-- 
Ville Syrjälä
Intel OTC

^ permalink raw reply	[flat|nested] 5+ messages in thread
* Re: [PATCH] drm/i915: Fix erroneous conversion to u8
  2014-08-08 18:32     ` Ville Syrjälä
@ 2014-08-08 18:52       ` Daniel Vetter
  0 siblings, 0 replies; 5+ messages in thread
From: Daniel Vetter @ 2014-08-08 18:52 UTC (permalink / raw)
  To: Ville Syrjälä; +Cc: intel-gfx

On Fri, Aug 08, 2014 at 09:32:20PM +0300, Ville Syrjälä wrote:
> On Fri, Aug 08, 2014 at 07:25:57PM +0100, Damien Lespiau wrote:
> > adj was defined as u8. The issue is last_adj can be negative and adj is
> > initialized with:
> > 
> >   adj = dev_priv->rps.last_adj;
> > 
> > and we were also happily doing things like:
> > 
> >   if (adj < 0)
> > 
> > (thank static analysers!)
> > 
> > v2: Make new_delay an int in case we overflow the u8 in the intermediate
> >     computations. new_delay will get clamped at the end anyway. (Ville)
> > 
> > Cc: Deepak S <deepak.s@linux.intel.com>
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
> 
> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Queued for -next, thanks for the patch.
-Daniel

> 
> > ---
> >  drivers/gpu/drm/i915/i915_irq.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> > index 9fdf738..8e6729e 100644
> > --- a/drivers/gpu/drm/i915/i915_irq.c
> > +++ b/drivers/gpu/drm/i915/i915_irq.c
> > @@ -1327,10 +1327,10 @@ static u32 vlv_c0_residency(struct drm_i915_private *dev_priv,
> >   * @dev_priv: DRM device private
> >   *
> >   */
> > -static u32 vlv_calc_delay_from_C0_counters(struct drm_i915_private *dev_priv)
> > +static int vlv_calc_delay_from_C0_counters(struct drm_i915_private *dev_priv)
> >  {
> >  	u32 residency_C0_up = 0, residency_C0_down = 0;
> > -	u8 new_delay, adj;
> > +	int new_delay, adj;
> >  
> >  	dev_priv->rps.ei_interrupt_count++;
> >  
> > -- 
> > 1.8.3.1
> 
> -- 
> Ville Syrjälä
> Intel OTC
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

^ permalink raw reply	[flat|nested] 5+ messages in thread
end of thread, other threads:[~2014-08-08 18:52 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-08-08 17:34 [PATCH] drm/i915: Fix erroneous conversion to u8 Damien Lespiau
2014-08-08 17:56 ` Ville Syrjälä
2014-08-08 18:25   ` Damien Lespiau
2014-08-08 18:32     ` Ville Syrjälä
2014-08-08 18:52       ` Daniel Vetter

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.