On Mon, 21 Aug 2023 16:43:31 +0200, Pierre-Louis Bossart wrote:
+static void tas2781_hda_playback_hook(struct device *dev, int action) +{
- struct tasdevice_priv *tas_priv = dev_get_drvdata(dev);
- dev_dbg(tas_priv->dev, "%s: action = %d\n", __func__, action);
- switch (action) {
- case HDA_GEN_PCM_ACT_OPEN:
pm_runtime_get_sync(dev);test if this actually works?
To be fair, most of driver codes don't check it, including the HD-audio core. (Actually, over 900 of 1300 calls have no check in the whole tree.)
It implies that forcing the check in each place is moot; rather the helper needs to be coded not to fail, IMO.
Maybe that's true for HDaudio, for the SoundWire parts we absolutely need to detect if the resume worked. There are more steps involved, the clock-stop mode entry/exit, context restoration, re-enumeration, etc.
I think it'd be a mistake to sit on our hands and assume the world is perfect. We have to track cases where the codec isn't properly resumed and prevent it from accessing resources that are just unavailable.
Yeah, I don't mean that it's wrong or bad to have the check. The check should be there.
But, I feel that it's time to rather switch to the proper call. Basically pm_runtime_resume_and_get() is the better alternative (except for its long naming), and we may think of converting the whole.
+static int tas2781_system_suspend(struct device *dev) +{
- struct tasdevice_priv *tas_priv = dev_get_drvdata(dev);
- int ret;
- dev_dbg(tas_priv->dev, "System Suspend\n");
- ret = pm_runtime_force_suspend(dev);
- if (ret)
return ret;that's usually the other way around, for system suspend you either want the device to be pm_runtime active, or if it's already suspended do nothing.
This is very odd to me.
This is a normal procedure, as stated in pm_runtime_force_suspend() definition:
/**
- pm_runtime_force_suspend - Force a device into suspend state if needed.
....
- Typically this function may be invoked from a system suspend callback to make
- sure the device is put into low power state and it should only be used during
- system-wide PM transitions to sleep states. It assumes that the analogous
- pm_runtime_force_resume() will be used to resume the device.
It's possible that it's fine for HDaudio, it wouldn't work in all cases for SoundWire where we have to make sure all pm_runtime suspended devices are brought back to D0 and then the regular system suspend happens. That's mainly because pm_runtime suspend relies on clock stop and system suspend does not.
In other words, this isn't a generic solution at all.
Well, I suppose rather that soundwire is an exception :)
For majority of devices, the system suspend/resume is nothing but pm_runtime_force_*() calls. e.g. take a look at DEFINE_RUNTIME_DEV_PM_OPS() in linux/pm_runtime.h.
Takashi