Re: [PATCH v2 1/3] ALSA: pcm: add snd_pcm_period_elapsed() variant without acquiring lock of PCM substream

[Takashi Iwai <tiwai@suse.de>]

On Fri, 11 Jun 2021 05:38:16 +0200,
Takashi Sakamoto wrote:
> 
> Hi,
> 
> On Thu, Jun 10, 2021 at 01:03:19PM +0200, Takashi Iwai wrote:
> > On Thu, 10 Jun 2021 12:12:43 +0200, Takashi Sakamoto wrote:
> > > 
> > > On Thu, Jun 10, 2021 at 10:36:57AM +0200, Takashi Iwai wrote:
> > > > Again, my *only* point is about the sleep.  You addition was:
> > > > 
> > > > + * Context: Any context in which lock of PCM substream is already acquired. This function may not
> > > > + * sleep.
> > > > 
> > > > where "This function may not sleep" is stated incorrectly.
> > > 
> > > Hm. Would I request you to show the detail case that the call of function
> > > (snd_pcm_period_elapsed_under_stream_lock()) goes sleep except for
> > > driver-side implementation of snd_pcm_ops.{pointer, trigger,
> > > get_time_info}? At least, in callgraph I find no function call to
> > > yield...
> > 
> > True.  But the fact that those callbacks may sleep means that the
> > function would go sleeping after all.
> 
> Thanks. After all, our discussion comes from the ambiguity that what
> has responsibility at yielding processor under the lock. I think it helpful
> to describe devide responsibilities about the yielding. I'm glad for you
> to review patch below:

Well, I don't think it's worth to mention "ALSA core may not sleep".
It's just casually so for now, but it doesn't mean that this will be
guaranteed in future.  After all, this function call may sleep in
the nonatomic mode (that's the very reason for that mode!), and the
caller has to be prepared for that, no matter whether you do sleep in
the callbacks or not.


thanks,

Takashi

> 
> ======== 8< --------
> 
> >From 98e1b8332a95935ae875c637d3ddc27e68689aa0 Mon Sep 17 00:00:00 2001
> From: Takashi Sakamoto <o-takashi@sakamocchi.jp>
> Date: Fri, 11 Jun 2021 11:03:46 +0900
> Subject: [PATCH] ALSA: pcm: add context section for documentation about
>  period-elapsed kernel APIs
> 
> This commit fulfils documentation of period-elapsed kernel APIs with their
> context section.
> 
> Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
> ---
>  sound/core/pcm_lib.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/sound/core/pcm_lib.c b/sound/core/pcm_lib.c
> index 7d5883432085..5d28d63a3216 100644
> --- a/sound/core/pcm_lib.c
> +++ b/sound/core/pcm_lib.c
> @@ -1803,6 +1803,10 @@ EXPORT_SYMBOL(snd_pcm_lib_ioctl);
>   * - .get_time_info - to retrieve audio time stamp if needed.
>   *
>   * Even if more than one periods have elapsed since the last call, you have to call this only once.
> + *
> + * Context: Any context in which lock of PCM substream is already acquired. The function may not
> + * sleep by ALSA PCM core. The function may sleep in the above callbacks by driver which should
> + * configures PCM device for it (@snd_pcm.nonatomic).
>   */
>  void snd_pcm_period_elapsed_under_stream_lock(struct snd_pcm_substream *substream)
>  {
> @@ -1836,6 +1840,10 @@ EXPORT_SYMBOL(snd_pcm_period_elapsed_under_stream_lock);
>   * It's typically called by any type of IRQ handler when hardware IRQ occurs to notify event that
>   * the batch of audio data frames as the same size as the period of buffer is already processed in
>   * audio data transmission.
> + *
> + * Context: Any context in which lock of PCM substream is not acquired yet. It depends on
> + * configuration of PCM device (@snd_pcm.nonatomic) by driver whether the function may or may not
> + * sleep by operating lock of PCM substream.
>   */
>  void snd_pcm_period_elapsed(struct snd_pcm_substream *substream)
>  {
> -- 
> 2.27.0
> 
> ======== 8< --------
> 
> Thanks
> 
> Takashi Sakamoto
>