Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20211029201402.428284-1-wander@redhat.com> <CAHp75VeZBp4gKvGBDzaD=EpGRDZ1-wTvD8K9Ui6Q59kDjmkXmQ@mail.gmail.com>
In-Reply-To: <CAHp75VeZBp4gKvGBDzaD=EpGRDZ1-wTvD8K9Ui6Q59kDjmkXmQ@mail.gmail.com>
From:   Wander Costa <wcosta@redhat.com>
Date:   Mon, 1 Nov 2021 12:22:25 -0300
Message-ID: <CAAq0SUmzHqEzNk3aw3SEgYVWRukQeHK1WtcJ3MjXcQKJrbC1Dw@mail.gmail.com>
Subject: Re: [PATCH] tty: serial: Use fifo in 8250 console driver
To:     Andy Shevchenko <andy.shevchenko@gmail.com>
Cc:     wander@redhat.com, Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Jiri Slaby <jirislaby@kernel.org>,
        "Maciej W. Rozycki" <macro@orcam.me.uk>,
        Johan Hovold <johan@kernel.org>,
        Andrew Jeffery <andrew@aj.id.au>,
        "open list:SERIAL DRIVERS" <linux-serial@vger.kernel.org>,
        open list <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

Em s=C3=A1b., 30 de out. de 2021 04:41, Andy Shevchenko
<andy.shevchenko@gmail.com> escreveu:
>
>
>
> On Friday, October 29, 2021, <wander@redhat.com> wrote:
>>
>> From: Wander Lairson Costa <wander@redhat.com>
>>
>> Note: I am using a small test app + driver located at [0] for the
>
>
> I don't see any links.

Oops, sorry about that. I must have accidentally deleted it while
editing the commit message.
Here it is https://github.com/walac/serial-console-test.
I will update the patch with the link.

> While at it, why can't you integrate your changes to [1], for example?
>
> [1]: https://github.com/cbrake/linux-serial-test
>
First of all, I was not aware of it, but afaik, /dev/ttyS doesn't
follow the same code path as
printk, which was my main goal here (I should have made this clear in
the commit message, my bad).


>

[snip]

>>
>
>
> On how many different UARTs have you tested this? Have you tested oops an=
d NMI contexts?
>
I only tested in a half dozen machines that I have available. I tried
it in panic, warnings, IRQ contexts, etc. Theoretically, this change
should not be affected by the context. Theoretically...

> What I would like to say here is that the code is being used on zillions =
of different 8250 implementations here and I would be rather skeptical abou=
t enabling the feature for everyone.
>
I did my homework and studied the 16550 datasheets, but yes, there is
always this risk. Maybe people more experienced with PC serial ports
than me might think the patch is not worth the risk of breaking some
unknown number of devices out there, and I am ok with that. It is a
valid point.


>>
>> Signed-off-by: Wander Lairson Costa <wander@redhat.com>
>> ---
>>  drivers/tty/serial/8250/8250_port.c | 61 ++++++++++++++++++++++++++---
>>  1 file changed, 55 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/82=
50/8250_port.c
>> index 66374704747e..edf88a8338a2 100644
>> --- a/drivers/tty/serial/8250/8250_port.c
>> +++ b/drivers/tty/serial/8250/8250_port.c
>> @@ -2063,10 +2063,7 @@ static void serial8250_break_ctl(struct uart_port=
 *port, int break_state)
>>         serial8250_rpm_put(up);
>>  }
>>
>> -/*
>> - *     Wait for transmitter & holding register to empty
>> - */
>> -static void wait_for_xmitr(struct uart_8250_port *up, int bits)
>> +static void wait_for_lsr(struct uart_8250_port *up, int bits)
>>  {
>>         unsigned int status, tmout =3D 10000;
>>
>> @@ -2083,6 +2080,16 @@ static void wait_for_xmitr(struct uart_8250_port =
*up, int bits)
>>                 udelay(1);
>>                 touch_nmi_watchdog();
>>         }
>> +}
>> +
>> +/*
>> + *     Wait for transmitter & holding register to empty
>> + */
>> +static void wait_for_xmitr(struct uart_8250_port *up, int bits)
>> +{
>> +       unsigned int tmout;
>> +
>> +       wait_for_lsr(up, bits);
>>
>>         /* Wait up to 1s for flow control if necessary */
>>         if (up->port.flags & UPF_CONS_FLOW) {
>> @@ -3319,6 +3326,35 @@ static void serial8250_console_restore(struct uar=
t_8250_port *up)
>>         serial8250_out_MCR(up, UART_MCR_DTR | UART_MCR_RTS);
>>  }
>>
>> +/*
>> + * Print a string to the serial port using the device FIFO
>> + *
>> + * It sends fifosize bytes and then waits for the fifo
>> + * to get empty.
>> + */
>> +static void serial8250_console_fifo_write(struct uart_8250_port *up,
>> +               const char *s, unsigned int count)
>> +{
>> +       int i;
>> +       const char *end =3D s + count;
>> +       unsigned int fifosize =3D up->port.fifosize;
>> +       bool cr_sent =3D false;
>> +
>> +       while (s !=3D end) {
>> +               wait_for_lsr(up, UART_LSR_THRE);
>> +
>> +               for (i =3D 0; i < fifosize && s !=3D end; ++i) {
>> +                       if (*s =3D=3D '\n' && !cr_sent) {
>> +                               serial_out(up, UART_TX, '\r');
>> +                               cr_sent =3D true;
>> +                       } else {
>> +                               serial_out(up, UART_TX, *s++);
>> +                               cr_sent =3D false;
>> +                       }
>> +               }
>> +       }
>> +}
>> +
>>  /*
>>   *     Print a string to the serial port trying not to disturb
>>   *     any possible real use of the port...
>> @@ -3334,7 +3370,7 @@ void serial8250_console_write(struct uart_8250_por=
t *up, const char *s,
>>         struct uart_8250_em485 *em485 =3D up->em485;
>>         struct uart_port *port =3D &up->port;
>>         unsigned long flags;
>> -       unsigned int ier;
>> +       unsigned int ier, use_fifo;
>>         int locked =3D 1;
>>
>>         touch_nmi_watchdog();
>> @@ -3366,7 +3402,20 @@ void serial8250_console_write(struct uart_8250_po=
rt *up, const char *s,
>>                 mdelay(port->rs485.delay_rts_before_send);
>>         }
>>
>> -       uart_console_write(port, s, count, serial8250_console_putchar);
>> +       use_fifo =3D (up->capabilities & UART_CAP_FIFO)
>> +               && port->fifosize > 1
>> +               && (serial_port_in(port, UART_FCR) & UART_FCR_ENABLE_FIF=
O)
>> +               /*
>> +                * After we put a data in the fifo, the controller will =
send
>> +                * it regardless of the CTS state. Therefore, only use f=
ifo
>> +                * if we don't use control flow.
>> +                */
>> +               && !(up->port.flags & UPF_CONS_FLOW);
>> +
>> +       if (likely(use_fifo))
>> +               serial8250_console_fifo_write(up, s, count);
>> +       else
>> +               uart_console_write(port, s, count, serial8250_console_pu=
tchar);
>>
>>         /*
>>          *      Finally, wait for transmitter to become empty
>> --
>> 2.27.0
>>
>
>
> --
> With Best Regards,
> Andy Shevchenko
>
>