From: VAMSHI GAJJELA <[email protected]>
This patch series introduces a set of enhancements to the serial core,
primarily focusing on improving type consistency and ensuring proper
handling of timeout values. The changes aim to enhance the reliability
and maintainability of the serial core.
VAMSHI GAJJELA (3):
serial: core: Potential overflow of frame_time
serial: core: Make local variable size to u64
serial: core: Update uart_poll_timeout function to return unsigned int
drivers/tty/serial/serial_core.c | 4 ++--
include/linux/serial_core.h | 8 ++++----
2 files changed, 6 insertions(+), 6 deletions(-)
--
2.42.0.655.g421f12c284-goog
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]>
---
include/linux/serial_core.h | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
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
From: VAMSHI GAJJELA <[email protected]>
The variable size has been changed from u32 to u64 to accommodate a
larger range of values without the need for explicit typecasting.
Signed-off-by: VAMSHI GAJJELA <[email protected]>
---
drivers/tty/serial/serial_core.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index 7bdc21d5e13b..fb4696d17a8b 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -410,10 +410,10 @@ void
uart_update_timeout(struct uart_port *port, unsigned int cflag,
unsigned int baud)
{
- unsigned int size = tty_get_frame_size(cflag);
+ u64 size = tty_get_frame_size(cflag);
u64 frame_time;
- frame_time = (u64)size * NSEC_PER_SEC;
+ frame_time = size * NSEC_PER_SEC;
port->frame_time = DIV64_U64_ROUND_UP(frame_time, baud);
}
EXPORT_SYMBOL(uart_update_timeout);
--
2.42.0.655.g421f12c284-goog
From: VAMSHI GAJJELA <[email protected]>
uart_fifo_timeout() returns unsigned value, hence the function
uart_poll_timeout has been modified to use an unsigned int type for
timeout values instead of a signed int. The return type of the function
has been changed from int to unsigned int for consistency with the type
of timeout values. The result of uart_fifo_timeout() is cast to u32,
indicating that the value is truncated.
Signed-off-by: VAMSHI GAJJELA <[email protected]>
---
include/linux/serial_core.h | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h
index b128513b009a..445a1ff7e502 100644
--- a/include/linux/serial_core.h
+++ b/include/linux/serial_core.h
@@ -773,9 +773,9 @@ static inline unsigned long uart_fifo_timeout(struct uart_port *port)
}
/* Base timer interval for polling */
-static inline int uart_poll_timeout(struct uart_port *port)
+static inline unsigned int uart_poll_timeout(struct uart_port *port)
{
- int timeout = uart_fifo_timeout(port);
+ unsigned int timeout = (u32)uart_fifo_timeout(port);
return timeout > 6 ? (timeout / 2 - 2) : 1;
}
--
2.42.0.655.g421f12c284-goog
On Sat, 14 Oct 2023, Vamshi Gajjela wrote:
> From: VAMSHI GAJJELA <[email protected]>
>
> uart_update_timeout() sets a u64 value to an unsigned int frame_time.
Yes it does, because uart_update_timeout() does math that requires u64 but
the result is always smaller than what requires u64. If you insist on
doing something add the cast there.
> While it can be cast to u32 before assignment, there's a specific case
> where frame_time is cast to u64.
Because it gets multipled with something that results in a big number
The cast is all correct too because the developer actually thought of
possiblity of an overflow on multiply (something every developer should
be conscious of), so there's nothing to see there either.
> Since frame_time consistently
> participates in u64 arithmetic, its data type is converted to u64 to
> eliminate the need for explicit casting.
You need a way more convincing argument that that since you're not even
converting it to u64 like you falsely stated so the sizes still won't
match on all architectures.
I see you've realized u32 is more than enough to store frame time for the
speeds UART operates with? So why exactly is this patch needed? Should all
the other cases where 64-bit arithmetic needs to be used in the kernel be
similarly upconverted to 64 bits?
Also, did you happen to realize frame_time also participates in 32-bit
arithmetic which you just make much worse with this change? (Yes, there
are 32-bit divides done for it.)
So NACK from me to this "fix" of a non-problem by causing much worse
problems you seem to be entirely unaware.
NACKED-by: Ilpo J?rvinen <[email protected]>
--
i.
> Signed-off-by: VAMSHI GAJJELA <[email protected]>
> ---
> include/linux/serial_core.h | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> 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;
>
On Sat, 14 Oct 2023, Vamshi Gajjela wrote:
> From: VAMSHI GAJJELA <[email protected]>
>
> The variable size has been changed from u32 to u64 to accommodate a
> larger range of values without the need for explicit typecasting.
Don't use too broad/generic terminology in shortlog (on [PATCH] line in
subject) or changelog but explicitly mention the variable names please.
> Signed-off-by: VAMSHI GAJJELA <[email protected]>
> ---
> drivers/tty/serial/serial_core.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
> index 7bdc21d5e13b..fb4696d17a8b 100644
> --- a/drivers/tty/serial/serial_core.c
> +++ b/drivers/tty/serial/serial_core.c
> @@ -410,10 +410,10 @@ void
> uart_update_timeout(struct uart_port *port, unsigned int cflag,
> unsigned int baud)
> {
> - unsigned int size = tty_get_frame_size(cflag);
> + u64 size = tty_get_frame_size(cflag);
> u64 frame_time;
>
> - frame_time = (u64)size * NSEC_PER_SEC;
> + frame_time = size * NSEC_PER_SEC;
> port->frame_time = DIV64_U64_ROUND_UP(frame_time, baud);
> }
> EXPORT_SYMBOL(uart_update_timeout);
This is actually a good cleanup all by itself unrelated to the other
change but you need to adapt the changelog to reflect why this is helpful
instead wording it based on the other change.
--
i.
On Sat, 14 Oct 2023, Vamshi Gajjela wrote:
> From: VAMSHI GAJJELA <[email protected]>
>
> uart_fifo_timeout() returns unsigned value, hence the function
> uart_poll_timeout has been modified to use an unsigned int type for
> timeout values instead of a signed int. The return type of the function
> has been changed from int to unsigned int for consistency with the type
> of timeout values. The result of uart_fifo_timeout() is cast to u32,
> indicating that the value is truncated.
>
> Signed-off-by: VAMSHI GAJJELA <[email protected]>
> ---
> include/linux/serial_core.h | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h
> index b128513b009a..445a1ff7e502 100644
> --- a/include/linux/serial_core.h
> +++ b/include/linux/serial_core.h
> @@ -773,9 +773,9 @@ static inline unsigned long uart_fifo_timeout(struct uart_port *port)
> }
>
> /* Base timer interval for polling */
> -static inline int uart_poll_timeout(struct uart_port *port)
> +static inline unsigned int uart_poll_timeout(struct uart_port *port)
This is in jiffies so why don't you return unsigned long instead also
here?
> {
> - int timeout = uart_fifo_timeout(port);
> + unsigned int timeout = (u32)uart_fifo_timeout(port);
Use unsigned long and avoid casting entirely?
>
> return timeout > 6 ? (timeout / 2 - 2) : 1;
> }
>
--
i.
On Mon, Oct 16, 2023 at 4:03 PM Ilpo Järvinen
<[email protected]> wrote:
>
> On Sat, 14 Oct 2023, Vamshi Gajjela wrote:
>
> > From: VAMSHI GAJJELA <[email protected]>
> >
> > uart_update_timeout() sets a u64 value to an unsigned int frame_time.
>
> Yes it does, because uart_update_timeout() does math that requires u64 but
> the result is always smaller than what requires u64. If you insist on
> doing something add the cast there.
Agree, will add a cast there. Can I do that as part of the patch series 2/3
+ u64 size = tty_get_frame_size(cflag);
in uart_update_timeout
>
> > While it can be cast to u32 before assignment, there's a specific case
> > where frame_time is cast to u64.
>
> Because it gets multipled with something that results in a big number
> The cast is all correct too because the developer actually thought of
> possiblity of an overflow on multiply (something every developer should
> be conscious of), so there's nothing to see there either.
Yes, nothing wrong.
>
> > Since frame_time consistently
> > participates in u64 arithmetic, its data type is converted to u64 to
> > eliminate the need for explicit casting.
>
> You need a way more convincing argument that that since you're not even
> converting it to u64 like you falsely stated so the sizes still won't
> match on all architectures.
"all architectures." is something I have missed while considering this patch.
>
> I see you've realized u32 is more than enough to store frame time for the
> speeds UART operates with? So why exactly is this patch needed? Should all
> the other cases where 64-bit arithmetic needs to be used in the kernel be
> similarly upconverted to 64 bits?
Certainly not for other 64-bit arithmetics, I will course correct.
yes u32 is sufficient to store frame time.
>
> Also, did you happen to realize frame_time also participates in 32-bit
> arithmetic which you just make much worse with this change? (Yes, there
> are 32-bit divides done for it.)
char_time = max(nsecs_to_jiffies(port->frame_time / 5), 1UL);
Here is that instance
>
> So NACK from me to this "fix" of a non-problem by causing much worse
> problems you seem to be entirely unaware.
>
> NACKED-by: Ilpo Järvinen <[email protected]>
>
> --
> i.
>
> > Signed-off-by: VAMSHI GAJJELA <[email protected]>
> > ---
> > include/linux/serial_core.h | 4 ++--
> > 1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > 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;
> >
On Mon, Oct 16, 2023 at 5:09 PM Ilpo Järvinen
<[email protected]> wrote:
>
> On Sat, 14 Oct 2023, Vamshi Gajjela wrote:
>
> > From: VAMSHI GAJJELA <[email protected]>
> >
> > The variable size has been changed from u32 to u64 to accommodate a
> > larger range of values without the need for explicit typecasting.
>
> Don't use too broad/generic terminology in shortlog (on [PATCH] line in
> subject) or changelog but explicitly mention the variable names please.
name of the variable is "size", may be now I will rename the variable
to "frame_size" in v2
>
> > Signed-off-by: VAMSHI GAJJELA <[email protected]>
> > ---
> > drivers/tty/serial/serial_core.c | 4 ++--
> > 1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
> > index 7bdc21d5e13b..fb4696d17a8b 100644
> > --- a/drivers/tty/serial/serial_core.c
> > +++ b/drivers/tty/serial/serial_core.c
> > @@ -410,10 +410,10 @@ void
> > uart_update_timeout(struct uart_port *port, unsigned int cflag,
> > unsigned int baud)
> > {
> > - unsigned int size = tty_get_frame_size(cflag);
> > + u64 size = tty_get_frame_size(cflag);
> > u64 frame_time;
> >
> > - frame_time = (u64)size * NSEC_PER_SEC;
> > + frame_time = size * NSEC_PER_SEC;
> > port->frame_time = DIV64_U64_ROUND_UP(frame_time, baud);
> > }
> > EXPORT_SYMBOL(uart_update_timeout);
>
> This is actually a good cleanup all by itself unrelated to the other
> change but you need to adapt the changelog to reflect why this is helpful
> instead wording it based on the other change.
I shall submit this as a separate patch. As mentioned in 1/3 patch
I will also add a cast u32 before assigning the value to port->frametime
along with variable name and size change.
>
> --
> i.
>