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@xxxxxxxxx>
> Reviewed-by: Amadeusz Sławiński <amadeuszx.slawinski@xxxxxxxxxxxxxxx>
> ---
> 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);
