Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <bce6407bc42eeec8f459f09bc1326761327c98ec.camel@ew.tq-group.com>
Subject: Re: 8250_omap: incorrect RS485 RTS on close during transmission
From:   Matthias Schiffer <matthias.schiffer@ew.tq-group.com>
To:     Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Jiri Slaby <jirislaby@kernel.org>,
        Lukas Wunner <lukas@wunner.de>
Cc:     Ilpo =?ISO-8859-1?Q?J=E4rvinen?= <ilpo.jarvinen@linux.intel.com>,
        linux-serial@vger.kernel.org, linux-kernel@vger.kernel.org
Date:   Tue, 22 Nov 2022 09:52:40 +0100
In-Reply-To: <f7cf39a9c152031a09608a68115e32988617f626.camel@ew.tq-group.com>
References: <f7cf39a9c152031a09608a68115e32988617f626.camel@ew.tq-group.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
User-Agent: Evolution 3.44.4-0ubuntu1 
MIME-Version: 1.0
Precedence: bulk

On Fri, 2022-11-18 at 12:57 +0100, Matthias Schiffer wrote:
> Hi Lukas,
>=20
> I've discovered a new issue with EM485 in 8250_omap (and probably other
> serial drivers as well?): When closing the TTY device while there is
> still an ongoing transmission, the RTS pin will get stuck active until
> the TTY is opened again. This can be easily reproduced by running `cat
> /dev/zero > /dev/ttySx` and then stopping it using Ctrl-C.
>=20
> The issue exists in current mainline (6.1-rc5+), and applying "serial:
> 8250: 8250_omap: Avoid RS485 RTS glitch on ->set_termios()" from next
> doesn't solve it. I think it should work with "serial: 8250: 8250_omap:
> Support native RS485", but we also need RS485 on AM57xx CPUs, which
> don't have native RS485.
>=20
> I intend to look into this myself next week, but as you've worked on
> this code a lot lately, maybe you already have an idea how to fix it?
>=20
> Regards,
> Matthias
>=20

Okay, I've narrowed down the issue to uart_shutdown() ->
uart_port_shutdown() - here, ops->shutdown() is called, which clears
the FIFOs (in the case of the default 8250_port and 8250_omap)
implementations, but doesn't wait for the clear to finish, and doesn't
call ops->stop_tx().

I believe a similar problem can occur in uart_suspend_port(). Here,
ops->stop_tx() is called before ops->shutdown(), but stop_tx() is a no-
op when the TX FIFO is not empty (again, for the 8250 case - I haven't
really looked at other drivers).

Given that EM485 is handled in 8250_port by __stop_tx(), which is
called both internally in the 8250 driver and from the serial core
through stop_tx(), it is not clear to me what the correct layer to fix
this is. Two ideas:

(1) Have ops->shutdown() wait until the TX FIFO is empty. Call
ops->stop_tx() after ops->shutdown() in serial core. For this to work,
calling stop_tx() after shutdown() must be allowed in all drivers; I'm
not sure if this is currently the case, and if it is desirable at all.

(2) Do the whole handling in the driver's ops->shutdown(), do not
further involve the serial core.

So far, I've implemented (1) locally for 8250_omap, and together with
"serial: 8250: 8250_omap: Avoid RS485 RTS glitch on ->set_termios()" it
fixes the issue on my test devices, but I'm not too happy about the
"shutdown then tx_stop" order of calls. So maybe (2) is the better
solution after all?