Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.38 as permitted sender) client-ip=23.128.96.38;
Message-ID: <fc843c33-03e4-4603-9cc9-0b30b2c21c66@kontron.de>
Date:   Wed, 22 Nov 2023 14:01:20 +0100
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH] serial: imx: also enable Transmit Complete interrupt in
 rs232 mode
To:     Rasmus Villemoes <rasmus.villemoes@prevas.dk>,
        Eberhard Stoll <eberhard.stoll@gmx.de>, sherry.sun@nxp.com
Cc:     festevam@gmail.com, gregkh@linuxfoundation.org,
        jirislaby@kernel.org, kernel@pengutronix.de,
        linux-arm-kernel@lists.infradead.org, linux-imx@nxp.com,
        linux-kernel@vger.kernel.org, linux-serial@vger.kernel.org,
        s.hauer@pengutronix.de, shawnguo@kernel.org,
        eberhard.stoll@kontron.de
References: <AS8PR04MB8404D066C247F5B2979CBD1F92BBA@AS8PR04MB8404.eurprd04.prod.outlook.com>
 <dbe5959d-2b68-4f16-89ee-32538aab4f34@gmx.de>
 <5ad44085-c90b-4a88-bb7b-8ddc2b612793@prevas.dk>
Content-Language: en-US, de-DE
From:   Frieder Schrempf <frieder.schrempf@kontron.de>
In-Reply-To: <5ad44085-c90b-4a88-bb7b-8ddc2b612793@prevas.dk>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?eS9lMHdGRFRHMC9TRm53RlBndm84cjZ6eTB3TU9rLzk2cjVhUDlWendXSEtM?=
 =?utf-8?B?L2hDckFqKzEySnBKbjdLTG1FS3dMYnl2Q3p1c0FFc0t4WEQzSDErNHJPdUgx?=
 =?utf-8?B?T0ZLMFYzV0xDOHpBSmpPb0JSRFVWeGdrcURDd3puaTZET0RGZDd5QkpCcDVU?=
 =?utf-8?B?Y1ZFblFjMU9BNUR6T0xUeWlHRlZLMDA3T1hyTDZWZWM1b2hnWTAvSkNtYVlK?=
 =?utf-8?B?UlFuQ2dtMGVjWno5NFAzR2d2bkhaaXJOcW5jZ0dMNTlLODVwSjR5ajAvM0s1?=
 =?utf-8?B?RHkyUEwxdytSblV1eWREWWx5TWtyc2RQdTArZHNlTHAxeHdUVHhYeks1ZW9M?=
 =?utf-8?B?bFlrTXZWZmR0QVYyQ2pHdS9FK0hicGtnV1R6ekJiOGlRaHBmc3BHVjgvQmVa?=
 =?utf-8?B?UllZWHRCZ3daeWY4RlpZZVhpWFkyaVJWV3hEaFdFd245V2c3VUdtcWJqaG9n?=
 =?utf-8?B?dkNxQmcycEhUYytBaE1OQ01seUxnc1V1UEJZaGc4V3JTR0FhbHg0alJUNjVk?=
 =?utf-8?B?VWJ5VHJaSjluUnV4SnNCY3FHZlNjVTNwMk45cUxmLzhVMldoUFhvaXJnMElz?=
 =?utf-8?B?NmczaGhUU1Z6WTdLUmtNU0F6R04wZEFwY2xmR1VPYXlnaTZyN2o0M3VsZ3Jz?=
 =?utf-8?B?a3M3SjFUeGpUQnEwWnlNQkVCSlBoMUZhY1V6ZWROT1pvQkRoamgyYnNnNVdC?=
 =?utf-8?B?dWwycFdwbjZOUmlUQTJ6bldhWW1HeUcyRGRtdFJwL0IveEQ4K25WQzg0NSt2?=
 =?utf-8?B?Y1Z6bWw1MFJ6Q0tteEJVZFZJQWl4UFRSN2d4M3laOGFWRlhSVXRjM2JHU28v?=
 =?utf-8?B?M3VXSkh5c0NZZnBwamJRc3F6bThpQ2E1RW9IWjR3MnIyZk9NS3UrZUY4TExw?=
 =?utf-8?B?TDhlTmd1aGVZUDlSTHhudFZqSnJxNW1pV2g3N09DMUV4VmcwVWx4cUNDMUJM?=
 =?utf-8?B?cjdWVE5EaC9QWjIzQ29zSVdvS01VVXhyL3FNeHM1SjRJaGYzODJXNmRZdEc2?=
 =?utf-8?B?Qm9YNGtGMkQ1K2E0L3VOU3pXZU5Bb2kwWGwrQjBuMDd4T0xmQ2JxNzM3WGg4?=
 =?utf-8?B?ZjJqaExuWCtNSTM2UlJUUDRkU3IvMUJPTHhCK0l2bm96UU40c2FtMUhZTUlL?=
 =?utf-8?B?REQ1cTVPMlEwRnBTVEp5MlB2eDQvc3krOEIzd2MxT1RsZ2JLdWo4R04xOFBT?=
 =?utf-8?B?RVJEa2FzUUszb1FVY2I2RW5LSFQwNUJWbG1BOWhtTmwwZnhGRENMM2JxWFJY?=
 =?utf-8?B?YXV4T28zVDBFZE1IdVBnSGFiZjVpQjBTMndld3Nyb2R3c1hwNDhqbVBZRkZY?=
 =?utf-8?B?eS9LR3dHcjB4dlA0WDFGMFZMc2RFRUQrbFJLbjEwTDluSUZuYzFOTE1CbzFO?=
 =?utf-8?B?ekczaTFrb3owaHQvOEFPdnhGVVdUekRYUWM2OHNkdkw0eUZ6UUk5UExCZ09i?=
 =?utf-8?B?dDFUeHFGQmo0aXlhTElkMmdZQmsrTEdrUU5BS3JnWjdRT2pJNE1BMUc5N1dB?=
 =?utf-8?B?a2x3SVFMdm5PY25vdjJEcEIwTGdnc1Z1ZHRROEExZE00UWppMFZnMGNlTGZI?=
 =?utf-8?B?RTJlOVJnYjlzSWpwcmdObFdwS2ZVckJGWVVDTkQ4VTYvSkthN1FsZU5XeXJD?=
 =?utf-8?B?MTI3TXZSSCtYbVJtdEJFWFJsNStwbERqaXAzYm4vQzJMYWIyLzlNS1FBczBW?=
 =?utf-8?B?VzBZU29DRUZmVHcvam5oMjhpcnFrQU9JeVQ2bDh4M2grZG43T3lUaUdxRkMw?=
 =?utf-8?B?WHM0ckxZVU9rTEg0dkhmL0RHd0RPY2VHcktNb1V5MzN0ekNwTmFqcGRvRHBw?=
 =?utf-8?B?Vk1rYjJkVWtha0RJT1lGL2ZCTDIzeEc5M2FhdUxzblE1WTVUclYyaVBvdFQv?=
 =?utf-8?B?Smp5U2hxZGtOZFJ1UFQ4bVpGNCtQb2hxblN3ZVgraEdlTVd4c2JYQmhwYVND?=
 =?utf-8?B?WUNqQW9DdHBSVE8vYlRHV1h5R2dEVUd0MFJxSWg1YWZ2M3ZweHdSVE1ZY0Zz?=
 =?utf-8?B?cFh5aFM2MzlnRnhMODBWaXBVbXR1Z3EvazIybmJCc203UmM2SGFTMEJmZ3Nj?=
 =?utf-8?B?M284UGVSM1FyVnRqTTlYeDVVOEpWaDg4Q0RobS92Skpib3R1QStlaUhBeTlt?=
 =?utf-8?B?VWttQVlMMGJPajBzelZkMkRQdFBvMDFkY0YvcUtxMTVEMUZEVU03SXExT0wy?=
 =?utf-8?B?N2c9PQ==?=
X-OriginatorOrg: kontron.de
X-MS-Exchange-CrossTenant-Network-Message-Id: b09cfec2-5899-41fb-ad1d-08dbeb5b2193
X-MS-Exchange-CrossTenant-AuthSource: PA4PR10MB5681.EURPRD10.PROD.OUTLOOK.COM
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 22 Nov 2023 13:01:24.2376
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 8c9d3c97-3fd9-41c8-a2b1-646f3942daf1
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: wtrjYW6pzF5PFsW58UXGdyj07G5tnQou9qhSNG0B5vAlaFZI6InBAw+rai2rQBSxZnpML3Gyp86iKbeI9sGA2lr9D2jD+nCwLZrOk7RGWfk=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PAWPR10MB7747
X-Spam-Status: No, score=-0.8 required=5.0 tests=DKIM_SIGNED,DKIM_VALID,
	HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,SPF_HELO_NONE,
	SPF_PASS,T_SCC_BODY_TEXT_LINE autolearn=unavailable autolearn_force=no
	version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on fry.vger.email
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
X-Greylist: Sender passed SPF test, not delayed by milter-greylist-4.6.4 (fry.vger.email [0.0.0.0]); Wed, 22 Nov 2023 05:01:52 -0800 (PST)

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@prevas.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.

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.