WM8962 crashing on suspend

WM8962 crashing on suspend

[Adam Ford]

I have an imx8m Mini with a wm8962 codec.  If I run a speaker test and
suspend the board while the speaker test is running, I get the
following upon wake:

wm8962 3-001a: ASoC: error at soc_component_read_no_lock on wm8962.3-001a: -16

This message repeats itself over and over again.  If I attempt to use
any audio, it fails until I reboot the board.

If I run the audio test, then exit and suspend, the audio works upon
resume, so it appears to be related to suspending while running.

I am hoping someone might have a suggestion as to what I might be able
to do or try to allow this to successfully suspend and resume if the
device is playing sound.

thank you,

adam

Re: WM8962 crashing on suspend

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1044 bytes --]

On Tue, Apr 26, 2022 at 11:36:26AM -0500, Adam Ford wrote:
> I have an imx8m Mini with a wm8962 codec.  If I run a speaker test and
> suspend the board while the speaker test is running, I get the
> following upon wake:
> 
> wm8962 3-001a: ASoC: error at soc_component_read_no_lock on wm8962.3-001a: -16
> 
> This message repeats itself over and over again.  If I attempt to use
> any audio, it fails until I reboot the board.
> 
> If I run the audio test, then exit and suspend, the audio works upon
> resume, so it appears to be related to suspending while running.
> 
> I am hoping someone might have a suggestion as to what I might be able
> to do or try to allow this to successfully suspend and resume if the
> device is playing sound.

I do note that wm8962 doesn't have any system suspend code which is
possibly an issue if power is lost during suspend, it only has runtime
PM operations.  If the device was runtime idle before suspend those will
figure everything out, if the device was active they won't kick in.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: WM8962 crashing on suspend

[Charles Keepax]

On Tue, Apr 26, 2022 at 11:36:26AM -0500, Adam Ford wrote:
> I have an imx8m Mini with a wm8962 codec.  If I run a speaker test and
> suspend the board while the speaker test is running, I get the
> following upon wake:
> 
> wm8962 3-001a: ASoC: error at soc_component_read_no_lock on wm8962.3-001a: -16
> 
> This message repeats itself over and over again.  If I attempt to use
> any audio, it fails until I reboot the board.
> 
> If I run the audio test, then exit and suspend, the audio works upon
> resume, so it appears to be related to suspending while running.
> 
> I am hoping someone might have a suggestion as to what I might be able
> to do or try to allow this to successfully suspend and resume if the
> device is playing sound.
> 

Hmm... EBUSY is what regmap returns when a volatile register is
read whilst the chip is still in cache only. The driver does
appear to be missing the usually fairly important work around to
avoid the IRQ and the PM runtime deadlocking on resume. Although
not sure that would actually lead to the error message you are
seeing.

Would be really handy to see a little more of the log leading up
to the failure if that is possible? And would be really awesome
if you had any idea which read was returning the error? You could
shove a dump_stack in _soc_component_ret next to the error
message.

Thanks,
Charles

Re: WM8962 crashing on suspend

[Adam Ford]

On Tue, Apr 26, 2022 at 12:41 PM Charles Keepax
<ckeepax@opensource.cirrus.com> wrote:
>
> On Tue, Apr 26, 2022 at 11:36:26AM -0500, Adam Ford wrote:
> > I have an imx8m Mini with a wm8962 codec.  If I run a speaker test and
> > suspend the board while the speaker test is running, I get the
> > following upon wake:
> >
> > wm8962 3-001a: ASoC: error at soc_component_read_no_lock on wm8962.3-001a: -16
> >
> > This message repeats itself over and over again.  If I attempt to use
> > any audio, it fails until I reboot the board.
> >
> > If I run the audio test, then exit and suspend, the audio works upon
> > resume, so it appears to be related to suspending while running.
> >
> > I am hoping someone might have a suggestion as to what I might be able
> > to do or try to allow this to successfully suspend and resume if the
> > device is playing sound.
> >
>
> Hmm... EBUSY is what regmap returns when a volatile register is
> read whilst the chip is still in cache only. The driver does
> appear to be missing the usually fairly important work around to
> avoid the IRQ and the PM runtime deadlocking on resume. Although
> not sure that would actually lead to the error message you are
> seeing.
>
> Would be really handy to see a little more of the log leading up
> to the failure if that is possible? And would be really awesome
> if you had any idea which read was returning the error? You could
> shove a dump_stack in _soc_component_ret next to the error
> message.

Because NXP had a downstream kernel, and it didn't appear to happen
when using their downstream kernel, I wanted to see the difference.
I found this:

static const struct dev_pm_ops wm8962_pm = {
+ SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend, pm_runtime_force_resume)
SET_RUNTIME_PM_OPS(wm8962_runtime_suspend, wm8962_runtime_resume, NULL)
};

I applied this, and it appears to make the issue go away on a 5.15
kernel.  I haven't tried it on a 5.18 yet.  If this fixes the issue,
would that be an acceptable solution to push upstream?

adam
>
> Thanks,
> Charles

Re: WM8962 crashing on suspend

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 597 bytes --]

On Wed, Apr 27, 2022 at 08:12:56AM -0500, Adam Ford wrote:

> I found this:

> static const struct dev_pm_ops wm8962_pm = {
> + SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend, pm_runtime_force_resume)
> SET_RUNTIME_PM_OPS(wm8962_runtime_suspend, wm8962_runtime_resume, NULL)
> };

> I applied this, and it appears to make the issue go away on a 5.15
> kernel.  I haven't tried it on a 5.18 yet.  If this fixes the issue,
> would that be an acceptable solution to push upstream?

Yes, that's fine - it's fixing the thing I was pointing out with only
having runtime suspend but no system suspend.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: WM8962 crashing on suspend

[Charles Keepax]

On Wed, Apr 27, 2022 at 02:21:26PM +0100, Mark Brown wrote:
> On Wed, Apr 27, 2022 at 08:12:56AM -0500, Adam Ford wrote:
> > I found this:
> > static const struct dev_pm_ops wm8962_pm = {
> > + SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend, pm_runtime_force_resume)
> > SET_RUNTIME_PM_OPS(wm8962_runtime_suspend, wm8962_runtime_resume, NULL)
> > };
> 
> > I applied this, and it appears to make the issue go away on a 5.15
> > kernel.  I haven't tried it on a 5.18 yet.  If this fixes the issue,
> > would that be an acceptable solution to push upstream?
> 
> Yes, that's fine - it's fixing the thing I was pointing out with only
> having runtime suspend but no system suspend.

:-) Well in that case ignore my email, I don't mind if we want to
go this way too.

Thanks,
Charles

Re: WM8962 crashing on suspend

[Charles Keepax]

On Wed, Apr 27, 2022 at 08:12:56AM -0500, Adam Ford wrote:
> On Tue, Apr 26, 2022 at 12:41 PM Charles Keepax
> <ckeepax@opensource.cirrus.com> wrote:
> >
> > On Tue, Apr 26, 2022 at 11:36:26AM -0500, Adam Ford wrote:
> > > I have an imx8m Mini with a wm8962 codec.  If I run a speaker test and
> > > suspend the board while the speaker test is running, I get the
> > > following upon wake:
> > >
> > > wm8962 3-001a: ASoC: error at soc_component_read_no_lock on wm8962.3-001a: -16
> > >
> > > This message repeats itself over and over again.  If I attempt to use
> > > any audio, it fails until I reboot the board.
> > >
> > > If I run the audio test, then exit and suspend, the audio works upon
> > > resume, so it appears to be related to suspending while running.
> > >
> > > I am hoping someone might have a suggestion as to what I might be able
> > > to do or try to allow this to successfully suspend and resume if the
> > > device is playing sound.
> > >
> >
> > Hmm... EBUSY is what regmap returns when a volatile register is
> > read whilst the chip is still in cache only. The driver does
> > appear to be missing the usually fairly important work around to
> > avoid the IRQ and the PM runtime deadlocking on resume. Although
> > not sure that would actually lead to the error message you are
> > seeing.
> >
> > Would be really handy to see a little more of the log leading up
> > to the failure if that is possible? And would be really awesome
> > if you had any idea which read was returning the error? You could
> > shove a dump_stack in _soc_component_ret next to the error
> > message.
> 
> Because NXP had a downstream kernel, and it didn't appear to happen
> when using their downstream kernel, I wanted to see the difference.
> I found this:
> 
> static const struct dev_pm_ops wm8962_pm = {
> + SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend, pm_runtime_force_resume)
> SET_RUNTIME_PM_OPS(wm8962_runtime_suspend, wm8962_runtime_resume, NULL)
> };
> 
> I applied this, and it appears to make the issue go away on a 5.15
> kernel.  I haven't tried it on a 5.18 yet.  If this fixes the issue,
> would that be an acceptable solution to push upstream?
> 

Feels like those operations should be runtime PM, like there is
no reason to keep the CODEC in a high power state than necessary.

Let me add the necessary stuff to at least avoid the race with the
IRQ and lets see if that has any effect, although I am not totally
convinced your symptoms sound like that is the issue. But it is
fixing an issue regardless so might as well start there. Any of
the debug I requested previously would also be handy though.

Thanks,
Charles

Re: WM8962 crashing on suspend

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 943 bytes --]

On Wed, Apr 27, 2022 at 02:57:30PM +0000, Charles Keepax wrote:
> On Wed, Apr 27, 2022 at 08:12:56AM -0500, Adam Ford wrote:

> > static const struct dev_pm_ops wm8962_pm = {
> > + SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend, pm_runtime_force_resume)
> > SET_RUNTIME_PM_OPS(wm8962_runtime_suspend, wm8962_runtime_resume, NULL)
> > };

> > I applied this, and it appears to make the issue go away on a 5.15
> > kernel.  I haven't tried it on a 5.18 yet.  If this fixes the issue,
> > would that be an acceptable solution to push upstream?

> Feels like those operations should be runtime PM, like there is
> no reason to keep the CODEC in a high power state than necessary.

The issue Adam reported was suspending during playback - if you suspend
during playback or capture the device is not idle at the point where we
start trying to suspend so it shouldn't be in runtime suspend and won't
by default be runtime suspended by the PM core.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: WM8962 crashing on suspend

[Charles Keepax]

On Wed, Apr 27, 2022 at 04:24:31PM +0100, Mark Brown wrote:
> On Wed, Apr 27, 2022 at 02:57:30PM +0000, Charles Keepax wrote:
> > On Wed, Apr 27, 2022 at 08:12:56AM -0500, Adam Ford wrote:
> > > static const struct dev_pm_ops wm8962_pm = {
> > > + SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend, pm_runtime_force_resume)
> > > SET_RUNTIME_PM_OPS(wm8962_runtime_suspend, wm8962_runtime_resume, NULL)
> > > };
> 
> > > I applied this, and it appears to make the issue go away on a 5.15
> > > kernel.  I haven't tried it on a 5.18 yet.  If this fixes the issue,
> > > would that be an acceptable solution to push upstream?
> 
> > Feels like those operations should be runtime PM, like there is
> > no reason to keep the CODEC in a high power state than necessary.
> 
> The issue Adam reported was suspending during playback - if you suspend
> during playback or capture the device is not idle at the point where we
> start trying to suspend so it shouldn't be in runtime suspend and won't
> by default be runtime suspended by the PM core.

Yeah in my head snd_soc_suspend would have been called which
would (assuming the DAI doesn't have ignore_suspend set) shut
down the DAPM graph for the audio route, causing the runtime
references to all be released and the CODEC to be suspended
through runtime_pm. Not sure if I missed something there, and
that also allows for systems where the CODEC doesn't suspend
during system suspend. That said guess there probably arn't
any use-cases for that on wm8962 and I am more than happy to
use the force_suspend ops if you are happy with it.

Thanks,
Charles

Re: WM8962 crashing on suspend

[Adam Ford]

On Wed, Apr 27, 2022 at 11:48 AM Charles Keepax
<ckeepax@opensource.cirrus.com> wrote:
>
> On Wed, Apr 27, 2022 at 04:24:31PM +0100, Mark Brown wrote:
> > On Wed, Apr 27, 2022 at 02:57:30PM +0000, Charles Keepax wrote:
> > > On Wed, Apr 27, 2022 at 08:12:56AM -0500, Adam Ford wrote:
> > > > static const struct dev_pm_ops wm8962_pm = {
> > > > + SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend, pm_runtime_force_resume)
> > > > SET_RUNTIME_PM_OPS(wm8962_runtime_suspend, wm8962_runtime_resume, NULL)
> > > > };
> >
> > > > I applied this, and it appears to make the issue go away on a 5.15
> > > > kernel.  I haven't tried it on a 5.18 yet.  If this fixes the issue,
> > > > would that be an acceptable solution to push upstream?
> >
> > > Feels like those operations should be runtime PM, like there is
> > > no reason to keep the CODEC in a high power state than necessary.
> >
> > The issue Adam reported was suspending during playback - if you suspend
> > during playback or capture the device is not idle at the point where we
> > start trying to suspend so it shouldn't be in runtime suspend and won't
> > by default be runtime suspended by the PM core.
>
> Yeah in my head snd_soc_suspend would have been called which
> would (assuming the DAI doesn't have ignore_suspend set) shut
> down the DAPM graph for the audio route, causing the runtime
> references to all be released and the CODEC to be suspended
> through runtime_pm. Not sure if I missed something there, and
> that also allows for systems where the CODEC doesn't suspend
> during system suspend. That said guess there probably arn't
> any use-cases for that on wm8962 and I am more than happy to
> use the force_suspend ops if you are happy with it.

I am not familiar with this driver or the force_suspend ops, so I am
not sure if there are going to be side-effects.
I don't mind collecting more data if it's helpful.  I probably won't
be able to add more debug info until this weekend at the earliest.

adam
>
> Thanks,
> Charles

Re: WM8962 crashing on suspend

[Charles Keepax]

On Wed, Apr 27, 2022 at 11:54:40AM -0500, Adam Ford wrote:
> On Wed, Apr 27, 2022 at 11:48 AM Charles Keepax
> <ckeepax@opensource.cirrus.com> wrote:
> > On Wed, Apr 27, 2022 at 04:24:31PM +0100, Mark Brown wrote:
> > > On Wed, Apr 27, 2022 at 02:57:30PM +0000, Charles Keepax wrote:
> > > > On Wed, Apr 27, 2022 at 08:12:56AM -0500, Adam Ford wrote:
> > > > > I applied this, and it appears to make the issue go away on a 5.15
> > > > > kernel.  I haven't tried it on a 5.18 yet.  If this fixes the issue,
> > > > > would that be an acceptable solution to push upstream?
> > >
> > > > Feels like those operations should be runtime PM, like there is
> > > > no reason to keep the CODEC in a high power state than necessary.
> > >
> > > The issue Adam reported was suspending during playback - if you suspend
> > > during playback or capture the device is not idle at the point where we
> > > start trying to suspend so it shouldn't be in runtime suspend and won't
> > > by default be runtime suspended by the PM core.
> >
> > Yeah in my head snd_soc_suspend would have been called which
> > would (assuming the DAI doesn't have ignore_suspend set) shut
> > down the DAPM graph for the audio route, causing the runtime
> > references to all be released and the CODEC to be suspended
> > through runtime_pm. Not sure if I missed something there, and
> > that also allows for systems where the CODEC doesn't suspend
> > during system suspend. That said guess there probably arn't
> > any use-cases for that on wm8962 and I am more than happy to
> > use the force_suspend ops if you are happy with it.
> 
> I am not familiar with this driver or the force_suspend ops, so I am
> not sure if there are going to be side-effects.
> I don't mind collecting more data if it's helpful.  I probably won't
> be able to add more debug info until this weekend at the earliest.
> 

Nah, its good your ok to upstream your out of tree patch, just
making sure I fill in the holes in my knowledge with Mark :-)

Thanks,
Charles

Re: WM8962 crashing on suspend

[Adam Ford]

On Thu, Apr 28, 2022 at 3:23 AM Charles Keepax
<ckeepax@opensource.cirrus.com> wrote:
>
> On Wed, Apr 27, 2022 at 11:54:40AM -0500, Adam Ford wrote:
> > On Wed, Apr 27, 2022 at 11:48 AM Charles Keepax
> > <ckeepax@opensource.cirrus.com> wrote:
> > > On Wed, Apr 27, 2022 at 04:24:31PM +0100, Mark Brown wrote:
> > > > On Wed, Apr 27, 2022 at 02:57:30PM +0000, Charles Keepax wrote:
> > > > > On Wed, Apr 27, 2022 at 08:12:56AM -0500, Adam Ford wrote:
> > > > > > I applied this, and it appears to make the issue go away on a 5.15
> > > > > > kernel.  I haven't tried it on a 5.18 yet.  If this fixes the issue,
> > > > > > would that be an acceptable solution to push upstream?
> > > >
> > > > > Feels like those operations should be runtime PM, like there is
> > > > > no reason to keep the CODEC in a high power state than necessary.
> > > >
> > > > The issue Adam reported was suspending during playback - if you suspend
> > > > during playback or capture the device is not idle at the point where we
> > > > start trying to suspend so it shouldn't be in runtime suspend and won't
> > > > by default be runtime suspended by the PM core.
> > >
> > > Yeah in my head snd_soc_suspend would have been called which
> > > would (assuming the DAI doesn't have ignore_suspend set) shut
> > > down the DAPM graph for the audio route, causing the runtime
> > > references to all be released and the CODEC to be suspended
> > > through runtime_pm. Not sure if I missed something there, and
> > > that also allows for systems where the CODEC doesn't suspend
> > > during system suspend. That said guess there probably arn't
> > > any use-cases for that on wm8962 and I am more than happy to
> > > use the force_suspend ops if you are happy with it.
> >
> > I am not familiar with this driver or the force_suspend ops, so I am
> > not sure if there are going to be side-effects.
> > I don't mind collecting more data if it's helpful.  I probably won't
> > be able to add more debug info until this weekend at the earliest.
> >
>
> Nah, its good your ok to upstream your out of tree patch, just
> making sure I fill in the holes in my knowledge with Mark :-)

I'd like to push the patch with a Fixes tag, but I am not sure that we
have a definitive hash to use.  Ideally, it'd get backported, but I am
not sure that I have the means to test it, because the hardware
platform I have doesn't go back that far.  Any thoughts? If not, I'll
just push it without a fixes tag.

adam
>
> Thanks,
> Charles

Re: WM8962 crashing on suspend

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 702 bytes --]

On Thu, Apr 28, 2022 at 07:21:39AM -0500, Adam Ford wrote:
> On Thu, Apr 28, 2022 at 3:23 AM Charles Keepax

> > Nah, its good your ok to upstream your out of tree patch, just
> > making sure I fill in the holes in my knowledge with Mark :-)

> I'd like to push the patch with a Fixes tag, but I am not sure that we
> have a definitive hash to use.  Ideally, it'd get backported, but I am
> not sure that I have the means to test it, because the hardware
> platform I have doesn't go back that far.  Any thoughts? If not, I'll
> just push it without a fixes tag.

Cc stable does just as well, though TBH just having the word "fix"
somewhere in the changelog will probably cause the bots to pick it up.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: WM8962 crashing on suspend

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 882 bytes --]

On Wed, Apr 27, 2022 at 04:48:25PM +0000, Charles Keepax wrote:

> Yeah in my head snd_soc_suspend would have been called which
> would (assuming the DAI doesn't have ignore_suspend set) shut
> down the DAPM graph for the audio route, causing the runtime
> references to all be released and the CODEC to be suspended
> through runtime_pm. Not sure if I missed something there, and

Runtime suspend won't do anything beyond tracking the reference count
when we're in the middle of system suspend IIRC, it won't actually call
the operations.

> that also allows for systems where the CODEC doesn't suspend
> during system suspend. That said guess there probably arn't
> any use-cases for that on wm8962 and I am more than happy to
> use the force_suspend ops if you are happy with it.

The other option would be to move the runtime PM stuff into the bias
level configuration I guess.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]