linux-kernel - [PATCH] x86/apic: Better document spurious_interrupt() and __spurious

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <aCWivMggS9mektCu@gmail.com>
Date: Thu, 15 May 2025 10:15:56 +0200
From: Ingo Molnar <mingo@...nel.org>
To: Thomas Gleixner <tglx@...utronix.de>
Cc: Shivank Garg <shivankg@....com>, mingo@...hat.com, bp@...en8.de,
	dave.hansen@...ux.intel.com, x86@...nel.org, hpa@...or.com,
	luto@...nel.org, peterz@...radead.org, rafael@...nel.org,
	pavel@...nel.org, akpm@...ux-foundation.org,
	linux-kernel@...r.kernel.org, linux-pm@...r.kernel.org,
	sohil.mehta@...el.com, rui.zhang@...el.com, yuntao.wang@...ux.dev,
	kai.huang@...el.com, xiaoyao.li@...el.com, peterx@...hat.com,
	sandipan.das@....com, ak@...ux.intel.com, rostedt@...dmis.org
Subject: [PATCH] x86/apic: Better document spurious_interrupt() and
 __spurious_interrupt()


* Thomas Gleixner <tglx@...utronix.de> wrote:

> On Thu, May 15 2025 at 12:03, Shivank Garg wrote:
> > On 5/14/2025 1:26 PM, Ingo Molnar wrote:
> >> This is incorrect and is based on a misunderstanding of what the code 
> >> does:
> >> 
> >> DEFINE_IDTENTRY_IRQ(spurious_interrupt)
> >> {
> >>         handle_spurious_interrupt(vector);
> >> }
> >
> > The kernel-doc tool doesn't handle macros properly.
> > Can I change it to a normal comment instead?
> > or if a kernel-doc comment is required how should I make it correct?
> 
> Fix the stupid tool and leave the comment alone.

Yeah, so the problem is that the kernel-doc tool is partially right to 
complain about the status quo:

	/**
	 * spurious_interrupt - Catch all for interrupts raised on unused vectors
	 * @regs:       Pointer to pt_regs on stack
	 * @vector:     The vector number
	 *
	 * This is invoked from ASM entry code to catch all interrupts which
	 * trigger on an entry which is routed to the common_spurious idtentry
	 * point.
	 */
	DEFINE_IDTENTRY_IRQ(spurious_interrupt)
	{
	        handle_spurious_interrupt(vector);
	}

This description is incorrect as-is: the parameters described are not 
that of the main 'spurious_interrupt()' handler, which is:

        extern __visible noinstr void spurious_interrupt(struct pt_regs *regs, unsigned long error_code);

Note that it has an 'error_code', not 'vector'. (Which, of course, are 
the same actual numeric value in this case, but are in different 
functions and different prototypes.)

But the description is that of the __spurious_interrupt() lower level 
(sub-)handler function:

	static void __spurious_interrupt(struct pt_regs *regs, u32 vector);

So yeah, this documention is arguably a bit messy, and not just because 
kernel-doc is confused about macros.

So I'd fix it like this:

	/*
	 * spurious_interrupt(): Catch all for interrupts raised on unused vectors
	 * @regs:	Pointer to pt_regs on stack
	 * @error_code: Hardware exception/interrupt data
	 *
	 * The spurious_interrupt() high level function is invoked from ASM entry code
	 * to catch all interrupts which trigger on an entry which is routed to the
	 * common_spurious idtentry point.
	 *
	 * __spurious_interrupt(): Catch all for interrupts raised on unused vectors
	 * @regs:	Pointer to pt_regs on stack
	 * @vector:	The IRQ vector number
	 *
	 * This is the lower level spurious interrupts handler function.
	 */
	DEFINE_IDTENTRY_IRQ(spurious_interrupt)
	{
		handle_spurious_interrupt(vector);
	}
	
... or so.

Which also moves it out of kernel-doc style, and should thus avoid 
kernel-doc's confusion. Patch below.

Or we could:

  s/spurious_interrupt
   /__spurious_interrupt

and remove the kernel-doc trigger line.

Whichever your preference is.

Thanks,

	Ingo

=============>
 arch/x86/kernel/apic/apic.c | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/arch/x86/kernel/apic/apic.c b/arch/x86/kernel/apic/apic.c
index d73ba5a7b623..462dcdb3af85 100644
--- a/arch/x86/kernel/apic/apic.c
+++ b/arch/x86/kernel/apic/apic.c
@@ -2128,14 +2128,20 @@ static noinline void handle_spurious_interrupt(u8 vector)
 	trace_spurious_apic_exit(vector);
 }
 
-/**
- * spurious_interrupt - Catch all for interrupts raised on unused vectors
+/*
+ * spurious_interrupt(): Catch all for interrupts raised on unused vectors
+ * @regs:	Pointer to pt_regs on stack
+ * @error_code: Hardware exception/interrupt data
+ *
+ * The spurious_interrupt() high level function is invoked from ASM entry code
+ * to catch all interrupts which trigger on an entry which is routed to the
+ * common_spurious idtentry point.
+ *
+ * __spurious_interrupt(): Catch all for interrupts raised on unused vectors
  * @regs:	Pointer to pt_regs on stack
  * @vector:	The IRQ vector number
  *
- * This is invoked from ASM entry code to catch all interrupts which
- * trigger on an entry which is routed to the common_spurious idtentry
- * point.
+ * This is the lower level spurious interrupts handler function.
  */
 DEFINE_IDTENTRY_IRQ(spurious_interrupt)
 {