From patchwork Thu Sep 24 09:49:57 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Takashi Iwai X-Patchwork-Id: 7254831 Return-Path: X-Original-To: patchwork-linux-arm@patchwork.kernel.org Delivered-To: patchwork-parsemail@patchwork2.web.kernel.org Received: from mail.kernel.org (mail.kernel.org [198.145.29.136]) by patchwork2.web.kernel.org (Postfix) with ESMTP id BA148BEEC1 for ; Thu, 24 Sep 2015 09:51:59 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id AFC3120804 for ; Thu, 24 Sep 2015 09:51:58 +0000 (UTC) Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.9]) (using TLSv1.2 with cipher AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 02272207F5 for ; Thu, 24 Sep 2015 09:51:53 +0000 (UTC) Received: from localhost ([127.0.0.1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.80.1 #2 (Red Hat Linux)) id 1Zf3AW-0008T2-6y; Thu, 24 Sep 2015 09:50:24 +0000 Received: from mx2.suse.de ([195.135.220.15]) by bombadil.infradead.org with esmtps (Exim 4.80.1 #2 (Red Hat Linux)) id 1Zf3AT-0007EK-5d for linux-arm-kernel@lists.infradead.org; Thu, 24 Sep 2015 09:50:22 +0000 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 mx2.suse.de (Postfix) with ESMTP id 1AC85AB1A; Thu, 24 Sep 2015 09:49:57 +0000 (UTC) Date: Thu, 24 Sep 2015 11:49:57 +0200 Message-ID: From: Takashi Iwai To: "Thierry Reding" Subject: Re: [PATCH 0/3] ALSA: hda - Avoid potential deadlock In-Reply-To: References: <1442484006-9614-1-git-send-email-thierry.reding@gmail.com> 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/24.5 (x86_64-suse-linux-gnu) MULE/6.0 (HANACHIRUSATO) MIME-Version: 1.0 (generated by SEMI 1.14.6 - "Maruoka") X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20150924_025021_596878_59EB52BF X-CRM114-Status: GOOD ( 32.56 ) X-Spam-Score: -4.2 (----) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.20 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: alsa-devel@alsa-project.org, Kevin Hilman , Tyler Baker , linux-kernel@vger.kernel.org, Jon Hunter , linux-tegra@vger.kernel.org, linux-arm-kernel@lists.infradead.org Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+patchwork-linux-arm=patchwork.kernel.org@lists.infradead.org X-Spam-Status: No, score=-4.2 required=5.0 tests=BAYES_00, RCVD_IN_DNSWL_MED, RP_MATCHES_RCVD, UNPARSEABLE_RELAY autolearn=unavailable version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on mail.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP On Wed, 23 Sep 2015 11:03:44 +0200, Takashi Iwai wrote: > > On Thu, 17 Sep 2015 12:00:03 +0200, > Thierry Reding wrote: > > > > From: Thierry Reding > > > > The Tegra HDA controller driver committed in v3.16 causes deadlocks when > > loaded as a module. The reason is that the driver core will lock the HDA > > controller device upon calling its probe callback and the probe callback > > then goes on to create child devices for detected codecs and loads their > > modules via a request_module() call. This is problematic because the new > > driver will immediately be bound to the device, which will in turn cause > > the parent of the codec device (the HDA controller device) to be locked > > again, causing a deadlock. > > > > This problem seems to have been present since the modularization of the > > HD-audio driver in commit 1289e9e8b42f ("ALSA: hda - Modularize HD-audio > > driver"). On Intel platforms this has been worked around by splitting up > > the probe sequence into a synchronous and an asynchronous part where the > > request_module() calls are asynchronous and hence avoid the deadlock. > > > > An alternative proposal is provided in this series of patches. Rather > > than relying on explicit request_module() calls to load kernel modules > > for HDA codec drivers, this implements a uevent callback for the HDA bus > > to advertises the MODALIAS information to the userspace helper. > > > > Effectively this results in the same modules being loaded, but it uses > > the more canonical infrastructure to perform this. Deferring the module > > loading to userspace removes the need for the explicit request_module() > > calls and works around the recursive locking issue because both drivers > > will be bound from separate contexts. > > While this looks definitely like the right direction to go, I'm afraid > that this will give a few major regressions. First off, there is no > way to bind with the generic codec driver. There are two generic > drivers, one for HDMI/DP and one for normal audio. Binding to them is > judged by parsing the codec widgets whether they are digital-only. > So, either user-space or kernel needs to parse the codec widgets > beforehand. If we rip off all binding magic as in your patch, this > has to be done by udev. With the sysfs stuff, now it should be > possible, but this would break the existing system. > > Another possible regression is the matching with the vendor-only > alias. Maybe the current wildcard works, but we need to double > check. > > So, unless these are addressed, I think we need another quick band-aid > over snd-hda-tegra just doing the async probe like snd-hda-intel. Does the patch below work? I only did a quick compile test. thanks, Takashi -- 8< -- From: Takashi Iwai Subject: [PATCH] ALSA: hda/tegra - async probe for avoiding module loading deadlock The Tegra HD-audio controller driver causes deadlocks when loaded as a module since the driver invokes request_module() at binding with the codec driver. This patch works around it by deferring the probe in a work like Intel HD-audio controller driver does. Although hovering the codec probe stuff into udev would be a better solution, it may cause other regressions, so let's try this band-aid fix until the more proper solution gets landed. Reported-by: Thierry Reding Cc: Signed-off-by: Takashi Iwai Tested-by: Thierry Reding --- sound/pci/hda/hda_tegra.c | 30 +++++++++++++++++++++++++----- 1 file changed, 25 insertions(+), 5 deletions(-) diff --git a/sound/pci/hda/hda_tegra.c b/sound/pci/hda/hda_tegra.c index 477742cb70a2..58c0aad37284 100644 --- a/sound/pci/hda/hda_tegra.c +++ b/sound/pci/hda/hda_tegra.c @@ -73,6 +73,7 @@ struct hda_tegra { struct clk *hda2codec_2x_clk; struct clk *hda2hdmi_clk; void __iomem *regs; + struct work_struct probe_work; }; #ifdef CONFIG_PM @@ -294,7 +295,9 @@ static int hda_tegra_dev_disconnect(struct snd_device *device) static int hda_tegra_dev_free(struct snd_device *device) { struct azx *chip = device->device_data; + struct hda_tegra *hda = container_of(chip, struct hda_tegra, chip); + cancel_work_sync(&hda->probe_work); if (azx_bus(chip)->chip_init) { azx_stop_all_streams(chip); azx_stop_chip(chip); @@ -426,6 +429,9 @@ static int hda_tegra_first_init(struct azx *chip, struct platform_device *pdev) /* * constructor */ + +static void hda_tegra_probe_work(struct work_struct *work); + static int hda_tegra_create(struct snd_card *card, unsigned int driver_caps, struct hda_tegra *hda) @@ -452,6 +458,8 @@ static int hda_tegra_create(struct snd_card *card, chip->single_cmd = false; chip->snoop = true; + INIT_WORK(&hda->probe_work, hda_tegra_probe_work); + err = azx_bus_init(chip, NULL, &hda_tegra_io_ops); if (err < 0) return err; @@ -499,6 +507,21 @@ static int hda_tegra_probe(struct platform_device *pdev) card->private_data = chip; dev_set_drvdata(&pdev->dev, card); + schedule_work(&hda->probe_work); + + return 0; + +out_free: + snd_card_free(card); + return err; +} + +static void hda_tegra_probe_work(struct work_struct *work) +{ + struct hda_tegra *hda = container_of(work, struct hda_tegra, probe_work); + struct azx *chip = &hda->chip; + struct platform_device *pdev = to_platform_device(hda->dev); + int err; err = hda_tegra_first_init(chip, pdev); if (err < 0) @@ -520,11 +543,8 @@ static int hda_tegra_probe(struct platform_device *pdev) chip->running = 1; snd_hda_set_power_save(&chip->bus, power_save * 1000); - return 0; - -out_free: - snd_card_free(card); - return err; + out_free: + return; /* no error return from async probe */ } static int hda_tegra_remove(struct platform_device *pdev)