| Message ID | 20170228140050.118232-1-michal.wajdeczko@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On ti, 2017-02-28 at 14:00 +0000, Michal Wajdeczko wrote: > Additionally use runtime check to catch invalid engine indices. > > Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com> > Cc: Jani Nikula <jani.nikula@intel.com> Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> Regards, Joonas
On Tue, Feb 28, 2017 at 02:00:50PM +0000, Michal Wajdeczko wrote: > Additionally use runtime check to catch invalid engine indices. Oh no you don't! > Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com> > Cc: Jani Nikula <jani.nikula@intel.com> > --- > drivers/gpu/drm/i915/intel_engine_cs.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c > index a238304..8df53ae 100644 > --- a/drivers/gpu/drm/i915/intel_engine_cs.c > +++ b/drivers/gpu/drm/i915/intel_engine_cs.c > @@ -89,6 +89,8 @@ intel_engine_setup(struct drm_i915_private *dev_priv, > const struct engine_info *info = &intel_engines[id]; > struct intel_engine_cs *engine; > > + BUILD_BUG_ON(ARRAY_SIZE(intel_engines) != I915_NUM_ENGINES); > + GEM_BUG_ON(id < 0 || id >= I915_NUM_ENGINES); Are you sure sparse/smatch won't complain? /me too lazy to check the C standard for signedness of an enum without a negative value. -Chris
On Tue, Feb 28, 2017 at 02:12:59PM +0000, Chris Wilson wrote: > On Tue, Feb 28, 2017 at 02:00:50PM +0000, Michal Wajdeczko wrote: > > Additionally use runtime check to catch invalid engine indices. > > Oh no you don't! Parse error on my part, GEM_BUG_ON didn't register as runtime. Conditionally runtime! -Chris
On 28/02/2017 14:00, Michal Wajdeczko wrote: > Additionally use runtime check to catch invalid engine indices. > > Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com> > Cc: Jani Nikula <jani.nikula@intel.com> > --- > drivers/gpu/drm/i915/intel_engine_cs.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c > index a238304..8df53ae 100644 > --- a/drivers/gpu/drm/i915/intel_engine_cs.c > +++ b/drivers/gpu/drm/i915/intel_engine_cs.c > @@ -89,6 +89,8 @@ intel_engine_setup(struct drm_i915_private *dev_priv, > const struct engine_info *info = &intel_engines[id]; > struct intel_engine_cs *engine; > > + BUILD_BUG_ON(ARRAY_SIZE(intel_engines) != I915_NUM_ENGINES); For some reason I feel this is too strict. ;) > + GEM_BUG_ON(id < 0 || id >= I915_NUM_ENGINES); The caller of this function iterates 0..ARRAY_SIZE(intel_engines) and also filters with HAS_ENGINE before calling it so not sure this is absolutely needed. Maybe instead: GEM_BUG_ON(id >= ARRAY_SIZE(dev_priv->engine)); ? > GEM_BUG_ON(dev_priv->engine[id]); > engine = kzalloc(sizeof(*engine), GFP_KERNEL); > if (!engine) > Regards, Tvrtko
On Tue, Feb 28, 2017 at 02:12:59PM +0000, Chris Wilson wrote: > On Tue, Feb 28, 2017 at 02:00:50PM +0000, Michal Wajdeczko wrote: > > Additionally use runtime check to catch invalid engine indices. > > Oh no you don't! We can skip it, but today there is no way to verify that our enums fits into [0..I915_NUM_ENGINES) range. Additionally, you've moved definition of the I915_NUM_ENGINES to separate file far away from enum intel_engine_id definition. > > > Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com> > > Cc: Jani Nikula <jani.nikula@intel.com> > > --- > > drivers/gpu/drm/i915/intel_engine_cs.c | 2 ++ > > 1 file changed, 2 insertions(+) > > > > diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c > > index a238304..8df53ae 100644 > > --- a/drivers/gpu/drm/i915/intel_engine_cs.c > > +++ b/drivers/gpu/drm/i915/intel_engine_cs.c > > @@ -89,6 +89,8 @@ intel_engine_setup(struct drm_i915_private *dev_priv, > > const struct engine_info *info = &intel_engines[id]; > > struct intel_engine_cs *engine; > > > > + BUILD_BUG_ON(ARRAY_SIZE(intel_engines) != I915_NUM_ENGINES); > > + GEM_BUG_ON(id < 0 || id >= I915_NUM_ENGINES); > > Are you sure sparse/smatch won't complain? > /me too lazy to check the C standard for signedness of an enum without a > negative value. enums are int's and as such can be negative ;) see http://en.cppreference.com/w/c/language/enum -Michal
On ti, 2017-02-28 at 14:21 +0000, Tvrtko Ursulin wrote: > The caller of this function iterates 0..ARRAY_SIZE(intel_engines) and > also filters with HAS_ENGINE before calling it so not sure this is > absolutely needed. Maybe instead: > > GEM_BUG_ON(id >= ARRAY_SIZE(dev_priv->engine)); I think that's even better. Regards, Joonas
On Tue, Feb 28, 2017 at 02:21:23PM +0000, Tvrtko Ursulin wrote: > > On 28/02/2017 14:00, Michal Wajdeczko wrote: > > Additionally use runtime check to catch invalid engine indices. > > > > Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com> > > Cc: Jani Nikula <jani.nikula@intel.com> > > --- > > drivers/gpu/drm/i915/intel_engine_cs.c | 2 ++ > > 1 file changed, 2 insertions(+) > > > > diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c > > index a238304..8df53ae 100644 > > --- a/drivers/gpu/drm/i915/intel_engine_cs.c > > +++ b/drivers/gpu/drm/i915/intel_engine_cs.c > > @@ -89,6 +89,8 @@ intel_engine_setup(struct drm_i915_private *dev_priv, > > const struct engine_info *info = &intel_engines[id]; > > struct intel_engine_cs *engine; > > > > + BUILD_BUG_ON(ARRAY_SIZE(intel_engines) != I915_NUM_ENGINES); > > For some reason I feel this is too strict. ;) It has to be strict to be useful. > > > + GEM_BUG_ON(id < 0 || id >= I915_NUM_ENGINES); > > The caller of this function iterates 0..ARRAY_SIZE(intel_engines) and also > filters with HAS_ENGINE before calling it so not sure this is absolutely > needed. Maybe instead: > > GEM_BUG_ON(id >= ARRAY_SIZE(dev_priv->engine)); > With your approach we could drop GEM_BUG_ON completely as with correct iteration we should never hit condition id > ARRAY_SIZE. If we could assume that everyone is doing right, then we should never need any asserts at all. Problem is that this function does not know anything about the caller. And also it does not know if enums were defined correctly. But then it uses these enums as index into two external arrays. In my opition we should do our best to catch any inproper usage/definitions. If not everywhere, then at least once during build or boot. -Michal
On 28/02/2017 14:52, Michal Wajdeczko wrote: > On Tue, Feb 28, 2017 at 02:21:23PM +0000, Tvrtko Ursulin wrote: [snip] >>> + GEM_BUG_ON(id < 0 || id >= I915_NUM_ENGINES); >> >> The caller of this function iterates 0..ARRAY_SIZE(intel_engines) and also >> filters with HAS_ENGINE before calling it so not sure this is absolutely >> needed. Maybe instead: >> >> GEM_BUG_ON(id >= ARRAY_SIZE(dev_priv->engine)); >> > > With your approach we could drop GEM_BUG_ON completely as with correct > iteration we should never hit condition id > ARRAY_SIZE. > > If we could assume that everyone is doing right, then we should never > need any asserts at all. That is not correct. I suggested the function should just check the size of the array it is concerned with, rather than assuming how I915_NUM_ENGINES relates to the same array. > Problem is that this function does not know anything about the caller. > And also it does not know if enums were defined correctly. > But then it uses these enums as index into two external arrays. > In my opition we should do our best to catch any inproper usage/definitions. > If not everywhere, then at least once during build or boot. Agreed, but intel_engine_setup does not care about the enum so much. It cares that it doesn't do out of bounds access to the two arrays. In the light of that, GEM_BUG_ON(id >= ARRAY_SIZE(intel_engines) || id >= ARRAY_SIZE(dev_priv->engines)) sounds like the most robust solution to me. Since the function handles failure as well, perhaps we could even upgrade that to a WARN_ON and return -EINVAL. Not sure. Regards, Tvrtko
On Tue, Feb 28, 2017 at 03:52:31PM +0100, Michal Wajdeczko wrote: > On Tue, Feb 28, 2017 at 02:21:23PM +0000, Tvrtko Ursulin wrote: > > > > On 28/02/2017 14:00, Michal Wajdeczko wrote: > > > Additionally use runtime check to catch invalid engine indices. > > > > > > Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com> > > > Cc: Jani Nikula <jani.nikula@intel.com> > > > --- > > > drivers/gpu/drm/i915/intel_engine_cs.c | 2 ++ > > > 1 file changed, 2 insertions(+) > > > > > > diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c > > > index a238304..8df53ae 100644 > > > --- a/drivers/gpu/drm/i915/intel_engine_cs.c > > > +++ b/drivers/gpu/drm/i915/intel_engine_cs.c > > > @@ -89,6 +89,8 @@ intel_engine_setup(struct drm_i915_private *dev_priv, > > > const struct engine_info *info = &intel_engines[id]; > > > struct intel_engine_cs *engine; > > > > > > + BUILD_BUG_ON(ARRAY_SIZE(intel_engines) != I915_NUM_ENGINES); > > > > For some reason I feel this is too strict. ;) > > It has to be strict to be useful. But is pointless if it doesn't apply to gen+1, or tomorrow's packing of sparse engines, which is where Tvrtko is coming from. -Chris
On Tue, Feb 28, 2017 at 03:08:19PM +0000, Chris Wilson wrote: > On Tue, Feb 28, 2017 at 03:52:31PM +0100, Michal Wajdeczko wrote: > > On Tue, Feb 28, 2017 at 02:21:23PM +0000, Tvrtko Ursulin wrote: > > > > > > On 28/02/2017 14:00, Michal Wajdeczko wrote: > > > > Additionally use runtime check to catch invalid engine indices. > > > > > > > > Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com> > > > > Cc: Jani Nikula <jani.nikula@intel.com> > > > > --- > > > > drivers/gpu/drm/i915/intel_engine_cs.c | 2 ++ > > > > 1 file changed, 2 insertions(+) > > > > > > > > diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c > > > > index a238304..8df53ae 100644 > > > > --- a/drivers/gpu/drm/i915/intel_engine_cs.c > > > > +++ b/drivers/gpu/drm/i915/intel_engine_cs.c > > > > @@ -89,6 +89,8 @@ intel_engine_setup(struct drm_i915_private *dev_priv, > > > > const struct engine_info *info = &intel_engines[id]; > > > > struct intel_engine_cs *engine; > > > > > > > > + BUILD_BUG_ON(ARRAY_SIZE(intel_engines) != I915_NUM_ENGINES); > > > > > > For some reason I feel this is too strict. ;) > > > > It has to be strict to be useful. > > But is pointless if it doesn't apply to gen+1, or tomorrow's packing of > sparse engines, which is where Tvrtko is coming from. But it applies. It shall still work as each ring bit used by HAS_ENGINE() will likely still represent single entry in dev_priv->engine[]. As design assumes strong relation between intel_engines[] and dev_priv->engine[] arrays, we should have some way to validate correctness of that assumption. Note that this check should help us catch any mistakes that Tvrtko introduces ;) Unless, there will be full redesign. -Michal ps. I'm coming from almost the same place as Tvrtko ;)
diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c index a238304..8df53ae 100644 --- a/drivers/gpu/drm/i915/intel_engine_cs.c +++ b/drivers/gpu/drm/i915/intel_engine_cs.c @@ -89,6 +89,8 @@ intel_engine_setup(struct drm_i915_private *dev_priv, const struct engine_info *info = &intel_engines[id]; struct intel_engine_cs *engine; + BUILD_BUG_ON(ARRAY_SIZE(intel_engines) != I915_NUM_ENGINES); + GEM_BUG_ON(id < 0 || id >= I915_NUM_ENGINES); GEM_BUG_ON(dev_priv->engine[id]); engine = kzalloc(sizeof(*engine), GFP_KERNEL); if (!engine)
Additionally use runtime check to catch invalid engine indices. Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com> Cc: Jani Nikula <jani.nikula@intel.com> --- drivers/gpu/drm/i915/intel_engine_cs.c | 2 ++ 1 file changed, 2 insertions(+)