Received-SPF: pass (google.com: domain of linux-kernel+bounces-212121-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Precedence: bulk
MIME-Version: 1.0
References: <20240610222515.3023730-1-dianders@chromium.org>
 <20240610152420.v4.2.I65a6430ab75f74d20c28b5c5f819dd5b8455933d@changeid> <0bb414fa-851b-40cf-ede9-fc6252c6b173@linux.intel.com>
In-Reply-To: <0bb414fa-851b-40cf-ede9-fc6252c6b173@linux.intel.com>
From: Doug Anderson <dianders@chromium.org>
Date: Wed, 12 Jun 2024 11:45:00 -0700
Message-ID: <CAD=FV=WWrTHRdwrbOFBrZ94HpWQo6v6FkLTxa1vgN31SC6GqDA@mail.gmail.com>
Subject: Re: [PATCH v4 2/8] tty: serial: Add uart_fifo_timeout_ms()
To: =?UTF-8?Q?Ilpo_J=C3=A4rvinen?= <ilpo.jarvinen@linux.intel.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>, Jiri Slaby <jirislaby@kernel.org>, 
	Yicong Yang <yangyicong@hisilicon.com>, Tony Lindgren <tony@atomide.com>, 
	Andy Shevchenko <andriy.shevchenko@linux.intel.com>, Johan Hovold <johan+linaro@kernel.org>, 
	John Ogness <john.ogness@linutronix.de>, linux-arm-msm@vger.kernel.org, 
	Bjorn Andersson <andersson@kernel.org>, Konrad Dybcio <konrad.dybcio@linaro.org>, 
	Stephen Boyd <swboyd@chromium.org>, linux-serial <linux-serial@vger.kernel.org>, 
	LKML <linux-kernel@vger.kernel.org>, 
	=?UTF-8?Q?Uwe_Kleine=2DK=C3=B6nig?= <u.kleine-koenig@pengutronix.de>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

Hi,

On Wed, Jun 12, 2024 at 12:38=E2=80=AFAM Ilpo J=C3=A4rvinen
<ilpo.jarvinen@linux.intel.com> wrote:
>
> On Mon, 10 Jun 2024, Douglas Anderson wrote:
>
> > The current uart_fifo_timeout() returns jiffies, which is not always
> > the most convenient for callers. Add a variant uart_fifo_timeout_ms()
> > that returns the timeout in milliseconds.
> >
> > NOTES:
> > - msecs_to_jiffies() rounds up, unlike nsecs_to_jiffies(). This is
> >   because msecs_to_jiffies() is actually intended for device drivers
> >   to calculate timeout value. This means we don't need to take the max
> >   of the timeout and "1" since the timeout will always be > 0 ms (we
> >   add 20 ms of slop).
> > - uart_fifo_timeout_ms() returns "unsigned int" but we leave
> >   uart_fifo_timeout() returning "unsigned long". This matches the
> >   types of msecs_to_jiffies().
> >
> > Suggested-by: Ilpo J=C3=A4rvinen <ilpo.jarvinen@linux.intel.com>
> > Signed-off-by: Douglas Anderson <dianders@chromium.org>
> > ---
> >
> > Changes in v4:
> > - New
> >
> >  include/linux/serial_core.h | 15 +++++++++++----
> >  1 file changed, 11 insertions(+), 4 deletions(-)
> >
> > diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h
> > index 8cb65f50e830..97968acfd564 100644
> > --- a/include/linux/serial_core.h
> > +++ b/include/linux/serial_core.h
> > @@ -889,14 +889,21 @@ unsigned int uart_get_divisor(struct uart_port *p=
ort, unsigned int baud);
> >  /*
> >   * Calculates FIFO drain time.
> >   */
> > -static inline unsigned long uart_fifo_timeout(struct uart_port *port)
> > +static inline unsigned int uart_fifo_timeout_ms(struct uart_port *port=
)
> >  {
> >       u64 fifo_timeout =3D (u64)READ_ONCE(port->frame_time) * port->fif=
osize;
> > +     unsigned int fifo_timeout_ms =3D div_u64(fifo_timeout, NSEC_PER_M=
SEC);
> >
> > -     /* Add .02 seconds of slop */
> > -     fifo_timeout +=3D 20 * NSEC_PER_MSEC;
> > +     /*
> > +      * Add .02 seconds of slop. This also helps account for the fact =
that
> > +      * when we converted from ns to ms that we didn't round up.
> > +      */
> > +     return fifo_timeout_ms + 20;
> > +}
> >
> > -     return max(nsecs_to_jiffies(fifo_timeout), 1UL);
> > +static inline unsigned long uart_fifo_timeout(struct uart_port *port)
> > +{
> > +     return msecs_to_jiffies(uart_fifo_timeout_ms(port));
> >  }
>
> Hi,
>
> This is definitely towards the right direction! However, it now does
> double conversion, first div_u64() and then msecs_to_jiffies(). Perhaps i=
t
> would be better to retain the nsecs version (maybe rename it to _ns for
> consistency) and add _ms variant that does the nsec -> msec conversion.

I spent a bit of time thinking about it and I don't agree. If you feel
very strongly about it or someone else wants to jump in and break the
tie then I can look again, but:

1. The comment before nsecs_to_jiffies() specifically states that it's
not supposed to be used for this purpose. Specifically, it says:

 * Unlike {m,u}secs_to_jiffies, type of input is not unsigned int but u64.
 * And this doesn't return MAX_JIFFY_OFFSET since this function is designed
 * for scheduler, not for use in device drivers to calculate timeout value.

...so switching away from nsecs_to_jiffies() to msecs_to_jiffies() is
arguably a "bugfix", or at least avoids using the API in a way that's
against the documentation.

2. As mentioned in the commit message, nsecs_to_jiffies() truncates
where msecs_to_jiffies() rounds up. Presumably this difference is
related to the comment above where the "ns" version is intended for
the scheduler. Using the "ms" version allows us to get rid of the
extra call to "max()" which is a benefit. Technically since the
timeout is at least 20 ms the minimum HZ is 100 I guess we didn't need
the max anyway, but I guess someone thought it was cleaner and now we
can definitely get rid of it.

3. These functions are inline anyway, so I don't think it's causing a
huge bloat of instructions. In fact, moving from 64-bit math to 32-bit
math sooner could make the code smaller.

4. I don't feel like it hurts the readability to convert down to ms
and then to jiffies. In fact, IMO it helps since it makes it more
obvious that we're working with ms.