[PATCH v2 0/3] ALSA: pcm:firewire: allow to operate for period elapse event in process context

[PATCH v2 0/3] ALSA: pcm:firewire: allow to operate for period elapse event in process context

[Takashi Sakamoto]

Hi,

This patchset is revised version of my previous one:
 * https://lore.kernel.org/alsa-devel/20210609012244.24296-1-o-takashi@sakamocchi.jp/

All of drivers in ALSA firewire stack have two chances to process
isochronous packets of any isochronous context; in software IRQ context
for 1394 OHCI, and in process context of ALSA PCM application.

In the process context, callbacks of .pointer and .ack are utilized. The
callbacks are done by ALSA PCM core under acquiring lock of PCM substream,

In design of ALSA PCM core, call of snd_pcm_period_elapsed() is used for
drivers to awaken user processes from waiting for available frames. The
function voluntarily acquires lock of PCM substream, therefore it is not
called in the process context since it causes dead lock. As a workaround
to avoid the dead lock, all of drivers in ALSA firewire stack use workqueue
to delegate the call.

This patchset is my attempt for the issue. A variant of 
'snd_pcm_period_elapsed()' without lock acquisition is going to be added,
named 'snd_pcm_period_elapsed_under_stream_lock()'. The call is available
in callbacks of .pointer and .ack of snd_pcm_ops structure.

Changes from v1:
 * fix context section of kernel API documentation

Takashi Sakamoto (3):
  ALSA: pcm: add snd_pcm_period_elapsed() variant without acquiring lock
    of PCM substream
  ALSA: firewire-lib: operate for period elapse event in process context
  ALSA: firewire-lib: obsolete workqueue for period update

 include/sound/pcm.h           |  1 +
 sound/core/pcm_lib.c          | 71 +++++++++++++++++++++++++++--------
 sound/firewire/amdtp-stream.c | 46 +++++++----------------
 sound/firewire/amdtp-stream.h |  1 -
 4 files changed, 71 insertions(+), 48 deletions(-)

-- 
2.27.0

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

[Takashi Sakamoto]

Current implementation of ALSA PCM core has a kernel API,
snd_pcm_period_elapsed(), for drivers to awaken user processes from waiting
for available frames. The function voluntarily acquires lock of PCM
substream, therefore it is not called in process context for any PCM
operation since the lock is already acquired.

It is convenient for packet-oriented driver, at least for drivers to audio
and music unit in IEEE 1394 bus. The drivers are allowed by Linux
FireWire subsystem to process isochronous packets queued till recent
isochronous cycle in process context in any time.

This commit adds snd_pcm_period_elapsed() variant,
snd_pcm_period_elapsed_without_lock(), for drivers to call in the
process context.

Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
---
 include/sound/pcm.h  |  1 +
 sound/core/pcm_lib.c | 71 ++++++++++++++++++++++++++++++++++----------
 2 files changed, 57 insertions(+), 15 deletions(-)

diff --git a/include/sound/pcm.h b/include/sound/pcm.h
index 2e1200d17d0c..bae90696cd06 100644
--- a/include/sound/pcm.h
+++ b/include/sound/pcm.h
@@ -1066,6 +1066,7 @@ void snd_pcm_set_ops(struct snd_pcm * pcm, int direction,
 void snd_pcm_set_sync(struct snd_pcm_substream *substream);
 int snd_pcm_lib_ioctl(struct snd_pcm_substream *substream,
 		      unsigned int cmd, void *arg);                      
+void snd_pcm_period_elapsed_under_stream_lock(struct snd_pcm_substream *substream);
 void snd_pcm_period_elapsed(struct snd_pcm_substream *substream);
 snd_pcm_sframes_t __snd_pcm_lib_xfer(struct snd_pcm_substream *substream,
 				     void *buf, bool interleaved,
diff --git a/sound/core/pcm_lib.c b/sound/core/pcm_lib.c
index b7e3d8f44511..3488ec1e3674 100644
--- a/sound/core/pcm_lib.c
+++ b/sound/core/pcm_lib.c
@@ -1778,27 +1778,41 @@ int snd_pcm_lib_ioctl(struct snd_pcm_substream *substream,
 EXPORT_SYMBOL(snd_pcm_lib_ioctl);
 
 /**
- * snd_pcm_period_elapsed - update the pcm status for the next period
- * @substream: the pcm substream instance
+ * snd_pcm_period_elapsed_under_stream_lock() - update the status of runtime for the next period
+ *						under acquired lock of PCM substream.
+ * @substream: the instance of pcm substream.
+ *
+ * This function is called when the batch of audio data frames as the same size as the period of
+ * buffer is already processed in audio data transmission.
+ *
+ * The call of function updates the status of runtime with the latest position of audio data
+ * transmission, checks overrun and underrun over buffer, awaken user processes from waiting for
+ * available audio data frames, sampling audio timestamp, and performs stop or drain the PCM
+ * substream according to configured threshold.
+ *
+ * The function is intended to use for the case that PCM driver operates audio data frames under
+ * acquired lock of PCM substream; e.g. in callback of any operation of &snd_pcm_ops in process
+ * context. In any interrupt context, it's preferrable to use ``snd_pcm_period_elapsed()`` instead
+ * since lock of PCM substream should be acquired in advance.
  *
- * This function is called from the interrupt handler when the
- * PCM has processed the period size.  It will update the current
- * pointer, wake up sleepers, etc.
+ * Developer should pay enough attention that some callbacks in &snd_pcm_ops are done by the call of
+ * function:
  *
- * Even if more than one periods have elapsed since the last call, you
- * have to call this only once.
+ * - .pointer - to retrieve current position of audio data transmission by frame count or XRUN state.
+ * - .trigger - with SNDRV_PCM_TRIGGER_STOP at XRUN or DRAINING state.
+ * - .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. This function may not
+ * sleep.
  */
-void snd_pcm_period_elapsed(struct snd_pcm_substream *substream)
+void snd_pcm_period_elapsed_under_stream_lock(struct snd_pcm_substream *substream)
 {
 	struct snd_pcm_runtime *runtime;
-	unsigned long flags;
-
-	if (snd_BUG_ON(!substream))
-		return;
 
-	snd_pcm_stream_lock_irqsave(substream, flags);
 	if (PCM_RUNTIME_CHECK(substream))
-		goto _unlock;
+		return;
 	runtime = substream->runtime;
 
 	if (!snd_pcm_running(substream) ||
@@ -1811,7 +1825,34 @@ void snd_pcm_period_elapsed(struct snd_pcm_substream *substream)
 #endif
  _end:
 	kill_fasync(&runtime->fasync, SIGIO, POLL_IN);
- _unlock:
+}
+EXPORT_SYMBOL(snd_pcm_period_elapsed_under_stream_lock);
+
+/**
+ * snd_pcm_period_elapsed() - update the status of runtime for the next period by acquiring lock of
+ *			      PCM substream.
+ * @substream: the instance of PCM substream.
+ *
+ * This function is mostly similar to ``snd_pcm_period_elapsed_under_stream_lock()`` except for
+ * acquiring lock of PCM substream voluntarily.
+ *
+ * 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 each driver whether this function may or
+ * may not sleep due to internal call of ``snd_pcm_stream_lock_irqsave()``.
+ */
+void snd_pcm_period_elapsed(struct snd_pcm_substream *substream)
+{
+	unsigned long flags;
+
+	if (snd_BUG_ON(!substream))
+		return;
+
+	snd_pcm_stream_lock_irqsave(substream, flags);
+	snd_pcm_period_elapsed_under_stream_lock(substream);
 	snd_pcm_stream_unlock_irqrestore(substream, flags);
 }
 EXPORT_SYMBOL(snd_pcm_period_elapsed);
-- 
2.27.0

[PATCH v2 2/3] ALSA: firewire-lib: operate for period elapse event in process context

[Takashi Sakamoto]

All of drivers in ALSA firewire stack processes two chances to process
isochronous packets in any isochronous context; in software IRQ context
for 1394 OHCI, and in process context of ALSA PCM application.

In the process context, callbacks of .pointer and .ack are utilized. The
callbacks are done by ALSA PCM core under acquiring lock of PCM substream,

In design of ALSA PCM core, call of snd_pcm_period_elapsed() is used for
drivers to awaken user processes from waiting for available frames. The
function voluntarily acquires lock of PCM substream, therefore it is not
called in the process context since it causes dead lock.

As a workaround to avoid the dead lock, all of drivers in ALSA firewire
stack uses workqueue to delegate the call. A variant of
snd_pcm_period_elapsed() without lock acquisition can obsolete the
workqueue.

An extra care is needed for the callback of .pointer since it's called
from snd_pcm_period_elapsed(). The isochronous context in Linux FireWire
subsystem is safe mostly for nested call except in software IRQ context.

Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
---
 sound/firewire/amdtp-stream.c | 31 ++++++++++++++-----------------
 1 file changed, 14 insertions(+), 17 deletions(-)

diff --git a/sound/firewire/amdtp-stream.c b/sound/firewire/amdtp-stream.c
index 150ee0b9e707..426a85b56cf1 100644
--- a/sound/firewire/amdtp-stream.c
+++ b/sound/firewire/amdtp-stream.c
@@ -613,8 +613,16 @@ static void update_pcm_pointers(struct amdtp_stream *s,
 		// The program in user process should periodically check the status of intermediate
 		// buffer associated to PCM substream to process PCM frames in the buffer, instead
 		// of receiving notification of period elapsed by poll wait.
-		if (!pcm->runtime->no_period_wakeup)
-			queue_work(system_highpri_wq, &s->period_work);
+		if (!pcm->runtime->no_period_wakeup) {
+			if (in_interrupt()) {
+				// In software IRQ context for 1394 OHCI.
+				snd_pcm_period_elapsed(pcm);
+			} else {
+				// In process context of ALSA PCM application under acquired lock of
+				// PCM substream.
+				snd_pcm_period_elapsed_under_stream_lock(pcm);
+			}
+		}
 	}
 }
 
@@ -1740,22 +1748,11 @@ unsigned long amdtp_domain_stream_pcm_pointer(struct amdtp_domain *d,
 {
 	struct amdtp_stream *irq_target = d->irq_target;
 
+	// Process isochronous packets queued till recent isochronous cycle to handle PCM frames.
 	if (irq_target && amdtp_stream_running(irq_target)) {
-		// This function is called in software IRQ context of
-		// period_work or process context.
-		//
-		// When the software IRQ context was scheduled by software IRQ
-		// context of IT contexts, queued packets were already handled.
-		// Therefore, no need to flush the queue in buffer furthermore.
-		//
-		// When the process context reach here, some packets will be
-		// already queued in the buffer. These packets should be handled
-		// immediately to keep better granularity of PCM pointer.
-		//
-		// Later, the process context will sometimes schedules software
-		// IRQ context of the period_work. Then, no need to flush the
-		// queue by the same reason as described in the above
-		if (current_work() != &s->period_work)
+		// In software IRQ context, the call causes dead-lock to disable the tasklet
+		// synchronously.
+		if (!in_interrupt())
 			fw_iso_context_flush_completions(irq_target->context);
 	}
 
-- 
2.27.0

[PATCH v2 3/3] ALSA: firewire-lib: obsolete workqueue for period update

[Takashi Sakamoto]

The workqueue to notify PCM period elapse is not used anymore.

Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
---
 sound/firewire/amdtp-stream.c | 15 ---------------
 sound/firewire/amdtp-stream.h |  1 -
 2 files changed, 16 deletions(-)

diff --git a/sound/firewire/amdtp-stream.c b/sound/firewire/amdtp-stream.c
index 426a85b56cf1..1d9bc7b07df1 100644
--- a/sound/firewire/amdtp-stream.c
+++ b/sound/firewire/amdtp-stream.c
@@ -77,8 +77,6 @@
 // overrun. Actual device can skip more, then this module stops the packet streaming.
 #define IR_JUMBO_PAYLOAD_MAX_SKIP_CYCLES	5
 
-static void pcm_period_work(struct work_struct *work);
-
 /**
  * amdtp_stream_init - initialize an AMDTP stream structure
  * @s: the AMDTP stream to initialize
@@ -107,7 +105,6 @@ int amdtp_stream_init(struct amdtp_stream *s, struct fw_unit *unit,
 	s->flags = flags;
 	s->context = ERR_PTR(-1);
 	mutex_init(&s->mutex);
-	INIT_WORK(&s->period_work, pcm_period_work);
 	s->packet_index = 0;
 
 	init_waitqueue_head(&s->ready_wait);
@@ -346,7 +343,6 @@ EXPORT_SYMBOL(amdtp_stream_get_max_payload);
  */
 void amdtp_stream_pcm_prepare(struct amdtp_stream *s)
 {
-	cancel_work_sync(&s->period_work);
 	s->pcm_buffer_pointer = 0;
 	s->pcm_period_pointer = 0;
 }
@@ -626,16 +622,6 @@ static void update_pcm_pointers(struct amdtp_stream *s,
 	}
 }
 
-static void pcm_period_work(struct work_struct *work)
-{
-	struct amdtp_stream *s = container_of(work, struct amdtp_stream,
-					      period_work);
-	struct snd_pcm_substream *pcm = READ_ONCE(s->pcm);
-
-	if (pcm)
-		snd_pcm_period_elapsed(pcm);
-}
-
 static int queue_packet(struct amdtp_stream *s, struct fw_iso_packet *params,
 			bool sched_irq)
 {
@@ -1808,7 +1794,6 @@ static void amdtp_stream_stop(struct amdtp_stream *s)
 		return;
 	}
 
-	cancel_work_sync(&s->period_work);
 	fw_iso_context_stop(s->context);
 	fw_iso_context_destroy(s->context);
 	s->context = ERR_PTR(-1);
diff --git a/sound/firewire/amdtp-stream.h b/sound/firewire/amdtp-stream.h
index b25592d5f6af..1f957c946c95 100644
--- a/sound/firewire/amdtp-stream.h
+++ b/sound/firewire/amdtp-stream.h
@@ -186,7 +186,6 @@ struct amdtp_stream {
 
 	/* For a PCM substream processing. */
 	struct snd_pcm_substream *pcm;
-	struct work_struct period_work;
 	snd_pcm_uframes_t pcm_buffer_pointer;
 	unsigned int pcm_period_pointer;
 
-- 
2.27.0

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

[Takashi Iwai]

On Wed, 09 Jun 2021 16:31:43 +0200,
Takashi Sakamoto wrote:
> 
> Current implementation of ALSA PCM core has a kernel API,
> snd_pcm_period_elapsed(), for drivers to awaken user processes from waiting
> for available frames. The function voluntarily acquires lock of PCM
> substream, therefore it is not called in process context for any PCM
> operation since the lock is already acquired.
> 
> It is convenient for packet-oriented driver, at least for drivers to audio
> and music unit in IEEE 1394 bus. The drivers are allowed by Linux
> FireWire subsystem to process isochronous packets queued till recent
> isochronous cycle in process context in any time.
> 
> This commit adds snd_pcm_period_elapsed() variant,
> snd_pcm_period_elapsed_without_lock(), for drivers to call in the
> process context.
> 
> Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
> ---
>  include/sound/pcm.h  |  1 +
>  sound/core/pcm_lib.c | 71 ++++++++++++++++++++++++++++++++++----------
>  2 files changed, 57 insertions(+), 15 deletions(-)
> 
> diff --git a/include/sound/pcm.h b/include/sound/pcm.h
> index 2e1200d17d0c..bae90696cd06 100644
> --- a/include/sound/pcm.h
> +++ b/include/sound/pcm.h
> @@ -1066,6 +1066,7 @@ void snd_pcm_set_ops(struct snd_pcm * pcm, int direction,
>  void snd_pcm_set_sync(struct snd_pcm_substream *substream);
>  int snd_pcm_lib_ioctl(struct snd_pcm_substream *substream,
>  		      unsigned int cmd, void *arg);                      
> +void snd_pcm_period_elapsed_under_stream_lock(struct snd_pcm_substream *substream);
>  void snd_pcm_period_elapsed(struct snd_pcm_substream *substream);
>  snd_pcm_sframes_t __snd_pcm_lib_xfer(struct snd_pcm_substream *substream,
>  				     void *buf, bool interleaved,
> diff --git a/sound/core/pcm_lib.c b/sound/core/pcm_lib.c
> index b7e3d8f44511..3488ec1e3674 100644
> --- a/sound/core/pcm_lib.c
> +++ b/sound/core/pcm_lib.c
> @@ -1778,27 +1778,41 @@ int snd_pcm_lib_ioctl(struct snd_pcm_substream *substream,
>  EXPORT_SYMBOL(snd_pcm_lib_ioctl);
>  
>  /**
> - * snd_pcm_period_elapsed - update the pcm status for the next period
> - * @substream: the pcm substream instance
> + * snd_pcm_period_elapsed_under_stream_lock() - update the status of runtime for the next period
> + *						under acquired lock of PCM substream.
> + * @substream: the instance of pcm substream.
> + *
> + * This function is called when the batch of audio data frames as the same size as the period of
> + * buffer is already processed in audio data transmission.
> + *
> + * The call of function updates the status of runtime with the latest position of audio data
> + * transmission, checks overrun and underrun over buffer, awaken user processes from waiting for
> + * available audio data frames, sampling audio timestamp, and performs stop or drain the PCM
> + * substream according to configured threshold.
> + *
> + * The function is intended to use for the case that PCM driver operates audio data frames under
> + * acquired lock of PCM substream; e.g. in callback of any operation of &snd_pcm_ops in process
> + * context. In any interrupt context, it's preferrable to use ``snd_pcm_period_elapsed()`` instead
> + * since lock of PCM substream should be acquired in advance.
>   *
> - * This function is called from the interrupt handler when the
> - * PCM has processed the period size.  It will update the current
> - * pointer, wake up sleepers, etc.
> + * Developer should pay enough attention that some callbacks in &snd_pcm_ops are done by the call of
> + * function:
>   *
> - * Even if more than one periods have elapsed since the last call, you
> - * have to call this only once.
> + * - .pointer - to retrieve current position of audio data transmission by frame count or XRUN state.
> + * - .trigger - with SNDRV_PCM_TRIGGER_STOP at XRUN or DRAINING state.
> + * - .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. This function may not
> + * sleep.

Hm, this text still remains here.  Overlooked?


Takashi

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

[Takashi Sakamoto]

On Wed, Jun 09, 2021 at 05:27:29PM +0200, Takashi Iwai wrote:
> On Wed, 09 Jun 2021 16:31:43 +0200,
> Takashi Sakamoto wrote:
> > diff --git a/sound/core/pcm_lib.c b/sound/core/pcm_lib.c
> > index b7e3d8f44511..3488ec1e3674 100644
> > --- a/sound/core/pcm_lib.c
> > +++ b/sound/core/pcm_lib.c
> > @@ -1778,27 +1778,41 @@ int snd_pcm_lib_ioctl(struct snd_pcm_substream *substream,
> >  EXPORT_SYMBOL(snd_pcm_lib_ioctl);
> >  
> >  /**
> > - * snd_pcm_period_elapsed - update the pcm status for the next period
> > - * @substream: the pcm substream instance
> > + * snd_pcm_period_elapsed_under_stream_lock() - update the status of runtime for the next period
> > + *						under acquired lock of PCM substream.
> > + * @substream: the instance of pcm substream.
> > + *
> > + * This function is called when the batch of audio data frames as the same size as the period of
> > + * buffer is already processed in audio data transmission.
> > + *
> > + * The call of function updates the status of runtime with the latest position of audio data
> > + * transmission, checks overrun and underrun over buffer, awaken user processes from waiting for
> > + * available audio data frames, sampling audio timestamp, and performs stop or drain the PCM
> > + * substream according to configured threshold.
> > + *
> > + * The function is intended to use for the case that PCM driver operates audio data frames under
> > + * acquired lock of PCM substream; e.g. in callback of any operation of &snd_pcm_ops in process
> > + * context. In any interrupt context, it's preferrable to use ``snd_pcm_period_elapsed()`` instead
> > + * since lock of PCM substream should be acquired in advance.
> >   *
> > - * This function is called from the interrupt handler when the
> > - * PCM has processed the period size.  It will update the current
> > - * pointer, wake up sleepers, etc.
> > + * Developer should pay enough attention that some callbacks in &snd_pcm_ops are done by the call of
> > + * function:
> >   *
> > - * Even if more than one periods have elapsed since the last call, you
> > - * have to call this only once.
> > + * - .pointer - to retrieve current position of audio data transmission by frame count or XRUN state.
> > + * - .trigger - with SNDRV_PCM_TRIGGER_STOP at XRUN or DRAINING state.
> > + * - .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. This function may not
> > + * sleep.
> 
> Hm, this text still remains here.  Overlooked?

It's my intension for documentation of
snd_pcm_period_elapsed_under_stream_lock() since it's expected to call
it under acquired lock. Its implementation doesn't yield processor
voluntarily by itself. If it yielded, it would depend on implementation
of each driver for struct snd_pcm_ops.{pointer, trigger, get_time_info},
but it's not preferable implementation of driver, in my opinion.

Hm. Addition of context section seems to bring more discussion since we
should consider about several types of context; e.g. threadirqs. Although
the documentation for the detail is the part of my intension in the
patchset, it's not the center. I'm sorry to reviewers but let me delete
the section in next version...


Thanks

Takashi Sakamoto

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

[Takashi Iwai]

On Thu, 10 Jun 2021 01:16:23 +0200,
Takashi Sakamoto wrote:
> 
> On Wed, Jun 09, 2021 at 05:27:29PM +0200, Takashi Iwai wrote:
> > On Wed, 09 Jun 2021 16:31:43 +0200,
> > Takashi Sakamoto wrote:
> > > diff --git a/sound/core/pcm_lib.c b/sound/core/pcm_lib.c
> > > index b7e3d8f44511..3488ec1e3674 100644
> > > --- a/sound/core/pcm_lib.c
> > > +++ b/sound/core/pcm_lib.c
> > > @@ -1778,27 +1778,41 @@ int snd_pcm_lib_ioctl(struct snd_pcm_substream *substream,
> > >  EXPORT_SYMBOL(snd_pcm_lib_ioctl);
> > >  
> > >  /**
> > > - * snd_pcm_period_elapsed - update the pcm status for the next period
> > > - * @substream: the pcm substream instance
> > > + * snd_pcm_period_elapsed_under_stream_lock() - update the status of runtime for the next period
> > > + *						under acquired lock of PCM substream.
> > > + * @substream: the instance of pcm substream.
> > > + *
> > > + * This function is called when the batch of audio data frames as the same size as the period of
> > > + * buffer is already processed in audio data transmission.
> > > + *
> > > + * The call of function updates the status of runtime with the latest position of audio data
> > > + * transmission, checks overrun and underrun over buffer, awaken user processes from waiting for
> > > + * available audio data frames, sampling audio timestamp, and performs stop or drain the PCM
> > > + * substream according to configured threshold.
> > > + *
> > > + * The function is intended to use for the case that PCM driver operates audio data frames under
> > > + * acquired lock of PCM substream; e.g. in callback of any operation of &snd_pcm_ops in process
> > > + * context. In any interrupt context, it's preferrable to use ``snd_pcm_period_elapsed()`` instead
> > > + * since lock of PCM substream should be acquired in advance.
> > >   *
> > > - * This function is called from the interrupt handler when the
> > > - * PCM has processed the period size.  It will update the current
> > > - * pointer, wake up sleepers, etc.
> > > + * Developer should pay enough attention that some callbacks in &snd_pcm_ops are done by the call of
> > > + * function:
> > >   *
> > > - * Even if more than one periods have elapsed since the last call, you
> > > - * have to call this only once.
> > > + * - .pointer - to retrieve current position of audio data transmission by frame count or XRUN state.
> > > + * - .trigger - with SNDRV_PCM_TRIGGER_STOP at XRUN or DRAINING state.
> > > + * - .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. This function may not
> > > + * sleep.
> > 
> > Hm, this text still remains here.  Overlooked?
> 
> It's my intension for documentation of
> snd_pcm_period_elapsed_under_stream_lock() since it's expected to call
> it under acquired lock. Its implementation doesn't yield processor
> voluntarily by itself. If it yielded, it would depend on implementation
> of each driver for struct snd_pcm_ops.{pointer, trigger, get_time_info},
> but it's not preferable implementation of driver, in my opinion.

My point is again about the sleep.  This function may sleep in the
nonatomic mode.  The type of the PCM stream lock depends on it.


Takashi

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

[Takashi Sakamoto]

On Thu, Jun 10, 2021 at 09:39:37AM +0200, Takashi Iwai wrote:
> On Thu, 10 Jun 2021 01:16:23 +0200,
> Takashi Sakamoto wrote:
> > 
> > On Wed, Jun 09, 2021 at 05:27:29PM +0200, Takashi Iwai wrote:
> > > On Wed, 09 Jun 2021 16:31:43 +0200,
> > > Takashi Sakamoto wrote:
> > > > diff --git a/sound/core/pcm_lib.c b/sound/core/pcm_lib.c
> > > > index b7e3d8f44511..3488ec1e3674 100644
> > > > --- a/sound/core/pcm_lib.c
> > > > +++ b/sound/core/pcm_lib.c
> > > > @@ -1778,27 +1778,41 @@ int snd_pcm_lib_ioctl(struct snd_pcm_substream *substream,
> > > >  EXPORT_SYMBOL(snd_pcm_lib_ioctl);
> > > >  
> > > >  /**
> > > > - * snd_pcm_period_elapsed - update the pcm status for the next period
> > > > - * @substream: the pcm substream instance
> > > > + * snd_pcm_period_elapsed_under_stream_lock() - update the status of runtime for the next period
> > > > + *						under acquired lock of PCM substream.
> > > > + * @substream: the instance of pcm substream.
> > > > + *
> > > > + * This function is called when the batch of audio data frames as the same size as the period of
> > > > + * buffer is already processed in audio data transmission.
> > > > + *
> > > > + * The call of function updates the status of runtime with the latest position of audio data
> > > > + * transmission, checks overrun and underrun over buffer, awaken user processes from waiting for
> > > > + * available audio data frames, sampling audio timestamp, and performs stop or drain the PCM
> > > > + * substream according to configured threshold.
> > > > + *
> > > > + * The function is intended to use for the case that PCM driver operates audio data frames under
> > > > + * acquired lock of PCM substream; e.g. in callback of any operation of &snd_pcm_ops in process
> > > > + * context. In any interrupt context, it's preferrable to use ``snd_pcm_period_elapsed()`` instead
> > > > + * since lock of PCM substream should be acquired in advance.
> > > >   *
> > > > - * This function is called from the interrupt handler when the
> > > > - * PCM has processed the period size.  It will update the current
> > > > - * pointer, wake up sleepers, etc.
> > > > + * Developer should pay enough attention that some callbacks in &snd_pcm_ops are done by the call of
> > > > + * function:
> > > >   *
> > > > - * Even if more than one periods have elapsed since the last call, you
> > > > - * have to call this only once.
> > > > + * - .pointer - to retrieve current position of audio data transmission by frame count or XRUN state.
> > > > + * - .trigger - with SNDRV_PCM_TRIGGER_STOP at XRUN or DRAINING state.
> > > > + * - .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. This function may not
> > > > + * sleep.
> > > 
> > > Hm, this text still remains here.  Overlooked?
> > 
> > It's my intension for documentation of
> > snd_pcm_period_elapsed_under_stream_lock() since it's expected to call
> > it under acquired lock. Its implementation doesn't yield processor
> > voluntarily by itself. If it yielded, it would depend on implementation
> > of each driver for struct snd_pcm_ops.{pointer, trigger, get_time_info},
> > but it's not preferable implementation of driver, in my opinion.
> 
> My point is again about the sleep.  This function may sleep in the
> nonatomic mode.  The type of the PCM stream lock depends on it.

Would I simply request you to show how the added function yields except
for the driver implementation? The lock of stream is expected to be
acquired already.


Regards

Takashi Sakamoto

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

[Takashi Iwai]

On Thu, 10 Jun 2021 10:05:21 +0200,
Takashi Sakamoto wrote:
> 
> On Thu, Jun 10, 2021 at 09:39:37AM +0200, Takashi Iwai wrote:
> > On Thu, 10 Jun 2021 01:16:23 +0200,
> > Takashi Sakamoto wrote:
> > > 
> > > On Wed, Jun 09, 2021 at 05:27:29PM +0200, Takashi Iwai wrote:
> > > > On Wed, 09 Jun 2021 16:31:43 +0200,
> > > > Takashi Sakamoto wrote:
> > > > > diff --git a/sound/core/pcm_lib.c b/sound/core/pcm_lib.c
> > > > > index b7e3d8f44511..3488ec1e3674 100644
> > > > > --- a/sound/core/pcm_lib.c
> > > > > +++ b/sound/core/pcm_lib.c
> > > > > @@ -1778,27 +1778,41 @@ int snd_pcm_lib_ioctl(struct snd_pcm_substream *substream,
> > > > >  EXPORT_SYMBOL(snd_pcm_lib_ioctl);
> > > > >  
> > > > >  /**
> > > > > - * snd_pcm_period_elapsed - update the pcm status for the next period
> > > > > - * @substream: the pcm substream instance
> > > > > + * snd_pcm_period_elapsed_under_stream_lock() - update the status of runtime for the next period
> > > > > + *						under acquired lock of PCM substream.
> > > > > + * @substream: the instance of pcm substream.
> > > > > + *
> > > > > + * This function is called when the batch of audio data frames as the same size as the period of
> > > > > + * buffer is already processed in audio data transmission.
> > > > > + *
> > > > > + * The call of function updates the status of runtime with the latest position of audio data
> > > > > + * transmission, checks overrun and underrun over buffer, awaken user processes from waiting for
> > > > > + * available audio data frames, sampling audio timestamp, and performs stop or drain the PCM
> > > > > + * substream according to configured threshold.
> > > > > + *
> > > > > + * The function is intended to use for the case that PCM driver operates audio data frames under
> > > > > + * acquired lock of PCM substream; e.g. in callback of any operation of &snd_pcm_ops in process
> > > > > + * context. In any interrupt context, it's preferrable to use ``snd_pcm_period_elapsed()`` instead
> > > > > + * since lock of PCM substream should be acquired in advance.
> > > > >   *
> > > > > - * This function is called from the interrupt handler when the
> > > > > - * PCM has processed the period size.  It will update the current
> > > > > - * pointer, wake up sleepers, etc.
> > > > > + * Developer should pay enough attention that some callbacks in &snd_pcm_ops are done by the call of
> > > > > + * function:
> > > > >   *
> > > > > - * Even if more than one periods have elapsed since the last call, you
> > > > > - * have to call this only once.
> > > > > + * - .pointer - to retrieve current position of audio data transmission by frame count or XRUN state.
> > > > > + * - .trigger - with SNDRV_PCM_TRIGGER_STOP at XRUN or DRAINING state.
> > > > > + * - .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. This function may not
> > > > > + * sleep.
> > > > 
> > > > Hm, this text still remains here.  Overlooked?
> > > 
> > > It's my intension for documentation of
> > > snd_pcm_period_elapsed_under_stream_lock() since it's expected to call
> > > it under acquired lock. Its implementation doesn't yield processor
> > > voluntarily by itself. If it yielded, it would depend on implementation
> > > of each driver for struct snd_pcm_ops.{pointer, trigger, get_time_info},
> > > but it's not preferable implementation of driver, in my opinion.
> > 
> > My point is again about the sleep.  This function may sleep in the
> > nonatomic mode.  The type of the PCM stream lock depends on it.
> 
> Would I simply request you to show how the added function yields except
> for the driver implementation? The lock of stream is expected to be
> acquired already.

In the nonatomic mode, the PCM stream lock is a mutex (no
spin_lock_irqsave), hence it can sleep -- which contradicts with the
added description above.

Or do I misunderstand your question...? 


Takashi

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

[Takashi Sakamoto]

On Thu, Jun 10, 2021 at 10:08:39AM +0200, Takashi Iwai wrote:
> On Thu, 10 Jun 2021 10:05:21 +0200,
> Takashi Sakamoto wrote:
> > 
> > On Thu, Jun 10, 2021 at 09:39:37AM +0200, Takashi Iwai wrote:
> > > On Thu, 10 Jun 2021 01:16:23 +0200,
> > > Takashi Sakamoto wrote:
> > > > 
> > > > On Wed, Jun 09, 2021 at 05:27:29PM +0200, Takashi Iwai wrote:
> > > > > On Wed, 09 Jun 2021 16:31:43 +0200,
> > > > > Takashi Sakamoto wrote:
> > > > > > diff --git a/sound/core/pcm_lib.c b/sound/core/pcm_lib.c
> > > > > > index b7e3d8f44511..3488ec1e3674 100644
> > > > > > --- a/sound/core/pcm_lib.c
> > > > > > +++ b/sound/core/pcm_lib.c
> > > > > > @@ -1778,27 +1778,41 @@ int snd_pcm_lib_ioctl(struct snd_pcm_substream *substream,
> > > > > >  EXPORT_SYMBOL(snd_pcm_lib_ioctl);
> > > > > >  
> > > > > >  /**
> > > > > > - * snd_pcm_period_elapsed - update the pcm status for the next period
> > > > > > - * @substream: the pcm substream instance
> > > > > > + * snd_pcm_period_elapsed_under_stream_lock() - update the status of runtime for the next period
> > > > > > + *						under acquired lock of PCM substream.
> > > > > > + * @substream: the instance of pcm substream.
> > > > > > + *
> > > > > > + * This function is called when the batch of audio data frames as the same size as the period of
> > > > > > + * buffer is already processed in audio data transmission.
> > > > > > + *
> > > > > > + * The call of function updates the status of runtime with the latest position of audio data
> > > > > > + * transmission, checks overrun and underrun over buffer, awaken user processes from waiting for
> > > > > > + * available audio data frames, sampling audio timestamp, and performs stop or drain the PCM
> > > > > > + * substream according to configured threshold.
> > > > > > + *
> > > > > > + * The function is intended to use for the case that PCM driver operates audio data frames under
> > > > > > + * acquired lock of PCM substream; e.g. in callback of any operation of &snd_pcm_ops in process
> > > > > > + * context. In any interrupt context, it's preferrable to use ``snd_pcm_period_elapsed()`` instead
> > > > > > + * since lock of PCM substream should be acquired in advance.
> > > > > >   *
> > > > > > - * This function is called from the interrupt handler when the
> > > > > > - * PCM has processed the period size.  It will update the current
> > > > > > - * pointer, wake up sleepers, etc.
> > > > > > + * Developer should pay enough attention that some callbacks in &snd_pcm_ops are done by the call of
> > > > > > + * function:
> > > > > >   *
> > > > > > - * Even if more than one periods have elapsed since the last call, you
> > > > > > - * have to call this only once.
> > > > > > + * - .pointer - to retrieve current position of audio data transmission by frame count or XRUN state.
> > > > > > + * - .trigger - with SNDRV_PCM_TRIGGER_STOP at XRUN or DRAINING state.
> > > > > > + * - .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. This function may not
> > > > > > + * sleep.
> > > > > 
> > > > > Hm, this text still remains here.  Overlooked?
> > > > 
> > > > It's my intension for documentation of
> > > > snd_pcm_period_elapsed_under_stream_lock() since it's expected to call
> > > > it under acquired lock. Its implementation doesn't yield processor
> > > > voluntarily by itself. If it yielded, it would depend on implementation
> > > > of each driver for struct snd_pcm_ops.{pointer, trigger, get_time_info},
> > > > but it's not preferable implementation of driver, in my opinion.
> > > 
> > > My point is again about the sleep.  This function may sleep in the
> > > nonatomic mode.  The type of the PCM stream lock depends on it.
> > 
> > Would I simply request you to show how the added function yields except
> > for the driver implementation? The lock of stream is expected to be
> > acquired already.
> 
> In the nonatomic mode, the PCM stream lock is a mutex (no
> spin_lock_irqsave), hence it can sleep -- which contradicts with the
> added description above.
> 
> Or do I misunderstand your question...? 

Thanks to clarify the role of PCM stream lock, and I'm ease that we have
the same understanding about the lock.

Here, let us see deleted/added line again.

> diff --git a/sound/core/pcm_lib.c b/sound/core/pcm_lib.c
> index b7e3d8f44511..3488ec1e3674 100644
> --- a/sound/core/pcm_lib.c
> +++ b/sound/core/pcm_lib.c
> @@ -1778,27 +1778,41 @@ int snd_pcm_lib_ioctl(struct snd_pcm_substream *substream,
>  EXPORT_SYMBOL(snd_pcm_lib_ioctl);
>  
>  /**
> - * snd_pcm_period_elapsed - update the pcm status for the next period
> - * @substream: the pcm substream instance
> + * snd_pcm_period_elapsed_under_stream_lock() - update the status of runtime for the next period
> + *						under acquired lock of PCM substream.
> + ...
> + * Context: Any context in which lock of PCM substream is already acquired. This function may not
> + * sleep.

The issued documentation is for the new function. Inner the function, the
lock of PCM substream is not acquired again since it causes dead lock
(it's not nest-able lock) regardless of usage of mutex or spin_lock.

The well-known function, snd_pcm_period_elapsed(), is rewritten to call
the new function between lock/unlock operations:

->snd_pcm_period_elapsed()
  ->snd_pcm_stream_lock_irqsave()
  ->snd_pcm_period_elapsed_under_stream_lock()
  ->snd_pcm_stream_unlock_irqrestore()

Or the new function can acquire the lock somewhere I overlook? However I
think it is unlikely since it necessarily causes dead lock or corruption
of irq context...


Thanks

Takashi Sakamoto

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

[Takashi Iwai]

On Thu, 10 Jun 2021 10:26:22 +0200,
Takashi Sakamoto wrote:
> 
> On Thu, Jun 10, 2021 at 10:08:39AM +0200, Takashi Iwai wrote:
> > On Thu, 10 Jun 2021 10:05:21 +0200,
> > Takashi Sakamoto wrote:
> > > 
> > > On Thu, Jun 10, 2021 at 09:39:37AM +0200, Takashi Iwai wrote:
> > > > On Thu, 10 Jun 2021 01:16:23 +0200,
> > > > Takashi Sakamoto wrote:
> > > > > 
> > > > > On Wed, Jun 09, 2021 at 05:27:29PM +0200, Takashi Iwai wrote:
> > > > > > On Wed, 09 Jun 2021 16:31:43 +0200,
> > > > > > Takashi Sakamoto wrote:
> > > > > > > diff --git a/sound/core/pcm_lib.c b/sound/core/pcm_lib.c
> > > > > > > index b7e3d8f44511..3488ec1e3674 100644
> > > > > > > --- a/sound/core/pcm_lib.c
> > > > > > > +++ b/sound/core/pcm_lib.c
> > > > > > > @@ -1778,27 +1778,41 @@ int snd_pcm_lib_ioctl(struct snd_pcm_substream *substream,
> > > > > > >  EXPORT_SYMBOL(snd_pcm_lib_ioctl);
> > > > > > >  
> > > > > > >  /**
> > > > > > > - * snd_pcm_period_elapsed - update the pcm status for the next period
> > > > > > > - * @substream: the pcm substream instance
> > > > > > > + * snd_pcm_period_elapsed_under_stream_lock() - update the status of runtime for the next period
> > > > > > > + *						under acquired lock of PCM substream.
> > > > > > > + * @substream: the instance of pcm substream.
> > > > > > > + *
> > > > > > > + * This function is called when the batch of audio data frames as the same size as the period of
> > > > > > > + * buffer is already processed in audio data transmission.
> > > > > > > + *
> > > > > > > + * The call of function updates the status of runtime with the latest position of audio data
> > > > > > > + * transmission, checks overrun and underrun over buffer, awaken user processes from waiting for
> > > > > > > + * available audio data frames, sampling audio timestamp, and performs stop or drain the PCM
> > > > > > > + * substream according to configured threshold.
> > > > > > > + *
> > > > > > > + * The function is intended to use for the case that PCM driver operates audio data frames under
> > > > > > > + * acquired lock of PCM substream; e.g. in callback of any operation of &snd_pcm_ops in process
> > > > > > > + * context. In any interrupt context, it's preferrable to use ``snd_pcm_period_elapsed()`` instead
> > > > > > > + * since lock of PCM substream should be acquired in advance.
> > > > > > >   *
> > > > > > > - * This function is called from the interrupt handler when the
> > > > > > > - * PCM has processed the period size.  It will update the current
> > > > > > > - * pointer, wake up sleepers, etc.
> > > > > > > + * Developer should pay enough attention that some callbacks in &snd_pcm_ops are done by the call of
> > > > > > > + * function:
> > > > > > >   *
> > > > > > > - * Even if more than one periods have elapsed since the last call, you
> > > > > > > - * have to call this only once.
> > > > > > > + * - .pointer - to retrieve current position of audio data transmission by frame count or XRUN state.
> > > > > > > + * - .trigger - with SNDRV_PCM_TRIGGER_STOP at XRUN or DRAINING state.
> > > > > > > + * - .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. This function may not
> > > > > > > + * sleep.
> > > > > > 
> > > > > > Hm, this text still remains here.  Overlooked?
> > > > > 
> > > > > It's my intension for documentation of
> > > > > snd_pcm_period_elapsed_under_stream_lock() since it's expected to call
> > > > > it under acquired lock. Its implementation doesn't yield processor
> > > > > voluntarily by itself. If it yielded, it would depend on implementation
> > > > > of each driver for struct snd_pcm_ops.{pointer, trigger, get_time_info},
> > > > > but it's not preferable implementation of driver, in my opinion.
> > > > 
> > > > My point is again about the sleep.  This function may sleep in the
> > > > nonatomic mode.  The type of the PCM stream lock depends on it.
> > > 
> > > Would I simply request you to show how the added function yields except
> > > for the driver implementation? The lock of stream is expected to be
> > > acquired already.
> > 
> > In the nonatomic mode, the PCM stream lock is a mutex (no
> > spin_lock_irqsave), hence it can sleep -- which contradicts with the
> > added description above.
> > 
> > Or do I misunderstand your question...? 
> 
> Thanks to clarify the role of PCM stream lock, and I'm ease that we have
> the same understanding about the lock.
> 
> Here, let us see deleted/added line again.
> 
> > diff --git a/sound/core/pcm_lib.c b/sound/core/pcm_lib.c
> > index b7e3d8f44511..3488ec1e3674 100644
> > --- a/sound/core/pcm_lib.c
> > +++ b/sound/core/pcm_lib.c
> > @@ -1778,27 +1778,41 @@ int snd_pcm_lib_ioctl(struct snd_pcm_substream *substream,
> >  EXPORT_SYMBOL(snd_pcm_lib_ioctl);
> >  
> >  /**
> > - * snd_pcm_period_elapsed - update the pcm status for the next period
> > - * @substream: the pcm substream instance
> > + * snd_pcm_period_elapsed_under_stream_lock() - update the status of runtime for the next period
> > + *						under acquired lock of PCM substream.
> > + ...
> > + * Context: Any context in which lock of PCM substream is already acquired. This function may not
> > + * sleep.
> 
> The issued documentation is for the new function. Inner the function, the
> lock of PCM substream is not acquired again since it causes dead lock
> (it's not nest-able lock) regardless of usage of mutex or spin_lock.
> 
> The well-known function, snd_pcm_period_elapsed(), is rewritten to call
> the new function between lock/unlock operations:
> 
> ->snd_pcm_period_elapsed()
>   ->snd_pcm_stream_lock_irqsave()
>   ->snd_pcm_period_elapsed_under_stream_lock()
>   ->snd_pcm_stream_unlock_irqrestore()
> 
> Or the new function can acquire the lock somewhere I overlook? However I
> think it is unlikely since it necessarily causes dead lock or corruption
> of irq context...

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.


Takashi

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

[Takashi Sakamoto]

On Thu, Jun 10, 2021 at 10:36:57AM +0200, Takashi Iwai wrote:
> On Thu, 10 Jun 2021 10:26:22 +0200,
> Takashi Sakamoto wrote:
> > 
> > On Thu, Jun 10, 2021 at 10:08:39AM +0200, Takashi Iwai wrote:
> > > On Thu, 10 Jun 2021 10:05:21 +0200,
> > > Takashi Sakamoto wrote:
> > > > 
> > > > On Thu, Jun 10, 2021 at 09:39:37AM +0200, Takashi Iwai wrote:
> > > > > On Thu, 10 Jun 2021 01:16:23 +0200,
> > > > > Takashi Sakamoto wrote:
> > > > > > 
> > > > > > On Wed, Jun 09, 2021 at 05:27:29PM +0200, Takashi Iwai wrote:
> > > > > > > On Wed, 09 Jun 2021 16:31:43 +0200,
> > > > > > > Takashi Sakamoto wrote:
> > > > > > > > diff --git a/sound/core/pcm_lib.c b/sound/core/pcm_lib.c
> > > > > > > > index b7e3d8f44511..3488ec1e3674 100644
> > > > > > > > --- a/sound/core/pcm_lib.c
> > > > > > > > +++ b/sound/core/pcm_lib.c
> > > > > > > > @@ -1778,27 +1778,41 @@ int snd_pcm_lib_ioctl(struct snd_pcm_substream *substream,
> > > > > > > >  EXPORT_SYMBOL(snd_pcm_lib_ioctl);
> > > > > > > >  
> > > > > > > >  /**
> > > > > > > > - * snd_pcm_period_elapsed - update the pcm status for the next period
> > > > > > > > - * @substream: the pcm substream instance
> > > > > > > > + * snd_pcm_period_elapsed_under_stream_lock() - update the status of runtime for the next period
> > > > > > > > + *						under acquired lock of PCM substream.
> > > > > > > > + * @substream: the instance of pcm substream.
> > > > > > > > + *
> > > > > > > > + * This function is called when the batch of audio data frames as the same size as the period of
> > > > > > > > + * buffer is already processed in audio data transmission.
> > > > > > > > + *
> > > > > > > > + * The call of function updates the status of runtime with the latest position of audio data
> > > > > > > > + * transmission, checks overrun and underrun over buffer, awaken user processes from waiting for
> > > > > > > > + * available audio data frames, sampling audio timestamp, and performs stop or drain the PCM
> > > > > > > > + * substream according to configured threshold.
> > > > > > > > + *
> > > > > > > > + * The function is intended to use for the case that PCM driver operates audio data frames under
> > > > > > > > + * acquired lock of PCM substream; e.g. in callback of any operation of &snd_pcm_ops in process
> > > > > > > > + * context. In any interrupt context, it's preferrable to use ``snd_pcm_period_elapsed()`` instead
> > > > > > > > + * since lock of PCM substream should be acquired in advance.
> > > > > > > >   *
> > > > > > > > - * This function is called from the interrupt handler when the
> > > > > > > > - * PCM has processed the period size.  It will update the current
> > > > > > > > - * pointer, wake up sleepers, etc.
> > > > > > > > + * Developer should pay enough attention that some callbacks in &snd_pcm_ops are done by the call of
> > > > > > > > + * function:
> > > > > > > >   *
> > > > > > > > - * Even if more than one periods have elapsed since the last call, you
> > > > > > > > - * have to call this only once.
> > > > > > > > + * - .pointer - to retrieve current position of audio data transmission by frame count or XRUN state.
> > > > > > > > + * - .trigger - with SNDRV_PCM_TRIGGER_STOP at XRUN or DRAINING state.
> > > > > > > > + * - .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. This function may not
> > > > > > > > + * sleep.
> > > > > > > 
> > > > > > > Hm, this text still remains here.  Overlooked?
> > > > > > 
> > > > > > It's my intension for documentation of
> > > > > > snd_pcm_period_elapsed_under_stream_lock() since it's expected to call
> > > > > > it under acquired lock. Its implementation doesn't yield processor
> > > > > > voluntarily by itself. If it yielded, it would depend on implementation
> > > > > > of each driver for struct snd_pcm_ops.{pointer, trigger, get_time_info},
> > > > > > but it's not preferable implementation of driver, in my opinion.
> > > > > 
> > > > > My point is again about the sleep.  This function may sleep in the
> > > > > nonatomic mode.  The type of the PCM stream lock depends on it.
> > > > 
> > > > Would I simply request you to show how the added function yields except
> > > > for the driver implementation? The lock of stream is expected to be
> > > > acquired already.
> > > 
> > > In the nonatomic mode, the PCM stream lock is a mutex (no
> > > spin_lock_irqsave), hence it can sleep -- which contradicts with the
> > > added description above.
> > > 
> > > Or do I misunderstand your question...? 
> > 
> > Thanks to clarify the role of PCM stream lock, and I'm ease that we have
> > the same understanding about the lock.
> > 
> > Here, let us see deleted/added line again.
> > 
> > > diff --git a/sound/core/pcm_lib.c b/sound/core/pcm_lib.c
> > > index b7e3d8f44511..3488ec1e3674 100644
> > > --- a/sound/core/pcm_lib.c
> > > +++ b/sound/core/pcm_lib.c
> > > @@ -1778,27 +1778,41 @@ int snd_pcm_lib_ioctl(struct snd_pcm_substream *substream,
> > >  EXPORT_SYMBOL(snd_pcm_lib_ioctl);
> > >  
> > >  /**
> > > - * snd_pcm_period_elapsed - update the pcm status for the next period
> > > - * @substream: the pcm substream instance
> > > + * snd_pcm_period_elapsed_under_stream_lock() - update the status of runtime for the next period
> > > + *						under acquired lock of PCM substream.
> > > + ...
> > > + * Context: Any context in which lock of PCM substream is already acquired. This function may not
> > > + * sleep.
> > 
> > The issued documentation is for the new function. Inner the function, the
> > lock of PCM substream is not acquired again since it causes dead lock
> > (it's not nest-able lock) regardless of usage of mutex or spin_lock.
> > 
> > The well-known function, snd_pcm_period_elapsed(), is rewritten to call
> > the new function between lock/unlock operations:
> > 
> > ->snd_pcm_period_elapsed()
> >   ->snd_pcm_stream_lock_irqsave()
> >   ->snd_pcm_period_elapsed_under_stream_lock()
> >   ->snd_pcm_stream_unlock_irqrestore()
> > 
> > Or the new function can acquire the lock somewhere I overlook? However I
> > think it is unlikely since it necessarily causes dead lock or corruption
> > of irq context...
> 
> 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...


Regards

Takashi Sakamoto

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

[Takashi Iwai]

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:
> > On Thu, 10 Jun 2021 10:26:22 +0200,
> > Takashi Sakamoto wrote:
> > > 
> > > On Thu, Jun 10, 2021 at 10:08:39AM +0200, Takashi Iwai wrote:
> > > > On Thu, 10 Jun 2021 10:05:21 +0200,
> > > > Takashi Sakamoto wrote:
> > > > > 
> > > > > On Thu, Jun 10, 2021 at 09:39:37AM +0200, Takashi Iwai wrote:
> > > > > > On Thu, 10 Jun 2021 01:16:23 +0200,
> > > > > > Takashi Sakamoto wrote:
> > > > > > > 
> > > > > > > On Wed, Jun 09, 2021 at 05:27:29PM +0200, Takashi Iwai wrote:
> > > > > > > > On Wed, 09 Jun 2021 16:31:43 +0200,
> > > > > > > > Takashi Sakamoto wrote:
> > > > > > > > > diff --git a/sound/core/pcm_lib.c b/sound/core/pcm_lib.c
> > > > > > > > > index b7e3d8f44511..3488ec1e3674 100644
> > > > > > > > > --- a/sound/core/pcm_lib.c
> > > > > > > > > +++ b/sound/core/pcm_lib.c
> > > > > > > > > @@ -1778,27 +1778,41 @@ int snd_pcm_lib_ioctl(struct snd_pcm_substream *substream,
> > > > > > > > >  EXPORT_SYMBOL(snd_pcm_lib_ioctl);
> > > > > > > > >  
> > > > > > > > >  /**
> > > > > > > > > - * snd_pcm_period_elapsed - update the pcm status for the next period
> > > > > > > > > - * @substream: the pcm substream instance
> > > > > > > > > + * snd_pcm_period_elapsed_under_stream_lock() - update the status of runtime for the next period
> > > > > > > > > + *						under acquired lock of PCM substream.
> > > > > > > > > + * @substream: the instance of pcm substream.
> > > > > > > > > + *
> > > > > > > > > + * This function is called when the batch of audio data frames as the same size as the period of
> > > > > > > > > + * buffer is already processed in audio data transmission.
> > > > > > > > > + *
> > > > > > > > > + * The call of function updates the status of runtime with the latest position of audio data
> > > > > > > > > + * transmission, checks overrun and underrun over buffer, awaken user processes from waiting for
> > > > > > > > > + * available audio data frames, sampling audio timestamp, and performs stop or drain the PCM
> > > > > > > > > + * substream according to configured threshold.
> > > > > > > > > + *
> > > > > > > > > + * The function is intended to use for the case that PCM driver operates audio data frames under
> > > > > > > > > + * acquired lock of PCM substream; e.g. in callback of any operation of &snd_pcm_ops in process
> > > > > > > > > + * context. In any interrupt context, it's preferrable to use ``snd_pcm_period_elapsed()`` instead
> > > > > > > > > + * since lock of PCM substream should be acquired in advance.
> > > > > > > > >   *
> > > > > > > > > - * This function is called from the interrupt handler when the
> > > > > > > > > - * PCM has processed the period size.  It will update the current
> > > > > > > > > - * pointer, wake up sleepers, etc.
> > > > > > > > > + * Developer should pay enough attention that some callbacks in &snd_pcm_ops are done by the call of
> > > > > > > > > + * function:
> > > > > > > > >   *
> > > > > > > > > - * Even if more than one periods have elapsed since the last call, you
> > > > > > > > > - * have to call this only once.
> > > > > > > > > + * - .pointer - to retrieve current position of audio data transmission by frame count or XRUN state.
> > > > > > > > > + * - .trigger - with SNDRV_PCM_TRIGGER_STOP at XRUN or DRAINING state.
> > > > > > > > > + * - .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. This function may not
> > > > > > > > > + * sleep.
> > > > > > > > 
> > > > > > > > Hm, this text still remains here.  Overlooked?
> > > > > > > 
> > > > > > > It's my intension for documentation of
> > > > > > > snd_pcm_period_elapsed_under_stream_lock() since it's expected to call
> > > > > > > it under acquired lock. Its implementation doesn't yield processor
> > > > > > > voluntarily by itself. If it yielded, it would depend on implementation
> > > > > > > of each driver for struct snd_pcm_ops.{pointer, trigger, get_time_info},
> > > > > > > but it's not preferable implementation of driver, in my opinion.
> > > > > > 
> > > > > > My point is again about the sleep.  This function may sleep in the
> > > > > > nonatomic mode.  The type of the PCM stream lock depends on it.
> > > > > 
> > > > > Would I simply request you to show how the added function yields except
> > > > > for the driver implementation? The lock of stream is expected to be
> > > > > acquired already.
> > > > 
> > > > In the nonatomic mode, the PCM stream lock is a mutex (no
> > > > spin_lock_irqsave), hence it can sleep -- which contradicts with the
> > > > added description above.
> > > > 
> > > > Or do I misunderstand your question...? 
> > > 
> > > Thanks to clarify the role of PCM stream lock, and I'm ease that we have
> > > the same understanding about the lock.
> > > 
> > > Here, let us see deleted/added line again.
> > > 
> > > > diff --git a/sound/core/pcm_lib.c b/sound/core/pcm_lib.c
> > > > index b7e3d8f44511..3488ec1e3674 100644
> > > > --- a/sound/core/pcm_lib.c
> > > > +++ b/sound/core/pcm_lib.c
> > > > @@ -1778,27 +1778,41 @@ int snd_pcm_lib_ioctl(struct snd_pcm_substream *substream,
> > > >  EXPORT_SYMBOL(snd_pcm_lib_ioctl);
> > > >  
> > > >  /**
> > > > - * snd_pcm_period_elapsed - update the pcm status for the next period
> > > > - * @substream: the pcm substream instance
> > > > + * snd_pcm_period_elapsed_under_stream_lock() - update the status of runtime for the next period
> > > > + *						under acquired lock of PCM substream.
> > > > + ...
> > > > + * Context: Any context in which lock of PCM substream is already acquired. This function may not
> > > > + * sleep.
> > > 
> > > The issued documentation is for the new function. Inner the function, the
> > > lock of PCM substream is not acquired again since it causes dead lock
> > > (it's not nest-able lock) regardless of usage of mutex or spin_lock.
> > > 
> > > The well-known function, snd_pcm_period_elapsed(), is rewritten to call
> > > the new function between lock/unlock operations:
> > > 
> > > ->snd_pcm_period_elapsed()
> > >   ->snd_pcm_stream_lock_irqsave()
> > >   ->snd_pcm_period_elapsed_under_stream_lock()
> > >   ->snd_pcm_stream_unlock_irqrestore()
> > > 
> > > Or the new function can acquire the lock somewhere I overlook? However I
> > > think it is unlikely since it necessarily causes dead lock or corruption
> > > of irq context...
> > 
> > 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.


Takashi

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

[Takashi Sakamoto]

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:

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

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

[Takashi Iwai]

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
> 

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

[Takashi Sakamoto]

On Fri, Jun 11, 2021 at 08:47:59AM +0200, Takashi Iwai wrote:
> 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.

I have an opinion that we should guarantee it as long as maintaining
existent in-kernel drivers, which call it in hw/sw IRQ context. This is
not the issue 'casually so for now'.

If you had a plan to rewrite or drop the drivers near future, you could say
it.

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


Regards

Takashi Sakamoto

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

[Takashi Iwai]

On Fri, 11 Jun 2021 09:07:57 +0200,
Takashi Sakamoto wrote:
> 
> On Fri, Jun 11, 2021 at 08:47:59AM +0200, Takashi Iwai wrote:
> > 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.
> 
> I have an opinion that we should guarantee it as long as maintaining
> existent in-kernel drivers, which call it in hw/sw IRQ context. This is
> not the issue 'casually so for now'.

It *is* casually so for now, and I see no big merit for the ALSA core
about such a limitation.  The PCM core might need to introduce another
lock in future for some reason, and that'll be a mutex in nonatomic
mode.  If we guarantee the current behavior, it would become
impossible.  After all, the preempt is still allowed even if there is
no sleeper in snd_pcm_period*() itself.

For atomic mode, it's under the stream spin lock, so it's clearly no
sleep / no preempt.
For non-atomic mode, it's under the stream mutex lock, and that's
all.  There should be no other restriction there.

We don't want to choke ourselves unnecessarily.


thanks,

Takashi

> 
> If you had a plan to rewrite or drop the drivers near future, you could say
> it.
> 
> > > ======== 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
> 
> 
> Regards
> 
> Takashi Sakamoto
>