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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
