Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [Medium] rv: stale parent container state on re-enable error in rv_update_single() --
commit 2db3de084bc5c2af773f028e5905ec5c314ba488 Author: Wei Li <[email protected]> rv: Show the mixed enable state of containers This commit converts struct rv_monitor::enabled into enum rv_monitor_state so container monitors can report a mixed ("X") enable state when only a subset of their nested monitors are enabled. It updates rv_enable_single() and rv_disable_single() to refresh the parent container state. > diff --git a/kernel/trace/rv/rv.c b/kernel/trace/rv/rv.c > index 4e689d4590aee..7846e4092b9c4 100644 > --- a/kernel/trace/rv/rv.c > +++ b/kernel/trace/rv/rv.c [ ... ] > @@ -269,10 +272,22 @@ static int __rv_disable_monitor(struct rv_monitor *mon, > bool sync) > > static void rv_disable_single(struct rv_monitor *mon) > { > + struct rv_monitor *cntr = mon->parent; > + struct rv_monitor *p = cntr; > + > __rv_disable_monitor(mon, true); > + if (!cntr) > + return; > + cntr->enabled = 0; > + list_for_each_entry_continue(p, &rv_monitors_list, list) { > + if (p->parent != cntr) > + break; > + if (p->enabled) > + cntr->enabled = 2; > + } > } > > -static int rv_enable_single(struct rv_monitor *mon) > +static int __rv_enable_monitor(struct rv_monitor *mon) > { > int retval; > > @@ -289,6 +304,27 @@ static int rv_enable_single(struct rv_monitor *mon) > return retval; > } > > +static int rv_enable_single(struct rv_monitor *mon) > +{ > + struct rv_monitor *cntr = mon->parent; > + struct rv_monitor *p = cntr; > + int retval; > + > + retval = __rv_enable_monitor(mon); > + > + if (!retval && cntr) { > + cntr->enabled = 1; > + list_for_each_entry_continue(p, &rv_monitors_list, list) { > + if (p->parent != cntr) > + break; > + if (!p->enabled) > + cntr->enabled = 2; > + } > + } > + > + return retval; > +} > + > static int rv_update_single(struct rv_monitor *mon, rvmon_update_fn_t > update, void *data) > { > int retval = 0; > @@ -297,7 +333,7 @@ static int rv_update_single(struct rv_monitor *mon, > rvmon_update_fn_t update, vo > enabled = __rv_disable_monitor(mon, true); > update(mon, data); > if (enabled) > - retval = rv_enable_single(mon); > + retval = __rv_enable_monitor(mon); [Severity: Medium] Can this leave mon->parent->enabled stale if re-enabling a nested monitor fails in rv_update_single()? When updating a single nested monitor (for example via monitor_reactors_write() -> monitor_swap_reactors() -> rv_update_monitor() -> rv_update_single()), __rv_disable_monitor() sets mon->enabled = 0 without updating mon->parent->enabled. If the subsequent __rv_enable_monitor(mon) call fails (such as enable_nomiss() failing in init_storage(false)), mon remains disabled while mon->parent->enabled still holds its previous enabled or mixed state. Also, this isn't a bug, but would it be cleaner to use a shared helper with the enum rv_monitor_state constants instead of open-coding the parent state loops with raw 0, 1, and 2 literals in rv_disable_single() and rv_enable_single()? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/cy8pr19mb70098bf493a3b67418c7b9abb7...@cy8pr19mb7009.namprd19.prod.outlook.com?part=3
