ALSA: sb: Convert timers to use timer_setup()

PATCH In review Kees Cook 2 participants 3 messages Oct 24, 2017 Sound

Share & tools

Kees Cook

author

2017-10-24 15:34 UTC #0 lore raw

PATCH 1/1 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.

Cc Jaroslav Kysela <[email protected]>

Cc Takashi Iwai <[email protected]>

Cc Takashi Sakamoto <[email protected]>

Cc [email protected]

Signed-off-by Kees Cook <[email protected]>

---
 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(-)

include/sound/sb.h +1 -0

index bacefaee411a..4aef395189ec 100644
@@ -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;
 	unsigned int p_dma_size;
 	unsigned int p_period_size;
 	unsigned int c_dma_size;

sound/isa/sb/emu8000_pcm.c +3 -3

index 8f34551abd8d..bc5af71d3bdb 100644
@@ -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;

sound/isa/sb/sb8_midi.c +6 -6

index bd672abb4854..cf23dc5d0507 100644
@@ -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->timer_substream;
 	unsigned long flags;
 
 	spin_lock_irqsave(&chip->open_lock, flags);

@@ -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);
 			mod_timer(&chip->midi_timer, 1 + jiffies);
 			chip->open |= SB_OPEN_MIDI_OUTPUT_TRIGGER;
 		}

Signature · 6 lines

2.7.4


-- 
Kees Cook
Pixel Security

Takashi Iwai

2017-10-24 19:24 UTC #1 lore raw

Quoted 21 lines · Kees Cook

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 <[email protected]>
Cc Takashi Iwai <[email protected]>
Cc Takashi Sakamoto <[email protected]>
Cc [email protected]
Signed-off-by Kees Cook <[email protected]>
---
 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(-)
include/sound/sb.h +0 -0
index bacefaee411a..4aef395189ec 100644

include/sound/sb.h struct snd_sb { · line 86

Quoted 4 lines

include/sound/sb.h +1 -0

@@ -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.

include/sound/sb.h static void snd_sb8dsp_midi_output_trigger(struct snd_rawmidi_substream *substre · line 235

Quoted 10 lines

include/sound/sb.h +3 -3

@@ -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

Takashi Iwai

2017-10-24 19:40 UTC #2 lore raw

Quoted 19 lines · Takashi Iwai

Quoted 16 lines · Kees Cook
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 <[email protected]>
Cc Takashi Iwai <[email protected]>
Cc Takashi Sakamoto <[email protected]>
Cc [email protected]
Signed-off-by Kees Cook <[email protected]>
---
 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(-)

Quoted 12 lines

include/sound/sb.h struct snd_sb { · line 86
Quoted 9 lines
include/sound/sb.h +1 -0
index bacefaee411a..4aef395189ec 100644
@@ -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.

Quoted 17 lines

include/sound/sb.h static void snd_sb8dsp_midi_output_trigger(struct snd_rawmidi_substream *substre · line 235
Quoted 10 lines
include/sound/sb.h +3 -3
@@ -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.

Quoted 3 lines

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 <[email protected]>
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 <[email protected]>

Signed-off-by Takashi Iwai <[email protected]>

---

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(-)

sound/isa/sb/emu8000_pcm.c +3 -3

index 8f34551abd8d..bc5af71d3bdb 100644
@@ -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;

sound/isa/sb/sb8_midi.c +5 -6

index bd672abb4854..05ba2c6b1a8c 100644
@@ -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;
 		}

Signature · 1 line

2.14.2