| Message ID | 87v9i8ku04.wl-kuninori.morimoto.gx@renesas.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ASoC: merge soc_pcm_open() rollback and soc_pcm_close() | expand |
Hi Morimoto-san, > +#define soc_dai_mark_push(dai, substream, tgt) ((dai)->mark_##tgt = substream) we may want a check that detects if the pointer is NULL before assigning it, otherwise we won't be able to detect bad configuration where a pointer is overwritten by 2 mark_push() calls on the same object? > +#define soc_dai_mark_pop(dai, substream, tgt) ((dai)->mark_##tgt = NULL) > +#define soc_dai_mark_match(dai, substream, tgt) ((dai)->mark_##tgt == substream) > + > /** > * snd_soc_dai_set_sysclk - configure DAI system or master clock. > * @dai: DAI > @@ -348,15 +356,26 @@ int snd_soc_dai_startup(struct snd_soc_dai *dai, > dai->driver->ops->startup) > ret = dai->driver->ops->startup(substream, dai); > > + /* mark substream if succeeded */ > + if (ret == 0) > + soc_dai_mark_push(dai, substream, startup); > + I am a bit concerned here about the case of a bi-directional DAI, it's my understanding that the .startup() callback could be called for each direction? soc-dapm.c: ret = snd_soc_dai_startup(source, substream); soc-dapm.c: ret = snd_soc_dai_startup(sink, substream); To convince myself of this, I added a dummy startup routine and I do see it called when I do playback and capture at the same time: [ 179.057494] plb: ssp2 startup stream 0 [ 183.976963] plb: ssp2 startup stream 1 That makes me nervous about having a single pointer and unbalanced calls between startup and shutdown. We had such issues in the past so I may be on the paranoid side here... Thanks -Pierre
On Tue, 2020-07-28 at 15:57 +0900, Kuninori Morimoto wrote: > From: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com> > > soc_pcm_open() does rollback when failed (A), > but, it is almost same as soc_pcm_close(). > > static int soc_pcm_open(xxx) > { > ... > if (ret < 0) > goto xxx_err; > ... > return 0; > > ^ config_err: > | ... > | rtd_startup_err: > (A) ... > | component_err: > | ... > v return ret; > } > > The difference is > soc_pcm_close() is for all dai/component/substream, > rollback is for succeeded part only. > > This kind of duplicated code can be a hotbed of bugs, > thus, we want to share soc_pcm_close() and rollback. > > Now, soc_pcm_open/close() are handling > => 1) snd_soc_dai_startup/shutdown() > 2) snd_soc_link_startup/shutdown() > 3) snd_soc_component_module_get/put() > 4) snd_soc_component_open/close() > 5) pm_runtime_put/get() > > This patch is for 1) snd_soc_dai_startup/shutdown(), > and adds new substream mark. > It will mark substream when startup was suceeded. > If rollback happen *after* that, it will check rollback flag > and marked substream. > > It cares *previous* startup() only now, > but we might want to check *whole* marked substream in the future. > This patch is using macro so that it can be easily adjust to it. > > Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com> > --- > include/sound/soc-dai.h | 5 ++++- > sound/soc/soc-dai.c | 21 ++++++++++++++++++++- > sound/soc/soc-dapm.c | 4 ++-- > sound/soc/soc-pcm.c | 4 ++-- > 4 files changed, 28 insertions(+), 6 deletions(-) > > diff --git a/include/sound/soc-dai.h b/include/sound/soc-dai.h > index 776a60529e70..86411f503c1c 100644 > --- a/include/sound/soc-dai.h > +++ b/include/sound/soc-dai.h > @@ -153,7 +153,7 @@ void snd_soc_dai_hw_free(struct snd_soc_dai *dai, > int snd_soc_dai_startup(struct snd_soc_dai *dai, > struct snd_pcm_substream *substream); > void snd_soc_dai_shutdown(struct snd_soc_dai *dai, > - struct snd_pcm_substream *substream); > + struct snd_pcm_substream *substream, int > rollback); > snd_pcm_sframes_t snd_soc_dai_delay(struct snd_soc_dai *dai, > struct snd_pcm_substream > *substream); > void snd_soc_dai_suspend(struct snd_soc_dai *dai); > @@ -388,6 +388,9 @@ struct snd_soc_dai { > > struct list_head list; > > + /* function mark */ > + struct snd_pcm_substream *mark_startup; > + > /* bit field */ > unsigned int probed:1; > }; > diff --git a/sound/soc/soc-dai.c b/sound/soc/soc-dai.c > index 693893420bf0..4f9f73800ab0 100644 > --- a/sound/soc/soc-dai.c > +++ b/sound/soc/soc-dai.c > @@ -32,6 +32,14 @@ static inline int _soc_dai_ret(struct snd_soc_dai > *dai, > return ret; > } > > +/* > + * We might want to check substream by using list. > + * In such case, we can update these macros. > + */ > +#define soc_dai_mark_push(dai, substream, tgt) ((dai)- > >mark_##tgt = substream) > +#define soc_dai_mark_pop(dai, substream, tgt) ((dai)- > >mark_##tgt = NULL) > +#define soc_dai_mark_match(dai, substream, tgt) ((dai)- > >mark_##tgt == substream) > + > /** > * snd_soc_dai_set_sysclk - configure DAI system or master clock. > * @dai: DAI > @@ -348,15 +356,26 @@ int snd_soc_dai_startup(struct snd_soc_dai > *dai, > dai->driver->ops->startup) > ret = dai->driver->ops->startup(substream, dai); > > + /* mark substream if succeeded */ > + if (ret == 0) > + soc_dai_mark_push(dai, substream, startup); > + > return soc_dai_ret(dai, ret); > } > > void snd_soc_dai_shutdown(struct snd_soc_dai *dai, > - struct snd_pcm_substream *substream) > + struct snd_pcm_substream *substream, > + int rollback) > { > + if (rollback && !soc_dai_mark_match(dai, substream, startup)) > + return; Morimoto-san, Im having a hard time undersdtanding why we need the second check? When will it ever be the case that this match will fail? I think if we want to roll-back in case of errors , we could simply have a bool in snd_soc_dai to indicate that the dai has been started up already and check that here to decide whether we should shut it down or not. Ideally, a helper function like snd_soc_dai_shutdown_all() which iterates through all the rtd dai's and shuts all of them that are marked as started up should suffice and will be more intuitive. Also, push/pop are associated with stacks and we're only really dealing with one object here. So it is a bit misleading nomemclature-wise. What do you think? Thanks, Ranjani
Hi Ranjani Thank you for your review > > void snd_soc_dai_shutdown(struct snd_soc_dai *dai, > > - struct snd_pcm_substream *substream) > > + struct snd_pcm_substream *substream, > > + int rollback) > > { > > + if (rollback && !soc_dai_mark_match(dai, substream, startup)) > > + return; > Morimoto-san, > Im having a hard time undersdtanding why we need the second check? When > will it ever be the case that this match will fail? for_each_rtd_dais(...) { ret = snd_soc_dai_startup(dai, substream); if (ret < 0) goto config_err; ... } ... config_err: for_each_rtd_dais(rtd, i, dai) snd_soc_dai_shutdown(dai, substream, 1); For example, if rtd had 10 DAIs, and .startup() failed at 3rd DAI, the mark will be /* xxx here means unknown */ dai[0].mark_startup = substream; dai[1].mark_startup = substream; dai[2].mark_startup = substream; dai[3].mark_startup = xxx; ... dai[7].mark_startup = xxx; dai[8].mark_startup = xxx; dai[9].mark_startup = xxx; Then, we need to call .shutdown() is for dai[0] - dai[2], In such case, .shutdown() will be called with rollback flag. if's second check will be failed on dai[3] - dai[9]. But please double check. > I think if we want to roll-back in case of errors , we could simply > have a bool in snd_soc_dai to indicate that the dai has been started up > already and check that here to decide whether we should shut it down or > not. Yes, my v1 patch was that. But someone (I don't remember who was he) indicated that it is not enough if same DAI was called many times. In my understanding, for example, if same DAI is used for 2xPlaybacks, and 1st Playback was successed, 2nd Playback was failed, and if it was using bool instead of pointer, 2nd Playback rollback doesn't need to call shutdown, but it has successed bit of 1st Playback. Then, 2nd Playback rollback will call unneeded shutdown, and 1st Playback's necessary shutdown will not be called. We can avoid such things if we use pointer instead of bool. But please double check. > Also, push/pop are associated with stacks and we're only really dealing > with one object here. So it is a bit misleading nomemclature-wise. Yes maybe. The reason why I used push/pop was that it might be updated to handle pointer list instead of single pointer (and maybe Pierre-Louis want it :). I think I indicated it on git log and comment. Thank you for your help !! Best regards --- Kuninori Morimoto
Hi Pierre-Louis Thank you for your review. > > +#define soc_dai_mark_push(dai, substream, tgt) ((dai)->mark_##tgt = substream) > > we may want a check that detects if the pointer is NULL before > assigning it, otherwise we won't be able to detect bad configuration > where a pointer is overwritten by 2 mark_push() calls on the same > object? One assumption here is that open() / close() pair are called same number of times. open() / close() unbalance is not mentioned here, it is other problem. My expectation for this mark is that it will be used only for rollback. The necessary things in such case is that marked pointer is match to current pointer, or not when rollback case. Thus, overwritten is not problem in my understanding. > I am a bit concerned here about the case of a bi-directional DAI, it's > my understanding that the .startup() callback could be called for each > direction? > > soc-dapm.c: ret = snd_soc_dai_startup(source, substream); > soc-dapm.c: ret = snd_soc_dai_startup(sink, substream); > > To convince myself of this, I added a dummy startup routine and I do > see it called when I do playback and capture at the same time: > > [ 179.057494] plb: ssp2 startup stream 0 > [ 183.976963] plb: ssp2 startup stream 1 > > That makes me nervous about having a single pointer and unbalanced > calls between startup and shutdown. > > We had such issues in the past so I may be on the paranoid side here... Thank you for sharing your experience. As I mentioned above, this mark is used only for rollback of open(), not related to close(). But hmm.. I now double checked the code, 1 concern is mutex_lock(). In final step, the soc_pcm_open() will be static int soc_pcm_open() { ... (1) mutex_lock_nested(...); ... for_each_rtd_dais(rtd, i, dai) { (A) ret = snd_soc_dai_startup(dai, substream); ... } ... err: (2) mutex_unlock(&rtd->card->pcm_mutex); if (ret < 0) (B) soc_pcm_clean(substream, 1); ... } (A) is called under (1) lock, but it will be unlocked at (2), and (B) is called if rollback. If - 2 x soc_pcm_open() were called in the same time - 1st soc_pcm_open() was failed - 2nd soc_pcm_open() was called between 1st soc_pcm_open()'s (2) and (B) Indeed single pointer is not good... !? But, soc_pcm_open() itself is called under othere mutex_lock() of pcm_native.c Above issue never happen ? Thank you for your help !! Best regards --- Kuninori Morimoto
diff --git a/include/sound/soc-dai.h b/include/sound/soc-dai.h index 776a60529e70..86411f503c1c 100644 --- a/include/sound/soc-dai.h +++ b/include/sound/soc-dai.h @@ -153,7 +153,7 @@ void snd_soc_dai_hw_free(struct snd_soc_dai *dai, int snd_soc_dai_startup(struct snd_soc_dai *dai, struct snd_pcm_substream *substream); void snd_soc_dai_shutdown(struct snd_soc_dai *dai, - struct snd_pcm_substream *substream); + struct snd_pcm_substream *substream, int rollback); snd_pcm_sframes_t snd_soc_dai_delay(struct snd_soc_dai *dai, struct snd_pcm_substream *substream); void snd_soc_dai_suspend(struct snd_soc_dai *dai); @@ -388,6 +388,9 @@ struct snd_soc_dai { struct list_head list; + /* function mark */ + struct snd_pcm_substream *mark_startup; + /* bit field */ unsigned int probed:1; }; diff --git a/sound/soc/soc-dai.c b/sound/soc/soc-dai.c index 693893420bf0..4f9f73800ab0 100644 --- a/sound/soc/soc-dai.c +++ b/sound/soc/soc-dai.c @@ -32,6 +32,14 @@ static inline int _soc_dai_ret(struct snd_soc_dai *dai, return ret; } +/* + * We might want to check substream by using list. + * In such case, we can update these macros. + */ +#define soc_dai_mark_push(dai, substream, tgt) ((dai)->mark_##tgt = substream) +#define soc_dai_mark_pop(dai, substream, tgt) ((dai)->mark_##tgt = NULL) +#define soc_dai_mark_match(dai, substream, tgt) ((dai)->mark_##tgt == substream) + /** * snd_soc_dai_set_sysclk - configure DAI system or master clock. * @dai: DAI @@ -348,15 +356,26 @@ int snd_soc_dai_startup(struct snd_soc_dai *dai, dai->driver->ops->startup) ret = dai->driver->ops->startup(substream, dai); + /* mark substream if succeeded */ + if (ret == 0) + soc_dai_mark_push(dai, substream, startup); + return soc_dai_ret(dai, ret); } void snd_soc_dai_shutdown(struct snd_soc_dai *dai, - struct snd_pcm_substream *substream) + struct snd_pcm_substream *substream, + int rollback) { + if (rollback && !soc_dai_mark_match(dai, substream, startup)) + return; + if (dai->driver->ops && dai->driver->ops->shutdown) dai->driver->ops->shutdown(substream, dai); + + /* remove marked substream */ + soc_dai_mark_pop(dai, substream, startup); } snd_pcm_sframes_t snd_soc_dai_delay(struct snd_soc_dai *dai, diff --git a/sound/soc/soc-dapm.c b/sound/soc/soc-dapm.c index 3273161e2787..980f2c330b87 100644 --- a/sound/soc/soc-dapm.c +++ b/sound/soc/soc-dapm.c @@ -3968,14 +3968,14 @@ static int snd_soc_dai_link_event(struct snd_soc_dapm_widget *w, snd_soc_dapm_widget_for_each_source_path(w, path) { source = path->source->priv; snd_soc_dai_deactivate(source, substream->stream); - snd_soc_dai_shutdown(source, substream); + snd_soc_dai_shutdown(source, substream, 0); } substream->stream = SNDRV_PCM_STREAM_PLAYBACK; snd_soc_dapm_widget_for_each_sink_path(w, path) { sink = path->sink->priv; snd_soc_dai_deactivate(sink, substream->stream); - snd_soc_dai_shutdown(sink, substream); + snd_soc_dai_shutdown(sink, substream, 0); } break; diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c index 10f703986be3..ae7e648cd21c 100644 --- a/sound/soc/soc-pcm.c +++ b/sound/soc/soc-pcm.c @@ -682,7 +682,7 @@ static int soc_pcm_close(struct snd_pcm_substream *substream) snd_soc_runtime_deactivate(rtd, substream->stream); for_each_rtd_dais(rtd, i, dai) - snd_soc_dai_shutdown(dai, substream); + snd_soc_dai_shutdown(dai, substream, 0); snd_soc_link_shutdown(substream); @@ -813,7 +813,7 @@ static int soc_pcm_open(struct snd_pcm_substream *substream) config_err: for_each_rtd_dais(rtd, i, dai) - snd_soc_dai_shutdown(dai, substream); + snd_soc_dai_shutdown(dai, substream, 1); snd_soc_link_shutdown(substream); rtd_startup_err: