Re: ALSA sound core device deinit crash

[b_lkasam@codeaurora.org]

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.

thanks
Kasam
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel