| Message ID | 20170301202708.113928-1-michal.wajdeczko@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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; > > } > > >
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
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; }
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(-)