[PATCH v2 2/2] ASoC: Intel: avs: Disconnect substream if suspend or resume fails

[Cezary Rojewski]

On 2022-11-14 3:26 PM, Takashi Iwai wrote:
> On Mon, 14 Nov 2022 15:08:17 +0100,
> Cezary Rojewski wrote:
>> On 2022-11-14 2:02 PM, Takashi Iwai wrote:
>>> Hm, that might work, but note that, once when the stream is set with
>>> the disconnected state, it won't be taken back to the normal state any
>>> longer on most sound backends.  That is, it'll be gone and won't
>>> revive unless you completely unload and reload the whole stuff.  If
>>> that's the intended behavior, that's fine, of course.  So just to make
>>> sure.
>>
>> Good point.
>>
>> Our intention: if we fail e.g.: on resume, we would like the framework
>> to invoke ->hw_free() and close us. Right now, if we pretend that
>> everything is okay, invalid actions can be performed on our
>> streams. This all comes down to userspace calling "just"
>> snd_pcm_resume(). If we had an option to opt-in to a _hw_params() +
>> _prepare() + _resume() path, then things would look differently.
> 
> So you'd rather like to make the stream appearing and working after
> re-opening the stream again?  Then DISCONNECTED state might be too
> strong.
> 
> If the broken state could be recovered by the PCM PREPARE phase, then
> we may (ab-)use XRUN state, instead.  Then application shall
> re-setup via PCM prepare call.  But if hw_free/hw_params is required,
> it won't work.

To resume properly an AudioDSP stream, operations typically found in 
hw_params() and prepare() need to be re-executed _before_ actual 
_TRIGGER_RESUME can be called on a stream.

Since implementations found in userspace apps such as aplay and 
pulseaudio first invoke snd_pcm_resume() and then snd_pcm_prepare() if 
the former fails, one could abuse this flow by doing hw_params-related 
operations in _resume() and returning a specific error code e.g.: 
-ESTRPIPE when they succeed (if they fail, the internal error code would 
be returned instead of course). Prepare tasks are left to 
snd_pcm_prepare() just like in standard case.

I believe such code is a good example of _code smell_ though.
First we abuse the error path, second we basically drop the 
_TRIGGER_RESUME entirely - after all, in such approach it mimics 
hw_params() and will always fail, either with an internal error code or 
hardcoded -ESTRPIPE. _TRIGGER_START/STOP would be reused once 
snd_pcm_prepare() succeeds causing another problem - no differentiation 
between start and resume and thus lack of information when to or when 
not to poll DRSM.

Thus, we ended up with 'state = DISCONNECTED' if we encounter a firmware 
issue during PCM-pm. Keeps things sane.

> Other than that, there is no such PCM state that forces to close and
> re-open, I'm afraid.  You can have an internal error state and let the
> stream returning an error at each operation, instead, too.
> 
> And, creating a new PCM state is very difficult.  It would influence
> way too much, IMO, as each application code has to be modified to
> handle the new PCM state.

Agree, this basically screams not to follow that path.


Regards,
Czarek