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