From patchwork Wed Jun 28 10:16:17 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Takashi Iwai X-Patchwork-Id: 9813875 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id 2E29060383 for ; Wed, 28 Jun 2017 10:16:22 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 2234D284AF for ; Wed, 28 Jun 2017 10:16:22 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 16DDA284EA; Wed, 28 Jun 2017 10:16:22 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-4.2 required=2.0 tests=BAYES_00, RCVD_IN_DNSWL_MED autolearn=ham version=3.3.1 Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) (using TLSv1.2 with cipher DHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.wl.linuxfoundation.org (Postfix) with ESMTPS id 4F43D284AF for ; Wed, 28 Jun 2017 10:16:21 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id DDE2B6E424; Wed, 28 Jun 2017 10:16:20 +0000 (UTC) X-Original-To: intel-gfx@lists.freedesktop.org Delivered-To: intel-gfx@lists.freedesktop.org Received: from mx1.suse.de (mx2.suse.de [195.135.220.15]) by gabe.freedesktop.org (Postfix) with ESMTPS id CDC7F6E424 for ; Wed, 28 Jun 2017 10:16:19 +0000 (UTC) X-Virus-Scanned: by amavisd-new at test-mx.suse.de Received: from relay2.suse.de (charybdis-ext.suse.de [195.135.220.254]) by mx1.suse.de (Postfix) with ESMTP id EE7D0ABF2; Wed, 28 Jun 2017 10:16:17 +0000 (UTC) Date: Wed, 28 Jun 2017 12:16:17 +0200 Message-ID: From: Takashi Iwai To: Daniel Vetter In-Reply-To: References: <1498057734-14622-1-git-send-email-daniel.vetter@ffwll.ch> <149805863799.15021.15278760577807511996@mail.alporthouse.com> <20170626161630.u4ubdzgxr37o63ip@phenom.ffwll.local> 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.2 (x86_64-suse-linux-gnu) MULE/6.0 (HANACHIRUSATO) MIME-Version: 1.0 (generated by SEMI 1.14.6 - "Maruoka") Cc: Vinod Koul , Hardik T Shah , Intel Graphics Development , Guneshwor Singh , Linux Kernel Mailing List , Julia Lawall , Jaroslav Kysela , "Subhransu S. Prusty" Subject: Re: [Intel-gfx] [PATCH] BUG-REPORT: snd-hda: hacked-together EPROBE_DEFER support X-BeenThere: intel-gfx@lists.freedesktop.org X-Mailman-Version: 2.1.18 Precedence: list List-Id: Intel graphics driver community testing & development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" X-Virus-Scanned: ClamAV using ClamSMTP On Mon, 26 Jun 2017 19:54:49 +0200, Daniel Vetter wrote: > > On Mon, Jun 26, 2017 at 7:47 PM, Takashi Iwai wrote: > > On Mon, 26 Jun 2017 18:16:30 +0200, > > Daniel Vetter wrote: > >> > >> On Wed, Jun 21, 2017 at 05:30:10PM +0200, Takashi Iwai wrote: > >> > On Wed, 21 Jun 2017 17:23:57 +0200, > >> > Chris Wilson wrote: > >> > > > >> > > Quoting Daniel Vetter (2017-06-21 16:08:54) > >> > > > So back when the i915 power well support landed in > >> > > > > >> > > > commit 99a2008d0b32d72dfc2a54e7be1eb698dd2e3bd6 > >> > > > Author: Wang Xingchao > >> > > > Date: Thu May 30 22:07:10 2013 +0800 > >> > > > > >> > > > ALSA: hda - Add power-welll support for haswell HDA > >> > > > > >> > > > the logic to handle the cross-module depencies was hand-rolled using a > >> > > > async work item, and that just doesn't work. > >> > > > > >> > > > The correct way to handle cross-module deps is either: > >> > > > - request_module + failing when the other module isn't there > >> > > > > >> > > > OR > >> > > > > >> > > > - failing the module load with EPROBE_DEFER. > >> > > > > >> > > > You can't mix them, if you do then the entire load path just > >> > > > busy-spins blowing through cpu cycles forever with no way to stop > >> > > > this. > >> > > > > >> > > > snd-hda-intel does mix it, because the hda codec drivers are loaded > >> > > > using request_module, but the i915 depency is handled using > >> > > > PROBE_DEFER (or well, should be, but I haven't found any code at all). > >> > > > This is a major pain when trying to debug i915 load failures. > >> > > > > >> > > > This patch here is a horrible hackish attempt at somewhat correctly > >> > > > wriing EPROBE_DEFER through. Stuff that's missing: > >> > > > - Check all the other places where load errors are conveniently > >> > > > dropped on the floor. > >> > > > - Also fix up the firmware_cb path. > >> > > > - Drop the debug noise I've left in to make it clear this isn't > >> > > > anything for merging. > >> > > > >> > > This tames "hdaudio hdaudioC0D0: Unable to bind the codec" which was > >> > > continuously spewing previously, and now the system is usable again. > >> > > >> > Could you give a failing scenario? I'm not opposing to the suggested > >> > solution, we need to fix the mess in anyway, but I just would like to > >> > know how to trigger the problem easily. > >> > >> Disable i915 loading e.g. with i915.modeset=0. Watch how snd-hda* > >> collective blow through 100% of the cpu time spewing into dmesg (and make > >> the system completely unuseable for kernel work because you can't find > >> your own debug printk anymore). > > > > Ah, that's the case we discussed in the past. We know that it's > > problematic for component binding, but we're ignoring this scenario > > because it's supposed to be no real use-case but only for some > > temporary workarounds. > > > > We had some bigger-hammer patchset, but it didn't justify for the > > further development of the reasoning above. > > > >> This is on a snb, where we don't even need the cross-module stuff ... But > >> I think it goes sideways in other cases too, if you simply build but don't > >> load i915. So every time an i915 breaks module load things become real > >> painful. > > > > Even on SNB, we still need i915 for the HDMI/DP ELD notification. The > > hardware inquiry over HD-audio verb was so unstable, so we rather take > > a path directly inquiring to the gfx driver. > > Ah right, forgot about that. > > >> Unfortunately the patch is a bit too big for our fixup branch in drm-tip, > >> so plan B would be to stop building snd-hda (which will make the intel > >> audio team unhappy, but mea culpa if they don't fix this mess). > > > > OK, let me think and take a look for older patchset, too. > > Yeah would be great if we can somehow address this, preferrably using > EPROBE_DEFER or something else that's standard. At least the component > stuff really doesn't work without wiring EPROBE_DEFER through. Now I took a closer look, and this appears rather like a brown paper bag bug, not about the deferred probe or module dependency. The fix patch is below. Could you check whether it works? thanks, Takashi -- 8< -- From: Takashi Iwai Subject: [PATCH] ALSA: hda - Fix endless loop of codec configure azx_codec_configure() loops over the codecs found on the given controller via a linked list. The code used to work in the past, but in the current version, this may lead to an endless loop when a codec binding returns an error. The culprit is that the snd_hda_codec_configure() unregisters the device upon error, and this eventually deletes the given codec object from the bus. Since the list is initialized via list_del_init(), the next object points to the same device itself. This behavior change was introduced at splitting the HD-audio code code, and forgotten to adapt it here. For fixing this bug, just use a *_safe() version of list iteration. Fixes: d068ebc25e6e ("ALSA: hda - Move some codes up to hdac_bus struct") Reported-by: Daniel Vetter Cc: Signed-off-by: Takashi Iwai Tested-by: Daniel Vetter --- sound/pci/hda/hda_codec.h | 2 ++ sound/pci/hda/hda_controller.c | 8 ++++++-- 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/sound/pci/hda/hda_codec.h b/sound/pci/hda/hda_codec.h index d6fb2d5d01a7..60ce1cfc300f 100644 --- a/sound/pci/hda/hda_codec.h +++ b/sound/pci/hda/hda_codec.h @@ -295,6 +295,8 @@ struct hda_codec { #define list_for_each_codec(c, bus) \ list_for_each_entry(c, &(bus)->core.codec_list, core.list) +#define list_for_each_codec_safe(c, n, bus) \ + list_for_each_entry_safe(c, n, &(bus)->core.codec_list, core.list) /* snd_hda_codec_read/write optional flags */ #define HDA_RW_NO_RESPONSE_FALLBACK (1 << 0) diff --git a/sound/pci/hda/hda_controller.c b/sound/pci/hda/hda_controller.c index 3715a5725613..1c60beb5b70a 100644 --- a/sound/pci/hda/hda_controller.c +++ b/sound/pci/hda/hda_controller.c @@ -1337,8 +1337,12 @@ EXPORT_SYMBOL_GPL(azx_probe_codecs); /* configure each codec instance */ int azx_codec_configure(struct azx *chip) { - struct hda_codec *codec; - list_for_each_codec(codec, &chip->bus) { + struct hda_codec *codec, *next; + + /* use _safe version here since snd_hda_codec_configure() deregisters + * the device upon error and deletes itself from the bus list. + */ + list_for_each_codec_safe(codec, next, &chip->bus) { snd_hda_codec_configure(codec); } return 0;