[PATCH] soc-core: let suspend/resume not called if the card is not instantiated

[PATCH] soc-core: let suspend/resume not called if the card is not instantiated

[Barry Song]

It doesn't make sense to call suspend/resume of all cpu/codec dais even
they are not initialized and registered. This will cause many problems
foraudio PM.
The condition card is not instantiated covers codec is null, so delete
the check that codec is null.

Signed-off-by: Barry Song <21cnbao@gmail.com>
---
 sound/soc/soc-core.c |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c
index e2b6d75..097f3b9 100644
--- a/sound/soc/soc-core.c
+++ b/sound/soc/soc-core.c
@@ -796,10 +796,7 @@ static int soc_suspend(struct device *dev)
 	struct snd_soc_codec *codec = card->codec;
 	int i;
 
-	/* If the initialization of this soc device failed, there is no codec
-	 * associated with it. Just bail out in this case.
-	 */
-	if (!codec)
+	if (!card->instantiated)
 		return 0;
 
 	/* Due to the resume being scheduled into a workqueue we could
@@ -940,6 +937,9 @@ static int soc_resume(struct device *dev)
 	struct snd_soc_card *card = socdev->card;
 	struct snd_soc_dai *cpu_dai = card->dai_link[0].cpu_dai;
 
+	if (!card->instantiated)
+		return 0;
+
 	/* AC97 devices might have other drivers hanging off them so
 	 * need to resume immediately.  Other drivers don't have that
 	 * problem and may take a substantial amount of time to resume
-- 
1.5.6.3

Re: [PATCH] soc-core: let suspend/resume not called if the card is not instantiated

[Mark Brown]

On Fri, Nov 13, 2009 at 12:51:18PM +0800, Barry Song wrote:
> It doesn't make sense to call suspend/resume of all cpu/codec dais even
> they are not initialized and registered. This will cause many problems
> foraudio PM.
> The condition card is not instantiated covers codec is null, so delete
> the check that codec is null.

Hrm.  It's not clear that this is the best approach here - it'd mean
that things like the CODEC drivers would need to have two suspend and
resume paths, one for use before the card comes up and one for use
afterwards.  As things stand this will fix whatever problem you are
seeing but will create problems for other systems where we'll stop
suspending devices that need it.

As with your TDM port issue pm_link is going to be really helpful here
once it hits mainline.  Let me have a think about what (if anything) we
can do in the meantime, this area is a little fragile.

Re: [PATCH] soc-core: let suspend/resume not called if the card is not instantiated

[Barry Song]

On Fri, Nov 13, 2009 at 9:38 PM, Mark Brown
<broonie@opensource.wolfsonmicro.com> wrote:
> On Fri, Nov 13, 2009 at 12:51:18PM +0800, Barry Song wrote:
>> It doesn't make sense to call suspend/resume of all cpu/codec dais even
>> they are not initialized and registered. This will cause many problems
>> foraudio PM.
>> The condition card is not instantiated covers codec is null, so delete
>> the check that codec is null.
>
> Hrm.  It's not clear that this is the best approach here - it'd mean
> that things like the CODEC drivers would need to have two suspend and
> resume paths, one for use before the card comes up and one for use
> afterwards.  As things stand this will fix whatever problem you are
> seeing but will create problems for other systems where we'll stop
> suspending devices that need it.
>
> As with your TDM port issue pm_link is going to be really helpful here
> once it hits mainline.  Let me have a think about what (if anything) we
> can do in the meantime, this area is a little fragile.
>
Mark,
You misunderstand my patch:-)
This patch is to solve a completely different problem with the last
one I sent. Its meaning is very simple actually.  While
card->instantiated is not 1, it means the card(cpu dai/codec dai and
related stuff) and the whole links are not built successfully at all.
So devices are not even initialized at all. And the audio system
doesn't start to work. Since so, suspend/resume should be not needed
for the whole system.
Thanks
Barry
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH] soc-core: let suspend/resume not called if the card is not instantiated

[Mark Brown]

On Fri, Nov 13, 2009 at 10:56:13PM +0800, Barry Song wrote:

> This patch is to solve a completely different problem with the last
> one I sent. Its meaning is very simple actually.  While

No, I get that it's fixing a different problem but it's in a similar
areas and similar issues apply.  Like I say, tweaking the ordering of
suspend and resume will actually fix the TDM problem anyway.

> card->instantiated is not 1, it means the card(cpu dai/codec dai and
> related stuff) and the whole links are not built successfully at all.
> So devices are not even initialized at all. And the audio system
> doesn't start to work. Since so, suspend/resume should be not needed
> for the whole system.

That's not the case unfortunately - some or all of the devices may have
initialised themselves prior to registering with the SoC core and will
be expecting the SoC core to give them a callback so that they can
suspend and resume.  At the minute there's basically an assumption that
the card is going to come up completely and all we're doing here is
avoiding race conditions during startup (which is realistic for
production systems, partially constructed cards aren't likely there
without broken hardware).

With pm_link we'll be able to move some or all of this ordering stuff
out of the ASoC core and into the device model which will allow things
to happen either way without two code paths.

Re: [PATCH] soc-core: let suspend/resume not called if the card is not instantiated

[Barry Song]

On Fri, Nov 13, 2009 at 11:02 PM, Mark Brown
<broonie@opensource.wolfsonmicro.com> wrote:
> On Fri, Nov 13, 2009 at 10:56:13PM +0800, Barry Song wrote:
>
>> This patch is to solve a completely different problem with the last
>> one I sent. Its meaning is very simple actually.  While
>
> No, I get that it's fixing a different problem but it's in a similar
> areas and similar issues apply.  Like I say, tweaking the ordering of
> suspend and resume will actually fix the TDM problem anyway.
>
>> card->instantiated is not 1, it means the card(cpu dai/codec dai and
>> related stuff) and the whole links are not built successfully at all.
>> So devices are not even initialized at all. And the audio system
>> doesn't start to work. Since so, suspend/resume should be not needed
>> for the whole system.
>
> That's not the case unfortunately - some or all of the devices may have
> initialised themselves prior to registering with the SoC core and will
> be expecting the SoC core to give them a callback so that they can
> suspend and resume.  At the minute there's basically an assumption that
> the card is going to come up completely and all we're doing here is
> avoiding race conditions during startup (which is realistic for
> production systems, partially constructed cards aren't likely there
> without broken hardware).
The logic seems different: in snd_soc_instantiate_card(), only while
all cpu DAIs and codec DAIs are found and registered in the system
except ac97, their probes will be called. Then card->instantiated
becomes 1.
A case current codes will cause crash is:
For a CPU DAI driver based on platform driver, if arch hasn't defined
the platform device for cpu dai, then cpu dai is not registered and
initialized in the probe() of its platform driver. Even though its
instance is in card->dai_link, it doesn't really enter the system. But
while suspend/resume, its entries will be called too since the call
doesn't check the existence:
static int soc_suspend(struct device *dev)
{
	...
	for (i = 0; i < card->num_links; i++) {
		struct snd_soc_dai  *cpu_dai = card->dai_link[i].cpu_dai;  /* not
registered but in the link!!! */
		if (cpu_dai->suspend && !cpu_dai->ac97_control)
			cpu_dai->suspend(cpu_dai);     /* not initialized but be suspended??? */
		if (platform->suspend)
			platform->suspend(cpu_dai);
	}
       ...
}
It can easily make system crash. Same case for a not-registered codes too.
I think suspend/resume of a DAI can only be called while it really
exists and can be found in the dai_list, at least.
So I add a overall condition for soc_suspend/resume just like
	if (!card->instantiated)
		return 0
has be done in soc_remove(),  soc_poweroff().

>
> With pm_link we'll be able to move some or all of this ordering stuff
> out of the ASoC core and into the device model which will allow things
> to happen either way without two code paths.
>
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH] soc-core: let suspend/resume not called if the card is not instantiated

[Mark Brown]

On Fri, Nov 13, 2009 at 11:29:39PM +0800, Barry Song wrote:

> The logic seems different: in snd_soc_instantiate_card(), only while
> all cpu DAIs and codec DAIs are found and registered in the system
> except ac97, their probes will be called. Then card->instantiated
> becomes 1.
> A case current codes will cause crash is:
> For a CPU DAI driver based on platform driver, if arch hasn't defined
> the platform device for cpu dai, then cpu dai is not registered and
> initialized in the probe() of its platform driver. Even though its
> instance is in card->dai_link, it doesn't really enter the system. But
> while suspend/resume, its entries will be called too since the call
> doesn't check the existence:

Yes, that can go wrong - but as I say the converse thing can also go
wrong where some but not all of the card has managed to register and the
bits that have managed to register expect to be suspended by the core.

> I think suspend/resume of a DAI can only be called while it really
> exists and can be found in the dai_list, at least.

Checking to see if the device is in the DAI relevant list would work
but...

> So I add a overall condition for soc_suspend/resume just like
> 	if (!card->instantiated)
> 		return 0
> has be done in soc_remove(),  soc_poweroff().

...this is too broad.

Re: [PATCH] soc-core: let suspend/resume not called if the card is not instantiated

[Barry Song]

On Sat, Nov 14, 2009 at 1:04 AM, Mark Brown
<broonie@opensource.wolfsonmicro.com> wrote:
> On Fri, Nov 13, 2009 at 11:29:39PM +0800, Barry Song wrote:
>
>> The logic seems different: in snd_soc_instantiate_card(), only while
>> all cpu DAIs and codec DAIs are found and registered in the system
>> except ac97, their probes will be called. Then card->instantiated
>> becomes 1.
>> A case current codes will cause crash is:
>> For a CPU DAI driver based on platform driver, if arch hasn't defined
>> the platform device for cpu dai, then cpu dai is not registered and
>> initialized in the probe() of its platform driver. Even though its
>> instance is in card->dai_link, it doesn't really enter the system. But
>> while suspend/resume, its entries will be called too since the call
>> doesn't check the existence:
>
> Yes, that can go wrong - but as I say the converse thing can also go
> wrong where some but not all of the card has managed to register and the
> bits that have managed to register expect to be suspended by the core.
>
>> I think suspend/resume of a DAI can only be called while it really
>> exists and can be found in the dai_list, at least.
>
> Checking to see if the device is in the DAI relevant list would work
> but...
>
>> So I add a overall condition for soc_suspend/resume just like
>>       if (!card->instantiated)
>>               return 0
>> has be done in soc_remove(),  soc_poweroff().
>
> ...this is too broad.
Then do you think it is workable to find the dai in dai_list before
calling its suspend/resume? My original plan was checking the
existence of DAI in dai_list, if DAI doesn't exist, don't call its PM
entries.  Then I found card->instantiated, I moved to check it.
If you have better plan for fixing this problem, I can wait and keep
this fix locally.

>
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH] soc-core: let suspend/resume not called if the card is not instantiated

[Mark Brown]

On Sat, Nov 14, 2009 at 08:46:05AM +0800, Barry Song wrote:

> Then do you think it is workable to find the dai in dai_list before
> calling its suspend/resume? My original plan was checking the
> existence of DAI in dai_list, if DAI doesn't exist, don't call its PM

I think so, yes.

> entries.  Then I found card->instantiated, I moved to check it.
> If you have better plan for fixing this problem, I can wait and keep
> this fix locally.

I don't have any immediate plan to work on this, like I say it's only
really an issue in development.