Re: [PATCH] 8250_dw: do not int overflow when rate can not be aplied
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 2400, 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
Re: [PATCH] 8250_dw: do not int overflow when rate can not be aplied
On 11/01/18 17:55, Nuno Gonçalves wrote: > So, for me clk_round_rate() always returns 2400, 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
So, for me clk_round_rate() always returns 2400, 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 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 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 (0x), 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 (150) 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
---
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
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 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 (0x), 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 (150) 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
>>> ---
>>> 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
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 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 (0x), 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 (150) 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
>> ---
>> 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
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 (0x), 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 (150) 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
> ---
> 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
[PATCH] 8250_dw: do not int overflow when rate can not be aplied
When target_rate is big enough and not permitted in hardware,
then i is looped to UART_DIV_MAX (0x), 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 (150) 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
---
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
