[PATCH] [RFC] media: camss-vfe: always initialize reg at vfe_set_xbar_cfg()

[PATCH] [RFC] media: camss-vfe: always initialize reg at vfe_set_xbar_cfg()

[Mauro Carvalho Chehab]

if output->wm_num is bigger than 1, the value for reg is
not initialized, as warned by smatch:
	drivers/media/platform/qcom/camss-8x16/camss-vfe.c:633 vfe_set_xbar_cfg() error: uninitialized symbol 'reg'.
	drivers/media/platform/qcom/camss-8x16/camss-vfe.c:637 vfe_set_xbar_cfg() error: uninitialized symbol 'reg'.

I didn't check the logic into its details, but there is at least
one point where wm_num is made equal to two. So, something
seem broken.

For now, I just reset it to zero, and added a FIXME. Hopefully,
the driver authors will know if this is OK, or if something else
is needed there.

Signed-off-by: Mauro Carvalho Chehab <mchehab@s-opensource.com>
---
 drivers/media/platform/qcom/camss-8x16/camss-vfe.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/media/platform/qcom/camss-8x16/camss-vfe.c b/drivers/media/platform/qcom/camss-8x16/camss-vfe.c
index b22d2dfcd3c2..388431f747fa 100644
--- a/drivers/media/platform/qcom/camss-8x16/camss-vfe.c
+++ b/drivers/media/platform/qcom/camss-8x16/camss-vfe.c
@@ -622,6 +622,8 @@ static void vfe_set_xbar_cfg(struct vfe_device *vfe, struct vfe_output *output,
 			reg = VFE_0_BUS_XBAR_CFG_x_M_PAIR_STREAM_EN;
 			if (p == V4L2_PIX_FMT_NV12 || p == V4L2_PIX_FMT_NV16)
 				reg |= VFE_0_BUS_XBAR_CFG_x_M_PAIR_STREAM_SWAP_INTER_INTRA;
+		} else {
+			reg = 0;	/* FIXME: is it the right value for i > 1? */
 		}
 
 		if (output->wm_idx[i] % 2 == 1)
-- 
2.13.6

Re: [PATCH] [RFC] media: camss-vfe: always initialize reg at vfe_set_xbar_cfg()

[Todor Tomov]

Hi Mauro,

Thank you for pointing to this.

On  1.11.2017 14:16, Mauro Carvalho Chehab wrote:
> if output->wm_num is bigger than 1, the value for reg is
If output->wn_num equals 2, we handle all cases (i == 0, i == 1) and set reg properly.
If output->wn_num is bigger than 2, then reg will not be initialized. However this is something that "cannot happen" and because of this the case is not handled.

So I think that there is nothing wrong really but we have to do something to remove the warning. I agree with your patch, it is technically not a right value for reg but any cases in which wm_num is bigger than 2 are not supported anyway and should not happen.

> not initialized, as warned by smatch:
> 	drivers/media/platform/qcom/camss-8x16/camss-vfe.c:633 vfe_set_xbar_cfg() error: uninitialized symbol 'reg'.
> 	drivers/media/platform/qcom/camss-8x16/camss-vfe.c:637 vfe_set_xbar_cfg() error: uninitialized symbol 'reg'.
> 
> I didn't check the logic into its details, but there is at least
> one point where wm_num is made equal to two. So, something
> seem broken.
> 
> For now, I just reset it to zero, and added a FIXME. Hopefully,
> the driver authors will know if this is OK, or if something else
> is needed there.
> 
> Signed-off-by: Mauro Carvalho Chehab <mchehab@s-opensource.com>
> ---
>  drivers/media/platform/qcom/camss-8x16/camss-vfe.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/media/platform/qcom/camss-8x16/camss-vfe.c b/drivers/media/platform/qcom/camss-8x16/camss-vfe.c
> index b22d2dfcd3c2..388431f747fa 100644
> --- a/drivers/media/platform/qcom/camss-8x16/camss-vfe.c
> +++ b/drivers/media/platform/qcom/camss-8x16/camss-vfe.c
> @@ -622,6 +622,8 @@ static void vfe_set_xbar_cfg(struct vfe_device *vfe, struct vfe_output *output,
>  			reg = VFE_0_BUS_XBAR_CFG_x_M_PAIR_STREAM_EN;
>  			if (p == V4L2_PIX_FMT_NV12 || p == V4L2_PIX_FMT_NV16)
>  				reg |= VFE_0_BUS_XBAR_CFG_x_M_PAIR_STREAM_SWAP_INTER_INTRA;
> +		} else {
> +			reg = 0;	/* FIXME: is it the right value for i > 1? */
>  		}
>  
>  		if (output->wm_idx[i] % 2 == 1)
> 

-- 
Best regards,
Todor Tomov

Re: [PATCH] [RFC] media: camss-vfe: always initialize reg at vfe_set_xbar_cfg()

[Mauro Carvalho Chehab]

Hi Todor,

Em Wed, 1 Nov 2017 14:38:02 +0200
Todor Tomov <todor.tomov@linaro.org> escreveu:

> Hi Mauro,
> 
> Thank you for pointing to this.
> 
> On  1.11.2017 14:16, Mauro Carvalho Chehab wrote:
> > if output->wm_num is bigger than 1, the value for reg is  
> If output->wn_num equals 2, we handle all cases (i == 0, i == 1) and set reg properly.
> If output->wn_num is bigger than 2, then reg will not be initialized. However this is something that "cannot happen" and because of this the case is not handled.
> 
> So I think that there is nothing wrong really but we have to do something to remove the warning. I agree with your patch, it is technically not a right value for reg but any cases in which wm_num is bigger than 2 are not supported anyway and should not happen.

Thanks for your promptly answer. Well, if i is always at the [0..1] range,
then I guess the enclosed patch is actually better.


Thanks,
Mauro


[PATCH] media: camss-vfe: always initialize reg at vfe_set_xbar_cfg()

if output->wm_num is bigger than 2, the value for reg is
not initialized, as warned by smatch:
	drivers/media/platform/qcom/camss-8x16/camss-vfe.c:633 vfe_set_xbar_cfg() error: uninitialized symbol 'reg'.
	drivers/media/platform/qcom/camss-8x16/camss-vfe.c:637 vfe_set_xbar_cfg() error: uninitialized symbol 'reg'.

That shouldn't happen in practice, so add a logic that will
break the loop if i > 1, fixing the warnings.

Signed-off-by: Mauro Carvalho Chehab <mchehab@s-opensource.com>

diff --git a/drivers/media/platform/qcom/camss-8x16/camss-vfe.c b/drivers/media/platform/qcom/camss-8x16/camss-vfe.c
index b22d2dfcd3c2..55232a912950 100644
--- a/drivers/media/platform/qcom/camss-8x16/camss-vfe.c
+++ b/drivers/media/platform/qcom/camss-8x16/camss-vfe.c
@@ -622,6 +622,9 @@ static void vfe_set_xbar_cfg(struct vfe_device *vfe, struct vfe_output *output,
 			reg = VFE_0_BUS_XBAR_CFG_x_M_PAIR_STREAM_EN;
 			if (p == V4L2_PIX_FMT_NV12 || p == V4L2_PIX_FMT_NV16)
 				reg |= VFE_0_BUS_XBAR_CFG_x_M_PAIR_STREAM_SWAP_INTER_INTRA;
+		} else {
+			/* On current devices output->wm_num is always <= 2 */
+			break;
 		}
 
 		if (output->wm_idx[i] % 2 == 1)

Re: [PATCH] [RFC] media: camss-vfe: always initialize reg at vfe_set_xbar_cfg()

[Todor Tomov]

On  1.11.2017 15:03, Mauro Carvalho Chehab wrote:
> Hi Todor,
> 
> Em Wed, 1 Nov 2017 14:38:02 +0200
> Todor Tomov <todor.tomov@linaro.org> escreveu:
> 
>> Hi Mauro,
>>
>> Thank you for pointing to this.
>>
>> On  1.11.2017 14:16, Mauro Carvalho Chehab wrote:
>>> if output->wm_num is bigger than 1, the value for reg is  
>> If output->wn_num equals 2, we handle all cases (i == 0, i == 1) and set reg properly.
>> If output->wn_num is bigger than 2, then reg will not be initialized. However this is something that "cannot happen" and because of this the case is not handled.
>>
>> So I think that there is nothing wrong really but we have to do something to remove the warning. I agree with your patch, it is technically not a right value for reg but any cases in which wm_num is bigger than 2 are not supported anyway and should not happen.
> 
> Thanks for your promptly answer. Well, if i is always at the [0..1] range,
> then I guess the enclosed patch is actually better.

I don't think that there is a lot of difference practically. If this one fixes the warning too, then it is fine for me. Thank you for working on this.

Best regards,
Todor

> 
> 
> Thanks,
> Mauro
> 
> 
> [PATCH] media: camss-vfe: always initialize reg at vfe_set_xbar_cfg()
> 
> if output->wm_num is bigger than 2, the value for reg is
> not initialized, as warned by smatch:
> 	drivers/media/platform/qcom/camss-8x16/camss-vfe.c:633 vfe_set_xbar_cfg() error: uninitialized symbol 'reg'.
> 	drivers/media/platform/qcom/camss-8x16/camss-vfe.c:637 vfe_set_xbar_cfg() error: uninitialized symbol 'reg'.
> 
> That shouldn't happen in practice, so add a logic that will
> break the loop if i > 1, fixing the warnings.
> 
> Signed-off-by: Mauro Carvalho Chehab <mchehab@s-opensource.com>
> 
> diff --git a/drivers/media/platform/qcom/camss-8x16/camss-vfe.c b/drivers/media/platform/qcom/camss-8x16/camss-vfe.c
> index b22d2dfcd3c2..55232a912950 100644
> --- a/drivers/media/platform/qcom/camss-8x16/camss-vfe.c
> +++ b/drivers/media/platform/qcom/camss-8x16/camss-vfe.c
> @@ -622,6 +622,9 @@ static void vfe_set_xbar_cfg(struct vfe_device *vfe, struct vfe_output *output,
>  			reg = VFE_0_BUS_XBAR_CFG_x_M_PAIR_STREAM_EN;
>  			if (p == V4L2_PIX_FMT_NV12 || p == V4L2_PIX_FMT_NV16)
>  				reg |= VFE_0_BUS_XBAR_CFG_x_M_PAIR_STREAM_SWAP_INTER_INTRA;
> +		} else {
> +			/* On current devices output->wm_num is always <= 2 */
> +			break;
>  		}
>  
>  		if (output->wm_idx[i] % 2 == 1)
> 

Re: [PATCH] [RFC] media: camss-vfe: always initialize reg at vfe_set_xbar_cfg()

[Mauro Carvalho Chehab]

Em Wed, 1 Nov 2017 15:09:36 +0200
Todor Tomov <todor.tomov@linaro.org> escreveu:

> On  1.11.2017 15:03, Mauro Carvalho Chehab wrote:
> > Hi Todor,
> > 
> > Em Wed, 1 Nov 2017 14:38:02 +0200
> > Todor Tomov <todor.tomov@linaro.org> escreveu:
> >   
> >> Hi Mauro,
> >>
> >> Thank you for pointing to this.
> >>
> >> On  1.11.2017 14:16, Mauro Carvalho Chehab wrote:  
> >>> if output->wm_num is bigger than 1, the value for reg is    
> >> If output->wn_num equals 2, we handle all cases (i == 0, i == 1) and set reg properly.
> >> If output->wn_num is bigger than 2, then reg will not be initialized. However this is something that "cannot happen" and because of this the case is not handled.
> >>
> >> So I think that there is nothing wrong really but we have to do something to remove the warning. I agree with your patch, it is technically not a right value for reg but any cases in which wm_num is bigger than 2 are not supported anyway and should not happen.  
> > 
> > Thanks for your promptly answer. Well, if i is always at the [0..1] range,
> > then I guess the enclosed patch is actually better.  
> 
> I don't think that there is a lot of difference practically. If this one fixes the warning too, then it is fine for me. Thank you for working on this.

Yes, it fixes the warning. I guess the main advantage of version 2 is
that, if it would ever be possible to have more than 2 outputs, it
should be clearer that the logic at break need changes.

So, if ok for you, I'll stick with version 2. It would be great if
you could give your ack on that.

Regards,
Mauro

Re: [PATCH] [RFC] media: camss-vfe: always initialize reg at vfe_set_xbar_cfg()

[Todor Tomov]

On  1.11.2017 15:25, Mauro Carvalho Chehab wrote:
> Em Wed, 1 Nov 2017 15:09:36 +0200
> Todor Tomov <todor.tomov@linaro.org> escreveu:
> 
>> On  1.11.2017 15:03, Mauro Carvalho Chehab wrote:
>>> Hi Todor,
>>>
>>> Em Wed, 1 Nov 2017 14:38:02 +0200
>>> Todor Tomov <todor.tomov@linaro.org> escreveu:
>>>   
>>>> Hi Mauro,
>>>>
>>>> Thank you for pointing to this.
>>>>
>>>> On  1.11.2017 14:16, Mauro Carvalho Chehab wrote:  
>>>>> if output->wm_num is bigger than 1, the value for reg is    
>>>> If output->wn_num equals 2, we handle all cases (i == 0, i == 1) and set reg properly.
>>>> If output->wn_num is bigger than 2, then reg will not be initialized. However this is something that "cannot happen" and because of this the case is not handled.
>>>>
>>>> So I think that there is nothing wrong really but we have to do something to remove the warning. I agree with your patch, it is technically not a right value for reg but any cases in which wm_num is bigger than 2 are not supported anyway and should not happen.  
>>>
>>> Thanks for your promptly answer. Well, if i is always at the [0..1] range,
>>> then I guess the enclosed patch is actually better.  
>>
>> I don't think that there is a lot of difference practically. If this one fixes the warning too, then it is fine for me. Thank you for working on this.
> 
> Yes, it fixes the warning. I guess the main advantage of version 2 is
> that, if it would ever be possible to have more than 2 outputs, it
> should be clearer that the logic at break need changes.
> 
> So, if ok for you, I'll stick with version 2. It would be great if
> you could give your ack on that.

Sure, and you can add:
Acked-by: Todor Tomov <todor.tomov@linaro.org>

Thank you.
Todor