From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751756AbdJXTZC (ORCPT ); Tue, 24 Oct 2017 15:25:02 -0400 Received: from mx2.suse.de ([195.135.220.15]:55899 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751267AbdJXTY7 (ORCPT ); Tue, 24 Oct 2017 15:24:59 -0400 Date: Tue, 24 Oct 2017 21:24:58 +0200 Message-ID: From: Takashi Iwai To: Kees Cook Cc: Jaroslav Kysela , Takashi Sakamoto , alsa-devel@alsa-project.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH] ALSA: sb: Convert timers to use timer_setup() In-Reply-To: <20171024153458.GA112893@beast> References: <20171024153458.GA112893@beast> User-Agent: Wanderlust/2.15.9 (Almost Unreal) SEMI/1.14.6 (Maruoka) FLIM/1.14.9 (=?UTF-8?B?R29qxY0=?=) APEL/10.8 Emacs/25.3 (x86_64-suse-linux-gnu) MULE/6.0 (HANACHIRUSATO) MIME-Version: 1.0 (generated by SEMI 1.14.6 - "Maruoka") Content-Type: text/plain; charset=US-ASCII Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tue, 24 Oct 2017 17:34:58 +0200, Kees Cook wrote: > > In preparation for unconditionally passing the struct timer_list pointer to > all timer callbacks, switch to using the new timer_setup() and from_timer() > to pass the timer pointer explicitly. > > Cc: Jaroslav Kysela > Cc: Takashi Iwai > Cc: Takashi Sakamoto > Cc: alsa-devel@alsa-project.org > Signed-off-by: Kees Cook > --- > include/sound/sb.h | 1 + > sound/isa/sb/emu8000_pcm.c | 6 +++--- > sound/isa/sb/sb8_midi.c | 12 ++++++------ > 3 files changed, 10 insertions(+), 9 deletions(-) > > diff --git a/include/sound/sb.h b/include/sound/sb.h > index bacefaee411a..4aef395189ec 100644 > --- a/include/sound/sb.h > +++ b/include/sound/sb.h > @@ -83,6 +83,7 @@ struct snd_sb { > unsigned int playback_format; > unsigned int capture_format; > struct timer_list midi_timer; > + struct snd_rawmidi_substream *timer_substream; This is superfluous. It's identical with chip->midi_substream_output, as this timer is used only for the output stream. > @@ -230,9 +230,9 @@ static void snd_sb8dsp_midi_output_trigger(struct snd_rawmidi_substream *substre > spin_lock_irqsave(&chip->open_lock, flags); > if (up) { > if (!(chip->open & SB_OPEN_MIDI_OUTPUT_TRIGGER)) { > - setup_timer(&chip->midi_timer, > - snd_sb8dsp_midi_output_timer, > - (unsigned long) substream); > + chip->timer_substream = substream; > + timer_setup(&chip->midi_timer, > + snd_sb8dsp_midi_output_timer, 0); I think it's OK to replace straightforwardly like this for this particular patch (so that the conversion becomes clearer), but ideally speaking, timer_setup() should go into either *_output_open() or snd_sb8dsp_midi() so that it's called only once per stream open or once at creation. And, looking at the code closely now, it seems that del_timer_sync() is missing at the close side, too, but maybe it didn't matter much for such an old hardware :) thanks, Takashi