[media-s3c-camif] question about arguments position

[media-s3c-camif] question about arguments position

[Gustavo A. R. Silva]

Hello everybody,

While looking into Coverity ID 1248800 I ran into the following piece  
of code at drivers/media/platform/s3c-camif/camif-capture.c:67:

/* Locking: called with camif->slock spinlock held */
static int s3c_camif_hw_init(struct camif_dev *camif, struct camif_vp *vp)
{
         const struct s3c_camif_variant *variant = camif->variant;

         if (camif->sensor.sd == NULL || vp->out_fmt == NULL)
                 return -EINVAL;

         if (variant->ip_revision == S3C244X_CAMIF_IP_REV)
                 camif_hw_clear_fifo_overflow(vp);
         camif_hw_set_camera_bus(camif);
         camif_hw_set_source_format(camif);
         camif_hw_set_camera_crop(camif);
         camif_hw_set_test_pattern(camif, camif->test_pattern);
         if (variant->has_img_effect)
                 camif_hw_set_effect(camif, camif->colorfx,
                                 camif->colorfx_cb, camif->colorfx_cr);
         if (variant->ip_revision == S3C6410_CAMIF_IP_REV)
                 camif_hw_set_input_path(vp);
         camif_cfg_video_path(vp);
         vp->state &= ~ST_VP_CONFIG;

         return 0;
}


The issue here is that the position of arguments in the call to  
camif_hw_set_effect() function do not match the order of the parameters:

camif->colorfx_cb is passed to cr
camif->colorfx_cr is passed to cb

This is the function prototype:

void camif_hw_set_effect(struct camif_dev *camif, unsigned int effect,
			unsigned int cr, unsigned int cb)

My question here is if this is intentional?

In case it is not, I will send a patch to fix it. But first it would  
be great to hear any comment about it.

By the way... the same is happening at  
drivers/media/platform/s3c-camif/camif-capture.c:366

Thank you
--
Gustavo A. R. Silva

Re: [media-s3c-camif] question about arguments position

[Sylwester Nawrocki]

Hi Gustavo,

On 05/04/2017 09:05 PM, Gustavo A. R. Silva wrote:
> The issue here is that the position of arguments in the call to
> camif_hw_set_effect() function do not match the order of the parameters:
> 
> camif->colorfx_cb is passed to cr
> camif->colorfx_cr is passed to cb
> 
> This is the function prototype:
> 
> void camif_hw_set_effect(struct camif_dev *camif, unsigned int effect,
>             unsigned int cr, unsigned int cb)
> 
> My question here is if this is intentional?
> 
> In case it is not, I will send a patch to fix it. But first it would be
> great to hear any comment about it.

You are right, it seems you have found a real bug. Feel free to send a patch.
The best thing to do now might be to change the function prototype to:

void camif_hw_set_effect(struct camif_dev *camif, unsigned int effect,
             unsigned int cb, unsigned int cr)

--
Regards,
Sylwester

Re: [media-s3c-camif] question about arguments position

[Gustavo A. R. Silva]

Hello Sylwester,

Quoting Sylwester Nawrocki <sylvester.nawrocki@gmail.com>:

> Hi Gustavo,
>
> On 05/04/2017 09:05 PM, Gustavo A. R. Silva wrote:
>> The issue here is that the position of arguments in the call to
>> camif_hw_set_effect() function do not match the order of the parameters:
>>
>> camif->colorfx_cb is passed to cr
>> camif->colorfx_cr is passed to cb
>>
>> This is the function prototype:
>>
>> void camif_hw_set_effect(struct camif_dev *camif, unsigned int effect,
>>             unsigned int cr, unsigned int cb)
>>
>> My question here is if this is intentional?
>>
>> In case it is not, I will send a patch to fix it. But first it would be
>> great to hear any comment about it.
>
> You are right, it seems you have found a real bug. Feel free to send a patch.
> The best thing to do now might be to change the function prototype to:
>
> void camif_hw_set_effect(struct camif_dev *camif, unsigned int effect,
>              unsigned int cb, unsigned int cr)
>

OK, I'll send a patch for this shortly.

Thanks for clarifying.
--
Gustavo A. R. Silva

[PATCH] media: platform: s3c-camif: fix function prototype

[Gustavo A. R. Silva]

Fix function prototype so the position of arguments camif->colorfx_cb and
camif->colorfx_cr match the order of the parameters when calling
camif_hw_set_effect() function.

Addresses-Coverity-ID: 1248800
Addresses-Coverity-ID: 1269141
Cc: Sylwester Nawrocki <sylvester.nawrocki@gmail.com>
Signed-off-by: Gustavo A. R. Silva <garsilva@embeddedor.com>
---
 drivers/media/platform/s3c-camif/camif-regs.c | 2 +-
 drivers/media/platform/s3c-camif/camif-regs.h | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/media/platform/s3c-camif/camif-regs.c b/drivers/media/platform/s3c-camif/camif-regs.c
index 812fb3a..d70ffef 100644
--- a/drivers/media/platform/s3c-camif/camif-regs.c
+++ b/drivers/media/platform/s3c-camif/camif-regs.c
@@ -58,7 +58,7 @@ void camif_hw_set_test_pattern(struct camif_dev *camif, unsigned int pattern)
 }
 
 void camif_hw_set_effect(struct camif_dev *camif, unsigned int effect,
-			unsigned int cr, unsigned int cb)
+			unsigned int cb, unsigned int cr)
 {
 	static const struct v4l2_control colorfx[] = {
 		{ V4L2_COLORFX_NONE,		CIIMGEFF_FIN_BYPASS },
diff --git a/drivers/media/platform/s3c-camif/camif-regs.h b/drivers/media/platform/s3c-camif/camif-regs.h
index 5ad36c1..dfb49a5 100644
--- a/drivers/media/platform/s3c-camif/camif-regs.h
+++ b/drivers/media/platform/s3c-camif/camif-regs.h
@@ -255,7 +255,7 @@ void camif_hw_set_output_dma(struct camif_vp *vp);
 void camif_hw_set_target_format(struct camif_vp *vp);
 void camif_hw_set_test_pattern(struct camif_dev *camif, unsigned int pattern);
 void camif_hw_set_effect(struct camif_dev *camif, unsigned int effect,
-			unsigned int cr, unsigned int cb);
+			unsigned int cb, unsigned int cr);
 void camif_hw_set_output_addr(struct camif_vp *vp, struct camif_addr *paddr,
 			      int index);
 void camif_hw_dump_regs(struct camif_dev *camif, const char *label);
-- 
2.5.0

Re: [PATCH] media: platform: s3c-camif: fix function prototype

[Hans Verkuil]

On 05/04/2017 11:42 PM, Gustavo A. R. Silva wrote:
> Fix function prototype so the position of arguments camif->colorfx_cb and
> camif->colorfx_cr match the order of the parameters when calling
> camif_hw_set_effect() function.
> 
> Addresses-Coverity-ID: 1248800
> Addresses-Coverity-ID: 1269141
> Cc: Sylwester Nawrocki <sylvester.nawrocki@gmail.com>
> Signed-off-by: Gustavo A. R. Silva <garsilva@embeddedor.com>
> ---
>  drivers/media/platform/s3c-camif/camif-regs.c | 2 +-
>  drivers/media/platform/s3c-camif/camif-regs.h | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/platform/s3c-camif/camif-regs.c b/drivers/media/platform/s3c-camif/camif-regs.c
> index 812fb3a..d70ffef 100644
> --- a/drivers/media/platform/s3c-camif/camif-regs.c
> +++ b/drivers/media/platform/s3c-camif/camif-regs.c
> @@ -58,7 +58,7 @@ void camif_hw_set_test_pattern(struct camif_dev *camif, unsigned int pattern)
>  }
>  
>  void camif_hw_set_effect(struct camif_dev *camif, unsigned int effect,
> -			unsigned int cr, unsigned int cb)
> +			unsigned int cb, unsigned int cr)
>  {
>  	static const struct v4l2_control colorfx[] = {
>  		{ V4L2_COLORFX_NONE,		CIIMGEFF_FIN_BYPASS },

This will also affect this line:

cfg |= cr | (cb << 13);

cr and cb are now swapped so this will result in a different color.

Sylwester, who is wrong here: the prototype or how this function is called?

I suspect that Gustavo is right and that the prototype is wrong. But in that
case this patch should also change the cfg assignment.

Regards,

	Hans

> diff --git a/drivers/media/platform/s3c-camif/camif-regs.h b/drivers/media/platform/s3c-camif/camif-regs.h
> index 5ad36c1..dfb49a5 100644
> --- a/drivers/media/platform/s3c-camif/camif-regs.h
> +++ b/drivers/media/platform/s3c-camif/camif-regs.h
> @@ -255,7 +255,7 @@ void camif_hw_set_output_dma(struct camif_vp *vp);
>  void camif_hw_set_target_format(struct camif_vp *vp);
>  void camif_hw_set_test_pattern(struct camif_dev *camif, unsigned int pattern);
>  void camif_hw_set_effect(struct camif_dev *camif, unsigned int effect,
> -			unsigned int cr, unsigned int cb);
> +			unsigned int cb, unsigned int cr);
>  void camif_hw_set_output_addr(struct camif_vp *vp, struct camif_addr *paddr,
>  			      int index);
>  void camif_hw_dump_regs(struct camif_dev *camif, const char *label);
> 

Re: [PATCH] media: platform: s3c-camif: fix function prototype

[Sylwester Nawrocki]

On 05/22/2017 11:02 AM, Hans Verkuil wrote:
>> --- a/drivers/media/platform/s3c-camif/camif-regs.c
>> +++ b/drivers/media/platform/s3c-camif/camif-regs.c
>> @@ -58,7 +58,7 @@ void camif_hw_set_test_pattern(struct camif_dev *camif, unsigned int pattern)
>>   }
>>   
>>   void camif_hw_set_effect(struct camif_dev *camif, unsigned int effect,
>> -			unsigned int cr, unsigned int cb)
>> +			unsigned int cb, unsigned int cr)
>>   {
>>   	static const struct v4l2_control colorfx[] = {
>>   		{ V4L2_COLORFX_NONE,		CIIMGEFF_FIN_BYPASS },
>
> This will also affect this line:
> 
> cfg |= cr | (cb << 13);
> 
> cr and cb are now swapped so this will result in a different color.
> 
> Sylwester, who is wrong here: the prototype or how this function is called?
> 
> I suspect that Gustavo is right and that the prototype is wrong. But in that
> case this patch should also change the cfg assignment.

The function is currently called in a wrong way, it's clear from looking
at the prototype. CR should end up in bits 0:7 and CR in bits 20:13 of
the register. So yes, colour will change after applying the patch - to the
expected one, matching the user API documentation.

Unfortunately I can't test it because I have only the s3c2440 SoC based 
evaluation board where the image effect is not supported.

Probably a more straightforward fix would be to amend the callers (apologies
Gustavo for misleading suggestions).  But I'm inclined to apply the $subject 
patch as is to just close this bug report case.

--
Regards,
Sylwester

[PATCH] media: platform: s3c-camif: fix arguments position in function call

[Gustavo A. R. Silva]

Hi Sylwester,

Here is another patch in case you decide that it is
better to apply this one.

Thanks
--
Gustavo A. R. Silva


==========

Fix the position of the arguments in function call.

Addresses-Coverity-ID: 1248800
Addresses-Coverity-ID: 1269141
Signed-off-by: Gustavo A. R. Silva <garsilva@embeddedor.com>
---
 drivers/media/platform/s3c-camif/camif-capture.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/media/platform/s3c-camif/camif-capture.c b/drivers/media/platform/s3c-camif/camif-capture.c
index 1b30be72..25c7a7d 100644
--- a/drivers/media/platform/s3c-camif/camif-capture.c
+++ b/drivers/media/platform/s3c-camif/camif-capture.c
@@ -80,7 +80,7 @@ static int s3c_camif_hw_init(struct camif_dev *camif, struct camif_vp *vp)
 	camif_hw_set_test_pattern(camif, camif->test_pattern);
 	if (variant->has_img_effect)
 		camif_hw_set_effect(camif, camif->colorfx,
-				camif->colorfx_cb, camif->colorfx_cr);
+				camif->colorfx_cr, camif->colorfx_cb);
 	if (variant->ip_revision == S3C6410_CAMIF_IP_REV)
 		camif_hw_set_input_path(vp);
 	camif_cfg_video_path(vp);
@@ -364,7 +364,7 @@ irqreturn_t s3c_camif_irq_handler(int irq, void *priv)
 		camif_hw_set_test_pattern(camif, camif->test_pattern);
 		if (camif->variant->has_img_effect)
 			camif_hw_set_effect(camif, camif->colorfx,
-				    camif->colorfx_cb, camif->colorfx_cr);
+				    camif->colorfx_cr, camif->colorfx_cb);
 		vp->state &= ~ST_VP_CONFIG;
 	}
 unlock:
-- 
2.5.0

Re: [PATCH] media: platform: s3c-camif: fix arguments position in function call

[Sylwester Nawrocki]

On 06/02/2017 05:43 AM, Gustavo A. R. Silva wrote:
> Hi Sylwester,
> 
> Here is another patch in case you decide that it is
> better to apply this one.

Thanks, I applied this patch.  In future please put any comments only after
the scissors ("---") line, the comments can be then discarded automatically
and there will be no need for manually editing the patch before applying.

--
Regards,
Sylwester

> Fix the position of the arguments in function call.
> 
> Addresses-Coverity-ID: 1248800
> Addresses-Coverity-ID: 1269141
> Signed-off-by: Gustavo A. R. Silva <garsilva@embeddedor.com>
> ---
^^^^^

>   drivers/media/platform/s3c-camif/camif-capture.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/platform/s3c-camif/camif-capture.c b/drivers/media/platform/s3c-camif/camif-capture.c
> index 1b30be72..25c7a7d 100644
> --- a/drivers/media/platform/s3c-camif/camif-capture.c
> +++ b/drivers/media/platform/s3c-camif/camif-capture.c
> @@ -80,7 +80,7 @@ static int s3c_camif_hw_init(struct camif_dev *camif, struct camif_vp *vp)
>   	camif_hw_set_test_pattern(camif, camif->test_pattern);
>   	if (variant->has_img_effect)
>   		camif_hw_set_effect(camif, camif->colorfx,
> -				camif->colorfx_cb, camif->colorfx_cr);
> +				camif->colorfx_cr, camif->colorfx_cb);
>   	if (variant->ip_revision == S3C6410_CAMIF_IP_REV)
>   		camif_hw_set_input_path(vp);
>   	camif_cfg_video_path(vp);
> @@ -364,7 +364,7 @@ irqreturn_t s3c_camif_irq_handler(int irq, void *priv)
>   		camif_hw_set_test_pattern(camif, camif->test_pattern);
>   		if (camif->variant->has_img_effect)
>   			camif_hw_set_effect(camif, camif->colorfx,
> -				    camif->colorfx_cb, camif->colorfx_cr);
> +				    camif->colorfx_cr, camif->colorfx_cb);
>   		vp->state &= ~ST_VP_CONFIG;
>   	}
>   unlock:

Re: [PATCH] media: platform: s3c-camif: fix arguments position in function call

[Gustavo A. R. Silva]

Hi Sylwester,

Quoting Sylwester Nawrocki <s.nawrocki@samsung.com>:

> On 06/02/2017 05:43 AM, Gustavo A. R. Silva wrote:
>> Hi Sylwester,
>>
>> Here is another patch in case you decide that it is
>> better to apply this one.
>
> Thanks, I applied this patch.  In future please put any comments only after
> the scissors ("---") line, the comments can be then discarded automatically
> and there will be no need for manually editing the patch before applying.
>

OK, I will do that.

> --
> Regards,
> Sylwester
>
>> Fix the position of the arguments in function call.
>>
>> Addresses-Coverity-ID: 1248800
>> Addresses-Coverity-ID: 1269141
>> Signed-off-by: Gustavo A. R. Silva <garsilva@embeddedor.com>
>> ---
> ^^^^^
>

I got it.

Thank you
--
Gustavo A. R. Silva