[alsa-devel] [PATCH 0/4] new fix for broadwell module removing failed issue
This patch series are based on broonie/sound.git for-next branch, commit 01a4d5083562fcdeab99d42b0873aabc5fcca018.
The broadwell platform driver had module removing failed issue, and the two commits("ASoC: Intel: remove unused function hsw_pcm_free_modules()" and "ASoC: Intel: fix broadwell module removing failed issue") workaround it.
But at the same time, we shouldn't leave the device as suspended state after module freed, it is not good to do runtime suspend at driver free either, so the fix was workaround only and not good enough.
Here revert the old fix, and replace it with new unified procedure for runtime modules freeing: suspends firmware ==> frees runtime modules ==> unloads firmware.
With this new procedure, we remove the runtime modules at both runtime suspend(D0->D3) and module(snd_soc_sst_haswell_pcm) removing, but make sure we do it only once.
Jie Yang (4): Revert "ASoC: Intel: remove unused function hsw_pcm_free_modules()" Revert "ASoC: Intel: fix broadwell module removing failed issue" ASoC: Intel: check and clear runtime module pointer ASoC: Intel: handle haswell pcm suspend including runtime modules freeing
sound/soc/intel/haswell/sst-haswell-pcm.c | 46 +++++++++++++++++++++++++------ 1 file changed, 37 insertions(+), 9 deletions(-)
This reverts commit 506c148ee5e1bfb836116353305927ca4c21a23e.
We still need this hsw_pcm_free_modules(), we plan to remove the runtime modules at both fw_unload(D0->D3) and snd_soc_sst_haswell_pcm module removing.
Signed-off-by: Jie Yang yang.jie@intel.com --- sound/soc/intel/haswell/sst-haswell-pcm.c | 15 +++++++++++++++ 1 file changed, 15 insertions(+)
diff --git a/sound/soc/intel/haswell/sst-haswell-pcm.c b/sound/soc/intel/haswell/sst-haswell-pcm.c index 1557e37..bd96629 100644 --- a/sound/soc/intel/haswell/sst-haswell-pcm.c +++ b/sound/soc/intel/haswell/sst-haswell-pcm.c @@ -920,6 +920,21 @@ err: return -ENODEV; }
+static void hsw_pcm_free_modules(struct hsw_priv_data *pdata) +{ + struct sst_hsw *hsw = pdata->hsw; + struct hsw_pcm_data *pcm_data; + int i; + + for (i = 0; i < ARRAY_SIZE(mod_map); i++) { + pcm_data = &pdata->pcm[mod_map[i].dai_id][mod_map[i].stream]; + sst_hsw_runtime_module_free(pcm_data->runtime); + } + if (sst_hsw_is_module_loaded(hsw, SST_HSW_MODULE_WAVES)) { + sst_hsw_runtime_module_free(pdata->runtime_waves); + } +} + static int hsw_pcm_new(struct snd_soc_pcm_runtime *rtd) { struct snd_pcm *pcm = rtd->pcm;
This reverts commit 01f202c7b4b40025f3ea4721c52e7f78545e3b07.
We shouldn't leave the device as suspended state after module freed, it is not good to do runtime suspend at driver free, here revert this fixing, and replace it with the procedure: suspends firmware ==> frees runtime modules ==> unloads firmware.
Signed-off-by: Jie Yang yang.jie@intel.com --- sound/soc/intel/haswell/sst-haswell-pcm.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/sound/soc/intel/haswell/sst-haswell-pcm.c b/sound/soc/intel/haswell/sst-haswell-pcm.c index bd96629..23ae040 100644 --- a/sound/soc/intel/haswell/sst-haswell-pcm.c +++ b/sound/soc/intel/haswell/sst-haswell-pcm.c @@ -1118,10 +1118,8 @@ static int hsw_pcm_remove(struct snd_soc_platform *platform) snd_soc_platform_get_drvdata(platform); int i;
- /* execute a suspend call to unload all FW resources */ - if (!pm_runtime_status_suspended(platform->dev)) - pm_runtime_put_sync_suspend(platform->dev); pm_runtime_disable(platform->dev); + hsw_pcm_free_modules(priv_data);
for (i = 0; i < ARRAY_SIZE(hsw_dais); i++) { if (hsw_dais[i].playback.channels_min)
Add check runtime module pointers before freeing them, and clear them to NULL after freed.
With this implemented, we can avoid NULL pointer dereference or double free errors.
Signed-off-by: Jie Yang yang.jie@intel.com --- sound/soc/intel/haswell/sst-haswell-pcm.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/sound/soc/intel/haswell/sst-haswell-pcm.c b/sound/soc/intel/haswell/sst-haswell-pcm.c index 23ae040..f97fa5a 100644 --- a/sound/soc/intel/haswell/sst-haswell-pcm.c +++ b/sound/soc/intel/haswell/sst-haswell-pcm.c @@ -928,10 +928,15 @@ static void hsw_pcm_free_modules(struct hsw_priv_data *pdata)
for (i = 0; i < ARRAY_SIZE(mod_map); i++) { pcm_data = &pdata->pcm[mod_map[i].dai_id][mod_map[i].stream]; - sst_hsw_runtime_module_free(pcm_data->runtime); + if (pcm_data->runtime){ + sst_hsw_runtime_module_free(pcm_data->runtime); + pcm_data->runtime = NULL; + } } - if (sst_hsw_is_module_loaded(hsw, SST_HSW_MODULE_WAVES)) { + if (sst_hsw_is_module_loaded(hsw, SST_HSW_MODULE_WAVES) && + pdata->runtime_waves) { sst_hsw_runtime_module_free(pdata->runtime_waves); + pdata->runtime_waves = NULL; } }
It needs free pcm runtime modules before unloading firmware, here add hsw_pcm_suspend() to handle this procedure: suspends firmware ==> frees runtime modules ==> unloads firmware.
This fixes the broadwell module unload failed issue.
Signed-off-by: Jie Yang yang.jie@intel.com --- sound/soc/intel/haswell/sst-haswell-pcm.c | 22 ++++++++++++++++------ 1 file changed, 16 insertions(+), 6 deletions(-)
diff --git a/sound/soc/intel/haswell/sst-haswell-pcm.c b/sound/soc/intel/haswell/sst-haswell-pcm.c index f97fa5a..e593e7a 100644 --- a/sound/soc/intel/haswell/sst-haswell-pcm.c +++ b/sound/soc/intel/haswell/sst-haswell-pcm.c @@ -1209,6 +1209,20 @@ static int hsw_pcm_runtime_idle(struct device *dev) return 0; }
+static int hsw_pcm_suspend(struct device *dev) +{ + struct hsw_priv_data *pdata = dev_get_drvdata(dev); + struct sst_hsw *hsw = pdata->hsw; + + /* enter D3 state and stall */ + sst_hsw_dsp_runtime_suspend(hsw); + /* free all runtime modules */ + hsw_pcm_free_modules(pdata); + /* put the DSP to sleep, fw unloaded after runtime modules freed */ + sst_hsw_dsp_runtime_sleep(hsw); + return 0; +} + static int hsw_pcm_runtime_suspend(struct device *dev) { struct hsw_priv_data *pdata = dev_get_drvdata(dev); @@ -1225,8 +1239,7 @@ static int hsw_pcm_runtime_suspend(struct device *dev) return ret; sst_hsw_set_module_enabled_rtd3(hsw, SST_HSW_MODULE_WAVES); } - sst_hsw_dsp_runtime_suspend(hsw); - sst_hsw_dsp_runtime_sleep(hsw); + hsw_pcm_suspend(dev); pdata->pm_state = HSW_PM_STATE_RTD3;
return 0; @@ -1366,10 +1379,7 @@ static int hsw_pcm_prepare(struct device *dev) if (err < 0) dev_err(dev, "failed to save context for PCM %d\n", i); } - /* enter D3 state and stall */ - sst_hsw_dsp_runtime_suspend(hsw); - /* put the DSP to sleep */ - sst_hsw_dsp_runtime_sleep(hsw); + hsw_pcm_suspend(dev); }
snd_soc_suspend(pdata->soc_card->dev);
On Sat, May 30, 2015 at 10:33:55PM +0800, Jie Yang wrote:
This patch series are based on broonie/sound.git for-next branch, commit 01a4d5083562fcdeab99d42b0873aabc5fcca018.
Applied all, thanks. Please use subject lines matcing the style for the subsystem, even for reverts.
-----Original Message----- From: Mark Brown [mailto:broonie@kernel.org] Sent: Tuesday, June 02, 2015 10:35 PM To: Jie, Yang Cc: alsa-devel@alsa-project.org; Girdwood, Liam R Subject: Re: [PATCH 0/4] new fix for broadwell module removing failed issue
On Sat, May 30, 2015 at 10:33:55PM +0800, Jie Yang wrote:
This patch series are based on broonie/sound.git for-next branch, commit 01a4d5083562fcdeab99d42b0873aabc5fcca018.
Applied all, thanks. Please use subject lines matcing the style for the subsystem, even for reverts.
Got it, will notice that next time, thanks.
~Keyon
participants (3)
-
Jie Yang -
Jie, Yang
-
Mark Brown