[PATCH] ASoC: Intel: fix broadwell module removing failed issue

[PATCH] ASoC: Intel: fix broadwell module removing failed issue

[Jie Yang]

From: Liam Girdwood <liam.r.girdwood@linux.intel.com>

In haswell-pcm module unloading, we can't free runtime modules
directly, for they may be already freed in runtime suspend.

Here add executing suspend call to unload runtime modules, only
for status not equal to RPM_SUSPEND, to fix broadwell module
removing failed issue.

Signed-off-by: Liam Girdwood <liam.r.girdwood@linux.intel.com>
Signed-off-by: Jie Yang <yang.jie@intel.com>
---
 sound/soc/intel/haswell/sst-haswell-pcm.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/sound/soc/intel/haswell/sst-haswell-pcm.c b/sound/soc/intel/haswell/sst-haswell-pcm.c
index 23ae040..bd96629 100644
--- a/sound/soc/intel/haswell/sst-haswell-pcm.c
+++ b/sound/soc/intel/haswell/sst-haswell-pcm.c
@@ -1118,8 +1118,10 @@ 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)
-- 
1.9.1

Re: [PATCH] ASoC: Intel: fix broadwell module removing failed issue

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 269 bytes --]

On Thu, May 28, 2015 at 02:14:18PM +0800, Jie Yang wrote:
> From: Liam Girdwood <liam.r.girdwood@linux.intel.com>
> 
> In haswell-pcm module unloading, we can't free runtime modules
> directly, for they may be already freed in runtime suspend.

Applied, thanks.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH] ASoC: Intel: fix broadwell module removing failed issue

[Takashi Iwai]

At Thu, 28 May 2015 14:14:18 +0800,
Jie Yang wrote:
> 
> From: Liam Girdwood <liam.r.girdwood@linux.intel.com>
> 
> In haswell-pcm module unloading, we can't free runtime modules
> directly, for they may be already freed in runtime suspend.
> 
> Here add executing suspend call to unload runtime modules, only
> for status not equal to RPM_SUSPEND, to fix broadwell module
> removing failed issue.

What if a kernel is built without PM support?  (Practically seen, it's
never any serious problem, though.)


Takashi

> 
> Signed-off-by: Liam Girdwood <liam.r.girdwood@linux.intel.com>
> Signed-off-by: Jie Yang <yang.jie@intel.com>
> ---
>  sound/soc/intel/haswell/sst-haswell-pcm.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/sound/soc/intel/haswell/sst-haswell-pcm.c b/sound/soc/intel/haswell/sst-haswell-pcm.c
> index 23ae040..bd96629 100644
> --- a/sound/soc/intel/haswell/sst-haswell-pcm.c
> +++ b/sound/soc/intel/haswell/sst-haswell-pcm.c
> @@ -1118,8 +1118,10 @@ 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)
> -- 
> 1.9.1
> 

Re: [PATCH] ASoC: Intel: fix broadwell module removing failed issue

[Liam Girdwood]

On Thu, 2015-05-28 at 17:09 +0200, Takashi Iwai wrote:
> At Thu, 28 May 2015 14:14:18 +0800,
> Jie Yang wrote:
> > 
> > From: Liam Girdwood <liam.r.girdwood@linux.intel.com>
> > 
> > In haswell-pcm module unloading, we can't free runtime modules
> > directly, for they may be already freed in runtime suspend.
> > 
> > Here add executing suspend call to unload runtime modules, only
> > for status not equal to RPM_SUSPEND, to fix broadwell module
> > removing failed issue.
> 
> What if a kernel is built without PM support?  (Practically seen, it's
> never any serious problem, though.)
> 

Keyon, it sounds like you will still need hsw_pcm_free_modules() and
should call it after the PM put/disable on module remove. You will need
to add some code too that will check the memory state prior to doing any
unloading though....

Liam



---------------------------------------------------------------------
Intel Corporation (UK) Limited
Registered No. 1134945 (England)
Registered Office: Pipers Way, Swindon SN3 1RJ
VAT No: 860 2173 47

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.

Re: [PATCH] ASoC: Intel: fix broadwell module removing failed issue

[Jie, Yang]

> -----Original Message-----
> From: Girdwood, Liam R
> Sent: Thursday, May 28, 2015 11:45 PM
> To: Takashi Iwai
> Cc: Jie, Yang; broonie@kernel.org; alsa-devel@alsa-project.org
> Subject: Re: [PATCH] ASoC: Intel: fix broadwell module removing failed issue
> 
> On Thu, 2015-05-28 at 17:09 +0200, Takashi Iwai wrote:
> > At Thu, 28 May 2015 14:14:18 +0800,
> > Jie Yang wrote:
> > >
> > > From: Liam Girdwood <liam.r.girdwood@linux.intel.com>
> > >
> > > In haswell-pcm module unloading, we can't free runtime modules
> > > directly, for they may be already freed in runtime suspend.
> > >
> > > Here add executing suspend call to unload runtime modules, only for
> > > status not equal to RPM_SUSPEND, to fix broadwell module removing
> > > failed issue.
> >
> > What if a kernel is built without PM support?  (Practically seen, it's
> > never any serious problem, though.)
 
I think or kernel built without PM, empty pm_runtime_xxx()s are called and
no problems for that.

> >
> 
> Keyon, it sounds like you will still need hsw_pcm_free_modules() and should
> call it after the PM put/disable on module remove. You will need to add
> some code too that will check the memory state prior to doing any unloading
> though....
 
Yes, I plan to add both check before runtime_module_free() and set
pcm_data->runtime to NULL after that. At the same time, make sure
hsw_pcm_free_modules() is called before fw_unload() called.

With those implemented, suppose the issue can be fixed neatly.

I will test it and submit then.

~Keyon

> 
> Liam
> 
> 

Re: [PATCH] ASoC: Intel: fix broadwell module removing failed issue

[Takashi Iwai]

At Fri, 29 May 2015 07:46:37 +0000,
Jie, Yang wrote:
> 
> > -----Original Message-----
> > From: Girdwood, Liam R
> > Sent: Thursday, May 28, 2015 11:45 PM
> > To: Takashi Iwai
> > Cc: Jie, Yang; broonie@kernel.org; alsa-devel@alsa-project.org
> > Subject: Re: [PATCH] ASoC: Intel: fix broadwell module removing failed issue
> > 
> > On Thu, 2015-05-28 at 17:09 +0200, Takashi Iwai wrote:
> > > At Thu, 28 May 2015 14:14:18 +0800,
> > > Jie Yang wrote:
> > > >
> > > > From: Liam Girdwood <liam.r.girdwood@linux.intel.com>
> > > >
> > > > In haswell-pcm module unloading, we can't free runtime modules
> > > > directly, for they may be already freed in runtime suspend.
> > > >
> > > > Here add executing suspend call to unload runtime modules, only for
> > > > status not equal to RPM_SUSPEND, to fix broadwell module removing
> > > > failed issue.
> > >
> > > What if a kernel is built without PM support?  (Practically seen, it's
> > > never any serious problem, though.)
>  
> I think or kernel built without PM, empty pm_runtime_xxx()s are called and
> no problems for that.

Well, but what about the modules?  Where will they be freed?  Your
patch description sounds as if they are freed in the runtime suspend
path.

Also, I'm not quite certain whether it's allowed to do runtime suspend
at driver free.  Usually it's even forcibly powered up, as we can't
leave the device as suspended state after free (who can wake up
rightly if no driver is bound?)


> > Keyon, it sounds like you will still need hsw_pcm_free_modules() and should
> > call it after the PM put/disable on module remove. You will need to add
> > some code too that will check the memory state prior to doing any unloading
> > though....
>  
> Yes, I plan to add both check before runtime_module_free() and set
> pcm_data->runtime to NULL after that. At the same time, make sure
> hsw_pcm_free_modules() is called before fw_unload() called.
> 
> With those implemented, suppose the issue can be fixed neatly.
> 
> I will test it and submit then.

Yes, this looks like the missing piece.


Takashi

Re: [PATCH] ASoC: Intel: fix broadwell module removing failed issue

[Jie, Yang]

> -----Original Message-----
> From: Takashi Iwai [mailto:tiwai@suse.de]
> Sent: Friday, May 29, 2015 6:52 PM
> To: Jie, Yang
> Cc: Girdwood, Liam R; broonie@kernel.org; alsa-devel@alsa-project.org
> Subject: Re: [PATCH] ASoC: Intel: fix broadwell module removing failed issue
> 
> At Fri, 29 May 2015 07:46:37 +0000,
> Jie, Yang wrote:
> >
> > > -----Original Message-----
> > > From: Girdwood, Liam R
> > > Sent: Thursday, May 28, 2015 11:45 PM
> > > To: Takashi Iwai
> > > Cc: Jie, Yang; broonie@kernel.org; alsa-devel@alsa-project.org
> > > Subject: Re: [PATCH] ASoC: Intel: fix broadwell module removing
> > > failed issue
> > >
> > > On Thu, 2015-05-28 at 17:09 +0200, Takashi Iwai wrote:
> > > > At Thu, 28 May 2015 14:14:18 +0800, Jie Yang wrote:
> > > > >
> > > > > From: Liam Girdwood <liam.r.girdwood@linux.intel.com>
> > > > >
> > > > > In haswell-pcm module unloading, we can't free runtime modules
> > > > > directly, for they may be already freed in runtime suspend.
> > > > >
> > > > > Here add executing suspend call to unload runtime modules, only
> > > > > for status not equal to RPM_SUSPEND, to fix broadwell module
> > > > > removing failed issue.
> > > >
> > > > What if a kernel is built without PM support?  (Practically seen,
> > > > it's never any serious problem, though.)
> >
> > I think or kernel built without PM, empty pm_runtime_xxx()s are called
> > and no problems for that.
> 
> Well, but what about the modules?  Where will they be freed?  Your patch
> description sounds as if they are freed in the runtime suspend path.

The freeing of modules is actually done in fw_unload() stage.

> 
> Also, I'm not quite certain whether it's allowed to do runtime suspend at
> driver free.  Usually it's even forcibly powered up, as we can't leave the
> device as suspended state after free (who can wake up rightly if no driver is
> bound?)
 
You are right. Will change it to that descripted below.

> 
> 
> > > Keyon, it sounds like you will still need hsw_pcm_free_modules() and
> > > should call it after the PM put/disable on module remove. You will
> > > need to add some code too that will check the memory state prior to
> > > doing any unloading though....
> >
> > Yes, I plan to add both check before runtime_module_free() and set
> > pcm_data->runtime to NULL after that. At the same time, make sure
> > hsw_pcm_free_modules() is called before fw_unload() called.
> >
> > With those implemented, suppose the issue can be fixed neatly.
> >
> > I will test it and submit then.
> 
> Yes, this looks like the missing piece.
 
OK, then I will follow this.

Hi Mark, can you help revert these 2 commits? Sorry for the inconvenience
caused.

ASoC: Intel: fix broadwell module removing failed issue
ASoC: Intel: remove unused function hsw_pcm_free_modules()

~Keyon

> 
> 
> Takashi

Re: [PATCH] ASoC: Intel: fix broadwell module removing failed issue

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 378 bytes --]

On Fri, May 29, 2015 at 03:06:09PM +0000, Jie, Yang wrote:

> OK, then I will follow this.

> Hi Mark, can you help revert these 2 commits? Sorry for the inconvenience
> caused.

> ASoC: Intel: fix broadwell module removing failed issue
> ASoC: Intel: remove unused function hsw_pcm_free_modules()

No, please send patches for any changes you want to integrate into the
kernel.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH] ASoC: Intel: fix broadwell module removing failed issue

[Jie, Yang]

> -----Original Message-----
> From: Mark Brown [mailto:broonie@kernel.org]
> Sent: Friday, May 29, 2015 11:19 PM
> To: Jie, Yang
> Cc: Takashi Iwai; Girdwood, Liam R; alsa-devel@alsa-project.org
> Subject: Re: [PATCH] ASoC: Intel: fix broadwell module removing failed issue
> 
> On Fri, May 29, 2015 at 03:06:09PM +0000, Jie, Yang wrote:
> 
> > OK, then I will follow this.
> 
> > Hi Mark, can you help revert these 2 commits? Sorry for the
> > inconvenience caused.
> 
> > ASoC: Intel: fix broadwell module removing failed issue
> > ASoC: Intel: remove unused function hsw_pcm_free_modules()
> 
> No, please send patches for any changes you want to integrate into the
> kernel.
 
Got it, thanks, Mark.

~Keyon