[PATCH] 8250_dw: do not int overflow when rate can not be aplied

[PATCH] 8250_dw: do not int overflow when rate can not be aplied

[Nuno Goncalves]

When target_rate is big enough and not permitted in hardware,
then i is looped to UART_DIV_MAX (0xFFFF), and i * max_rate will overflow
(32b signed).

A fix is to quit the loop early enough, as soon as rate < i * min_rate as it
means the rate is not permitted.

This avoids arbitraty rates to be applied. Still in my hardware the max
allowed rate (1500000) is aplied when a higher is requested. This seems a
artifact of clk_round_rate which is not understood by me and independent of
this fix. Might or might not be another bug.

Signed-off-by: Nuno Goncalves <nunojpg@gmail.com>
---
 drivers/tty/serial/8250/8250_dw.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/tty/serial/8250/8250_dw.c b/drivers/tty/serial/8250/8250_dw.c
index 5bb0c42c88dd..a27ea916abbf 100644
--- a/drivers/tty/serial/8250/8250_dw.c
+++ b/drivers/tty/serial/8250/8250_dw.c
@@ -267,7 +267,13 @@ static void dw8250_set_termios(struct uart_port *p, struct ktermios *termios,
 
 	for (i = 1; i <= UART_DIV_MAX; i++) {
 		rate = clk_round_rate(d->clk, i * target_rate);
-		if (rate >= i * min_rate && rate <= i * max_rate)
+
+		if (rate < i * min_rate) {
+			i = UART_DIV_MAX + 1;
+			break;
+		}
+
+		if (rate <= i * max_rate)
 			break;
 	}
 	if (i <= UART_DIV_MAX) {
-- 
2.11.0

Re: [PATCH] 8250_dw: do not int overflow when rate can not be aplied

[Ed Blake]

Hi Nuno,

Thanks for reporting this and the patch.

On 11/01/18 13:38, Nuno Goncalves wrote:
> When target_rate is big enough and not permitted in hardware,
> then i is looped to UART_DIV_MAX (0xFFFF), and i * max_rate will overflow
> (32b signed).
>
> A fix is to quit the loop early enough, as soon as rate < i * min_rate as it
> means the rate is not permitted.

'rate < i * min_rate' does not mean the rate is not permitted.  clk_round_rate() gives the nearest achievable rate to the one requested, which may be lower than i * min_rate.  This is not an error and in this case we want to continue the loop searching for an acceptable rate.


>
> This avoids arbitraty rates to be applied. Still in my hardware the max
> allowed rate (1500000) is aplied when a higher is requested. This seems a
> artifact of clk_round_rate which is not understood by me and independent of
> this fix. Might or might not be another bug.
>
> Signed-off-by: Nuno Goncalves <nunojpg@gmail.com>
> ---
>  drivers/tty/serial/8250/8250_dw.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/tty/serial/8250/8250_dw.c b/drivers/tty/serial/8250/8250_dw.c
> index 5bb0c42c88dd..a27ea916abbf 100644
> --- a/drivers/tty/serial/8250/8250_dw.c
> +++ b/drivers/tty/serial/8250/8250_dw.c
> @@ -267,7 +267,13 @@ static void dw8250_set_termios(struct uart_port *p, struct ktermios *termios,
>  
>  	for (i = 1; i <= UART_DIV_MAX; i++) {
>  		rate = clk_round_rate(d->clk, i * target_rate);
> -		if (rate >= i * min_rate && rate <= i * max_rate)
> +
> +		if (rate < i * min_rate) {
> +			i = UART_DIV_MAX + 1;
> +			break;
> +		}
> +
> +		if (rate <= i * max_rate)
>  			break;
>  	}
>  	if (i <= UART_DIV_MAX) {

-- 
Ed

Re: [PATCH] 8250_dw: do not int overflow when rate can not be aplied

[Nuno Gonçalves]

I have to disagree :)

if (rate < i * min_rate) is true to i=a, then

(rate >= i * min_rate && rate <= i * max_rate) will always be false
for any i=b, where b>a.

If this condition is true, it means the old condition would be always
false for the remaining of the iteration.

My patch "only" avoids integer overflow and terminates the search as
soon as possible, since no need to search for bigger dividers if the
current one would already mean a rate below min_rate (that it, this is
the closer).

So in fact we also break when the closer divider have been found.

Thanks,
Nuno

On Thu, Jan 11, 2018 at 6:18 PM, Ed Blake <ed.blake@sondrel.com> wrote:
> Hi Nuno,
>
> Thanks for reporting this and the patch.
>
> On 11/01/18 13:38, Nuno Goncalves wrote:
>> When target_rate is big enough and not permitted in hardware,
>> then i is looped to UART_DIV_MAX (0xFFFF), and i * max_rate will overflow
>> (32b signed).
>>
>> A fix is to quit the loop early enough, as soon as rate < i * min_rate as it
>> means the rate is not permitted.
>
> 'rate < i * min_rate' does not mean the rate is not permitted.  clk_round_rate() gives the nearest achievable rate to the one requested, which may be lower than i * min_rate.  This is not an error and in this case we want to continue the loop searching for an acceptable rate.
>
>
>>
>> This avoids arbitraty rates to be applied. Still in my hardware the max
>> allowed rate (1500000) is aplied when a higher is requested. This seems a
>> artifact of clk_round_rate which is not understood by me and independent of
>> this fix. Might or might not be another bug.
>>
>> Signed-off-by: Nuno Goncalves <nunojpg@gmail.com>
>> ---
>>  drivers/tty/serial/8250/8250_dw.c | 8 +++++++-
>>  1 file changed, 7 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/tty/serial/8250/8250_dw.c b/drivers/tty/serial/8250/8250_dw.c
>> index 5bb0c42c88dd..a27ea916abbf 100644
>> --- a/drivers/tty/serial/8250/8250_dw.c
>> +++ b/drivers/tty/serial/8250/8250_dw.c
>> @@ -267,7 +267,13 @@ static void dw8250_set_termios(struct uart_port *p, struct ktermios *termios,
>>
>>       for (i = 1; i <= UART_DIV_MAX; i++) {
>>               rate = clk_round_rate(d->clk, i * target_rate);
>> -             if (rate >= i * min_rate && rate <= i * max_rate)
>> +
>> +             if (rate < i * min_rate) {
>> +                     i = UART_DIV_MAX + 1;
>> +                     break;
>> +             }
>> +
>> +             if (rate <= i * max_rate)
>>                       break;
>>       }
>>       if (i <= UART_DIV_MAX) {
>
> --
> Ed
>

Re: [PATCH] 8250_dw: do not int overflow when rate can not be aplied

[Ed Blake]

On 11/01/18 17:28, Nuno Gonçalves wrote:
> I have to disagree :)
>
> if (rate < i * min_rate) is true to i=a, then
>
> (rate >= i * min_rate && rate <= i * max_rate) will always be false
> for any i=b, where b>a.

No, because 'rate' is assigned from clk_round_rate() each iteration.

The idea of this code is to iterate through integer multiples of baud *
16 until you find an achievable rate that is within the +/- 1.6% range. 
Until then, the rate returned from clk_round_rate() could be lower than
i * min_rate or higher than i * max_rate, in which case you keep going.

> If this condition is true, it means the old condition would be always
> false for the remaining of the iteration.
>
> My patch "only" avoids integer overflow and terminates the search as
> soon as possible, since no need to search for bigger dividers if the
> current one would already mean a rate below min_rate (that it, this is
> the closer).

It terminates the search as soon as the rate returned from
clk_round_rate() is lower than i * min_rate, which is too soon.

> So in fact we also break when the closer divider have been found.
>
> Thanks,
> Nuno
>
> On Thu, Jan 11, 2018 at 6:18 PM, Ed Blake <ed.blake@sondrel.com> wrote:
>> Hi Nuno,
>>
>> Thanks for reporting this and the patch.
>>
>> On 11/01/18 13:38, Nuno Goncalves wrote:
>>> When target_rate is big enough and not permitted in hardware,
>>> then i is looped to UART_DIV_MAX (0xFFFF), and i * max_rate will overflow
>>> (32b signed).
>>>
>>> A fix is to quit the loop early enough, as soon as rate < i * min_rate as it
>>> means the rate is not permitted.
>> 'rate < i * min_rate' does not mean the rate is not permitted.  clk_round_rate() gives the nearest achievable rate to the one requested, which may be lower than i * min_rate.  This is not an error and in this case we want to continue the loop searching for an acceptable rate.
>>
>>
>>> This avoids arbitraty rates to be applied. Still in my hardware the max
>>> allowed rate (1500000) is aplied when a higher is requested. This seems a
>>> artifact of clk_round_rate which is not understood by me and independent of
>>> this fix. Might or might not be another bug.
>>>
>>> Signed-off-by: Nuno Goncalves <nunojpg@gmail.com>
>>> ---
>>>  drivers/tty/serial/8250/8250_dw.c | 8 +++++++-
>>>  1 file changed, 7 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/tty/serial/8250/8250_dw.c b/drivers/tty/serial/8250/8250_dw.c
>>> index 5bb0c42c88dd..a27ea916abbf 100644
>>> --- a/drivers/tty/serial/8250/8250_dw.c
>>> +++ b/drivers/tty/serial/8250/8250_dw.c
>>> @@ -267,7 +267,13 @@ static void dw8250_set_termios(struct uart_port *p, struct ktermios *termios,
>>>
>>>       for (i = 1; i <= UART_DIV_MAX; i++) {
>>>               rate = clk_round_rate(d->clk, i * target_rate);
>>> -             if (rate >= i * min_rate && rate <= i * max_rate)
>>> +
>>> +             if (rate < i * min_rate) {
>>> +                     i = UART_DIV_MAX + 1;
>>> +                     break;
>>> +             }
>>> +
>>> +             if (rate <= i * max_rate)
>>>                       break;
>>>       }
>>>       if (i <= UART_DIV_MAX) {
>> --
>> Ed
>>

-- 
Ed

Re: [PATCH] 8250_dw: do not int overflow when rate can not be aplied

[Nuno Gonçalves]

So, for me clk_round_rate() always returns 24000000, and only the loop
variable i changes, so the search is monotonic, from the highest baud
to the lowest (increasing divider).

I am using a Allwiner H2+, with the serial port configuration from
sunxi-h3-h5.dtsi.

Are you sure that clk_round_rate can return differet values? Is that
because some boards might have several clock options beside the
adjustable divider?

I really need to understand what is the problem, to be able to suggest
a solution to the integer overflow that is being allowed to happen.

Thanks,
Nuno



On Thu, Jan 11, 2018 at 6:48 PM, Ed Blake <ed.blake@sondrel.com> wrote:
> On 11/01/18 17:28, Nuno Gonçalves wrote:
>> I have to disagree :)
>>
>> if (rate < i * min_rate) is true to i=a, then
>>
>> (rate >= i * min_rate && rate <= i * max_rate) will always be false
>> for any i=b, where b>a.
>
> No, because 'rate' is assigned from clk_round_rate() each iteration.
>
> The idea of this code is to iterate through integer multiples of baud *
> 16 until you find an achievable rate that is within the +/- 1.6% range.
> Until then, the rate returned from clk_round_rate() could be lower than
> i * min_rate or higher than i * max_rate, in which case you keep going.
>
>> If this condition is true, it means the old condition would be always
>> false for the remaining of the iteration.
>>
>> My patch "only" avoids integer overflow and terminates the search as
>> soon as possible, since no need to search for bigger dividers if the
>> current one would already mean a rate below min_rate (that it, this is
>> the closer).
>
> It terminates the search as soon as the rate returned from
> clk_round_rate() is lower than i * min_rate, which is too soon.
>
>> So in fact we also break when the closer divider have been found.
>>
>> Thanks,
>> Nuno
>>
>> On Thu, Jan 11, 2018 at 6:18 PM, Ed Blake <ed.blake@sondrel.com> wrote:
>>> Hi Nuno,
>>>
>>> Thanks for reporting this and the patch.
>>>
>>> On 11/01/18 13:38, Nuno Goncalves wrote:
>>>> When target_rate is big enough and not permitted in hardware,
>>>> then i is looped to UART_DIV_MAX (0xFFFF), and i * max_rate will overflow
>>>> (32b signed).
>>>>
>>>> A fix is to quit the loop early enough, as soon as rate < i * min_rate as it
>>>> means the rate is not permitted.
>>> 'rate < i * min_rate' does not mean the rate is not permitted.  clk_round_rate() gives the nearest achievable rate to the one requested, which may be lower than i * min_rate.  This is not an error and in this case we want to continue the loop searching for an acceptable rate.
>>>
>>>
>>>> This avoids arbitraty rates to be applied. Still in my hardware the max
>>>> allowed rate (1500000) is aplied when a higher is requested. This seems a
>>>> artifact of clk_round_rate which is not understood by me and independent of
>>>> this fix. Might or might not be another bug.
>>>>
>>>> Signed-off-by: Nuno Goncalves <nunojpg@gmail.com>
>>>> ---
>>>>  drivers/tty/serial/8250/8250_dw.c | 8 +++++++-
>>>>  1 file changed, 7 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/tty/serial/8250/8250_dw.c b/drivers/tty/serial/8250/8250_dw.c
>>>> index 5bb0c42c88dd..a27ea916abbf 100644
>>>> --- a/drivers/tty/serial/8250/8250_dw.c
>>>> +++ b/drivers/tty/serial/8250/8250_dw.c
>>>> @@ -267,7 +267,13 @@ static void dw8250_set_termios(struct uart_port *p, struct ktermios *termios,
>>>>
>>>>       for (i = 1; i <= UART_DIV_MAX; i++) {
>>>>               rate = clk_round_rate(d->clk, i * target_rate);
>>>> -             if (rate >= i * min_rate && rate <= i * max_rate)
>>>> +
>>>> +             if (rate < i * min_rate) {
>>>> +                     i = UART_DIV_MAX + 1;
>>>> +                     break;
>>>> +             }
>>>> +
>>>> +             if (rate <= i * max_rate)
>>>>                       break;
>>>>       }
>>>>       if (i <= UART_DIV_MAX) {
>>> --
>>> Ed
>>>
>
> --
> Ed
>

Re: [PATCH] 8250_dw: do not int overflow when rate can not be aplied

[Ed Blake]

On 11/01/18 17:55, Nuno Gonçalves wrote:
> So, for me clk_round_rate() always returns 24000000, and only the loop
> variable i changes, so the search is monotonic, from the highest baud
> to the lowest (increasing divider).
>
> I am using a Allwiner H2+, with the serial port configuration from
> sunxi-h3-h5.dtsi.
>
> Are you sure that clk_round_rate can return differet values? Is that
> because some boards might have several clock options beside the
> adjustable divider?

Yes I'm sure.  Some platforms allow the clock rate to be varied, hence
the existence of clk_round_rate() and clk_set_rate().

> I really need to understand what is the problem, to be able to suggest
> a solution to the integer overflow that is being allowed to happen.

Some sort of overflow check on i * max_rate could work?

> Thanks,
> Nuno

Re: [PATCH] 8250_dw: do not int overflow when rate can not be aplied

[Ed Blake]

On 11/01/18 18:03, Ed Blake wrote:
> On 11/01/18 17:55, Nuno Gonçalves wrote:
>> So, for me clk_round_rate() always returns 24000000, and only the loop
>> variable i changes, so the search is monotonic, from the highest baud
>> to the lowest (increasing divider).
>>
>> I am using a Allwiner H2+, with the serial port configuration from
>> sunxi-h3-h5.dtsi.
>>
>> Are you sure that clk_round_rate can return differet values? Is that
>> because some boards might have several clock options beside the
>> adjustable divider?
> Yes I'm sure.  Some platforms allow the clock rate to be varied, hence
> the existence of clk_round_rate() and clk_set_rate().
>
>> I really need to understand what is the problem, to be able to suggest
>> a solution to the integer overflow that is being allowed to happen.
> Some sort of overflow check on i * max_rate could work?

Actually I have another suggestion.  I'll submit a separate patch.

-- 
Ed