Regression: serial: imx: overrun errors on debug UART

[Stefan Wahren]

Hi,

Am 25.03.23 um 16:11 schrieb Uwe Kleine-König:
> Hello,
> 
> On Sat, Mar 25, 2023 at 12:31:01PM +0100, Stefan Wahren wrote:
>> Am 24.03.23 um 16:00 schrieb Stefan Wahren:
>>> Am 24.03.23 um 13:57 schrieb Fabio Estevam:
>>>> On Fri, Mar 24, 2023 at 8:48 AM Ilpo Järvinen
>>>> <ilpo.jarvinen at linux.intel.com> wrote:
>>>>
>>>>> This has come up earlier, see e.g.:
>>>>>
>>>>> https://lore.kernel.org/linux-serial/20221003110850.GA28338@francesco-nb.int.toradex.com/
>>>>>
>>>>> My somewhat uninformed suggestion: if the overrun problems
>>>>> mostly show up
>>>>> with console ports, maybe the trigger level could depend on the port
>>>>> being a console or not?
>>>> Does the change below help? Taking Ilpo's suggestion into account:
>>> this breaks the boot / debug console completely, but i got the idea.
>>>>
>>
>> based on your patch, i successfully tested this:
>>
>> diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
>> index f07c4f9ff13c..1aacaa637ede 100644
>> --- a/drivers/tty/serial/imx.c
>> +++ b/drivers/tty/serial/imx.c
>> @@ -1277,6 +1277,7 @@ static void imx_uart_clear_rx_errors(struct imx_port
>> *sport)
>>   }
>>
>>   #define TXTL_DEFAULT 2 /* reset default */
>> +#define RXTL_DEFAULT_CONSOLE 1 /* reset default */
>>   #define RXTL_DEFAULT 8 /* 8 characters or aging timer */
>>   #define TXTL_DMA 8 /* DMA burst setting */
>>   #define RXTL_DMA 9 /* DMA burst setting */
>> @@ -1286,6 +1287,9 @@ static void imx_uart_setup_ufcr(struct imx_port
>> *sport,
>>   {
>>   	unsigned int val;
>>
>> +	if (uart_console(&sport->port))
>> +		rxwl = RXTL_DEFAULT_CONSOLE; // fallback
>> +
>>   	/* set receiver / transmitter trigger level */
>>   	val = imx_uart_readl(sport, UFCR) & (UFCR_RFDIV | UFCR_DCEDTE);
>>   	val |= txwl << UFCR_TXTL_SHF | rxwl;
> 
> So the current theory that the issue occurs because of a combination of:
> 
>   - With a higher watermark value the irq triggers later and so there is
>     less time the until the ISR must run before an overflow happens; and
> 
>   - serial console activity disables irqs for a (relative) long time
> 
> right?
> 
> So on an UP system the problem should occur also on a non-console port?
> Local irqs are only disabled if some printk is about to be emitted,
> isn't it? Does this match the error you're seeing?
> 
> That makes me wonder if the error doesn't relate to the UART being a
> console port, but the UART being used without DMA?! (So the patch above
> fixes the problem for you because on the console port no DMA is used?)

today i had time to do some testing. At first i tested with different 
RXTL_DEFAULT values.

1 No overrun
2 No overrun
4 No overrun
8 Overruns

After that i look at the # echo 0 > /proc/sys/kernel/printk approach, 
but this didn't change anything. The kernel is usually silent about log 
message after boot and the console works still with echo. Enforcing some 
driver to call printk periodically would make the console unusuable.

Finally i tried to disabled the spin_lock in imx_uart_console_write:

diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
index f07c4f9ff13c..c342559ff1a2 100644
--- a/drivers/tty/serial/imx.c
+++ b/drivers/tty/serial/imx.c
@@ -2007,14 +2007,12 @@ imx_uart_console_write(struct console *co, const 
char *s, unsigned int count)
  	struct imx_port_ucrs old_ucr;
  	unsigned long flags;
  	unsigned int ucr1;
-	int locked = 1;
+	int locked = 0;

  	if (sport->port.sysrq)
  		locked = 0;
  	else if (oops_in_progress)
  		locked = spin_trylock_irqsave(&sport->port.lock, flags);
-	else
-		spin_lock_irqsave(&sport->port.lock, flags);

  	/*
  	 *	First, save UCR1/2/3 and then disable interrupts

But the overruns still occured. Is this because the serial core already 
helds a lock?

> 
> Best regards
> Uwe
>