From: VAMSHI GAJJELA <[email protected]>
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.
Signed-off-by: VAMSHI GAJJELA <[email protected]>
---
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
On Sat, 14 Oct 2023 23:43:33 +0530
Vamshi Gajjela <[email protected]> wrote:
> From: VAMSHI GAJJELA <[email protected]>
Hi,
your commit title doesn't really explain what this patch is doing.
Please see: https://cbea.ms/git-commit/#imperative
> 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?
Hugo.
> Signed-off-by: VAMSHI GAJJELA <[email protected]>
> ---
> 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
>
On Sun, Oct 15, 2023 at 4:44 AM Hugo Villeneuve <[email protected]> wrote:
>
> On Sat, 14 Oct 2023 23:43:33 +0530
> Vamshi Gajjela <[email protected]> wrote:
>
> > From: VAMSHI GAJJELA <[email protected]>
>
> 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 <[email protected]>
> > ---
> > 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
> >
On 14. 10. 23, 20:13, Vamshi Gajjela wrote:
> From: VAMSHI GAJJELA <[email protected]>
>
> 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 <[email protected]>
> Signed-off-by: VAMSHI GAJJELA <[email protected]>
> ---
> 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;
--
js
suse labs
On Mon, 16 Oct 2023, Jiri Slaby wrote:
> On 14. 10. 23, 20:13, Vamshi Gajjela wrote:
> > From: VAMSHI GAJJELA <[email protected]>
> >
> > 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 <[email protected]>
I sorry but I have to concur Jiri heavily here,
NACKED-by: Ilpo J?rvinen <[email protected]>
> > Signed-off-by: VAMSHI GAJJELA <[email protected]>
> > ---
> > 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;
>
>
On Mon, Oct 16, 2023 at 4:30 PM Ilpo Järvinen
<[email protected]> wrote:
>
> On Mon, 16 Oct 2023, Jiri Slaby wrote:
>
> > On 14. 10. 23, 20:13, Vamshi Gajjela wrote:
> > > From: VAMSHI GAJJELA <[email protected]>
> > >
> > > 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 <[email protected]>
>
> I sorry but I have to concur Jiri heavily here,
>
> NACKED-by: Ilpo Järvinen <[email protected]>
>
> > > Signed-off-by: VAMSHI GAJJELA <[email protected]>
> > > ---
> > > 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.
Apologies for the inconvenience caused, I should have waited for the response
on v1 before making v2, by leaving a comment about the anticipated change.
I have clearly not considered all the architectures in the patch, and
the overhead
of division on 32-archs, mistake from my side. once again apologies for that.
Thanks Jiri Slaby & Ilpo Järvinen for the review.
>
> > >
> > > 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;
> >
> >