From patchwork Fri Jul 8 05:53:21 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Takashi Iwai X-Patchwork-Id: 9220055 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 40E36607D9 for ; Fri, 8 Jul 2016 05:53:41 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 2DBE8283FE for ; Fri, 8 Jul 2016 05:53:41 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 20D9528452; Fri, 8 Jul 2016 05:53:41 +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=-1.9 required=2.0 tests=BAYES_00, RCVD_IN_DNSWL_NONE autolearn=ham version=3.3.1 Received: from alsa0.perex.cz (alsa0.perex.cz [77.48.224.243]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id E6EB6283FE for ; Fri, 8 Jul 2016 05:53:39 +0000 (UTC) Received: by alsa0.perex.cz (Postfix, from userid 1000) id AF97426698C; Fri, 8 Jul 2016 07:53:37 +0200 (CEST) Received: from alsa0.perex.cz (localhost [127.0.0.1]) by alsa0.perex.cz (Postfix) with ESMTP id 9CEEE265344; Fri, 8 Jul 2016 07:53:30 +0200 (CEST) X-Original-To: alsa-devel@alsa-project.org Delivered-To: alsa-devel@alsa-project.org Received: by alsa0.perex.cz (Postfix, from userid 1000) id AAFC626545F; Fri, 8 Jul 2016 07:53:28 +0200 (CEST) Received: from mx2.suse.de (mx2.suse.de [195.135.220.15]) by alsa0.perex.cz (Postfix) with ESMTP id 9DC9E260851 for ; Fri, 8 Jul 2016 07:53:21 +0200 (CEST) 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 3CC08AAAD; Fri, 8 Jul 2016 05:53:21 +0000 (UTC) Date: Fri, 08 Jul 2016 07:53:21 +0200 Message-ID: From: Takashi Iwai To: b_lkasam@codeaurora.org In-Reply-To: <075f5f6f919170ebb646d475a0e36f02@codeaurora.org> References: <075f5f6f919170ebb646d475a0e36f02@codeaurora.org> 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") Cc: alsa-devel@alsa-project.org Subject: Re: [alsa-devel] ALSA sound core device deinit crash X-BeenThere: alsa-devel@alsa-project.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: "Alsa-devel mailing list for ALSA developers - http://www.alsa-project.org" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: alsa-devel-bounces@alsa-project.org Sender: alsa-devel-bounces@alsa-project.org X-Virus-Scanned: ClamAV using ClamSMTP (Please don't drop Cc to ML) On Fri, 08 Jul 2016 07:35:37 +0200, b_lkasam@codeaurora.org wrote: > > On 2016-07-08 10:58, Takashi Iwai wrote: > > On Fri, 08 Jul 2016 07:19:05 +0200, > > b_lkasam@codeaurora.org wrote: > >> > >> Hi Alsa team, > >> There is kernel crash observed when soundcard register failure case as > >> below -> > >> > >> diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c old mode > >> 100644 new mode 100755 index 0495890..60a1eb0 > >> --- a/sound/soc/soc-core.c > >> +++ b/sound/soc/soc-core.c > >> @@ -1382,6 +1382,7 @@ static int soc_probe_link_dais(struct > >> snd_soc_card *card, int num, int order) > >> /* do machine specific initialization */ if (dai_link->init) { ret = > >> dai_link->init(rtd); > >> + ret = -ENODEV; // -> we can force error here to reproduce crash > >> easily. > >> if (ret < 0) { > >> dev_err(card->dev, "ASoC: failed to init %s: %d\n", dai_link->name, > >> ret); > >> > >> If sound card fails at initialize at above, it is crashing in > >> pcm_chmap_ctl_private_free(). > >> > >> <1>[ 40.646642] [01-01-2016 00:07:16 CPU:0x3] Unable to handle > >> kernel > >> paging request at virtual address ffffffc0da644b68 > >> <1>[ 40.646664] [01-01-2016 00:07:16 CPU:0x3] pgd = ffffffc001d28000 > >> <1>[ 40.646673] [01-01-2016 00:07:16 CPU:0x3] [ffffffc0da644b68] > >> *pgd=00000000857fc003, *pud=00000000857fc003, *pmd=000000017ddd8003, > >> *pte=00c000015a644793 > >> <0>[ 40.646697] [01-01-2016 00:07:16 CPU:0x3] Internal error: Oops: > >> 9600004f [#1] PREEMPT SMP > >> <6>[ 40.646708] [01-01-2016 00:07:16 CPU:0x3] Modules linked in: > >> brcm_bt_drv fm_drv brcm_hci_ldisc texfat(PO) > >> <6>[ 40.646735] [01-01-2016 00:07:16 CPU:0x3] CPU: 3 PID: 299 Comm: > >> kworker/u8:8 Tainted: P O 3.18.24-g41b2dda2-00002-gbe25a74 > >> #1 > >> <6>[ 40.646744] [01-01-2016 00:07:16 CPU:0x3] Hardware name: > >> Qualcomm > >> Technologies, Inc. MSM 8996 v3.x + PMI8996 MTP (DT) > >> <6>[ 40.646763] [01-01-2016 00:07:16 CPU:0x3] Workqueue: deferwq > >> deferred_probe_work_func > >> <6>[ 40.646773] [01-01-2016 00:07:16 CPU:0x3] task: ffffffc0e951c880 > >> ti: ffffffc03368c000 task.ti: ffffffc03368c000 > >> <6>[ 40.646787] [01-01-2016 00:07:16 CPU:0x3] PC is at > >> pcm_chmap_ctl_private_free+0x1c/0x2c > >> <6>[ 40.646798] [01-01-2016 00:07:16 CPU:0x3] LR is at > >> snd_ctl_free_one+0x20/0x34 > >> > >> > >> FIX: > >> > >> Can you look at the change below and share your comments on this? > >> diff --git a/sound/core/device.c b/sound/core/device.c old mode 100644 > >> new mode 100755 index 41bec30..eaffde1 > >> --- a/sound/core/device.c > >> +++ b/sound/core/device.c > >> @@ -219,6 +219,7 @@ void snd_device_free_all(struct snd_card *card) > >> > >> if (snd_BUG_ON(!card)) > >> return; > >> - list_for_each_entry_safe_reverse(dev, next, &card->devices, > >> list) > >> + list_for_each_entry_safe(dev, next, &card->devices, > >> list) > >> __snd_device_free(dev); } > >> > >> > >> Since control sound device has the lowest type value > >> (SNDRV_DEV_CONTROL), it will be the first entry linked in the > >> card->devices linked list head and will be the last one to be freed. > >> > >> This issue seems to be resolved by modifying the sequence the sound > >> devices in the card->devices list are freed as shown below (from > >> “prev” > >> direction to “next” direction) but I’m not sure if this is a right > >> approach from ALSA perspective. > > > > This doesn't look correct. The strange thing is that this error > > shouldn't happen no matter which free loop direction is. The chmap > > ctl should have been already removed by the disconnection before > > freeing. It means that either the disconnection isn't done properly > > or something else is missing. > > > > Could you give the full stack trace? It's important to know which > > code path triggers it. > > > > > > thanks, > > > > Takashi > > > Hi Takashi, > > Here is full stack trace --> > > > [Callstack] > : (struct snd_pcm_chmap *)info = 0xFFFFFFC0DA6A5200 > : (struct snd_pcm *)pcm = 0xFFFFFFC0DA644A80 //part of buddy page, > read-only > : info->stream = 0 > : snd_card_free() was called > > -012|pcm_chmap_ctl_private_free() > //info->pcm->streams[info->stream].chmap_kctl = NULL > -013|snd_ctl_free_one() > -014|snd_ctl_remove() > -015|snd_ctl_dev_free() > -016|__snd_device_free() > -017|snd_device_free_all() > -018|snd_card_do_free(inline) > -018|release_card_device() > -019|device_release() > -020|kobject_cleanup(inline) > -020|kobject_release() > -021|kobject_put() > -022|put_device() > -023|snd_card_free_when_closed() > -024|snd_card_free() > -025|snd_soc_instantiate_card(inline) -> //Here instantiate card > failed for some reason, then triggers snd_card_free > -025|snd_soc_register_card() > -026|msm8996_asoc_machine_probe() > -027|platform_drv_probe() > -028|really_probe(inline) > -028|driver_probe_device() > -029|__device_attach() > -030|bus_for_each_drv() > -031|device_attach() > -032|bus_probe_device() > -033|deferred_probe_work_func() > -034|static_key_count(inline) > -034|static_key_false(inline) > -034|trace_workqueue_execute_end(inline) > -034|process_one_work() > -035|worker_thread() > -036|kthread() > -037|ret_from_fork(asm) > ---|end of frame > > > ----------------------- > trigger point in soc-core.c > in API snd_soc_instantiate_card(), > > card_probe_error: > if (card->remove) > card->remove(card); > > snd_card_free(card->snd_card); -> this is > snd_card_free which internally leads to above function call. > ------------------------------------------ OK, thanks. So this is the case where it frees before registering, and indeed there is a bug in the PCM chmap code. It's freed at disconnection but the disconnect is called only when it was registered. Below is a quick fix. Give it a try. thanks, Takashi diff --git a/sound/core/control.c b/sound/core/control.c index 9ff081cd03f4..fb096cb20a80 100644 --- a/sound/core/control.c +++ b/sound/core/control.c @@ -160,6 +160,8 @@ void snd_ctl_notify(struct snd_card *card, unsigned int mask, if (snd_BUG_ON(!card || !id)) return; + if (card->shutdown) + return; read_lock(&card->ctl_files_rwlock); #if IS_ENABLED(CONFIG_SND_MIXER_OSS) card->mixer_oss_change_count++; diff --git a/sound/core/pcm.c b/sound/core/pcm.c index 308c9ecf73db..8e980aa678d0 100644 --- a/sound/core/pcm.c +++ b/sound/core/pcm.c @@ -849,6 +849,14 @@ int snd_pcm_new_internal(struct snd_card *card, const char *id, int device, } EXPORT_SYMBOL(snd_pcm_new_internal); +static void free_chmap(struct snd_pcm_str *pstr) +{ + if (pstr->chmap_kctl) { + snd_ctl_remove(pstr->pcm->card, pstr->chmap_kctl); + pstr->chmap_kctl = NULL; + } +} + static void snd_pcm_free_stream(struct snd_pcm_str * pstr) { struct snd_pcm_substream *substream, *substream_next; @@ -871,6 +879,7 @@ static void snd_pcm_free_stream(struct snd_pcm_str * pstr) kfree(setup); } #endif + free_chmap(pstr); if (pstr->substream_count) put_device(&pstr->dev); } @@ -1135,10 +1144,7 @@ static int snd_pcm_dev_disconnect(struct snd_device *device) for (cidx = 0; cidx < 2; cidx++) { if (!pcm->internal) snd_unregister_device(&pcm->streams[cidx].dev); - if (pcm->streams[cidx].chmap_kctl) { - snd_ctl_remove(pcm->card, pcm->streams[cidx].chmap_kctl); - pcm->streams[cidx].chmap_kctl = NULL; - } + free_chmap(&pcm->streams[cidx]); } mutex_unlock(&pcm->open_mutex); mutex_unlock(®ister_mutex);