On Tue, 15 Oct 2019, Petr Mladek wrote:
> On Mon 2019-10-14 11:17:19, Steven Rostedt wrote:
> > On Mon, 14 Oct 2019 12:59:23 +0200
> > Miroslav Benes <mbenes@xxxxxxx> wrote:
> >
> > > int
> > > ftrace_enable_sysctl(struct ctl_table *table, int write,
> > > void __user *buffer, size_t *lenp,
> > > @@ -6740,8 +6754,6 @@ ftrace_enable_sysctl(struct ctl_table *table, int write,
> > > if (ret || !write || (last_ftrace_enabled == !!ftrace_enabled))
> > > goto out;
> > >
> > > - last_ftrace_enabled = !!ftrace_enabled;
> > > -
> > > if (ftrace_enabled) {
> > >
> > > /* we are starting ftrace again */
> > > @@ -6752,12 +6764,19 @@ ftrace_enable_sysctl(struct ctl_table *table, int write,
> > > ftrace_startup_sysctl();
> > >
> > > } else {
> > > + if (is_permanent_ops_registered()) {
> > > + ftrace_enabled = last_ftrace_enabled;
> >
> > Although this is not incorrect, but may be somewhat confusing.
> >
> > At this location, last_ftrace_enabled is always true.
> >
> > I'm thinking this would be better to simply set it to false here.
>
> IMHO, we want to set ftrace_enabled = true here.
>
> It was set to "false" by writing to the sysfs file. But the change
> gets rejected. ftrace will stay enabled. So, we should set
> the value back to "true".
That's correct.
I can make it explicit as proposed. I just thought that 'ftrace_enabled =
last_ftrace_enabled' was clear enough given Petr's explanation.
> > > + ret = -EBUSY;
> > > + goto out;
> > > + }
> > > +
> > > /* stopping ftrace calls (just send to ftrace_stub) */
> > > ftrace_trace_function = ftrace_stub;
> > >
> > > ftrace_shutdown_sysctl();
> > > }
> > >
> > > + last_ftrace_enabled = !!ftrace_enabled;
> > > out:
> >
> > And move the assignment of last_ftrace_enabled to after the "out:"
> > label.
>
> This change might make sense anyway. But it is not strictly necessary
> from my POV.
If it stays before "out:" label, last_ftrace_enabled is set if and only if
it has to be set. I think it is better, but I can, of course, move it in
v3 if Steven prefers it.
Thanks
Miroslav
