2018-01-11 12:47:54

by Nuno Gonçalves

Subject: 8250_dw bug

Dear Ed and Greg,

There is a small bug on de9e33bdfa22e607a88494ff21e9196d00bf4550, at
least on 32bit devices.

Line 274 if (rate >= i * min_rate && rate <= i * max_rate)

This will overflow when min_rate/max_rate is large and can not be
achieved in the hardware.

Eg.

stty -F /dev/ttyS2 raw 3500000 (not achievable on my board).

target_rate=56000000 (for 3500000baud)
min_rate=55125000
max_rate=56875000
rate=24000000 (clk_round_rate for my board)

Since my board can only do 1500000baud, this loop will keep
incrementing i until i=77*56875000 will overflow, and there are
unexpected results.

I suggest this patch:

--- 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;
+ break;
+ }
+
+ if (rate <= i * max_rate)
break;
}
if (i <= UART_DIV_MAX) {


Let me know if you want me to submit the formal patch.

Thanks,
Nuno

2018-01-11 13:19:49

by Greg Kroah-Hartman

Subject: Re: 8250_dw bug

On Thu, Jan 11, 2018 at 01:47:31PM +0100, Nuno Gon?alves wrote:
> Dear Ed and Greg,
>
> There is a small bug on de9e33bdfa22e607a88494ff21e9196d00bf4550, at
> least on 32bit devices.
>
> Line 274 if (rate >= i * min_rate && rate <= i * max_rate)
>
> This will overflow when min_rate/max_rate is large and can not be
> achieved in the hardware.
>
> Eg.
>
> stty -F /dev/ttyS2 raw 3500000 (not achievable on my board).
>
> target_rate=56000000 (for 3500000baud)
> min_rate=55125000
> max_rate=56875000
> rate=24000000 (clk_round_rate for my board)
>
> Since my board can only do 1500000baud, this loop will keep
> incrementing i until i=77*56875000 will overflow, and there are
> unexpected results.
>
> I suggest this patch:
>
> --- 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;
> + break;
> + }
> +
> + if (rate <= i * max_rate)
> break;
> }
> if (i <= UART_DIV_MAX) {
>
>
> Let me know if you want me to submit the formal patch.

Why wouldn't you submit a "formal patch", that way we could apply it if
it is correct :)

also cc: the linux-serial mailing list when you do so, thanks.

greg k-h