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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 21 Aug 2023 09:43:31 -0500
From:   Pierre-Louis Bossart <pierre-louis.bossart@...ux.intel.com>
To:     Takashi Iwai <tiwai@...e.de>
Cc:     Shenghao Ding <shenghao-ding@...com>, robh+dt@...nel.org,
        lgirdwood@...il.com, perex@...ex.cz, kevin-lu@...com,
        13916275206@....com, alsa-devel@...a-project.org,
        linux-kernel@...r.kernel.org, liam.r.girdwood@...el.com,
        mengdong.lin@...el.com, baojun.xu@...com,
        thomas.gfeller@...rop.com, peeyush@...com, navada@...com,
        broonie@...nel.org, gentuser@...il.com
Subject: Re: [PATCH v3 1/2] ALSA: hda/tas2781: Add tas2781 HDA driver


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

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