Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.35 as permitted sender) client-ip=23.128.96.35;
Date:   Mon, 30 Oct 2023 11:33:30 +0200 (EET)
From:   =?ISO-8859-15?Q?Ilpo_J=E4rvinen?= <ilpo.jarvinen@linux.intel.com>
To:     Vamshi Gajjela <vamshigajjela@google.com>
cc:     Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Jiri Slaby <jirislaby@kernel.org>,
        linux-serial <linux-serial@vger.kernel.org>,
        LKML <linux-kernel@vger.kernel.org>, manugautam@google.com,
        Subhash Jadavani <sjadavani@google.com>,
        Channa Kadabi <kadabi@google.com>
Subject: Re: [PATCH v5 2/2] serial: core: Clean up uart_update_timeout()
 function
In-Reply-To: <20231030073542.251281-3-vamshigajjela@google.com>
Message-ID: <275b9b10-204f-534e-2155-98f623d9f63a@linux.intel.com>
References: <20231030073542.251281-1-vamshigajjela@google.com> <20231030073542.251281-3-vamshigajjela@google.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Precedence: bulk

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.