Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
New issues: - [Low] Mixing `goto`-based error handling with scope-based `guard()` cleanup in `timerlat_fd_open()`. Pre-existing issues: - [High] Uninitialized per-CPU mutex and CPU ID for hotplugged CPUs in `init_osnoise_tracer()`. -- --- Patch [4]: [PATCH 4/4] tracing/osnoise: Take trace_types_lock in timerlat_fd_open --- commit d0fbb203fad6d30d667803da9a34c6adb7c10f72 Author: Crystal Wood <[email protected]> tracing/osnoise: Take trace_types_lock in timerlat_fd_open This commit replaces the use of interface_lock with trace_types_lock using a guard(mutex) in timerlat_fd_open. It prevents a user fd from being opened when osnoise_options_write has already stopped threads but not yet set OSN_WORKLOAD. > diff --git a/kernel/trace/trace_osnoise.c b/kernel/trace/trace_osnoise.c > index 0f27af6ade3e1..3d12ce8a591e7 100644 > --- a/kernel/trace/trace_osnoise.c > +++ b/kernel/trace/trace_osnoise.c > @@ -2466,16 +2466,14 @@ static int timerlat_fd_open(struct inode *inode, > struct file *file) > long cpu = (long) inode->i_cdev; > int ret = 0; > > - mutex_lock(&interface_lock); > + guard(mutex)(&trace_types_lock); [Severity: Low] Does this mix goto-based error handling with scope-based guard cleanup? The function still relies on a goto err; block for error handling (which unlocks osn_var->lock and calls migrate_enable()), but it now uses guard for trace_types_lock. The cleanup guidelines suggest converting all resources to scope-based cleanup, or none of them, to avoid mixing semantics and ownership rules in the same routine. [ ... ] [Severity: High] This is a pre-existing issue, but does init_osnoise_tracer fail to initialize per-CPU data for hotplugged CPUs? Looking at init_osnoise_tracer in kernel/trace/trace_osnoise.c: kernel/trace/trace_osnoise.c:init_osnoise_tracer() { ... for_each_online_cpu(cpu) { struct osnoise_variables *osn = cpu_osn_var(cpu); mutex_init(&osn->lock); osn->cpu = cpu; } ... } Because this iterates over online CPUs rather than possible CPUs, if a CPU is offline during boot or hot-added later, its mutex and cpu ID will remain uninitialized. When that CPU comes online, osnoise_hotplug_workfn calls start_kthread(cpu), which attempts to lock the uninitialized osn->lock. Could this cause a lockdep warning or kernel crash due to uninitialized list_head pointers? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=4
