All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 1/2] ASoC: compress: Correct handling of compressed ops
@ 2018-01-24 13:55 Charles Keepax
  2018-01-24 13:55 ` [PATCH 2/2] ASoC: compress: Fixup some minor style issues Charles Keepax
                   ` (2 more replies)
  0 siblings, 3 replies; 16+ messages in thread
From: Charles Keepax @ 2018-01-24 13:55 UTC (permalink / raw)
  To: broonie
  Cc: vinod.koul, alsa-devel, dan.carpenter, lgirdwood, kuninori.morimoto.gx

The soc_compr_copy callback is currently broken. Since the
changes to move the compr_ops over to the component the return
value is not correctly propagated, always returning zero on
success rather than the number of bytes copied. This causes
user-space to stall continuously reading as it does not believe
it has received any data.

Furthermore, the changes to move the compr_ops over to the
component iterate through the list of components and will
call the compressed operation for any that have compressed
ops. However, there would be signifcantly more work required
to support such a system. There isn't currently any sensible
mechanism to forward the ops to multiple components. For example
what should be done about merging data from copy?  Or what should
be returned through the pointer call, each component may have
different amounts of data available, but user-space expects a
single answer?

As such clean up the code a little by factoring out the search
for the relevant component and bring things back to looking for
a single component that supports compressed ops on a single
runtime. These refactorings also put back the original handling
for the copy callback, correcting the return value.

Fixes: 9e7e3738ab0e ("ASoC: snd_soc_component_driver has snd_compr_ops")
Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com>
---

These refactorings should also fix the static analysis warnings
that Dan Carpenter was seeing [1]. Which were being caused
due to the loops that could "in theory" call the free on more
than one component.

[1] http://mailman.alsa-project.org/pipermail/alsa-devel/2018-January/130822.html

 sound/soc/soc-compress.c | 587 ++++++++++-------------------------------------
 1 file changed, 118 insertions(+), 469 deletions(-)

diff --git a/sound/soc/soc-compress.c b/sound/soc/soc-compress.c
index 81232f4ab614..34834c02dda8 100644
--- a/sound/soc/soc-compress.c
+++ b/sound/soc/soc-compress.c
@@ -26,14 +26,42 @@
 #include <sound/initval.h>
 #include <sound/soc-dpcm.h>
 
-static int soc_compr_open(struct snd_compr_stream *cstream)
+static struct snd_soc_component *
+soc_compr_get_component(struct snd_soc_pcm_runtime *rtd)
 {
-	struct snd_soc_pcm_runtime *rtd = cstream->private_data;
 	struct snd_soc_platform *platform = rtd->platform;
-	struct snd_soc_component *component;
 	struct snd_soc_rtdcom_list *rtdcom;
+
+	if (platform)
+		return &platform->component;
+
+	for_each_rtdcom(rtd, rtdcom) {
+		if (rtdcom->component->driver->compr_ops)
+			return rtdcom->component;
+	}
+
+	return NULL;
+}
+
+static const struct snd_compr_ops *
+soc_compr_get_ops(struct snd_soc_pcm_runtime *rtd,
+		  struct snd_soc_component *component)
+{
+	struct snd_soc_platform *platform = rtd->platform;
+
+	if (platform)
+		return platform->driver->compr_ops;
+	else
+		return component->driver->compr_ops;
+}
+
+static int soc_compr_open(struct snd_compr_stream *cstream)
+{
+	struct snd_soc_pcm_runtime *rtd = cstream->private_data;
+	struct snd_soc_component *component = soc_compr_get_component(rtd);
+	const struct snd_compr_ops *compr_ops = soc_compr_get_ops(rtd, component);
 	struct snd_soc_dai *cpu_dai = rtd->cpu_dai;
-	int ret = 0, __ret;
+	int ret = 0;
 
 	mutex_lock_nested(&rtd->pcm_mutex, rtd->pcm_subclass);
 
@@ -46,36 +74,15 @@ static int soc_compr_open(struct snd_compr_stream *cstream)
 		}
 	}
 
-	if (platform && platform->driver->compr_ops && platform->driver->compr_ops->open) {
-		ret = platform->driver->compr_ops->open(cstream);
+	if (compr_ops && compr_ops->open) {
+		ret = compr_ops->open(cstream);
 		if (ret < 0) {
 			pr_err("compress asoc: can't open platform %s\n",
-				platform->component.name);
+				component->name);
 			goto plat_err;
 		}
 	}
 
-	for_each_rtdcom(rtd, rtdcom) {
-		component = rtdcom->component;
-
-		/* ignore duplication for now */
-		if (platform && (component == &platform->component))
-			continue;
-
-		if (!component->driver->compr_ops ||
-		    !component->driver->compr_ops->open)
-			continue;
-
-		__ret = component->driver->compr_ops->open(cstream);
-		if (__ret < 0) {
-			pr_err("compress asoc: can't open platform %s\n",
-			       component->name);
-			ret = __ret;
-		}
-	}
-	if (ret < 0)
-		goto machine_err;
-
 	if (rtd->dai_link->compr_ops && rtd->dai_link->compr_ops->startup) {
 		ret = rtd->dai_link->compr_ops->startup(cstream);
 		if (ret < 0) {
@@ -91,22 +98,8 @@ static int soc_compr_open(struct snd_compr_stream *cstream)
 	return 0;
 
 machine_err:
-	for_each_rtdcom(rtd, rtdcom) {
-		component = rtdcom->component;
-
-		/* ignore duplication for now */
-		if (platform && (component == &platform->component))
-			continue;
-
-		if (!component->driver->compr_ops ||
-		    !component->driver->compr_ops->free)
-			continue;
-
-		component->driver->compr_ops->free(cstream);
-	}
-
-	if (platform && platform->driver->compr_ops && platform->driver->compr_ops->free)
-		platform->driver->compr_ops->free(cstream);
+	if (compr_ops && compr_ops->free)
+		compr_ops->free(cstream);
 plat_err:
 	if (cpu_dai->driver->cops && cpu_dai->driver->cops->shutdown)
 		cpu_dai->driver->cops->shutdown(cstream, cpu_dai);
@@ -118,16 +111,15 @@ static int soc_compr_open(struct snd_compr_stream *cstream)
 static int soc_compr_open_fe(struct snd_compr_stream *cstream)
 {
 	struct snd_soc_pcm_runtime *fe = cstream->private_data;
+	struct snd_soc_component *component = soc_compr_get_component(fe);
+	const struct snd_compr_ops *compr_ops = soc_compr_get_ops(fe, component);
 	struct snd_pcm_substream *fe_substream =
 		 fe->pcm->streams[cstream->direction].substream;
-	struct snd_soc_platform *platform = fe->platform;
-	struct snd_soc_component *component;
-	struct snd_soc_rtdcom_list *rtdcom;
 	struct snd_soc_dai *cpu_dai = fe->cpu_dai;
 	struct snd_soc_dpcm *dpcm;
 	struct snd_soc_dapm_widget_list *list;
 	int stream;
-	int ret = 0, __ret;
+	int ret = 0;
 
 	if (cstream->direction == SND_COMPRESS_PLAYBACK)
 		stream = SNDRV_PCM_STREAM_PLAYBACK;
@@ -145,37 +137,15 @@ static int soc_compr_open_fe(struct snd_compr_stream *cstream)
 		}
 	}
 
-
-	if (platform && platform->driver->compr_ops && platform->driver->compr_ops->open) {
-		ret = platform->driver->compr_ops->open(cstream);
+	if (compr_ops && compr_ops->open) {
+		ret = compr_ops->open(cstream);
 		if (ret < 0) {
 			pr_err("compress asoc: can't open platform %s\n",
-				platform->component.name);
+				component->name);
 			goto plat_err;
 		}
 	}
 
-	for_each_rtdcom(fe, rtdcom) {
-		component = rtdcom->component;
-
-		/* ignore duplication for now */
-		if (platform && (component == &platform->component))
-			continue;
-
-		if (!component->driver->compr_ops ||
-		    !component->driver->compr_ops->open)
-			continue;
-
-		__ret = component->driver->compr_ops->open(cstream);
-		if (__ret < 0) {
-			pr_err("compress asoc: can't open platform %s\n",
-			       component->name);
-			ret = __ret;
-		}
-	}
-	if (ret < 0)
-		goto machine_err;
-
 	if (fe->dai_link->compr_ops && fe->dai_link->compr_ops->startup) {
 		ret = fe->dai_link->compr_ops->startup(cstream);
 		if (ret < 0) {
@@ -227,22 +197,8 @@ static int soc_compr_open_fe(struct snd_compr_stream *cstream)
 	if (fe->dai_link->compr_ops && fe->dai_link->compr_ops->shutdown)
 		fe->dai_link->compr_ops->shutdown(cstream);
 machine_err:
-	for_each_rtdcom(fe, rtdcom) {
-		component = rtdcom->component;
-
-		/* ignore duplication for now */
-		if (platform && (component == &platform->component))
-			continue;
-
-		if (!component->driver->compr_ops ||
-		    !component->driver->compr_ops->free)
-			continue;
-
-		component->driver->compr_ops->free(cstream);
-	}
-
-	if (platform && platform->driver->compr_ops && platform->driver->compr_ops->free)
-		platform->driver->compr_ops->free(cstream);
+	if (compr_ops && compr_ops->free)
+		compr_ops->free(cstream);
 plat_err:
 	if (cpu_dai->driver->cops && cpu_dai->driver->cops->shutdown)
 		cpu_dai->driver->cops->shutdown(cstream, cpu_dai);
@@ -283,9 +239,8 @@ static void close_delayed_work(struct work_struct *work)
 static int soc_compr_free(struct snd_compr_stream *cstream)
 {
 	struct snd_soc_pcm_runtime *rtd = cstream->private_data;
-	struct snd_soc_platform *platform = rtd->platform;
-	struct snd_soc_component *component;
-	struct snd_soc_rtdcom_list *rtdcom;
+	struct snd_soc_component *component = soc_compr_get_component(rtd);
+	const struct snd_compr_ops *compr_ops = soc_compr_get_ops(rtd, component);
 	struct snd_soc_dai *cpu_dai = rtd->cpu_dai;
 	struct snd_soc_dai *codec_dai = rtd->codec_dai;
 	int stream;
@@ -311,22 +266,8 @@ static int soc_compr_free(struct snd_compr_stream *cstream)
 	if (rtd->dai_link->compr_ops && rtd->dai_link->compr_ops->shutdown)
 		rtd->dai_link->compr_ops->shutdown(cstream);
 
-	for_each_rtdcom(rtd, rtdcom) {
-		component = rtdcom->component;
-
-		/* ignore duplication for now */
-		if (platform && (component == &platform->component))
-			continue;
-
-		if (!component->driver->compr_ops ||
-		    !component->driver->compr_ops->free)
-			continue;
-
-		component->driver->compr_ops->free(cstream);
-	}
-
-	if (platform && platform->driver->compr_ops && platform->driver->compr_ops->free)
-		platform->driver->compr_ops->free(cstream);
+	if (compr_ops && compr_ops->free)
+		compr_ops->free(cstream);
 
 	if (cpu_dai->driver->cops && cpu_dai->driver->cops->shutdown)
 		cpu_dai->driver->cops->shutdown(cstream, cpu_dai);
@@ -356,9 +297,8 @@ static int soc_compr_free(struct snd_compr_stream *cstream)
 static int soc_compr_free_fe(struct snd_compr_stream *cstream)
 {
 	struct snd_soc_pcm_runtime *fe = cstream->private_data;
-	struct snd_soc_platform *platform = fe->platform;
-	struct snd_soc_component *component;
-	struct snd_soc_rtdcom_list *rtdcom;
+	struct snd_soc_component *component = soc_compr_get_component(fe);
+	const struct snd_compr_ops *compr_ops = soc_compr_get_ops(fe, component);
 	struct snd_soc_dai *cpu_dai = fe->cpu_dai;
 	struct snd_soc_dpcm *dpcm;
 	int stream, ret;
@@ -396,22 +336,8 @@ static int soc_compr_free_fe(struct snd_compr_stream *cstream)
 	if (fe->dai_link->compr_ops && fe->dai_link->compr_ops->shutdown)
 		fe->dai_link->compr_ops->shutdown(cstream);
 
-	if (platform && platform->driver->compr_ops && platform->driver->compr_ops->free)
-		platform->driver->compr_ops->free(cstream);
-
-	for_each_rtdcom(fe, rtdcom) {
-		component = rtdcom->component;
-
-		/* ignore duplication for now */
-		if (platform && (component == &platform->component))
-			continue;
-
-		if (!component->driver->compr_ops ||
-		    !component->driver->compr_ops->free)
-			continue;
-
-		component->driver->compr_ops->free(cstream);
-	}
+	if (compr_ops && compr_ops->free)
+		compr_ops->free(cstream);
 
 	if (cpu_dai->driver->cops && cpu_dai->driver->cops->shutdown)
 		cpu_dai->driver->cops->shutdown(cstream, cpu_dai);
@@ -424,43 +350,23 @@ static int soc_compr_trigger(struct snd_compr_stream *cstream, int cmd)
 {
 
 	struct snd_soc_pcm_runtime *rtd = cstream->private_data;
-	struct snd_soc_platform *platform = rtd->platform;
-	struct snd_soc_component *component;
-	struct snd_soc_rtdcom_list *rtdcom;
+	struct snd_soc_component *component = soc_compr_get_component(rtd);
+	const struct snd_compr_ops *compr_ops = soc_compr_get_ops(rtd, component);
 	struct snd_soc_dai *codec_dai = rtd->codec_dai;
 	struct snd_soc_dai *cpu_dai = rtd->cpu_dai;
-	int ret = 0, __ret;
+	int ret = 0;
 
 	mutex_lock_nested(&rtd->pcm_mutex, rtd->pcm_subclass);
 
-	if (platform && platform->driver->compr_ops && platform->driver->compr_ops->trigger) {
-		ret = platform->driver->compr_ops->trigger(cstream, cmd);
+	if (compr_ops && compr_ops->trigger) {
+		ret = compr_ops->trigger(cstream, cmd);
 		if (ret < 0)
 			goto out;
 	}
 
-	for_each_rtdcom(rtd, rtdcom) {
-		component = rtdcom->component;
-
-		/* ignore duplication for now */
-		if (platform && (component == &platform->component))
-			continue;
-
-		if (!component->driver->compr_ops ||
-		    !component->driver->compr_ops->trigger)
-			continue;
-
-		__ret = component->driver->compr_ops->trigger(cstream, cmd);
-		if (__ret < 0)
-			ret = __ret;
-	}
-	if (ret < 0)
-		goto out;
-
 	if (cpu_dai->driver->cops && cpu_dai->driver->cops->trigger)
 		cpu_dai->driver->cops->trigger(cstream, cmd, cpu_dai);
 
-
 	switch (cmd) {
 	case SNDRV_PCM_TRIGGER_START:
 		snd_soc_dai_digital_mute(codec_dai, 0, cstream->direction);
@@ -478,37 +384,16 @@ static int soc_compr_trigger(struct snd_compr_stream *cstream, int cmd)
 static int soc_compr_trigger_fe(struct snd_compr_stream *cstream, int cmd)
 {
 	struct snd_soc_pcm_runtime *fe = cstream->private_data;
-	struct snd_soc_platform *platform = fe->platform;
-	struct snd_soc_component *component;
-	struct snd_soc_rtdcom_list *rtdcom;
+	struct snd_soc_component *component = soc_compr_get_component(fe);
+	const struct snd_compr_ops *compr_ops = soc_compr_get_ops(fe, component);
 	struct snd_soc_dai *cpu_dai = fe->cpu_dai;
-	int ret = 0, __ret, stream;
+	int ret = 0, stream;
 
 	if (cmd == SND_COMPR_TRIGGER_PARTIAL_DRAIN ||
 		cmd == SND_COMPR_TRIGGER_DRAIN) {
 
-		if (platform &&
-		    platform->driver->compr_ops &&
-		    platform->driver->compr_ops->trigger)
-			return platform->driver->compr_ops->trigger(cstream,
-								    cmd);
-
-		for_each_rtdcom(fe, rtdcom) {
-			component = rtdcom->component;
-
-			/* ignore duplication for now */
-			if (platform && (component == &platform->component))
-				continue;
-
-			if (!component->driver->compr_ops ||
-			    !component->driver->compr_ops->trigger)
-				continue;
-
-			__ret = component->driver->compr_ops->trigger(cstream, cmd);
-			if (__ret < 0)
-				ret = __ret;
-		}
-		return ret;
+		if (compr_ops && compr_ops->trigger)
+			return compr_ops->trigger(cstream, cmd);
 	}
 
 	if (cstream->direction == SND_COMPRESS_PLAYBACK)
@@ -525,30 +410,12 @@ static int soc_compr_trigger_fe(struct snd_compr_stream *cstream, int cmd)
 			goto out;
 	}
 
-	if (platform && platform->driver->compr_ops && platform->driver->compr_ops->trigger) {
-		ret = platform->driver->compr_ops->trigger(cstream, cmd);
+	if (compr_ops && compr_ops->trigger) {
+		ret = compr_ops->trigger(cstream, cmd);
 		if (ret < 0)
 			goto out;
 	}
 
-	for_each_rtdcom(fe, rtdcom) {
-		component = rtdcom->component;
-
-		/* ignore duplication for now */
-		if (platform && (component == &platform->component))
-			continue;
-
-		if (!component->driver->compr_ops ||
-		    !component->driver->compr_ops->trigger)
-			continue;
-
-		__ret = component->driver->compr_ops->trigger(cstream, cmd);
-		if (__ret < 0)
-			ret = __ret;
-	}
-	if (ret < 0)
-		goto out;
-
 	fe->dpcm[stream].runtime_update = SND_SOC_DPCM_UPDATE_FE;
 
 	ret = dpcm_be_dai_trigger(fe, stream, cmd);
@@ -578,11 +445,10 @@ static int soc_compr_set_params(struct snd_compr_stream *cstream,
 					struct snd_compr_params *params)
 {
 	struct snd_soc_pcm_runtime *rtd = cstream->private_data;
-	struct snd_soc_platform *platform = rtd->platform;
-	struct snd_soc_component *component;
-	struct snd_soc_rtdcom_list *rtdcom;
+	struct snd_soc_component *component = soc_compr_get_component(rtd);
+	const struct snd_compr_ops *compr_ops = soc_compr_get_ops(rtd, component);
 	struct snd_soc_dai *cpu_dai = rtd->cpu_dai;
-	int ret = 0, __ret;
+	int ret = 0;
 
 	mutex_lock_nested(&rtd->pcm_mutex, rtd->pcm_subclass);
 
@@ -598,30 +464,12 @@ static int soc_compr_set_params(struct snd_compr_stream *cstream,
 			goto err;
 	}
 
-	if (platform && platform->driver->compr_ops && platform->driver->compr_ops->set_params) {
-		ret = platform->driver->compr_ops->set_params(cstream, params);
+	if (compr_ops && compr_ops->set_params) {
+		ret = compr_ops->set_params(cstream, params);
 		if (ret < 0)
 			goto err;
 	}
 
-	for_each_rtdcom(rtd, rtdcom) {
-		component = rtdcom->component;
-
-		/* ignore duplication for now */
-		if (platform && (component == &platform->component))
-			continue;
-
-		if (!component->driver->compr_ops ||
-		    !component->driver->compr_ops->set_params)
-			continue;
-
-		__ret = component->driver->compr_ops->set_params(cstream, params);
-		if (__ret < 0)
-			ret = __ret;
-	}
-	if (ret < 0)
-		goto err;
-
 	if (rtd->dai_link->compr_ops && rtd->dai_link->compr_ops->set_params) {
 		ret = rtd->dai_link->compr_ops->set_params(cstream);
 		if (ret < 0)
@@ -654,11 +502,10 @@ static int soc_compr_set_params_fe(struct snd_compr_stream *cstream,
 	struct snd_soc_pcm_runtime *fe = cstream->private_data;
 	struct snd_pcm_substream *fe_substream =
 		 fe->pcm->streams[cstream->direction].substream;
-	struct snd_soc_platform *platform = fe->platform;
-	struct snd_soc_component *component;
-	struct snd_soc_rtdcom_list *rtdcom;
+	struct snd_soc_component *component = soc_compr_get_component(fe);
+	const struct snd_compr_ops *compr_ops = soc_compr_get_ops(fe, component);
 	struct snd_soc_dai *cpu_dai = fe->cpu_dai;
-	int ret = 0, __ret, stream;
+	int ret = 0, stream;
 
 	if (cstream->direction == SND_COMPRESS_PLAYBACK)
 		stream = SNDRV_PCM_STREAM_PLAYBACK;
@@ -673,30 +520,12 @@ static int soc_compr_set_params_fe(struct snd_compr_stream *cstream,
 			goto out;
 	}
 
-	if (platform && platform->driver->compr_ops && platform->driver->compr_ops->set_params) {
-		ret = platform->driver->compr_ops->set_params(cstream, params);
+	if (compr_ops && compr_ops->set_params) {
+		ret = compr_ops->set_params(cstream, params);
 		if (ret < 0)
 			goto out;
 	}
 
-	for_each_rtdcom(fe, rtdcom) {
-		component = rtdcom->component;
-
-		/* ignore duplication for now */
-		if (platform && (component == &platform->component))
-			continue;
-
-		if (!component->driver->compr_ops ||
-		    !component->driver->compr_ops->set_params)
-			continue;
-
-		__ret = component->driver->compr_ops->set_params(cstream, params);
-		if (__ret < 0)
-			ret = __ret;
-	}
-	if (ret < 0)
-		goto out;
-
 	if (fe->dai_link->compr_ops && fe->dai_link->compr_ops->set_params) {
 		ret = fe->dai_link->compr_ops->set_params(cstream);
 		if (ret < 0)
@@ -734,11 +563,10 @@ static int soc_compr_get_params(struct snd_compr_stream *cstream,
 					struct snd_codec *params)
 {
 	struct snd_soc_pcm_runtime *rtd = cstream->private_data;
-	struct snd_soc_platform *platform = rtd->platform;
-	struct snd_soc_component *component;
-	struct snd_soc_rtdcom_list *rtdcom;
+	struct snd_soc_component *component = soc_compr_get_component(rtd);
+	const struct snd_compr_ops *compr_ops = soc_compr_get_ops(rtd, component);
 	struct snd_soc_dai *cpu_dai = rtd->cpu_dai;
-	int ret = 0, __ret;
+	int ret = 0;
 
 	mutex_lock_nested(&rtd->pcm_mutex, rtd->pcm_subclass);
 
@@ -748,27 +576,8 @@ static int soc_compr_get_params(struct snd_compr_stream *cstream,
 			goto err;
 	}
 
-	if (platform && platform->driver->compr_ops && platform->driver->compr_ops->get_params) {
-		ret = platform->driver->compr_ops->get_params(cstream, params);
-		if (ret < 0)
-			goto err;
-	}
-
-	for_each_rtdcom(rtd, rtdcom) {
-		component = rtdcom->component;
-
-		/* ignore duplication for now */
-		if (platform && (component == &platform->component))
-			continue;
-
-		if (!component->driver->compr_ops ||
-		    !component->driver->compr_ops->get_params)
-			continue;
-
-		__ret = component->driver->compr_ops->get_params(cstream, params);
-		if (__ret < 0)
-			ret = __ret;
-	}
+	if (compr_ops && compr_ops->get_params)
+		ret = compr_ops->get_params(cstream, params);
 
 err:
 	mutex_unlock(&rtd->pcm_mutex);
@@ -779,36 +588,15 @@ static int soc_compr_get_caps(struct snd_compr_stream *cstream,
 				struct snd_compr_caps *caps)
 {
 	struct snd_soc_pcm_runtime *rtd = cstream->private_data;
-	struct snd_soc_platform *platform = rtd->platform;
-	struct snd_soc_component *component;
-	struct snd_soc_rtdcom_list *rtdcom;
-	int ret = 0, __ret;
+	struct snd_soc_component *component = soc_compr_get_component(rtd);
+	const struct snd_compr_ops *compr_ops = soc_compr_get_ops(rtd, component);
+	int ret = 0;
 
 	mutex_lock_nested(&rtd->pcm_mutex, rtd->pcm_subclass);
 
-	if (platform && platform->driver->compr_ops && platform->driver->compr_ops->get_caps) {
-		ret = platform->driver->compr_ops->get_caps(cstream, caps);
-		if (ret < 0)
-			goto err;
-	}
-
-	for_each_rtdcom(rtd, rtdcom) {
-		component = rtdcom->component;
-
-		/* ignore duplication for now */
-		if (platform && (component == &platform->component))
-			continue;
-
-		if (!component->driver->compr_ops ||
-		    !component->driver->compr_ops->get_caps)
-			continue;
+	if (compr_ops && compr_ops->get_caps)
+		ret = compr_ops->get_caps(cstream, caps);
 
-		__ret = component->driver->compr_ops->get_caps(cstream, caps);
-		if (__ret < 0)
-			ret = __ret;
-	}
-
-err:
 	mutex_unlock(&rtd->pcm_mutex);
 	return ret;
 }
@@ -817,36 +605,15 @@ static int soc_compr_get_codec_caps(struct snd_compr_stream *cstream,
 				struct snd_compr_codec_caps *codec)
 {
 	struct snd_soc_pcm_runtime *rtd = cstream->private_data;
-	struct snd_soc_platform *platform = rtd->platform;
-	struct snd_soc_component *component;
-	struct snd_soc_rtdcom_list *rtdcom;
-	int ret = 0, __ret;
+	struct snd_soc_component *component = soc_compr_get_component(rtd);
+	const struct snd_compr_ops *compr_ops = soc_compr_get_ops(rtd, component);
+	int ret = 0;
 
 	mutex_lock_nested(&rtd->pcm_mutex, rtd->pcm_subclass);
 
-	if (platform && platform->driver->compr_ops && platform->driver->compr_ops->get_codec_caps) {
-		ret = platform->driver->compr_ops->get_codec_caps(cstream, codec);
-		if (ret < 0)
-			goto err;
-	}
-
-	for_each_rtdcom(rtd, rtdcom) {
-		component = rtdcom->component;
+	if (compr_ops && compr_ops->get_codec_caps)
+		ret = compr_ops->get_codec_caps(cstream, codec);
 
-		/* ignore duplication for now */
-		if (platform && (component == &platform->component))
-			continue;
-
-		if (!component->driver->compr_ops ||
-		    !component->driver->compr_ops->get_codec_caps)
-			continue;
-
-		__ret = component->driver->compr_ops->get_codec_caps(cstream, codec);
-		if (__ret < 0)
-			ret = __ret;
-	}
-
-err:
 	mutex_unlock(&rtd->pcm_mutex);
 	return ret;
 }
@@ -854,11 +621,10 @@ static int soc_compr_get_codec_caps(struct snd_compr_stream *cstream,
 static int soc_compr_ack(struct snd_compr_stream *cstream, size_t bytes)
 {
 	struct snd_soc_pcm_runtime *rtd = cstream->private_data;
-	struct snd_soc_platform *platform = rtd->platform;
-	struct snd_soc_component *component;
-	struct snd_soc_rtdcom_list *rtdcom;
+	struct snd_soc_component *component = soc_compr_get_component(rtd);
+	const struct snd_compr_ops *compr_ops = soc_compr_get_ops(rtd, component);
 	struct snd_soc_dai *cpu_dai = rtd->cpu_dai;
-	int ret = 0, __ret;
+	int ret = 0;
 
 	mutex_lock_nested(&rtd->pcm_mutex, rtd->pcm_subclass);
 
@@ -868,27 +634,8 @@ static int soc_compr_ack(struct snd_compr_stream *cstream, size_t bytes)
 			goto err;
 	}
 
-	if (platform && platform->driver->compr_ops && platform->driver->compr_ops->ack) {
-		ret = platform->driver->compr_ops->ack(cstream, bytes);
-		if (ret < 0)
-			goto err;
-	}
-
-	for_each_rtdcom(rtd, rtdcom) {
-		component = rtdcom->component;
-
-		/* ignore duplication for now */
-		if (platform && (component == &platform->component))
-			continue;
-
-		if (!component->driver->compr_ops ||
-		    !component->driver->compr_ops->ack)
-			continue;
-
-		__ret = component->driver->compr_ops->ack(cstream, bytes);
-		if (__ret < 0)
-			ret = __ret;
-	}
+	if (compr_ops && compr_ops->ack)
+		ret = compr_ops->ack(cstream, bytes);
 
 err:
 	mutex_unlock(&rtd->pcm_mutex);
@@ -899,10 +646,9 @@ static int soc_compr_pointer(struct snd_compr_stream *cstream,
 			struct snd_compr_tstamp *tstamp)
 {
 	struct snd_soc_pcm_runtime *rtd = cstream->private_data;
-	struct snd_soc_platform *platform = rtd->platform;
-	struct snd_soc_component *component;
-	struct snd_soc_rtdcom_list *rtdcom;
-	int ret = 0, __ret;
+	struct snd_soc_component *component = soc_compr_get_component(rtd);
+	const struct snd_compr_ops *compr_ops = soc_compr_get_ops(rtd, component);
+	int ret = 0;
 	struct snd_soc_dai *cpu_dai = rtd->cpu_dai;
 
 	mutex_lock_nested(&rtd->pcm_mutex, rtd->pcm_subclass);
@@ -910,29 +656,9 @@ static int soc_compr_pointer(struct snd_compr_stream *cstream,
 	if (cpu_dai->driver->cops && cpu_dai->driver->cops->pointer)
 		cpu_dai->driver->cops->pointer(cstream, tstamp, cpu_dai);
 
-	if (platform && platform->driver->compr_ops && platform->driver->compr_ops->pointer) {
-		ret = platform->driver->compr_ops->pointer(cstream, tstamp);
-		if (ret < 0)
-			goto err;
-	}
-
-	for_each_rtdcom(rtd, rtdcom) {
-		component = rtdcom->component;
-
-		/* ignore duplication for now */
-		if (platform && (component == &platform->component))
-			continue;
-
-		if (!component->driver->compr_ops ||
-		    !component->driver->compr_ops->pointer)
-			continue;
+	if (compr_ops && compr_ops->pointer)
+		ret = compr_ops->pointer(cstream, tstamp);
 
-		__ret = component->driver->compr_ops->pointer(cstream, tstamp);
-		if (__ret < 0)
-			ret = __ret;
-	}
-
-err:
 	mutex_unlock(&rtd->pcm_mutex);
 	return ret;
 }
@@ -941,35 +667,15 @@ static int soc_compr_copy(struct snd_compr_stream *cstream,
 			  char __user *buf, size_t count)
 {
 	struct snd_soc_pcm_runtime *rtd = cstream->private_data;
-	struct snd_soc_platform *platform = rtd->platform;
-	struct snd_soc_component *component;
-	struct snd_soc_rtdcom_list *rtdcom;
-	int ret = 0, __ret;
+	struct snd_soc_component *component = soc_compr_get_component(rtd);
+	const struct snd_compr_ops *compr_ops = soc_compr_get_ops(rtd, component);
+	int ret = 0;
 
 	mutex_lock_nested(&rtd->pcm_mutex, rtd->pcm_subclass);
 
-	if (platform && platform->driver->compr_ops && platform->driver->compr_ops->copy) {
-		ret = platform->driver->compr_ops->copy(cstream, buf, count);
-		if (ret < 0)
-			goto err;
-	}
-
-	for_each_rtdcom(rtd, rtdcom) {
-		component = rtdcom->component;
-
-		/* ignore duplication for now */
-		if (platform && (component == &platform->component))
-			continue;
+	if (compr_ops && compr_ops->copy)
+		ret = compr_ops->copy(cstream, buf, count);
 
-		if (!component->driver->compr_ops ||
-		    !component->driver->compr_ops->copy)
-			continue;
-
-		__ret = component->driver->compr_ops->copy(cstream, buf, count);
-		if (__ret < 0)
-			ret = __ret;
-	}
-err:
 	mutex_unlock(&rtd->pcm_mutex);
 	return ret;
 }
@@ -978,11 +684,10 @@ static int soc_compr_set_metadata(struct snd_compr_stream *cstream,
 				struct snd_compr_metadata *metadata)
 {
 	struct snd_soc_pcm_runtime *rtd = cstream->private_data;
-	struct snd_soc_platform *platform = rtd->platform;
-	struct snd_soc_component *component;
-	struct snd_soc_rtdcom_list *rtdcom;
+	struct snd_soc_component *component = soc_compr_get_component(rtd);
+	const struct snd_compr_ops *compr_ops = soc_compr_get_ops(rtd, component);
 	struct snd_soc_dai *cpu_dai = rtd->cpu_dai;
-	int ret = 0, __ret;
+	int ret = 0;
 
 	if (cpu_dai->driver->cops && cpu_dai->driver->cops->set_metadata) {
 		ret = cpu_dai->driver->cops->set_metadata(cstream, metadata, cpu_dai);
@@ -990,27 +695,8 @@ static int soc_compr_set_metadata(struct snd_compr_stream *cstream,
 			return ret;
 	}
 
-	if (platform && platform->driver->compr_ops && platform->driver->compr_ops->set_metadata) {
-		ret = platform->driver->compr_ops->set_metadata(cstream, metadata);
-		if (ret < 0)
-			return ret;
-	}
-
-	for_each_rtdcom(rtd, rtdcom) {
-		component = rtdcom->component;
-
-		/* ignore duplication for now */
-		if (platform && (component == &platform->component))
-			continue;
-
-		if (!component->driver->compr_ops ||
-		    !component->driver->compr_ops->set_metadata)
-			continue;
-
-		__ret = component->driver->compr_ops->set_metadata(cstream, metadata);
-		if (__ret < 0)
-			ret = __ret;
-	}
+	if (compr_ops && compr_ops->set_metadata)
+		ret = compr_ops->set_metadata(cstream, metadata);
 
 	return ret;
 }
@@ -1019,11 +705,10 @@ static int soc_compr_get_metadata(struct snd_compr_stream *cstream,
 				struct snd_compr_metadata *metadata)
 {
 	struct snd_soc_pcm_runtime *rtd = cstream->private_data;
-	struct snd_soc_platform *platform = rtd->platform;
-	struct snd_soc_component *component;
-	struct snd_soc_rtdcom_list *rtdcom;
+	struct snd_soc_component *component = soc_compr_get_component(rtd);
+	const struct snd_compr_ops *compr_ops = soc_compr_get_ops(rtd, component);
 	struct snd_soc_dai *cpu_dai = rtd->cpu_dai;
-	int ret = 0, __ret;
+	int ret = 0;
 
 	if (cpu_dai->driver->cops && cpu_dai->driver->cops->get_metadata) {
 		ret = cpu_dai->driver->cops->get_metadata(cstream, metadata, cpu_dai);
@@ -1031,27 +716,8 @@ static int soc_compr_get_metadata(struct snd_compr_stream *cstream,
 			return ret;
 	}
 
-	if (platform && platform->driver->compr_ops && platform->driver->compr_ops->get_metadata) {
-		ret = platform->driver->compr_ops->get_metadata(cstream, metadata);
-		if (ret < 0)
-			return ret;
-	}
-
-	for_each_rtdcom(rtd, rtdcom) {
-		component = rtdcom->component;
-
-		/* ignore duplication for now */
-		if (platform && (component == &platform->component))
-			continue;
-
-		if (!component->driver->compr_ops ||
-		    !component->driver->compr_ops->get_metadata)
-			continue;
-
-		__ret = component->driver->compr_ops->get_metadata(cstream, metadata);
-		if (__ret < 0)
-			ret = __ret;
-	}
+	if (compr_ops && compr_ops->get_metadata)
+		ret = compr_ops->get_metadata(cstream, metadata);
 
 	return ret;
 }
@@ -1096,9 +762,8 @@ static struct snd_compr_ops soc_compr_dyn_ops = {
  */
 int snd_soc_new_compress(struct snd_soc_pcm_runtime *rtd, int num)
 {
-	struct snd_soc_platform *platform = rtd->platform;
-	struct snd_soc_component *component;
-	struct snd_soc_rtdcom_list *rtdcom;
+	struct snd_soc_component *component = soc_compr_get_component(rtd);
+	const struct snd_compr_ops *compr_ops = soc_compr_get_ops(rtd, component);
 	struct snd_soc_dai *codec_dai = rtd->codec_dai;
 	struct snd_soc_dai *cpu_dai = rtd->cpu_dai;
 	struct snd_compr *compr;
@@ -1174,25 +839,9 @@ int snd_soc_new_compress(struct snd_soc_pcm_runtime *rtd, int num)
 		memcpy(compr->ops, &soc_compr_ops, sizeof(soc_compr_ops));
 	}
 
-
 	/* Add copy callback for not memory mapped DSPs */
-	if (platform && platform->driver->compr_ops && platform->driver->compr_ops->copy)
-		compr->ops->copy = soc_compr_copy;
-
-	for_each_rtdcom(rtd, rtdcom) {
-		component = rtdcom->component;
-
-		/* ignore duplication for now */
-		if (platform && (component == &platform->component))
-			continue;
-
-		if (!component->driver->compr_ops ||
-		    !component->driver->compr_ops->copy)
-			continue;
-
+	if (compr_ops && compr_ops->copy)
 		compr->ops->copy = soc_compr_copy;
-	}
-
 
 	mutex_init(&compr->lock);
 	ret = snd_compress_new(rtd->card->snd_card, num, direction,
-- 
2.11.0

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

* [PATCH 2/2] ASoC: compress: Fixup some minor style issues
  2018-01-24 13:55 [PATCH 1/2] ASoC: compress: Correct handling of compressed ops Charles Keepax
@ 2018-01-24 13:55 ` Charles Keepax
  2018-01-24 14:33   ` Ladislav Michl
  2018-01-24 14:29 ` [PATCH 1/2] ASoC: compress: Correct handling of compressed ops Vinod Koul
  2018-01-25  1:18 ` Kuninori Morimoto
  2 siblings, 1 reply; 16+ messages in thread
From: Charles Keepax @ 2018-01-24 13:55 UTC (permalink / raw)
  To: broonie
  Cc: vinod.koul, alsa-devel, dan.carpenter, lgirdwood, kuninori.morimoto.gx

Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com>
---
 sound/soc/soc-compress.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/sound/soc/soc-compress.c b/sound/soc/soc-compress.c
index 34834c02dda8..7c1955e9533a 100644
--- a/sound/soc/soc-compress.c
+++ b/sound/soc/soc-compress.c
@@ -262,7 +262,6 @@ static int soc_compr_free(struct snd_compr_stream *cstream)
 	if (!codec_dai->active)
 		codec_dai->rate = 0;
 
-
 	if (rtd->dai_link->compr_ops && rtd->dai_link->compr_ops->shutdown)
 		rtd->dai_link->compr_ops->shutdown(cstream);
 
@@ -401,7 +400,6 @@ static int soc_compr_trigger_fe(struct snd_compr_stream *cstream, int cmd)
 	else
 		stream = SNDRV_PCM_STREAM_CAPTURE;
 
-
 	mutex_lock_nested(&fe->card->mutex, SND_SOC_CARD_CLASS_RUNTIME);
 
 	if (cpu_dai->driver->cops && cpu_dai->driver->cops->trigger) {
@@ -859,7 +857,7 @@ int snd_soc_new_compress(struct snd_soc_pcm_runtime *rtd, int num)
 	rtd->compr = compr;
 	compr->private_data = rtd;
 
-	printk(KERN_INFO "compress asoc: %s <-> %s mapping ok\n", codec_dai->name,
+	pr_info("compress asoc: %s <-> %s mapping ok\n", codec_dai->name,
 		cpu_dai->name);
 	return ret;
 
-- 
2.11.0

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

* Re: [PATCH 1/2] ASoC: compress: Correct handling of compressed ops
  2018-01-24 13:55 [PATCH 1/2] ASoC: compress: Correct handling of compressed ops Charles Keepax
  2018-01-24 13:55 ` [PATCH 2/2] ASoC: compress: Fixup some minor style issues Charles Keepax
@ 2018-01-24 14:29 ` Vinod Koul
  2018-01-24 14:49   ` Charles Keepax
  2018-01-25  1:18 ` Kuninori Morimoto
  2 siblings, 1 reply; 16+ messages in thread
From: Vinod Koul @ 2018-01-24 14:29 UTC (permalink / raw)
  To: Charles Keepax
  Cc: alsa-devel, dan.carpenter, broonie, lgirdwood, kuninori.morimoto.gx

On Wed, Jan 24, 2018 at 01:55:24PM +0000, Charles Keepax wrote:
> The soc_compr_copy callback is currently broken. Since the
> changes to move the compr_ops over to the component the return
> value is not correctly propagated, always returning zero on
> success rather than the number of bytes copied. This causes
> user-space to stall continuously reading as it does not believe
> it has received any data.
> 
> Furthermore, the changes to move the compr_ops over to the
> component iterate through the list of components and will
> call the compressed operation for any that have compressed
> ops. However, there would be signifcantly more work required
> to support such a system. There isn't currently any sensible
> mechanism to forward the ops to multiple components. For example
> what should be done about merging data from copy?  Or what should
> be returned through the pointer call, each component may have
> different amounts of data available, but user-space expects a
> single answer?
> 
> As such clean up the code a little by factoring out the search
> for the relevant component and bring things back to looking for
> a single component that supports compressed ops on a single
> runtime. These refactorings also put back the original handling
> for the copy callback, correcting the return value.

Thank you Charles for spotting this and fixing it. The changes look good to
me. But should we split the copy fix from rest of the handling and backport
that one to stable?

> 
> Fixes: 9e7e3738ab0e ("ASoC: snd_soc_component_driver has snd_compr_ops")
> Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com>
> ---
> 
> These refactorings should also fix the static analysis warnings
> that Dan Carpenter was seeing [1]. Which were being caused
> due to the loops that could "in theory" call the free on more
> than one component.
> 
> [1] http://mailman.alsa-project.org/pipermail/alsa-devel/2018-January/130822.html
> 
>  sound/soc/soc-compress.c | 587 ++++++++++-------------------------------------
>  1 file changed, 118 insertions(+), 469 deletions(-)
> 
> diff --git a/sound/soc/soc-compress.c b/sound/soc/soc-compress.c
> index 81232f4ab614..34834c02dda8 100644
> --- a/sound/soc/soc-compress.c
> +++ b/sound/soc/soc-compress.c
> @@ -26,14 +26,42 @@
>  #include <sound/initval.h>
>  #include <sound/soc-dpcm.h>
>  
> -static int soc_compr_open(struct snd_compr_stream *cstream)
> +static struct snd_soc_component *
> +soc_compr_get_component(struct snd_soc_pcm_runtime *rtd)
>  {
> -	struct snd_soc_pcm_runtime *rtd = cstream->private_data;
>  	struct snd_soc_platform *platform = rtd->platform;
> -	struct snd_soc_component *component;
>  	struct snd_soc_rtdcom_list *rtdcom;
> +
> +	if (platform)
> +		return &platform->component;
> +
> +	for_each_rtdcom(rtd, rtdcom) {
> +		if (rtdcom->component->driver->compr_ops)
> +			return rtdcom->component;
> +	}
> +
> +	return NULL;
> +}
> +
> +static const struct snd_compr_ops *
> +soc_compr_get_ops(struct snd_soc_pcm_runtime *rtd,
> +		  struct snd_soc_component *component)
> +{
> +	struct snd_soc_platform *platform = rtd->platform;
> +
> +	if (platform)
> +		return platform->driver->compr_ops;
> +	else
> +		return component->driver->compr_ops;
> +}
> +
> +static int soc_compr_open(struct snd_compr_stream *cstream)
> +{
> +	struct snd_soc_pcm_runtime *rtd = cstream->private_data;
> +	struct snd_soc_component *component = soc_compr_get_component(rtd);
> +	const struct snd_compr_ops *compr_ops = soc_compr_get_ops(rtd, component);
>  	struct snd_soc_dai *cpu_dai = rtd->cpu_dai;
> -	int ret = 0, __ret;
> +	int ret = 0;
>  
>  	mutex_lock_nested(&rtd->pcm_mutex, rtd->pcm_subclass);
>  
> @@ -46,36 +74,15 @@ static int soc_compr_open(struct snd_compr_stream *cstream)
>  		}
>  	}
>  
> -	if (platform && platform->driver->compr_ops && platform->driver->compr_ops->open) {
> -		ret = platform->driver->compr_ops->open(cstream);
> +	if (compr_ops && compr_ops->open) {
> +		ret = compr_ops->open(cstream);
>  		if (ret < 0) {
>  			pr_err("compress asoc: can't open platform %s\n",
> -				platform->component.name);
> +				component->name);
>  			goto plat_err;
>  		}
>  	}
>  
> -	for_each_rtdcom(rtd, rtdcom) {
> -		component = rtdcom->component;
> -
> -		/* ignore duplication for now */
> -		if (platform && (component == &platform->component))
> -			continue;
> -
> -		if (!component->driver->compr_ops ||
> -		    !component->driver->compr_ops->open)
> -			continue;
> -
> -		__ret = component->driver->compr_ops->open(cstream);
> -		if (__ret < 0) {
> -			pr_err("compress asoc: can't open platform %s\n",
> -			       component->name);
> -			ret = __ret;
> -		}
> -	}
> -	if (ret < 0)
> -		goto machine_err;
> -
>  	if (rtd->dai_link->compr_ops && rtd->dai_link->compr_ops->startup) {
>  		ret = rtd->dai_link->compr_ops->startup(cstream);
>  		if (ret < 0) {
> @@ -91,22 +98,8 @@ static int soc_compr_open(struct snd_compr_stream *cstream)
>  	return 0;
>  
>  machine_err:
> -	for_each_rtdcom(rtd, rtdcom) {
> -		component = rtdcom->component;
> -
> -		/* ignore duplication for now */
> -		if (platform && (component == &platform->component))
> -			continue;
> -
> -		if (!component->driver->compr_ops ||
> -		    !component->driver->compr_ops->free)
> -			continue;
> -
> -		component->driver->compr_ops->free(cstream);
> -	}
> -
> -	if (platform && platform->driver->compr_ops && platform->driver->compr_ops->free)
> -		platform->driver->compr_ops->free(cstream);
> +	if (compr_ops && compr_ops->free)
> +		compr_ops->free(cstream);
>  plat_err:
>  	if (cpu_dai->driver->cops && cpu_dai->driver->cops->shutdown)
>  		cpu_dai->driver->cops->shutdown(cstream, cpu_dai);
> @@ -118,16 +111,15 @@ static int soc_compr_open(struct snd_compr_stream *cstream)
>  static int soc_compr_open_fe(struct snd_compr_stream *cstream)
>  {
>  	struct snd_soc_pcm_runtime *fe = cstream->private_data;
> +	struct snd_soc_component *component = soc_compr_get_component(fe);
> +	const struct snd_compr_ops *compr_ops = soc_compr_get_ops(fe, component);
>  	struct snd_pcm_substream *fe_substream =
>  		 fe->pcm->streams[cstream->direction].substream;
> -	struct snd_soc_platform *platform = fe->platform;
> -	struct snd_soc_component *component;
> -	struct snd_soc_rtdcom_list *rtdcom;
>  	struct snd_soc_dai *cpu_dai = fe->cpu_dai;
>  	struct snd_soc_dpcm *dpcm;
>  	struct snd_soc_dapm_widget_list *list;
>  	int stream;
> -	int ret = 0, __ret;
> +	int ret = 0;
>  
>  	if (cstream->direction == SND_COMPRESS_PLAYBACK)
>  		stream = SNDRV_PCM_STREAM_PLAYBACK;
> @@ -145,37 +137,15 @@ static int soc_compr_open_fe(struct snd_compr_stream *cstream)
>  		}
>  	}
>  
> -
> -	if (platform && platform->driver->compr_ops && platform->driver->compr_ops->open) {
> -		ret = platform->driver->compr_ops->open(cstream);
> +	if (compr_ops && compr_ops->open) {
> +		ret = compr_ops->open(cstream);
>  		if (ret < 0) {
>  			pr_err("compress asoc: can't open platform %s\n",
> -				platform->component.name);
> +				component->name);
>  			goto plat_err;
>  		}
>  	}
>  
> -	for_each_rtdcom(fe, rtdcom) {
> -		component = rtdcom->component;
> -
> -		/* ignore duplication for now */
> -		if (platform && (component == &platform->component))
> -			continue;
> -
> -		if (!component->driver->compr_ops ||
> -		    !component->driver->compr_ops->open)
> -			continue;
> -
> -		__ret = component->driver->compr_ops->open(cstream);
> -		if (__ret < 0) {
> -			pr_err("compress asoc: can't open platform %s\n",
> -			       component->name);
> -			ret = __ret;
> -		}
> -	}
> -	if (ret < 0)
> -		goto machine_err;
> -
>  	if (fe->dai_link->compr_ops && fe->dai_link->compr_ops->startup) {
>  		ret = fe->dai_link->compr_ops->startup(cstream);
>  		if (ret < 0) {
> @@ -227,22 +197,8 @@ static int soc_compr_open_fe(struct snd_compr_stream *cstream)
>  	if (fe->dai_link->compr_ops && fe->dai_link->compr_ops->shutdown)
>  		fe->dai_link->compr_ops->shutdown(cstream);
>  machine_err:
> -	for_each_rtdcom(fe, rtdcom) {
> -		component = rtdcom->component;
> -
> -		/* ignore duplication for now */
> -		if (platform && (component == &platform->component))
> -			continue;
> -
> -		if (!component->driver->compr_ops ||
> -		    !component->driver->compr_ops->free)
> -			continue;
> -
> -		component->driver->compr_ops->free(cstream);
> -	}
> -
> -	if (platform && platform->driver->compr_ops && platform->driver->compr_ops->free)
> -		platform->driver->compr_ops->free(cstream);
> +	if (compr_ops && compr_ops->free)
> +		compr_ops->free(cstream);
>  plat_err:
>  	if (cpu_dai->driver->cops && cpu_dai->driver->cops->shutdown)
>  		cpu_dai->driver->cops->shutdown(cstream, cpu_dai);
> @@ -283,9 +239,8 @@ static void close_delayed_work(struct work_struct *work)
>  static int soc_compr_free(struct snd_compr_stream *cstream)
>  {
>  	struct snd_soc_pcm_runtime *rtd = cstream->private_data;
> -	struct snd_soc_platform *platform = rtd->platform;
> -	struct snd_soc_component *component;
> -	struct snd_soc_rtdcom_list *rtdcom;
> +	struct snd_soc_component *component = soc_compr_get_component(rtd);
> +	const struct snd_compr_ops *compr_ops = soc_compr_get_ops(rtd, component);
>  	struct snd_soc_dai *cpu_dai = rtd->cpu_dai;
>  	struct snd_soc_dai *codec_dai = rtd->codec_dai;
>  	int stream;
> @@ -311,22 +266,8 @@ static int soc_compr_free(struct snd_compr_stream *cstream)
>  	if (rtd->dai_link->compr_ops && rtd->dai_link->compr_ops->shutdown)
>  		rtd->dai_link->compr_ops->shutdown(cstream);
>  
> -	for_each_rtdcom(rtd, rtdcom) {
> -		component = rtdcom->component;
> -
> -		/* ignore duplication for now */
> -		if (platform && (component == &platform->component))
> -			continue;
> -
> -		if (!component->driver->compr_ops ||
> -		    !component->driver->compr_ops->free)
> -			continue;
> -
> -		component->driver->compr_ops->free(cstream);
> -	}
> -
> -	if (platform && platform->driver->compr_ops && platform->driver->compr_ops->free)
> -		platform->driver->compr_ops->free(cstream);
> +	if (compr_ops && compr_ops->free)
> +		compr_ops->free(cstream);
>  
>  	if (cpu_dai->driver->cops && cpu_dai->driver->cops->shutdown)
>  		cpu_dai->driver->cops->shutdown(cstream, cpu_dai);
> @@ -356,9 +297,8 @@ static int soc_compr_free(struct snd_compr_stream *cstream)
>  static int soc_compr_free_fe(struct snd_compr_stream *cstream)
>  {
>  	struct snd_soc_pcm_runtime *fe = cstream->private_data;
> -	struct snd_soc_platform *platform = fe->platform;
> -	struct snd_soc_component *component;
> -	struct snd_soc_rtdcom_list *rtdcom;
> +	struct snd_soc_component *component = soc_compr_get_component(fe);
> +	const struct snd_compr_ops *compr_ops = soc_compr_get_ops(fe, component);
>  	struct snd_soc_dai *cpu_dai = fe->cpu_dai;
>  	struct snd_soc_dpcm *dpcm;
>  	int stream, ret;
> @@ -396,22 +336,8 @@ static int soc_compr_free_fe(struct snd_compr_stream *cstream)
>  	if (fe->dai_link->compr_ops && fe->dai_link->compr_ops->shutdown)
>  		fe->dai_link->compr_ops->shutdown(cstream);
>  
> -	if (platform && platform->driver->compr_ops && platform->driver->compr_ops->free)
> -		platform->driver->compr_ops->free(cstream);
> -
> -	for_each_rtdcom(fe, rtdcom) {
> -		component = rtdcom->component;
> -
> -		/* ignore duplication for now */
> -		if (platform && (component == &platform->component))
> -			continue;
> -
> -		if (!component->driver->compr_ops ||
> -		    !component->driver->compr_ops->free)
> -			continue;
> -
> -		component->driver->compr_ops->free(cstream);
> -	}
> +	if (compr_ops && compr_ops->free)
> +		compr_ops->free(cstream);
>  
>  	if (cpu_dai->driver->cops && cpu_dai->driver->cops->shutdown)
>  		cpu_dai->driver->cops->shutdown(cstream, cpu_dai);
> @@ -424,43 +350,23 @@ static int soc_compr_trigger(struct snd_compr_stream *cstream, int cmd)
>  {
>  
>  	struct snd_soc_pcm_runtime *rtd = cstream->private_data;
> -	struct snd_soc_platform *platform = rtd->platform;
> -	struct snd_soc_component *component;
> -	struct snd_soc_rtdcom_list *rtdcom;
> +	struct snd_soc_component *component = soc_compr_get_component(rtd);
> +	const struct snd_compr_ops *compr_ops = soc_compr_get_ops(rtd, component);
>  	struct snd_soc_dai *codec_dai = rtd->codec_dai;
>  	struct snd_soc_dai *cpu_dai = rtd->cpu_dai;
> -	int ret = 0, __ret;
> +	int ret = 0;
>  
>  	mutex_lock_nested(&rtd->pcm_mutex, rtd->pcm_subclass);
>  
> -	if (platform && platform->driver->compr_ops && platform->driver->compr_ops->trigger) {
> -		ret = platform->driver->compr_ops->trigger(cstream, cmd);
> +	if (compr_ops && compr_ops->trigger) {
> +		ret = compr_ops->trigger(cstream, cmd);
>  		if (ret < 0)
>  			goto out;
>  	}
>  
> -	for_each_rtdcom(rtd, rtdcom) {
> -		component = rtdcom->component;
> -
> -		/* ignore duplication for now */
> -		if (platform && (component == &platform->component))
> -			continue;
> -
> -		if (!component->driver->compr_ops ||
> -		    !component->driver->compr_ops->trigger)
> -			continue;
> -
> -		__ret = component->driver->compr_ops->trigger(cstream, cmd);
> -		if (__ret < 0)
> -			ret = __ret;
> -	}
> -	if (ret < 0)
> -		goto out;
> -
>  	if (cpu_dai->driver->cops && cpu_dai->driver->cops->trigger)
>  		cpu_dai->driver->cops->trigger(cstream, cmd, cpu_dai);
>  
> -
>  	switch (cmd) {
>  	case SNDRV_PCM_TRIGGER_START:
>  		snd_soc_dai_digital_mute(codec_dai, 0, cstream->direction);
> @@ -478,37 +384,16 @@ static int soc_compr_trigger(struct snd_compr_stream *cstream, int cmd)
>  static int soc_compr_trigger_fe(struct snd_compr_stream *cstream, int cmd)
>  {
>  	struct snd_soc_pcm_runtime *fe = cstream->private_data;
> -	struct snd_soc_platform *platform = fe->platform;
> -	struct snd_soc_component *component;
> -	struct snd_soc_rtdcom_list *rtdcom;
> +	struct snd_soc_component *component = soc_compr_get_component(fe);
> +	const struct snd_compr_ops *compr_ops = soc_compr_get_ops(fe, component);
>  	struct snd_soc_dai *cpu_dai = fe->cpu_dai;
> -	int ret = 0, __ret, stream;
> +	int ret = 0, stream;
>  
>  	if (cmd == SND_COMPR_TRIGGER_PARTIAL_DRAIN ||
>  		cmd == SND_COMPR_TRIGGER_DRAIN) {
>  
> -		if (platform &&
> -		    platform->driver->compr_ops &&
> -		    platform->driver->compr_ops->trigger)
> -			return platform->driver->compr_ops->trigger(cstream,
> -								    cmd);
> -
> -		for_each_rtdcom(fe, rtdcom) {
> -			component = rtdcom->component;
> -
> -			/* ignore duplication for now */
> -			if (platform && (component == &platform->component))
> -				continue;
> -
> -			if (!component->driver->compr_ops ||
> -			    !component->driver->compr_ops->trigger)
> -				continue;
> -
> -			__ret = component->driver->compr_ops->trigger(cstream, cmd);
> -			if (__ret < 0)
> -				ret = __ret;
> -		}
> -		return ret;
> +		if (compr_ops && compr_ops->trigger)
> +			return compr_ops->trigger(cstream, cmd);
>  	}
>  
>  	if (cstream->direction == SND_COMPRESS_PLAYBACK)
> @@ -525,30 +410,12 @@ static int soc_compr_trigger_fe(struct snd_compr_stream *cstream, int cmd)
>  			goto out;
>  	}
>  
> -	if (platform && platform->driver->compr_ops && platform->driver->compr_ops->trigger) {
> -		ret = platform->driver->compr_ops->trigger(cstream, cmd);
> +	if (compr_ops && compr_ops->trigger) {
> +		ret = compr_ops->trigger(cstream, cmd);
>  		if (ret < 0)
>  			goto out;
>  	}
>  
> -	for_each_rtdcom(fe, rtdcom) {
> -		component = rtdcom->component;
> -
> -		/* ignore duplication for now */
> -		if (platform && (component == &platform->component))
> -			continue;
> -
> -		if (!component->driver->compr_ops ||
> -		    !component->driver->compr_ops->trigger)
> -			continue;
> -
> -		__ret = component->driver->compr_ops->trigger(cstream, cmd);
> -		if (__ret < 0)
> -			ret = __ret;
> -	}
> -	if (ret < 0)
> -		goto out;
> -
>  	fe->dpcm[stream].runtime_update = SND_SOC_DPCM_UPDATE_FE;
>  
>  	ret = dpcm_be_dai_trigger(fe, stream, cmd);
> @@ -578,11 +445,10 @@ static int soc_compr_set_params(struct snd_compr_stream *cstream,
>  					struct snd_compr_params *params)
>  {
>  	struct snd_soc_pcm_runtime *rtd = cstream->private_data;
> -	struct snd_soc_platform *platform = rtd->platform;
> -	struct snd_soc_component *component;
> -	struct snd_soc_rtdcom_list *rtdcom;
> +	struct snd_soc_component *component = soc_compr_get_component(rtd);
> +	const struct snd_compr_ops *compr_ops = soc_compr_get_ops(rtd, component);
>  	struct snd_soc_dai *cpu_dai = rtd->cpu_dai;
> -	int ret = 0, __ret;
> +	int ret = 0;
>  
>  	mutex_lock_nested(&rtd->pcm_mutex, rtd->pcm_subclass);
>  
> @@ -598,30 +464,12 @@ static int soc_compr_set_params(struct snd_compr_stream *cstream,
>  			goto err;
>  	}
>  
> -	if (platform && platform->driver->compr_ops && platform->driver->compr_ops->set_params) {
> -		ret = platform->driver->compr_ops->set_params(cstream, params);
> +	if (compr_ops && compr_ops->set_params) {
> +		ret = compr_ops->set_params(cstream, params);
>  		if (ret < 0)
>  			goto err;
>  	}
>  
> -	for_each_rtdcom(rtd, rtdcom) {
> -		component = rtdcom->component;
> -
> -		/* ignore duplication for now */
> -		if (platform && (component == &platform->component))
> -			continue;
> -
> -		if (!component->driver->compr_ops ||
> -		    !component->driver->compr_ops->set_params)
> -			continue;
> -
> -		__ret = component->driver->compr_ops->set_params(cstream, params);
> -		if (__ret < 0)
> -			ret = __ret;
> -	}
> -	if (ret < 0)
> -		goto err;
> -
>  	if (rtd->dai_link->compr_ops && rtd->dai_link->compr_ops->set_params) {
>  		ret = rtd->dai_link->compr_ops->set_params(cstream);
>  		if (ret < 0)
> @@ -654,11 +502,10 @@ static int soc_compr_set_params_fe(struct snd_compr_stream *cstream,
>  	struct snd_soc_pcm_runtime *fe = cstream->private_data;
>  	struct snd_pcm_substream *fe_substream =
>  		 fe->pcm->streams[cstream->direction].substream;
> -	struct snd_soc_platform *platform = fe->platform;
> -	struct snd_soc_component *component;
> -	struct snd_soc_rtdcom_list *rtdcom;
> +	struct snd_soc_component *component = soc_compr_get_component(fe);
> +	const struct snd_compr_ops *compr_ops = soc_compr_get_ops(fe, component);
>  	struct snd_soc_dai *cpu_dai = fe->cpu_dai;
> -	int ret = 0, __ret, stream;
> +	int ret = 0, stream;
>  
>  	if (cstream->direction == SND_COMPRESS_PLAYBACK)
>  		stream = SNDRV_PCM_STREAM_PLAYBACK;
> @@ -673,30 +520,12 @@ static int soc_compr_set_params_fe(struct snd_compr_stream *cstream,
>  			goto out;
>  	}
>  
> -	if (platform && platform->driver->compr_ops && platform->driver->compr_ops->set_params) {
> -		ret = platform->driver->compr_ops->set_params(cstream, params);
> +	if (compr_ops && compr_ops->set_params) {
> +		ret = compr_ops->set_params(cstream, params);
>  		if (ret < 0)
>  			goto out;
>  	}
>  
> -	for_each_rtdcom(fe, rtdcom) {
> -		component = rtdcom->component;
> -
> -		/* ignore duplication for now */
> -		if (platform && (component == &platform->component))
> -			continue;
> -
> -		if (!component->driver->compr_ops ||
> -		    !component->driver->compr_ops->set_params)
> -			continue;
> -
> -		__ret = component->driver->compr_ops->set_params(cstream, params);
> -		if (__ret < 0)
> -			ret = __ret;
> -	}
> -	if (ret < 0)
> -		goto out;
> -
>  	if (fe->dai_link->compr_ops && fe->dai_link->compr_ops->set_params) {
>  		ret = fe->dai_link->compr_ops->set_params(cstream);
>  		if (ret < 0)
> @@ -734,11 +563,10 @@ static int soc_compr_get_params(struct snd_compr_stream *cstream,
>  					struct snd_codec *params)
>  {
>  	struct snd_soc_pcm_runtime *rtd = cstream->private_data;
> -	struct snd_soc_platform *platform = rtd->platform;
> -	struct snd_soc_component *component;
> -	struct snd_soc_rtdcom_list *rtdcom;
> +	struct snd_soc_component *component = soc_compr_get_component(rtd);
> +	const struct snd_compr_ops *compr_ops = soc_compr_get_ops(rtd, component);
>  	struct snd_soc_dai *cpu_dai = rtd->cpu_dai;
> -	int ret = 0, __ret;
> +	int ret = 0;
>  
>  	mutex_lock_nested(&rtd->pcm_mutex, rtd->pcm_subclass);
>  
> @@ -748,27 +576,8 @@ static int soc_compr_get_params(struct snd_compr_stream *cstream,
>  			goto err;
>  	}
>  
> -	if (platform && platform->driver->compr_ops && platform->driver->compr_ops->get_params) {
> -		ret = platform->driver->compr_ops->get_params(cstream, params);
> -		if (ret < 0)
> -			goto err;
> -	}
> -
> -	for_each_rtdcom(rtd, rtdcom) {
> -		component = rtdcom->component;
> -
> -		/* ignore duplication for now */
> -		if (platform && (component == &platform->component))
> -			continue;
> -
> -		if (!component->driver->compr_ops ||
> -		    !component->driver->compr_ops->get_params)
> -			continue;
> -
> -		__ret = component->driver->compr_ops->get_params(cstream, params);
> -		if (__ret < 0)
> -			ret = __ret;
> -	}
> +	if (compr_ops && compr_ops->get_params)
> +		ret = compr_ops->get_params(cstream, params);
>  
>  err:
>  	mutex_unlock(&rtd->pcm_mutex);
> @@ -779,36 +588,15 @@ static int soc_compr_get_caps(struct snd_compr_stream *cstream,
>  				struct snd_compr_caps *caps)
>  {
>  	struct snd_soc_pcm_runtime *rtd = cstream->private_data;
> -	struct snd_soc_platform *platform = rtd->platform;
> -	struct snd_soc_component *component;
> -	struct snd_soc_rtdcom_list *rtdcom;
> -	int ret = 0, __ret;
> +	struct snd_soc_component *component = soc_compr_get_component(rtd);
> +	const struct snd_compr_ops *compr_ops = soc_compr_get_ops(rtd, component);
> +	int ret = 0;
>  
>  	mutex_lock_nested(&rtd->pcm_mutex, rtd->pcm_subclass);
>  
> -	if (platform && platform->driver->compr_ops && platform->driver->compr_ops->get_caps) {
> -		ret = platform->driver->compr_ops->get_caps(cstream, caps);
> -		if (ret < 0)
> -			goto err;
> -	}
> -
> -	for_each_rtdcom(rtd, rtdcom) {
> -		component = rtdcom->component;
> -
> -		/* ignore duplication for now */
> -		if (platform && (component == &platform->component))
> -			continue;
> -
> -		if (!component->driver->compr_ops ||
> -		    !component->driver->compr_ops->get_caps)
> -			continue;
> +	if (compr_ops && compr_ops->get_caps)
> +		ret = compr_ops->get_caps(cstream, caps);
>  
> -		__ret = component->driver->compr_ops->get_caps(cstream, caps);
> -		if (__ret < 0)
> -			ret = __ret;
> -	}
> -
> -err:
>  	mutex_unlock(&rtd->pcm_mutex);
>  	return ret;
>  }
> @@ -817,36 +605,15 @@ static int soc_compr_get_codec_caps(struct snd_compr_stream *cstream,
>  				struct snd_compr_codec_caps *codec)
>  {
>  	struct snd_soc_pcm_runtime *rtd = cstream->private_data;
> -	struct snd_soc_platform *platform = rtd->platform;
> -	struct snd_soc_component *component;
> -	struct snd_soc_rtdcom_list *rtdcom;
> -	int ret = 0, __ret;
> +	struct snd_soc_component *component = soc_compr_get_component(rtd);
> +	const struct snd_compr_ops *compr_ops = soc_compr_get_ops(rtd, component);
> +	int ret = 0;
>  
>  	mutex_lock_nested(&rtd->pcm_mutex, rtd->pcm_subclass);
>  
> -	if (platform && platform->driver->compr_ops && platform->driver->compr_ops->get_codec_caps) {
> -		ret = platform->driver->compr_ops->get_codec_caps(cstream, codec);
> -		if (ret < 0)
> -			goto err;
> -	}
> -
> -	for_each_rtdcom(rtd, rtdcom) {
> -		component = rtdcom->component;
> +	if (compr_ops && compr_ops->get_codec_caps)
> +		ret = compr_ops->get_codec_caps(cstream, codec);
>  
> -		/* ignore duplication for now */
> -		if (platform && (component == &platform->component))
> -			continue;
> -
> -		if (!component->driver->compr_ops ||
> -		    !component->driver->compr_ops->get_codec_caps)
> -			continue;
> -
> -		__ret = component->driver->compr_ops->get_codec_caps(cstream, codec);
> -		if (__ret < 0)
> -			ret = __ret;
> -	}
> -
> -err:
>  	mutex_unlock(&rtd->pcm_mutex);
>  	return ret;
>  }
> @@ -854,11 +621,10 @@ static int soc_compr_get_codec_caps(struct snd_compr_stream *cstream,
>  static int soc_compr_ack(struct snd_compr_stream *cstream, size_t bytes)
>  {
>  	struct snd_soc_pcm_runtime *rtd = cstream->private_data;
> -	struct snd_soc_platform *platform = rtd->platform;
> -	struct snd_soc_component *component;
> -	struct snd_soc_rtdcom_list *rtdcom;
> +	struct snd_soc_component *component = soc_compr_get_component(rtd);
> +	const struct snd_compr_ops *compr_ops = soc_compr_get_ops(rtd, component);
>  	struct snd_soc_dai *cpu_dai = rtd->cpu_dai;
> -	int ret = 0, __ret;
> +	int ret = 0;
>  
>  	mutex_lock_nested(&rtd->pcm_mutex, rtd->pcm_subclass);
>  
> @@ -868,27 +634,8 @@ static int soc_compr_ack(struct snd_compr_stream *cstream, size_t bytes)
>  			goto err;
>  	}
>  
> -	if (platform && platform->driver->compr_ops && platform->driver->compr_ops->ack) {
> -		ret = platform->driver->compr_ops->ack(cstream, bytes);
> -		if (ret < 0)
> -			goto err;
> -	}
> -
> -	for_each_rtdcom(rtd, rtdcom) {
> -		component = rtdcom->component;
> -
> -		/* ignore duplication for now */
> -		if (platform && (component == &platform->component))
> -			continue;
> -
> -		if (!component->driver->compr_ops ||
> -		    !component->driver->compr_ops->ack)
> -			continue;
> -
> -		__ret = component->driver->compr_ops->ack(cstream, bytes);
> -		if (__ret < 0)
> -			ret = __ret;
> -	}
> +	if (compr_ops && compr_ops->ack)
> +		ret = compr_ops->ack(cstream, bytes);
>  
>  err:
>  	mutex_unlock(&rtd->pcm_mutex);
> @@ -899,10 +646,9 @@ static int soc_compr_pointer(struct snd_compr_stream *cstream,
>  			struct snd_compr_tstamp *tstamp)
>  {
>  	struct snd_soc_pcm_runtime *rtd = cstream->private_data;
> -	struct snd_soc_platform *platform = rtd->platform;
> -	struct snd_soc_component *component;
> -	struct snd_soc_rtdcom_list *rtdcom;
> -	int ret = 0, __ret;
> +	struct snd_soc_component *component = soc_compr_get_component(rtd);
> +	const struct snd_compr_ops *compr_ops = soc_compr_get_ops(rtd, component);
> +	int ret = 0;
>  	struct snd_soc_dai *cpu_dai = rtd->cpu_dai;
>  
>  	mutex_lock_nested(&rtd->pcm_mutex, rtd->pcm_subclass);
> @@ -910,29 +656,9 @@ static int soc_compr_pointer(struct snd_compr_stream *cstream,
>  	if (cpu_dai->driver->cops && cpu_dai->driver->cops->pointer)
>  		cpu_dai->driver->cops->pointer(cstream, tstamp, cpu_dai);
>  
> -	if (platform && platform->driver->compr_ops && platform->driver->compr_ops->pointer) {
> -		ret = platform->driver->compr_ops->pointer(cstream, tstamp);
> -		if (ret < 0)
> -			goto err;
> -	}
> -
> -	for_each_rtdcom(rtd, rtdcom) {
> -		component = rtdcom->component;
> -
> -		/* ignore duplication for now */
> -		if (platform && (component == &platform->component))
> -			continue;
> -
> -		if (!component->driver->compr_ops ||
> -		    !component->driver->compr_ops->pointer)
> -			continue;
> +	if (compr_ops && compr_ops->pointer)
> +		ret = compr_ops->pointer(cstream, tstamp);
>  
> -		__ret = component->driver->compr_ops->pointer(cstream, tstamp);
> -		if (__ret < 0)
> -			ret = __ret;
> -	}
> -
> -err:
>  	mutex_unlock(&rtd->pcm_mutex);
>  	return ret;
>  }
> @@ -941,35 +667,15 @@ static int soc_compr_copy(struct snd_compr_stream *cstream,
>  			  char __user *buf, size_t count)
>  {
>  	struct snd_soc_pcm_runtime *rtd = cstream->private_data;
> -	struct snd_soc_platform *platform = rtd->platform;
> -	struct snd_soc_component *component;
> -	struct snd_soc_rtdcom_list *rtdcom;
> -	int ret = 0, __ret;
> +	struct snd_soc_component *component = soc_compr_get_component(rtd);
> +	const struct snd_compr_ops *compr_ops = soc_compr_get_ops(rtd, component);
> +	int ret = 0;
>  
>  	mutex_lock_nested(&rtd->pcm_mutex, rtd->pcm_subclass);
>  
> -	if (platform && platform->driver->compr_ops && platform->driver->compr_ops->copy) {
> -		ret = platform->driver->compr_ops->copy(cstream, buf, count);
> -		if (ret < 0)
> -			goto err;
> -	}
> -
> -	for_each_rtdcom(rtd, rtdcom) {
> -		component = rtdcom->component;
> -
> -		/* ignore duplication for now */
> -		if (platform && (component == &platform->component))
> -			continue;
> +	if (compr_ops && compr_ops->copy)
> +		ret = compr_ops->copy(cstream, buf, count);
>  
> -		if (!component->driver->compr_ops ||
> -		    !component->driver->compr_ops->copy)
> -			continue;
> -
> -		__ret = component->driver->compr_ops->copy(cstream, buf, count);
> -		if (__ret < 0)
> -			ret = __ret;
> -	}
> -err:
>  	mutex_unlock(&rtd->pcm_mutex);
>  	return ret;
>  }
> @@ -978,11 +684,10 @@ static int soc_compr_set_metadata(struct snd_compr_stream *cstream,
>  				struct snd_compr_metadata *metadata)
>  {
>  	struct snd_soc_pcm_runtime *rtd = cstream->private_data;
> -	struct snd_soc_platform *platform = rtd->platform;
> -	struct snd_soc_component *component;
> -	struct snd_soc_rtdcom_list *rtdcom;
> +	struct snd_soc_component *component = soc_compr_get_component(rtd);
> +	const struct snd_compr_ops *compr_ops = soc_compr_get_ops(rtd, component);
>  	struct snd_soc_dai *cpu_dai = rtd->cpu_dai;
> -	int ret = 0, __ret;
> +	int ret = 0;
>  
>  	if (cpu_dai->driver->cops && cpu_dai->driver->cops->set_metadata) {
>  		ret = cpu_dai->driver->cops->set_metadata(cstream, metadata, cpu_dai);
> @@ -990,27 +695,8 @@ static int soc_compr_set_metadata(struct snd_compr_stream *cstream,
>  			return ret;
>  	}
>  
> -	if (platform && platform->driver->compr_ops && platform->driver->compr_ops->set_metadata) {
> -		ret = platform->driver->compr_ops->set_metadata(cstream, metadata);
> -		if (ret < 0)
> -			return ret;
> -	}
> -
> -	for_each_rtdcom(rtd, rtdcom) {
> -		component = rtdcom->component;
> -
> -		/* ignore duplication for now */
> -		if (platform && (component == &platform->component))
> -			continue;
> -
> -		if (!component->driver->compr_ops ||
> -		    !component->driver->compr_ops->set_metadata)
> -			continue;
> -
> -		__ret = component->driver->compr_ops->set_metadata(cstream, metadata);
> -		if (__ret < 0)
> -			ret = __ret;
> -	}
> +	if (compr_ops && compr_ops->set_metadata)
> +		ret = compr_ops->set_metadata(cstream, metadata);
>  
>  	return ret;
>  }
> @@ -1019,11 +705,10 @@ static int soc_compr_get_metadata(struct snd_compr_stream *cstream,
>  				struct snd_compr_metadata *metadata)
>  {
>  	struct snd_soc_pcm_runtime *rtd = cstream->private_data;
> -	struct snd_soc_platform *platform = rtd->platform;
> -	struct snd_soc_component *component;
> -	struct snd_soc_rtdcom_list *rtdcom;
> +	struct snd_soc_component *component = soc_compr_get_component(rtd);
> +	const struct snd_compr_ops *compr_ops = soc_compr_get_ops(rtd, component);
>  	struct snd_soc_dai *cpu_dai = rtd->cpu_dai;
> -	int ret = 0, __ret;
> +	int ret = 0;
>  
>  	if (cpu_dai->driver->cops && cpu_dai->driver->cops->get_metadata) {
>  		ret = cpu_dai->driver->cops->get_metadata(cstream, metadata, cpu_dai);
> @@ -1031,27 +716,8 @@ static int soc_compr_get_metadata(struct snd_compr_stream *cstream,
>  			return ret;
>  	}
>  
> -	if (platform && platform->driver->compr_ops && platform->driver->compr_ops->get_metadata) {
> -		ret = platform->driver->compr_ops->get_metadata(cstream, metadata);
> -		if (ret < 0)
> -			return ret;
> -	}
> -
> -	for_each_rtdcom(rtd, rtdcom) {
> -		component = rtdcom->component;
> -
> -		/* ignore duplication for now */
> -		if (platform && (component == &platform->component))
> -			continue;
> -
> -		if (!component->driver->compr_ops ||
> -		    !component->driver->compr_ops->get_metadata)
> -			continue;
> -
> -		__ret = component->driver->compr_ops->get_metadata(cstream, metadata);
> -		if (__ret < 0)
> -			ret = __ret;
> -	}
> +	if (compr_ops && compr_ops->get_metadata)
> +		ret = compr_ops->get_metadata(cstream, metadata);
>  
>  	return ret;
>  }
> @@ -1096,9 +762,8 @@ static struct snd_compr_ops soc_compr_dyn_ops = {
>   */
>  int snd_soc_new_compress(struct snd_soc_pcm_runtime *rtd, int num)
>  {
> -	struct snd_soc_platform *platform = rtd->platform;
> -	struct snd_soc_component *component;
> -	struct snd_soc_rtdcom_list *rtdcom;
> +	struct snd_soc_component *component = soc_compr_get_component(rtd);
> +	const struct snd_compr_ops *compr_ops = soc_compr_get_ops(rtd, component);
>  	struct snd_soc_dai *codec_dai = rtd->codec_dai;
>  	struct snd_soc_dai *cpu_dai = rtd->cpu_dai;
>  	struct snd_compr *compr;
> @@ -1174,25 +839,9 @@ int snd_soc_new_compress(struct snd_soc_pcm_runtime *rtd, int num)
>  		memcpy(compr->ops, &soc_compr_ops, sizeof(soc_compr_ops));
>  	}
>  
> -
>  	/* Add copy callback for not memory mapped DSPs */
> -	if (platform && platform->driver->compr_ops && platform->driver->compr_ops->copy)
> -		compr->ops->copy = soc_compr_copy;
> -
> -	for_each_rtdcom(rtd, rtdcom) {
> -		component = rtdcom->component;
> -
> -		/* ignore duplication for now */
> -		if (platform && (component == &platform->component))
> -			continue;
> -
> -		if (!component->driver->compr_ops ||
> -		    !component->driver->compr_ops->copy)
> -			continue;
> -
> +	if (compr_ops && compr_ops->copy)
>  		compr->ops->copy = soc_compr_copy;
> -	}
> -
>  
>  	mutex_init(&compr->lock);
>  	ret = snd_compress_new(rtd->card->snd_card, num, direction,
> -- 
> 2.11.0
> 

-- 
~Vinod

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

* Re: [PATCH 2/2] ASoC: compress: Fixup some minor style issues
  2018-01-24 13:55 ` [PATCH 2/2] ASoC: compress: Fixup some minor style issues Charles Keepax
@ 2018-01-24 14:33   ` Ladislav Michl
  2018-01-24 14:50     ` Charles Keepax
  0 siblings, 1 reply; 16+ messages in thread
From: Ladislav Michl @ 2018-01-24 14:33 UTC (permalink / raw)
  To: Charles Keepax
  Cc: alsa-devel, kuninori.morimoto.gx, vinod.koul, lgirdwood, broonie,
	dan.carpenter

On Wed, Jan 24, 2018 at 01:55:25PM +0000, Charles Keepax wrote:
> Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com>
> ---
>  sound/soc/soc-compress.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/sound/soc/soc-compress.c b/sound/soc/soc-compress.c
> index 34834c02dda8..7c1955e9533a 100644
> --- a/sound/soc/soc-compress.c
> +++ b/sound/soc/soc-compress.c
> @@ -262,7 +262,6 @@ static int soc_compr_free(struct snd_compr_stream *cstream)
>  	if (!codec_dai->active)
>  		codec_dai->rate = 0;
>  
> -
>  	if (rtd->dai_link->compr_ops && rtd->dai_link->compr_ops->shutdown)
>  		rtd->dai_link->compr_ops->shutdown(cstream);
>  
> @@ -401,7 +400,6 @@ static int soc_compr_trigger_fe(struct snd_compr_stream *cstream, int cmd)
>  	else
>  		stream = SNDRV_PCM_STREAM_CAPTURE;
>  
> -
>  	mutex_lock_nested(&fe->card->mutex, SND_SOC_CARD_CLASS_RUNTIME);
>  
>  	if (cpu_dai->driver->cops && cpu_dai->driver->cops->trigger) {
> @@ -859,7 +857,7 @@ int snd_soc_new_compress(struct snd_soc_pcm_runtime *rtd, int num)
>  	rtd->compr = compr;
>  	compr->private_data = rtd;
>  
> -	printk(KERN_INFO "compress asoc: %s <-> %s mapping ok\n", codec_dai->name,
> +	pr_info("compress asoc: %s <-> %s mapping ok\n", codec_dai->name,
>  		cpu_dai->name);

What about:
	dev_info(rtd->card->dev, ...
or better drop this change and make separate patch fixing logging
soc-compress-wide?

>  	return ret;
>  
> -- 
> 2.11.0
> 
> _______________________________________________
> Alsa-devel mailing list
> Alsa-devel@alsa-project.org
> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

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

* Re: [PATCH 1/2] ASoC: compress: Correct handling of compressed ops
  2018-01-24 14:29 ` [PATCH 1/2] ASoC: compress: Correct handling of compressed ops Vinod Koul
@ 2018-01-24 14:49   ` Charles Keepax
  2018-01-24 15:33     ` Mark Brown
  0 siblings, 1 reply; 16+ messages in thread
From: Charles Keepax @ 2018-01-24 14:49 UTC (permalink / raw)
  To: Vinod Koul
  Cc: alsa-devel, broonie, lgirdwood, dan.carpenter, kuninori.morimoto.gx

On Wed, Jan 24, 2018 at 07:59:01PM +0530, Vinod Koul wrote:
> On Wed, Jan 24, 2018 at 01:55:24PM +0000, Charles Keepax wrote:
> > The soc_compr_copy callback is currently broken. Since the
> > changes to move the compr_ops over to the component the return
> > value is not correctly propagated, always returning zero on
> > success rather than the number of bytes copied. This causes
> > user-space to stall continuously reading as it does not believe
> > it has received any data.
> > 
> > Furthermore, the changes to move the compr_ops over to the
> > component iterate through the list of components and will
> > call the compressed operation for any that have compressed
> > ops. However, there would be signifcantly more work required
> > to support such a system. There isn't currently any sensible
> > mechanism to forward the ops to multiple components. For example
> > what should be done about merging data from copy?  Or what should
> > be returned through the pointer call, each component may have
> > different amounts of data available, but user-space expects a
> > single answer?
> > 
> > As such clean up the code a little by factoring out the search
> > for the relevant component and bring things back to looking for
> > a single component that supports compressed ops on a single
> > runtime. These refactorings also put back the original handling
> > for the copy callback, correcting the return value.
> 
> Thank you Charles for spotting this and fixing it. The changes look good to
> me. But should we split the copy fix from rest of the handling and backport
> that one to stable?
> 
> > 
> > Fixes: 9e7e3738ab0e ("ASoC: snd_soc_component_driver has snd_compr_ops")

The offending change here is only in the v4.15-rc's so if we can
get the change in before 4.16 there shouldn't be any need to send
anything to stable.

I had thought about splitting this into two changes but the fix
to the copy stuff becomes a little odd since you end up with the
copy callback returning as soon as it hits a viable compressed
ops but all the other callbacks keep running. Or you could fix up
all the callbacks but then it starts to feel like you might as
well apply this patch. Where I got to was really the primary bug
here is that the code would try to call multiple callbacks it
just so happens that the only thing that manifests is the copy
return on my system.

That said if others are keen for me to split it as well I don't
mind doing so.

Thanks,
Charles

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

* Re: [PATCH 2/2] ASoC: compress: Fixup some minor style issues
  2018-01-24 14:33   ` Ladislav Michl
@ 2018-01-24 14:50     ` Charles Keepax
  0 siblings, 0 replies; 16+ messages in thread
From: Charles Keepax @ 2018-01-24 14:50 UTC (permalink / raw)
  To: Ladislav Michl
  Cc: alsa-devel, kuninori.morimoto.gx, vinod.koul, lgirdwood, broonie,
	dan.carpenter

On Wed, Jan 24, 2018 at 03:33:19PM +0100, Ladislav Michl wrote:
> On Wed, Jan 24, 2018 at 01:55:25PM +0000, Charles Keepax wrote:
> > Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com>
> > ---
> >  sound/soc/soc-compress.c | 4 +---
> >  1 file changed, 1 insertion(+), 3 deletions(-)
> > 
> > diff --git a/sound/soc/soc-compress.c b/sound/soc/soc-compress.c
> > index 34834c02dda8..7c1955e9533a 100644
> > --- a/sound/soc/soc-compress.c
> > +++ b/sound/soc/soc-compress.c
> > @@ -262,7 +262,6 @@ static int soc_compr_free(struct snd_compr_stream *cstream)
> >  	if (!codec_dai->active)
> >  		codec_dai->rate = 0;
> >  
> > -
> >  	if (rtd->dai_link->compr_ops && rtd->dai_link->compr_ops->shutdown)
> >  		rtd->dai_link->compr_ops->shutdown(cstream);
> >  
> > @@ -401,7 +400,6 @@ static int soc_compr_trigger_fe(struct snd_compr_stream *cstream, int cmd)
> >  	else
> >  		stream = SNDRV_PCM_STREAM_CAPTURE;
> >  
> > -
> >  	mutex_lock_nested(&fe->card->mutex, SND_SOC_CARD_CLASS_RUNTIME);
> >  
> >  	if (cpu_dai->driver->cops && cpu_dai->driver->cops->trigger) {
> > @@ -859,7 +857,7 @@ int snd_soc_new_compress(struct snd_soc_pcm_runtime *rtd, int num)
> >  	rtd->compr = compr;
> >  	compr->private_data = rtd;
> >  
> > -	printk(KERN_INFO "compress asoc: %s <-> %s mapping ok\n", codec_dai->name,
> > +	pr_info("compress asoc: %s <-> %s mapping ok\n", codec_dai->name,
> >  		cpu_dai->name);
> 
> What about:
> 	dev_info(rtd->card->dev, ...
> or better drop this change and make separate patch fixing logging
> soc-compress-wide?
> 

yes I had thought about doing that but then laziness got the
better of me :-)

Will perhaps split into a whitespace fix and update all the
logging to use dev_* unless anyone objects?

Thanks,
Charles

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

* Re: [PATCH 1/2] ASoC: compress: Correct handling of compressed ops
  2018-01-24 14:49   ` Charles Keepax
@ 2018-01-24 15:33     ` Mark Brown
  2018-01-24 16:00       ` Charles Keepax
  0 siblings, 1 reply; 16+ messages in thread
From: Mark Brown @ 2018-01-24 15:33 UTC (permalink / raw)
  To: Charles Keepax
  Cc: Vinod Koul, alsa-devel, lgirdwood, dan.carpenter, kuninori.morimoto.gx


[-- Attachment #1.1: Type: text/plain, Size: 295 bytes --]

On Wed, Jan 24, 2018 at 02:49:15PM +0000, Charles Keepax wrote:

> The offending change here is only in the v4.15-rc's so if we can
> get the change in before 4.16 there shouldn't be any need to send
> anything to stable.

This doesn't apply against Linus' tree so that's not going to happen...

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]



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

* Re: [PATCH 1/2] ASoC: compress: Correct handling of compressed ops
  2018-01-24 15:33     ` Mark Brown
@ 2018-01-24 16:00       ` Charles Keepax
  2018-01-24 16:05         ` Mark Brown
  0 siblings, 1 reply; 16+ messages in thread
From: Charles Keepax @ 2018-01-24 16:00 UTC (permalink / raw)
  To: Mark Brown
  Cc: Vinod Koul, alsa-devel, lgirdwood, dan.carpenter, kuninori.morimoto.gx

On Wed, Jan 24, 2018 at 03:33:30PM +0000, Mark Brown wrote:
> On Wed, Jan 24, 2018 at 02:49:15PM +0000, Charles Keepax wrote:
> 
> > The offending change here is only in the v4.15-rc's so if we can
> > get the change in before 4.16 there shouldn't be any need to send
> > anything to stable.
> 
> This doesn't apply against Linus' tree so that's not going to happen...

Shall I split it into a crufty fix and then a tidy up?

Thanks,
Charles

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

* Re: [PATCH 1/2] ASoC: compress: Correct handling of compressed ops
  2018-01-24 16:00       ` Charles Keepax
@ 2018-01-24 16:05         ` Mark Brown
  2018-01-24 17:14           ` Charles Keepax
  0 siblings, 1 reply; 16+ messages in thread
From: Mark Brown @ 2018-01-24 16:05 UTC (permalink / raw)
  To: Charles Keepax
  Cc: Vinod Koul, alsa-devel, lgirdwood, dan.carpenter, kuninori.morimoto.gx


[-- Attachment #1.1: Type: text/plain, Size: 348 bytes --]

On Wed, Jan 24, 2018 at 04:00:29PM +0000, Charles Keepax wrote:
> On Wed, Jan 24, 2018 at 03:33:30PM +0000, Mark Brown wrote:

> > This doesn't apply against Linus' tree so that's not going to happen...

> Shall I split it into a crufty fix and then a tidy up?

Without seeing how awkward the crufty fix looks it's hard to say, use
your judgement.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]



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

* Re: [PATCH 1/2] ASoC: compress: Correct handling of compressed ops
  2018-01-24 16:05         ` Mark Brown
@ 2018-01-24 17:14           ` Charles Keepax
  2018-01-25 11:03             ` Charles Keepax
  0 siblings, 1 reply; 16+ messages in thread
From: Charles Keepax @ 2018-01-24 17:14 UTC (permalink / raw)
  To: Mark Brown
  Cc: Vinod Koul, alsa-devel, lgirdwood, dan.carpenter, kuninori.morimoto.gx

On Wed, Jan 24, 2018 at 04:05:51PM +0000, Mark Brown wrote:
> On Wed, Jan 24, 2018 at 04:00:29PM +0000, Charles Keepax wrote:
> > On Wed, Jan 24, 2018 at 03:33:30PM +0000, Mark Brown wrote:
> 
> > > This doesn't apply against Linus' tree so that's not going to happen...
> 
> > Shall I split it into a crufty fix and then a tidy up?
> 
> Without seeing how awkward the crufty fix looks it's hard to say, use
> your judgement.

My feeling is still the same really that we should stick with this
patch even though its a bit more to put into stable, since really
the looping stuff is a bug as well.

But that said I will have a go at drafting up a separated patch,
see what it looks like and report back tomorrow.

Thanks,
Charles

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

* Re: [PATCH 1/2] ASoC: compress: Correct handling of compressed ops
  2018-01-24 13:55 [PATCH 1/2] ASoC: compress: Correct handling of compressed ops Charles Keepax
  2018-01-24 13:55 ` [PATCH 2/2] ASoC: compress: Fixup some minor style issues Charles Keepax
  2018-01-24 14:29 ` [PATCH 1/2] ASoC: compress: Correct handling of compressed ops Vinod Koul
@ 2018-01-25  1:18 ` Kuninori Morimoto
  2018-01-25  9:47   ` Charles Keepax
  2 siblings, 1 reply; 16+ messages in thread
From: Kuninori Morimoto @ 2018-01-25  1:18 UTC (permalink / raw)
  To: Charles Keepax; +Cc: vinod.koul, alsa-devel, broonie, lgirdwood, dan.carpenter


Hi Charles

Thank you for your patch.
I'm sorry that my patch breaks your system.

I'm not good at soc_compr_xxx,
thus, basically I have no objection about this patch.
This is just question.

> The soc_compr_copy callback is currently broken. Since the
> changes to move the compr_ops over to the component the return
> value is not correctly proqpagated, always returning zero on
> success rather than the number of bytes copied. This causes
> user-space to stall continuously reading as it does not believe
> it has received any data.
> 
> Furthermore, the changes to move the compr_ops over to the
> component iterate through the list of components and will
> call the compressed operation for any that have compressed
> ops. However, there would be signifcantly more work required
> to support such a system. There isn't currently any sensible
> mechanism to forward the ops to multiple components. For example
> what should be done about merging data from copy?  Or what should
> be returned through the pointer call, each component may have
> different amounts of data available, but user-space expects a
> single answer?
> 
> As such clean up the code a little by factoring out the search
> for the relevant component and bring things back to looking for
> a single component that supports compressed ops on a single
> runtime. These refactorings also put back the original handling
> for the copy callback, correcting the return value.
> 
> Fixes: 9e7e3738ab0e ("ASoC: snd_soc_component_driver has snd_compr_ops")
> Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com>
> ---
(snip)
> -static int soc_compr_open(struct snd_compr_stream *cstream)
> +static struct snd_soc_component *
> +soc_compr_get_component(struct snd_soc_pcm_runtime *rtd)
>  {
> -	struct snd_soc_pcm_runtime *rtd = cstream->private_data;
>  	struct snd_soc_platform *platform = rtd->platform;
> -	struct snd_soc_component *component;
>  	struct snd_soc_rtdcom_list *rtdcom;
> +
> +	if (platform)
> +		return &platform->component;
> +
> +	for_each_rtdcom(rtd, rtdcom) {
> +		if (rtdcom->component->driver->compr_ops)
> +			return rtdcom->component;
> +	}
> +
> +	return NULL;
> +}
(snip)
> +static int soc_compr_open(struct snd_compr_stream *cstream)
> +{
> +	struct snd_soc_pcm_runtime *rtd = cstream->private_data;
> +	struct snd_soc_component *component = soc_compr_get_component(rtd);
> +	const struct snd_compr_ops *compr_ops = soc_compr_get_ops(rtd, component);
>  	struct snd_soc_dai *cpu_dai = rtd->cpu_dai;
> -	int ret = 0, __ret;
> +	int ret = 0;
>  
>  	mutex_lock_nested(&rtd->pcm_mutex, rtd->pcm_subclass);
>  
> @@ -46,36 +74,15 @@ static int soc_compr_open(struct snd_compr_stream *cstream)
>  		}
>  	}
>  
> -	if (platform && platform->driver->compr_ops && platform->driver->compr_ops->open) {
> -		ret = platform->driver->compr_ops->open(cstream);
> +	if (compr_ops && compr_ops->open) {
> +		ret = compr_ops->open(cstream);
>  		if (ret < 0) {
>  			pr_err("compress asoc: can't open platform %s\n",
> -				platform->component.name);
> +				component->name);
>  			goto plat_err;
>  		}
>  	}
>  
> -	for_each_rtdcom(rtd, rtdcom) {
> -		component = rtdcom->component;
> -
> -		/* ignore duplication for now */
> -		if (platform && (component == &platform->component))
> -			continue;
> -
> -		if (!component->driver->compr_ops ||
> -		    !component->driver->compr_ops->open)
> -			continue;
> -
> -		__ret = component->driver->compr_ops->open(cstream);
> -		if (__ret < 0) {
> -			pr_err("compress asoc: can't open platform %s\n",
> -			       component->name);
> -			ret = __ret;
> -		}
> -	}

soc_compr_get_component() searches 1st component which have compr_ops,
but it doesn't check compr_ops->xxx, In above case, compr_ops->open.
Is it OK ?

For me, it looks like we can solve it by using "break" and "goto".
"goto" is quick hack for now.
For example like below (not tested).
If we can use rtd->platform, use it, and skip rtdcom loop by using "goto".
If we use rtdcom, call "break" after calling 1st callback.
soc_rtdcom_xxx() in soc-pcm.c is doing similar things.
I don't know this is good idea, or not.

---------------
diff --git a/sound/soc/soc-compress.c b/sound/soc/soc-compress.c
index 81232f4..a147a03 100644
--- a/sound/soc/soc-compress.c
+++ b/sound/soc/soc-compress.c
@@ -53,6 +53,7 @@ static int soc_compr_open(struct snd_compr_stream *cstream)
 				platform->component.name);
 			goto plat_err;
 		}
+		goto rtdcom_skip:
 	}
 
 	for_each_rtdcom(rtd, rtdcom) {
@@ -72,10 +73,13 @@ static int soc_compr_open(struct snd_compr_stream *cstream)
 			       component->name);
 			ret = __ret;
 		}
+		break;
 	}
 	if (ret < 0)
 		goto machine_err;
 
+rtdcom_skip:
+
 	if (rtd->dai_link->compr_ops && rtd->dai_link->compr_ops->startup) {
 		ret = rtd->dai_link->compr_ops->startup(cstream);
 		if (ret < 0) {
---------------



Best regards
---
Kuninori Morimoto

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

* Re: [PATCH 1/2] ASoC: compress: Correct handling of compressed ops
  2018-01-25  1:18 ` Kuninori Morimoto
@ 2018-01-25  9:47   ` Charles Keepax
  2018-01-26  0:33     ` Kuninori Morimoto
  0 siblings, 1 reply; 16+ messages in thread
From: Charles Keepax @ 2018-01-25  9:47 UTC (permalink / raw)
  To: Kuninori Morimoto
  Cc: vinod.koul, alsa-devel, broonie, lgirdwood, dan.carpenter

On Thu, Jan 25, 2018 at 01:18:37AM +0000, Kuninori Morimoto wrote:
> soc_compr_get_component() searches 1st component which have compr_ops,
> but it doesn't check compr_ops->xxx, In above case, compr_ops->open.
> Is it OK ?
> 
> For me, it looks like we can solve it by using "break" and "goto".
> "goto" is quick hack for now.
> For example like below (not tested).
> If we can use rtd->platform, use it, and skip rtdcom loop by using "goto".
> If we use rtdcom, call "break" after calling 1st callback.
> soc_rtdcom_xxx() in soc-pcm.c is doing similar things.
> I don't know this is good idea, or not.
> 
> ---------------
> diff --git a/sound/soc/soc-compress.c b/sound/soc/soc-compress.c
> index 81232f4..a147a03 100644
> --- a/sound/soc/soc-compress.c
> +++ b/sound/soc/soc-compress.c
> @@ -53,6 +53,7 @@ static int soc_compr_open(struct snd_compr_stream *cstream)
>  				platform->component.name);
>  			goto plat_err;
>  		}
> +		goto rtdcom_skip:
>  	}
>  
>  	for_each_rtdcom(rtd, rtdcom) {
> @@ -72,10 +73,13 @@ static int soc_compr_open(struct snd_compr_stream *cstream)
>  			       component->name);
>  			ret = __ret;
>  		}
> +		break;
>  	}
>  	if (ret < 0)
>  		goto machine_err;
>  
> +rtdcom_skip:
> +
>  	if (rtd->dai_link->compr_ops && rtd->dai_link->compr_ops->startup) {
>  		ret = rtd->dai_link->compr_ops->startup(cstream);
>  		if (ret < 0) {
> ---------------

Yes I had considered adding in breaks as an initial fix, however,
two things drove me in this direction.

Firstly, it didn't feel like it made sense to be calling
different compr_ops on different components. Not sure if anyone
feels there might be use-cases for that?

And what should happen if two components had the same
callback? Which do we call, should we call both as the current
code does, but then what to do with the results? In short I
felt there is significantly more work to be done to support
systems where we have multiple components providing compressed
functionality on a single runtime, so better to not look like we
support it at the moment.

Secondly it feels like a lot of code duplication having those
almost identical loops in every callback and this solution keeps
things much neater.

I will have a look through the soc-pcm stuff as well and have a
think if there are any issues there as well, although that does
all seem to work on my system.

Thanks,
Charles

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

* Re: [PATCH 1/2] ASoC: compress: Correct handling of compressed ops
  2018-01-24 17:14           ` Charles Keepax
@ 2018-01-25 11:03             ` Charles Keepax
  0 siblings, 0 replies; 16+ messages in thread
From: Charles Keepax @ 2018-01-25 11:03 UTC (permalink / raw)
  To: Mark Brown
  Cc: Vinod Koul, alsa-devel, lgirdwood, dan.carpenter, kuninori.morimoto.gx

On Wed, Jan 24, 2018 at 05:14:40PM +0000, Charles Keepax wrote:
> On Wed, Jan 24, 2018 at 04:05:51PM +0000, Mark Brown wrote:
> > On Wed, Jan 24, 2018 at 04:00:29PM +0000, Charles Keepax wrote:
> > > On Wed, Jan 24, 2018 at 03:33:30PM +0000, Mark Brown wrote:
> > 
> > > > This doesn't apply against Linus' tree so that's not going to happen...
> > 
> > > Shall I split it into a crufty fix and then a tidy up?
> > 
> > Without seeing how awkward the crufty fix looks it's hard to say, use
> > your judgement.
> 
> My feeling is still the same really that we should stick with this
> patch even though its a bit more to put into stable, since really
> the looping stuff is a bug as well.
> 
> But that said I will have a go at drafting up a separated patch,
> see what it looks like and report back tomorrow.

A simpler fix does indeed apply to Linus's tree, given there is
also still some discussion on the patch I will split the patch
into the copy fix and the separate refactoring so we can get the
fix in now.

Thanks,
Charles

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

* Re: [PATCH 1/2] ASoC: compress: Correct handling of compressed ops
  2018-01-25  9:47   ` Charles Keepax
@ 2018-01-26  0:33     ` Kuninori Morimoto
  2018-01-26 10:03       ` Charles Keepax
  0 siblings, 1 reply; 16+ messages in thread
From: Kuninori Morimoto @ 2018-01-26  0:33 UTC (permalink / raw)
  To: Charles Keepax; +Cc: vinod.koul, alsa-devel, broonie, lgirdwood, dan.carpenter


Hi Charles

Thank you for your feedback

> Yes I had considered adding in breaks as an initial fix, however,
> two things drove me in this direction.
> 
> Firstly, it didn't feel like it made sense to be calling
> different compr_ops on different components. Not sure if anyone
> feels there might be use-cases for that?
> 
> And what should happen if two components had the same
> callback? Which do we call, should we call both as the current
> code does, but then what to do with the results? In short I
> felt there is significantly more work to be done to support
> systems where we have multiple components providing compressed
> functionality on a single runtime, so better to not look like we
> support it at the moment.
> 
> Secondly it feels like a lot of code duplication having those
> almost identical loops in every callback and this solution keeps
> things much neater.
> 
> I will have a look through the soc-pcm stuff as well and have a
> think if there are any issues there as well, although that does
> all seem to work on my system.

In existing drivers, the component which have compr_ops *was* originally
"platform" component only.
The main purpose why we want to replace platform (and codec) to component is that
CPU/Codec/Platform categorize is no longer best match for modern device.

Thus, if we can replace all CPU/Codec/Platform into component, every device
can have every feature, with no categorize !
In other words, we don't know how many device have it.

But anyway, we don't have such situation, and we don't know how to adjust to it
at this point.
Thus Yes !! I can 100% agree your opinion.
Thank you for your help

One note is that "rtd->platform" itself will be removed if all platforms were replaced.
Then, soc_compr_get_component() / soc_compr_get_ops() might be merged or adjusted.

Best regards
---
Kuninori Morimoto

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

* Re: [PATCH 1/2] ASoC: compress: Correct handling of compressed ops
  2018-01-26  0:33     ` Kuninori Morimoto
@ 2018-01-26 10:03       ` Charles Keepax
  2018-01-26 11:48         ` Mark Brown
  0 siblings, 1 reply; 16+ messages in thread
From: Charles Keepax @ 2018-01-26 10:03 UTC (permalink / raw)
  To: Kuninori Morimoto
  Cc: vinod.koul, alsa-devel, broonie, lgirdwood, dan.carpenter

On Fri, Jan 26, 2018 at 12:33:53AM +0000, Kuninori Morimoto wrote:
> In existing drivers, the component which have compr_ops *was* originally
> "platform" component only.
> The main purpose why we want to replace platform (and codec) to component is that
> CPU/Codec/Platform categorize is no longer best match for modern device.
> 
> Thus, if we can replace all CPU/Codec/Platform into component, every device
> can have every feature, with no categorize !
> In other words, we don't know how many device have it.
> 

Yeah I think there is quite a lot more work to get to that point
though, we need to understand how we will present this to
user-space.

> But anyway, we don't have such situation, and we don't know how to adjust to it
> at this point.
> Thus Yes !! I can 100% agree your opinion.
> Thank you for your help
> 
> One note is that "rtd->platform" itself will be removed if all platforms were replaced.
> Then, soc_compr_get_component() / soc_compr_get_ops() might be merged or adjusted.
> 

Yes that was my thinking as well, once you remove that you should
only need some minor refactoring to make the updates.

Thanks,
Charles

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

* Re: [PATCH 1/2] ASoC: compress: Correct handling of compressed ops
  2018-01-26 10:03       ` Charles Keepax
@ 2018-01-26 11:48         ` Mark Brown
  0 siblings, 0 replies; 16+ messages in thread
From: Mark Brown @ 2018-01-26 11:48 UTC (permalink / raw)
  To: Charles Keepax
  Cc: vinod.koul, alsa-devel, dan.carpenter, lgirdwood, Kuninori Morimoto


[-- Attachment #1.1: Type: text/plain, Size: 702 bytes --]

On Fri, Jan 26, 2018 at 10:03:51AM +0000, Charles Keepax wrote:
> On Fri, Jan 26, 2018 at 12:33:53AM +0000, Kuninori Morimoto wrote:

> > Thus, if we can replace all CPU/Codec/Platform into component, every device
> > can have every feature, with no categorize !
> > In other words, we don't know how many device have it.

> Yeah I think there is quite a lot more work to get to that point
> though, we need to understand how we will present this to
> user-space.

I think some of this will sort itself out for the immediate future
through implementation choices in that we've not yet seen a system where
it'd make sense to combine multiple DAIs with compressed ops and don't
seem likely to right now.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]



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

end of thread, other threads:[~2018-01-26 11:49 UTC | newest]

Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-01-24 13:55 [PATCH 1/2] ASoC: compress: Correct handling of compressed ops Charles Keepax
2018-01-24 13:55 ` [PATCH 2/2] ASoC: compress: Fixup some minor style issues Charles Keepax
2018-01-24 14:33   ` Ladislav Michl
2018-01-24 14:50     ` Charles Keepax
2018-01-24 14:29 ` [PATCH 1/2] ASoC: compress: Correct handling of compressed ops Vinod Koul
2018-01-24 14:49   ` Charles Keepax
2018-01-24 15:33     ` Mark Brown
2018-01-24 16:00       ` Charles Keepax
2018-01-24 16:05         ` Mark Brown
2018-01-24 17:14           ` Charles Keepax
2018-01-25 11:03             ` Charles Keepax
2018-01-25  1:18 ` Kuninori Morimoto
2018-01-25  9:47   ` Charles Keepax
2018-01-26  0:33     ` Kuninori Morimoto
2018-01-26 10:03       ` Charles Keepax
2018-01-26 11:48         ` Mark Brown

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.