[PATCH v5 0/2] serial core type consistency and clean up

[PATCH v5 0/2] serial core type consistency and clean up

[Vamshi Gajjela]

This patch series primarily focus on improving type consistency and
ensuring proper handling of timeout values. The changes aim to enhance
the redability and maintainability of the serial core.

Greg, sorry for the confusion.

I started the patch series with 3 patches. I submitted improvised
patches based on review comments for each patch in the series instead
of submitting all the patches at once. This may have caused confusion,
as each patch is now at a different version, with one patch at
v2 (NACKED) the second one ready for v2, and the third at v4.

From now on, I will increment the version for the whole series instead
of for individual patches.

I have now submitted the series as v5 to avoid duplicate version numbers.
In v5 it has two patches [v4 3/3] and [v2 2/3] while [v2 1/3] discarded.

Vamshi Gajjela (2):
  serial: core: Update uart_poll_timeout() function to return unsigned
    long
  serial: core: Clean up uart_update_timeout() function

 drivers/tty/serial/serial_core.c | 7 +++----
 include/linux/serial_core.h      | 4 ++--
 2 files changed, 5 insertions(+), 6 deletions(-)

-- 
2.42.0.820.g83a721a137-goog

[PATCH v5 1/2] serial: core: Update uart_poll_timeout() function to return unsigned long

[Vamshi Gajjela]

The function uart_fifo_timeout() returns an unsigned long value, which
is the number of jiffies. Therefore, change the variable timeout in the
function uart_poll_timeout() from int to unsigned long.
Change the return type of the function uart_poll_timeout() from int to
unsigned long to be consistent with the type of timeout values.

Signed-off-by: Vamshi Gajjela <vamshigajjela@google.com>
---
v5:
- no change. Consistent version for series
v4:
- author name in capitals to lowercase
v3:
- updated description
v2:
- unsigned long instead of unsigned int
- added () after function name in short log
- updated description

 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..6916a1d7e477 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 long uart_poll_timeout(struct uart_port *port)
 {
-	int timeout = uart_fifo_timeout(port);
+	unsigned long timeout = uart_fifo_timeout(port);
 
 	return timeout > 6 ? (timeout / 2 - 2) : 1;
 }
-- 
2.42.0.820.g83a721a137-goog

Re: [PATCH v5 1/2] serial: core: Update uart_poll_timeout() function to return unsigned long

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 1495 bytes --]

On Mon, 30 Oct 2023, Vamshi Gajjela wrote:

> The function uart_fifo_timeout() returns an unsigned long value, which
> is the number of jiffies. Therefore, change the variable timeout in the
> function uart_poll_timeout() from int to unsigned long.
> Change the return type of the function uart_poll_timeout() from int to
> unsigned long to be consistent with the type of timeout values.
> 
> Signed-off-by: Vamshi Gajjela <vamshigajjela@google.com>
> ---
> v5:
> - no change. Consistent version for series
> v4:
> - author name in capitals to lowercase
> v3:
> - updated description
> v2:
> - unsigned long instead of unsigned int
> - added () after function name in short log
> - updated description
> 
>  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..6916a1d7e477 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 long uart_poll_timeout(struct uart_port *port)
>  {
> -	int timeout = uart_fifo_timeout(port);
> +	unsigned long timeout = uart_fifo_timeout(port);
>  
>  	return timeout > 6 ? (timeout / 2 - 2) : 1;
>  }
> 

Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>

-- 
 i.

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

[Vamshi Gajjela]

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 <vamshigajjela@google.com>
---
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

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

[Ilpo Järvinen]

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 <vamshigajjela@google.com>
> ---
> 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.