Received-SPF: pass (google.com: domain of linux-kernel+bounces-126560-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Date: Mon, 1 Apr 2024 11:39:15 +0200
From: Greg KH <gregkh@linuxfoundation.org>
To: "fengchunguo@126.com" <fengchunguo@126.com>
Cc: ilpo.jarvinen@linux.intel.com, andriy.shevchenko@linux.intel.com,
	jirislaby@kernel.org, linux-kernel@vger.kernel.org,
	"chunguo.feng" <chunguo.feng@semidrive.com>
Subject: Re: [PATCH] tty: serial: fixed uart irq maybe cause irq storm
Message-ID: <2024040147-stinging-payback-04f5@gregkh>
References: <1711963801-184362-1-git-send-email-fengchunguo@126.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <1711963801-184362-1-git-send-email-fengchunguo@126.com>

On Mon, Apr 01, 2024 at 05:30:01PM +0800, fengchunguo@126.com wrote:
> From: "chunguo.feng" <chunguo.feng@semidrive.com>

Please use your name, which is not with a "." in it, right?

> if not disable uart irq before disable clk,  uart irq maybe triggered after
> disabled clk immediately, then maybe cause irq storm.
> 
> Reproduced the below call trace, see i2c not be connected, but irq storm
> was triggered.
> 
> i2c_designware 30b70000.i2c: controller timed out
> CPU: 2 PID: 2932 Comm: gnss@1.0-servic 
> Tainted: G O 5.14.61-00094-geaa0149416cc-dirty #8
> Hardware name: Semidrive kunlun x9 REF Board (DT)
> Call trace:
> [<ffff00000808a3cc>] dump_backtrace+0x0/0x3c0
> [<ffff00000808a7a0>] show_stack+0x14/0x1c
> [<ffff000008cef43c>] dump_stack+0xc4/0xfc
> [<ffff00000814eb80>] __report_bad_irq+0x50/0xe0
> [<ffff00000814eaec>] note_interrupt+0x248/0x28c
> [<ffff00000814c0e8>] handle_irq_event+0x78/0xa4
> [<ffff00000814fcb8>] handle_fasteoi_irq+0xe4/0x1b4
> [<ffff00000814b060>] __handle_domain_irq+0x7c/0xbc
> [<ffff00000808176c>] gic_handle_irq+0x4c/0xb8
> [<ffff000008083230>] el1_irq+0xb0/0x124
> [<ffff000008d09f5c>] _raw_spin_unlock_irqrestore+0x10/0x44
> [<ffff00000865b784>] dw8250_set_termios+0x48/0xf4
> [<ffff0000086545a4>] serial8250_set_termios+0x14/0x28
> [<ffff00000864c4f4>] uart_change_speed+0x38/0x10c
> [<ffff00000864e458>] uart_set_termios+0xd0/0x17c
> [<ffff000008630ad4>] tty_set_termios+0x160/0x1e4
> [<ffff00000863165c>] set_termios+0x32c/0x3bc
> [<ffff000008631248>] tty_mode_ioctl+0x6f0/0x7d8
> [<ffff000008631a6c>] n_tty_ioctl_helper+0x10c/0x1e8
> [<ffff00000862d2c4>] n_tty_ioctl+0x120/0x194
> [<ffff00000862a724>] tty_ioctl+0x658/0xa34
> [<ffff0000082a8f40>] do_vfs_ioctl+0x554/0x810
> [<ffff0000082a9368>] SyS_ioctl+0x88/0x94
> Exception stack(0xffff00000ccf3ec0 to 0xffff00000ccf4000
> 
> Signed-off-by: chunguo.feng <chunguo.feng@semidrive.com>
> ---
>  drivers/tty/serial/8250/8250_dw.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/drivers/tty/serial/8250/8250_dw.c b/drivers/tty/serial/8250/8250_dw.c
> index c1d43f0..133c24e 100644
> --- a/drivers/tty/serial/8250/8250_dw.c
> +++ b/drivers/tty/serial/8250/8250_dw.c
> @@ -359,6 +359,12 @@ static void dw8250_set_termios(struct uart_port *p, struct ktermios *termios,
>  
>  	rate = clk_round_rate(d->clk, newrate);
>  	if (rate > 0 && p->uartclk != rate) {
> +		/*Need disable uart irq before disabled clk, because uart irq maybe triggered after

Space needed after '*', right?

> +		 * disabled clk immediately, then cause irq storm.
> +		 */
> +		if (p->irq)
> +			disabled_irq(p->irq);

This feels wrong, the irq should not be turned off here, what happens if
some data comes in while this function is running?  Will it be lost?

thanks,

greg k-h