linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Stephan Gerhold <stephan@gerhold.net>
To: Mark Brown <broonie@kernel.org>
Cc: Sameer Pujar <spujar@nvidia.com>,
	tiwai@suse.com, perex@perex.cz, alsa-devel@alsa-project.org,
	linux-kernel@vger.kernel.org,
	Jerome Brunet <jbrunet@baylibre.com>
Subject: Re: [alsa-devel] [RFC] ASoC: soc-pcm: crash in snd_soc_dapm_new_dai
Date: Mon, 17 Feb 2020 18:19:47 +0100	[thread overview]
Message-ID: <20200217171947.GA24287@gerhold.net> (raw)
In-Reply-To: <20200217171245.GA881@gerhold.net>

On Mon, Feb 17, 2020 at 06:12:53PM +0100, Stephan Gerhold wrote:
> On Mon, Feb 17, 2020 at 03:43:01PM +0000, Mark Brown wrote:
> > On Mon, Feb 17, 2020 at 03:41:20PM +0100, Stephan Gerhold wrote:
> > 
> > > I'm a bit confused about this patch, isn't SNDRV_PCM_STREAM_PLAYBACK
> > > used for both cpu_dai and codec_dai in the playback case?
> > 
> > It is in the normal case, but with a CODEC<->CODEC link (which was what
> > this was targeting) we need to bodge things by swapping playback and
> > capture on one end of the link.
> 
> I see. Looking at the code again I'm guessing the cause of the crash
> "fixed" by this patch is commit a342031cdd08 ("ASoC: create pcm for
> codec2codec links as well") where the codec2codec case was sort of
> patched in. This is what we had before this patch:
> 
> 		/* Adapt stream for codec2codec links */
> 		struct snd_soc_pcm_stream *cpu_capture = rtd->dai_link->params ?
> 			&cpu_dai->driver->playback : &cpu_dai->driver->capture;
> 		struct snd_soc_pcm_stream *cpu_playback = rtd->dai_link->params ?
> 			&cpu_dai->driver->capture : &cpu_dai->driver->playback;
> 
> This does the swapping you mentioned, so I guess rtd->dai_link->params
> is only set for the codec2codec case?
> 
> 		for_each_rtd_codec_dai(rtd, i, codec_dai) {
> 			if (snd_soc_dai_stream_valid(codec_dai, SNDRV_PCM_STREAM_PLAYBACK) &&
> 			    snd_soc_dai_stream_valid(cpu_dai,   SNDRV_PCM_STREAM_PLAYBACK))
> 				playback = 1;
> 			if (snd_soc_dai_stream_valid(codec_dai, SNDRV_PCM_STREAM_CAPTURE) &&
> 			    snd_soc_dai_stream_valid(cpu_dai,   SNDRV_PCM_STREAM_CAPTURE))
> 				capture = 1;
> 		}
> 
> 		capture = capture && cpu_capture->channels_min;
> 		playback = playback && cpu_playback->channels_min;
> 
> And this does a part of the check in snd_soc_dai_stream_valid(),
> but without the NULL check of cpu_capture/cpu_playback.
> (Maybe that is the cause of the crash.)

Uh, no, I am completely wrong here. :)
cpu_capture/cpu_playback cannot actually be NULL...
I should have looked more carefully at snd_soc_dai_stream_valid()...

But I still wonder if the approach below would be easier?

> 
> From my limited understanding, I would say that a much simpler way to
> implement this would be:
> 
> 	/* Adapt stream for codec2codec links */
> 	int cpu_capture = rtd->dai_link->params ?
> 		SNDRV_PCM_STREAM_PLAYBACK : SNDRV_PCM_STREAM_CAPTURE;
> 	int cpu_playback = rtd->dai_link->params ?
> 		SNDRV_PCM_STREAM_CAPTURE : SNDRV_PCM_STREAM_PLAYBACK;
> 
> 	for_each_rtd_codec_dai(rtd, i, codec_dai) {
> 		if (snd_soc_dai_stream_valid(codec_dai, SNDRV_PCM_STREAM_PLAYBACK) &&
> 		    snd_soc_dai_stream_valid(cpu_dai,   cpu_playback))
> 			playback = 1;
> 		if (snd_soc_dai_stream_valid(codec_dai, SNDRV_PCM_STREAM_CAPTURE) &&
> 		    snd_soc_dai_stream_valid(cpu_dai,   cpu_capture))
> 			capture = 1;
> 	}
> 
> since snd_soc_dai_stream_valid() does both the NULL-check and the 
> "channels_min" check.
> 
> But I'm really not familar with the codec2codec case and am unable to
> test it :) What do you think?
> 
> Thanks,
> Stephan

  reply	other threads:[~2020-02-17 17:20 UTC|newest]

Thread overview: 8+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-01-19 14:19 [RFC] ASoC: soc-pcm: crash in snd_soc_dapm_new_dai Sameer Pujar
2020-01-21 17:28 ` Applied "ASoC: soc-pcm: crash in snd_soc_dapm_new_dai" to the asoc tree Mark Brown
2020-01-23  9:55 ` [RFC] ASoC: soc-pcm: crash in snd_soc_dapm_new_dai Sameer Pujar
2020-02-17 14:41 ` [alsa-devel] " Stephan Gerhold
2020-02-17 15:43   ` Mark Brown
2020-02-17 17:12     ` Stephan Gerhold
2020-02-17 17:19       ` Stephan Gerhold [this message]
2020-02-17 17:30       ` Mark Brown

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20200217171947.GA24287@gerhold.net \
    --to=stephan@gerhold.net \
    --cc=alsa-devel@alsa-project.org \
    --cc=broonie@kernel.org \
    --cc=jbrunet@baylibre.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=perex@perex.cz \
    --cc=spujar@nvidia.com \
    --cc=tiwai@suse.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
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).