R-Car VSP1 TLB optimisation
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(-)
Signature · 3 lines
Regards, Laurent Pinchart
Em Sun, 20 May 2018 15:10:50 +0300
Laurent Pinchart <[email protected]> escreveu:
Quoted 11 lines
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.gitv4l2/vsp1/next
Quoted 7 lines
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.
Quoted 36 lines
----------------------------------------------------------------
Geert Uytterhoeven (1):
media: vsp1: Drop OF dependency ofVIDEO_RENESAS_VSP1Kieran 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 dlbdrivers/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 ++++++++++---------
Quoted 9 lines
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
Hi Mauro,
(CC'ing Kieran)
Quoted 15 lines · 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:
Quoted 14 lines
Hi Mauro,
The following changes since commit
8ed8bba70b4355b1ba029b151ade84475dd12991:
media: imx274: remove non-indexed pointers frommode_table(2018-05-17
06:22:08 -0400)are available in the Git repository at:
git://linuxtv.org/pinchartl/media.gitv4l2/vsp1/nextfor you to fetch changes up to
429f256501652c90a4ed82f2416618f82a77d37c:
media: vsp1: Move video configuration to a cached dlb (2018-05-20 09:46:51 +0300)
Quoted 7 lines
Quoted 2 lines
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 ?
Quoted 4 lines
To the already existing one:
drivers/media/platform/vsp1/vsp1_drm.c:336vsp1_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 ?
Quoted 50 lines
(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.
Quoted 44 lines
----------------------------------------------------------------
Geert Uytterhoeven (1):
media: vsp1: Drop OF dependency ofVIDEO_RENESAS_VSP1Kieran 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 dlbdrivers/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(-)
Signature · 3 lines
Regards, Laurent Pinchart
Hi Mauro,
Quoted 29 lines · On Saturday, 26 May 2018 02:39:16 EEST Laurent Pinchart wrote:
Quoted 17 lines · 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:
Quoted 16 lines
Hi Mauro,
The following changes since commit
8ed8bba70b4355b1ba029b151ade84475dd12991:
media: imx274: remove non-indexed pointers frommode_table(2018-05-1706:22:08 -0400)
are available in the Git repository at:
git://linuxtv.org/pinchartl/media.gitv4l2/vsp1/nextfor you to fetch changes up to
429f256501652c90a4ed82f2416618f82a77d37c:
media: vsp1: Move video configuration to a cached dlb (2018-05-20 09:46:51 +0300)Quoted 7 lines
Quoted 2 lines
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 ?
Quoted 7 lines
Quoted 4 lines
To the already existing one:
drivers/media/platform/vsp1/vsp1_drm.c:336vsp1_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 = &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 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 = &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;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.
Quoted 2 lines
Quoted 2 lines
(there's also a Spectre warning too, but I'll looking into those in separate).
[snip]
Signature · 3 lines
Regards, Laurent Pinchart
Em Sat, 26 May 2018 03:24:00 +0300
Laurent Pinchart <[email protected]> escreveu:
Quoted 112 lines
Hi Mauro,
Quoted 29 lines · On Saturday, 26 May 2018 02:39:16 EEST Laurent Pinchart wrote:
Quoted 17 lines · 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:
Quoted 16 lines
Hi Mauro,
The following changes since commit
8ed8bba70b4355b1ba029b151ade84475dd12991:
media: imx274: remove non-indexed pointers frommode_table(2018-05-1706:22:08 -0400)
are available in the Git repository at:
git://linuxtv.org/pinchartl/media.gitv4l2/vsp1/nextfor you to fetch changes up to
429f256501652c90a4ed82f2416618f82a77d37c:
media: vsp1: Move video configuration to a cached dlb (2018-05-20 09:46:51 +0300)Quoted 7 lines
Quoted 2 lines
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 ?
Quoted 7 lines
Quoted 4 lines
To the already existing one:
drivers/media/platform/vsp1/vsp1_drm.c:336vsp1_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. structvsp1_entity{4. struct vsp1_pipeline *pipe; 5. struct vsp1_entity *sink; 6. unsigned int source_pad;7. };
8.
9. structvsp1_pipeline{
10. structvsp1_entity*brx;
11. };
12.
13. structvsp1_brx{
14. structvsp1_entityentity;
15. };
16.
17. structvsp1_device{
18. structvsp1_brx*bru;
19. structvsp1_brx*brs;
20. };
21.
22. unsigned intfrob(struct vsp1_device *vsp1, struct vsp1_pipeline *pipe)
23. {
24. structvsp1_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. returnpipe->brx->source_pad;
37. }The reason why smatch complains is that it has no guarantee that
vsp1->brsis not NULL. It's quite tricky:- On line 26, smatch assumes that
pipe->brxcan be NULL
- On line 27, brx is assigned a non-NULL value (aspipe->brxis not NULL due to line 26)
- On line 28, smatch assumes thatvsp1->bruis not NULL
- On line 29, brx is assigned a non-NULL value (asvsp1->bruis not NULL due to line 28)
- On line 31, brx is assigned a possibly NULL value (as there's no information regardingvsp1->brs)
- On line 34,pipe->brxis not assigned a non-NULL value if brx is NULL
- On line 36pipe->brxis dereferencedThe problem comes from the fact that smatch assumes that
vsp1->brsisn't NULL. Adding a "(void)vsp1->brs->entity;" statement on line 25 makes the warning disappear.So how do we know that
vsp1->brsisn'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;
Quoted 7 lines
A VSP1 instance can have no brs, so in general
vsp1->brscan be NULL. However, when that's the case, the following conditions are fulfilled.-
drm_pipe->force_brx_releasewill be false
- eitherpipe->brxwill be non-NULL, orvsp1->bru->entity.pipewill be NULLThe 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:
drivers/media/platform/vsp1/vsp1_drm.c +1 -1
@@ -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.
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:
drivers/media/platform/vsp1/vsp1_drm.c +3 -0
@@ -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.
Thanks,
Mauro
Hi Mauro,
Quoted 1 line · 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]
Quoted 84 lines
Quoted 70 lines
I've reproduced the issue and created a minimal test case.
1. struct
vsp1_pipeline;
2.
3. structvsp1_entity{4. struct vsp1_pipeline *pipe; 5. struct vsp1_entity *sink; 6. unsigned int source_pad;7. };
8.
9. structvsp1_pipeline{
10. structvsp1_entity*brx;
11. };
12.
13. structvsp1_brx{
14. structvsp1_entityentity;
15. };
16.
17. structvsp1_device{
18. structvsp1_brx*bru;
19. structvsp1_brx*brs;
20. };
21.
22. unsigned intfrob(struct vsp1_device *vsp1, struct vsp1_pipeline *pipe)
23. {
24. structvsp1_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. returnpipe->brx->source_pad;
37. }The reason why smatch complains is that it has no guarantee that
vsp1->brsis not NULL. It's quite tricky:- On line 26, smatch assumes that
pipe->brxcan be NULL
- On line 27, brx is assigned a non-NULL value (aspipe->brxis not NULL due to line 26)
- On line 28, smatch assumes thatvsp1->bruis not NULL
- On line 29, brx is assigned a non-NULL value (asvsp1->bruis not NULL due to line 28)
- On line 31, brx is assigned a possibly NULL value (as there's no information regardingvsp1->brs)
- On line 34,pipe->brxis not assigned a non-NULL value if brx is NULL
- On line 36pipe->brxis dereferencedThe problem comes from the fact that smatch assumes that
vsp1->brsisn't NULL. Adding a "(void)vsp1->brs->entity;" statement on line 25 makes the warning disappear.So how do we know that
vsp1->brsisn'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;Quoted 8 lines
A VSP1 instance can have no brs, so in general
vsp1->brscan be NULL. However, when that's the case, the following conditions are fulfilled.-
drm_pipe->force_brx_releasewill be false
- eitherpipe->brxwill be non-NULL, orvsp1->bru->entity.pipewill be NULLThe 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->brxcan be NULL, as the function explicly checks ifpipe->brx!= NULL.
Quoted 6 lines
When Smatch handles this if:
if (brx != pipe->brx) {It wrongly assumes that this could be false if
pipe->brxis 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.
Quoted 28 lines
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->brxis null, then brx won't be NULL. So, the logic that "Switch BRx if needed." will always be called:(quoted diff) +0
-0
b/drivers/media/platform/vsp1/vsp1_drm.cindex095dc48aa25a..cb6b60843400100644drivers/media/platform/vsp1/vsp1_drm.c +1
-1@@ -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. */
Quoted 6 lines
The code with switches BRx ensures that
pipe->brxwon'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-
Quoted 1 line
force_brx_releasewere needed, any of those on its own didn't fix the
problem.
Quoted 25 lines
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->brxis NULL.-
Another way to solve would be to explicitly check if
pipe->brxis still null before de-referencing:(quoted diff) +0
-0
b/drivers/media/platform/vsp1/vsp1_drm.cindexedb35a5c57ea..9fe063d6df31100644drivers/media/platform/vsp1/vsp1_drm.c +3
-0@@ -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
Quoted 6 lines
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.
Signature · 3 lines
Regards, Laurent Pinchart
Hi Mauro and Dan,
Dan, there's a question for you below.
Quoted 212 lines · On Monday, 28 May 2018 11:28:41 EEST Laurent Pinchart wrote:
Quoted 1 line · 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]
Quoted 81 lines
Quoted 70 lines
I've reproduced the issue and created a minimal test case.
1. struct
vsp1_pipeline;
2.
3. structvsp1_entity{4. struct vsp1_pipeline *pipe; 5. struct vsp1_entity *sink; 6. unsigned int source_pad;7. };
8.
9. structvsp1_pipeline{
10. structvsp1_entity*brx;
11. };
12.
13. structvsp1_brx{
14. structvsp1_entityentity;
15. };
16.
17. structvsp1_device{
18. structvsp1_brx*bru;
19. structvsp1_brx*brs;
20. };
21.
22. unsigned intfrob(struct vsp1_device *vsp1, struct vsp1_pipeline *pipe)
23. {
24. structvsp1_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. returnpipe->brx->source_pad;
37. }The reason why smatch complains is that it has no guarantee that
vsp1->brsis not NULL. It's quite tricky:- On line 26, smatch assumes that
pipe->brxcan be NULL
- On line 27, brx is assigned a non-NULL value (aspipe->brxis not NULL due to line 26)
- On line 28, smatch assumes thatvsp1->bruis not NULL
- On line 29, brx is assigned a non-NULL value (asvsp1->bruis not NULL due to line 28)
- On line 31, brx is assigned a possibly NULL value (as there's no information regardingvsp1->brs)
- On line 34,pipe->brxis not assigned a non-NULL value if brx is NULL
- On line 36pipe->brxis dereferencedThe problem comes from the fact that smatch assumes that
vsp1->brsisn't NULL. Adding a "(void)vsp1->brs->entity;" statement on line 25 makes the warning disappear.So how do we know that
vsp1->brsisn'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;Quoted 8 lines
A VSP1 instance can have no brs, so in general
vsp1->brscan be NULL. However, when that's the case, the following conditions are fulfilled.-
drm_pipe->force_brx_releasewill be false
- eitherpipe->brxwill be non-NULL, orvsp1->bru->entity.pipewill be NULLThe fourth branch should thus never be taken.
I don't think that adding a forth branch there would solve.
Quoted 8 lines
The thing is that Smatch knows that
pipe->brxcan be NULL, as the function explicly checks ifpipe->brx!= NULL.When Smatch handles this if:
if (brx != pipe->brx) {It wrongly assumes that this could be false if
pipe->brxis 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->brsis NULL (as the entity field is first in thevsp1->brsstructure, so&vsp1->brs->entityhas the same address asvsp1->brs).
vsp1->brscan be NULL on some devices, but in that case we have the following guarantees:-
drm_pipe->force_brx_releasewill always be FALSE
- eitherpipe->brxwill be non-NULL orvsp1->bru->entity.pipewill be NULLSo 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->brscan be NULL, or from somewhere else.Quoted 28 lines
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->brxis null, then brx won't be NULL. So, the logic that "Switch BRx if needed." will always be called:(quoted diff) +0
-0
b/drivers/media/platform/vsp1/vsp1_drm.cindex095dc48aa25a..cb6b60843400100644drivers/media/platform/vsp1/vsp1_drm.c +1
-1@@ -185,7 +185,7 @@ static int vsp1_du_pipeline_setup_brx(structvsp1_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. */
Quoted 6 lines
The code with switches BRx ensures that
pipe->brxwon't be null, asin 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->brscan be NULL when it can't. I'd rather modify the code in a way that explicitly tests forvsp1->brs. However, smatch won't accept that happily :-/ I triedif (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-
Quoted 1 line
force_brx_releasewere needed, any of those on its own didn't fix theproblem.
Quoted 31 lines
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->brxis NULL.-
Another way to solve would be to explicitly check if
pipe->brxis still null before de-referencing:(quoted diff) +0
-0
b/drivers/media/platform/vsp1/vsp1_drm.cindexedb35a5c57ea..9fe063d6df31100644drivers/media/platform/vsp1/vsp1_drm.c +3
-0@@ -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 drivers/media/platform/vsp1/vsp1_drm.c +0
-0the * requested format. We don't set the media bus code as it is configuredThe right fix would be, instead, to fix Smatch to handle the:
if (brx != pipe->brx)
Quoted 6 lines
Quoted 2 lines
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->brxcheck. 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 ?
Signature · 3 lines
Regards, Laurent Pinchart
Em Mon, 28 May 2018 11:31:01 +0300
Laurent Pinchart <[email protected]> escreveu:
Quoted 238 lines
Hi Mauro and Dan,
Dan, there's a question for you below.
Quoted 212 lines · On Monday, 28 May 2018 11:28:41 EEST Laurent Pinchart wrote:
Quoted 1 line · 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]
Quoted 81 lines
Quoted 70 lines
I've reproduced the issue and created a minimal test case.
1. struct
vsp1_pipeline;
2.
3. structvsp1_entity{4. struct vsp1_pipeline *pipe; 5. struct vsp1_entity *sink; 6. unsigned int source_pad;7. };
8.
9. structvsp1_pipeline{
10. structvsp1_entity*brx;
11. };
12.
13. structvsp1_brx{
14. structvsp1_entityentity;
15. };
16.
17. structvsp1_device{
18. structvsp1_brx*bru;
19. structvsp1_brx*brs;
20. };
21.
22. unsigned intfrob(struct vsp1_device *vsp1, struct vsp1_pipeline *pipe)
23. {
24. structvsp1_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. returnpipe->brx->source_pad;
37. }The reason why smatch complains is that it has no guarantee that
vsp1->brsis not NULL. It's quite tricky:- On line 26, smatch assumes that
pipe->brxcan be NULL
- On line 27, brx is assigned a non-NULL value (aspipe->brxis not NULL due to line 26)
- On line 28, smatch assumes thatvsp1->bruis not NULL
- On line 29, brx is assigned a non-NULL value (asvsp1->bruis not NULL due to line 28)
- On line 31, brx is assigned a possibly NULL value (as there's no information regardingvsp1->brs)
- On line 34,pipe->brxis not assigned a non-NULL value if brx is NULL
- On line 36pipe->brxis dereferencedThe problem comes from the fact that smatch assumes that
vsp1->brsisn't NULL. Adding a "(void)vsp1->brs->entity;" statement on line 25 makes the warning disappear.So how do we know that
vsp1->brsisn'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;Quoted 8 lines
A VSP1 instance can have no brs, so in general
vsp1->brscan be NULL. However, when that's the case, the following conditions are fulfilled.-
drm_pipe->force_brx_releasewill be false
- eitherpipe->brxwill be non-NULL, orvsp1->bru->entity.pipewill be NULLThe fourth branch should thus never be taken.
I don't think that adding a forth branch there would solve.
Quoted 8 lines
The thing is that Smatch knows that
pipe->brxcan be NULL, as the function explicly checks ifpipe->brx!= NULL.When Smatch handles this if:
if (brx != pipe->brx) {It wrongly assumes that this could be false if
pipe->brxis 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->brsis NULL (as the entity field is first in thevsp1->brsstructure, so&vsp1->brs->entityhas the same address asvsp1->brs).
vsp1->brscan be NULL on some devices, but in that case we have the following guarantees:-
drm_pipe->force_brx_releasewill always be FALSE
- eitherpipe->brxwill be non-NULL orvsp1->bru->entity.pipewill be NULLSo 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->brscan be NULL, or from somewhere else.Quoted 28 lines
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->brxis null, then brx won't be NULL. So, the logic that "Switch BRx if needed." will always be called:(quoted diff) +0
-0
b/drivers/media/platform/vsp1/vsp1_drm.cindex095dc48aa25a..cb6b60843400100644drivers/media/platform/vsp1/vsp1_drm.c +1
-1@@ -185,7 +185,7 @@ static int vsp1_du_pipeline_setup_brx(structvsp1_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. */
Quoted 6 lines
The code with switches BRx ensures that
pipe->brxwon't be null, asin 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->brscan be NULL when it can't. I'd rather modify the code in a way that explicitly tests forvsp1->brs. However, smatch won't accept that happily :-/ I triedif (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-
Quoted 1 line
force_brx_releasewere needed, any of those on its own didn't fix theproblem.
Quoted 31 lines
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->brxis NULL.-
Another way to solve would be to explicitly check if
pipe->brxis still null before de-referencing:(quoted diff) +0
-0
b/drivers/media/platform/vsp1/vsp1_drm.cindexedb35a5c57ea..9fe063d6df31100644drivers/media/platform/vsp1/vsp1_drm.c +3
-0@@ -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 drivers/media/platform/vsp1/vsp1_drm.c +0
-0the * requested format. We don't set the media bus code as it is configuredThe right fix would be, instead, to fix Smatch to handle the:
if (brx != pipe->brx)Quoted 6 lines
Quoted 2 lines
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->brxcheck. 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;
Quoted 3 lines
and that didn't help either... Dan, would you have some light to shed on this problem ?
This (obviously wrong patch) shut up the warning:
drivers/media/platform/vsp1/vsp1_drm.c +3 -0
@@ -248,6 +248,9 @@ static int vsp1_du_pipeline_setup_brx(struct vsp1_device *vsp1,else brx = &vsp1->brs->entity; +if (pipe->brx == brx) + pipe->brx = &vsp1->brs->entity; +/* Switch BRx if needed. */ if (brx != pipe->brx) {struct vsp1_entity *released_brx =NULL ;
The problem here is that:
1) Smatch knows that pipe->brx can be NULL;
2) Smatch is not smart enough to detect that, if pipe->brx == NULL, brx won't be NULL and the if will always succeed.
However, Smatch seems to be smart enough to detect that brx is not null, as it properly identifies that this code segment:
pipe->brx = brx;
pipe->brx->pipe = pipe;
pipe->brx->sink = &pipe->output->entity;
pipe->brx->sink_pad = 0;will initialize pipe->brx with a non-NULL value.
Thanks,
Mauro
Quoted 6 lines · Mauro Carvalho Chehab
This (obviously wrong patch) shut up the warning:
drivers/media/platform/vsp1/vsp1_drm.c static int vsp1_du_pipeline_setup_brx(struct vsp1_device *vsp1, · line 256
Quoted 9 lines
drivers/media/platform/vsp1/vsp1_drm.c +3
-0@@ -248,6 +248,9 @@ static int vsp1_du_pipeline_setup_brx(struct vsp1_device *vsp1,else brx = &vsp1->brs->entity; +if (pipe->brx == brx) + pipe->brx = &vsp1->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
Quoted 14 lines · Laurent Pinchart
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;
Quoted 2 lines
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 &vsp1->brs->entity, vsp1->brs, &vsp1->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 == &vsp1->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
Hi Dan,
Quoted 26 lines · On Monday, 28 May 2018 13:36:08 EEST Dan Carpenter wrote:
Quoted 14 lines · Laurent Pinchart
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;Quoted 2 lines
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
&vsp1->brs->entity,vsp1->brs,&vsp1->bru->entityandvsp1->bruall 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.
Quoted 5 lines
The other potential problem here is that Smatch stores comparisons and values separately. In other words
smatch_comparison.chas all the information about brx ==&vsp1->bru->entityandsmatch_extra.chas 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 = &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;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.
Signature · 3 lines
Regards, Laurent Pinchart
Quoted 24 lines · Laurent Pinchart
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 = &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;1. if (!brx)
return -EINVAL;2. if (brx != pipe->brx) { ... 3. pipe->brx = brx; ... }
Quoted 6 lines
4. format.pad =
pipe->brx->source_pad(1) ensures that brx can't be NULL. (2) is thus always true if
pipe->brxis NULL. (3) then assigns a non-NULL value topipe->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
Em Mon, 28 May 2018 11:28:41 +0300
Laurent Pinchart <[email protected]> escreveu:
Quoted 93 lines
Hi Mauro,
Quoted 1 line · 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]
Quoted 86 lines
Quoted 70 lines
I've reproduced the issue and created a minimal test case.
1. struct
vsp1_pipeline;
2.
3. structvsp1_entity{4. struct vsp1_pipeline *pipe; 5. struct vsp1_entity *sink; 6. unsigned int source_pad;7. };
8.
9. structvsp1_pipeline{
10. structvsp1_entity*brx;
11. };
12.
13. structvsp1_brx{
14. structvsp1_entityentity;
15. };
16.
17. structvsp1_device{
18. structvsp1_brx*bru;
19. structvsp1_brx*brs;
20. };
21.
22. unsigned intfrob(struct vsp1_device *vsp1, struct vsp1_pipeline *pipe)
23. {
24. structvsp1_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. returnpipe->brx->source_pad;
37. }The reason why smatch complains is that it has no guarantee that
vsp1->brsis not NULL. It's quite tricky:- On line 26, smatch assumes that
pipe->brxcan be NULL
- On line 27, brx is assigned a non-NULL value (aspipe->brxis not NULL due to line 26)
- On line 28, smatch assumes thatvsp1->bruis not NULL
- On line 29, brx is assigned a non-NULL value (asvsp1->bruis not NULL due to line 28)
- On line 31, brx is assigned a possibly NULL value (as there's no information regardingvsp1->brs)
- On line 34,pipe->brxis not assigned a non-NULL value if brx is NULL
- On line 36pipe->brxis dereferencedThe problem comes from the fact that smatch assumes that
vsp1->brsisn't NULL. Adding a "(void)vsp1->brs->entity;" statement on line 25 makes the warning disappear.So how do we know that
vsp1->brsisn'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;Quoted 8 lines
A VSP1 instance can have no brs, so in general
vsp1->brscan be NULL. However, when that's the case, the following conditions are fulfilled.-
drm_pipe->force_brx_releasewill be false
- eitherpipe->brxwill be non-NULL, orvsp1->bru->entity.pipewill be NULLThe 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->brxcan be NULL, as the function explicly checks ifpipe->brx!= NULL.When Smatch handles this if:
Quoted 8 lines
Quoted 4 lines
if (brx != pipe->brx) {It wrongly assumes that this could be false if
pipe->brxis 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->brsis NULL (as the entity field is first in thevsp1->brsstructure, so&vsp1->brs->entityhas the same address asvsp1->brs).
I can't see how brx can be NULL. At the sequence of ifs:
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;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.
Quoted 44 lines
vsp1->brscan be NULL on some devices, but in that case we have the following guarantees:-
drm_pipe->force_brx_releasewill always be FALSE
- eitherpipe->brxwill be non-NULL orvsp1->bru->entity.pipewill be NULLSo 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->brscan be NULL, or from somewhere else.Quoted 31 lines
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->brxis null, then brx won't be NULL. So, the logic that "Switch BRx if needed." will always be called:(quoted diff) +0
-0
b/drivers/media/platform/vsp1/vsp1_drm.cindex095dc48aa25a..cb6b60843400100644drivers/media/platform/vsp1/vsp1_drm.c +1
-1@@ -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->brxwon't be null, as in the end, it sets:
Quoted 6 lines
Quoted 3 lines
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->brscan be NULL when it can't.
No, that's not Smatch assumption. If it where, it would print a different kind of warning, at this statement:
brx = &vsp1->brs->entity;
It only complains later at this line:
format.pad = pipe->brx->source_pad;
because it thinks that pipe->brx can be NULL. This happens only if pipe->brx is NULL and this if fails:
/* Switch BRx if needed. */
if (pipe->brx != brx) {Quoted 60 lines
I'd rather
modify the code in a way that explicitly tests forvsp1->brs. However, smatch won't accept that happily :-/ I triedif (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-
Quoted 1 line
force_brx_releasewere needed, any of those on its own didn't fix theproblem.
Quoted 27 lines
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->brxis NULL.-
Another way to solve would be to explicitly check if
pipe->brxis still null before de-referencing:(quoted diff) +0
-0
b/drivers/media/platform/vsp1/vsp1_drm.cindexedb35a5c57ea..9fe063d6df31100644drivers/media/platform/vsp1/vsp1_drm.c +3
-0@@ -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:
Quoted 8 lines
Quoted 4 lines
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->brxcheck. The problem seems to come from the fact that is believes brx can be NULL.
The brx dependency logic is complex, with, IMHO, tricks enough Smatch to not be able of properly evaluate that the if will always be true if pipe->brx is NULL.
Thanks,
Mauro
Hi Mauro,
Quoted 112 lines · 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:
Quoted 92 lines · On Saturday, 26 May 2018 14:28:18 EEST Mauro Carvalho Chehab wrote:
Quoted 1 line
Em Sat, 26 May 2018 03:24:00 +0300 Laurent Pinchart escreveu:
[snip]
Quoted 88 lines
Quoted 71 lines
I've reproduced the issue and created a minimal test case.
1. struct
vsp1_pipeline;
2.
3. structvsp1_entity{4. struct vsp1_pipeline *pipe; 5. struct vsp1_entity *sink; 6. unsigned int source_pad;7. };
8.
9. structvsp1_pipeline{
10. structvsp1_entity*brx;
11. };
12.
13. structvsp1_brx{
14. structvsp1_entityentity;
15. };
16.
17. structvsp1_device{
18. structvsp1_brx*bru;
19. structvsp1_brx*brs;
20. };
21.
22. unsigned intfrob(struct vsp1_device *vsp1, struct vsp1_pipeline *pipe)
23. {
24. structvsp1_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. returnpipe->brx->source_pad;
37. }The reason why smatch complains is that it has no guarantee that
vsp1->brsis not NULL. It's quite tricky:- On line 26, smatch assumes that
pipe->brxcan be NULL
- On line 27, brx is assigned a non-NULL value (aspipe->brxis not NULL due to line 26)
- On line 28, smatch assumes thatvsp1->bruis not NULL
- On line 29, brx is assigned a non-NULL value (asvsp1->bruis not NULL due to line 28)
- On line 31, brx is assigned a possibly NULL value (as there's no information regardingvsp1->brs)
- On line 34,pipe->brxis not assigned a non-NULL value if brx is NULL
- On line 36pipe->brxis dereferencedThe problem comes from the fact that smatch assumes that
vsp1->brsisn't NULL. Adding a "(void)vsp1->brs->entity;" statement on line 25 makes the warning disappear.So how do we know that
vsp1->brsisn'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;Quoted 8 lines
A VSP1 instance can have no brs, so in general
vsp1->brscan be NULL. However, when that's the case, the following conditions are fulfilled.-
drm_pipe->force_brx_releasewill be false
- eitherpipe->brxwill be non-NULL, orvsp1->bru->entity.pipewill be NULLThe 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->brxcan be NULL, as the function explicly checks ifpipe->brx!= NULL.When Smatch handles this if:
if (brx != pipe->brx) {Quoted 6 lines
Quoted 2 lines
It wrongly assumes that this could be false if
pipe->brxis 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->brsis NULL (as the entity field is first in thevsp1->brsstructure, so&vsp1->brs->entityhas the same address asvsp1->brs).I can't see how brx can be NULL. At the sequence of ifs:
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;
Quoted 1 line
The usage of brx = &(something) will always return a non NULL value[1].
I don't agree. When vsp1->brs is NULL, &vsp1->brs->entity will evaluate to NULL as well.
Quoted 11 lines
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->brxis 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.
Quoted 5 lines
So, brx will always be non-NULL, even if
pipe->brxis NULL.[1] It might be doing a NULL deref - with seems to be your concern when you're talking about the case where
vsp1->brsis NULL - but that's not what Smatch is complaining here.
&vsp1->brs->entity will not cause a NULL dereference, it doesn't cause vsp1-
Quoted 59 lines
brs to be dereferenced.
Quoted 44 lines
vsp1->brscan be NULL on some devices, but in that case we have the following guarantees:-
drm_pipe->force_brx_releasewill always be FALSE
- eitherpipe->brxwill be non-NULL orvsp1->bru->entity.pipewill be NULLSo 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->brscan be NULL, or from somewhere else.Quoted 32 lines
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->brxis null, then brx won't be NULL. So, the logic that "Switch BRx if needed." will always be called:(quoted diff) +0
-0
b/drivers/media/platform/vsp1/vsp1_drm.cindex095dc48aa25a..cb6b60843400
100644drivers/media/platform/vsp1/vsp1_drm.c +1
-1@@ -185,7 +185,7 @@ static int vsp1_du_pipeline_setup_brx(structvsp1_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->brxwon't be null, as in the end, it sets:Quoted 6 lines
Quoted 3 lines
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->brscan be NULL when it can't.No, that's not Smatch assumption. If it where, it would print a different kind of warning, at this statement:
brx = &vsp1->brs->entity;It only complains later at this line:
Quoted 7 lines
format.pad = pipe->brx->source_pad;because it thinks that
pipe->brxcan be NULL. This happens only ifpipe->brxis NULL and this if fails:/* Switch BRx if needed. */if (
pipe->brx!= brx) {
Correct, and I think that smatch believes that branch is not necessarily taken when pipe->brx is NULL, because brx can also be NULL.
Quoted 74 lines
Quoted 64 lines
I'd rather modify the code in a way that explicitly tests for
vsp1->brs. However, smatch won't accept that happily :-/ I triedif (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_releasewere needed, any of those on its own didn't fix the problem.Quoted 32 lines
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->brxis NULL.-
Another way to solve would be to explicitly check if
pipe->brxis still null before de-referencing:(quoted diff) +0
-0
b/drivers/media/platform/vsp1/vsp1_drm.cindexedb35a5c57ea..9fe063d6df31
100644drivers/media/platform/vsp1/vsp1_drm.c +3
-0@@ -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 drivers/media/platform/vsp1/vsp1_drm.c +0
-0the * requested format. We don't set the media bus code as it is configuredThe right fix would be, instead, to fix Smatch to handle the:
if (brx != pipe->brx)Quoted 6 lines
Quoted 2 lines
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->brxcheck. The problem seems to come from the fact that is believes brx can be NULL.The brx dependency logic is complex, with, IMHO, tricks enough Smatch to not be able of properly evaluate that the if will always be true if
pipe->brxis NULL.
Signature · 3 lines
Regards, Laurent Pinchart
Quoted 12 lines · Mauro Carvalho Chehab
I can't see how brx can be NULL. At the sequence of ifs:
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;
Quoted 2 lines
The usage of brx = &(something) will always return a non NULL value[1].
That's not right. It can be NULL if it's &foo->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.
Quoted 3 lines
[1] It might be doing a NULL deref - with seems to be your concern when you're talking about the case where
vsp1->brsis 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 = &vsp1->bru->entity;
Then Smatch sees that vsp1->bru is dereferenced and marks "brx" as non-NULL.
regards,
dan carpenter
Quoted 14 lines · Laurent Pinchart
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; }
Quoted 3 lines
Both the (void)vsp1->brs->entity and the removal of the !drm_pipe-
Quoted 1 line
force_brx_releasewere needed, any of those on its own didn't fix theproblem.
The problem in this case is the first "brx = &vsp1->bru->entity;". Smatch assumes &vsp1->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
Hi Dan,
Thank you for your quick reply.
Quoted 15 lines · On Monday, 28 May 2018 13:20:49 EEST Dan Carpenter wrote:
Quoted 14 lines · Laurent Pinchart
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; }
Quoted 6 lines
Quoted 3 lines
Both the (void)vsp1->brs->entity and the removal of the !drm_pipe->
force_brx_releasewere needed, any of those on its own didn't fix the problem.The problem in this case is the first "brx =
&vsp1->bru->entity;". Smatch assumes&vsp1->bru->entitycan be == topipe->brxand NULL.
Why does smatch assume that &vsp1->bru->entity can be NULL, when the previous line dereferences vsp1->bru ?
Quoted 2 lines
Adding a "(void)vsp1->bru->entity;" on that path will silence the warning (hopefully).
Signature · 3 lines
Regards, Laurent Pinchart
Quoted 24 lines · Laurent Pinchart
Hi Dan,
Thank you for your quick reply.
Quoted 19 lines · On Monday, 28 May 2018 13:20:49 EEST Dan Carpenter wrote:
Quoted 17 lines · Laurent Pinchart
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_releasewere needed, any of those on its own didn't fix the problem.
Quoted 5 lines
Quoted 2 lines
The problem in this case is the first "brx =
&vsp1->bru->entity;". Smatch assumes&vsp1->bru->entitycan be == topipe->brxand NULL.Why does smatch assume that
&vsp1->bru->entitycan be NULL, when the previous line dereferencesvsp1->bru?
I'm talking about when pipe->num_inputs > 2. For the second "brx = &vsp1->bru->entity;" assignment, then Smatch parses it correctly as you say.
regards,
dan carpenter
Quoted 104 lines · Mauro Carvalho Chehab
Em Sat, 26 May 2018 03:24:00 +0300
Laurent Pinchart <[email protected]> escreveu:Quoted 101 lines
Hi Mauro,
Quoted 29 lines · On Saturday, 26 May 2018 02:39:16 EEST Laurent Pinchart wrote:
Quoted 17 lines · 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:
Quoted 16 lines
Hi Mauro,
The following changes since commit
8ed8bba70b4355b1ba029b151ade84475dd12991:
media: imx274: remove non-indexed pointers frommode_table(2018-05-1706:22:08 -0400)
are available in the Git repository at:
git://linuxtv.org/pinchartl/media.gitv4l2/vsp1/nextfor you to fetch changes up to
429f256501652c90a4ed82f2416618f82a77d37c:
media: vsp1: Move video configuration to a cached dlb (2018-05-20 09:46:51 +0300)Quoted 7 lines
Quoted 2 lines
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 ?
Quoted 7 lines
Quoted 4 lines
To the already existing one:
drivers/media/platform/vsp1/vsp1_drm.c:336vsp1_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. structvsp1_entity{4. struct vsp1_pipeline *pipe; 5. struct vsp1_entity *sink; 6. unsigned int source_pad;7. };
8.
9. structvsp1_pipeline{
10. structvsp1_entity*brx;
11. };
12.
13. structvsp1_brx{
14. structvsp1_entityentity;
15. };
16.
17. structvsp1_device{
18. structvsp1_brx*bru;
19. structvsp1_brx*brs;
20. };
21.
22. unsigned intfrob(struct vsp1_device *vsp1, struct vsp1_pipeline *pipe)
23. {
24. structvsp1_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. returnpipe->brx->source_pad;
37. }The reason why smatch complains is that it has no guarantee that
vsp1->brsis not NULL. It's quite tricky:- On line 26, smatch assumes that
pipe->brxcan be NULL
- On line 27, brx is assigned a non-NULL value (aspipe->brxis not NULL due to line 26)
- On line 28, smatch assumes thatvsp1->bruis not NULL
- On line 29, brx is assigned a non-NULL value (asvsp1->bruis not NULL due to line 28)
- On line 31, brx is assigned a possibly NULL value (as there's no information regardingvsp1->brs)
- On line 34,pipe->brxis not assigned a non-NULL value if brx is NULL
- On line 36pipe->brxis dereferencedThe problem comes from the fact that smatch assumes that
vsp1->brsisn'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(&vsp1->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
Hi Mauro, Laurent,
Quoted 23 lines · Laurent Pinchart
Hi Mauro,
(CC'ing Kieran)
Quoted 18 lines · 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:
Quoted 16 lines
Hi Mauro,
The following changes since commit
8ed8bba70b4355b1ba029b151ade84475dd12991:
media: imx274: remove non-indexed pointers frommode_table(2018-05-17
06:22:08 -0400)are available in the Git repository at:
git://linuxtv.org/pinchartl/media.gitv4l2/vsp1/nextfor 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.
Quoted 7 lines
Quoted 4 lines
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
Quoted 59 lines
Quoted 4 lines
To the already existing one:
drivers/media/platform/vsp1/vsp1_drm.c:336vsp1_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 ?
Quoted 50 lines
(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.
Quoted 44 lines
----------------------------------------------------------------
Geert Uytterhoeven (1):
media: vsp1: Drop OF dependency ofVIDEO_RENESAS_VSP1Kieran 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 dlbdrivers/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(-)