[Intel-gfx] [PATCH v2] drm/i915: Use BUILD_BUG_ON to detected missing engine definitions

[Tvrtko Ursulin]

On 01/03/2017 20:07, Michal Wajdeczko wrote:
> On Wed, Mar 01, 2017 at 07:29:15PM +0000, Tvrtko Ursulin wrote:
>>
>> On 01/03/2017 17:39, Michal Wajdeczko wrote:
>>> Engine related definitions are located in different files
>>> and it is easy to break their cross dependency.
>>>
>>> Additionally use GEM_WARN_ON to catch invalid engine index.
>>>
>>> v2: compare against array size
>>>
>>> Signed-off-by: Michal Wajdeczko <michal.wajdeczko at intel.com>
>>> Cc: Jani Nikula <jani.nikula at intel.com>
>>> Cc: Joonas Lahtinen <joonas.lahtinen at linux.intel.com>
>>> Cc: Chris Wilson <chris at chris-wilson.co.uk>
>>> Cc: Tvrtko Ursulin <tvrtko.ursulin at 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..c1f58b5 100644
>>> --- a/drivers/gpu/drm/i915/intel_engine_cs.c
>>> +++ b/drivers/gpu/drm/i915/intel_engine_cs.c
>>> @@ -84,11 +84,16 @@ static const struct engine_info {
>>>
>>>  static int
>>>  intel_engine_setup(struct drm_i915_private *dev_priv,
>>> -		   enum intel_engine_id id)
>>> +		   unsigned int id)
>>>  {
>>>  	const struct engine_info *info = &intel_engines[id];
>>>  	struct intel_engine_cs *engine;
>>>
>>> +	BUILD_BUG_ON(ARRAY_SIZE(intel_engines) !=
>>> +		     ARRAY_SIZE(dev_priv->engine));
>>> +	if (GEM_WARN_ON(id >= ARRAY_SIZE(intel_engines)))
>>> +		return -EINVAL;
>>> +
>>>  	GEM_BUG_ON(dev_priv->engine[id]);
>>>  	engine = kzalloc(sizeof(*engine), GFP_KERNEL);
>>>  	if (!engine)
>>>
>>
>> I would add asserts for id >= ARRAY_SIZE(intel_engines) and id >=
>> ARRAY_SIZE(dev_priv->engine). That provides guarantees for no out of bounds
>> access within this function and should be enough for this function.
>
> True, but then we will loose early (ie. at build time) detection that our
> engine array definitions are not in sync (which was primary reason for this
> patch).
>
>>
>> The rest sounds just like trouble for now.
>
> I would not call extra check as a trouble.
> But if you prefer freedom over robustness, then I will step back.

Lets call it flexibility and pragmatism. :)

>> Also I am not sure about negative enum, do we elsewhere check for that class
>> of errors? Is it worth it? Maybe then just cast to unsigned in the above
>> mentioned asserts?
>
> Note that the caller function is not using enum at all, this id/index is defined
> there as plain "unsigned". Then it is promoted in this function only, where we
> start to use it as index again (except id assignment).

Maybe we should make the loop in intel_engines_init_early use the id 
instead of i then...

> IMHO enums are not the best choice for indexing arrays, and if you really want
> to do so, you should add some guards to prevent unexpected use.

... although it is still a local function with a single call site so 
doesn't get me too excited. But since I'm usually all for data type tidy 
so feel free to do it.

> Using cast in these two asserts will do the trick.
>
> Let's try this as compromise ;)

Yes I think this compromise is robust enough! ;)

Regards,

Tvrtko