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@xxxxxxxxxxx> 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@xxxxxxxxxxx> 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@xxxxxxxxx>
>>>> ---
>>>>  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
>