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

[Shivendra Pratap <shivendra.pratap@oss.qualcomm.com>]

On 9/28/2026 11:56 AM, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [Medium] power: reset: reboot-mode: duplicate sysfs entries for predefined modes
> --
> 
> commit 613bc6d4edd9d0928986367eb5de6c3fc84d4d4e
> Author: Shivendra Pratap <shivendra.pratap@oss.qualcomm.com>
> 
> power: reset: reboot-mode: Add support for predefined reboot modes
>      
> reboot-mode based drivers can define a reboot-mode by adding it under
> the reboot-mode node in device tree. This limits such drivers, to define
> any predefined reboot-modes statically within the driver and creates a
> dependency on device-tree.
> 
>> 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);
> 
> [Severity: Medium]
> If a predefined mode shares a name with a device tree mode, does this
> unconditionally add it to reboot->head, resulting in a duplicate entry?
> 
> Since reboot_mode_create_device() directly iterates over reboot->head and
> copies these modes into priv->head:
> 
> reboot_mode_create_device() {
>      ...
>      list_for_each_entry(info, &reboot->head, list) {
>          ...
>          sysfs_info->mode = kstrdup_const(info->mode, GFP_KERNEL);
>          ...
>          list_add_tail(&sysfs_info->list, &priv->head);
>      }
> }
> 
> Will this cause the reboot_modes sysfs attribute to emit duplicate string
> values (e.g., 'normal normal') to userspace?

This is documented in header while adding pre-defined modes.
Should a developer/SOC vendor take care of this instead of making this 
code complex?
Will wait for more reviews on this.

thanks,
Shivendra