[PATCH] v4l2-dv-timings: fix overflow in gtf timings calculation

2015-05-04 Thread Prashant Laddha
The intermediate calculation in the expression for hblank can exceed
32 bit signed range. This overflow can lead to negative values for
hblank. Typecasting intermediate variable to higher precision.

Cc: Hans Verkuil 
Cc: Martin Bugge 
Signed-off-by: Prashant Laddha 
---
 drivers/media/v4l2-core/v4l2-dv-timings.c | 16 
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/media/v4l2-core/v4l2-dv-timings.c 
b/drivers/media/v4l2-core/v4l2-dv-timings.c
index 86e11d1..9d27f05 100644
--- a/drivers/media/v4l2-core/v4l2-dv-timings.c
+++ b/drivers/media/v4l2-core/v4l2-dv-timings.c
@@ -573,15 +573,15 @@ bool v4l2_detect_gtf(unsigned frame_height,
 
/* Horizontal */
if (default_gtf)
-   h_blank = ((image_width * GTF_D_C_PRIME * hfreq) -
-   (image_width * GTF_D_M_PRIME * 1000) +
-   (hfreq * (100 - GTF_D_C_PRIME) + GTF_D_M_PRIME * 1000) 
/ 2) /
-   (hfreq * (100 - GTF_D_C_PRIME) + GTF_D_M_PRIME * 1000);
+   h_blank = ((image_width * GTF_D_C_PRIME * (long long)hfreq) -
+ ((long long)image_width * GTF_D_M_PRIME * 1000) +
+ ((long long)hfreq * (100 - GTF_D_C_PRIME) + 
GTF_D_M_PRIME * 1000) / 2) /
+ ((long long)hfreq * (100 - GTF_D_C_PRIME) + 
GTF_D_M_PRIME * 1000);
else
-   h_blank = ((image_width * GTF_S_C_PRIME * hfreq) -
-   (image_width * GTF_S_M_PRIME * 1000) +
-   (hfreq * (100 - GTF_S_C_PRIME) + GTF_S_M_PRIME * 1000) 
/ 2) /
-   (hfreq * (100 - GTF_S_C_PRIME) + GTF_S_M_PRIME * 1000);
+   h_blank = ((image_width * GTF_S_C_PRIME * (long long)hfreq) -
+ ((long long)image_width * GTF_S_M_PRIME * 1000) +
+ ((long long)hfreq * (100 - GTF_S_C_PRIME) + 
GTF_S_M_PRIME * 1000) / 2) /
+ ((long long)hfreq * (100 - GTF_S_C_PRIME) + 
GTF_S_M_PRIME * 1000);
 
h_blank = ((h_blank + GTF_CELL_GRAN) / (2 * GTF_CELL_GRAN)) *
  (2 * GTF_CELL_GRAN);
-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH] v4l2-dv-timings: fix overflow in gtf timings calculation

2015-05-04 Thread Hans Verkuil
On 05/02/2015 07:23 PM, Prashant Laddha wrote:
> The intermediate calculation in the expression for hblank can exceed
> 32 bit signed range. This overflow can lead to negative values for
> hblank. Typecasting intermediate variable to higher precision.
> 
> Cc: Hans Verkuil 
> Cc: Martin Bugge 
> Signed-off-by: Prashant Laddha 
> ---
>  drivers/media/v4l2-core/v4l2-dv-timings.c | 16 
>  1 file changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-dv-timings.c 
> b/drivers/media/v4l2-core/v4l2-dv-timings.c
> index 86e11d1..9d27f05 100644
> --- a/drivers/media/v4l2-core/v4l2-dv-timings.c
> +++ b/drivers/media/v4l2-core/v4l2-dv-timings.c
> @@ -573,15 +573,15 @@ bool v4l2_detect_gtf(unsigned frame_height,
>  
>   /* Horizontal */
>   if (default_gtf)
> - h_blank = ((image_width * GTF_D_C_PRIME * hfreq) -
> - (image_width * GTF_D_M_PRIME * 1000) +
> - (hfreq * (100 - GTF_D_C_PRIME) + GTF_D_M_PRIME * 1000) 
> / 2) /
> - (hfreq * (100 - GTF_D_C_PRIME) + GTF_D_M_PRIME * 1000);
> + h_blank = ((image_width * GTF_D_C_PRIME * (long long)hfreq) -
> +   ((long long)image_width * GTF_D_M_PRIME * 1000) +
> +   ((long long)hfreq * (100 - GTF_D_C_PRIME) + 
> GTF_D_M_PRIME * 1000) / 2) /
> +   ((long long)hfreq * (100 - GTF_D_C_PRIME) + 
> GTF_D_M_PRIME * 1000);

This will not work on systems that cannot divide 64 bit numbers. Use the do_div
function for this. It's a common mistake to make when developing on Intel CPUs.
Been there, done that :-) Multiple times, in fact...

And I think it will help a lot if some additional variables are introduced since
this calculation is getting complex.

Also replace "/ 2" by ">> 1". That will guarantee that you do not need do_div 
for
that. Probably unnecessary since I would expect gcc to be smart enough to 
replace
/ 2 by >> 1 anyway, but it doesn't hurt.

Regards,

Hans

>   else
> - h_blank = ((image_width * GTF_S_C_PRIME * hfreq) -
> - (image_width * GTF_S_M_PRIME * 1000) +
> - (hfreq * (100 - GTF_S_C_PRIME) + GTF_S_M_PRIME * 1000) 
> / 2) /
> - (hfreq * (100 - GTF_S_C_PRIME) + GTF_S_M_PRIME * 1000);
> + h_blank = ((image_width * GTF_S_C_PRIME * (long long)hfreq) -
> +   ((long long)image_width * GTF_S_M_PRIME * 1000) +
> +   ((long long)hfreq * (100 - GTF_S_C_PRIME) + 
> GTF_S_M_PRIME * 1000) / 2) /
> +   ((long long)hfreq * (100 - GTF_S_C_PRIME) + 
> GTF_S_M_PRIME * 1000);
>  
>   h_blank = ((h_blank + GTF_CELL_GRAN) / (2 * GTF_CELL_GRAN)) *
> (2 * GTF_CELL_GRAN);
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html