Re: [PATCH 09/25] serial: sh-sci: Avoid overflow in sci_baud_calc_hscif()

[Geert Uytterhoeven]

On Thu, Nov 19, 2015 at 9:37 PM, Laurent Pinchart
 wrote:
>> --- a/drivers/tty/serial/sh-sci.c
>> +++ b/drivers/tty/serial/sh-sci.c
>> @@ -1870,7 +1870,7 @@ static unsigned int sci_scbrr_calc(struct sci_port *s,
>> unsigned int bps, static void sci_baud_calc_hscif(unsigned int bps,
>> unsigned long freq, int *brr, unsigned int *srr, unsigned int *cks)
>>  {
>> - unsigned int sr, br, c;
>> + unsigned int sr, br, a, b, c;
>>   int err, recv_margin;
>>   int min_err = 1000; /* 100% */
>>   int recv_max_margin = 0;
>> @@ -1880,12 +1880,14 @@ static void sci_baud_calc_hscif(unsigned int bps,
>> unsigned long freq, int *brr, for (sr = 8; sr <= 32; sr++) {
>>   for (c = 0; c <= 3; c++) {
>>   /* integerized formulas from HSCIF documentation */
>> - br = DIV_ROUND_CLOSEST(freq, (sr *
>> -   (1 << (2 * c + 1)) * bps));
>> + a = sr * (1 << (2 * c + 1));
>> + if (bps > UINT_MAX / a)
>> + break;
>> +
>> + b = a * bps;
>
> This is becoming unreadable. Could you please use proper variable names ? A

Sometimes it's different to find good variable names ("t1" and "t2"?).

> comment that explains the calculation would also be useful.

Thanks, that would indeed help.

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 09/25] serial: sh-sci: Avoid overflow in sci_baud_calc_hscif()

[Laurent Pinchart]

Hi Geert,

Thank you for the patch.

On Thursday 19 November 2015 19:38:48 Geert Uytterhoeven wrote:
> If bps >= 1048576, the multiplication of "(sr * (1 << (2 * c + 1))" and
> "bps" will overflow, and both br and err will contain bogus values.
> Skip the current and all higher clock select predividers when overflow
> is detected.  Simplify the calculations using intermediates while we're
> at it.
> 
> Signed-off-by: Geert Uytterhoeven 
> ---
>  drivers/tty/serial/sh-sci.c | 14 --
>  1 file changed, 8 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c
> index fba1e1eea15dc3a1..97a0f8ef5adc55a2 100644
> --- a/drivers/tty/serial/sh-sci.c
> +++ b/drivers/tty/serial/sh-sci.c
> @@ -1870,7 +1870,7 @@ static unsigned int sci_scbrr_calc(struct sci_port *s,
> unsigned int bps, static void sci_baud_calc_hscif(unsigned int bps,
> unsigned long freq, int *brr, unsigned int *srr, unsigned int *cks)
>  {
> - unsigned int sr, br, c;
> + unsigned int sr, br, a, b, c;
>   int err, recv_margin;
>   int min_err = 1000; /* 100% */
>   int recv_max_margin = 0;
> @@ -1880,12 +1880,14 @@ static void sci_baud_calc_hscif(unsigned int bps,
> unsigned long freq, int *brr, for (sr = 8; sr <= 32; sr++) {
>   for (c = 0; c <= 3; c++) {
>   /* integerized formulas from HSCIF documentation */
> - br = DIV_ROUND_CLOSEST(freq, (sr *
> -   (1 << (2 * c + 1)) * bps));
> + a = sr * (1 << (2 * c + 1));
> + if (bps > UINT_MAX / a)
> + break;
> +
> + b = a * bps;

This is becoming unreadable. Could you please use proper variable names ? A 
comment that explains the calculation would also be useful.

> + br = DIV_ROUND_CLOSEST(freq, b);
>   br = clamp(br, 1U, 256U);
> - err = DIV_ROUND_CLOSEST(freq, (br * bps * sr *
> -(1 << (2 * c + 1)) / 1000)) -
> -1000;
> + err = DIV_ROUND_CLOSEST(freq, (br * b) / 1000) - 1000;
>   /* Calc recv margin
>* M: Receive margin (%)
>* N: Ratio of bit rate to clock (N = sampling rate)

-- 
Regards,

Laurent Pinchart

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

[PATCH 09/25] serial: sh-sci: Avoid overflow in sci_baud_calc_hscif()

[Geert Uytterhoeven]

If bps >= 1048576, the multiplication of "(sr * (1 << (2 * c + 1))" and
"bps" will overflow, and both br and err will contain bogus values.
Skip the current and all higher clock select predividers when overflow
is detected.  Simplify the calculations using intermediates while we're
at it.

Signed-off-by: Geert Uytterhoeven 
---
 drivers/tty/serial/sh-sci.c | 14 --
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c
index fba1e1eea15dc3a1..97a0f8ef5adc55a2 100644
--- a/drivers/tty/serial/sh-sci.c
+++ b/drivers/tty/serial/sh-sci.c
@@ -1870,7 +1870,7 @@ static unsigned int sci_scbrr_calc(struct sci_port *s, 
unsigned int bps,
 static void sci_baud_calc_hscif(unsigned int bps, unsigned long freq, int *brr,
unsigned int *srr, unsigned int *cks)
 {
-   unsigned int sr, br, c;
+   unsigned int sr, br, a, b, c;
int err, recv_margin;
int min_err = 1000; /* 100% */
int recv_max_margin = 0;
@@ -1880,12 +1880,14 @@ static void sci_baud_calc_hscif(unsigned int bps, 
unsigned long freq, int *brr,
for (sr = 8; sr <= 32; sr++) {
for (c = 0; c <= 3; c++) {
/* integerized formulas from HSCIF documentation */
-   br = DIV_ROUND_CLOSEST(freq, (sr *
- (1 << (2 * c + 1)) * bps));
+   a = sr * (1 << (2 * c + 1));
+   if (bps > UINT_MAX / a)
+   break;
+
+   b = a * bps;
+   br = DIV_ROUND_CLOSEST(freq, b);
br = clamp(br, 1U, 256U);
-   err = DIV_ROUND_CLOSEST(freq, (br * bps * sr *
-  (1 << (2 * c + 1)) / 1000)) -
-  1000;
+   err = DIV_ROUND_CLOSEST(freq, (br * b) / 1000) - 1000;
/* Calc recv margin
 * M: Receive margin (%)
 * N: Ratio of bit rate to clock (N = sampling rate)
-- 
1.9.1

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

[PATCH 09/25] serial: sh-sci: Avoid overflow in sci_baud_calc_hscif()

[Geert Uytterhoeven]

If bps >= 1048576, the multiplication of "(sr * (1 << (2 * c + 1))" and
"bps" will overflow, and both br and err will contain bogus values.
Skip the current and all higher clock select predividers when overflow
is detected.  Simplify the calculations using intermediates while we're
at it.

Signed-off-by: Geert Uytterhoeven 
---
 drivers/tty/serial/sh-sci.c | 14 --
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c
index fba1e1eea15dc3a1..97a0f8ef5adc55a2 100644
--- a/drivers/tty/serial/sh-sci.c
+++ b/drivers/tty/serial/sh-sci.c
@@ -1870,7 +1870,7 @@ static unsigned int sci_scbrr_calc(struct sci_port *s, 
unsigned int bps,
 static void sci_baud_calc_hscif(unsigned int bps, unsigned long freq, int *brr,
unsigned int *srr, unsigned int *cks)
 {
-   unsigned int sr, br, c;
+   unsigned int sr, br, a, b, c;
int err, recv_margin;
int min_err = 1000; /* 100% */
int recv_max_margin = 0;
@@ -1880,12 +1880,14 @@ static void sci_baud_calc_hscif(unsigned int bps, 
unsigned long freq, int *brr,
for (sr = 8; sr <= 32; sr++) {
for (c = 0; c <= 3; c++) {
/* integerized formulas from HSCIF documentation */
-   br = DIV_ROUND_CLOSEST(freq, (sr *
- (1 << (2 * c + 1)) * bps));
+   a = sr * (1 << (2 * c + 1));
+   if (bps > UINT_MAX / a)
+   break;
+
+   b = a * bps;
+   br = DIV_ROUND_CLOSEST(freq, b);
br = clamp(br, 1U, 256U);
-   err = DIV_ROUND_CLOSEST(freq, (br * bps * sr *
-  (1 << (2 * c + 1)) / 1000)) -
-  1000;
+   err = DIV_ROUND_CLOSEST(freq, (br * b) / 1000) - 1000;
/* Calc recv margin
 * M: Receive margin (%)
 * N: Ratio of bit rate to clock (N = sampling rate)
-- 
1.9.1

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

Re: [PATCH 09/25] serial: sh-sci: Avoid overflow in sci_baud_calc_hscif()

[Geert Uytterhoeven]

On Thu, Nov 19, 2015 at 9:37 PM, Laurent Pinchart
 wrote:
>> --- a/drivers/tty/serial/sh-sci.c
>> +++ b/drivers/tty/serial/sh-sci.c
>> @@ -1870,7 +1870,7 @@ static unsigned int sci_scbrr_calc(struct sci_port *s,
>> unsigned int bps, static void sci_baud_calc_hscif(unsigned int bps,
>> unsigned long freq, int *brr, unsigned int *srr, unsigned int *cks)
>>  {
>> - unsigned int sr, br, c;
>> + unsigned int sr, br, a, b, c;
>>   int err, recv_margin;
>>   int min_err = 1000; /* 100% */
>>   int recv_max_margin = 0;
>> @@ -1880,12 +1880,14 @@ static void sci_baud_calc_hscif(unsigned int bps,
>> unsigned long freq, int *brr, for (sr = 8; sr <= 32; sr++) {
>>   for (c = 0; c <= 3; c++) {
>>   /* integerized formulas from HSCIF documentation */
>> - br = DIV_ROUND_CLOSEST(freq, (sr *
>> -   (1 << (2 * c + 1)) * bps));
>> + a = sr * (1 << (2 * c + 1));
>> + if (bps > UINT_MAX / a)
>> + break;
>> +
>> + b = a * bps;
>
> This is becoming unreadable. Could you please use proper variable names ? A

Sometimes it's different to find good variable names ("t1" and "t2"?).

> comment that explains the calculation would also be useful.

Thanks, that would indeed help.

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 09/25] serial: sh-sci: Avoid overflow in sci_baud_calc_hscif()

[Laurent Pinchart]

Hi Geert,

Thank you for the patch.

On Thursday 19 November 2015 19:38:48 Geert Uytterhoeven wrote:
> If bps >= 1048576, the multiplication of "(sr * (1 << (2 * c + 1))" and
> "bps" will overflow, and both br and err will contain bogus values.
> Skip the current and all higher clock select predividers when overflow
> is detected.  Simplify the calculations using intermediates while we're
> at it.
> 
> Signed-off-by: Geert Uytterhoeven 
> ---
>  drivers/tty/serial/sh-sci.c | 14 --
>  1 file changed, 8 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c
> index fba1e1eea15dc3a1..97a0f8ef5adc55a2 100644
> --- a/drivers/tty/serial/sh-sci.c
> +++ b/drivers/tty/serial/sh-sci.c
> @@ -1870,7 +1870,7 @@ static unsigned int sci_scbrr_calc(struct sci_port *s,
> unsigned int bps, static void sci_baud_calc_hscif(unsigned int bps,
> unsigned long freq, int *brr, unsigned int *srr, unsigned int *cks)
>  {
> - unsigned int sr, br, c;
> + unsigned int sr, br, a, b, c;
>   int err, recv_margin;
>   int min_err = 1000; /* 100% */
>   int recv_max_margin = 0;
> @@ -1880,12 +1880,14 @@ static void sci_baud_calc_hscif(unsigned int bps,
> unsigned long freq, int *brr, for (sr = 8; sr <= 32; sr++) {
>   for (c = 0; c <= 3; c++) {
>   /* integerized formulas from HSCIF documentation */
> - br = DIV_ROUND_CLOSEST(freq, (sr *
> -   (1 << (2 * c + 1)) * bps));
> + a = sr * (1 << (2 * c + 1));
> + if (bps > UINT_MAX / a)
> + break;
> +
> + b = a * bps;

This is becoming unreadable. Could you please use proper variable names ? A 
comment that explains the calculation would also be useful.

> + br = DIV_ROUND_CLOSEST(freq, b);
>   br = clamp(br, 1U, 256U);
> - err = DIV_ROUND_CLOSEST(freq, (br * bps * sr *
> -(1 << (2 * c + 1)) / 1000)) -
> -1000;
> + err = DIV_ROUND_CLOSEST(freq, (br * b) / 1000) - 1000;
>   /* Calc recv margin
>* M: Receive margin (%)
>* N: Ratio of bit rate to clock (N = sampling rate)

-- 
Regards,

Laurent Pinchart

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