Re: [PATCH] media: vsp1: cleanup a false positive warning

[Mauro Carvalho Chehab <mchehab+samsung@kernel.org>]

Em Mon, 07 May 2018 17:05:24 +0300
Laurent Pinchart <laurent.pinchart@ideasonboard.com> escreveu:

> Hi Mauro,
> 
> Thank you for the patch.
> 
> On Friday, 4 May 2018 15:13:58 EEST Mauro Carvalho Chehab wrote:
> > With the new vsp1 code changes introduced by changeset
> > f81f9adc4ee1 ("media: v4l: vsp1: Assign BRU and BRS to pipelines
> > dynamically"), smatch complains with:
> > 	drivers/media/platform/vsp1/vsp1_drm.c:262 vsp1_du_pipeline_setup_bru()
> > error: we previously assumed 'pipe->bru' could be null (see line 180)
> > 
> > This is a false positive, as, if pipe->bru is NULL, the brx
> > var will be different, with ends by calling a code that will
> > set pipe->bru to another value.
> > 
> > Yet, cleaning this false positive is as easy as adding an explicit
> > check if pipe->bru is NULL.  
> 
> It's not very difficult indeed, but it really is a false positive. I think the 
> proposed change decreases readability, the condition currently reads as "if 
> (new brx != old brx)", why does smatch even flag that as an error ?

I've no idea. Never studied smatch code. If you don't think that
this is a fix for it, do you have an alternative patch (either to
smatch or to vsp1)?

Regards,
Mauro

> 
> > Signed-off-by: Mauro Carvalho Chehab <mchehab+samsung@kernel.org>
> > ---
> >  drivers/media/platform/vsp1/vsp1_drm.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > 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. */  
> 



Thanks,
Mauro