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

[Dan Carpenter]

On Sat, May 26, 2018 at 08:28:18AM -0300, Mauro Carvalho Chehab wrote:
> Em Sat, 26 May 2018 03:24:00 +0300
> Laurent Pinchart  escreveu:
> 
> > Hi Mauro,
> > 
> > On Saturday, 26 May 2018 02:39:16 EEST Laurent Pinchart wrote:
> > > On Saturday, 26 May 2018 02:10:27 EEST Mauro Carvalho Chehab wrote:  
> > > > Em Sun, 20 May 2018 15:10:50 +0300 Laurent Pinchart escreveu:  
> > > >> Hi Mauro,
> > > >> 
> > > >> The following changes since commit
> > > >> 
> > > >> 8ed8bba70b4355b1ba029b151ade84475dd12991:
> > > >>   media: imx274: remove non-indexed pointers from mode_table 
> > > >> (2018-05-17
> > > >> 
> > > >> 06:22:08 -0400)
> > > >> 
> > > >> are available in the Git repository at:
> > > >>   git://linuxtv.org/pinchartl/media.git v4l2/vsp1/next
> > > >> 
> > > >> for you to fetch changes up to 
> > > >> 429f256501652c90a4ed82f2416618f82a77d37c:
> > > >>   media: vsp1: Move video configuration to a cached dlb (2018-05-20
> > > >>   09:46:51 +0300)
> > > >> 
> > > >> The branch passes the VSP and DU test suites, both on its own and when
> > > >> merged with the drm-next branch.  
> > > > 
> > > > This series added a new warning:
> > > > 
> > > > drivers/media/platform/vsp1/vsp1_dl.c:69: warning: Function parameter or
> > > > member 'refcnt' not described in 'vsp1_dl_body'  
> > > 
> > > We'll fix that. Kieran, as you authored the code, would you like to give 
> > > it
> > > a go ?
> > >   
> > > > To the already existing one:
> > > > 
> > > > drivers/media/platform/vsp1/vsp1_drm.c:336 vsp1_du_pipeline_setup_brx()
> > > > error: we previously assumed 'pipe->brx' could be null (see line 244)  
> > > 
> > > That's still on my todo list. I tried to give it a go but received plenty 
> > > of
> > > SQL errors. How do you run smatch ?  
> > 
> > Nevermind, I found out what was wrong (had to specify the data directory 
> > manually).
> > 
> > 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 = >bru->entity;
> > 30. else
> > 31. brx = >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.
> > 

I will respond to the other emails in this thread.  You guys are
basically spot on.  All this analysis is 100% correct.  Btw, if you want
to see Smatch's internal state you can do:

#include "/home/whatever/smatch/check_debug.h"

else if (!vsp1->bru->entity.pipe) {
__smatch_implied(>bru->entity);

And it tells you what Smatch thinks it is at that point.  The
__smatch_about() output can also be useful.

regards,
dan carpenter

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

[Mauro Carvalho Chehab]

Em Mon, 28 May 2018 11:31:01 +0300
Laurent Pinchart  escreveu:

> 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 = >bru->entity;
> > > > 30. else
> > > > 31. brx = >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 = >bru->entity;
> > > > else if (pipe->brx && !drm_pipe->force_brx_release)
> > > > brx = pipe->brx;
> > > > else if (!vsp1->bru->entity.pipe)
> > > > brx = >bru->entity;
> > > > else
> > > > brx = >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 >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 

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

[Dan Carpenter]

On Mon, May 28, 2018 at 11:28:41AM +0300, Laurent Pinchart wrote:
> 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 = >bru->entity;
> else if (pipe->brx)
> brx = pipe->brx;
> else if (!vsp1->bru->entity.pipe)
> brx = >bru->entity;
> else { 
> (void)vsp1->brs->entity;
> brx = >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.

The problem in this case is the first "brx = >bru->entity;".
Smatch assumes >bru->entity can be == to pipe->brx and NULL.
Adding a "(void)vsp1->bru->entity;" on that path will silence the
warning (hopefully).

regards,
dan carpenter

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

[Kieran Bingham]

Hi Mauro, Laurent,

On 26/05/18 00:39, Laurent Pinchart wrote:
> Hi Mauro,
> 
> (CC'ing Kieran)
> 
> On Saturday, 26 May 2018 02:10:27 EEST Mauro Carvalho Chehab wrote:
>> Em Sun, 20 May 2018 15:10:50 +0300 Laurent Pinchart escreveu:
>>> Hi Mauro,
>>>
>>> The following changes since commit
>>> 8ed8bba70b4355b1ba029b151ade84475dd12991:
>>>   media: imx274: remove non-indexed pointers from mode_table (2018-05-17
>>> 06:22:08 -0400)
>>>
>>> are available in the Git repository at:
>>>   git://linuxtv.org/pinchartl/media.git v4l2/vsp1/next
>>>
>>> for you to fetch changes up to 429f256501652c90a4ed82f2416618f82a77d37c:
>>>   media: vsp1: Move video configuration to a cached dlb (2018-05-20
>>>   09:46:51 +0300)
>>>
>>> The branch passes the VSP and DU test suites, both on its own and when
>>> merged with the drm-next branch.
>>
>> This series added a new warning:
>>
>> drivers/media/platform/vsp1/vsp1_dl.c:69: warning: Function parameter or
>> member 'refcnt' not described in 'vsp1_dl_body'
> 
> We'll fix that. Kieran, as you authored the code, would you like to give it a 
> go ?

Sent!

Thanks for the catch.

--
Kieran



>> To the already existing one:
>>
>> drivers/media/platform/vsp1/vsp1_drm.c:336 vsp1_du_pipeline_setup_brx()
>> error: we previously assumed 'pipe->brx' could be null (see line 244)
> 
> That's still on my todo list. I tried to give it a go but received plenty of 
> SQL errors. How do you run smatch ?
> 
>> (there's also a Spectre warning too, but I'll looking into those
>> in separate).
>>
>> For now, I'll apply it, but I reserve the right of not pulling any
>> new patchsets that would add more warnings.
>>
>>> 
>>>
>>> Geert Uytterhoeven (1):
>>>   media: vsp1: Drop OF dependency of VIDEO_RENESAS_VSP1
>>>
>>> Kieran Bingham (10):
>>>   media: vsp1: Release buffers for each video node
>>>   media: vsp1: Move video suspend resume handling to video object
>>>   media: vsp1: Reword uses of 'fragment' as 'body'
>>>   media: vsp1: Protect bodies against overflow
>>>   media: vsp1: Provide a body pool
>>>   media: vsp1: Convert display lists to use new body pool
>>>   media: vsp1: Use reference counting for bodies
>>>   media: vsp1: Refactor display list configure operations
>>>   media: vsp1: Adapt entities to configure into a body
>>>   media: vsp1: Move video configuration to a cached dlb
>>>  
>>>  drivers/media/platform/Kconfig|   2 +-
>>>  drivers/media/platform/vsp1/vsp1_brx.c|  32 ++--
>>>  drivers/media/platform/vsp1/vsp1_clu.c| 113 ++-
>>>  drivers/media/platform/vsp1/vsp1_clu.h|   1 +
>>>  drivers/media/platform/vsp1/vsp1_dl.c | 388 ++---
>>>  drivers/media/platform/vsp1/vsp1_dl.h |  21 ++-
>>>  drivers/media/platform/vsp1/vsp1_drm.c|  18 +-
>>>  drivers/media/platform/vsp1/vsp1_drv.c|   4 +-
>>>  drivers/media/platform/vsp1/vsp1_entity.c |  34 +++-
>>>  drivers/media/platform/vsp1/vsp1_entity.h |  45 +++--
>>>  drivers/media/platform/vsp1/vsp1_hgo.c|  26 ++-
>>>  drivers/media/platform/vsp1/vsp1_hgt.c|  28 ++-
>>>  drivers/media/platform/vsp1/vsp1_hsit.c   |  20 +-
>>>  drivers/media/platform/vsp1/vsp1_lif.c|  25 ++-
>>>  drivers/media/platform/vsp1/vsp1_lut.c|  80 +---
>>>  drivers/media/platform/vsp1/vsp1_lut.h|   1 +
>>>  drivers/media/platform/vsp1/vsp1_pipe.c   |  74 +---
>>>  drivers/media/platform/vsp1/vsp1_pipe.h   |  12 +-
>>>  drivers/media/platform/vsp1/vsp1_rpf.c| 189 ++-
>>>  drivers/media/platform/vsp1/vsp1_sru.c|  24 +--
>>>  drivers/media/platform/vsp1/vsp1_uds.c|  73 +++
>>>  drivers/media/platform/vsp1/vsp1_uds.h|   2 +-
>>>  drivers/media/platform/vsp1/vsp1_uif.c|  35 ++--
>>>  drivers/media/platform/vsp1/vsp1_video.c  | 177 -
>>>  drivers/media/platform/vsp1/vsp1_video.h  |   3 +
>>>  drivers/media/platform/vsp1/vsp1_wpf.c| 326 ++---
>>>  26 files changed, 967 insertions(+), 786 deletions(-)
> 

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

[Dan Carpenter]

On Mon, May 28, 2018 at 11:31:01AM +0300, Laurent Pinchart wrote:
> And that being said, I just tried
> 
> if (pipe->num_inputs > 2)
> brx = >bru->entity;
> else if (pipe->brx && !drm_pipe->force_brx_release)
> brx = pipe->brx;
> else if (!vsp1->bru->entity.pipe)
> brx = >bru->entity;
> else
> brx = >brs->entity;
> 
> if (!brx)
> return -EINVAL;
> 
> and that didn't help either... Dan, would you have some light to shed on this 
> problem ?

This is a problem in Smatch.

We should be able to go backwards and say that "If we know 'brx' is
non-NULL then let's mark >brs->entity, vsp1->brs,
>bru->entity and vsp1->bru all as non-NULL as well".  But Smatch
doesn't go backwards like that.  The information is mostly there to do
it, but my instinct is that it's really hard to implement.

The other potential problem here is that Smatch stores comparisons and
values separately.  In other words smatch_comparison.c has all the
information about brx == >bru->entity and smatch_extra.c has the
information about if brx is NULL or non-NULL.  They don't really share
information very well.

regards,
dan carpenter

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

[Laurent Pinchart]

Hi Mauro,

On Monday, 28 May 2018 13:03:05 EEST Mauro Carvalho Chehab wrote:
> Em Mon, 28 May 2018 11:28:41 +0300 Laurent Pinchart escreveu:
> > 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 = >bru->entity;
> >>> 30. else
> >>> 31. brx = >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 = >bru->entity;
> >>> else if (pipe->brx && !drm_pipe->force_brx_release)
> >>> brx = pipe->brx;
> >>> else if (!vsp1->bru->entity.pipe)
> >>> brx = >bru->entity;
> >>> else
> >>> brx = >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 >brs->entity has the same
> > address as vsp1->brs).
> 
> I can't see how brx can be NULL. At the sequence of ifs:
> 
>   if (pipe->num_inputs > 2)
> brx = >bru->entity;
> else if (pipe->brx && !drm_pipe->force_brx_release)
> brx = pipe->brx;
> else if (!vsp1->bru->entity.pipe)
> brx = >bru->entity;
> else
> brx = >brs->entity;
> 
> 
> The usage of brx = &(something) will always return a non NULL value[1].

I don't agree. When vsp1->brs is NULL, >brs->entity will evaluate to 
NULL as well.

> The only clause where it doesn't use this pattern is:
> 
>   ...
>   if (pipe->brx && !drm_pipe->force_brx_release)
> brx = pipe->brx;
>   ...
> 
> This one explicitly checks if pipe->brx is NULL, so it will only
> hit on a non-NULL value. If it doesn't hit, brx will be initialized
> by a pointer too either bru or brs entity.
> 
> So, brx will always be non-NULL, even if pipe->brx is NULL.
> 
> [1] It might be doing a NULL deref - with seems to be your concern
> when you're talking about the case where vsp1->brs is NULL - but
> that's not what Smatch is complaining here.

>brs->entity will not cause a NULL dereference, it doesn't cause vsp1-
>brs to be 

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

[Laurent Pinchart]

Hi Dan,

Thank you for your quick reply.

On Monday, 28 May 2018 13:20:49 EEST Dan Carpenter wrote:
> On Mon, May 28, 2018 at 11:28:41AM +0300, Laurent Pinchart wrote:
> > 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 = >bru->entity;
> > else if (pipe->brx)
> > brx = pipe->brx;
> > else if (!vsp1->bru->entity.pipe)
> > brx = >bru->entity;
> > else {
> > (void)vsp1->brs->entity;
> > brx = >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.
> 
> The problem in this case is the first "brx = >bru->entity;".
> Smatch assumes >bru->entity can be == to pipe->brx and NULL.

Why does smatch assume that >bru->entity can be NULL, when the previous 
line dereferences vsp1->bru ?

> Adding a "(void)vsp1->bru->entity;" on that path will silence the
> warning (hopefully).

-- 
Regards,

Laurent Pinchart

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

[Dan Carpenter]

On Mon, May 28, 2018 at 07:03:05AM -0300, Mauro Carvalho Chehab wrote:
> I can't see how brx can be NULL. At the sequence of ifs:
> 
>   if (pipe->num_inputs > 2)
> brx = >bru->entity;
> else if (pipe->brx && !drm_pipe->force_brx_release)
> brx = pipe->brx;
> else if (!vsp1->bru->entity.pipe)
> brx = >bru->entity;
> else
> brx = >brs->entity;
> 
> 
> The usage of brx = &(something) will always return a non NULL
> value[1].

That's not right.  It can be NULL if it's >first_struct_member
and ->entity is the first struct member.  If it weren't the first struct
member then Smatch would say that brx was non-NULL.

> [1] It might be doing a NULL deref - with seems to be your concern
> when you're talking about the case where vsp1->brs is NULL - but 
> that's not what Smatch is complaining here.

If vsp1->bru were NULL, it wouldn't be a NULL dereference because it's
not dereferencing it; it's just taking the address.  On the path where
we do:
else if (!vsp1->bru->entity.pipe)
brx = >bru->entity;

Then Smatch sees that vsp1->bru is dereferenced and marks "brx" as
non-NULL.

regards,
dan carpenter

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

[Laurent Pinchart]

Hi Dan,

On Monday, 28 May 2018 13:36:08 EEST Dan Carpenter wrote:
> On Mon, May 28, 2018 at 11:31:01AM +0300, Laurent Pinchart wrote:
> > And that being said, I just tried
> > 
> > if (pipe->num_inputs > 2)
> > brx = >bru->entity;
> > else if (pipe->brx && !drm_pipe->force_brx_release)
> > brx = pipe->brx;
> > else if (!vsp1->bru->entity.pipe)
> > brx = >bru->entity;
> > else
> > brx = >brs->entity;
> > 
> > if (!brx)
> > return -EINVAL;
> > 
> > and that didn't help either... Dan, would you have some light to shed on
> > this problem ?
> 
> This is a problem in Smatch.
> 
> We should be able to go backwards and say that "If we know 'brx' is
> non-NULL then let's mark >brs->entity, vsp1->brs,
> >bru->entity and vsp1->bru all as non-NULL as well".  But Smatch
> doesn't go backwards like that.  The information is mostly there to do
> it, but my instinct is that it's really hard to implement.
> 
> The other potential problem here is that Smatch stores comparisons and
> values separately.  In other words smatch_comparison.c has all the
> information about brx == >bru->entity and smatch_extra.c has the
> information about if brx is NULL or non-NULL.  They don't really share
> information very well.

It would indeed be useful to implement, but I share your concern that this 
would be pretty difficult.

However, there's still something that puzzles me. Let's add a bit more 
context.

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

1.  if (!brx)
return -EINVAL;

2.  if (brx != pipe->brx) {
...
3.  pipe->brx = brx;
...
}

4.  format.pad = pipe->brx->source_pad


(1) ensures that brx can't be NULL. (2) is thus always true if pipe->brx is 
NULL. (3) then assigns a non-NULL value to pipe->brx. Smatch should thus never 
complain about (4), even if it can't backtrack.

-- 
Regards,

Laurent Pinchart

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

[Dan Carpenter]

On Mon, May 28, 2018 at 01:58:41PM +0300, Laurent Pinchart wrote:
> It would indeed be useful to implement, but I share your concern that this 
> would be pretty difficult.
> 
> However, there's still something that puzzles me. Let's add a bit more 
> context.
> 
> if (pipe->num_inputs > 2)
> brx = >bru->entity;
> else if (pipe->brx && !drm_pipe->force_brx_release)
> brx = pipe->brx;
> else if (!vsp1->bru->entity.pipe)
> brx = >bru->entity;
> else
> brx = >brs->entity;
> 
> 1.  if (!brx)
> return -EINVAL;
> 
> 2.  if (brx != pipe->brx) {
> ...
> 3.  pipe->brx = brx;
> ...
> }
> 
> 4.  format.pad = pipe->brx->source_pad
> 
> 
> (1) ensures that brx can't be NULL. (2) is thus always true if pipe->brx is 
> NULL. (3) then assigns a non-NULL value to pipe->brx. Smatch should thus 
> never 
> complain about (4), even if it can't backtrack.

Oh wow...  That's a very basic and ancient bug.  I've written a fix and
will push it out tomorrow if it passes my testing.

regards,
dan carpenter

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

[Dan Carpenter]

On Mon, May 28, 2018 at 01:54:18PM +0300, Laurent Pinchart wrote:
> Hi Dan,
> 
> Thank you for your quick reply.
> 
> On Monday, 28 May 2018 13:20:49 EEST Dan Carpenter wrote:
> > On Mon, May 28, 2018 at 11:28:41AM +0300, Laurent Pinchart wrote:
> > > 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 = >bru->entity;
> > > else if (pipe->brx)
> > > brx = pipe->brx;
> > > else if (!vsp1->bru->entity.pipe)
> > > brx = >bru->entity;
> > > else {
> > > (void)vsp1->brs->entity;
> > > brx = >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.
> > 
> > The problem in this case is the first "brx = >bru->entity;".
> > Smatch assumes >bru->entity can be == to pipe->brx and NULL.
> 
> Why does smatch assume that >bru->entity can be NULL, when the previous 
> line dereferences vsp1->bru ?

I'm talking about when pipe->num_inputs > 2.  For the second
"brx = >bru->entity;" assignment, then Smatch parses it correctly
as you say.

regards,
dan carpenter

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

[Dan Carpenter]

On Mon, May 28, 2018 at 07:17:54AM -0300, Mauro Carvalho Chehab wrote:
> This (obviously wrong patch) shut up the warning:
> 
> --- a/drivers/media/platform/vsp1/vsp1_drm.c
> +++ b/drivers/media/platform/vsp1/vsp1_drm.c
> @@ -248,6 +248,9 @@ static int vsp1_du_pipeline_setup_brx(struct vsp1_device 
> *vsp1,
> else
> brx = >brs->entity;
>  
> +   if (pipe->brx == brx)
> +   pipe->brx = >brs->entity;
> +
> /* Switch BRx if needed. */
> if (brx != pipe->brx) {
> struct vsp1_entity *released_brx = NULL;
> 

Just to be clear.  After this patch, Smatch does *NOT* think that
"pipe->brx" is necessarily non-NULL.  What this patch does it that
Smatch says "pipe->brx has been modified on every code path since we
checked for NULL, so forget about the earlier check".

regards,
dan carpenter

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

[Mauro Carvalho Chehab]

Em Mon, 28 May 2018 11:28:41 +0300
Laurent Pinchart  escreveu:

> Hi Mauro,
> 
> 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 = >bru->entity;
> > > 30. else
> > > 31. brx = >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 = >bru->entity;
> > > else if (pipe->brx && !drm_pipe->force_brx_release)
> > > brx = pipe->brx;
> > > else if (!vsp1->bru->entity.pipe)
> > > brx = >bru->entity;
> > > else
> > > brx = >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 >brs->entity has the same address 
> as vsp1->brs).

I can't see how brx can be NULL. At the sequence of ifs:

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


The usage of brx = &(something) will always return a non NULL
value[1]. The only clause where it doesn't use this pattern is:

...
if (pipe->brx && !drm_pipe->force_brx_release)
brx = pipe->brx;
...

This one explicitly checks if pipe->brx is NULL, so it will only
hit on a non-NULL value. If it doesn't hit, brx will be initialized
by a pointer too either bru or brs entity.

So, brx will always be non-NULL, even if pipe->brx is NULL.

[1] It might be doing a NULL deref - with seems to be your concern
when you're talking about the case where vsp1->brs is NULL - but 
that's not what Smatch is complaining here.

> 
> 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 

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

[Laurent Pinchart]

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 = >bru->entity;
> > > 30. else
> > > 31. brx = >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 = >bru->entity;
> > > else if (pipe->brx && !drm_pipe->force_brx_release)
> > > brx = pipe->brx;
> > > else if (!vsp1->bru->entity.pipe)
> > > brx = >bru->entity;
> > > else
> > > brx = >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 >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 = >brs->entity;
> > 
> > /* Switch BRx if needed. */
> > -   

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

[Laurent Pinchart]

Hi Mauro,

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 = >bru->entity;
> > 30. else
> > 31. brx = >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 = >bru->entity;
> > else if (pipe->brx && !drm_pipe->force_brx_release)
> > brx = pipe->brx;
> > else if (!vsp1->bru->entity.pipe)
> > brx = >bru->entity;
> > else
> > brx = >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 >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 = >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 

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

[Mauro Carvalho Chehab]

Em Sat, 26 May 2018 03:24:00 +0300
Laurent Pinchart  escreveu:

> Hi Mauro,
> 
> On Saturday, 26 May 2018 02:39:16 EEST Laurent Pinchart wrote:
> > On Saturday, 26 May 2018 02:10:27 EEST Mauro Carvalho Chehab wrote:  
> > > Em Sun, 20 May 2018 15:10:50 +0300 Laurent Pinchart escreveu:  
> > >> Hi Mauro,
> > >> 
> > >> The following changes since commit
> > >> 
> > >> 8ed8bba70b4355b1ba029b151ade84475dd12991:
> > >>   media: imx274: remove non-indexed pointers from mode_table (2018-05-17
> > >> 
> > >> 06:22:08 -0400)
> > >> 
> > >> are available in the Git repository at:
> > >>   git://linuxtv.org/pinchartl/media.git v4l2/vsp1/next
> > >> 
> > >> for you to fetch changes up to 429f256501652c90a4ed82f2416618f82a77d37c:
> > >>   media: vsp1: Move video configuration to a cached dlb (2018-05-20
> > >>   09:46:51 +0300)
> > >> 
> > >> The branch passes the VSP and DU test suites, both on its own and when
> > >> merged with the drm-next branch.  
> > > 
> > > This series added a new warning:
> > > 
> > > drivers/media/platform/vsp1/vsp1_dl.c:69: warning: Function parameter or
> > > member 'refcnt' not described in 'vsp1_dl_body'  
> > 
> > We'll fix that. Kieran, as you authored the code, would you like to give it
> > a go ?
> >   
> > > To the already existing one:
> > > 
> > > drivers/media/platform/vsp1/vsp1_drm.c:336 vsp1_du_pipeline_setup_brx()
> > > error: we previously assumed 'pipe->brx' could be null (see line 244)  
> > 
> > That's still on my todo list. I tried to give it a go but received plenty of
> > SQL errors. How do you run smatch ?  
> 
> Nevermind, I found out what was wrong (had to specify the data directory 
> manually).
> 
> 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 = >bru->entity;
> 30. else
> 31. brx = >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 = >bru->entity;
> else if (pipe->brx && !drm_pipe->force_brx_release)
> brx = pipe->brx;
> else if (!vsp1->bru->entity.pipe)
> brx = >bru->entity;
> else
> brx = >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.

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 

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

[Laurent Pinchart]

Hi Mauro,

On Saturday, 26 May 2018 02:39:16 EEST Laurent Pinchart wrote:
> On Saturday, 26 May 2018 02:10:27 EEST Mauro Carvalho Chehab wrote:
> > Em Sun, 20 May 2018 15:10:50 +0300 Laurent Pinchart escreveu:
> >> Hi Mauro,
> >> 
> >> The following changes since commit
> >> 
> >> 8ed8bba70b4355b1ba029b151ade84475dd12991:
> >>   media: imx274: remove non-indexed pointers from mode_table (2018-05-17
> >> 
> >> 06:22:08 -0400)
> >> 
> >> are available in the Git repository at:
> >>   git://linuxtv.org/pinchartl/media.git v4l2/vsp1/next
> >> 
> >> for you to fetch changes up to 429f256501652c90a4ed82f2416618f82a77d37c:
> >>   media: vsp1: Move video configuration to a cached dlb (2018-05-20
> >>   09:46:51 +0300)
> >> 
> >> The branch passes the VSP and DU test suites, both on its own and when
> >> merged with the drm-next branch.
> > 
> > This series added a new warning:
> > 
> > drivers/media/platform/vsp1/vsp1_dl.c:69: warning: Function parameter or
> > member 'refcnt' not described in 'vsp1_dl_body'
> 
> We'll fix that. Kieran, as you authored the code, would you like to give it
> a go ?
> 
> > To the already existing one:
> > 
> > drivers/media/platform/vsp1/vsp1_drm.c:336 vsp1_du_pipeline_setup_brx()
> > error: we previously assumed 'pipe->brx' could be null (see line 244)
> 
> That's still on my todo list. I tried to give it a go but received plenty of
> SQL errors. How do you run smatch ?

Nevermind, I found out what was wrong (had to specify the data directory 
manually).

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 = >bru->entity;
30. else
31. brx = >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 = >bru->entity;
else if (pipe->brx && !drm_pipe->force_brx_release)
brx = pipe->brx;
else if (!vsp1->bru->entity.pipe)
brx = >bru->entity;
else
brx = >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 we could teach smatch to detect this, it's too complicated. One 
possible fix for the warning that wouldn't just silence it artificially could 
be

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

But running the test case again, this still produces a warning. Now I'm 
getting puzzled, I don't see how smatch can still believe brx could be NULL.

> > (there's also a Spectre warning too, but I'll looking into those
> > in separate).

[snip]

-- 
Regards,

Laurent Pinchart

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

[Laurent Pinchart]

Hi Mauro,

(CC'ing Kieran)

On Saturday, 26 May 2018 02:10:27 EEST Mauro Carvalho Chehab wrote:
> Em Sun, 20 May 2018 15:10:50 +0300 Laurent Pinchart escreveu:
> > Hi Mauro,
> > 
> > The following changes since commit
> > 8ed8bba70b4355b1ba029b151ade84475dd12991:
> >   media: imx274: remove non-indexed pointers from mode_table (2018-05-17
> > 06:22:08 -0400)
> > 
> > are available in the Git repository at:
> >   git://linuxtv.org/pinchartl/media.git v4l2/vsp1/next
> > 
> > for you to fetch changes up to 429f256501652c90a4ed82f2416618f82a77d37c:
> >   media: vsp1: Move video configuration to a cached dlb (2018-05-20
> >   09:46:51 +0300)
> > 
> > The branch passes the VSP and DU test suites, both on its own and when
> > merged with the drm-next branch.
> 
> This series added a new warning:
> 
> drivers/media/platform/vsp1/vsp1_dl.c:69: warning: Function parameter or
> member 'refcnt' not described in 'vsp1_dl_body'

We'll fix that. Kieran, as you authored the code, would you like to give it a 
go ?

> To the already existing one:
> 
> drivers/media/platform/vsp1/vsp1_drm.c:336 vsp1_du_pipeline_setup_brx()
> error: we previously assumed 'pipe->brx' could be null (see line 244)

That's still on my todo list. I tried to give it a go but received plenty of 
SQL errors. How do you run smatch ?

> (there's also a Spectre warning too, but I'll looking into those
> in separate).
> 
> For now, I'll apply it, but I reserve the right of not pulling any
> new patchsets that would add more warnings.
> 
> > 
> > 
> > Geert Uytterhoeven (1):
> >   media: vsp1: Drop OF dependency of VIDEO_RENESAS_VSP1
> > 
> > Kieran Bingham (10):
> >   media: vsp1: Release buffers for each video node
> >   media: vsp1: Move video suspend resume handling to video object
> >   media: vsp1: Reword uses of 'fragment' as 'body'
> >   media: vsp1: Protect bodies against overflow
> >   media: vsp1: Provide a body pool
> >   media: vsp1: Convert display lists to use new body pool
> >   media: vsp1: Use reference counting for bodies
> >   media: vsp1: Refactor display list configure operations
> >   media: vsp1: Adapt entities to configure into a body
> >   media: vsp1: Move video configuration to a cached dlb
> >  
> >  drivers/media/platform/Kconfig|   2 +-
> >  drivers/media/platform/vsp1/vsp1_brx.c|  32 ++--
> >  drivers/media/platform/vsp1/vsp1_clu.c| 113 ++-
> >  drivers/media/platform/vsp1/vsp1_clu.h|   1 +
> >  drivers/media/platform/vsp1/vsp1_dl.c | 388 ++---
> >  drivers/media/platform/vsp1/vsp1_dl.h |  21 ++-
> >  drivers/media/platform/vsp1/vsp1_drm.c|  18 +-
> >  drivers/media/platform/vsp1/vsp1_drv.c|   4 +-
> >  drivers/media/platform/vsp1/vsp1_entity.c |  34 +++-
> >  drivers/media/platform/vsp1/vsp1_entity.h |  45 +++--
> >  drivers/media/platform/vsp1/vsp1_hgo.c|  26 ++-
> >  drivers/media/platform/vsp1/vsp1_hgt.c|  28 ++-
> >  drivers/media/platform/vsp1/vsp1_hsit.c   |  20 +-
> >  drivers/media/platform/vsp1/vsp1_lif.c|  25 ++-
> >  drivers/media/platform/vsp1/vsp1_lut.c|  80 +---
> >  drivers/media/platform/vsp1/vsp1_lut.h|   1 +
> >  drivers/media/platform/vsp1/vsp1_pipe.c   |  74 +---
> >  drivers/media/platform/vsp1/vsp1_pipe.h   |  12 +-
> >  drivers/media/platform/vsp1/vsp1_rpf.c| 189 ++-
> >  drivers/media/platform/vsp1/vsp1_sru.c|  24 +--
> >  drivers/media/platform/vsp1/vsp1_uds.c|  73 +++
> >  drivers/media/platform/vsp1/vsp1_uds.h|   2 +-
> >  drivers/media/platform/vsp1/vsp1_uif.c|  35 ++--
> >  drivers/media/platform/vsp1/vsp1_video.c  | 177 -
> >  drivers/media/platform/vsp1/vsp1_video.h  |   3 +
> >  drivers/media/platform/vsp1/vsp1_wpf.c| 326 ++---
> >  26 files changed, 967 insertions(+), 786 deletions(-)

-- 
Regards,

Laurent Pinchart

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

[Mauro Carvalho Chehab]

Em Sun, 20 May 2018 15:10:50 +0300
Laurent Pinchart  escreveu:

> Hi Mauro,
> 
> The following changes since commit 8ed8bba70b4355b1ba029b151ade84475dd12991:
> 
>   media: imx274: remove non-indexed pointers from mode_table (2018-05-17 
> 06:22:08 -0400)
> 
> are available in the Git repository at:
> 
>   git://linuxtv.org/pinchartl/media.git v4l2/vsp1/next
> 
> for you to fetch changes up to 429f256501652c90a4ed82f2416618f82a77d37c:
> 
>   media: vsp1: Move video configuration to a cached dlb (2018-05-20 09:46:51 
> +0300)
> 
> The branch passes the VSP and DU test suites, both on its own and when merged 
> with the drm-next branch.

This series added a new warning:

drivers/media/platform/vsp1/vsp1_dl.c:69: warning: Function parameter or member 
'refcnt' not described in 'vsp1_dl_body'

To the already existing one:

drivers/media/platform/vsp1/vsp1_drm.c:336 vsp1_du_pipeline_setup_brx() error: 
we previously assumed 'pipe->brx' could be null (see line 244)

(there's also a Spectre warning too, but I'll looking into those
in separate).

For now, I'll apply it, but I reserve the right of not pulling any
new patchsets that would add more warnings.



> 
> 
> Geert Uytterhoeven (1):
>   media: vsp1: Drop OF dependency of VIDEO_RENESAS_VSP1
> 
> Kieran Bingham (10):
>   media: vsp1: Release buffers for each video node
>   media: vsp1: Move video suspend resume handling to video object
>   media: vsp1: Reword uses of 'fragment' as 'body'
>   media: vsp1: Protect bodies against overflow
>   media: vsp1: Provide a body pool
>   media: vsp1: Convert display lists to use new body pool
>   media: vsp1: Use reference counting for bodies
>   media: vsp1: Refactor display list configure operations
>   media: vsp1: Adapt entities to configure into a body
>   media: vsp1: Move video configuration to a cached dlb
> 
>  drivers/media/platform/Kconfig|   2 +-
>  drivers/media/platform/vsp1/vsp1_brx.c|  32 ++--
>  drivers/media/platform/vsp1/vsp1_clu.c| 113 ++-
>  drivers/media/platform/vsp1/vsp1_clu.h|   1 +
>  drivers/media/platform/vsp1/vsp1_dl.c | 388 -
>  drivers/media/platform/vsp1/vsp1_dl.h |  21 ++-
>  drivers/media/platform/vsp1/vsp1_drm.c|  18 +-
>  drivers/media/platform/vsp1/vsp1_drv.c|   4 +-
>  drivers/media/platform/vsp1/vsp1_entity.c |  34 +++-
>  drivers/media/platform/vsp1/vsp1_entity.h |  45 +++--
>  drivers/media/platform/vsp1/vsp1_hgo.c|  26 ++-
>  drivers/media/platform/vsp1/vsp1_hgt.c|  28 ++-
>  drivers/media/platform/vsp1/vsp1_hsit.c   |  20 +-
>  drivers/media/platform/vsp1/vsp1_lif.c|  25 ++-
>  drivers/media/platform/vsp1/vsp1_lut.c|  80 +---
>  drivers/media/platform/vsp1/vsp1_lut.h|   1 +
>  drivers/media/platform/vsp1/vsp1_pipe.c   |  74 +---
>  drivers/media/platform/vsp1/vsp1_pipe.h   |  12 +-
>  drivers/media/platform/vsp1/vsp1_rpf.c| 189 ++-
>  drivers/media/platform/vsp1/vsp1_sru.c|  24 +--
>  drivers/media/platform/vsp1/vsp1_uds.c|  73 +++
>  drivers/media/platform/vsp1/vsp1_uds.h|   2 +-
>  drivers/media/platform/vsp1/vsp1_uif.c|  35 ++--
>  drivers/media/platform/vsp1/vsp1_video.c  | 177 -
>  drivers/media/platform/vsp1/vsp1_video.h  |   3 +
>  drivers/media/platform/vsp1/vsp1_wpf.c| 326 ++-
>  26 files changed, 967 insertions(+), 786 deletions(-)
> 



Thanks,
Mauro

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

[Laurent Pinchart]

Hi Mauro,

The following changes since commit 8ed8bba70b4355b1ba029b151ade84475dd12991:

  media: imx274: remove non-indexed pointers from mode_table (2018-05-17 
06:22:08 -0400)

are available in the Git repository at:

  git://linuxtv.org/pinchartl/media.git v4l2/vsp1/next

for you to fetch changes up to 429f256501652c90a4ed82f2416618f82a77d37c:

  media: vsp1: Move video configuration to a cached dlb (2018-05-20 09:46:51 
+0300)

The branch passes the VSP and DU test suites, both on its own and when merged 
with the drm-next branch.


Geert Uytterhoeven (1):
  media: vsp1: Drop OF dependency of VIDEO_RENESAS_VSP1

Kieran Bingham (10):
  media: vsp1: Release buffers for each video node
  media: vsp1: Move video suspend resume handling to video object
  media: vsp1: Reword uses of 'fragment' as 'body'
  media: vsp1: Protect bodies against overflow
  media: vsp1: Provide a body pool
  media: vsp1: Convert display lists to use new body pool
  media: vsp1: Use reference counting for bodies
  media: vsp1: Refactor display list configure operations
  media: vsp1: Adapt entities to configure into a body
  media: vsp1: Move video configuration to a cached dlb

 drivers/media/platform/Kconfig|   2 +-
 drivers/media/platform/vsp1/vsp1_brx.c|  32 ++--
 drivers/media/platform/vsp1/vsp1_clu.c| 113 ++-
 drivers/media/platform/vsp1/vsp1_clu.h|   1 +
 drivers/media/platform/vsp1/vsp1_dl.c | 388 -
 drivers/media/platform/vsp1/vsp1_dl.h |  21 ++-
 drivers/media/platform/vsp1/vsp1_drm.c|  18 +-
 drivers/media/platform/vsp1/vsp1_drv.c|   4 +-
 drivers/media/platform/vsp1/vsp1_entity.c |  34 +++-
 drivers/media/platform/vsp1/vsp1_entity.h |  45 +++--
 drivers/media/platform/vsp1/vsp1_hgo.c|  26 ++-
 drivers/media/platform/vsp1/vsp1_hgt.c|  28 ++-
 drivers/media/platform/vsp1/vsp1_hsit.c   |  20 +-
 drivers/media/platform/vsp1/vsp1_lif.c|  25 ++-
 drivers/media/platform/vsp1/vsp1_lut.c|  80 +---
 drivers/media/platform/vsp1/vsp1_lut.h|   1 +
 drivers/media/platform/vsp1/vsp1_pipe.c   |  74 +---
 drivers/media/platform/vsp1/vsp1_pipe.h   |  12 +-
 drivers/media/platform/vsp1/vsp1_rpf.c| 189 ++-
 drivers/media/platform/vsp1/vsp1_sru.c|  24 +--
 drivers/media/platform/vsp1/vsp1_uds.c|  73 +++
 drivers/media/platform/vsp1/vsp1_uds.h|   2 +-
 drivers/media/platform/vsp1/vsp1_uif.c|  35 ++--
 drivers/media/platform/vsp1/vsp1_video.c  | 177 -
 drivers/media/platform/vsp1/vsp1_video.h  |   3 +
 drivers/media/platform/vsp1/vsp1_wpf.c| 326 ++-
 26 files changed, 967 insertions(+), 786 deletions(-)

-- 
Regards,

Laurent Pinchart