Re: [PATCH v27 02/10] power: reset: reboot-mode: Add support for predefined reboot modes

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] reboot-mode: duplicate mode entries in reboot_mode_add_predefined_modes()
--

commit 06a52b916a06b4ea372bbbe59e432cb1a7e4b431
Author: Shivendra Pratap <shivendra.pratap@oss.qualcomm.com>

power: reset: reboot-mode: Add support for predefined reboot modes

This commit extends the reboot-mode framework to support driver-defined
predefined reboot modes and introduces reboot_mode_driver_init() to
initialize driver state.

> diff --git a/drivers/power/reset/reboot-mode.c b/drivers/power/reset/reboot-mode.c
> index 328fa37598df3..fd61de2263836 100644
> --- a/drivers/power/reset/reboot-mode.c
> +++ b/drivers/power/reset/reboot-mode.c

[ ... ]

> @@ -160,11 +234,38 @@ static int reboot_mode_create_device(struct reboot_mode_driver *reboot)
>  	return 0;
>  
>  error:
> -	reboot_mode_release_list(priv);
> +	reboot_mode_release_list(&priv->head);
>  	kfree(priv);
>  	return ret;
>  }
>  
> +static int reboot_mode_add_predefined_modes(struct reboot_mode_driver *reboot)
> +{
> +	const struct reboot_mode_entry *modes = reboot->predefined_modes;
> +	struct mode_info *info;
> +	size_t i;
> +
> +	if (!modes)
> +		return 0;
> +
> +	for (i = 0; i < reboot->predefined_mode_count; i++) {
> +		if (modes[i].name && strpbrk(modes[i].name, "\n ,/"))
> +			return -EINVAL;
> +
> +		if (!modes[i].count || modes[i].count > ARRAY_SIZE(modes[i].magic))
> +			return -EINVAL;
> +
> +		info = reboot_mode_create_info(reboot->dev, modes[i].name,
> +					       modes[i].magic, modes[i].count);
> +		if (IS_ERR(info))
> +			return PTR_ERR(info);
> +
> +		list_add_tail(&info->list, &reboot->head);
> +	}
> +
> +	return 0;
> +}

[Severity: Medium]
In include/linux/reboot-mode.h, the comment on struct reboot_mode_driver
states that if a predefined mode name duplicates a DT-defined mode name, the
DT-defined mode takes precedence:

    struct reboot_mode_driver {
        ...
        /*
         * Optional predefined reboot-mode table.
         *
         * Must be initialized before reboot_mode_register() and remain valid
         * until reboot_mode_unregister().
         *
         * If a predefined mode name duplicates a DT-defined mode name,
         * the DT-defined mode takes precedence.
         */
        const struct reboot_mode_entry *predefined_modes;

However, reboot_mode_add_predefined_modes() unconditionally allocates and
appends every entry in reboot->predefined_modes to &reboot->head without
checking whether a mode with the same name was already added from Device Tree
(or earlier in predefined_modes).

While get_reboot_mode_info() returns the first matching entry in
&reboot->head, reboot_mode_create_device() copies every entry from
&reboot->head into &priv->head, and reboot_modes_show() emits all of them:

    reboot_mode_register()
      reboot_mode_add_predefined_modes()
        list_add_tail(&info->list, &reboot->head);
      reboot_mode_create_device()
        list_for_each_entry(info, &reboot->head, list) {
            ...
            list_add_tail(&sysfs_info->list, &priv->head);
        }

    reboot_modes_show()
      list_for_each_entry(sysfs_info, &priv->head, list)
          size += sysfs_emit_at(buf, size, "%s ", sysfs_info->mode);

When a predefined mode name is overridden in Device Tree, won't this allocate
a redundant struct mode_info and cause /sys/class/reboot-mode/<driver>/reboot_modes
to output duplicate mode names to userspace? Should
reboot_mode_add_predefined_modes() skip entries whose name already exists in
&reboot->head?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261029-arm-psci-system_reset2-vendor-reboots-v27-0-fa4b3ff1359b@oss.qualcomm.com?part=2