Re: [PATCH] tty: vt: make do_con_write() no-op if IRQ is disabled

From: Jiri Slaby <jirislaby@kernel.org>
To: Linus Torvalds <torvalds@linux-foundation.org>,
	Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp>
Cc: "Fabio M. De Francesco" <fmdefrancesco@gmail.com>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	Peter Hurley <peter@hurleysoftware.com>,
	LKML <linux-kernel@vger.kernel.org>,
	Andrew Morton <akpm@linux-foundation.org>
Subject: Re: [PATCH] tty: vt: make do_con_write() no-op if IRQ is disabled
Date: Fri, 3 Dec 2021 06:03:49 +0100	[thread overview]
Message-ID: <5ac5c18d-d99c-460c-bfff-6674b3651549@kernel.org> (raw)
In-Reply-To: <CAHk-=wiXNJ86W=gwAHH1qd+cE9dmfk_dEKFmNa89XH19NhPNkg@mail.gmail.com>

On 02. 12. 21, 19:35, Linus Torvalds wrote:
> On Thu, Dec 2, 2021 at 7:41 AM Tetsuo Handa
> <penguin-kernel@i-love.sakura.ne.jp> wrote:
>>
>>> Looking at the backtrace, I see
>>>
>>>     n_hdlc_send_frames+0x24b/0x490 drivers/tty/n_hdlc.c:290
>>>     tty_wakeup+0xe1/0x120 drivers/tty/tty_io.c:534
>>>     __start_tty drivers/tty/tty_io.c:806 [inline]
>>>     __start_tty+0xfb/0x130 drivers/tty/tty_io.c:799
>>>
>>> and apparently it's that hdlc line discipline (and
>>> n_hdlc_send_frames() in particular) that is the problem here.
>>>
>>> I think that's where the fix should be.
>>
>> Do you mean that we should change the behavior of n_hdlc_send_frames()
>> rather than trying to make __start_tty() schedulable again?
> 
> I wouldn't change n_hdlc_send_frames() itself. It does what it says it does.
> 
> But n_hdlc_tty_wakeup() probably shouldn't call it directly. Other tty
> line disciplines don't do that kind of thing - although I only looked
> at a couple. They all seem to just set bits and prepare things. Like a
> wakeup function should do.
> 
> So I think n_hdlc_tty_wakeup() should perhaps only do a
> "schedule_work()" or similar to get that n_hdlc_send_frames() started,
> rather than doing it itself.

Concurred, this is even documented:
write_wakeup
     [DRV] void ()(struct tty_struct *tty)
This function is called by the low-level tty driver to signal that line 
discpline should try to send more characters to the low-level driver for 
transmission. If the line discpline does not have any more data to send, 
it can just return. If the line discipline does have some data to send, 
please arise a tasklet or workqueue to do the real data transfer. Do not 
send data in this hook, it may lead to a deadlock.

thanks,
-- 
js
suse labs