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

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

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@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>Cc: dri-
> Cc: dri-devel@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.

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.

And this use of array allocator in Etnaviv is strange since they are 
allocating an unstructured buffer, but whatever, it existing and 
external code. (I am not even sure would I bother touching it, since, is 
the logical view to allocate *one* _buffer_ of a specified size, or 
specified size of bytes make a buffer? :) )

>   	cmdbuf = etnaviv_gpu_cmdbuf_new(gpu, ALIGN(args->stream_size, 8) + 8,
>   					args->nr_bos);
>   	if (!bos || !relocs || !stream || !cmdbuf) {
> diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> index 18a5dcc..865876d 100644
> --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> @@ -1687,8 +1687,8 @@ static bool only_mappable_for_reloc(unsigned int flags)
>   	}
>
>   	/* Copy in the exec list from userland */
> -	exec_list = drm_malloc_ab(sizeof(*exec_list), args->buffer_count);
> -	exec2_list = drm_malloc_ab(sizeof(*exec2_list), args->buffer_count);
> +	exec_list = drm_malloc_ab(args->buffer_count, sizeof(*exec_list));
> +	exec2_list = drm_malloc_ab(args->buffer_count, sizeof(*exec2_list));
>   	if (exec_list == NULL || exec2_list == NULL) {
>   		DRM_DEBUG("Failed to allocate exec list for %d buffers\n",
>   			  args->buffer_count);
> @@ -1776,8 +1776,8 @@ static bool only_mappable_for_reloc(unsigned int flags)
>   		return -EINVAL;
>   	}
>
> -	exec2_list = drm_malloc_gfp(sizeof(*exec2_list),
> -				    args->buffer_count,
> +	exec2_list = drm_malloc_gfp(args->buffer_count,
> +				    sizeof(*exec2_list),
>   				    GFP_TEMPORARY);
>   	if (exec2_list == NULL) {
>   		DRM_DEBUG("Failed to allocate exec list for %d buffers\n",
> diff --git a/include/drm/drm_mem_util.h b/include/drm/drm_mem_util.h
> index 741ce75..886ff0a 100644
> --- a/include/drm/drm_mem_util.h
> +++ b/include/drm/drm_mem_util.h
> @@ -29,7 +29,7 @@
>
>   #include <linux/vmalloc.h>
>
> -static __inline__ void *drm_calloc_large(size_t nmemb, size_t size)
> +static __inline__ void *__drm_calloc_large(size_t nmemb, const size_t size)
>   {
>   	if (size != 0 && nmemb > SIZE_MAX / size)
>   		return NULL;
> @@ -41,8 +41,15 @@ static __inline__ void *drm_calloc_large(size_t nmemb, size_t size)
>   			 GFP_KERNEL | __GFP_HIGHMEM | __GFP_ZERO, PAGE_KERNEL);
>   }
>
> +#define	drm_calloc_large(nmemb, size)					\
> +({									\
> +	BUILD_BUG_ON_MSG(!__builtin_constant_p(size),			\
> +		"Non-constant 'size' - check argument ordering?");	\
> +	__drm_calloc_large(nmemb, size);				\
> +})
> +
>   /* Modeled after cairo's malloc_ab, it's like calloc but without the zeroing. */
> -static __inline__ void *drm_malloc_ab(size_t nmemb, size_t size)
> +static __inline__ void *__drm_malloc_ab(size_t nmemb, const size_t size)
>   {
>   	if (size != 0 && nmemb > SIZE_MAX / size)
>   		return NULL;
> @@ -54,8 +61,18 @@ static __inline__ void *drm_malloc_ab(size_t nmemb, size_t size)
>   			 GFP_KERNEL | __GFP_HIGHMEM, PAGE_KERNEL);
>   }
>
> -static __inline__ void *drm_malloc_gfp(size_t nmemb, size_t size, gfp_t gfp)
> +#define	drm_malloc_ab(nmemb, size)					\
> +({									\
> +	BUILD_BUG_ON_MSG(!__builtin_constant_p(size),			\
> +		"Non-constant 'size' - check argument ordering?");	\
> +	__drm_malloc_ab(nmemb, size);					\
> +})
> +
> +static __inline__ void *__drm_malloc_gfp(size_t nmemb, const size_t size, gfp_t gfp)
>   {
> +	BUILD_BUG_ON_MSG(!__builtin_constant_p(size),
> +		"Non-constant 'size' - check argument ordering?");
> +
>   	if (size != 0 && nmemb > SIZE_MAX / size)
>   		return NULL;
>
> @@ -73,6 +90,13 @@ static __inline__ void *drm_malloc_gfp(size_t nmemb, size_t size, gfp_t gfp)
>   			 gfp | __GFP_HIGHMEM, PAGE_KERNEL);
>   }
>
> +#define	drm_malloc_gfp(nmemb, size, gfp)				\
> +({									\
> +	BUILD_BUG_ON_MSG(!__builtin_constant_p(size),			\
> +		"Non-constant 'size' - check argument ordering?");	\
> +	__drm_malloc_gfp(nmemb, size, gfp);				\
> +})
> +
>   static __inline void drm_free_large(void *ptr)
>   {
>   	kvfree(ptr);
>

i915 cleanups are good but I am unsure of whether it is good to add this 
constant constraints. All current code seems to use it like that, true, 
but I am not sure that it should be a requirement.

Regards,

Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx