Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751657AbdF1KQi (ORCPT ); Wed, 28 Jun 2017 06:16:38 -0400 Received: from mx2.suse.de ([195.135.220.15]:40099 "EHLO mx1.suse.de" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1751536AbdF1KQ3 (ORCPT ); Wed, 28 Jun 2017 06:16:29 -0400 Date: Wed, 28 Jun 2017 12:16:17 +0200 Message-ID: From: Takashi Iwai To: Daniel Vetter Cc: Chris Wilson , Intel Graphics Development , Guneshwor Singh , Hardik T Shah , Libin Yang , "Subhransu S. Prusty" , Vinod Koul , Julia Lawall , Jaroslav Kysela , Linux Kernel Mailing List Subject: Re: [PATCH] BUG-REPORT: snd-hda: hacked-together EPROBE_DEFER support 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") Content-Type: text/plain; charset=US-ASCII Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 6565 Lines: 158 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 --- 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; -- 2.13.2