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 <[email protected]> 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/[email protected]?part=4
