On Mon, 11 Jul 2016 08:45:20 +0200, b_lkasam@codeaurora.org wrote:

On 2016-07-08 11:23, Takashi Iwai wrote:

...

(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(&register_mutex);

hi Takashi, Thanks for the patch.

It is working fine after above patch.

Good to hear. I queued the fix now. It'll be likely included in 4.7-final.

thanks,

Takashi