[PATCH] ALSA: x86: Fix spinlock usage in the trigger callback

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>

Simply doing 'xset dpms force off' while playback is active seems
sufficient to anger lockdep [1]. And it's of course correct that it's
not safe to use regular spin_lock() outside the irq handler when
the irq handler also wants to acquire the same lock. I believe the
trigger callback can get called from both irq and non-irq context,
so we need to to use spin_lock_irqsave() & co. here.

The whole locking here looks somewhat suspicious, but I don't feel
like digging into further, so here's just the minimal fix.

[1]
[  168.303913] [drm:ilk_audio_codec_disable [i915]] Disable audio codec on port B, pipe B
[  168.306245] =================================
[  168.311110] [ INFO: inconsistent lock state ]
[  168.315975] 4.10.0-rc8-ffrd+ #698 Tainted: G        W
[  168.322198] ---------------------------------
[  168.327062] inconsistent {IN-HARDIRQ-W} -> {HARDIRQ-ON-W} usage.
[  168.333773] kworker/2:1/44 [HC0[0]:SC0[0]:HE1:SE1] takes:
[  168.339802]  (&(&ctx->had_spinlock)->rlock){?.-...}, at: [<ffffffffa02c67e0>] had_pcm_trigger+0x60/0x160 [snd_hdmi_lpe_audio]
[  168.352449] {IN-HARDIRQ-W} state was registered at:
[  168.357901]   __lock_acquire+0x7b2/0x16c0
[  168.362378]   lock_acquire+0xba/0x220
[  168.366468]   _raw_spin_lock_irqsave+0x47/0x60
[  168.371431]   had_substream_get+0x23/0x50 [snd_hdmi_lpe_audio]
[  168.377950]   display_pipe_interrupt_handler+0x78/0x1a0 [snd_hdmi_lpe_audio]
[  168.385828]   __handle_irq_event_percpu+0x3f/0x3a0
[  168.391180]   handle_irq_event_percpu+0x24/0x60
[  168.396239]   handle_irq_event+0x3a/0x60
[  168.400619]   handle_simple_irq+0x79/0xa0
[  168.405096]   generic_handle_irq+0x22/0x30
[  168.409718]   intel_lpe_audio_irq_handler+0x24/0x80 [i915]
[  168.415878]   valleyview_irq_handler+0x1da/0x230 [i915]
[  168.421715]   __handle_irq_event_percpu+0x3f/0x3a0
[  168.427067]   handle_irq_event_percpu+0x24/0x60
[  168.432126]   handle_irq_event+0x3a/0x60
[  168.436505]   handle_edge_irq+0x94/0x150
[  168.440886]   handle_irq+0xb3/0x190
[  168.444781]   do_IRQ+0x69/0x130
[  168.448287]   ret_from_intr+0x0/0x19
[  168.452280]   cpuidle_enter_state+0xdb/0x360
[  168.457047]   cpuidle_enter+0x17/0x20
[  168.461136]   call_cpuidle+0x23/0x40
[  168.465126]   do_idle+0x18a/0x200
[  168.468826]   cpu_startup_entry+0x1d/0x30
[  168.473303]   rest_init+0x12c/0x140
[  168.477200]   start_kernel+0x402/0x40f
[  168.481385]   x86_64_start_reservations+0x2a/0x2c
[  168.486640]   x86_64_start_kernel+0xea/0xed
[  168.491313]   verify_cpu+0x0/0xfc
[  168.495012] irq event stamp: 28481
[  168.498810] hardirqs last  enabled at (28481): [<ffffffff8167feac>] _raw_spin_unlock_irq+0x2c/0x60
[  168.508824] hardirqs last disabled at (28480): [<ffffffff8167fcc8>] _raw_spin_lock_irq+0x18/0x60
[  168.518645] softirqs last  enabled at (28442): [<ffffffff815d6cf9>] neigh_periodic_work+0x219/0x320
[  168.528755] softirqs last disabled at (28438): [<ffffffff815d6b19>] neigh_periodic_work+0x39/0x320
[  168.538767]
               other info that might help us debug this:
[  168.546052]  Possible unsafe locking scenario:

[  168.552664]        CPU0
[  168.555390]        ----
[  168.558116]   lock(&(&ctx->had_spinlock)->rlock);
[  168.563371]   <Interrupt>
[  168.566291]     lock(&(&ctx->had_spinlock)->rlock);
[  168.571741]
                *** DEADLOCK ***

[  168.578355] 3 locks held by kworker/2:1/44:
[  168.583025]  #0:  ("events"){.+.+.+}, at: [<ffffffff810a467f>] process_one_work+0x15f/0x6e0
[  168.592357]  #1:  ((&ctx->hdmi_audio_wq)){+.+.+.}, at: [<ffffffff810a467f>] process_one_work+0x15f/0x6e0
[  168.602959]  #2:  (&ctx->mutex){+.+.+.}, at: [<ffffffffa02c6ee8>] had_audio_wq+0x48/0x6de [snd_hdmi_lpe_audio]
[  168.614145]
               stack backtrace:
[  168.619011] CPU: 2 PID: 44 Comm: kworker/2:1 Tainted: G        W       4.10.0-rc8-ffrd+ #698
[  168.628441] Hardware name: Intel Corp. VALLEYVIEW C0 PLATFORM/BYT-T FFD8, BIOS BLAKFF81.X64.0088.R10.1403240443 FFD8_X64_R_2014_13_1_00 03/24/2014
[  168.643111] Workqueue: events had_audio_wq [snd_hdmi_lpe_audio]
[  168.649725] Call Trace:
[  168.652448]  dump_stack+0x67/0x90
[  168.656149]  print_usage_bug+0x20b/0x210
[  168.660530]  mark_lock+0x648/0x6c0
[  168.664328]  ? print_shortest_lock_dependencies+0x1d0/0x1d0
[  168.670553]  __lock_acquire+0x638/0x16c0
[  168.674935]  lock_acquire+0xba/0x220
[  168.678929]  ? had_pcm_trigger+0x60/0x160 [snd_hdmi_lpe_audio]
[  168.685447]  _raw_spin_lock+0x3f/0x50
[  168.689536]  ? had_pcm_trigger+0x60/0x160 [snd_hdmi_lpe_audio]
[  168.696054]  had_pcm_trigger+0x60/0x160 [snd_hdmi_lpe_audio]
[  168.702383]  snd_pcm_do_stop+0x45/0x50 [snd_pcm]
[  168.707545]  snd_pcm_action_single+0x32/0x80 [snd_pcm]
[  168.713289]  snd_pcm_action+0x118/0x120 [snd_pcm]
[  168.718549]  snd_pcm_stop+0x1a/0x20 [snd_pcm]
[  168.723415]  had_audio_wq+0x35e/0x6de [snd_hdmi_lpe_audio]
[  168.729545]  process_one_work+0x1eb/0x6e0
[  168.734022]  ? process_one_work+0x15f/0x6e0
[  168.738693]  worker_thread+0x4f/0x4a0
[  168.742781]  ? schedule+0x4a/0x90
[  168.746483]  ? preempt_count_sub+0xa6/0x110
[  168.751154]  kthread+0x10e/0x150
[  168.754757]  ? apply_wqattrs_commit+0x90/0x90
[  168.759621]  ? kthread_create_on_node+0x40/0x40
[  168.764681]  ? umh_complete+0x30/0x30
[  168.768770]  ? call_usermodehelper_exec_async+0x13a/0x150
[  168.774802]  ret_from_fork+0x2e/0x40
[  168.779412] [drm:intel_disable_pipe [i915]] disabling pipe B

Cc: Takashi Iwai <tiwai@xxxxxxx>
Cc: Pierre-Louis Bossart <pierre-louis.bossart@xxxxxxxxxxxxxxx>
Signed-off-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
---
 sound/x86/intel_hdmi_audio.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/sound/x86/intel_hdmi_audio.c b/sound/x86/intel_hdmi_audio.c
index c0a080e5d1f4..2748766b3f25 100644
--- a/sound/x86/intel_hdmi_audio.c
+++ b/sound/x86/intel_hdmi_audio.c
@@ -1154,6 +1154,7 @@ static int had_pcm_hw_free(struct snd_pcm_substream *substream)
 static int had_pcm_trigger(struct snd_pcm_substream *substream, int cmd)
 {
 	int retval = 0;
+	unsigned long flags;
 	struct snd_intelhad *intelhaddata;
 
 	intelhaddata = snd_pcm_substream_chip(substream);
@@ -1180,12 +1181,12 @@ static int had_pcm_trigger(struct snd_pcm_substream *substream, int cmd)
 	case SNDRV_PCM_TRIGGER_STOP:
 	case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
 	case SNDRV_PCM_TRIGGER_SUSPEND:
-		spin_lock(&intelhaddata->had_spinlock);
+		spin_lock_irqsave(&intelhaddata->had_spinlock, flags);
 
 		/* Stop reporting BUFFER_DONE/UNDERRUN to above layers */
 
 		intelhaddata->stream_info.running = false;
-		spin_unlock(&intelhaddata->had_spinlock);
+		spin_unlock_irqrestore(&intelhaddata->had_spinlock, flags);
 		/* Disable Audio */
 		had_enable_audio(intelhaddata, false);
 		/* Reset buffer pointers */
-- 
2.10.2

_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx




[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]
  Powered by Linux