Re: [REVIEWv2 PATCH for v3.15 2/4] videobuf2-core: fix sparse errors.

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Hans,

On Monday 17 March 2014 14:30:51 Hans Verkuil wrote:
> On 03/17/2014 02:27 PM, Laurent Pinchart wrote:
> > On Monday 17 March 2014 13:47:35 Hans Verkuil wrote:
> >> On 03/17/2014 01:41 PM, Laurent Pinchart wrote:
> >>> On Monday 17 March 2014 13:32:44 Hans Verkuil wrote:
> >>>> On 03/17/2014 01:26 PM, Laurent Pinchart wrote:
> >>>>> On Monday 17 March 2014 11:58:08 Hans Verkuil wrote:
> >>>>>> (Fixed typo pointed out by Pawel, but more importantly made an
> >>>>>> additional change to __qbuf_dmabuf. See last paragraph in the commit
> >>>>>> log)
> >>>>> 
> >>>>> [snip]
> >>>>> 
> >>>>>> I made one other change: in __qbuf_dmabuf the result of the memop
> >>>>>> call attach_dmabuf() is checked by IS_ERR() instead of
> >>>>>> IS_ERR_OR_NULL(). Since the call_ptr_memop macro checks for
> >>>>>> IS_ERR_OR_NULL and since a NULL pointer makes no sense anyway, I've
> >>>>>> changed the IS_ERR to IS_ERR_OR_NULL to remain consistent, both with
> >>>>>> the call_ptr_memop macro, but also with all other cases where a
> >>>>>> pointer is checked.
> >>>>> 
> >>>>> Could you please split this to a separate patch ?
> >>>>> 
> >>>>>> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> >>>>>> ---
> >>>>>> 
> >>>>>>  drivers/media/v4l2-core/videobuf2-core.c | 215 ++++++++++++---------
> >>>>>>  1 file changed, 132 insertions(+), 83 deletions(-)
> >>>>>> 
> >>>>>> diff --git a/drivers/media/v4l2-core/videobuf2-core.c
> >>>>>> b/drivers/media/v4l2-core/videobuf2-core.c index f9059bb..fb1ee86
> >>>>>> 100644
> >>>>>> --- a/drivers/media/v4l2-core/videobuf2-core.c
> >>>>>> +++ b/drivers/media/v4l2-core/videobuf2-core.c
> >>>>> 
> >>>>> [snip]
> >>>>> 
> >>>>>> @@ -1401,12 +1458,11 @@ static int __qbuf_dmabuf(struct vb2_buffer
> >>>>>> *vb,
> >>>>>> const struct v4l2_buffer *b) memset(&vb->v4l2_planes[plane], 0,
> >>>>>> sizeof(struct v4l2_plane));
> >>>>>>  		/* Acquire each plane's memory */
> >>>>>> -		mem_priv = call_memop(vb, attach_dmabuf, q-
>alloc_ctx[plane],
> >>>>>> +		mem_priv = call_ptr_memop(vb, attach_dmabuf, q-
>alloc_ctx[plane],
> >>>>>>  			dbuf, planes[plane].length, write);
> >>>>>> 
> >>>>>> -		if (IS_ERR(mem_priv)) {
> >>>>>> +		if (IS_ERR_OR_NULL(mem_priv)) {
> >>>>>>  			dprintk(1, "qbuf: failed to attach dmabuf\n");
> >>>>>> -			fail_memop(vb, attach_dmabuf);
> >>>>>> -			ret = PTR_ERR(mem_priv);
> >>>>>> +			ret = mem_priv ? PTR_ERR(mem_priv) : -EINVAL;
> >>>>> 
> >>>>> That gets confusing. Wouldn't it be better to switch the other memop
> >>>>> calls that return pointers to return an ERR_PTR() in error cases
> >>>>> instead of NULL ?
> >>>> 
> >>>> I don't see why it is confusing as long as everyone sticks to the same
> >>>> scheme.
> >>> 
> >>> Because that would be mixing two schemes. For one thing, the -EINVAL
> >>> error code above is arbitrary. The construct is also confusing, and it
> >>> would be easy to write
> >>> 
> >>> 	if (IS_ERR_OR_NULL(foo)) {
> >>> 	
> >>> 		...
> >>> 		ret = PTR_ERR(foo);
> >>> 		...
> >>> 
> >>> which would return success even though an error occurs. That error will
> >>> be more difficult to debug than accepting a NULL pointer by mistake,
> >>> which would result in an oops pretty soon.
> >> 
> >> I don't want an oops, I want an error. It all goes through
> >> videobuf2-core, so this should be handled there.
> > 
> > A NULL pointer returned by a memop is a bug in the videobuf2 memop
> > implementation. It's in my opinion a problem that will be caught during
> > development. We of course want to make sure it will be caught.
> 
> I disagree: a NULL pointer here could very well be caused by a rare corner
> case (e.g. out of memory or something) that is very difficult to test for
> during development. The vb2 core must be able to handle that without causing
> an oops.

We have three memops implementations, it wouldn't be difficult to make sure 
they return proper error codes.

> >>>> I actually prefer this way, since it is more robust as it will catch
> >>>> cases where the memop unintentionally returned NULL. If I would just
> >>>> check for IS_ERR, then that would be missed. Especially in a core piece
> >>>> of code like this I'd like to err on the robust side.
> >>> 
> >>> You can always add a WARN_ON(mem_priv == NULL) if you really want to
> >>> catch that.
> >>> 
> >>>>>>  			dma_buf_put(dbuf);
> >>>>>>  			goto err;
> >>>>>>  		
> >>>>>>  		}
> >> 
> >> I'm not going to make such relatively large changes for 3.15. If you want
> >> to make a patch for 3.16, that's fine.
> > 
> > Isn't Mauro's tree closed for v3.15 anyway ?
> 
> Not for fixes. I don't think we should release code with new sparse errors.
> Certainly not for core code like this.

-- 
Regards,

Laurent Pinchart