Re: [RFC PATCH v2 04/15] misc/arm-cla: Probe firmware-described devices

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] core: mixed goto-based error handling and scoped_guard in cla_module_init()
--

commit 0ee32d948034264a0bab6422d831ac58dbde58cd
Author: Jean-Philippe Brucker <jpb@kernel.org>

misc/arm-cla: Probe firmware-described devices

This commit discovers and instantiates CLA devices described by firmware 
using platform_driver_probe. It also adds topology grouping for domains and 
registers CPU hotplug callbacks to manage hardware setup per CLA-local CPU.

> diff --git a/drivers/misc/arm-cla/cla-init.c b/drivers/misc/arm-cla/cla-init.c
> new file mode 100644
> --- /dev/null
> +++ b/drivers/misc/arm-cla/cla-init.c
[ ... ]
> +static int __init cla_module_init(void)
> +{
> +	bool driver_registered = false;
> +	int ret;
> +
> +	/*
> +	 * For now, we reject any CLAs discovered via FW tables, whose CPU is
> +	 * offline during init. This keeps things simple by allowing us to probe
> +	 * all HW at init time. Keep the online CPU set stable so that the
> +	 * cpu_online() check in cla_probe() is consistent with the online cpus
> +	 * when we call cpuhp_setup_state().
> +	 */
> +	scoped_guard(cpus_read_lock) {
> +		/*
> +		 * CPUs may be hotplugged, but all CLAs are described by
> +		 * firmware so the probe can be synchronous. This only sets up
> +		 * the resources, and CPUHP callbacks do the actual peeking and
> +		 * poking. This returns an error when no CLA has an online CPU.
> +		 */
> +		ret = platform_driver_probe(&cla_driver, cla_probe);
> +		if (ret) {
> +			if (ret != -ENODEV)
> +				pr_err("arm-cla: probe failed with %d\n", ret);
> +			/* Some domains may have been created during probe. */
> +			goto err_hardware;

[Severity: Medium]
The function cla_module_init() uses a scoped_guard(cpus_read_lock) block while
also jumping to a manual err_hardware cleanup label.

According to the cleanup and guard subsystem guidelines in
include/linux/cleanup.h, mixing goto-based error handling with scope-based
cleanups in the same function is considered an anti-pattern. This creates
confusing ownership semantics and risks double-frees or resource leaks during
future refactoring. 

Could this function be refactored to consistently use either only scope-based
cleanup or only goto-based error handling?

> +		}
[ ... ]
> +
> +err_hardware:
> +	if (cla_cpuhp_state >= 0)
> +		cpuhp_remove_state(cla_cpuhp_state);
> +	cla_domains_free();
> +	if (driver_registered)
> +		platform_driver_unregister(&cla_driver);
> +	return ret;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261015142552.4128028-1-ryan.roberts@arm.com?part=4