Re: [PATCH v22 07/13] power: reset: Add psci-reboot-mode driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] Leaked predefined reboot modes if devm_reboot_mode_register() fails.
- [Low] Missing explicit header inclusions for macros.
- [Low] Missing MODULE_DESCRIPTION macro.
--

commit 4f9479b4f7059dfd06d1f3ce3dd92984cbf01921
Author: Shivendra Pratap <shivendra.pratap@oss.qualcomm.com>

power: reset: Add psci-reboot-mode driver

This commit adds a psci-reboot-mode driver to handle predefined and
vendor-specific PSCI resets via the reboot-mode framework. It allows
userspace to trigger supported PSCI resets using standard reboot system
calls.

[ ... ]

> diff --git a/drivers/power/reset/psci-reboot-mode.c b/drivers/power/reset/psci-reboot-mode.c
> new file mode 100644
> index 0000000000000..845b2a1816ce5
> --- /dev/null
> +++ b/drivers/power/reset/psci-reboot-mode.c
> @@ -0,0 +1,72 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (c) Qualcomm Technologies, Inc. and/or its subsidiaries.
> + */
> +
> +#include <linux/device.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/psci.h>
> +#include <linux/reboot-mode.h>
> +#include <linux/types.h>

This isn't a bug, but does this file need to explicitly include linux/bits.h
for the BIT_ULL macro, and linux/array_size.h for ARRAY_SIZE? Relying on
indirect inclusion can sometimes lead to sudden build failures if upstream
headers change.

[ ... ]

> +static int psci_reboot_mode_probe(struct platform_device *pdev)
> +{
> +	struct reboot_mode_driver *reboot;
> +	size_t count;
> +	int ret;
> +
> +	reboot = devm_kzalloc(&pdev->dev, sizeof(*reboot), GFP_KERNEL);
> +	if (!reboot)
> +		return -ENOMEM;
> +
> +	reboot_mode_driver_init(reboot, &pdev->dev, psci_reboot_mode_write);
> +
> +	/* Skip PSCI SYSTEM_RESET2 modes if unsupported */
> +	count = psci_has_system_reset2_support() ? ARRAY_SIZE(psci_resets) : 1;
> +	ret = reboot_mode_add_predefined_modes(reboot, psci_resets, count);
> +	if (ret)
> +		return ret;
> +
> +	return devm_reboot_mode_register(&pdev->dev, reboot);

Does this leak the dynamically allocated predefined modes if
devm_reboot_mode_register() fails?

Looking at reboot_mode_add_predefined_modes(), it allocates mode_info structs
and adds them to reboot->predefined_modes. If devm_reboot_mode_register()
fails (for example, if its internal devres_alloc() returns -ENOMEM), it simply
returns the error without calling reboot_mode_register().

While devres will clean up the reboot object itself, is there any mechanism
that frees the mode_info items added to the predefined_modes list on this
error path?

> +}

[ ... ]

> +module_platform_driver(psci_reboot_mode_driver);
> +
> +MODULE_LICENSE("GPL");

This isn't a bug, but should there be a MODULE_DESCRIPTION macro here to avoid
warnings or build failures with recent compiler checks?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260514-arm-psci-system_reset2-vendor-reboots-v22-0-28a5bde07483@oss.qualcomm.com?part=7