Em Sat, 26 May 2018 03:24:00 +0300
Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx> 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
