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

[Frieder Schrempf]

On 22.11.23 09:03, Rasmus Villemoes wrote:
> 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.

Is there a valid use-case for switching the mode while the device is
transmitting? Is this something we need to support for whatever reason?
It sounds rather an obscure thing to do.

If not I would strongly vote for disallowing it in favor of a more
stable and less complex driver.