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

[Pierre-Louis Bossart]

On 8/21/23 09:57, Takashi Iwai wrote:
> 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.

Oh, I broke so many drivers by trying a well-indented conversion to
pm_runtime_resume_and_get().
The flow is different wrt -EACCESs and we ended-up with multiple errors.
In hindsight I wish we had left the legacy code alone.

>>>>> +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.

I guess you are right. SoundWire has indeed these quirky modes and
differences between SOC vendors that will force us to be extra careful
in what the codec driver implements.