On Tue, 24 Oct 2017 21:24:58 +0200,
Takashi Iwai wrote:
>
> 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 <perex@xxxxxxxx>
> > Cc: Takashi Iwai <tiwai@xxxxxxxx>
> > Cc: Takashi Sakamoto <o-takashi@xxxxxxxxxxxxx>
> > Cc: alsa-devel@xxxxxxxxxxxxxxxx
> > Signed-off-by: Kees Cook <keescook@xxxxxxxxxxxx>
> > ---
> > 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.
... and the revised patch is below.
> > @@ -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 :)
In the end, I kept timer_setup() call placed as is in v1 patch.
This call will be moved to the more appropriate place in another
patch, as well as the additional call of missing del_timer_sync() at
*_close().
thanks,
Takashi
-- 8< --
From: Kees Cook <keescook@xxxxxxxxxxxx>
Subject: [PATCH v2] ALSA: sb: Convert timers to use timer_setup()
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.
Signed-off-by: Kees Cook <keescook@xxxxxxxxxxxx>
Signed-off-by: Takashi Iwai <tiwai@xxxxxxx>
---
v1->v2: Re-use the existing chip->midi_substream_output instead of assigning
a new field to struct snd_sb
sound/isa/sb/emu8000_pcm.c | 6 +++---
sound/isa/sb/sb8_midi.c | 11 +++++------
2 files changed, 8 insertions(+), 9 deletions(-)
diff --git a/sound/isa/sb/emu8000_pcm.c b/sound/isa/sb/emu8000_pcm.c
index 8f34551abd8d..bc5af71d3bdb 100644
--- a/sound/isa/sb/emu8000_pcm.c
+++ b/sound/isa/sb/emu8000_pcm.c
@@ -193,9 +193,9 @@ static inline int emu8k_get_curpos(struct snd_emu8k_pcm *rec, int ch)
* timer interrupt handler
* check the current position and update the period if necessary.
*/
-static void emu8k_pcm_timer_func(unsigned long data)
+static void emu8k_pcm_timer_func(struct timer_list *t)
{
- struct snd_emu8k_pcm *rec = (struct snd_emu8k_pcm *)data;
+ struct snd_emu8k_pcm *rec = from_timer(rec, t, timer);
int ptr, delta;
spin_lock(&rec->timer_lock);
@@ -241,7 +241,7 @@ static int emu8k_pcm_open(struct snd_pcm_substream *subs)
runtime->private_data = rec;
spin_lock_init(&rec->timer_lock);
- setup_timer(&rec->timer, emu8k_pcm_timer_func, (unsigned long)rec);
+ timer_setup(&rec->timer, emu8k_pcm_timer_func, 0);
runtime->hw = emu8k_pcm_hw;
runtime->hw.buffer_bytes_max = emu->mem_size - LOOP_BLANK_SIZE * 3;
diff --git a/sound/isa/sb/sb8_midi.c b/sound/isa/sb/sb8_midi.c
index bd672abb4854..05ba2c6b1a8c 100644
--- a/sound/isa/sb/sb8_midi.c
+++ b/sound/isa/sb/sb8_midi.c
@@ -209,10 +209,10 @@ static void snd_sb8dsp_midi_output_write(struct snd_rawmidi_substream *substream
}
}
-static void snd_sb8dsp_midi_output_timer(unsigned long data)
+static void snd_sb8dsp_midi_output_timer(struct timer_list *t)
{
- struct snd_rawmidi_substream *substream = (struct snd_rawmidi_substream *) data;
- struct snd_sb * chip = substream->rmidi->private_data;
+ struct snd_sb *chip = from_timer(chip, t, midi_timer);
+ struct snd_rawmidi_substream *substream = chip->midi_substream_output;
unsigned long flags;
spin_lock_irqsave(&chip->open_lock, flags);
@@ -230,9 +230,8 @@ 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);
+ timer_setup(&chip->midi_timer,
+ snd_sb8dsp_midi_output_timer, 0);
mod_timer(&chip->midi_timer, 1 + jiffies);
chip->open |= SB_OPEN_MIDI_OUTPUT_TRIGGER;
}
--
2.14.2
_______________________________________________
Alsa-devel mailing list
Alsa-devel@xxxxxxxxxxxxxxxx
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
