Re: [PATCH v3 1/2] ALSA: hda/tas2781: Add tas2781 HDA driver

[Takashi Iwai]

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