[alsa-devel] [PATCH] soc-core: let suspend/resume not called if the card is not instantiated
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
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.
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
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.
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.
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.
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.
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.
participants (2)
-
Barry Song -
Mark Brown