[Intel-gfx] [PATCH v5 2/7] drm: add parameter-order checking to drm memory allocators

[Emil Velikov]

 On 29 February 2016 at 16:16, Tvrtko Ursulin
<tvrtko.ursulin at linux.intel.com> wrote:
>
> On 29/02/16 11:13, Dave Gordon wrote:
>>
>> After the recent addition of drm_malloc_gfp(), it was noticed that
>> some callers of these functions has swapped the parameters in the
>> call - it's supposed to be 'number of members' and 'sizeof(element)',
>> but a few callers had got the size first and the count second. This
>> isn't otherwise detected because they're both type 'size_t', and
>> the implementation at present just multiplies them anyway, so the
>> result is still right. But some future implementation might treat
>> them differently (e.g. allowing 0 elements but not zero size), so
>> let's add some compile-time checks and complain if the second (size)
>> parameter isn't a sizeof() expression, or at least a compile-time
>> constant.
>>
>> This patch also fixes those callers where the order was wrong.
>>
>> Signed-off-by: Dave Gordon <david.s.gordon at intel.com>
>> Cc: Chris Wilson <chris at chris-wilson.co.uk>
>> Cc: Ville Syrjälä <ville.syrjala at linux.intel.com>Cc: dri-
>> Cc: dri-devel at lists.freedesktop.org
>> ---
>>   drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c |  2 +-
>>   drivers/gpu/drm/i915/i915_gem_execbuffer.c   |  8 ++++----
>>   include/drm/drm_mem_util.h                   | 30
>> +++++++++++++++++++++++++---
>>   3 files changed, 32 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c
>> b/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c
>> index 1aba01a..9ae4a71 100644
>> --- a/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c
>> +++ b/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c
>> @@ -340,7 +340,7 @@ int etnaviv_ioctl_gem_submit(struct drm_device *dev,
>> void *data,
>>          */
>>         bos = drm_malloc_ab(args->nr_bos, sizeof(*bos));
>>         relocs = drm_malloc_ab(args->nr_relocs, sizeof(*relocs));
>> -       stream = drm_malloc_ab(1, args->stream_size);
>> +       stream = drm_malloc_ab(args->stream_size, sizeof(*stream));
>
>
> I was surprised sizeof(void) == 1. On further research that seems to be an
> GCC extension.
>
Afaict pointer arithmetic (-Wpointer-arith) has/is been used in the
kernel extensively. In userspace we try to avoid it libdrm and mesa,
there might be a few cases where it's still around. No too sure about
libva{,-intel}.

> I am not sure how active projects to compile the kernel with for example ICC
> are, just remember some talks about it in the past. Maybe it is even
> possible? In that case it would be better to just leave "1" there to not
> rely on GCC extensions.
>
While I'm all for the idea, I doubt we'll get there any time soon. My
last suggestion (do not use zero sized arrays) went down in flames :-(

Regards,
Emil