Re: [PATCH 3/3] sparc: Implement arch_do_panic

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] The newly introduced SPARC implementation of `arch_do_panic()` is never called because the overriding macro is not visible in `kernel/panic.c`, resulting in the empty fallback stub being used and the SPARC implementation becoming dead code.
- [Low] Redundant local `extern` declaration of `stop_a_enabled`.
--

--- Patch [3]: [PATCH 3/3] sparc: Implement arch_do_panic ---
commit 0468bb7524180e5a2aa478f794a79dc389197120
Author: Mete Durlu <meted@linux.ibm.com>

sparc: Implement arch_do_panic

This patch implements a sparc-specific arch_do_panic function to handle
the Stop-A hardware sequence. It removes the architecture-specific ifdef
blocks from the generic panic code.

> diff --git a/arch/sparc/include/asm/setup.h b/arch/sparc/include/asm/setup.h
> index 21bed55140288..557e8277a6407 100644
> --- a/arch/sparc/include/asm/setup.h
> +++ b/arch/sparc/include/asm/setup.h

[ ... ]

> @@ -67,4 +66,7 @@ void sun_do_break(void);
>  extern int stop_a_enabled;
>  extern int scons_pwroff;
>  
> +void arch_do_panic(void);
> +#define arch_do_panic arch_do_panic
> +
>  #endif /* _SPARC_SETUP_H */

> diff --git a/arch/sparc/kernel/setup.c b/arch/sparc/kernel/setup.c
> index 4975867d9001b..87d3569c520f8 100644
> --- a/arch/sparc/kernel/setup.c
> +++ b/arch/sparc/kernel/setup.c

[ ... ]

> @@ -36,6 +36,14 @@ static const struct ctl_table sparc_sysctl_table[] = {
>  #endif
>  };
>  
> +void arch_do_panic(void)
> +{
> +	extern int stop_a_enabled;

[Severity: Low]
This isn't a bug, but is this local extern declaration necessary?
Since asm/setup.h is included at the top of this file, the global
declaration of stop_a_enabled should already be visible.

> +	/* Make sure the user can actually press Stop-A (L1-A) */
> +	stop_a_enabled = 1;
> +	pr_emerg("Press Stop-A (L1-A) from sun keyboard or send break\n"
> +		 "twice on console to return to the boot prom\n");
> +}

> diff --git a/kernel/panic.c b/kernel/panic.c
> index de0bda946cabb..593aa2c24d93f 100644
> --- a/kernel/panic.c
> +++ b/kernel/panic.c

[ ... ]

> @@ -747,15 +747,6 @@ void vpanic(const char *fmt, va_list args)
>  			reboot_mode = panic_reboot_mode;
>  		emergency_restart();
>  	}
> -#ifdef __sparc__
> -	{
> -		extern int stop_a_enabled;
> -		/* Make sure the user can actually press Stop-A (L1-A) */
> -		stop_a_enabled = 1;
> -		pr_emerg("Press Stop-A (L1-A) from sun keyboard or send break\n"
> -			 "twice on console to return to the boot prom\n");
> -	}
> -#endif
>  	pr_emerg("---[ end Kernel panic - not syncing: %s ]---\n", buf);
>  
>  	arch_do_panic();

[Severity: High]
Will this actually call the sparc-specific implementation?

Looking at kernel/panic.c, it does not appear to include asm/setup.h where
the override macro for arch_do_panic is defined. Because the override macro
isn't visible in this translation unit, won't vpanic end up executing
the default empty inline fallback instead of the newly added sparc
implementation, effectively dropping the sparc panic behavior?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260727-arch_do_panic-v1-0-ca1777547c38@linux.ibm.com?part=3