| Message ID | 1456850039-25856-8-git-send-email-david.s.gordon@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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@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 Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@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); >
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);
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@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 | 27 ++++++++++++++++++++++++--- 3 files changed, 29 insertions(+), 8 deletions(-)