| Message ID | 20171004130839.20285-1-boris.brezillon@free-electrons.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Oct 04, 2017 at 03:08:39PM +0200, Boris Brezillon wrote: > drm_gem_cma_create() prints an error message when dma_alloc_wc() fails to > allocate the amount of memory we requested. This can lead to annoying > error messages when CMA is only one possible source of memory for the BO > allocation. > > Turn this error message into a debug one and add a __must_check specifier > to make sure all callers are checking the return value. > > Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com> Assuming gcc doesn't spot any driver that now stumbles over the __must_check: Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> Would be good to get Laurent's ack too. -Daniel > --- > Hello, > > This problem happens with the VC4 driver which can flush its internal > cache if case of CMA allocation failures. We should only complain if the > last CMA allocation fails (the one happening after all internal caches > have been flushed). > > Regards, > > Boris > --- > drivers/gpu/drm/drm_gem_cma_helper.c | 4 ++-- > include/drm/drm_gem_cma_helper.h | 4 ++-- > 2 files changed, 4 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/drm_gem_cma_helper.c b/drivers/gpu/drm/drm_gem_cma_helper.c > index 373e33f22be4..e96c95c4fd23 100644 > --- a/drivers/gpu/drm/drm_gem_cma_helper.c > +++ b/drivers/gpu/drm/drm_gem_cma_helper.c > @@ -95,7 +95,7 @@ __drm_gem_cma_create(struct drm_device *drm, size_t size) > * > * Returns: > * A struct drm_gem_cma_object * on success or an ERR_PTR()-encoded negative > - * error code on failure. > + * error code on failure. Callers must check the return value. > */ > struct drm_gem_cma_object *drm_gem_cma_create(struct drm_device *drm, > size_t size) > @@ -112,7 +112,7 @@ struct drm_gem_cma_object *drm_gem_cma_create(struct drm_device *drm, > cma_obj->vaddr = dma_alloc_wc(drm->dev, size, &cma_obj->paddr, > GFP_KERNEL | __GFP_NOWARN); > if (!cma_obj->vaddr) { > - dev_err(drm->dev, "failed to allocate buffer with size %zu\n", > + dev_dbg(drm->dev, "failed to allocate buffer with size %zu\n", > size); > ret = -ENOMEM; > goto error; > diff --git a/include/drm/drm_gem_cma_helper.h b/include/drm/drm_gem_cma_helper.h > index 58a739bf15f1..1b7d938a31a0 100644 > --- a/include/drm/drm_gem_cma_helper.h > +++ b/include/drm/drm_gem_cma_helper.h > @@ -77,8 +77,8 @@ int drm_gem_cma_dumb_create(struct drm_file *file_priv, > int drm_gem_cma_mmap(struct file *filp, struct vm_area_struct *vma); > > /* allocate physical memory */ > -struct drm_gem_cma_object *drm_gem_cma_create(struct drm_device *drm, > - size_t size); > +struct drm_gem_cma_object * > +__must_check drm_gem_cma_create(struct drm_device *drm, size_t size); > > extern const struct vm_operations_struct drm_gem_cma_vm_ops; > > -- > 2.11.0 >
Boris Brezillon <boris.brezillon@free-electrons.com> writes: > drm_gem_cma_create() prints an error message when dma_alloc_wc() fails to > allocate the amount of memory we requested. This can lead to annoying > error messages when CMA is only one possible source of memory for the BO > allocation. > > Turn this error message into a debug one and add a __must_check specifier > to make sure all callers are checking the return value. > > Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com> The __must_check seems unnecessary to me -- you're definitely going to be doing something with the return value, because otherwise why did you call the object allocate function? That said, thanks for cleaning up the error message.
On Wednesday, 2017-10-04 22:28:54 +0000, Eric Anholt wrote: > Boris Brezillon <boris.brezillon@free-electrons.com> writes: > > > drm_gem_cma_create() prints an error message when dma_alloc_wc() fails to > > allocate the amount of memory we requested. This can lead to annoying > > error messages when CMA is only one possible source of memory for the BO > > allocation. > > > > Turn this error message into a debug one and add a __must_check specifier > > to make sure all callers are checking the return value. > > > > Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com> > > The __must_check seems unnecessary to me -- you're definitely going to > be doing something with the return value, because otherwise why did you > call the object allocate function? Indeed, `__must_check` (aka `warn_unused_result`) only makes sure the return value is not discarded, which will probably always be true here. > The `warn_unused_result` attribute causes a warning to be emitted if > a caller of the function with this attribute does not use its return > value. I think we need a sparse attribute to check that the return value is IS_ERR()-checked? (not volunteering, I have no idea how to add a sparse attribute :)
On Thu, 5 Oct 2017 09:48:24 +0100 Eric Engestrom <eric.engestrom@imgtec.com> wrote: > On Wednesday, 2017-10-04 22:28:54 +0000, Eric Anholt wrote: > > Boris Brezillon <boris.brezillon@free-electrons.com> writes: > > > > > drm_gem_cma_create() prints an error message when dma_alloc_wc() fails to > > > allocate the amount of memory we requested. This can lead to annoying > > > error messages when CMA is only one possible source of memory for the BO > > > allocation. > > > > > > Turn this error message into a debug one and add a __must_check specifier > > > to make sure all callers are checking the return value. > > > > > > Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com> > > > > The __must_check seems unnecessary to me -- you're definitely going to > > be doing something with the return value, because otherwise why did you > > call the object allocate function? > > Indeed, `__must_check` (aka `warn_unused_result`) only makes sure the > return value is not discarded, which will probably always be true here. I agree, this __must_check is not really useful here. > > > The `warn_unused_result` attribute causes a warning to be emitted if > > a caller of the function with this attribute does not use its return > > value. > > I think we need a sparse attribute to check that the return value is > IS_ERR()-checked? Can we just turn the dev_err() into dev_dbg() for now? The 'force callers to check IS_ERR()' is kind of orthogonal to the change I'm doing here. > (not volunteering, I have no idea how to add a sparse attribute :)
On Thursday, 2017-10-05 10:57:47 +0200, Boris Brezillon wrote: > On Thu, 5 Oct 2017 09:48:24 +0100 > Eric Engestrom <eric.engestrom@imgtec.com> wrote: > > > On Wednesday, 2017-10-04 22:28:54 +0000, Eric Anholt wrote: > > > Boris Brezillon <boris.brezillon@free-electrons.com> writes: > > > > > > > drm_gem_cma_create() prints an error message when dma_alloc_wc() fails to > > > > allocate the amount of memory we requested. This can lead to annoying > > > > error messages when CMA is only one possible source of memory for the BO > > > > allocation. > > > > > > > > Turn this error message into a debug one and add a __must_check specifier > > > > to make sure all callers are checking the return value. > > > > > > > > Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com> > > > > > > The __must_check seems unnecessary to me -- you're definitely going to > > > be doing something with the return value, because otherwise why did you > > > call the object allocate function? > > > > Indeed, `__must_check` (aka `warn_unused_result`) only makes sure the > > return value is not discarded, which will probably always be true here. > > I agree, this __must_check is not really useful here. > > > > > > The `warn_unused_result` attribute causes a warning to be emitted if > > > a caller of the function with this attribute does not use its return > > > value. > > > > I think we need a sparse attribute to check that the return value is > > IS_ERR()-checked? > > Can we just turn the dev_err() into dev_dbg() for now? The 'force > callers to check IS_ERR()' is kind of orthogonal to the change I'm > doing here. Definitely, the function you're touching just happens to be a user of this potential new attribute. IMO just drop the header part of the change, and this is Reviewed-by: Eric Engestrom <eric.engestrom@imgtec.com> On the sparse attribute discussion, anyone knows someone with experience doing that?
diff --git a/drivers/gpu/drm/drm_gem_cma_helper.c b/drivers/gpu/drm/drm_gem_cma_helper.c index 373e33f22be4..e96c95c4fd23 100644 --- a/drivers/gpu/drm/drm_gem_cma_helper.c +++ b/drivers/gpu/drm/drm_gem_cma_helper.c @@ -95,7 +95,7 @@ __drm_gem_cma_create(struct drm_device *drm, size_t size) * * Returns: * A struct drm_gem_cma_object * on success or an ERR_PTR()-encoded negative - * error code on failure. + * error code on failure. Callers must check the return value. */ struct drm_gem_cma_object *drm_gem_cma_create(struct drm_device *drm, size_t size) @@ -112,7 +112,7 @@ struct drm_gem_cma_object *drm_gem_cma_create(struct drm_device *drm, cma_obj->vaddr = dma_alloc_wc(drm->dev, size, &cma_obj->paddr, GFP_KERNEL | __GFP_NOWARN); if (!cma_obj->vaddr) { - dev_err(drm->dev, "failed to allocate buffer with size %zu\n", + dev_dbg(drm->dev, "failed to allocate buffer with size %zu\n", size); ret = -ENOMEM; goto error; diff --git a/include/drm/drm_gem_cma_helper.h b/include/drm/drm_gem_cma_helper.h index 58a739bf15f1..1b7d938a31a0 100644 --- a/include/drm/drm_gem_cma_helper.h +++ b/include/drm/drm_gem_cma_helper.h @@ -77,8 +77,8 @@ int drm_gem_cma_dumb_create(struct drm_file *file_priv, int drm_gem_cma_mmap(struct file *filp, struct vm_area_struct *vma); /* allocate physical memory */ -struct drm_gem_cma_object *drm_gem_cma_create(struct drm_device *drm, - size_t size); +struct drm_gem_cma_object * +__must_check drm_gem_cma_create(struct drm_device *drm, size_t size); extern const struct vm_operations_struct drm_gem_cma_vm_ops;
drm_gem_cma_create() prints an error message when dma_alloc_wc() fails to allocate the amount of memory we requested. This can lead to annoying error messages when CMA is only one possible source of memory for the BO allocation. Turn this error message into a debug one and add a __must_check specifier to make sure all callers are checking the return value. Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com> --- Hello, This problem happens with the VC4 driver which can flush its internal cache if case of CMA allocation failures. We should only complain if the last CMA allocation fails (the one happening after all internal caches have been flushed). Regards, Boris --- drivers/gpu/drm/drm_gem_cma_helper.c | 4 ++-- include/drm/drm_gem_cma_helper.h | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-)