Re: [PATCH v1] soundwire: intel_auxdevice: Fix system suspend/resume handling

Rafael J. Wysocki
2025-04-25T18:10:50Z

On Fri, Apr 25, 2025 at 7:14 PM Pierre-Louis Bossart
<pierre-louis.bossart@linux.dev> wrote:
>
> On 4/24/25 20:13, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >
> > The code in intel_suspend() and intel_resume() needs to be properly
> > synchronized with runtime PM which is not the case currently, so fix
> > it.
> >
> > First of all, prevent runtime PM from triggering after intel_suspend()
> > has started because the changes made by it to the device might be
> > undone by a runtime resume of the device. For this purpose, add a
> > pm_runtime_disable() call to intel_suspend().
>
> Allow me to push back on this, because we have to be very careful with a hidden state transition that needs to happen.
>
> If a controller was suspended by pm_runtime, it will enter the clock stop mode.
>
> If the system needs to suspend, the controller has to be forced to exit the clock stop mode and the bus has to restart before we can suspend it, and that's why we had those pm_runtime_resume().
>
> Disabling pm_runtime when entering system suspend would be problematic for Intel hardware, it's a known can of worms.

No, it wouldn't AFAICS.

> It's quite possible that some of the code in intel_suspend() is no longer required because the .prepare will resume the bus properly, but I wanted to make sure this state transition out of clock-stop is known and taken into consideration.

This patch doesn't change the functionality in intel_suspend(), it
just prevents runtime resume running in parallel with it or after it
from messing up with the hardware.

I don't see why it would be unsafe to do and please feel free to prove me wrong.

What can happen after adding this pm_runtime_disable() that will make
things not work?

> Bard, is this a configuration you've tested?
>
> > Next, once runtime PM has been disabled, the runtime PM status of the
> > device cannot change, so pm_runtime_status_suspended() can be used
> > instead of pm_runtime_suspended() in intel_suspend().
> >
> > Moreover, checking the runtime PM status of the device in intel_resume()
> > is pointless because the device is going to be resumed anyway and the
> > code running in the case when the pm_runtime_suspended() check in
> > intel_resume() returns 'true' is harmful. Namely, calling
> > pm_runtime_resume() right after pm_runtime_set_active() has no effect
> > and calling pm_runtime_idle() on the device at that point is invalid
> > because the device is technically still suspended (it has not been
> > resumed yet). Remove that code altogether along with the check leading
> > to it.
> >
> > Finally, call pm_runtime_set_active() at the end intel_resume() to set
> > the runtime PM status of the device to "active" because it has just been
> > resumed and re-enable runtime PM for it after that.
>
>
>
> > Additionally, drop the setting of DPM_FLAG_SMART_SUSPEND from the
> > driver because it has never been necessary and should not have been
> > done.
>
> IIRC it was your recommendation to add this flag many moons ago... No issue to remove it, it's just not something any of the 'SoundWire' folks are knowledgeable with.

But not in this driver. It doesn't have any early-late or noirq
callbacks anyway.

Thanks!