[PATCH 4/4] drm/scheduler: do not keep a copy of sched list

[Nirmoy]

On 12/10/19 6:32 PM, Christian König wrote:
>
>>>> Maybe make this "num_sched_list > 1 ? sched_list : NULL" to avoid 
>>>> accidentally dereferencing a stale pointer to the stack.
>>> Do you mean "num_sched_list >= 1 ? sched_list : NULL"
>
> No, the entity->sched_list field should be NULL when num_sched_list==1.
>
> When num_sched_list==1 the entity->sched_list shouldn't be used and we 
> can use a dummy list on the stack as parameter. But we should set the 
> pointer to NULL in this case just to make sure that nobody is 
> dereferencing it.
Okay I understand now.
>
>>>>
>>>>>   -    if(!entity->sched_list)
>>>>> -        return -ENOMEM;
>>>>>         init_completion(&entity->entity_idle);
>>>>> -
>>>>> -    for (i = 0; i < num_sched_list; i++)
>>>>> -        entity->sched_list[i] = sched_list[i];
>>>>> -
>>>>>       if (num_sched_list)
>>>>
>>>> That check can actually be dropped as well. We return -EINVAL when 
>>>> the num_sched_list is zero.
>>>
>>> This  one took me some time to understand. So we don't really return 
>>> -EINVAL if num_sched_list == 0
>>>
>>> if (!(entity && sched_list && (num_sched_list == 0 || sched_list[0])))
>>>                 return -EINVAL;
>>>
>>> This is coming from below patch. Are we suppose to tolerate IPs with 
>>> uninitialized sched so that ctx creation dosn't return error ?
>
> Yes, exactly. That's intentionally this way.
>
> GPU reset sometimes resulted in schedulers being disabled because we 
> couldn't re-init them. In this case we had entities with an empty 
> scheduler list.
>
> That can't happen any more after you make the scheduler arrays 
> constant, but I would stick with that behavior.

So I will keep the check.

>
> Regards,
> Christian.
>
>
Regards,

Nirmoy