alsa-devel.alsa-project.org archive mirror
 help / color / mirror / Atom feed
* [alsa-devel] Is this bug ? dpcm_prune_paths()
@ 2019-10-08  6:53 Kuninori Morimoto
  2019-10-08 14:58 ` Pierre-Louis Bossart
  0 siblings, 1 reply; 3+ messages in thread
From: Kuninori Morimoto @ 2019-10-08  6:53 UTC (permalink / raw)
  To: Mark Brown; +Cc: Linux-ALSA


Hi ALSA ML.

I'm checking soc-pcm.c, and I noticed strange code at
dpcm_prune_paths().

	static int dpcm_prune_paths(...)
	{
		...
		/* Destroy any old FE <--> BE connections */
(1)		for_each_dpcm_be(fe, stream, dpcm) {
			unsigned int i;

 -			/* is there a valid CPU DAI widget for this BE */
 |			widget = dai_get_widget(dpcm->be->cpu_dai, stream);
 |
(A)			/* prune the BE if it's no longer in our active list */
 |			if (widget && widget_in_list(list, widget))
 |(a)				continue;
 -
 |			/* is there a valid CODEC DAI widget for this BE */
 |(2)			for_each_rtd_codec_dai(dpcm->be, i, dai) {
 |				widget = dai_get_widget(dai, stream);
(B)
 |				/* prune the BE if it's no longer in our active list */
 |				if (widget && widget_in_list(list, widget))
 |(b)					continue;
 -			}

 -			...
 |			dpcm->state = ...
(C)			dpcm->be->dpcm[stream].runtime_update = ...
 |			prune++;
 -		}
		...
	}

In my understanding, (A) part is for CPU, and (B) part is for Codec.
Guessing from (a), I think it want to skip setup (C) if CPU widget
exist and matches to list.
If so, I guess (b) is maybe bug ?
This continue is for (2) loop, thus, it is totally do nothing now.
But maybe it want to be continue for (1) loop ?

Thank you for your help !!
Best regards
---
Kuninori Morimoto
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [alsa-devel] Is this bug ? dpcm_prune_paths()
  2019-10-08  6:53 [alsa-devel] Is this bug ? dpcm_prune_paths() Kuninori Morimoto
@ 2019-10-08 14:58 ` Pierre-Louis Bossart
  2019-10-09  0:58   ` Kuninori Morimoto
  0 siblings, 1 reply; 3+ messages in thread
From: Pierre-Louis Bossart @ 2019-10-08 14:58 UTC (permalink / raw)
  To: Kuninori Morimoto, Mark Brown; +Cc: Linux-ALSA, Benoit Cousson



On 10/8/19 1:53 AM, Kuninori Morimoto wrote:
> 
> Hi ALSA ML.
> 
> I'm checking soc-pcm.c, and I noticed strange code at
> dpcm_prune_paths().
> 
> 	static int dpcm_prune_paths(...)
> 	{
> 		...
> 		/* Destroy any old FE <--> BE connections */
> (1)		for_each_dpcm_be(fe, stream, dpcm) {
> 			unsigned int i;
> 
>   -			/* is there a valid CPU DAI widget for this BE */
>   |			widget = dai_get_widget(dpcm->be->cpu_dai, stream);
>   |
> (A)			/* prune the BE if it's no longer in our active list */
>   |			if (widget && widget_in_list(list, widget))
>   |(a)				continue;
>   -
>   |			/* is there a valid CODEC DAI widget for this BE */
>   |(2)			for_each_rtd_codec_dai(dpcm->be, i, dai) {
>   |				widget = dai_get_widget(dai, stream);
> (B)
>   |				/* prune the BE if it's no longer in our active list */
>   |				if (widget && widget_in_list(list, widget))
>   |(b)					continue;
>   -			}
> 
>   -			...
>   |			dpcm->state = ...
> (C)			dpcm->be->dpcm[stream].runtime_update = ...
>   |			prune++;
>   -		}
> 		...
> 	}
> 
> In my understanding, (A) part is for CPU, and (B) part is for Codec.
> Guessing from (a), I think it want to skip setup (C) if CPU widget
> exist and matches to list.
> If so, I guess (b) is maybe bug ?
> This continue is for (2) loop, thus, it is totally do nothing now.
> But maybe it want to be continue for (1) loop ?

Nice catch. This looks like a problem added during the transition to 
multi-codec.

The code before 2e5894d73789 ('ASoC: pcm: Add support for DAI 
multicodec') was this:

		/* is there a valid CODEC DAI widget for this BE */
		widget = dai_get_widget(dpcm->be->codec_dai, stream);

		/* prune the BE if it's no longer in our active list */
		if (widget && widget_in_list(list, widget))
			continue;

and the addition of the loop for the codec seems incorrect

		/* is there a valid CODEC DAI widget for this BE */
		for (i = 0; i < dpcm->be->num_codecs; i++) {
			struct snd_soc_dai *dai = dpcm->be->codec_dais[i];
			widget = dai_get_widget(dai, stream);

			/* prune the BE if it's no longer in our active list */
			if (widget && widget_in_list(list, widget))
				continue;
		}

the continue was not meant to continue the for loop on num_codecs, but 
the outer loop for dpcm.



_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [alsa-devel] Is this bug ? dpcm_prune_paths()
  2019-10-08 14:58 ` Pierre-Louis Bossart
@ 2019-10-09  0:58   ` Kuninori Morimoto
  0 siblings, 0 replies; 3+ messages in thread
From: Kuninori Morimoto @ 2019-10-09  0:58 UTC (permalink / raw)
  To: Pierre-Louis Bossart; +Cc: Linux-ALSA, Mark Brown, Benoit Cousson


Hi Pierre-Louis

> > 	static int dpcm_prune_paths(...)
> > 	{
> > 		...
> > 		/* Destroy any old FE <--> BE connections */
> > (1)		for_each_dpcm_be(fe, stream, dpcm) {
> > 			unsigned int i;
> > 
> >   -			/* is there a valid CPU DAI widget for this BE */
> >   |			widget = dai_get_widget(dpcm->be->cpu_dai, stream);
> >   |
> > (A)			/* prune the BE if it's no longer in our active list */
> >   |			if (widget && widget_in_list(list, widget))
> >   |(a)				continue;
> >   -
> >   |			/* is there a valid CODEC DAI widget for this BE */
> >   |(2)			for_each_rtd_codec_dai(dpcm->be, i, dai) {
> >   |				widget = dai_get_widget(dai, stream);
> > (B)
> >   |				/* prune the BE if it's no longer in our active list */
> >   |				if (widget && widget_in_list(list, widget))
> >   |(b)					continue;
> >   -			}
> > 
> >   -			...
> >   |			dpcm->state = ...
> > (C)			dpcm->be->dpcm[stream].runtime_update = ...
> >   |			prune++;
> >   -		}
> > 		...
> > 	}
(snip)
> Nice catch. This looks like a problem added during the transition to
> multi-codec.
(snip)
> the continue was not meant to continue the for loop on num_codecs, but
> the outer loop for dpcm.

Thank for checking !!
OK, it is bug.
I will post patch for it.

Thank you for your help !!
Best regards
---
Kuninori Morimoto
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2019-10-09  0:59 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-10-08  6:53 [alsa-devel] Is this bug ? dpcm_prune_paths() Kuninori Morimoto
2019-10-08 14:58 ` Pierre-Louis Bossart
2019-10-09  0:58   ` Kuninori Morimoto

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).