Re: [PATCH v3 5/5] panic: add note that panic_print sysctl interface is deprecated

[Petr Mladek <pmladek@suse.com>]

On Wed 2025-08-13 09:05:30, Feng Tang wrote:
> On Tue, Aug 12, 2025 at 01:52:58PM +0200, Petr Mladek wrote:
> > On Thu 2025-07-03 10:10:04, Feng Tang wrote:
> > > Add a dedicated core parameter 'panic_console_replay' for controlling
> > > console replay, and add note that 'panic_print' sysctl interface  will
> > > be obsoleted by 'panic_sys_info' and 'panic_console_replay'.  When it
> > > happens, the SYS_INFO_PANIC_CONSOLE_REPLAY can be removed as well.
> > > 
> > > --- a/kernel/panic.c
> > > +++ b/kernel/panic.c
> > > @@ -77,6 +78,13 @@ ATOMIC_NOTIFIER_HEAD(panic_notifier_list);
> > >  EXPORT_SYMBOL(panic_notifier_list);
> > >  
> > >  #ifdef CONFIG_SYSCTL
> > > +static int sysctl_panic_print_handler(const struct ctl_table *table, int write,
> > > +			   void *buffer, size_t *lenp, loff_t *ppos)
> > > +{
> > > +	pr_info_once("Kernel: 'panic_print' sysctl interface will be obsoleted by both 'panic_sys_info' and 'panic_console_replay'\n");
> > > +	return proc_doulongvec_minmax(table, write, buffer, lenp, ppos);
> > > +}
> > 
> > This warning is printed "only" when the value is accessed via the
> > procfs. It would be great to print it also when it is set
> > via the command line parameter.
> 
> Yes, this is indeed a remaining issue to be solved, as mentioned in
> the cover letter.

I see now.

> > It would require replacing
> > 
> > core_param(panic_print, panic_print, ulong, 0644);
> > 
> > with
> > 
> > core_param_cb(panic_print, &panic_print_ops, &panic_print, 0644);
> 
> When testing the change, I found  a problem:  'core_param_cb' is not
> the real counterpart of 'core_param', that it is a module parameter
> instead of kernel/core parameter, and adds the module.prefix to the
> parameter, say, the effective cmdline parameter is changed to
> 'panic.panic_print=' instead of 'panic_print='.

I see. It is pity that it is messed like this.

> While below patch of adding a new 'kernel_param_cb' can work without
> the "panic" prefix, but I'm not sure if it is worth the change:

I think that it is worth adding. IMHO, the parameter will primary be used
from the command line. So, this is an important path how to make people
aware of the obsoleting.

> ---
> diff --git a/include/linux/moduleparam.h b/include/linux/moduleparam.h
> index bfb85fd13e1f..71053d078cea 100644
> --- a/include/linux/moduleparam.h
> +++ b/include/linux/moduleparam.h
> @@ -194,6 +194,9 @@ struct kparam_array
>  #define core_param_cb(name, ops, arg, perm)		\
>  	__level_param_cb(name, ops, arg, perm, 1)
>  
> +#define kernel_param_cb(name, ops, arg, perm) \
> +	__module_param_call("", name, ops, arg, perm, -1, 0)
> +

I would call it __core_param_cb(). And I move the definition
down to the section where core_param() and core_param_unsafe()
are defined. Also it would deserve a comment explaining
why the "__" prefix is used.

>  /**
>   * postcore_param_cb - general callback for a module/cmdline parameter
>   *                     to be evaluated before postcore initcall level

Best Regards,
Petr