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

[Takashi Iwai <tiwai@suse.de>]

On Thu, 13 Jan 2022 14:32:21 +0100,
Jaroslav Kysela wrote:
> 
> 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@ivitera.com>
> >> Signed-off-by: Jaroslav Kysela <perex@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).

OK.  So this must be for a specific new use case...

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

Right, that'll become a compatibility headache.

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

Hm, right, mmap is messy.

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

Hmm.  Reading more about those restrictions and requirements, I feel
that this might be better implemented in the gadget driver side
locally at first.  Basically we can handle similarly: add a new local
flag, set it at the stream stop, and return an error at prepare until
hw_params gets reconfigured.  This might be even smaller changes?


thanks,

Takashi