linux-kernel - Re: [PATCH] ALSA: sb: Convert timers to use timer

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <s5htvyopcvp.wl-tiwai@suse.de>
Date:   Tue, 24 Oct 2017 21:24:58 +0200
From:   Takashi Iwai <tiwai@...e.de>
To:     Kees Cook <keescook@...omium.org>
Cc:     Jaroslav Kysela <perex@...ex.cz>,
        Takashi Sakamoto <o-takashi@...amocchi.jp>,
        alsa-devel@...a-project.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] ALSA: sb: Convert timers to use timer_setup()

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@...ex.cz>
> Cc: Takashi Iwai <tiwai@...e.com>
> Cc: Takashi Sakamoto <o-takashi@...amocchi.jp>
> Cc: alsa-devel@...a-project.org
> Signed-off-by: Kees Cook <keescook@...omium.org>
> ---
>  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