2023-10-30 07:36:26

by VAMSHI GAJJELA

Subject: [PATCH v5 2/2] serial: core: Clean up uart_update_timeout() function

Rename the variable size to temp and change its data type from
unsigned int to u64 to avoid type casting in multiplication. Remove the
intermediate variable frame_time and use temp instead to accommodate
the nanoseconds. port->frame_time is an unsigned int, therefore an
explicit cast is used to improve readability.

Signed-off-by: Vamshi Gajjela <[email protected]>
---
v5:
- shortlog changed from "serial: core: Make local variable size to
u64" to "Clean up uart_update_timeout() function"
- renamed local variable size to temp, generic name
- removed intermediate variable frame_time
- added typecast "unsigned int" while assigning to port->frame_time
v4:
- no change, not submitted with series
v3:
- no change, not submitted with series
v2:
- no change, not submitted with series

drivers/tty/serial/serial_core.c | 7 +++----
1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index 7bdc21d5e13b..21d345a9812a 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -410,11 +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 frame_time;
+ u64 temp = tty_get_frame_size(cflag);

- frame_time = (u64)size * NSEC_PER_SEC;
- port->frame_time = DIV64_U64_ROUND_UP(frame_time, baud);
+ temp *= NSEC_PER_SEC;
+ port->frame_time = (unsigned int)DIV64_U64_ROUND_UP(temp, baud);
}
EXPORT_SYMBOL(uart_update_timeout);

--
2.42.0.820.g83a721a137-goog

2023-10-30 09:33:52

by Ilpo Järvinen

Subject: Re: [PATCH v5 2/2] serial: core: Clean up uart_update_timeout() function

On Mon, 30 Oct 2023, Vamshi Gajjela wrote:

> Rename the variable size to temp and change its data type from
> unsigned int to u64 to avoid type casting in multiplication. Remove the
> intermediate variable frame_time and use temp instead to accommodate
> the nanoseconds. port->frame_time is an unsigned int, therefore an
> explicit cast is used to improve readability.

You should focus more on why instead of what. So add explanation that the
frame time is small (you could even calculate the largest value and add
it to the commit message) and therefore it always fits safely to unsigned
int. And that we do not upconvert the type to avoid unnecessary costly
64-bit arithmetic done in a few places in the serial code.

> Signed-off-by: Vamshi Gajjela <[email protected]>
> ---
> v5:
> - shortlog changed from "serial: core: Make local variable size to
> u64" to "Clean up uart_update_timeout() function"
> - renamed local variable size to temp, generic name
> - removed intermediate variable frame_time
> - added typecast "unsigned int" while assigning to port->frame_time
> v4:
> - no change, not submitted with series
> v3:
> - no change, not submitted with series
> v2:
> - no change, not submitted with series
>
> drivers/tty/serial/serial_core.c | 7 +++----
> 1 file changed, 3 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
> index 7bdc21d5e13b..21d345a9812a 100644
> --- a/drivers/tty/serial/serial_core.c
> +++ b/drivers/tty/serial/serial_core.c
> @@ -410,11 +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 frame_time;
> + u64 temp = tty_get_frame_size(cflag);
>
> - frame_time = (u64)size * NSEC_PER_SEC;
> - port->frame_time = DIV64_U64_ROUND_UP(frame_time, baud);
> + temp *= NSEC_PER_SEC;
> + port->frame_time = (unsigned int)DIV64_U64_ROUND_UP(temp, baud);
> }
> EXPORT_SYMBOL(uart_update_timeout);
>
>

--
i.