Re: [GIT PULL FOR v4.18] R-Car VSP1 TLB optimisation

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

Hi Mauro and Dan,

Dan, there's a question for you below.

On Monday, 28 May 2018 11:28:41 EEST Laurent Pinchart wrote:
> On Saturday, 26 May 2018 14:28:18 EEST Mauro Carvalho Chehab wrote:
> > Em Sat, 26 May 2018 03:24:00 +0300 Laurent Pinchart escreveu:
> [snip]
> 
> > > I've reproduced the issue and created a minimal test case.
> > > 
> > >  1. struct vsp1_pipeline;
> > >  2.
> > >  3. struct vsp1_entity {
> > >  4.         struct vsp1_pipeline *pipe;
> > >  5.         struct vsp1_entity *sink;
> > >  6.         unsigned int source_pad;
> > >  7. };
> > >  8.
> > >  9. struct vsp1_pipeline {
> > > 10.         struct vsp1_entity *brx;
> > > 11. };
> > > 12.
> > > 13. struct vsp1_brx {
> > > 14.         struct vsp1_entity entity;
> > > 15. };
> > > 16.
> > > 17. struct vsp1_device {
> > > 18.         struct vsp1_brx *bru;
> > > 19.         struct vsp1_brx *brs;
> > > 20. };
> > > 21.
> > > 22. unsigned int frob(struct vsp1_device *vsp1, struct vsp1_pipeline
> > > *pipe)
> > > 23. {
> > > 24.         struct vsp1_entity *brx;
> > > 25.
> > > 26.         if (pipe->brx)
> > > 27.                 brx = pipe->brx;
> > > 28.         else if (!vsp1->bru->entity.pipe)
> > > 29.                 brx = &vsp1->bru->entity;
> > > 30.         else
> > > 31.                 brx = &vsp1->brs->entity;
> > > 32.
> > > 33.         if (brx != pipe->brx)
> > > 34.                 pipe->brx = brx;
> > > 35.
> > > 36.         return pipe->brx->source_pad;
> > > 37. }
> > > 
> > > The reason why smatch complains is that it has no guarantee that
> > > vsp1->brs is not NULL. It's quite tricky:
> > > 
> > > - On line 26, smatch assumes that pipe->brx can be NULL
> > > - On line 27, brx is assigned a non-NULL value (as pipe->brx is not NULL
> > > due to line 26)
> > > - On line 28, smatch assumes that vsp1->bru is not NULL
> > > - On line 29, brx is assigned a non-NULL value (as vsp1->bru is not NULL
> > > due to line 28)
> > > - On line 31, brx is assigned a possibly NULL value (as there's no
> > > information regarding vsp1->brs)
> > > - On line 34, pipe->brx is not assigned a non-NULL value if brx is NULL
> > > - On line 36 pipe->brx is dereferenced
> > > 
> > > The problem comes from the fact that smatch assumes that vsp1->brs isn't
> > > NULL. Adding a "(void)vsp1->brs->entity;" statement on line 25 makes the
> > > warning disappear.
> > > 
> > > So how do we know that vsp1->brs isn't NULL in the original code ?
> > > 
> > >         if (pipe->num_inputs > 2)
> > >                 brx = &vsp1->bru->entity;
> > >         else if (pipe->brx && !drm_pipe->force_brx_release)
> > >                 brx = pipe->brx;
> > >         else if (!vsp1->bru->entity.pipe)
> > >                 brx = &vsp1->bru->entity;
> > >         else
> > >                 brx = &vsp1->brs->entity;
> > > 
> > > A VSP1 instance can have no brs, so in general vsp1->brs can be NULL.
> > > However, when that's the case, the following conditions are fulfilled.
> > > 
> > > - drm_pipe->force_brx_release will be false
> > > - either pipe->brx will be non-NULL, or vsp1->bru->entity.pipe will be
> > > NULL
> > > 
> > > The fourth branch should thus never be taken.
> > 
> > I don't think that adding a forth branch there would solve.
> > 
> > The thing is that Smatch knows that pipe->brx can be NULL, as the function
> > explicly checks if pipe->brx != NULL.
> > 
> > When Smatch handles this if:
> > 	if (brx != pipe->brx) {
> > 
> > It wrongly assumes that this could be false if pipe->brx is NULL.
> > I don't know why, as Smatch should know that brx can't be NULL.
> 
> brx can be NULL here if an only if vsp1->brs is NULL (as the entity field is
> first in the vsp1->brs structure, so &vsp1->brs->entity has the same
> address as vsp1->brs).
> 
> vsp1->brs can be NULL on some devices, but in that case we have the
> following guarantees:
> 
> - drm_pipe->force_brx_release will always be FALSE
> - either pipe->brx will be non-NULL or vsp1->bru->entity.pipe will be NULL
> 
> So the fourth branch is never taken.
> 
> The above conditions come from outside this function, and smatch can't know
> about them. However, I don't know whether the problems comes from smatch
> assuming that vsp1->brs can be NULL, or from somewhere else.
> 
> > On such case, the next code to be executed would be:
> > 	format.pad = pipe->brx->source_pad;
> > 
> > With would be trying to de-ref a NULL pointer.
> > 
> > There are two ways to fix it:
> > 
> > 1) with my patch.
> > 
> > It is based to the fact that, if pipe->brx is null, then brx won't be
> > NULL. So, the logic that "Switch BRx if needed." will always be called:
> > 
> > diff --git a/drivers/media/platform/vsp1/vsp1_drm.c
> > b/drivers/media/platform/vsp1/vsp1_drm.c index 095dc48aa25a..cb6b60843400
> > 100644
> > --- a/drivers/media/platform/vsp1/vsp1_drm.c
> > +++ b/drivers/media/platform/vsp1/vsp1_drm.c
> > @@ -185,7 +185,7 @@ static int vsp1_du_pipeline_setup_brx(struct
> > vsp1_device *vsp1,
> >  	brx = &vsp1->brs->entity;
> > 
> >  	/* Switch BRx if needed. */
> > -	if (brx != pipe->brx) {
> > +	if (brx != pipe->brx || !pipe->brx) {
> >  		struct vsp1_entity *released_brx = NULL;
> >  		
> >  		/* Release our BRx if we have one. */
> > 
> > The code with switches BRx ensures that pipe->brx won't be null, as
> > 
> > in the end, it sets:
> > 	pipe->brx = brx;
> > 
> > And brx can't be NULL.
> 
> The reason I don't like this is because the problem originally comes from
> the fact that smatch assumes that vsp1->brs can be NULL when it can't. I'd
> rather modify the code in a way that explicitly tests for vsp1->brs.
> However, smatch won't accept that happily :-/ I tried
> 
>         if (pipe->num_inputs > 2)
>                 brx = &vsp1->bru->entity;
>         else if (pipe->brx && !drm_pipe->force_brx_release)
>                 brx = pipe->brx;
>         else if (!vsp1->bru->entity.pipe)
>                 brx = &vsp1->bru->entity;
>         else if (vsp1->brs)
>                 brx = &vsp1->brs->entity;
>         else
>                 return -EINVAL;
> 
> and I still get the same warning. I had to write the following (which is
> obviously not correct) to silence the warning.
> 
>         if (pipe->num_inputs > 2)
>                 brx = &vsp1->bru->entity;
>         else if (pipe->brx)
>                 brx = pipe->brx;
>         else if (!vsp1->bru->entity.pipe)
>                 brx = &vsp1->bru->entity;
>         else {
>                 (void)vsp1->brs->entity;
>                 brx = &vsp1->brs->entity;
>         }
> 
> Both the (void)vsp1->brs->entity and the removal of the !drm_pipe-
> 
> >force_brx_release were needed, any of those on its own didn't fix the
> 
> problem.
> 
> > From my PoV, this patch has the advantage of explicitly showing
> > to humans that the code inside the if statement will always be
> > executed when pipe->brx is NULL.
> > 
> > -
> > 
> > Another way to solve would be to explicitly check if pipe->brx is still
> > null before de-referencing:
> > 
> > diff --git a/drivers/media/platform/vsp1/vsp1_drm.c
> > b/drivers/media/platform/vsp1/vsp1_drm.c index edb35a5c57ea..9fe063d6df31
> > 100644
> > --- a/drivers/media/platform/vsp1/vsp1_drm.c
> > +++ b/drivers/media/platform/vsp1/vsp1_drm.c
> > @@ -327,6 +327,9 @@ static int vsp1_du_pipeline_setup_brx(struct
> > vsp1_device *vsp1,
> >  		list_add_tail(&pipe->brx->list_pipe, &pipe->entities);
> >  	}
> > 
> > +	if (!pipe->brx)
> > +		return -EINVAL;
> > +
> >  	/*
> >  	 * Configure the format on the BRx source and verify that it matches
> >  	 the
> >  	 * requested format. We don't set the media bus code as it is
> >  	 configured
> > 
> > The right fix would be, instead, to fix Smatch to handle the:
> > 	if (brx != pipe->brx)
> > 
> > for the cases where one var can be NULL while the other can't be NULL,
> > but, as I said before, I suspect that this can be a way more complex.
> 
> I'm not sure smatch is faulty here, or at least not when it interprets the
> brx != pipe->brx check. The problem seems to come from the fact that is
> believes brx can be NULL.

And that being said, I just tried

        if (pipe->num_inputs > 2)
                brx = &vsp1->bru->entity;
        else if (pipe->brx && !drm_pipe->force_brx_release)
                brx = pipe->brx;
        else if (!vsp1->bru->entity.pipe)
                brx = &vsp1->bru->entity;
        else
                brx = &vsp1->brs->entity;

        if (!brx)
                return -EINVAL;

and that didn't help either... Dan, would you have some light to shed on this 
problem ?

-- 
Regards,

Laurent Pinchart