linux-kernel - Re: [PATCH v3 2/7] serial: qcom-geni: Fix the timeout in qcom_geni_serial_poll

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <CAD=FV=VLa1pZv1gy7WONzThopx6RXf_8Uh3L0wCWdY6_Mo1KTA@mail.gmail.com>
Date: Mon, 10 Jun 2024 15:26:15 -0700
From: Doug Anderson <dianders@...omium.org>
To: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
Cc: Greg Kroah-Hartman <gregkh@...uxfoundation.org>, Jiri Slaby <jirislaby@...nel.org>, 
	Uwe Kleine-König <u.kleine-koenig@...gutronix.de>, 
	Andy Shevchenko <andriy.shevchenko@...ux.intel.com>, linux-arm-msm@...r.kernel.org, 
	Konrad Dybcio <konrad.dybcio@...aro.org>, LKML <linux-kernel@...r.kernel.org>, 
	linux-serial <linux-serial@...r.kernel.org>, John Ogness <john.ogness@...utronix.de>, 
	Yicong Yang <yangyicong@...ilicon.com>, Tony Lindgren <tony@...mide.com>, 
	Stephen Boyd <swboyd@...omium.org>, Johan Hovold <johan+linaro@...nel.org>, 
	Bjorn Andersson <andersson@...nel.org>, Thomas Gleixner <tglx@...utronix.de>, 
	Vijaya Krishna Nivarthi <quic_vnivarth@...cinc.com>
Subject: Re: [PATCH v3 2/7] serial: qcom-geni: Fix the timeout in qcom_geni_serial_poll_bit()

Hi,

On Fri, Jun 7, 2024 at 12:50 AM Ilpo Järvinen
<ilpo.jarvinen@...ux.intel.com> wrote:
>
> > +     /*
> > +      * This function is used to poll bits, some of which (like CMD_DONE)
> > +      * might take as long as it takes for the FIFO plus the temp register
> > +      * on the geni side to drain. The Linux core calculates such a timeout
> > +      * for us and we can get it from uart_fifo_timeout().
> > +      *
> > +      * It should be noted that during earlycon the variables that
> > +      * uart_fifo_timeout() makes use of in "uport" may not be setup yet.
> > +      * It's difficult to set things up for earlycon since it can't
> > +      * necessarily figure out the baud rate and reading the FIFO depth
> > +      * from the wrapper means some extra MMIO maps that we don't get by
> > +      * default. This isn't a big problem, though, since uart_fifo_timeout()
> > +      * gives back its "slop" of 20ms as a minimum and that should be
> > +      * plenty of time for earlycon unless we're running at an extremely
> > +      * low baud rate.
> > +      */
> > +     timeout_us = jiffies_to_usecs(uart_fifo_timeout(uport));
>
> Hi,
>
> While this is not exactly incorrect, the back and forth conversions nsecs
> -> jiffies -> usecs feels somewhat odd, perhaps reworking
> uart_fifo_timeout()'s return type from jiffies to e.g. usecs would be
> preferrable. As is, the jiffies as its return type seems a small obstacle
> for using uart_fifo_timeout() which has come up in other contexts too.

Sure. I'll change it to "ms" instead of "us". We don't need the
fidelity of "us" here given that the function is adding 20 ms of slop
anyway so might as well return ms so that callers don't need to do so
much math and don't need to work with u64.

This means that I'll have to add a "* USEC_PER_MSEC" in my driver, but
it still feels like the more correct thing to do. It also has the nice
side effect of allowing the driver to remove the awkward
"DIV_ROUND_UP(timeout_us, 10) * 10" because we know that the timeout
will always be a proper multiple.

I'll also add a new function with the _ms suffix instead of changing
the old one. The suffix makes it clear to the caller what the unit of
the returned value is and we might as well leave the old wrapper
there--otherwise we just need to move the jiffies conversion into the
existing callers.