[PATCH] i915: Increase *_latency array size

[PATCH] i915: Increase *_latency array size

[Andi Kleen]

From: Andi Kleen <andi@firstfloor.org>

Newer gcc prints the following warning:

drivers/gpu/drm/i915/intel_pm.c:3057:9: warning: ‘intel_print_wm_latency’ reading 16 bytes from a region of size 10 [-Wstringop-overread]
and some other related warnings in similar functions.

gcc has a point here. Some of the latency arrays only have 5 members,
but print_wm_latency may read up to max_level returned by ilk_wm_max_level,
which can be upto 7 for the >= GEN9 case.

So it will read some fields beyond the array.

Increase all the latency fields to 8 members, which is enough for SKL.

I don't know if they are correctly initialized upto 8, but dev_priv
should start out as zero, so presumably they will be zero.

Signed-off-by: Andi Kleen <andi@firstfloor.org>
---
 drivers/gpu/drm/i915/i915_drv.h | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index cb62ddba2035..c80add5f6d33 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1095,11 +1095,11 @@ struct drm_i915_private {
 		 * in 0.5us units for WM1+.
 		 */
 		/* primary */
-		u16 pri_latency[5];
+		u16 pri_latency[8];
 		/* sprite */
-		u16 spr_latency[5];
+		u16 spr_latency[8];
 		/* cursor */
-		u16 cur_latency[5];
+		u16 cur_latency[8];
 		/*
 		 * Raw watermark memory latency values
 		 * for SKL for all 8 levels
-- 
2.25.4

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] i915: Increase *_latency array size

[Jani Nikula]

On Tue, 04 May 2021, Andi Kleen <ak@linux.intel.com> wrote:
> From: Andi Kleen <andi@firstfloor.org>
>
> Newer gcc prints the following warning:
>
> drivers/gpu/drm/i915/intel_pm.c:3057:9: warning: ‘intel_print_wm_latency’ reading 16 bytes from a region of size 10 [-Wstringop-overread]
> and some other related warnings in similar functions.
>
> gcc has a point here. Some of the latency arrays only have 5 members,
> but print_wm_latency may read up to max_level returned by ilk_wm_max_level,
> which can be upto 7 for the >= GEN9 case.
>
> So it will read some fields beyond the array.
>
> Increase all the latency fields to 8 members, which is enough for SKL.
>
> I don't know if they are correctly initialized upto 8, but dev_priv
> should start out as zero, so presumably they will be zero.

Thanks, the warning should be fixed by commit

c6deb5e97ded ("drm/i915/pm: Make the wm parameter of print_wm_latency a pointer")

in drm-intel-next.

There doesn't actually seem to be a bug here, but I wonder if we should
send that to stable or v5.13-rc1+ anyway to stop people spending time on
the same issue.

BR,
Jani.

>
> Signed-off-by: Andi Kleen <andi@firstfloor.org>
> ---
>  drivers/gpu/drm/i915/i915_drv.h | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index cb62ddba2035..c80add5f6d33 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1095,11 +1095,11 @@ struct drm_i915_private {
>  		 * in 0.5us units for WM1+.
>  		 */
>  		/* primary */
> -		u16 pri_latency[5];
> +		u16 pri_latency[8];
>  		/* sprite */
> -		u16 spr_latency[5];
> +		u16 spr_latency[8];
>  		/* cursor */
> -		u16 cur_latency[5];
> +		u16 cur_latency[8];
>  		/*
>  		 * Raw watermark memory latency values
>  		 * for SKL for all 8 levels

-- 
Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] i915: Increase *_latency array size

[Andi Kleen]

> > Increase all the latency fields to 8 members, which is enough for SKL.
> >
> > I don't know if they are correctly initialized upto 8, but dev_priv
> > should start out as zero, so presumably they will be zero.
> 
> Thanks, the warning should be fixed by commit
> 
> c6deb5e97ded ("drm/i915/pm: Make the wm parameter of print_wm_latency a pointer")
> 
> in drm-intel-next.

That's just hiding the problem.

> 
> There doesn't actually seem to be a bug here,

Can you explain that please?

This is the loop in question

 max_level = ilk_wm_max_level(dev_priv);

        for (level = 0; level <= max_level; level++) {
                unsigned int latency = wm[level];

                if (latency == 0) {
                        drm_dbg_kms(&dev_priv->drm,
                                    "%s WM%d latency not provided\n",
                                    name, level);
                        continue;
                }

		...
	}

(no other loop termination condition)

and ilk_wm_max_level is

int ilk_wm_max_level(const struct drm_i915_private *dev_priv)
{
        /* how many WM levels are we expecting */
        if (INTEL_GEN(dev_priv) >= 9)
                return 7;
        else if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv))
                return 4;
        else if (INTEL_GEN(dev_priv) >= 6)
                return 3;
        else
                return 2;
}

There is no loop termination in the loop above, it will always read
every member through the max level reported. And on GEN>=9 it will be 7, while
the input array for several of the cases has only 5 members.

So it will read beyond the array and gcc is correct in complaining.

What do I miss when you say there is no bug?

-Andi

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Intel-gfx] [PATCH] i915: Increase *_latency array size

[Ville Syrjälä]

On Wed, May 05, 2021 at 07:18:30AM -0700, Andi Kleen wrote:
> > > Increase all the latency fields to 8 members, which is enough for SKL.
> > >
> > > I don't know if they are correctly initialized upto 8, but dev_priv
> > > should start out as zero, so presumably they will be zero.
> > 
> > Thanks, the warning should be fixed by commit
> > 
> > c6deb5e97ded ("drm/i915/pm: Make the wm parameter of print_wm_latency a pointer")
> > 
> > in drm-intel-next.
> 
> That's just hiding the problem.
> 
> > 
> > There doesn't actually seem to be a bug here,
> 
> Can you explain that please?
> 
> This is the loop in question
> 
>  max_level = ilk_wm_max_level(dev_priv);
> 
>         for (level = 0; level <= max_level; level++) {
>                 unsigned int latency = wm[level];
> 
>                 if (latency == 0) {
>                         drm_dbg_kms(&dev_priv->drm,
>                                     "%s WM%d latency not provided\n",
>                                     name, level);
>                         continue;
>                 }
> 
> 		...
> 	}
> 
> (no other loop termination condition)
> 
> and ilk_wm_max_level is
> 
> int ilk_wm_max_level(const struct drm_i915_private *dev_priv)
> {
>         /* how many WM levels are we expecting */
>         if (INTEL_GEN(dev_priv) >= 9)
>                 return 7;
>         else if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv))
>                 return 4;
>         else if (INTEL_GEN(dev_priv) >= 6)
>                 return 3;
>         else
>                 return 2;
> }
> 
> There is no loop termination in the loop above, it will always read
> every member through the max level reported. And on GEN>=9 it will be 7, while
> the input array for several of the cases has only 5 members.
> 
> So it will read beyond the array and gcc is correct in complaining.
> 
> What do I miss when you say there is no bug?

We always use dev_priv->wm.skl_latency[] for gen9+. See
{pri,spr,cur}_wm_latency_show(), skl_setup_wm_latency(), etc.

-- 
Ville Syrjälä
Intel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Intel-gfx] [PATCH] i915: Increase *_latency array size

[Jani Nikula]

On Wed, 05 May 2021, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> On Wed, May 05, 2021 at 07:18:30AM -0700, Andi Kleen wrote:
>> What do I miss when you say there is no bug?
>
> We always use dev_priv->wm.skl_latency[] for gen9+. See
> {pri,spr,cur}_wm_latency_show(), skl_setup_wm_latency(), etc.

Granted, we should probably make this more obvious and/or pass in the
buf size to make it easier to see what's going on.

BR,
Jani.


-- 
Jani Nikula, Intel Open Source Graphics Center