On Mon, 11 Jul 2016 08:45:20 +0200,
b_lkasam@xxxxxxxxxxxxxx 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@xxxxxxxxxxxxxx wrote:
> >>
> >> On 2016-07-08 10:58, Takashi Iwai wrote:
> >> > On Fri, 08 Jul 2016 07:19:05 +0200,
> >> > b_lkasam@xxxxxxxxxxxxxx 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);
>
>
> 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
_______________________________________________
Alsa-devel mailing list
Alsa-devel@xxxxxxxxxxxxxxxx
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
