[PATCH v3] PM: Delete timer before removing wakeup source from list

[PATCH v3] PM: Delete timer before removing wakeup source from list

[Kaushlendra Kumar]

Replace timer_delete_sync() with timer_shutdown_sync() and move
it before list_del_rcu() in wakeup_source_remove() to improve the
cleanup ordering and code clarity. This change ensures that the
timer is stopped before removing the wakeup source from the
events list, providing a more logical cleanup sequence.

While the current ordering is functionally correct, stopping the timer
first makes the cleanup flow more intuitive and follows the general
pattern of disabling active components before removing data structures.

Signed-off-by: Kaushlendra Kumar <kaushlendra.kumar@intel.com>
---
Changes in v3:
- Use timer_shutdown_sync() instead of timer_delete_sync() to prevent
  timer re-arming as per review feedback
- Remove timer.function clearing as timer_shutdown_sync() handles it

Changes in v2:
- Reframed as cleanup/improvement rather than fix

 drivers/base/power/wakeup.c | 8 +-------
 1 file changed, 1 insertion(+), 7 deletions(-)

diff --git a/drivers/base/power/wakeup.c b/drivers/base/power/wakeup.c
index d1283ff1080b..ab3eee23a52d 100644
--- a/drivers/base/power/wakeup.c
+++ b/drivers/base/power/wakeup.c
@@ -189,17 +189,11 @@ static void wakeup_source_remove(struct wakeup_source *ws)
 	if (WARN_ON(!ws))
 		return;
 
+	timer_shutdown_sync(&ws->timer);
 	raw_spin_lock_irqsave(&events_lock, flags);
 	list_del_rcu(&ws->entry);
 	raw_spin_unlock_irqrestore(&events_lock, flags);
 	synchronize_srcu(&wakeup_srcu);
-
-	timer_delete_sync(&ws->timer);
-	/*
-	 * Clear timer.function to make wakeup_source_not_registered() treat
-	 * this wakeup source as not registered.
-	 */
-	ws->timer.function = NULL;
 }
 
 /**
-- 
2.34.1

Re: [PATCH v3] PM: Delete timer before removing wakeup source from list

[Rafael J. Wysocki]

On Mon, Oct 27, 2025 at 5:43 AM Kaushlendra Kumar
<kaushlendra.kumar@intel.com> wrote:
>
> Replace timer_delete_sync() with timer_shutdown_sync() and move
> it before list_del_rcu() in wakeup_source_remove() to improve the
> cleanup ordering and code clarity. This change ensures that the
> timer is stopped before removing the wakeup source from the
> events list, providing a more logical cleanup sequence.
>
> While the current ordering is functionally correct, stopping the timer
> first makes the cleanup flow more intuitive and follows the general
> pattern of disabling active components before removing data structures.
>
> Signed-off-by: Kaushlendra Kumar <kaushlendra.kumar@intel.com>
> ---
> Changes in v3:
> - Use timer_shutdown_sync() instead of timer_delete_sync() to prevent
>   timer re-arming as per review feedback
> - Remove timer.function clearing as timer_shutdown_sync() handles it

So you need to delete the WARN_ONCE() from wakeup_source_activate()
along with wakeup_source_not_registered() because now it may trigger
before removing the wakeup source from the list.

Frankly, I'm not sure you know what you are doing here and I'm not
going to consider any new versions of this patch until I'm convinced
that this is the case.

Thanks!

RE: [PATCH v3] PM: Delete timer before removing wakeup source from list

[Kumar, Kaushlendra]

On [Date], [Reviewer Name] wrote:
> So you need to delete the WARN_ONCE() from wakeup_source_activate() along 
> with wakeup_source_not_registered() because now it may trigger before 
> removing the wakeup source from the list.
>
> Frankly, I'm not sure you know what you are doing here and I'm not going 
> to consider any new versions of this patch until I'm convinced that this 
> is the case.

Thank you for the feedback. 
When timer_shutdown_sync() is called, it internally sets ws->timer.function to NULL within the __try_to_del_timer_sync() function. Since timer_shutdown_sync() already handles clearing the function pointer, the explicit ws->timer.function = NULL assignment that follows becomes redundant and can be safely removed.

Are you suggesting here a simpler approach: just replace timer_delete_sync(&ws->timer) with timer_shutdown_sync(&ws->timer) and remove the ws->timer.function = NULL line, without changing the ordering of operations?

Am I missing something here?

BR,
Kaushlendra

Re: [PATCH v3] PM: Delete timer before removing wakeup source from list

[Rafael J. Wysocki]

On Sat, Nov 8, 2025 at 8:17 AM Kumar, Kaushlendra
<kaushlendra.kumar@intel.com> wrote:
>
> On [Date], [Reviewer Name] wrote:
> > So you need to delete the WARN_ONCE() from wakeup_source_activate() along
> > with wakeup_source_not_registered() because now it may trigger before
> > removing the wakeup source from the list.
> >
> > Frankly, I'm not sure you know what you are doing here and I'm not going
> > to consider any new versions of this patch until I'm convinced that this
> > is the case.
>
> Thank you for the feedback.
> When timer_shutdown_sync() is called, it internally sets ws->timer.function to NULL within the __try_to_del_timer_sync() function. Since timer_shutdown_sync() already handles clearing the function pointer, the explicit ws->timer.function = NULL assignment that follows becomes redundant and can be safely removed.
>
> Are you suggesting here a simpler approach: just replace timer_delete_sync(&ws->timer) with timer_shutdown_sync(&ws->timer) and remove the ws->timer.function = NULL line, without changing the ordering of operations?

I thought about doing that, but on second thought the $subject patch
may be a good change after all because people are generally expected
to stop using the given wakeup source before calling
wakeup_source_unregister() on it.

So I'm going to apply it, but I will send a follow-up patch to make
some more changes on top of it.

Thanks!