[PATCH -next] trace/fgraph: Fix the warning caused by missing unregister notifier

[PATCH -next] trace/fgraph: Fix the warning caused by missing unregister notifier

[Ye Weihua]

This warning was triggered during testing on v6.16:

notifier callback ftrace_suspend_notifier_call already registered
WARNING: CPU: 2 PID: 86 at kernel/notifier.c:23 notifier_chain_register+0x44/0xb0
...
Call Trace:
 <TASK>
 blocking_notifier_chain_register+0x34/0x60
 register_ftrace_graph+0x330/0x410
 ftrace_profile_write+0x1e9/0x340
 vfs_write+0xf8/0x420
 ? filp_flush+0x8a/0xa0
 ? filp_close+0x1f/0x30
 ? do_dup2+0xaf/0x160
 ksys_write+0x65/0xe0
 do_syscall_64+0xa4/0x260
 entry_SYSCALL_64_after_hwframe+0x77/0x7f

When writing to the function_profile_enabled interface, the notifier was
not unregistered after start_graph_tracing failed, causing a warning the
next time function_profile_enabled was written.

Fixed by adding unregister_pm_notifier in the exception path.

Fixes: 4a2b8dda3f870 ("tracing/function-graph-tracer: fix a regression while suspend to disk")
Signed-off-by: Ye Weihua <yeweihua4@huawei.com>
---
 kernel/trace/fgraph.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/kernel/trace/fgraph.c b/kernel/trace/fgraph.c
index c5b207992fb4..dac2d58f3949 100644
--- a/kernel/trace/fgraph.c
+++ b/kernel/trace/fgraph.c
@@ -1391,10 +1391,11 @@ int register_ftrace_graph(struct fgraph_ops *gops)
 error:
 	if (ret) {
 		ftrace_graph_active--;
 		gops->saved_func = NULL;
 		fgraph_lru_release_index(i);
+		unregister_pm_notifier(&ftrace_suspend_notifier);
 	}
 	return ret;
 }
 
 void unregister_ftrace_graph(struct fgraph_ops *gops)
-- 
2.34.1

Re: [PATCH -next] trace/fgraph: Fix the warning caused by missing unregister notifier

[Masami Hiramatsu]

On Mon, 18 Aug 2025 07:33:32 +0000
Ye Weihua <yeweihua4@huawei.com> wrote:

> This warning was triggered during testing on v6.16:
> 
> notifier callback ftrace_suspend_notifier_call already registered
> WARNING: CPU: 2 PID: 86 at kernel/notifier.c:23 notifier_chain_register+0x44/0xb0
> ...
> Call Trace:
>  <TASK>
>  blocking_notifier_chain_register+0x34/0x60
>  register_ftrace_graph+0x330/0x410
>  ftrace_profile_write+0x1e9/0x340
>  vfs_write+0xf8/0x420
>  ? filp_flush+0x8a/0xa0
>  ? filp_close+0x1f/0x30
>  ? do_dup2+0xaf/0x160
>  ksys_write+0x65/0xe0
>  do_syscall_64+0xa4/0x260
>  entry_SYSCALL_64_after_hwframe+0x77/0x7f
> 
> When writing to the function_profile_enabled interface, the notifier was
> not unregistered after start_graph_tracing failed, causing a warning the
> next time function_profile_enabled was written.
> 
> Fixed by adding unregister_pm_notifier in the exception path.
> 

This looks good to me.

Acked-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>

Thanks!

> Fixes: 4a2b8dda3f870 ("tracing/function-graph-tracer: fix a regression while suspend to disk")
> Signed-off-by: Ye Weihua <yeweihua4@huawei.com>
> ---
>  kernel/trace/fgraph.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/kernel/trace/fgraph.c b/kernel/trace/fgraph.c
> index c5b207992fb4..dac2d58f3949 100644
> --- a/kernel/trace/fgraph.c
> +++ b/kernel/trace/fgraph.c
> @@ -1391,10 +1391,11 @@ int register_ftrace_graph(struct fgraph_ops *gops)
>  error:
>  	if (ret) {
>  		ftrace_graph_active--;
>  		gops->saved_func = NULL;
>  		fgraph_lru_release_index(i);
> +		unregister_pm_notifier(&ftrace_suspend_notifier);
>  	}
>  	return ret;
>  }
>  
>  void unregister_ftrace_graph(struct fgraph_ops *gops)
> -- 
> 2.34.1
> 


-- 
Masami Hiramatsu (Google) <mhiramat@kernel.org>

Re: [PATCH -next] trace/fgraph: Fix the warning caused by missing unregister notifier

[Guenter Roeck]

Hi,

On Mon, Aug 18, 2025 at 07:33:32AM +0000, Ye Weihua wrote:
> This warning was triggered during testing on v6.16:
> 
> notifier callback ftrace_suspend_notifier_call already registered
> WARNING: CPU: 2 PID: 86 at kernel/notifier.c:23 notifier_chain_register+0x44/0xb0
> ...
> Call Trace:
>  <TASK>
>  blocking_notifier_chain_register+0x34/0x60
>  register_ftrace_graph+0x330/0x410
>  ftrace_profile_write+0x1e9/0x340
>  vfs_write+0xf8/0x420
>  ? filp_flush+0x8a/0xa0
>  ? filp_close+0x1f/0x30
>  ? do_dup2+0xaf/0x160
>  ksys_write+0x65/0xe0
>  do_syscall_64+0xa4/0x260
>  entry_SYSCALL_64_after_hwframe+0x77/0x7f
> 
> When writing to the function_profile_enabled interface, the notifier was
> not unregistered after start_graph_tracing failed, causing a warning the
> next time function_profile_enabled was written.
> 
> Fixed by adding unregister_pm_notifier in the exception path.
> 
> Fixes: 4a2b8dda3f870 ("tracing/function-graph-tracer: fix a regression while suspend to disk")
> Signed-off-by: Ye Weihua <yeweihua4@huawei.com>
> ---
>  kernel/trace/fgraph.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/kernel/trace/fgraph.c b/kernel/trace/fgraph.c
> index c5b207992fb4..dac2d58f3949 100644
> --- a/kernel/trace/fgraph.c
> +++ b/kernel/trace/fgraph.c
> @@ -1391,10 +1391,11 @@ int register_ftrace_graph(struct fgraph_ops *gops)
>  error:
>  	if (ret) {
>  		ftrace_graph_active--;
>  		gops->saved_func = NULL;
>  		fgraph_lru_release_index(i);
> +		unregister_pm_notifier(&ftrace_suspend_notifier);

Is this really correct ? The pm notifier is only registered if
ftrace_graph_active==1, but not if it is larger than that.
The above code unregisters it unconditionally, even if
ftrace_graph_active > 1. I can see that the resulting double
unregistration in unregister_ftrace_graph() doesn't really
matter since the error return will be ignored, but is it really
irrelevant for the successful registered graphs no longer get the
benefit of the pm notifier callback ?

Guenter

Re: [PATCH -next] trace/fgraph: Fix the warning caused by missing unregister notifier

[Steven Rostedt]

On Fri, 5 Sep 2025 16:19:02 -0700
Guenter Roeck <linux@roeck-us.net> wrote:

> > +++ b/kernel/trace/fgraph.c
> > @@ -1391,10 +1391,11 @@ int register_ftrace_graph(struct fgraph_ops *gops)
> >  error:
> >  	if (ret) {
> >  		ftrace_graph_active--;
> >  		gops->saved_func = NULL;
> >  		fgraph_lru_release_index(i);
> > +		unregister_pm_notifier(&ftrace_suspend_notifier);  
> 
> Is this really correct ? The pm notifier is only registered if
> ftrace_graph_active==1, but not if it is larger than that.
> The above code unregisters it unconditionally, even if
> ftrace_graph_active > 1. I can see that the resulting double
> unregistration in unregister_ftrace_graph() doesn't really
> matter since the error return will be ignored, but is it really
> irrelevant for the successful registered graphs no longer get the
> benefit of the pm notifier callback ?

Ah right, it should be:

error:
	if (ret) {
		ftrace_graph_active--;
		gops->saved_func = NULL;
		fgraph_lru_release_index(i);
		if (!ftrace_graph_active)
			unregister_pm_notifier(&ftrace_suspend_notifier);
	}
	return ret;

I missed that there's a:

	ret = ftrace_startup_subops(&graph_ops, &gops->ops, command);
	if (!ret)
		fgraph_array[i] = gops;

Just before the error label, so the goto error isn't the only path there
that can affect the ret variable.

I could add a patch or you could send one.

-- Steve

Re: [PATCH -next] trace/fgraph: Fix the warning caused by missing unregister notifier

[Guenter Roeck]

On 9/5/25 19:30, Steven Rostedt wrote:
> On Fri, 5 Sep 2025 16:19:02 -0700
> Guenter Roeck <linux@roeck-us.net> wrote:
> 
>>> +++ b/kernel/trace/fgraph.c
>>> @@ -1391,10 +1391,11 @@ int register_ftrace_graph(struct fgraph_ops *gops)
>>>   error:
>>>   	if (ret) {
>>>   		ftrace_graph_active--;
>>>   		gops->saved_func = NULL;
>>>   		fgraph_lru_release_index(i);
>>> +		unregister_pm_notifier(&ftrace_suspend_notifier);
>>
>> Is this really correct ? The pm notifier is only registered if
>> ftrace_graph_active==1, but not if it is larger than that.
>> The above code unregisters it unconditionally, even if
>> ftrace_graph_active > 1. I can see that the resulting double
>> unregistration in unregister_ftrace_graph() doesn't really
>> matter since the error return will be ignored, but is it really
>> irrelevant for the successful registered graphs no longer get the
>> benefit of the pm notifier callback ?
> 
> Ah right, it should be:
> 
> error:
> 	if (ret) {
> 		ftrace_graph_active--;
> 		gops->saved_func = NULL;
> 		fgraph_lru_release_index(i);
> 		if (!ftrace_graph_active)
> 			unregister_pm_notifier(&ftrace_suspend_notifier);
> 	}
> 	return ret;
> 
> I missed that there's a:
> 
> 	ret = ftrace_startup_subops(&graph_ops, &gops->ops, command);
> 	if (!ret)
> 		fgraph_array[i] = gops;
> 
> Just before the error label, so the goto error isn't the only path there
> that can affect the ret variable.
> 
> I could add a patch or you could send one.
> 


I'll try to send a patch later tonight.

Guenter