[PATCH] ALSA: pcm: accept the OPEN state for snd_pcm_stop()

[Jaroslav Kysela]

On 13. 01. 22 13:56, Takashi Iwai wrote:
> On Thu, 13 Jan 2022 12:31:30 +0100,
> Jaroslav Kysela wrote:
>>
>> It may be useful to completely invalidate streaming under some
>> circumstances like the USB gadget detach. This case is a bit different
>> than the complete disconnection. The applications should be notified
>> that the PCM streaming is no longer available, but the recovery may be
>> expected.
>>
>> This patch adds support for SNDRV_PCM_STATE_OPEN passed
>> to snd_pcm_stop() function. Only the hw_free ioctl is allowed to free
>> the buffers in this state for a full recovery operation or the PCM file
>> handle must be closed.
>>
>> In the OPEN state, ioctls return EBADFD error (with the added hw_free
>> exception above). The applications which are not aware about this new
>> state transition (and recovery) will fail with an error. This operation
>> is expected.
>>
>> Link: https://lore.kernel.org/alsa-devel/4e17c99b-1d8a-8ebe-972c-9b1299952ff8@ivitera.com/
>> Cc: Pavel Hofman <pavel.hofman at ivitera.com>
>> Signed-off-by: Jaroslav Kysela <perex at perex.cz>
> 
> I find the idea neat, but I'm afraid that it's a bit confusing from
> the user POV as is.  Namely, the state is in OPEN, but you'd have to
> perform hw_free manually at first for moving forward; how can user
> know it?

Thanks for the feedback.

The state ioctls are also not blocked, so the PCM state can be checked when 
EBADFD is returned for the updated applications. But as noted in the comment, 
it's expected that current applications will fail (like for the disconnect).

The OPEN state can be set only when hw_params fails in the current code. So 
the applications can distinguish the hw_params error / invalidate stream 
cases. We may also add this info (flag) to the PCM status structure.

This extension can be also implemented using a new PCM state, but in the 
regard of our discussion a few months ago, it's probably not a way.

> Maybe PCM core should do hw_free by itself when hw_params is
> called with hw_free_queued.

Yes, it's a possible optimization, too. I had same idea when I post the patch. 
But the mmap is an issue here - applications must do unmap before hw_params, 
so I'm not convinced to add this auto-free to hw_params, because hw_free has 
the straight semantics (munmap before). It would be probably clever to keep 
those steps separated.

Also ideally, there may be a check in hw_params, if parameters (buffers) are 
changed, but the implementation is not so easy. Maybe we can allow OPEN -> 
PREPARE transition for this case, so the applications may just restart the 
streaming in the most light way.

This extension can be also implemented using a new PCM state, but in the 
regard of our discussion a few months ago, it's probably not a way.

> Also, do_hw_free() will skip the actual free because it's in OPEN
> state, no?

Yes, I forgot to add it. I'll sent v2 when we settle those little details.

					Jaroslav

-- 
Jaroslav Kysela <perex at perex.cz>
Linux Sound Maintainer; ALSA Project; Red Hat, Inc.