Messages in this thread Patch in this message | | | Date | Thu, 7 Apr 2022 21:36:44 -0400 | From | Steven Rostedt <> | Subject | Re: [RFC][PATCH] timers: Add del_time_free() to be called before freeing timers |
| |
On Thu, 7 Apr 2022 17:58:09 -0700 Guenter Roeck <linux@roeck-us.net> wrote:
> >>> Add a del_timer_free() that not only does a del_timer_sync() but will mark > >> > >> This limits the use case to situations where del_timer_sync() can actually > >> be called. There is, however, code where this is not possible. > >> Specifically, it doesn't work if the code triggered with the timer uses a > >> lock, and del_timer() is also called under that same lock. An example for > >> that is the code in sound/synth/emux/emux.c. How do you suggest to handle > >> that situation ? > > > > Easy. Tell me how that situation is not a bug? > > > > Sure, fixing the problem is of course the right thing to do. But replacing > del_timer() with your suggested version of del_timer_free() doesn't work
I meant replacing the entire block with del_timer_free().
diff --git a/sound/synth/emux/emux.c b/sound/synth/emux/emux.c index 5ed8e36d2e04..f631e090e074 100644 --- a/sound/synth/emux/emux.c +++ b/sound/synth/emux/emux.c @@ -131,10 +131,7 @@ int snd_emux_free(struct snd_emux *emu) if (! emu) return -EINVAL; - spin_lock_irqsave(&emu->voice_lock, flags); - if (emu->timer_active) - del_timer(&emu->tlist); - spin_unlock_irqrestore(&emu->voice_lock, flags); + del_timer_free(&emu->tlist); snd_emux_proc_free(emu); snd_emux_delete_virmidi(emu); It doesn't hurt to delete it if it wasn't queued. I'm not sure what the dance with spinlocks are all about.
The above may actually be enough. I don't see where the timer could be enqueued again after that.
That code goes back to original git history, so it was probably trying to do it's own del_timer_sync() albeit poorly.
> with this code because it would deadlock. Sure, that would not fix the > underlying problem anyway, but that isn't the point I was trying to make: > I think it would be beneficial to be able to replace del_timer() with a > version that can not result in deadlocks but would still identify problems > such as the one in the code in emux.c. > > Can we have del_timer_free() and del_timer_sync_free() ? Or am I missing > something and that doesn't really make sense ?
No, that doesn't make sense.
-- Steve
| |