| Message ID | 20220621115758.3154933-1-cezary.rojewski@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ASoC: core: Exit all links before removing their components | expand |
On 6/21/22 06:57, Cezary Rojewski wrote: > Flows leading to link->init() and link->exit() are not symmetric. > Currently the relevant part of card probe sequence goes as: > > for_each_card_rtds(card, rtd) > for_each_rtd_components(rtd, i, component) > component->probe() > for_each_card_rtds(card, rtd) > for_each_rtd_dais(rtd, i, dai) > dai->probe() > for_each_card_rtds(card, rtd) > rtd->init() > > On the other side, equivalent remove sequence goes as: > > for_each_card_rtds(card, rtd) > for_each_rtd_dais(rtd, i, dai) > dai->remove() > for_each_card_rtds(card, rtd) > for_each_rtd_components(rtd, i, component) > component->remove() > for_each_card_rtds(card, rtd) > rtd->exit() > > what can lead to errors as link->exit() may still operate on resources > owned by its components despite the probability of them being freed > during the component->remove(). > > This change modifies the remove sequence to: > > for_each_card_rtds(card, rtd) > rtd->exit() > for_each_card_rtds(card, rtd) > for_each_rtd_dais(rtd, i, dai) > dai->remove() > for_each_card_rtds(card, rtd) > for_each_rtd_components(rtd, i, component) > component->remove() > > so code found in link->exit() is safe to touch any component stuff as > component->remove() has not been called yet. This looks harmless as described, but... > > Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com> > Reviewed-by: Amadeusz Sławiński <amadeuszx.slawinski@linux.intel.com> > --- > sound/soc/soc-core.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c > index 4a3fca50a536..638e781df3b0 100644 > --- a/sound/soc/soc-core.c > +++ b/sound/soc/soc-core.c > @@ -935,9 +935,6 @@ void snd_soc_remove_pcm_runtime(struct snd_soc_card *card, > { > lockdep_assert_held(&client_mutex); > > - /* release machine specific resources */ > - snd_soc_link_exit(rtd); > - > /* > * Notify the machine driver for extra destruction > */ .... what is not shown here is the soc_free_pcm_runtime(rtd); which will call device_unregister(rtd->dev); .... > @@ -1888,6 +1885,9 @@ static void soc_cleanup_card_resources(struct snd_soc_card *card) > > snd_soc_dapm_shutdown(card); > > + /* release machine specific resources */ > + for_each_card_rtds(card, rtd) > + snd_soc_link_exit(rtd); ... so there's still a risk that the link exit refers to memory that's been released already. We would need to make sure rtd->dev is not used in any of the existing callbacks - or other functions such as e.g. snd_soc_close_delayed_work() which makes use of rtd->dev > /* remove and free each DAI */ > soc_remove_link_dais(card); > soc_remove_link_components(card);
On 2022-06-21 3:31 PM, Pierre-Louis Bossart wrote: > On 6/21/22 06:57, Cezary Rojewski wrote: >> diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c >> index 4a3fca50a536..638e781df3b0 100644 >> --- a/sound/soc/soc-core.c >> +++ b/sound/soc/soc-core.c >> @@ -935,9 +935,6 @@ void snd_soc_remove_pcm_runtime(struct snd_soc_card *card, >> { >> lockdep_assert_held(&client_mutex); >> >> - /* release machine specific resources */ >> - snd_soc_link_exit(rtd); >> - >> /* >> * Notify the machine driver for extra destruction >> */ > > .... what is not shown here is the > > soc_free_pcm_runtime(rtd); > > which will call device_unregister(rtd->dev); > > .... > >> @@ -1888,6 +1885,9 @@ static void soc_cleanup_card_resources(struct snd_soc_card *card) >> >> snd_soc_dapm_shutdown(card); >> >> + /* release machine specific resources */ >> + for_each_card_rtds(card, rtd) >> + snd_soc_link_exit(rtd); > > ... so there's still a risk that the link exit refers to memory that's > been released already. > > We would need to make sure rtd->dev is not used in any of the existing > callbacks - or other functions such as e.g. snd_soc_close_delayed_work() > which makes use of rtd->dev > The lack of soc_free_pcm_runtime() included here is done on purpose. After revisiting probe and remove flows it seems that there are more de-synchronization than just link->init() and link->exit(). However, I believe providing incremental changes is better than providing one single big patch which has much larger impact of the framework. Moving just the snd_soc_link_exit() has very limited impact. In regard to the flow, we have to remember that 'someone' has to be the first one, 'someone' has to be the last one too. If the probe flow ends with link->init() (for the selected range of functions), then it feels natural for remove flow to begin with link->exit(). I've scanned the repo for usages of link->exit() and it seems only Intel platforms do so. Given their (functions) current content, status quo is achieved. AVS and CATPT drivers reload-tests show no regression too. The DAPM-pin warning I'd mentioned earlier in the bdw_rt286 discussion is also gone. Regards, Czarek
On 2022-06-21 4:53 PM, Cezary Rojewski wrote: > On 2022-06-21 3:31 PM, Pierre-Louis Bossart wrote: >> On 6/21/22 06:57, Cezary Rojewski wrote: > >>> diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c >>> index 4a3fca50a536..638e781df3b0 100644 >>> --- a/sound/soc/soc-core.c >>> +++ b/sound/soc/soc-core.c >>> @@ -935,9 +935,6 @@ void snd_soc_remove_pcm_runtime(struct >>> snd_soc_card *card, >>> { >>> lockdep_assert_held(&client_mutex); >>> - /* release machine specific resources */ >>> - snd_soc_link_exit(rtd); >>> - >>> /* >>> * Notify the machine driver for extra destruction >>> */ >> >> .... what is not shown here is the >> >> soc_free_pcm_runtime(rtd); >> >> which will call device_unregister(rtd->dev); >> >> .... >> >>> @@ -1888,6 +1885,9 @@ static void soc_cleanup_card_resources(struct >>> snd_soc_card *card) >>> snd_soc_dapm_shutdown(card); >>> + /* release machine specific resources */ >>> + for_each_card_rtds(card, rtd) >>> + snd_soc_link_exit(rtd); >> >> ... so there's still a risk that the link exit refers to memory that's >> been released already. >> >> We would need to make sure rtd->dev is not used in any of the existing >> callbacks - or other functions such as e.g. snd_soc_close_delayed_work() >> which makes use of rtd->dev >> > > The lack of soc_free_pcm_runtime() included here is done on purpose. > After revisiting probe and remove flows it seems that there are more > de-synchronization than just link->init() and link->exit(). However, I > believe providing incremental changes is better than providing one > single big patch which has much larger impact of the framework. Moving > just the snd_soc_link_exit() has very limited impact. > > In regard to the flow, we have to remember that 'someone' has to be the > first one, 'someone' has to be the last one too. If the probe flow ends > with link->init() (for the selected range of functions), then it feels > natural for remove flow to begin with link->exit(). > > I've scanned the repo for usages of link->exit() and it seems only Intel > platforms do so. Given their (functions) current content, status quo is > achieved. AVS and CATPT drivers reload-tests show no regression too. The > DAPM-pin warning I'd mentioned earlier in the bdw_rt286 discussion is > also gone. Hello, Are there any questions left unanswered here? This patch is a dependency for jack handling changes for avs/catpt machine boards. Regards, Czarek
diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c index 4a3fca50a536..638e781df3b0 100644 --- a/sound/soc/soc-core.c +++ b/sound/soc/soc-core.c @@ -935,9 +935,6 @@ void snd_soc_remove_pcm_runtime(struct snd_soc_card *card, { lockdep_assert_held(&client_mutex); - /* release machine specific resources */ - snd_soc_link_exit(rtd); - /* * Notify the machine driver for extra destruction */ @@ -1888,6 +1885,9 @@ static void soc_cleanup_card_resources(struct snd_soc_card *card) snd_soc_dapm_shutdown(card); + /* release machine specific resources */ + for_each_card_rtds(card, rtd) + snd_soc_link_exit(rtd); /* remove and free each DAI */ soc_remove_link_dais(card); soc_remove_link_components(card);