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

[Tvrtko Ursulin]

On 01/03/16 16:33, 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.
>
> v6: removed duplicate BUILD_BUG_ON_MSG(); avoided renaming functions
>      by shadowing them with #defines and then calling the function
>      (non-recursively!) from inside the #define [Chris Wilson]
>
> 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

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin at intel.com>

Daniel, there are two DRM core patches in this series and only thing 
missing is convincing Chris that 6/7 does bring some improvement, 
especially looking forward to following GuC refactoring it will enable.

Assuming that gets resolved, I assume because of the core DRM bits I 
will need to ping you to pickup the series?

Regards,

Tvrtko

> ---
>   drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c |  2 +-
>   drivers/gpu/drm/i915/i915_gem_execbuffer.c   |  8 ++++----
>   include/drm/drm_mem_util.h                   | 27 ++++++++++++++++++++++++---
>   3 files changed, 29 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));
>   	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 f734b3c..1a136d9 100644
> --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> @@ -1686,8 +1686,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);
> @@ -1775,8 +1775,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..5b0111c 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,7 +61,14 @@ 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)
>   {
>   	if (size != 0 && nmemb > SIZE_MAX / size)
>   		return NULL;
> @@ -73,6 +87,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);
>