linux-kernel - Re: [PATCH] serial: imx: also enable Transmit Complete interrupt in rs232 mode

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <5ad44085-c90b-4a88-bb7b-8ddc2b612793@prevas.dk>
Date:   Wed, 22 Nov 2023 09:03:18 +0100
From:   Rasmus Villemoes <rasmus.villemoes@...vas.dk>
To:     Eberhard Stoll <eberhard.stoll@....de>, sherry.sun@....com
Cc:     festevam@...il.com, gregkh@...uxfoundation.org,
        jirislaby@...nel.org, kernel@...gutronix.de,
        linux-arm-kernel@...ts.infradead.org, linux-imx@....com,
        linux-kernel@...r.kernel.org, linux-serial@...r.kernel.org,
        s.hauer@...gutronix.de, shawnguo@...nel.org,
        eberhard.stoll@...tron.de
Subject: Re: [PATCH] serial: imx: also enable Transmit Complete interrupt in
 rs232 mode

On 21/11/2023 21.49, Eberhard Stoll wrote:
>> Currently, if one switches to rs232 mode, writes something to the
>> device, and then switches to rs485 mode, the imx_port's ->tx_state is
>> left as SEND. This then prevents a subsequent write in rs485 mode from
>> properly asserting the rts pin (i.e. enabling the transceiver),
>> because imx_uart_start_rx() does not enter the "if (sport->tx_state ==
>> OFF)" branch. Hence nothing is actually transmitted.
>>
>> The problem is that in rs232 mode, ->tx_state never gets set to OFF,
>> due to
>>
>>     usr2 = imx_uart_readl(sport, USR2);
>>     if (!(usr2 & USR2_TXDC)) {
>>         /* The shifter is still busy, so retry once TC triggers */
>>         return;
>>     }
>>
>> in imx_uart_stop_tx(), and TC never triggers because the Transmit
>> Complete interrupt is not enabled for rs232.
>>
>> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@...vas.dk>
>> ---
>> I'm not sure this is the best fix.
>>
>> At first I considered doing something much more targeted, but
>> definitely also more hacky: In imx_uart_rs485_config(), if switching
>> on rs485 mode, simply add "sport->tx_state = OFF;".
>>
>> If someone has a better suggestion, I'm all ears.
> 
> 
> Hello Rasmus,
> 
> i can observe a very similar situation, but with a litte different
> configuration. This is how i can trigger the situation very quickly:
> 
>   1) open the port
>   2) send 1 byte out
>   3) close the port

Hi Eberhard

Thanks for chiming in. I assume this is all in rs485 mode, no switching
to rs232 and back involved?


> Do it in a loop. As faster, the lockup may occur earlier (but not
> mandatory, 100ms is sufficient in my setup at 115200 Baud on an
> i.mx8mm board).
> With this configuration i get the lockup in around 1 minute.
> 
> For my setup it's clear what happens:
> 
>   - when the tty is closed imx_uart_shutdown() is called. This calls
>     imx_uart_stop_tx()
>   - for a lockup, the shifter is still busy and imx_uart_stop_tx()
>     returns early (as you explained) without modifying ->tx_state.
>   - imx_uart_shutdown() proceeds and finally closes the port. Due to
>     imx_uart_stop_tx() is not executed completely tx_state is left in
>     state ->tx_state == SEND.

Yes, and imx_uart_shutdown() disables the TCEN which would otherwise
cause _stop_tx to get called when the transmitter is no longer busy.

>   - When the port is opened again, tx_state is SEND and nothing can
>     be transmitted any more. The tx path has locked up!
> 
> Setting ->tx_state = SEND in imx_uart_shutdown() helps for my issue
> (and should be ok IMHO).

[I assume you mean tx_state = OFF]. Yes, I suppose doing that would be
ok, but I'm not sure it's a complete fix. In my simple test cast, I have
separate programs invoked to do the I/O and do the mode switch, but in a
real scenario, I'd expect the application itself to just open the device
once, and then do I/O and mode switching as appropriate for the
application logic, and I don't think uart_shutdown would then ever get
called.

> But IMHO there is one next issue with this situation: When the port
> operates with WAIT_AFTER_RTS and WAIT_AFTER_SEND then some timers
> for callback functions might be active. I did not discover where they
> are stopped for the case when the serial port is closed. Maybe stopping
> is not required ...

Indeed, that's an extra complication. Adding two hrtimer_try_to_cancel()
in shutdown would probably not hurt, along with setting tx_state OFF.

I wonder if at least mode switching should simply be disallowed (-EBUSY)
if tx_state is anything but OFF.

Rasmus