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

On 8/3/2026 3:29 PM, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
> 
> New issues:
> - [Low] Duplicate predefined reboot modes are not properly deduplicated against DT-defined modes, violating precedence rules and causing duplicate sysfs entries.
> - [Low] The patch silently changes how device tree arrays are parsed, skipping valid properties with more than 3 elements.
> 
> Pre-existing issues:
> - [High] Memory leak of `struct device` in `reboot_mode_unregister_device` due to a missing `put_device()`.
> - [High] Multiple instances of the same reboot-mode driver cause sysfs name collisions and incorrect device unregistration, leading to a Use-After-Free.
> --

[snip..]

>> +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: Low]
> Does this code append predefined modes to the list unconditionally without
> checking if a mode with the same name was already populated from the DT?
> 
> If a predefined mode has the same name as a mode from the Device Tree, both
> entries will be added to the list. When reboot_mode_create_device() iterates
> the list, it will create duplicate sysfs entries. Should we check for existing
> modes and deduplicate them to honor the DT precedence rules mentioned in the
> commit message?

This is documented for pre-defined modes.

> 
>> +	}
>> +
>> +	return 0;
>> +}
> [ ... ]
>> @@ -183,52 +284,36 @@ int reboot_mode_register(struct reboot_mode_driver *reboot)
>>   
>>   	INIT_LIST_HEAD(&reboot->head);
>>   
>> -	for_each_property_of_node(np, prop) {
>> -		memset(magic, 0, sizeof(magic));
> [ ... ]
>> +	if (np) {
>> +		for_each_property_of_node(np, prop) {
>> +			memset(magic, 0, sizeof(magic));
>> +			if (strncmp(prop->name, PREFIX, len))
>> +				continue;
>> +
>> +			count = device_property_count_u32(reboot->dev, prop->name);
>> +
>> +			if (count <= 0 || count > ARRAY_SIZE(magic) ||
>> +			    device_property_read_u32_array(reboot->dev, prop->name, magic, count)) {
> 
> [Severity: Low]
> Does this condition strictly reject properties with more than 3 elements
> instead of reading up to the maximum supported count?
> 
> If a DT configuration defines a reboot mode property with more than 3 items,
> it appears this would skip the valid properties entirely. Previously, would
> device_property_read_u32_array() have successfully read the first element
> and functioned correctly while ignoring extra elements?

A driver using reboot-mode framework should handle this case imo.

thanks,
Shivendra