On Sun, Oct 15, 2023 at 4:44 AM Hugo Villeneuve <hugo@xxxxxxxxxxx> wrote: > > On Sat, 14 Oct 2023 23:43:33 +0530 > Vamshi Gajjela <vamshigajjela@xxxxxxxxxx> wrote: > > > From: VAMSHI GAJJELA <vamshigajjela@xxxxxxxxxx> > > Hi, > your commit title doesn't really explain what this patch is doing. > > Please see: https://cbea.ms/git-commit/#imperative Thanks Hugo for the review, I will provide details in the following response. > > > > uart_update_timeout() sets a u64 value to an unsigned int frame_time. > > While it can be cast to u32 before assignment, there's a specific case > > where frame_time is cast to u64. Since frame_time consistently > > participates in u64 arithmetic, its data type is converted to u64 to > > eliminate the need for explicit casting. > > Again, refering to your title commit message, I would expect that you > would describe precisely how a potential overflow can happen here, and > I am not seeing it. > > And based on your log message, it seems that your commit is simply some > kind of optimization, not a fix? In the function uart_update_timeout() within serial_core.c, a u64 value is assigned to an "unsigned int" variable frame_time. This raises concerns about potential overflow. While the code in the patch doesn't explicitly manifest the issue in the following line of uart_update_timeout() "port->frame_time = DIV64_U64_ROUND_UP(frame_time, baud);" lacks a u32 typecast for frame_time. In the function "uart_fifo_timeout" has the following line of code u64 fifo_timeout = (u64)READ_ONCE(port->frame_time) * port->fifosize; In the above line frame_time is typecast to u64 also, timeout values in the serial core associated with frame_time are used in the u64 arithmetic. To maintain consistency and readability, I've updated the size of frame_time from "unsigned int" to "unsigned long". This ensures uniformity with typecasts elsewhere in the code, addressing potential issues and enhancing clarity. I hope this provides clarity. Would you find it helpful if I were to provide further details in the commit message? > > Hugo. > > > > > Signed-off-by: VAMSHI GAJJELA <vamshigajjela@xxxxxxxxxx> > > --- > > v2: > > - use DIV64_U64_ROUND_UP with frame_time > > > > drivers/tty/serial/8250/8250_port.c | 2 +- > > include/linux/serial_core.h | 4 ++-- > > 2 files changed, 3 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c > > index 141627370aab..d1bf794498c4 100644 > > --- a/drivers/tty/serial/8250/8250_port.c > > +++ b/drivers/tty/serial/8250/8250_port.c > > @@ -1510,7 +1510,7 @@ static inline void __stop_tx(struct uart_8250_port *p) > > * rather than after it is fully sent. > > * Roughly estimate 1 extra bit here with / 7. > > */ > > - stop_delay = p->port.frame_time + DIV_ROUND_UP(p->port.frame_time, 7); > > + stop_delay = p->port.frame_time + DIV64_U64_ROUND_UP(p->port.frame_time, 7); > > } > > > > __stop_tx_rs485(p, stop_delay); > > diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h > > index bb6f073bc159..b128513b009a 100644 > > --- a/include/linux/serial_core.h > > +++ b/include/linux/serial_core.h > > @@ -558,7 +558,7 @@ struct uart_port { > > > > bool hw_stopped; /* sw-assisted CTS flow state */ > > unsigned int mctrl; /* current modem ctrl settings */ > > - unsigned int frame_time; /* frame timing in ns */ > > + unsigned long frame_time; /* frame timing in ns */ > > unsigned int type; /* port type */ > > const struct uart_ops *ops; > > unsigned int custom_divisor; > > @@ -764,7 +764,7 @@ unsigned int uart_get_divisor(struct uart_port *port, unsigned int baud); > > */ > > static inline unsigned long uart_fifo_timeout(struct uart_port *port) > > { > > - u64 fifo_timeout = (u64)READ_ONCE(port->frame_time) * port->fifosize; > > + u64 fifo_timeout = READ_ONCE(port->frame_time) * port->fifosize; > > > > /* Add .02 seconds of slop */ > > fifo_timeout += 20 * NSEC_PER_MSEC; > > -- > > 2.42.0.655.g421f12c284-goog > >
