Re: [PATCH 3/5] power: reset: syscon-reboot-mode: enable supplies for the next stage

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] pm: memory leak of regulator_bulk_data array in probe error path and teardown
--

commit e442fa70c7e3dbcc8c8ea6a28287f1aa26600fe2
Author: Alexey Charkov <alchark@flipper.net>

power: reset: syscon-reboot-mode: enable supplies for the next stage

This commit requests the boot mode to explicitly enable regulators used
by the reboot mode sequence, preventing early boot code from hanging if
the running system left them powered down. The supplies are deliberately
left enabled to be available for the next stage.

> diff --git a/drivers/power/reset/syscon-reboot-mode.c b/drivers/power/reset/syscon-reboot-mode.c
> index e0772c9f70f7a..3072922f1716a 100644
> --- a/drivers/power/reset/syscon-reboot-mode.c
> +++ b/drivers/power/reset/syscon-reboot-mode.c

[ ... ]

> @@ -28,6 +31,20 @@ static int syscon_reboot_mode_write(struct reboot_mode_driver *reboot,
>  
>  	syscon_rbm = container_of(reboot, struct syscon_reboot_mode, reboot);
>  
> +	/*
> +	 * Whatever acts on the mode (e.g. boot ROM) runs before the operating
> +	 * system, and may need supplies that the running system had powered
> +	 * down. Enable them here, and deliberately leave them enabled: the
> +	 * system is on its way down, and what runs next may not know how to
> +	 * turn them on.
> +	 */
> +	if (syscon_rbm->num_supplies) {
> +		ret = regulator_bulk_enable(syscon_rbm->num_supplies,
> +					    syscon_rbm->supplies);

[Severity: High]
This isn't a bug introduced by this patch, but does calling
regulator_bulk_enable() here risk an uninitialized pointer dereference if it
hits an error?

of_regulator_bulk_get_all() dynamically allocates the array with kmalloc_objs()
without zeroing it, leaving the .supply field of the returned structures
uninitialized.

If regulator_bulk_enable() fails, its error path attempts to log the failure
using the uninitialized .supply field:

    pr_err("Failed to enable %s...", consumers[i].supply)

Passing this uninitialized pointer to the %s format specifier would cause an
out-of-bounds memory read and a kernel oops.

> +		if (ret < 0)
> +			dev_err(reboot->dev, "enabling reboot mode supplies failed\n");
> +	}
> +
>  	ret = regmap_update_bits(syscon_rbm->map, syscon_rbm->offset,
>  				 syscon_rbm->mask, magic);
>  	if (ret < 0)
> @@ -36,6 +53,13 @@ static int syscon_reboot_mode_write(struct reboot_mode_driver *reboot,
>  	return ret;
>  }
>  
> +static void syscon_reboot_mode_put_supplies(void *data)
> +{
> +	struct syscon_reboot_mode *syscon_rbm = data;
> +
> +	regulator_bulk_free(syscon_rbm->num_supplies, syscon_rbm->supplies);

[Severity: Medium]
Does this code leak the dynamically allocated syscon_rbm->supplies array?

The array is dynamically allocated via of_regulator_bulk_get_all() during
probe. While regulator_bulk_free() releases the individual regulator
references, it does not free the array memory itself. Does this cleanup
action need an explicit kfree(syscon_rbm->supplies) to avoid a leak on device
unbind or probe failure?

> +}
> +
>  static int syscon_reboot_mode_probe(struct platform_device *pdev)
>  {
>  	int ret;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260928-b4-rk3576-reboot-mode-v1-0-65486b03bd41@flipper.net?part=3