Re: [PATCH v2 3/9] printk: Separate code for adding/updating preferred console metadata

[John Ogness]

On 2026-04-23, Petr Mladek <pmladek@xxxxxxxx> wrote:
> diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
> index 13c98285892b..d251bf8e104f 100644
> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> @@ -2544,18 +2544,119 @@ asmlinkage __visible void early_printk(const char *fmt, ...)
>  }
>  #endif
>  
> -static void set_user_specified(struct preferred_console *pc, bool user_specified)
> +/** update_preferred_console - Update a given entry in the preferred_consoles[]
> + *	table.

Perhaps the description should be:

Update or add a given entry in the preferred_consoles[] table.

If the caller didn't match an existing preferred_console, this function
is called to add a new entry. This is obvious if you are looking at the
caller, but it is not obvious at all if you are only looking at this
function, i.e. strscpy'ing in @name or @devname is the only clue that
you are dealing with a new entry.

> + * @i: index of the entry in @preferred_consoles table which should get updated.
> + * @name: The name of the preferred console driver.
> + * @idx: Preferred console index, e.g. port number.
> + * @devname: The name of the preferred physical device.
> + * @options: Options used when setting up the console driver.
> + * @brl_options: Options used when setting up the console driver
> + *	as a braille console.
> + * @user_specified: True if preferred via the kernel command line.
> + *
> + * The function ensures that the given values are consistent. Also
> + * it updates some global variables which are used to make the right
> + * decisions in register_console().
> + *
> + * Rules:
> + *
> + *   1. Either @name and valid @idx OR @devname and @idx=-1 are allowed.
> + *	Note that a valid @name and @idx will get assigned later when
> + *	@devname matches during the device initialization.
> + *   2. Specify @brl_options if the console should be enabled as
> + *	a Braille console [*]
> + *   3. Only matching entries can be updated.
> + *   4. @options passed via the command line are used when the same
> + *	console is preferred also by some platform-specific code.
> + *
> + * [*] Braille console is using the mechanism for registering consoles
> + *     but it is very special. It is primarily used for user interaction
> + *     with the system. It neither gets printk() messages nor is associated
> + *     with /dev/console.
> + */
> +static int update_preferred_console(unsigned int i,
> +				    const char *name, const short idx,
> +				    const char *devname, char *options,
> +				    char *brl_options, bool user_specified)
>  {
> -	if (!user_specified)
> -		return;
> +	struct preferred_console *pc;
> +
> +	if (i >= MAX_PREFERRED_CONSOLES)
> +		return -E2BIG;
> +
> +	pc = &preferred_consoles[i];
> +
> +	if (!name && !devname)
> +		return -EINVAL;

You are adding detailed error messages for all possible wrong calls
except for this on. Maybe this should be covered as well with:

	if (WARN_ON(!name && !devname))
		return -EINVAL;

> +	if (devname) {
> +		/*
> +		 * A valid console name and index will get assigned when
> +		 * a matching device gets registered.
> +		 */
> +		if (name) {
> +			pr_err("Adding a preferred console devname with a hard-coded console name: %s, %s\n",
> +			       devname, name);
> +			return -EINVAL;
> +		}
> +		if (pc->name[0]) {
> +			pr_err("Updating a preferred console entry with an already assigned console name via devname: %s, %s\n",
> +			       devname, pc->name);
> +			return -EINVAL;
> +		}
> +		if (idx != -1) {
> +			pr_err("Adding a preferred console devname with a hard-coded index: %s, %d\n",
> +			       devname, idx);
> +			return -EINVAL;
> +		}
> +
> +		if (!pc->devname[0]) {
> +			strscpy(pc->devname, devname);
> +			pc->index = idx;

This block is about setting the devname. Setting the index here as well
is copied code and well hidden. I would keep the index setting outside
the devname and name blocks (as it was previously).

> +		} else if (strcmp(pc->devname, devname) != 0) {
> +			pr_err("Updating a preferred console with an invalid devname: %s vs. %s\n",
> +			       pc->devname, devname);
> +			return -EINVAL;
> +		}
> +	}
> +
> +	if (name) {
> +		/* A console name must be defined with a valid index. */
> +		if (idx < 0) {
> +			pr_err("Adding a preferred console with an invalid index: %s, %d\n",
> +			       name, idx);
> +			return -EINVAL;
> +		}
> +
> +		if (!pc->name[0]) {
> +			strscpy(pc->name, name);
> +			pc->index = idx;

Here is the copied code. This block is about setting the name.

> +		} else if (strcmp(pc->name, name) != 0 || pc->index != idx) {
> +			pr_err("Updating a preferred console with an invalid name or index: %s%d vs. %s%d\n",
> +			       pc->name, pc->index, name, idx);
> +			return -EINVAL;
> +		}
> +	}

I would put the index setting here as it is relevant regardless of how
the devname or name was updated.

	pc->index = idx;

With or without implementing my suggestions:

Reviewed-by: John Ogness <john.ogness@xxxxxxxxxxxxx>