[PATCH] ASoC: soc-pcm: Revert "call snd_soc_component_open/close() once"
ASoC component open/close and snd_soc_component_module_get/put are called independently for each component-substream pair, so the logic in the reverted patch was not sufficient and led to PCM playback and module unload errors.
Fixes: dd03907bf129 ("ASoC: soc-pcm: call snd_soc_component_open/close() once") Signed-off-by: Kai Vehmanen kai.vehmanen@linux.intel.com --- include/sound/soc-component.h | 7 ++----- sound/soc/soc-component.c | 35 +++++++---------------------------- sound/soc/soc-pcm.c | 19 +++++++++++++------ 3 files changed, 22 insertions(+), 39 deletions(-)
diff --git a/include/sound/soc-component.h b/include/sound/soc-component.h index 1866ecc8e94b..154d02fbbfed 100644 --- a/include/sound/soc-component.h +++ b/include/sound/soc-component.h @@ -147,6 +147,8 @@ struct snd_soc_component {
unsigned int active;
+ unsigned int suspended:1; /* is in suspend PM state */ + struct list_head list; struct list_head card_aux_list; /* for auxiliary bound components */ struct list_head card_list; @@ -180,11 +182,6 @@ struct snd_soc_component { struct dentry *debugfs_root; const char *debugfs_prefix; #endif - - /* bit field */ - unsigned int suspended:1; /* is in suspend PM state */ - unsigned int opened:1; - unsigned int module:1; };
#define for_each_component_dais(component, dai)\ diff --git a/sound/soc/soc-component.c b/sound/soc/soc-component.c index ee00c09df5e7..14e175cdeeb8 100644 --- a/sound/soc/soc-component.c +++ b/sound/soc/soc-component.c @@ -297,55 +297,34 @@ EXPORT_SYMBOL_GPL(snd_soc_component_set_jack); int snd_soc_component_module_get(struct snd_soc_component *component, int upon_open) { - if (component->module) - return 0; - if (component->driver->module_get_upon_open == !!upon_open && !try_module_get(component->dev->driver->owner)) return -ENODEV;
- component->module = 1; - return 0; }
void snd_soc_component_module_put(struct snd_soc_component *component, int upon_open) { - if (component->module && - component->driver->module_get_upon_open == !!upon_open) + if (component->driver->module_get_upon_open == !!upon_open) module_put(component->dev->driver->owner); - - component->module = 0; }
int snd_soc_component_open(struct snd_soc_component *component, struct snd_pcm_substream *substream) { - int ret = 0; - - if (!component->opened && - component->driver->open) - ret = component->driver->open(component, substream); - - if (ret == 0) - component->opened = 1; - - return ret; + if (component->driver->open) + return component->driver->open(component, substream); + return 0; }
int snd_soc_component_close(struct snd_soc_component *component, struct snd_pcm_substream *substream) { - int ret = 0; - - if (component->opened && - component->driver->close) - ret = component->driver->close(component, substream); - - component->opened = 0; - - return ret; + if (component->driver->close) + return component->driver->close(component, substream); + return 0; }
int snd_soc_component_prepare(struct snd_soc_component *component, diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c index 63f67eb7c077..0c3a12d6290a 100644 --- a/sound/soc/soc-pcm.c +++ b/sound/soc/soc-pcm.c @@ -466,13 +466,16 @@ static void soc_pcm_init_runtime_hw(struct snd_pcm_substream *substream) hw->rate_max = min_not_zero(hw->rate_max, rate_max); }
-static int soc_pcm_components_open(struct snd_pcm_substream *substream) +static int soc_pcm_components_open(struct snd_pcm_substream *substream, + struct snd_soc_component **last) { struct snd_soc_pcm_runtime *rtd = substream->private_data; struct snd_soc_component *component; int i, ret = 0;
for_each_rtd_components(rtd, i, component) { + *last = component; + ret = snd_soc_component_module_get_when_open(component); if (ret < 0) { dev_err(component->dev, @@ -489,17 +492,21 @@ static int soc_pcm_components_open(struct snd_pcm_substream *substream) return ret; } } - + *last = NULL; return 0; }
-static int soc_pcm_components_close(struct snd_pcm_substream *substream) +static int soc_pcm_components_close(struct snd_pcm_substream *substream, + struct snd_soc_component *last) { struct snd_soc_pcm_runtime *rtd = substream->private_data; struct snd_soc_component *component; int i, r, ret = 0;
for_each_rtd_components(rtd, i, component) { + if (component == last) + break; + r = snd_soc_component_close(component, substream); if (r < 0) ret = r; /* use last ret */ @@ -536,7 +543,7 @@ static int soc_pcm_close(struct snd_pcm_substream *substream)
soc_rtd_shutdown(rtd, substream);
- soc_pcm_components_close(substream); + soc_pcm_components_close(substream, NULL);
snd_soc_dapm_stream_stop(rtd, substream->stream);
@@ -577,7 +584,7 @@ static int soc_pcm_open(struct snd_pcm_substream *substream)
mutex_lock_nested(&rtd->card->pcm_mutex, rtd->card->pcm_subclass);
- ret = soc_pcm_components_open(substream); + ret = soc_pcm_components_open(substream, &component); if (ret < 0) goto component_err;
@@ -682,7 +689,7 @@ static int soc_pcm_open(struct snd_pcm_substream *substream)
soc_rtd_shutdown(rtd, substream); component_err: - soc_pcm_components_close(substream); + soc_pcm_components_close(substream, component);
mutex_unlock(&rtd->card->pcm_mutex);
Hey,
On Wed, 19 Feb 2020, Kai Vehmanen wrote:
ASoC component open/close and snd_soc_component_module_get/put are called independently for each component-substream pair, so the logic in the reverted patch was not sufficient and led to PCM playback and module unload errors.
I tried to keep part of the original patch at first, but I kept hitting new issues either in component load, or in module unload-reload flow. Thanks to Pierre and Ranjani for early reviews.
So in the end I ended up with a full revert. This at least works on all SOF device topologies I tested with.
At the root of the problem is that component open is called with multiple substreams and driver open (and close) should be called for each substream as well. This also caused problems to the added module refcounting logic.. so that is reverted as well.
Br, Kai
On 2/19/20 12:53 PM, Kai Vehmanen wrote:
Hey,
On Wed, 19 Feb 2020, Kai Vehmanen wrote:
ASoC component open/close and snd_soc_component_module_get/put are called independently for each component-substream pair, so the logic in the reverted patch was not sufficient and led to PCM playback and module unload errors.
I tried to keep part of the original patch at first, but I kept hitting new issues either in component load, or in module unload-reload flow. Thanks to Pierre and Ranjani for early reviews.
So in the end I ended up with a full revert. This at least works on all SOF device topologies I tested with.
At the root of the problem is that component open is called with multiple substreams and driver open (and close) should be called for each substream as well. This also caused problems to the added module refcounting logic.. so that is reverted as well.
I also tried to find a fix in parallel with Kai's work, but no luck. First I am not sure why we need to add a 'module' state in addition to the existing module ref-counting, and the 'opened' state management looks too simple. revert-and-revisit seems like the best course of action indeed.
19.02.2020 21:26, Kai Vehmanen пишет:
ASoC component open/close and snd_soc_component_module_get/put are called independently for each component-substream pair, so the logic in the reverted patch was not sufficient and led to PCM playback and module unload errors.
Fixes: dd03907bf129 ("ASoC: soc-pcm: call snd_soc_component_open/close() once") Signed-off-by: Kai Vehmanen kai.vehmanen@linux.intel.com
Thanks,
Tested-by: Dmitry Osipenko digetx@gmail.com
Hi Kai, Pierre-Louis
Thank you for your test / review
ASoC component open/close and snd_soc_component_module_get/put are called independently for each component-substream pair, so the logic in the reverted patch was not sufficient and led to PCM playback and module unload errors.
Fixes: dd03907bf129 ("ASoC: soc-pcm: call snd_soc_component_open/close() once") Signed-off-by: Kai Vehmanen kai.vehmanen@linux.intel.com
OK, some component (SOF, and maybe DPCM too ?) want to be opened for each substreams. If so, indeed current code is not enough for it.
But, unfortunately I don't want spaghetti error handling code again. I think we can solve it if we can *count* open / module ? Can you please test this patch ?
--- 8< --- 8< --- 8< --- 8< --- 8< --- 8< --- 8< --- 8< --- 8< --- diff --git a/include/sound/soc-component.h b/include/sound/soc-component.h index 1866ecc8e94b..4e78925858c0 100644 --- a/include/sound/soc-component.h +++ b/include/sound/soc-component.h @@ -181,10 +181,11 @@ struct snd_soc_component { const char *debugfs_prefix; #endif
+ u8 opened; + u8 module; + /* bit field */ unsigned int suspended:1; /* is in suspend PM state */ - unsigned int opened:1; - unsigned int module:1; };
#define for_each_component_dais(component, dai)\ diff --git a/sound/soc/soc-component.c b/sound/soc/soc-component.c index ee00c09df5e7..a2526a1ffcbe 100644 --- a/sound/soc/soc-component.c +++ b/sound/soc/soc-component.c @@ -297,14 +297,16 @@ EXPORT_SYMBOL_GPL(snd_soc_component_set_jack); int snd_soc_component_module_get(struct snd_soc_component *component, int upon_open) { - if (component->module) - return 0; + if (unlikely(component->module == 0xff)) { + dev_warn(component->dev, "too many module get (%s)\n", component->name); + return -EBUSY; + }
if (component->driver->module_get_upon_open == !!upon_open && !try_module_get(component->dev->driver->owner)) return -ENODEV;
- component->module = 1; + component->module++;
return 0; } @@ -316,7 +318,7 @@ void snd_soc_component_module_put(struct snd_soc_component *component, component->driver->module_get_upon_open == !!upon_open) module_put(component->dev->driver->owner);
- component->module = 0; + component->module--; }
int snd_soc_component_open(struct snd_soc_component *component, @@ -324,12 +326,15 @@ int snd_soc_component_open(struct snd_soc_component *component, { int ret = 0;
- if (!component->opened && - component->driver->open) + if (unlikely(component->opened == 0xff)) { + dev_warn(component->dev, "too many open (%s)\n", component->name); + return -EBUSY; + } + + if (component->driver->open) ret = component->driver->open(component, substream);
- if (ret == 0) - component->opened = 1; + component->opened++;
return ret; } @@ -343,7 +348,7 @@ int snd_soc_component_close(struct snd_soc_component *component, component->driver->close) ret = component->driver->close(component, substream);
- component->opened = 0; + component->opened--;
return ret; }
Thank you for your help !! Best regards --- Kuninori Morimoto
ASoC component open/close and snd_soc_component_module_get/put are called once for each component, but we need it for each substream. To solve this issue, this patch counts open / get, and call close / put accordingly.
Fixes: dd03907bf129 ("ASoC: soc-pcm: call snd_soc_component_open/close() once") Reported-by: Kai Vehmanen kai.vehmanen@linux.intel.com Signed-off-by: Kuninori Morimoto kuninori.morimoto.gx@renesas.com --- I tidyuped code. I hope it can solve the issue.
include/sound/soc-component.h | 5 +++-- sound/soc/soc-component.c | 35 ++++++++++++++++++++++------------- 2 files changed, 25 insertions(+), 15 deletions(-)
diff --git a/include/sound/soc-component.h b/include/sound/soc-component.h index 1866ecc8e94b..4e78925858c0 100644 --- a/include/sound/soc-component.h +++ b/include/sound/soc-component.h @@ -181,10 +181,11 @@ struct snd_soc_component { const char *debugfs_prefix; #endif
+ u8 opened; + u8 module; + /* bit field */ unsigned int suspended:1; /* is in suspend PM state */ - unsigned int opened:1; - unsigned int module:1; };
#define for_each_component_dais(component, dai)\ diff --git a/sound/soc/soc-component.c b/sound/soc/soc-component.c index ee00c09df5e7..bdd36be1fb70 100644 --- a/sound/soc/soc-component.c +++ b/sound/soc/soc-component.c @@ -297,14 +297,16 @@ EXPORT_SYMBOL_GPL(snd_soc_component_set_jack); int snd_soc_component_module_get(struct snd_soc_component *component, int upon_open) { - if (component->module) - return 0; + if (unlikely(component->module == 0xff)) { + dev_warn(component->dev, "too many module get (%s)\n", component->name); + return -EBUSY; + }
if (component->driver->module_get_upon_open == !!upon_open && !try_module_get(component->dev->driver->owner)) return -ENODEV;
- component->module = 1; + component->module++;
return 0; } @@ -312,11 +314,13 @@ int snd_soc_component_module_get(struct snd_soc_component *component, void snd_soc_component_module_put(struct snd_soc_component *component, int upon_open) { - if (component->module && - component->driver->module_get_upon_open == !!upon_open) + if (!component->module) + return; + + if (component->driver->module_get_upon_open == !!upon_open) module_put(component->dev->driver->owner);
- component->module = 0; + component->module--; }
int snd_soc_component_open(struct snd_soc_component *component, @@ -324,12 +328,15 @@ int snd_soc_component_open(struct snd_soc_component *component, { int ret = 0;
- if (!component->opened && - component->driver->open) + if (unlikely(component->opened == 0xff)) { + dev_warn(component->dev, "too many open (%s)\n", component->name); + return -EBUSY; + } + + if (component->driver->open) ret = component->driver->open(component, substream);
- if (ret == 0) - component->opened = 1; + component->opened++;
return ret; } @@ -339,11 +346,13 @@ int snd_soc_component_close(struct snd_soc_component *component, { int ret = 0;
- if (component->opened && - component->driver->close) + if (!component->opened) + return; + + if (component->driver->close) ret = component->driver->close(component, substream);
- component->opened = 0; + component->opened--;
return ret; }
On Wed, Feb 19, 2020 at 5:01 PM Kuninori Morimoto < kuninori.morimoto.gx@renesas.com> wrote:
ASoC component open/close and snd_soc_component_module_get/put are called once for each component, but we need it for each substream. To solve this issue, this patch counts open / get, and call close / put accordingly.
Fixes: dd03907bf129 ("ASoC: soc-pcm: call snd_soc_component_open/close() once") Reported-by: Kai Vehmanen kai.vehmanen@linux.intel.com Signed-off-by: Kuninori Morimoto kuninori.morimoto.gx@renesas.com
I tidyuped code. I hope it can solve the issue.
include/sound/soc-component.h | 5 +++-- sound/soc/soc-component.c | 35 ++++++++++++++++++++++------------- 2 files changed, 25 insertions(+), 15 deletions(-)
diff --git a/include/sound/soc-component.h b/include/sound/soc-component.h index 1866ecc8e94b..4e78925858c0 100644 --- a/include/sound/soc-component.h +++ b/include/sound/soc-component.h @@ -181,10 +181,11 @@ struct snd_soc_component { const char *debugfs_prefix; #endif
u8 opened;u8 module;/* bit field */ unsigned int suspended:1; /* is in suspend PM state */
unsigned int opened:1;unsigned int module:1;};
#define for_each_component_dais(component, dai)\ diff --git a/sound/soc/soc-component.c b/sound/soc/soc-component.c index ee00c09df5e7..bdd36be1fb70 100644 --- a/sound/soc/soc-component.c +++ b/sound/soc/soc-component.c @@ -297,14 +297,16 @@ EXPORT_SYMBOL_GPL(snd_soc_component_set_jack); int snd_soc_component_module_get(struct snd_soc_component *component, int upon_open) {
if (component->module)return 0;
if (unlikely(component->module == 0xff)) {dev_warn(component->dev, "too many module get (%s)\n",component->name);
return -EBUSY;} if (component->driver->module_get_upon_open == !!upon_open && !try_module_get(component->dev->driver->owner)) return -ENODEV;
component->module = 1;
component->module++;
Thanks, Morimoto-san for the alternate fix. I understand the rationale for having a count for component->opened, but what is the rationale for the module count? What it is intended to protect?
Thanks, Ranjani
Hi Sridharan
ASoC component open/close and snd_soc_component_module_get/put are called once for each component, but we need it for each substream. To solve this issue, this patch counts open / get, and call close / put accordingly. Fixes: dd03907bf129 ("ASoC: soc-pcm: call snd_soc_component_open/close() once") Reported-by: Kai Vehmanen <kai.vehmanen@linux.intel.com> Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com> ---
(snip)
@@ -297,14 +297,16 @@ EXPORT_SYMBOL_GPL(snd_soc_component_set_jack); int snd_soc_component_module_get(struct snd_soc_component *component, int upon_open) { - if (component->module) - return 0; + if (unlikely(component->module == 0xff)) { + dev_warn(component->dev, "too many module get (%s)\n", component->name); + return -EBUSY; + } if (component->driver->module_get_upon_open == !!upon_open && !try_module_get(component->dev->driver->owner)) return -ENODEV; - component->module = 1; + component->module++;Thanks, Morimoto-san for the alternate fix. I understand the rationale for having a count for component->opened, but what is the rationale for the module count? What it is intended to protect?
I think same as open ? It protects calling put() from not-get-component. Because module_put() has WARN_ON(ret < 0).
Thank you for your help !! Best regards --- Kuninori Morimoto
On Wed, Feb 19, 2020 at 5:42 PM Kuninori Morimoto < kuninori.morimoto.gx@renesas.com> wrote:
Hi Sridharan
ASoC component open/close and snd_soc_component_module_get/put are called once for each component, but we need it for each substream. To solve this issue, this patch counts open / get, and call close / put accordingly. Fixes: dd03907bf129 ("ASoC: soc-pcm: callsnd_soc_component_open/close() once")
Reported-by: Kai Vehmanen <kai.vehmanen@linux.intel.com> Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com> ---(snip)
@@ -297,14 +297,16 @@ EXPORT_SYMBOL_GPL(snd_soc_component_set_jack); int snd_soc_component_module_get(struct snd_soc_component*component,
int upon_open) { - if (component->module) - return 0; + if (unlikely(component->module == 0xff)) { + dev_warn(component->dev, "too many module get(%s)\n", component->name);
+ return -EBUSY; + } if (component->driver->module_get_upon_open == !!upon_open && !try_module_get(component->dev->driver->owner)) return -ENODEV; - component->module = 1; + component->module++;Thanks, Morimoto-san for the alternate fix. I understand the rationale
for having a count for component->opened, but what is the rationale for the module count? What it is
intended to protect?
I think same as open ? It protects calling put() from not-get-component. Because module_put() has WARN_ON(ret < 0).
Can we use the module_refcount instead of adding a new field?
Thanks, Ranjani
Hi Sridharan
> ASoC component open/close and snd_soc_component_module_get/put are > called once for each component, but we need it for each substream. > To solve this issue, this patch counts open / get, > and call close / put accordingly. > > Fixes: dd03907bf129 ("ASoC: soc-pcm: call snd_soc_component_open/close() once") > Reported-by: Kai Vehmanen <kai.vehmanen@linux.intel.com> > Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com> > ---
(snip)
I think same as open ? It protects calling put() from not-get-component. Because module_put() has WARN_ON(ret < 0).Can we use the module_refcount instead of adding a new field?
Ahh, maybe yes. I can try to use it in non-RFC patch.
Thank you for your help !! Best regards --- Kuninori Morimoto
Hi,
On Thu, 20 Feb 2020, Kuninori Morimoto wrote:
ASoC component open/close and snd_soc_component_module_get/put are called independently for each component-substream pair, so the logic in the reverted patch was not sufficient and led to PCM playback and module unload errors.
But, unfortunately I don't want spaghetti error handling code again. I think we can solve it if we can *count* open / module ? Can you please test this patch ?
I tested and this version works as well, so: Reviewed-by: Kai Vehmanen kai.vehmanen@linux.intel.com
... but, but, I have some doubts about th "opened" tracking as a solution.
A single counter will track the overall number of component-substream combinations, but if we have bugs in calling code and e.g. same component-substream is passed multiple times to open or close, the the single counter will go out of sync.
So if the primary problem is the messy rollback in case one open fails, what if we do the rollback directly in soc_pcm_components_open() and do not add any additional tracking..?
Let me send a proposal patch for that.
Br, Kai
Hi Kai
Thank you for your help
But, unfortunately I don't want spaghetti error handling code again. I think we can solve it if we can *count* open / module ? Can you please test this patch ?
I tested and this version works as well, so: Reviewed-by: Kai Vehmanen kai.vehmanen@linux.intel.com
... but, but, I have some doubts about th "opened" tracking as a solution.
A single counter will track the overall number of component-substream combinations, but if we have bugs in calling code and e.g. same component-substream is passed multiple times to open or close, the the single counter will go out of sync.
So if the primary problem is the messy rollback in case one open fails, what if we do the rollback directly in soc_pcm_components_open() and do not add any additional tracking..?
Let me send a proposal patch for that.
Hmm... It seems the patch was not so good cleanup...
Thank you for your proposal patch. I checked it. But, if it rollback when error with *last, my opinion is original code (= soc_pcm_components_close() needs *last parameter) (= same as just revert the patch) is better.
Thank you for your help !! Best regards --- Kuninori Morimoto
Hello Morimoto-san,
On Fri, 21 Feb 2020, Kuninori Morimoto wrote:
Kai Vehmanen wrote:
So if the primary problem is the messy rollback in case one open fails, what if we do the rollback directly in soc_pcm_components_open() and do not add any additional tracking..?
Let me send a proposal patch for that.
It seems the patch was not so good cleanup...
Thank you for your proposal patch. I checked it. But, if it rollback when error with *last, my opinion is original code (= soc_pcm_components_close() needs *last parameter) (= same as just revert the patch) is better.
well, I do think the original code could still need a cleanup, so the effort is very much welcome. Having to pass the "last" parameter to components_open() just so that we can clean up if errors happen, seems a bit out of place.
But yeah, easier said than done. We have now three alternatives to solve this:
1. do the cleanup locally in soc_pcm_components_open() [PATCH] ASoC: soc-pcm: fix state tracking error in snd_soc_component_open/close()
2. revert to original implementation with "last" passed to components_open() [PATCH] ASoC: soc-pcm: Revert "call snd_soc_component_open/close() once"
3. implement opened/module counters [PATCH][RFC] ASoC: soc-component: count snd_soc_component_open/close()
... I have tested all three and they seem to work.
I still don't really like (1), but I agree (2) adds more code in total. I worry (3) might backfire with some component-substream combinations.
So maybe we'll go with (1)? We do need to merge one of them, because current merged code is broken on many platforms.
Br, Kai
Hi Kai
Thank you for your feedback / test / review
do the cleanup locally in soc_pcm_components_open() [PATCH] ASoC: soc-pcm: fix state tracking error in snd_soc_component_open/close()
revert to original implementation with "last" passed to components_open() [PATCH] ASoC: soc-pcm: Revert "call snd_soc_component_open/close() once"
implement opened/module counters [PATCH][RFC] ASoC: soc-component: count snd_soc_component_open/close()
... I have tested all three and they seem to work.
I still don't really like (1), but I agree (2) adds more code in total. I worry (3) might backfire with some component-substream combinations.
So maybe we'll go with (1)? We do need to merge one of them, because current merged code is broken on many platforms.
It seems Mark accepted (1), and I have no big objection about it. Thank you for your hard work
Thank you for your help !! Best regards --- Kuninori Morimoto
Hi Kai, again Cc Mark
Maybe this is too late, but I want to tell the reason why I wanted to add flag on each component.
do the cleanup locally in soc_pcm_components_open() [PATCH] ASoC: soc-pcm: fix state tracking error in snd_soc_component_open/close()
revert to original implementation with "last" passed to components_open() [PATCH] ASoC: soc-pcm: Revert "call snd_soc_component_open/close() once"
implement opened/module counters [PATCH][RFC] ASoC: soc-component: count snd_soc_component_open/close()
I can see this kind of implementation at many place.
For example, soc_pcm_open() has error handling. But, it is same as soc_pcm_close(), but it can't call it directly, because it needs to care about "successed until where" for now.
If each component / rtd / dai have "done" flag or count, soc_pcm_open() can call soc_pcm_close() directly without thinking about "until", because each flag can handle/indicate it.
The good point is we can reduce duplicate implementation. And it can avoid future bug. Because today, we need to care both soc_pcm_close() and error handling in soc_pcm_open(), it is not good for me.
static int soc_pcm_open(struct snd_pcm_substream *substream) { ... return 0;
/* * From here, "implementation" is same as soc_pcm_close() */
config_err: for_each_rtd_dais(rtd, i, dai) snd_soc_dai_shutdown(dai, substream);
soc_rtd_shutdown(rtd, substream); rtd_startup_err: soc_pcm_components_close(substream, NULL); component_err: mutex_unlock(&rtd->card->pcm_mutex);
for_each_rtd_components(rtd, i, component) { pm_runtime_mark_last_busy(component->dev); pm_runtime_put_autosuspend(component->dev); }
for_each_rtd_components(rtd, i, component) if (!component->active) pinctrl_pm_select_sleep_state(component->dev);
return ret; }
Thank you for your help !! Best regards --- Kuninori Morimoto
On Wed, Feb 26, 2020 at 09:55:47AM +0900, Kuninori Morimoto wrote:
If each component / rtd / dai have "done" flag or count, soc_pcm_open() can call soc_pcm_close() directly without thinking about "until", because each flag can handle/indicate it.
The good point is we can reduce duplicate implementation. And it can avoid future bug. Because today, we need to care both soc_pcm_close() and error handling in soc_pcm_open(), it is not good for me.
That goal definitely makes sense, if we can avoid problems like the ones here it seems like a useful change.
Hi Mark
If each component / rtd / dai have "done" flag or count, soc_pcm_open() can call soc_pcm_close() directly without thinking about "until", because each flag can handle/indicate it.
The good point is we can reduce duplicate implementation. And it can avoid future bug. Because today, we need to care both soc_pcm_close() and error handling in soc_pcm_open(), it is not good for me.
That goal definitely makes sense, if we can avoid problems like the ones here it seems like a useful change.
Thank you for your feedback. The one big headache is that, as Kai mentioned, how to know the "handled substream". We need list or array, but... I will postpone this kind of patches now, and post other patches. And I will re-think about it again after that.
Thank you for your help !! Best regards --- Kuninori Morimoto
Hello Morimoto-san,
On Wed, 26 Feb 2020, Kuninori Morimoto wrote:
Maybe this is too late, but I want to tell the reason why I wanted to add flag on each component.
[...]
If each component / rtd / dai have "done" flag or count, soc_pcm_open() can call soc_pcm_close() directly without thinking about "until", because each flag can handle/indicate it.
thanks for the longer explanation. It's true we have a lot of code with for_each_xxx() loops, and loop logic where errors can occur. It seems the most common approach is to store the index and use for_each_xxx_rollback() macros to clean up in case error happens. This does lead to the problem of essentially duplicated logic e.g. for soc_pcm_close() and error handling of snd_pcm_open(). And yeah, it's also a bit error prone as the logic is not exactly the same. E.g. I'm not convinced this is quite right in soc_pcm_open():
» for_each_rtd_codec_dai(rtd, i, codec_dai) { » » ret = snd_soc_dai_startup(codec_dai, substream); » » if (ret < 0) { » » » dev_err(codec_dai->dev, » » » » "ASoC: can't open codec %s: %d\n", » » » » codec_dai->name, ret); » » » goto config_err; » » } ... config_err: » for_each_rtd_codec_dai(rtd, i, codec_dai) » » snd_soc_dai_shutdown(codec_dai, substream); » i = rtd->num_cpus;
... i.e. we should use _rollback() macro here, but we don't so shutdown is called on all codec dais if I read this right.
Having a single cleanup path would be easier, but I think in the end it comes down to how cleanly you can track the opened state. It seems biggest issue is how to cleanly track the component-substream pairs. Ideally we'd have a dedicated state object to represent an opened component-substream pair, but that's not how the APIs are defined now. But something to that effect is needed.
Br, Kai
Hi Kai
Thank you for feedback
thanks for the longer explanation. It's true we have a lot of code with for_each_xxx() loops, and loop logic where errors can occur. It seems the most common approach is to store the index and use for_each_xxx_rollback() macros to clean up in case error happens. This does lead to the problem of essentially duplicated logic e.g. for soc_pcm_close() and error handling of snd_pcm_open().
Thank you for understanding my headache. My opinion is that complex duplicated code never bring luck to us.
And yeah, it's also a bit error prone as the logic is not exactly the same. E.g. I'm not convinced this is quite right in soc_pcm_open():
» for_each_rtd_codec_dai(rtd, i, codec_dai) { » » ret = snd_soc_dai_startup(codec_dai, substream); » » if (ret < 0) { » » » dev_err(codec_dai->dev, » » » » "ASoC: can't open codec %s: %d\n", » » » » codec_dai->name, ret); » » » goto config_err; » » } ... config_err: » for_each_rtd_codec_dai(rtd, i, codec_dai) » » snd_soc_dai_shutdown(codec_dai, substream); » i = rtd->num_cpus;
Yeah indeed. I think this is just one of the BUG. I guess we can find similar issue everywhere.
Having a single cleanup path would be easier, but I think in the end it comes down to how cleanly you can track the opened state. It seems biggest issue is how to cleanly track the component-substream pairs. Ideally we'd have a dedicated state object to represent an opened component-substream pair, but that's not how the APIs are defined now. But something to that effect is needed.
Yeah, I can understand your concern, but not 100% yet. In my understanding, counting start vs stop is not enough but not so bad. If my understanding was correct, your concern is counting only is not enough, because wrong component-substream pair can be used, like this ?
start(substream-A); <= start(substream-B); start(substream-C);
stop(substream-Z); <= stop(substream-B); stop(substream-C);
But I wonder is it really happen ? If there is clear such case, yes, we need to consider component-substream pair list, somehow.
If you are worry about some kind of BUG, I guess we need to solve is such bug, not error handling method. But how about step-by-step approach (I like it :) ? At first, add counting method as 1st step. Indeed it is not enough, but we can cleanup error handling. If we found/noticed above not-enough-case, start to consider about component-substream pair list ?
Thank you for your help !! Best regards --- Kuninori Morimoto
Hi Kai, again
Having a single cleanup path would be easier, but I think in the end it comes down to how cleanly you can track the opened state. It seems biggest issue is how to cleanly track the component-substream pairs. Ideally we'd have a dedicated state object to represent an opened component-substream pair, but that's not how the APIs are defined now. But something to that effect is needed.
Yeah, I can understand your concern, but not 100% yet. In my understanding, counting start vs stop is not enough but not so bad. If my understanding was correct, your concern is counting only is not enough, because wrong component-substream pair can be used, like this ?
start(substream-A); <= start(substream-B); start(substream-C);
stop(substream-Z); <= stop(substream-B); stop(substream-C);
But I wonder is it really happen ?
Ohh, yes indeed !! I was confused. But Hmm... I don't want to have substream list on each component... Hmm... I will re-consider it again.
Thank you for your help !! Best regards --- Kuninori Morimoto
Hi Kai, again, and, again
start(substream-A); <= start(substream-B); start(substream-C);
stop(substream-Z); <= stop(substream-B); stop(substream-C);
(snip)
Ohh, yes indeed !! I was confused. But Hmm... I don't want to have substream list on each component... Hmm... I will re-consider it again.
I don't want to have substream list on each components, and keep simple code as much as possible.
My current idea is using ID. What do you think ? It is not super simple though...
int soc_pcm_components_open(struct snd_pcm_substream *substream, u8 id) { int ret = 0;
/* * Add ID to each component to know "which open". */ for_each_rtd_components(rtd, i, component) { if (component->driver->open) { ret = component->driver->open(component, substream); if (ret < 0) return ret;
component->open_id = id; /* add ID */ } }
return 0; }
int soc_pcm_components_close(struct snd_pcm_substream *substream, u8 id) { /* * if ID > 0, it is only target. * if ID == 0, all components are the target */ for_each_rtd_components(rtd, i, component) { if ((id == 0 || id == component->open_id) && component->driver->close) component->driver->close(component, substream); } ... }
=> int soc_pcm_clear(..., u8 id) { ... /* * if ID > 0, it is only target. * if ID == 0, all components are the target */ soc_pcm_components_close(substream, id); ... }
int soc_pcm_close(...) { /* * ID = 0 * All components are target of close */ => soc_pcm_clear(xxx, 0); }
int soc_pcm_open(...) { static u8 id;
/* update ID */ id++; if (id == 0) id++;
... ret = soc_pcm_components_open(substream, id); if (ret < 0) goto open_err; ...
return 0; /* success */
open_err: /* * ID = id * Only this IDs are the target */ => soc_pcm_clear(xxx, id)
return ret; }
Thank you for your help !! Best regards --- Kuninori Morimoto
Hey,
catching up with the thread :)
On Fri, 28 Feb 2020, Kuninori Morimoto wrote:
start(substream-A); <= start(substream-B); start(substream-C);
stop(substream-Z); <= stop(substream-B); stop(substream-C);
[snip]
I don't want to have substream list on each components, and keep simple code as much as possible.
[snip]
My current idea is using ID. What do you think ? It is not super simple though...
Hmm, I think then we end up with new problems managing the IDs. Specifically:
int soc_pcm_open(...) { static u8 id;
/* update ID */ id++; if (id == 0) id++;
... this really isn't solid. If you have a complex scenario and something goes wrong, debugging the ids is going to be painful if they are assigned this way.
I think in the end we should go back to this:
int snd_soc_component_open(struct snd_soc_component *component, » » » struct snd_pcm_substream *substream)
... this essentially creates new state by assigning a new substream to the component, and we should explicitly track it. I know you wanted to avoid this, but I think in the end it's the cleanest solution and aligned to rest of ALSA. Aside cleaning up implementation of close(), this will help also in other methods, like e.g.:
int snd_soc_component_prepare(struct snd_soc_component *component, » » » struct snd_pcm_substream *substream) { » if (component->driver->prepare) » » return component->driver->prepare(component, substream); » return 0; }
.. if prepare() is called with a substream that is not opened for this component, we could catch it here if we were tracking substreams.
Br, Kai
Hi Kai
Thank you for your feedback
int snd_soc_component_open(struct snd_soc_component *component, » » » struct snd_pcm_substream *substream)
(snip)
int snd_soc_component_prepare(struct snd_soc_component *component, » » » struct snd_pcm_substream *substream) { » if (component->driver->prepare) » » return component->driver->prepare(component, substream); » return 0; }
I guess you are thinking more big scale tracking/solution (?). Indeed it is needed, but my indicated one is not for it. It is just for "we want to use soc_pcm_close() as soc_pcm_open() error handling".
int soc_pcm_open(...) { static u8 id;
/* update ID */ id++; if (id == 0) id++;... this really isn't solid. If you have a complex scenario and something goes wrong, debugging the ids is going to be painful if they are assigned this way.
Maybe the naming of "ID" makes you confused ? It is just "mark" for this "soc_pcm_open()". If error happen during open, "error handling soc_pcm_close()" cares only this mark. It is just for avoiding mismatch close.
Your big scale tracking (open/prepare/...) is maybe next step / more advanced problem. My got feeling is that it is similar to SND_SOC_DPCM_STATE_xxx ?
Thank you for your help !! Best regards --- Kuninori Morimoto
Hi,
On Mon, 2 Mar 2020, Kuninori Morimoto wrote:
I guess you are thinking more big scale tracking/solution (?). Indeed it is needed, but my indicated one is not for it. It is just for "we want to use soc_pcm_close() as soc_pcm_open() error handling".
yes, I'm thinking it would be better to do proper tracking and not do an intermediate solution with IDs.
int soc_pcm_open(...) { static u8 id;
/* update ID */ id++; if (id == 0) id++;Maybe the naming of "ID" makes you confused ? It is just "mark" for this "soc_pcm_open()". If error happen during open, "error handling soc_pcm_close()" cares only this mark. It is just for avoiding mismatch close.
Yes, the main issues I see: - the name (maybe "open_tag" would be better than "open_id"), - declaration of the id with "static u8 id" -- if multiple unrelated streams are opened concurrently, the id management needs to be handled in a thread safe way, - the "component->open_id" field only refers to the last substream open -- i.e. field is only relevant in contact of error rollbacks
The "new id" really just refers to the substream being opened, so you could create it from the substream pointer as well. For error rollback, you want to close all components of the substream being opened. But this is still a bit unelegant as you'd end up storing the last substream opened to component struct (3rd bullet above).
I was thinking more in the lines of:
/* add list of opened substreams to snd_soc_component */ struct snd_soc_component { ... struct list_head substream_list;
int snd_soc_component_open(struct snd_soc_component *component, » » » struct snd_pcm_substream *substream) { int res = 0; if (component->driver->open) res = component->driver->open(component, substream);
/* on success, add proxy of substream to component->substream_list */ ...
int snd_soc_component_close(struct snd_soc_component *component, » » » struct snd_pcm_substream *substream) { /* * lookup substream from "component->substream_List", * only call driver->close() if found */ ...
... this is arguably more code, but makes the state created in snd_soc_component_open() explicit. Upon error in middle of components_open(), one can just call soc_pcm_components_close() which will close all components for that substream (that had been succesfully opened).
Br, Kai
Hi Kai
Thank you for your feedback
/* add list of opened substreams to snd_soc_component */ struct snd_soc_component { ... struct list_head substream_list;
int snd_soc_component_open(struct snd_soc_component *component, » » » struct snd_pcm_substream *substream) { int res = 0; if (component->driver->open) res = component->driver->open(component, substream);
/* on success, add proxy of substream to component->substream_list */...
int snd_soc_component_close(struct snd_soc_component *component, » » » struct snd_pcm_substream *substream) { /* * lookup substream from "component->substream_List", * only call driver->close() if found */ ...
... this is arguably more code, but makes the state created in snd_soc_component_open() explicit. Upon error in middle of components_open(), one can just call soc_pcm_components_close() which will close all components for that substream (that had been succesfully opened).
k Yes, totally agree to your code. But, 1 point I still not yet understand.
close() will be called 1) when open failed, or, 2) normal close. If my understanding was correct, your code is caring that 2) normal close() might be called without open().
I don't think it happen, but are you worrying that it might be happen by some BUG, or for some accident ? If so, I can 100% understand your idea.
Thank you for your help !! Best regards --- Kuninori Morimoto
Hey,
On Tue, 3 Mar 2020, Kuninori Morimoto wrote:
Kai Vehmanen wrote:
int snd_soc_component_close(struct snd_soc_component *component, » » » struct snd_pcm_substream *substream) { /* * lookup substream from "component->substream_List", * only call driver->close() if found */ ...
... this is arguably more code, but makes the state created in snd_soc_component_open() explicit. Upon error in middle of
But, 1 point I still not yet understand.
close() will be called 1) when open failed, or, 2) normal close. If my understanding was correct, your code is caring that 2) normal close() might be called without open().
it also covers case (1) when open fails. So instead of the currently merged error case rollback code in soc_pcm_components_open(), we'd just call snd_pcm_components_close(substream) directly in case of error. With tracking of opened substreams in soc-component.c, close() is safe to call also in error case.
But you are right, I don't really see how (2) could be hit, so we are essentially talking about how to avoid the <10 lines of rollback code in soc_pcm_components_open(). :)
That considered, I'm fine if you can come up with a cleaner version to handle just case (1), without tracking substreams. Maybe worth a try and if it doesn't work (e.g. with ID/tag), we can look at substream tracking.
Br, Kai
Hi Kai
Thank you for your feedback
close() will be called 1) when open failed, or, 2) normal close. If my understanding was correct, your code is caring that 2) normal close() might be called without open().
it also covers case (1) when open fails. So instead of the currently merged error case rollback code in soc_pcm_components_open(), we'd just call snd_pcm_components_close(substream) directly in case of error. With tracking of opened substreams in soc-component.c, close() is safe to call also in error case.
Oh, yes, of course. I'm sorry to you for my lack of words. My question was "My idea cares open-close mismatch only for 1). and your's cared both 1) and 2) ?", I mean.
That considered, I'm fine if you can come up with a cleaner version to handle just case (1), without tracking substreams. Maybe worth a try and if it doesn't work (e.g. with ID/tag), we can look at substream tracking.
I see. Let's use step-by-step approach. I will try to care 1) as 1st step. If it was not enough, or, if we want to have tracker, let's update it by your idea as 2nd step.
Thank you for your help !!
Best regards --- Kuninori Morimoto
participants (6)
-
Dmitry Osipenko -
Kai Vehmanen -
Kuninori Morimoto -
Mark Brown -
Pierre-Louis Bossart -
Sridharan, Ranjani