Re: [PATCH 3/7] kgdb: Add request_nmi() to the io ops table for kgdboc

From: Daniel Thompson <daniel.thompson@linaro.org>
To: Sumit Garg <sumit.garg@linaro.org>
Cc: kgdb-bugreport@lists.sourceforge.net,
	linux-serial@vger.kernel.org,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	Jason Wessel <jason.wessel@windriver.com>,
	Douglas Anderson <dianders@chromium.org>,
	Jiri Slaby <jslaby@suse.com>,
	Russell King - ARM Linux admin <linux@armlinux.org.uk>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH 3/7] kgdb: Add request_nmi() to the io ops table for kgdboc
Date: Tue, 23 Jun 2020 11:59:34 +0100	[thread overview]
Message-ID: <20200623105934.wvyidi3xgqgd35af@holly.lan> (raw)
In-Reply-To: <CAFA6WYOmQT-OQvjpy1pVPq2mx5S264bJPd-XfwnDY2BjeoWekg@mail.gmail.com>

On Tue, Jun 23, 2020 at 02:07:47PM +0530, Sumit Garg wrote:
> On Mon, 22 Jun 2020 at 21:33, Daniel Thompson
> <daniel.thompson@linaro.org> wrote:
> > > +     irq_set_status_flags(irq, IRQ_NOAUTOEN);
> > > +     res = request_nmi(irq, fn, IRQF_PERCPU, "kgdboc", dev_id);
> >
> > Why do we need IRQF_PERCPU here. A UART interrupt is not normally
> > per-cpu?
> >
> 
> Have a look at this comment [1] and corresponding check in
> request_nmi(). So essentially yes UART interrupt is not normally
> per-cpu but in order to make it an NMI, we need to request it in
> per-cpu mode.
> 
> [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/kernel/irq/manage.c#n2112

Thanks! This is clear.

> > > +     if (res) {
> > > +             res = request_irq(irq, fn, IRQF_SHARED, "kgdboc", dev_id);
> >
> > IRQF_SHARED?
> >
> > Currrently there is nothing that prevents concurrent activation of
> > ttyNMI0 and the underlying serial driver. Using IRQF_SHARED means it
> > becomes possible for both drivers to try to service the same interrupt.
> > That risks some rather "interesting" problems.
> >
> 
> Could you elaborate more on "interesting" problems?

Er... one of the serial drivers we have allowed the userspace to open
will, at best, be stone dead and not passing any characters.

> BTW, I noticed one more problem with this patch that is IRQF_SHARED
> doesn't go well with IRQ_NOAUTOEN status flag. Earlier I tested it
> with auto enable set.
> 
> But if we agree that both shouldn't be active at the same time due to
> some real problems(?) then I can rid of IRQF_SHARED as well. Also, I
> think we should unregister underlying tty driver (eg. /dev/ttyAMA0) as
> well as otherwise it would provide a broken interface to user-space.

I don't have a particular strong opinion on whether IRQF_SHARED is
correct or not correct since I think that misses the point.

Firstly, using IRQF_SHARED shows us that there is no interlocking
between kgdb_nmi and the underlying serial driver. That probably tells
us about the importance of the interlock than about IRQF_SHARED.

To some extent I'm also unsure that kgdb_nmi could ever actually know
the correct flags to use in all cases (that was another reason for the
TODO comment about poll_get_irq() being a bogus API).

Daniel.