Em Sat, 26 May 2018 03:24:00 +0300
Laurent Pinchart <laurent.pinch...@ideasonboard.com> 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 = &vsp1->bru->entity;
> 30. else
> 31. brx = &vsp1->brs->entity;
> 32.
> 33. if (brx != pipe->brx)
> 34. pipe->brx = brx;
> 35.
> 36. return pipe->brx->source_pad;
> 37. }
>
> The reason why smatch complains is that it has no guarantee that vsp1->brs is
> not NULL. It's quite tricky:
>
> - On line 26, smatch assumes that pipe->brx can be NULL
> - On line 27, brx is assigned a non-NULL value (as pipe->brx is not NULL due
> to line 26)
> - On line 28, smatch assumes that vsp1->bru is not NULL
> - On line 29, brx is assigned a non-NULL value (as vsp1->bru is not NULL due
> to line 28)
> - On line 31, brx is assigned a possibly NULL value (as there's no
> information
> regarding vsp1->brs)
> - On line 34, pipe->brx is not assigned a non-NULL value if brx is NULL
> - On line 36 pipe->brx is dereferenced
>
> The problem comes from the fact that smatch assumes that vsp1->brs isn't
> NULL.
> Adding a "(void)vsp1->brs->entity;" statement on line 25 makes the warning
> disappear.
>
> So how do we know that vsp1->brs isn't NULL in the original code ?
>
> if (pipe->num_inputs > 2)
> brx = &vsp1->bru->entity;
> else if (pipe->brx && !drm_pipe->force_brx_release)
> brx = pipe->brx;
> else if (!vsp1->bru->entity.pipe)
> brx = &vsp1->bru->entity;
> else
> brx = &vsp1->brs->entity;
>
> A VSP1 instance can have no brs, so in general vsp1->brs can be NULL.
> However,
> when that's the case, the following conditions are fulfilled.
>
> - drm_pipe->force_brx_release will be false
> - either pipe->brx will be non-NULL, or vsp1->bru->entity.pipe will be NULL
>
> The fourth branch should thus never be taken.
I don't think that adding a forth branch there would solve.
The thing is that Smatch knows that pipe->brx can be NULL, as the function
explicly checks if pipe->brx != NULL.
When Smatch handles this if:
if (brx != pipe->brx) {
It wrongly assumes that this could be false if pipe->brx is NULL.
I don't know why, as Smatch should know that brx can't be NULL.
On such case, the next code to be executed would be:
format.pad = pipe->brx->source_pad;
With would be trying to de-ref a NULL pointer.
There are two ways to fix it:
1) with my patch.
It is based to the fact that, if pipe->brx is null, then brx won't be
NULL. So, the logic that "Switch BRx if needed." will always be called:
diff --git a/drivers/media/platform/vsp1/vsp1_drm.c
b/drivers/media/platform/vsp1/vsp1_drm.c
index 095dc48aa25a..cb6b60843400 100644
--- a/drivers/media/platform/vsp1/vsp1_drm.c
+++ b/drivers/media/platform/vsp1/vsp1_drm.c
@@ -185,7 +185,7 @@ static int vsp1_du_pipeline_setup_brx(struct vsp1_device
*vsp1,
brx = &vsp1->brs->entity;
/* Switch BRx if needed. */
- if (brx != pipe->brx) {
+ if (brx != pipe->brx || !pipe->brx) {
struct vsp1_entity *released_brx = NULL;
/* Release our BRx if we have one. */
The code with switches BRx ensures that pipe->brx won't be null, as
in the end, it sets:
pipe->brx = brx;
And brx can't be NULL.
From my PoV, this patch has the advantage of explicitly showing
to humans that the code inside the if statement will always be
executed when pipe->brx is NULL.
-
Another way to solve would be to explicitly check if pipe->brx is still
null before de-referencing:
diff --git a/drivers/media/platform/vsp1/vsp1_drm.c
b/drivers/media/platform/vsp1/vsp1_drm.c
index edb35a5c57ea..9fe063d6df31 100644
--- a/drivers/media/platform/vsp1/vsp1_drm.c
+++ b/drivers/media/platform/vsp1/vsp1_drm.c
@@ -327,6 +327,9 @@ static int vsp1_du_pipeline_setup_brx(struct vsp1_device
*vsp1,
list_add_tail(&pipe->brx->list_pipe, &pipe->entities);
}
+ if (!pipe->brx)
+ return -EINVAL;
+
/*
* Configure the format on the BRx source and verify that it matches the
* requested format. We don't set the media bus code as it is configured
The right fix would be, instead, to fix Smatch to handle the:
if (brx != pipe->brx)
for the cases where one var can be NULL while the other can't be NULL,
but, as I said before, I suspect that this can be a way more complex.
Thanks,
Mauro
