* [PATCH 2/2] [media] vb2: Fix error handling in '__vb2_buf_mem_alloc'
@ 2017-04-23 21:40 Christophe JAILLET
2017-04-24 14:23 ` Sakari Ailus
0 siblings, 1 reply; 4+ messages in thread
From: Christophe JAILLET @ 2017-04-23 21:40 UTC (permalink / raw)
To: pawel, m.szyprowski, kyungmin.park, mchehab
Cc: linux-media, linux-kernel, kernel-janitors, Christophe JAILLET
'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;
--
2.11.0
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH 2/2] [media] vb2: Fix error handling in '__vb2_buf_mem_alloc'
2017-04-23 21:40 [PATCH 2/2] [media] vb2: Fix error handling in '__vb2_buf_mem_alloc' Christophe JAILLET
@ 2017-04-24 14:23 ` Sakari Ailus
2017-04-24 20:25 ` Christophe JAILLET
0 siblings, 1 reply; 4+ messages in thread
From: Sakari Ailus @ 2017-04-24 14:23 UTC (permalink / raw)
To: Christophe JAILLET
Cc: pawel, m.szyprowski, kyungmin.park, mchehab, linux-media,
linux-kernel, kernel-janitors
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)?
--
Regards,
Sakari Ailus
e-mail: sakari.ailus@iki.fi XMPP: sailus@retiisi.org.uk
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH 2/2] [media] vb2: Fix error handling in '__vb2_buf_mem_alloc'
2017-04-24 14:23 ` Sakari Ailus
@ 2017-04-24 20:25 ` Christophe JAILLET
2017-04-24 20:39 ` Sakari Ailus
0 siblings, 1 reply; 4+ messages in thread
From: Christophe JAILLET @ 2017-04-24 20:25 UTC (permalink / raw)
To: Sakari Ailus
Cc: pawel, m.szyprowski, kyungmin.park, mchehab, linux-media,
linux-kernel, kernel-janitors
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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH 2/2] [media] vb2: Fix error handling in '__vb2_buf_mem_alloc'
2017-04-24 20:25 ` Christophe JAILLET
@ 2017-04-24 20:39 ` Sakari Ailus
0 siblings, 0 replies; 4+ messages in thread
From: Sakari Ailus @ 2017-04-24 20:39 UTC (permalink / raw)
To: Christophe JAILLET
Cc: pawel, m.szyprowski, kyungmin.park, mchehab, linux-media,
linux-kernel, kernel-janitors
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.
--
Kind regards,
Sakari Ailus
e-mail: sakari.ailus@iki.fi XMPP: sailus@retiisi.org.uk
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2017-04-24 20:40 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-04-23 21:40 [PATCH 2/2] [media] vb2: Fix error handling in '__vb2_buf_mem_alloc' Christophe JAILLET
2017-04-24 14:23 ` Sakari Ailus
2017-04-24 20:25 ` Christophe JAILLET
2017-04-24 20:39 ` Sakari Ailus
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).