Re: [RFC PATCH] tracing: fix return value in __ftrace_event_enable_disable for TRACE_REG_UNREGISTER

[Gabriele Paoloni]

On Wed, Mar 19, 2025 at 10:13 AM Steven Rostedt <rostedt@xxxxxxxxxxx> wrote:
>
> On Tue, 18 Mar 2025 11:07:00 +0900
> Masami Hiramatsu (Google) <mhiramat@xxxxxxxxxx> wrote:
>
> > > --- a/kernel/trace/trace_events.c
> > > +++ b/kernel/trace/trace_events.c
> > > @@ -790,7 +790,7 @@ static int __ftrace_event_enable_disable(struct trace_event_file *file,
> > >                             clear_bit(EVENT_FILE_FL_RECORDED_TGID_BIT, &file->flags);
> > >                     }
> > >
> > > -                   call->class->reg(call, TRACE_REG_UNREGISTER, file);
> > > +                   ret = call->class->reg(call, TRACE_REG_UNREGISTER, file);
> >
> > This is not enough. As same as enable failure, this function needs to handle
> > this error to report it and break.
>
> Perhaps all we should do here is:
>
>                         WARN_ON_ONCE(ret);

Yes I was trying to address the request from Masami however,
if we look at the error path of TRACE_REG_REGISTER what happens is:

ret = call->class->reg(call, TRACE_REG_REGISTER, file);
if (ret) {
   if (cmd)
       tracing_stop_cmdline_record();
    if (tgid)
       tracing_stop_tgid_record();
    pr_info("event trace: Could not enable event "
       "%s\n", trace_event_name(call));
    break;

In this case it makes sense since it will undo what was done before
enabling the trace event. However I don't think it would make sense
to invoke tracing_start_cmdline_record and/or tracing_start_tgid_record
if the unregister callbacks fails since the event could be in a sort of
unknown state (i.e. we cannot trust the event to still be enabled).

Also it is probably not appropriate to log the event using exclusively

         pr_info("event trace: Could not disable event "
                 "%s\n", trace_event_name(call));

So I would agree with Steven or else

      if (ret)
           pr_warn("event trace: Could not disable event "
                   "%s\n", trace_event_name(call));

Many thanks for the review
Gab
>
> -- Steve
>