| Message ID | 20170423214030.14854-1-christophe.jaillet@wanadoo.fr (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Christophe, On Sun, Apr 23, 2017 at 11:40:30PM +0200, Christophe JAILLET wrote: > 'call_ptr_memop' can return NULL, so we must test its return value with > 'IS_ERR_OR_NULL'. Otherwise, the test 'if (mem_priv)' is meaningless. > > Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr> > --- > Note that error checking after 'call_ptr_memop' calls is not consistent > in this file. I guess that 'IS_ERR_OR_NULL' should be used everywhere > and that the corresponding error handling code should be tweaked just as > the code in this function. > --- > drivers/media/v4l2-core/videobuf2-core.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c > index c0175ea7e7ad..d1d3f5dd57b9 100644 > --- a/drivers/media/v4l2-core/videobuf2-core.c > +++ b/drivers/media/v4l2-core/videobuf2-core.c > @@ -210,7 +210,7 @@ static int __vb2_buf_mem_alloc(struct vb2_buffer *vb) > mem_priv = call_ptr_memop(vb, alloc, > q->alloc_devs[plane] ? : q->dev, > q->dma_attrs, size, dma_dir, q->gfp_flags); > - if (IS_ERR(mem_priv)) { > + if (IS_ERR_OR_NULL(mem_priv)) { > if (mem_priv) > ret = PTR_ERR(mem_priv); > goto free; If NULL will always equate -ENOMEM, shouldn't call_ptr_memop() be changed instead to convert NULL to ERR_PTR(-ENOMEM)?
Le 24/04/2017 à 16:23, Sakari Ailus a écrit : > Hi Christophe, > > On Sun, Apr 23, 2017 at 11:40:30PM +0200, Christophe JAILLET wrote: >> 'call_ptr_memop' can return NULL, so we must test its return value with >> 'IS_ERR_OR_NULL'. Otherwise, the test 'if (mem_priv)' is meaningless. >> >> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr> >> --- >> Note that error checking after 'call_ptr_memop' calls is not consistent >> in this file. I guess that 'IS_ERR_OR_NULL' should be used everywhere >> and that the corresponding error handling code should be tweaked just as >> the code in this function. >> --- >> drivers/media/v4l2-core/videobuf2-core.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c >> index c0175ea7e7ad..d1d3f5dd57b9 100644 >> --- a/drivers/media/v4l2-core/videobuf2-core.c >> +++ b/drivers/media/v4l2-core/videobuf2-core.c >> @@ -210,7 +210,7 @@ static int __vb2_buf_mem_alloc(struct vb2_buffer *vb) >> mem_priv = call_ptr_memop(vb, alloc, >> q->alloc_devs[plane] ? : q->dev, >> q->dma_attrs, size, dma_dir, q->gfp_flags); >> - if (IS_ERR(mem_priv)) { >> + if (IS_ERR_OR_NULL(mem_priv)) { >> if (mem_priv) >> ret = PTR_ERR(mem_priv); >> goto free; > If NULL will always equate -ENOMEM, shouldn't call_ptr_memop() be changed > instead to convert NULL to ERR_PTR(-ENOMEM)? > I agree with you, but in fact, I don't know if "NULL will always equate -ENOMEM" The return value of 'call_ptr_memop' is likely the result of a function called via a function pointer. I don't know if this function can return NULL or not. I don't know the code enough to see if it would be safe and if this assertion is correct. So the easiest for me is to just propose a fix to accept NULL. *** Digging deeper *** In 'videobuf2-core.h', there is: /** * struct vb2_mem_ops - memory handling/memory allocator operations * @alloc: allocate video memory and, optionally, allocator private data, * return ERR_PTR() on failure or a pointer to allocator private, * per-buffer data on success; the returned private structure * will then be passed as @buf_priv argument to other ops in this * structure. Additional gfp_flags to use when allocating the * are also passed to this operation. These flags are from the * gfp_flags field of vb2_queue. So the 'alloc' function should return an ERR_PTR in case of error. 'vb2_vmalloc_alloc', 'vb2_dc_alloc' and 'vb2_dma_sg_alloc' does. (confirmed by code inspection, based on the 'standard' list of alloc functions found in see https://lwn.net/Articles/447435/) But what if a module implements its own set of functions and returns NULL in such a case? Anyway, I will propose a patch that returns ERR_PTR(-ENOMEM) instead of NULL, but I won't be able to test it on my own. Thanks, CJ
Hi Christophe, On Mon, Apr 24, 2017 at 10:25:18PM +0200, Christophe JAILLET wrote: > Le 24/04/2017 à 16:23, Sakari Ailus a écrit : > >Hi Christophe, > > > >On Sun, Apr 23, 2017 at 11:40:30PM +0200, Christophe JAILLET wrote: > >>'call_ptr_memop' can return NULL, so we must test its return value with > >>'IS_ERR_OR_NULL'. Otherwise, the test 'if (mem_priv)' is meaningless. > >> > >>Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr> > >>--- > >>Note that error checking after 'call_ptr_memop' calls is not consistent > >>in this file. I guess that 'IS_ERR_OR_NULL' should be used everywhere > >>and that the corresponding error handling code should be tweaked just as > >>the code in this function. > >>--- > >> drivers/media/v4l2-core/videobuf2-core.c | 2 +- > >> 1 file changed, 1 insertion(+), 1 deletion(-) > >> > >>diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c > >>index c0175ea7e7ad..d1d3f5dd57b9 100644 > >>--- a/drivers/media/v4l2-core/videobuf2-core.c > >>+++ b/drivers/media/v4l2-core/videobuf2-core.c > >>@@ -210,7 +210,7 @@ static int __vb2_buf_mem_alloc(struct vb2_buffer *vb) > >> mem_priv = call_ptr_memop(vb, alloc, > >> q->alloc_devs[plane] ? : q->dev, > >> q->dma_attrs, size, dma_dir, q->gfp_flags); > >>- if (IS_ERR(mem_priv)) { > >>+ if (IS_ERR_OR_NULL(mem_priv)) { > >> if (mem_priv) > >> ret = PTR_ERR(mem_priv); > >> goto free; > >If NULL will always equate -ENOMEM, shouldn't call_ptr_memop() be changed > >instead to convert NULL to ERR_PTR(-ENOMEM)? > > > I agree with you, but in fact, I don't know if "NULL will always equate > -ENOMEM" > > The return value of 'call_ptr_memop' is likely the result of a function > called via a function pointer. I don't know if this function can return NULL > or not. > I don't know the code enough to see if it would be safe and if this > assertion is correct. > > So the easiest for me is to just propose a fix to accept NULL. Quite right. There actually seem to be a few callers that need NULL, e.g. vb2_plane_vaddr(). Looking at the definition of call_ptr_memop(): #define call_ptr_memop(vb, op, args...) \ ((vb)->vb2_queue->mem_ops->op ? \ (vb)->vb2_queue->mem_ops->op(args) : NULL) strongly suggests that the callers should expect that NULL is a possible return value. I'd be a little surprised if that was an actual case right now: it would require one of the ops not to be defined for a memtype. That said, surprising things do happen as demonstrated by your previous patch.
diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c index c0175ea7e7ad..d1d3f5dd57b9 100644 --- a/drivers/media/v4l2-core/videobuf2-core.c +++ b/drivers/media/v4l2-core/videobuf2-core.c @@ -210,7 +210,7 @@ static int __vb2_buf_mem_alloc(struct vb2_buffer *vb) mem_priv = call_ptr_memop(vb, alloc, q->alloc_devs[plane] ? : q->dev, q->dma_attrs, size, dma_dir, q->gfp_flags); - if (IS_ERR(mem_priv)) { + if (IS_ERR_OR_NULL(mem_priv)) { if (mem_priv) ret = PTR_ERR(mem_priv); goto free;
'call_ptr_memop' can return NULL, so we must test its return value with 'IS_ERR_OR_NULL'. Otherwise, the test 'if (mem_priv)' is meaningless. Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr> --- Note that error checking after 'call_ptr_memop' calls is not consistent in this file. I guess that 'IS_ERR_OR_NULL' should be used everywhere and that the corresponding error handling code should be tweaked just as the code in this function. --- drivers/media/v4l2-core/videobuf2-core.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)