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

[Rasmus Villemoes]

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@xxxxxxxxx>
>> ---
>> 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