On Mon, 16 Oct 2023, Jiri Slaby wrote:
> On 14. 10. 23, 20:13, Vamshi Gajjela wrote:
> > From: VAMSHI GAJJELA <vamshigajjela@xxxxxxxxxx>
> >
> > 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.
>
> And make the divisions by the order of magnutude slower for no good reason?
> (Hint: have you looked what DIV64_U64_ROUND_UP() looks like on 32bit yet?)
>
> Unless you provide a reason it can overflow in real (in fact you spell the
> opposite in the text above):
> NACKED-by: Jiri Slaby <jirislaby@xxxxxxxxxx>
I sorry but I have to concur Jiri heavily here,
NACKED-by: Ilpo Järvinen <ilpo.jarvinen@xxxxxxxxxxxxxxx>
> > Signed-off-by: VAMSHI GAJJELA <vamshigajjela@xxxxxxxxxx>
> > ---
> > v2:
> > - use DIV64_U64_ROUND_UP with frame_time
Please, I barely managed to read your v1 and it's v2 already, don't send
the next version this soon. There's absolutely no need to fill our inboxes
with n versions of your patch over a weekend, just remain patient
and wait reasonable amount of time to gather feedback, please. ...I know
it's often hard to wait, it's hard for me too at times.
You even failed to convert the other divide done on ->frame_time to
DIV64_u64_ROUND_UP(), which looks a mindboggling oversight to me.
So far you've managed to cause so many problems with these two attempts to
fix a non-problem I heavily suggest you focus on something that doesn't
relate to fixing types. It takes time to understand the related code when
doing something as simple as type change, which you clearly did not have
as demonstrated by you missing that other divide which can be trivially
found with git grep.
> >
> > 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
> > */
As with v1, u64 != unsigned long, I hope you do know that much about
different architectures...
--
i.
> > 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;
>
>
