[PATCH] drm/i915: Use GEM_WARN_ON to catch invalid engine indices.

[PATCH] drm/i915: Use GEM_WARN_ON to catch invalid engine indices.

[Michal Wajdeczko]

Engine related definitions are located in different files
and it is easy to break their cross dependency.

v2: compare against array size
v3: don't use BUILD_BUG (Tvrtko)

Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: Jani Nikula <jani.nikula@intel.com>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 drivers/gpu/drm/i915/intel_engine_cs.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c
index a238304..8675164 100644
--- a/drivers/gpu/drm/i915/intel_engine_cs.c
+++ b/drivers/gpu/drm/i915/intel_engine_cs.c
@@ -89,7 +89,11 @@ intel_engine_setup(struct drm_i915_private *dev_priv,
 	const struct engine_info *info = &intel_engines[id];
 	struct intel_engine_cs *engine;
 
-	GEM_BUG_ON(dev_priv->engine[id]);
+	if (GEM_WARN_ON((unsigned)id >= ARRAY_SIZE(intel_engines)))
+		return -EINVAL;
+	if (GEM_WARN_ON((unsigned)id >= ARRAY_SIZE(dev_priv->engine)))
+		return -EINVAL;
+
 	engine = kzalloc(sizeof(*engine), GFP_KERNEL);
 	if (!engine)
 		return -ENOMEM;
@@ -105,6 +109,7 @@ intel_engine_setup(struct drm_i915_private *dev_priv,
 	/* Nothing to do here, execute in order of dependencies */
 	engine->schedule = NULL;
 
+	GEM_BUG_ON(dev_priv->engine[id]);
 	dev_priv->engine[id] = engine;
 	return 0;
 }
-- 
2.7.4

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

✗ Fi.CI.BAT: warning for drm/i915: Use GEM_WARN_ON to catch invalid engine indices.

[Patchwork]

== Series Details ==

Series: drm/i915: Use GEM_WARN_ON to catch invalid engine indices.
URL   : https://patchwork.freedesktop.org/series/20486/
State : warning

== Summary ==

Series 20486v1 drm/i915: Use GEM_WARN_ON to catch invalid engine indices.
https://patchwork.freedesktop.org/api/1.0/series/20486/revisions/1/mbox/

Test drv_module_reload:
        Subgroup basic-reload:
                pass       -> DMESG-WARN (fi-skl-6770hq)
Test gem_exec_suspend:
        Subgroup basic-s4-devices:
                dmesg-warn -> PASS       (fi-bxt-t5700)

fi-bdw-5557u     total:278  pass:267  dwarn:0   dfail:0   fail:0   skip:11 
fi-bsw-n3050     total:278  pass:239  dwarn:0   dfail:0   fail:0   skip:39 
fi-bxt-j4205     total:278  pass:259  dwarn:0   dfail:0   fail:0   skip:19 
fi-bxt-t5700     total:278  pass:258  dwarn:0   dfail:0   fail:0   skip:20 
fi-byt-j1900     total:278  pass:251  dwarn:0   dfail:0   fail:0   skip:27 
fi-byt-n2820     total:278  pass:247  dwarn:0   dfail:0   fail:0   skip:31 
fi-hsw-4770      total:278  pass:262  dwarn:0   dfail:0   fail:0   skip:16 
fi-hsw-4770r     total:278  pass:262  dwarn:0   dfail:0   fail:0   skip:16 
fi-ilk-650       total:278  pass:192  dwarn:36  dfail:0   fail:0   skip:50 
fi-ivb-3520m     total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18 
fi-ivb-3770      total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18 
fi-kbl-7500u     total:278  pass:259  dwarn:1   dfail:0   fail:0   skip:18 
fi-skl-6260u     total:278  pass:268  dwarn:0   dfail:0   fail:0   skip:10 
fi-skl-6700hq    total:278  pass:261  dwarn:0   dfail:0   fail:0   skip:17 
fi-skl-6700k     total:278  pass:216  dwarn:44  dfail:0   fail:0   skip:18 
fi-skl-6770hq    total:278  pass:267  dwarn:1   dfail:0   fail:0   skip:10 
fi-snb-2520m     total:278  pass:250  dwarn:0   dfail:0   fail:0   skip:28 
fi-snb-2600      total:278  pass:249  dwarn:0   dfail:0   fail:0   skip:29 

a777f02a6f1fbe44b42f245d6397b3f960a0a257 drm-tip: 2017y-03m-01d-20h-32m-24s UTC integration manifest
c8051c6 drm/i915: Use GEM_WARN_ON to catch invalid engine indices.

== Logs ==

For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_4025/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Use GEM_WARN_ON to catch invalid engine indices.

[Tvrtko Ursulin]

On 01/03/2017 20:27, Michal Wajdeczko wrote:
> Engine related definitions are located in different files
> and it is easy to break their cross dependency.
>
> v2: compare against array size
> v3: don't use BUILD_BUG (Tvrtko)
>
> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: Jani Nikula <jani.nikula@intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_engine_cs.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c
> index a238304..8675164 100644
> --- a/drivers/gpu/drm/i915/intel_engine_cs.c
> +++ b/drivers/gpu/drm/i915/intel_engine_cs.c
> @@ -89,7 +89,11 @@ intel_engine_setup(struct drm_i915_private *dev_priv,
>  	const struct engine_info *info = &intel_engines[id];

Just need to move this assignment after the assert.

>  	struct intel_engine_cs *engine;
>
> -	GEM_BUG_ON(dev_priv->engine[id]);
> +	if (GEM_WARN_ON((unsigned)id >= ARRAY_SIZE(intel_engines)))
> +		return -EINVAL;
> +	if (GEM_WARN_ON((unsigned)id >= ARRAY_SIZE(dev_priv->engine)))
> +		return -EINVAL;
> +
>  	engine = kzalloc(sizeof(*engine), GFP_KERNEL);
>  	if (!engine)
>  		return -ENOMEM;
> @@ -105,6 +109,7 @@ intel_engine_setup(struct drm_i915_private *dev_priv,
>  	/* Nothing to do here, execute in order of dependencies */
>  	engine->schedule = NULL;
>
> +	GEM_BUG_ON(dev_priv->engine[id]);

It doesn't really matter but I think moving this one was not needed.

>  	dev_priv->engine[id] = engine;
>  	return 0;
>  }
>

Regards,

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

Re: [PATCH] drm/i915: Use GEM_WARN_ON to catch invalid engine indices.

[Michal Wajdeczko]

On Thu, Mar 02, 2017 at 07:33:26AM +0000, Tvrtko Ursulin wrote:
> 
> On 01/03/2017 20:27, Michal Wajdeczko wrote:
> > Engine related definitions are located in different files
> > and it is easy to break their cross dependency.
> > 
> > v2: compare against array size
> > v3: don't use BUILD_BUG (Tvrtko)
> > 
> > Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
> > Cc: Jani Nikula <jani.nikula@intel.com>
> > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_engine_cs.c | 7 ++++++-
> >  1 file changed, 6 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c
> > index a238304..8675164 100644
> > --- a/drivers/gpu/drm/i915/intel_engine_cs.c
> > +++ b/drivers/gpu/drm/i915/intel_engine_cs.c
> > @@ -89,7 +89,11 @@ intel_engine_setup(struct drm_i915_private *dev_priv,
> >  	const struct engine_info *info = &intel_engines[id];
> 
> Just need to move this assignment after the assert.

I was thinking about it, but decided to leave as is, as this is just
pointer assignment and there is no read/write from this pointer.
But I'll move it, to make this code even more robust ;)

> 
> >  	struct intel_engine_cs *engine;
> > 
> > -	GEM_BUG_ON(dev_priv->engine[id]);
> > +	if (GEM_WARN_ON((unsigned)id >= ARRAY_SIZE(intel_engines)))
> > +		return -EINVAL;
> > +	if (GEM_WARN_ON((unsigned)id >= ARRAY_SIZE(dev_priv->engine)))
> > +		return -EINVAL;
> > +
> >  	engine = kzalloc(sizeof(*engine), GFP_KERNEL);
> >  	if (!engine)
> >  		return -ENOMEM;
> > @@ -105,6 +109,7 @@ intel_engine_setup(struct drm_i915_private *dev_priv,
> >  	/* Nothing to do here, execute in order of dependencies */
> >  	engine->schedule = NULL;
> > 
> > +	GEM_BUG_ON(dev_priv->engine[id]);
> 
> It doesn't really matter but I think moving this one was not needed.

Moved here just to correlate this assert with the actual critical assignment
from the line below. I can revert it if you prefer old way.

> 
> >  	dev_priv->engine[id] = engine;
> >  	return 0;
> >  }
> > 
> 

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

Re: [PATCH] drm/i915: Use GEM_WARN_ON to catch invalid engine indices.

[Chris Wilson]

On Thu, Mar 02, 2017 at 10:42:05AM +0100, Michal Wajdeczko wrote:
> On Thu, Mar 02, 2017 at 07:33:26AM +0000, Tvrtko Ursulin wrote:
> > 
> > On 01/03/2017 20:27, Michal Wajdeczko wrote:
> > > +	GEM_BUG_ON(dev_priv->engine[id]);
> > 
> > It doesn't really matter but I think moving this one was not needed.
> 
> Moved here just to correlate this assert with the actual critical assignment
> from the line below. I can revert it if you prefer old way.

It just comes down to whether we want to document the function
preconditions, or indicate the critical relationships.  Either is good.
In this case we choose to document that it would be silly to call setup
multiple times for the same engine.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx