diff mbox series

x86/apic: Better document spurious_interrupt() and __spurious_interrupt()

Message ID aCWivMggS9mektCu@gmail.com
State New
Headers show
Series x86/apic: Better document spurious_interrupt() and __spurious_interrupt() | expand

Commit Message

Ingo Molnar May 15, 2025, 8:15 a.m. UTC 
* Thomas Gleixner <tglx@linutronix.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 mbox series

Patch

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)
 {