All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v4 0/7] ALSA: aloop: Support sound timer as clock source instead of jiffies
@ 2019-11-20 11:58 ` Andrew Gabbasov
  0 siblings, 0 replies; 26+ messages in thread
From: Andrew Gabbasov @ 2019-11-20 11:58 UTC (permalink / raw)
  To: alsa-devel, linux-kernel, Jaroslav Kysela, Takashi Iwai,
	Timo Wischer, Andrew Gabbasov

This patch set is an updated version of patches by Timo Wischer:
https://mailman.alsa-project.org/pipermail/alsa-devel/2019-March/146871.html

This patch set is required for forwarding audio data between a HW sound
card and an aloop device without the usage of an asynchronous sample rate
converter.

Most of sound and timers related code is kept the same as in previous set.
The code, related to snd_pcm_link() functionality and its using for
timer source setting, is removed (as rejected earlier). The changes in this
update are mainly related to the parameters handling and some cleanup.

The timer source can be initially selected by "timer_source" kernel module
parameter. It is supposed to have the following format:
    [<pref>:](<card name>|<card idx>)[{.,}<dev idx>[{.,}<subdev idx>]]
For example: "hw:I82801AAICH.1.0", or "1.1", or just "I82801AAICH".
(Prefix is ignored, just allowed here to be able to use the strings,
the user got used to).
Although the parsing function recognizes both '.' and ',' as a separator,
module parameters handling routines use ',' to separate parameters for
different module instances (array elements), so we have to use '.'
to separate device and subdevice numbers from the card name or number
in module parameters.
Empty string indicates using jiffies as a timer source.

Besides "static" selection of timer source at module load time,
it is possible to dynamically change it via sound "info" interface
(using "/proc/asound/<card>/timer_source" file in read-write mode.
The contents of this file is used as a timer source string for
a particular loopback card, e.g. "hw:0,0,0" (and here ',' can be used
as a separator).

The timer source string value can be changed at any time, but it is
latched by PCM substream open callback "loopback_open()" (the first
one for a particular cable). At this point it is actually used,
that is the string is parsed, and the timer is looked up and opened.
This seems to be a good trade-off between flexibility of updates and
synchronizations or racing complexity.

The timer source is set for a loopback card (the same as initial setting
by module parameter), but every cable uses the value, current at the moment
of opening. Theoretically, it's possible to set the timer source for each
cable independently (via separate files), but it would be inconsistent
with the initial setting via module parameters on a per-card basis.

v2:
https://mailman.alsa-project.org/pipermail/alsa-devel/2019-November/157961.html

v3:
https://mailman.alsa-project.org/pipermail/alsa-devel/2019-November/158312.html
- Change sound card lookup to use snd_card_ref() and avoid direct access
  to sound cards array
- Squash commits on returning error codes for timer start and stop
- Some locking related fixes
- Some code cleanup

v4:
- Change to use updated API for snd_timer_open() (separate timer instance)
- Change to use snd_timer_close() returning void
- Some code cleanup


Andrew Gabbasov (1):
  ALSA: aloop: Support runtime change of snd_timer via info interface

Timo Wischer (6):
  ALSA: aloop: Describe units of variables
  ALSA: aloop: Support return of error code for timer start and stop
  ALSA: aloop: Use callback functions for timer specific implementations
  ALSA: aloop: Rename all jiffies timer specific functions
  ALSA: aloop: Move CABLE_VALID_BOTH to the top of file
  ALSA: aloop: Support selection of snd_timer instead of jiffies

 sound/drivers/aloop.c | 663 +++++++++++++++++++++++++++++++++++++++---
 1 file changed, 628 insertions(+), 35 deletions(-)

-- 
2.21.0


^ permalink raw reply	[flat|nested] 26+ messages in thread

* [alsa-devel] [PATCH v4 0/7] ALSA: aloop: Support sound timer as clock source instead of jiffies
@ 2019-11-20 11:58 ` Andrew Gabbasov
  0 siblings, 0 replies; 26+ messages in thread
From: Andrew Gabbasov @ 2019-11-20 11:58 UTC (permalink / raw)
  To: alsa-devel, linux-kernel, Jaroslav Kysela, Takashi Iwai,
	Timo Wischer, Andrew Gabbasov

This patch set is an updated version of patches by Timo Wischer:
https://mailman.alsa-project.org/pipermail/alsa-devel/2019-March/146871.html

This patch set is required for forwarding audio data between a HW sound
card and an aloop device without the usage of an asynchronous sample rate
converter.

Most of sound and timers related code is kept the same as in previous set.
The code, related to snd_pcm_link() functionality and its using for
timer source setting, is removed (as rejected earlier). The changes in this
update are mainly related to the parameters handling and some cleanup.

The timer source can be initially selected by "timer_source" kernel module
parameter. It is supposed to have the following format:
    [<pref>:](<card name>|<card idx>)[{.,}<dev idx>[{.,}<subdev idx>]]
For example: "hw:I82801AAICH.1.0", or "1.1", or just "I82801AAICH".
(Prefix is ignored, just allowed here to be able to use the strings,
the user got used to).
Although the parsing function recognizes both '.' and ',' as a separator,
module parameters handling routines use ',' to separate parameters for
different module instances (array elements), so we have to use '.'
to separate device and subdevice numbers from the card name or number
in module parameters.
Empty string indicates using jiffies as a timer source.

Besides "static" selection of timer source at module load time,
it is possible to dynamically change it via sound "info" interface
(using "/proc/asound/<card>/timer_source" file in read-write mode.
The contents of this file is used as a timer source string for
a particular loopback card, e.g. "hw:0,0,0" (and here ',' can be used
as a separator).

The timer source string value can be changed at any time, but it is
latched by PCM substream open callback "loopback_open()" (the first
one for a particular cable). At this point it is actually used,
that is the string is parsed, and the timer is looked up and opened.
This seems to be a good trade-off between flexibility of updates and
synchronizations or racing complexity.

The timer source is set for a loopback card (the same as initial setting
by module parameter), but every cable uses the value, current at the moment
of opening. Theoretically, it's possible to set the timer source for each
cable independently (via separate files), but it would be inconsistent
with the initial setting via module parameters on a per-card basis.

v2:
https://mailman.alsa-project.org/pipermail/alsa-devel/2019-November/157961.html

v3:
https://mailman.alsa-project.org/pipermail/alsa-devel/2019-November/158312.html
- Change sound card lookup to use snd_card_ref() and avoid direct access
  to sound cards array
- Squash commits on returning error codes for timer start and stop
- Some locking related fixes
- Some code cleanup

v4:
- Change to use updated API for snd_timer_open() (separate timer instance)
- Change to use snd_timer_close() returning void
- Some code cleanup


Andrew Gabbasov (1):
  ALSA: aloop: Support runtime change of snd_timer via info interface

Timo Wischer (6):
  ALSA: aloop: Describe units of variables
  ALSA: aloop: Support return of error code for timer start and stop
  ALSA: aloop: Use callback functions for timer specific implementations
  ALSA: aloop: Rename all jiffies timer specific functions
  ALSA: aloop: Move CABLE_VALID_BOTH to the top of file
  ALSA: aloop: Support selection of snd_timer instead of jiffies

 sound/drivers/aloop.c | 663 +++++++++++++++++++++++++++++++++++++++---
 1 file changed, 628 insertions(+), 35 deletions(-)

-- 
2.21.0

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

^ permalink raw reply	[flat|nested] 26+ messages in thread

* [PATCH v4 1/7] ALSA: aloop: Describe units of variables
  2019-11-20 11:58 ` [alsa-devel] " Andrew Gabbasov
@ 2019-11-20 11:58   ` Andrew Gabbasov
  -1 siblings, 0 replies; 26+ messages in thread
From: Andrew Gabbasov @ 2019-11-20 11:58 UTC (permalink / raw)
  To: alsa-devel, linux-kernel, Jaroslav Kysela, Takashi Iwai,
	Timo Wischer, Andrew Gabbasov

From: Timo Wischer <twischer@de.adit-jv.com>

Describe the unit of the variables used to calculate the hw pointer
depending on jiffies ticks.

Signed-off-by: Timo Wischer <twischer@de.adit-jv.com>
Signed-off-by: Andrew Gabbasov <andrew_gabbasov@mentor.com>
---
 sound/drivers/aloop.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/sound/drivers/aloop.c b/sound/drivers/aloop.c
index 54f8b17476a1..573b06cf7cf5 100644
--- a/sound/drivers/aloop.c
+++ b/sound/drivers/aloop.c
@@ -102,8 +102,10 @@ struct loopback_pcm {
 	/* flags */
 	unsigned int period_update_pending :1;
 	/* timer stuff */
-	unsigned int irq_pos;		/* fractional IRQ position */
-	unsigned int period_size_frac;
+	unsigned int irq_pos;		/* fractional IRQ position in jiffies
+					 * ticks
+					 */
+	unsigned int period_size_frac;	/* period size in jiffies ticks */
 	unsigned int last_drift;
 	unsigned long last_jiffies;
 	struct timer_list timer;
-- 
2.21.0


^ permalink raw reply related	[flat|nested] 26+ messages in thread

* [alsa-devel] [PATCH v4 1/7] ALSA: aloop: Describe units of variables
@ 2019-11-20 11:58   ` Andrew Gabbasov
  0 siblings, 0 replies; 26+ messages in thread
From: Andrew Gabbasov @ 2019-11-20 11:58 UTC (permalink / raw)
  To: alsa-devel, linux-kernel, Jaroslav Kysela, Takashi Iwai,
	Timo Wischer, Andrew Gabbasov

From: Timo Wischer <twischer@de.adit-jv.com>

Describe the unit of the variables used to calculate the hw pointer
depending on jiffies ticks.

Signed-off-by: Timo Wischer <twischer@de.adit-jv.com>
Signed-off-by: Andrew Gabbasov <andrew_gabbasov@mentor.com>
---
 sound/drivers/aloop.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/sound/drivers/aloop.c b/sound/drivers/aloop.c
index 54f8b17476a1..573b06cf7cf5 100644
--- a/sound/drivers/aloop.c
+++ b/sound/drivers/aloop.c
@@ -102,8 +102,10 @@ struct loopback_pcm {
 	/* flags */
 	unsigned int period_update_pending :1;
 	/* timer stuff */
-	unsigned int irq_pos;		/* fractional IRQ position */
-	unsigned int period_size_frac;
+	unsigned int irq_pos;		/* fractional IRQ position in jiffies
+					 * ticks
+					 */
+	unsigned int period_size_frac;	/* period size in jiffies ticks */
 	unsigned int last_drift;
 	unsigned long last_jiffies;
 	struct timer_list timer;
-- 
2.21.0

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

^ permalink raw reply related	[flat|nested] 26+ messages in thread

* [PATCH v4 2/7] ALSA: aloop: Support return of error code for timer start and stop
  2019-11-20 11:58   ` [alsa-devel] " Andrew Gabbasov
@ 2019-11-20 11:58     ` Andrew Gabbasov
  -1 siblings, 0 replies; 26+ messages in thread
From: Andrew Gabbasov @ 2019-11-20 11:58 UTC (permalink / raw)
  To: alsa-devel, linux-kernel, Jaroslav Kysela, Takashi Iwai,
	Timo Wischer, Andrew Gabbasov

From: Timo Wischer <twischer@de.adit-jv.com>

This is required for additional timer implementations which could detect
errors and want to throw them.

Signed-off-by: Timo Wischer <twischer@de.adit-jv.com>
Signed-off-by: Andrew Gabbasov <andrew_gabbasov@mentor.com>
---
 sound/drivers/aloop.c | 30 +++++++++++++++++++-----------
 1 file changed, 19 insertions(+), 11 deletions(-)

diff --git a/sound/drivers/aloop.c b/sound/drivers/aloop.c
index 573b06cf7cf5..7919006f70a5 100644
--- a/sound/drivers/aloop.c
+++ b/sound/drivers/aloop.c
@@ -155,7 +155,7 @@ static inline unsigned int get_rate_shift(struct loopback_pcm *dpcm)
 }
 
 /* call in cable->lock */
-static void loopback_timer_start(struct loopback_pcm *dpcm)
+static int loopback_timer_start(struct loopback_pcm *dpcm)
 {
 	unsigned long tick;
 	unsigned int rate_shift = get_rate_shift(dpcm);
@@ -171,18 +171,24 @@ static void loopback_timer_start(struct loopback_pcm *dpcm)
 	tick = dpcm->period_size_frac - dpcm->irq_pos;
 	tick = (tick + dpcm->pcm_bps - 1) / dpcm->pcm_bps;
 	mod_timer(&dpcm->timer, jiffies + tick);
+
+	return 0;
 }
 
 /* call in cable->lock */
-static inline void loopback_timer_stop(struct loopback_pcm *dpcm)
+static inline int loopback_timer_stop(struct loopback_pcm *dpcm)
 {
 	del_timer(&dpcm->timer);
 	dpcm->timer.expires = 0;
+
+	return 0;
 }
 
-static inline void loopback_timer_stop_sync(struct loopback_pcm *dpcm)
+static inline int loopback_timer_stop_sync(struct loopback_pcm *dpcm)
 {
 	del_timer_sync(&dpcm->timer);
+
+	return 0;
 }
 
 #define CABLE_VALID_PLAYBACK	(1 << SNDRV_PCM_STREAM_PLAYBACK)
@@ -251,7 +257,7 @@ static int loopback_trigger(struct snd_pcm_substream *substream, int cmd)
 	struct snd_pcm_runtime *runtime = substream->runtime;
 	struct loopback_pcm *dpcm = runtime->private_data;
 	struct loopback_cable *cable = dpcm->cable;
-	int err, stream = 1 << substream->stream;
+	int err = 0, stream = 1 << substream->stream;
 
 	switch (cmd) {
 	case SNDRV_PCM_TRIGGER_START:
@@ -264,7 +270,7 @@ static int loopback_trigger(struct snd_pcm_substream *substream, int cmd)
 		spin_lock(&cable->lock);	
 		cable->running |= stream;
 		cable->pause &= ~stream;
-		loopback_timer_start(dpcm);
+		err = loopback_timer_start(dpcm);
 		spin_unlock(&cable->lock);
 		if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
 			loopback_active_notify(dpcm);
@@ -273,7 +279,7 @@ static int loopback_trigger(struct snd_pcm_substream *substream, int cmd)
 		spin_lock(&cable->lock);	
 		cable->running &= ~stream;
 		cable->pause &= ~stream;
-		loopback_timer_stop(dpcm);
+		err = loopback_timer_stop(dpcm);
 		spin_unlock(&cable->lock);
 		if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
 			loopback_active_notify(dpcm);
@@ -282,7 +288,7 @@ static int loopback_trigger(struct snd_pcm_substream *substream, int cmd)
 	case SNDRV_PCM_TRIGGER_SUSPEND:
 		spin_lock(&cable->lock);	
 		cable->pause |= stream;
-		loopback_timer_stop(dpcm);
+		err = loopback_timer_stop(dpcm);
 		spin_unlock(&cable->lock);
 		if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
 			loopback_active_notify(dpcm);
@@ -292,7 +298,7 @@ static int loopback_trigger(struct snd_pcm_substream *substream, int cmd)
 		spin_lock(&cable->lock);
 		dpcm->last_jiffies = jiffies;
 		cable->pause &= ~stream;
-		loopback_timer_start(dpcm);
+		err = loopback_timer_start(dpcm);
 		spin_unlock(&cable->lock);
 		if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
 			loopback_active_notify(dpcm);
@@ -300,7 +306,7 @@ static int loopback_trigger(struct snd_pcm_substream *substream, int cmd)
 	default:
 		return -EINVAL;
 	}
-	return 0;
+	return err;
 }
 
 static void params_change(struct snd_pcm_substream *substream)
@@ -321,9 +327,11 @@ static int loopback_prepare(struct snd_pcm_substream *substream)
 	struct snd_pcm_runtime *runtime = substream->runtime;
 	struct loopback_pcm *dpcm = runtime->private_data;
 	struct loopback_cable *cable = dpcm->cable;
-	int bps, salign;
+	int err, bps, salign;
 
-	loopback_timer_stop_sync(dpcm);
+	err = loopback_timer_stop_sync(dpcm);
+	if (err < 0)
+		return err;
 
 	salign = (snd_pcm_format_physical_width(runtime->format) *
 						runtime->channels) / 8;
-- 
2.21.0


^ permalink raw reply related	[flat|nested] 26+ messages in thread

* [alsa-devel] [PATCH v4 2/7] ALSA: aloop: Support return of error code for timer start and stop
@ 2019-11-20 11:58     ` Andrew Gabbasov
  0 siblings, 0 replies; 26+ messages in thread
From: Andrew Gabbasov @ 2019-11-20 11:58 UTC (permalink / raw)
  To: alsa-devel, linux-kernel, Jaroslav Kysela, Takashi Iwai,
	Timo Wischer, Andrew Gabbasov

From: Timo Wischer <twischer@de.adit-jv.com>

This is required for additional timer implementations which could detect
errors and want to throw them.

Signed-off-by: Timo Wischer <twischer@de.adit-jv.com>
Signed-off-by: Andrew Gabbasov <andrew_gabbasov@mentor.com>
---
 sound/drivers/aloop.c | 30 +++++++++++++++++++-----------
 1 file changed, 19 insertions(+), 11 deletions(-)

diff --git a/sound/drivers/aloop.c b/sound/drivers/aloop.c
index 573b06cf7cf5..7919006f70a5 100644
--- a/sound/drivers/aloop.c
+++ b/sound/drivers/aloop.c
@@ -155,7 +155,7 @@ static inline unsigned int get_rate_shift(struct loopback_pcm *dpcm)
 }
 
 /* call in cable->lock */
-static void loopback_timer_start(struct loopback_pcm *dpcm)
+static int loopback_timer_start(struct loopback_pcm *dpcm)
 {
 	unsigned long tick;
 	unsigned int rate_shift = get_rate_shift(dpcm);
@@ -171,18 +171,24 @@ static void loopback_timer_start(struct loopback_pcm *dpcm)
 	tick = dpcm->period_size_frac - dpcm->irq_pos;
 	tick = (tick + dpcm->pcm_bps - 1) / dpcm->pcm_bps;
 	mod_timer(&dpcm->timer, jiffies + tick);
+
+	return 0;
 }
 
 /* call in cable->lock */
-static inline void loopback_timer_stop(struct loopback_pcm *dpcm)
+static inline int loopback_timer_stop(struct loopback_pcm *dpcm)
 {
 	del_timer(&dpcm->timer);
 	dpcm->timer.expires = 0;
+
+	return 0;
 }
 
-static inline void loopback_timer_stop_sync(struct loopback_pcm *dpcm)
+static inline int loopback_timer_stop_sync(struct loopback_pcm *dpcm)
 {
 	del_timer_sync(&dpcm->timer);
+
+	return 0;
 }
 
 #define CABLE_VALID_PLAYBACK	(1 << SNDRV_PCM_STREAM_PLAYBACK)
@@ -251,7 +257,7 @@ static int loopback_trigger(struct snd_pcm_substream *substream, int cmd)
 	struct snd_pcm_runtime *runtime = substream->runtime;
 	struct loopback_pcm *dpcm = runtime->private_data;
 	struct loopback_cable *cable = dpcm->cable;
-	int err, stream = 1 << substream->stream;
+	int err = 0, stream = 1 << substream->stream;
 
 	switch (cmd) {
 	case SNDRV_PCM_TRIGGER_START:
@@ -264,7 +270,7 @@ static int loopback_trigger(struct snd_pcm_substream *substream, int cmd)
 		spin_lock(&cable->lock);	
 		cable->running |= stream;
 		cable->pause &= ~stream;
-		loopback_timer_start(dpcm);
+		err = loopback_timer_start(dpcm);
 		spin_unlock(&cable->lock);
 		if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
 			loopback_active_notify(dpcm);
@@ -273,7 +279,7 @@ static int loopback_trigger(struct snd_pcm_substream *substream, int cmd)
 		spin_lock(&cable->lock);	
 		cable->running &= ~stream;
 		cable->pause &= ~stream;
-		loopback_timer_stop(dpcm);
+		err = loopback_timer_stop(dpcm);
 		spin_unlock(&cable->lock);
 		if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
 			loopback_active_notify(dpcm);
@@ -282,7 +288,7 @@ static int loopback_trigger(struct snd_pcm_substream *substream, int cmd)
 	case SNDRV_PCM_TRIGGER_SUSPEND:
 		spin_lock(&cable->lock);	
 		cable->pause |= stream;
-		loopback_timer_stop(dpcm);
+		err = loopback_timer_stop(dpcm);
 		spin_unlock(&cable->lock);
 		if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
 			loopback_active_notify(dpcm);
@@ -292,7 +298,7 @@ static int loopback_trigger(struct snd_pcm_substream *substream, int cmd)
 		spin_lock(&cable->lock);
 		dpcm->last_jiffies = jiffies;
 		cable->pause &= ~stream;
-		loopback_timer_start(dpcm);
+		err = loopback_timer_start(dpcm);
 		spin_unlock(&cable->lock);
 		if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
 			loopback_active_notify(dpcm);
@@ -300,7 +306,7 @@ static int loopback_trigger(struct snd_pcm_substream *substream, int cmd)
 	default:
 		return -EINVAL;
 	}
-	return 0;
+	return err;
 }
 
 static void params_change(struct snd_pcm_substream *substream)
@@ -321,9 +327,11 @@ static int loopback_prepare(struct snd_pcm_substream *substream)
 	struct snd_pcm_runtime *runtime = substream->runtime;
 	struct loopback_pcm *dpcm = runtime->private_data;
 	struct loopback_cable *cable = dpcm->cable;
-	int bps, salign;
+	int err, bps, salign;
 
-	loopback_timer_stop_sync(dpcm);
+	err = loopback_timer_stop_sync(dpcm);
+	if (err < 0)
+		return err;
 
 	salign = (snd_pcm_format_physical_width(runtime->format) *
 						runtime->channels) / 8;
-- 
2.21.0

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

^ permalink raw reply related	[flat|nested] 26+ messages in thread

* [PATCH v4 3/7] ALSA: aloop: Use callback functions for timer specific implementations
  2019-11-20 11:58     ` [alsa-devel] " Andrew Gabbasov
@ 2019-11-20 11:58       ` Andrew Gabbasov
  -1 siblings, 0 replies; 26+ messages in thread
From: Andrew Gabbasov @ 2019-11-20 11:58 UTC (permalink / raw)
  To: alsa-devel, linux-kernel, Jaroslav Kysela, Takashi Iwai,
	Timo Wischer, Andrew Gabbasov

From: Timo Wischer <twischer@de.adit-jv.com>

This commit only refactors the implementation. It does not change the
behaviour.
It is required to support other timers (e.g sound timer).

Signed-off-by: Timo Wischer <twischer@de.adit-jv.com>
Signed-off-by: Andrew Gabbasov <andrew_gabbasov@mentor.com>
---
 sound/drivers/aloop.c | 113 +++++++++++++++++++++++++++++++++++-------
 1 file changed, 94 insertions(+), 19 deletions(-)

diff --git a/sound/drivers/aloop.c b/sound/drivers/aloop.c
index 7919006f70a5..3bfd7c32803c 100644
--- a/sound/drivers/aloop.c
+++ b/sound/drivers/aloop.c
@@ -55,8 +55,39 @@ MODULE_PARM_DESC(pcm_notify, "Break capture when PCM format/rate/channels change
 
 #define NO_PITCH 100000
 
+struct loopback_cable;
 struct loopback_pcm;
 
+struct loopback_ops {
+	/* optional
+	 * call in loopback->cable_lock
+	 */
+	int (*open)(struct loopback_pcm *dpcm);
+	/* required
+	 * call in cable->lock
+	 */
+	int (*start)(struct loopback_pcm *dpcm);
+	/* required
+	 * call in cable->lock
+	 */
+	int (*stop)(struct loopback_pcm *dpcm);
+	/* optional */
+	int (*stop_sync)(struct loopback_pcm *dpcm);
+	/* optional */
+	int (*close_substream)(struct loopback_pcm *dpcm);
+	/* optional
+	 * call in loopback->cable_lock
+	 */
+	int (*close_cable)(struct loopback_pcm *dpcm);
+	/* optional
+	 * call in cable->lock
+	 */
+	unsigned int (*pos_update)(struct loopback_cable *cable);
+	/* optional */
+	void (*dpcm_info)(struct loopback_pcm *dpcm,
+			  struct snd_info_buffer *buffer);
+};
+
 struct loopback_cable {
 	spinlock_t lock;
 	struct loopback_pcm *streams[2];
@@ -65,6 +96,8 @@ struct loopback_cable {
 	unsigned int valid;
 	unsigned int running;
 	unsigned int pause;
+	/* timer specific */
+	struct loopback_ops *ops;
 };
 
 struct loopback_setup {
@@ -270,7 +303,7 @@ static int loopback_trigger(struct snd_pcm_substream *substream, int cmd)
 		spin_lock(&cable->lock);	
 		cable->running |= stream;
 		cable->pause &= ~stream;
-		err = loopback_timer_start(dpcm);
+		err = cable->ops->start(dpcm);
 		spin_unlock(&cable->lock);
 		if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
 			loopback_active_notify(dpcm);
@@ -279,7 +312,7 @@ static int loopback_trigger(struct snd_pcm_substream *substream, int cmd)
 		spin_lock(&cable->lock);	
 		cable->running &= ~stream;
 		cable->pause &= ~stream;
-		err = loopback_timer_stop(dpcm);
+		err = cable->ops->stop(dpcm);
 		spin_unlock(&cable->lock);
 		if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
 			loopback_active_notify(dpcm);
@@ -288,7 +321,7 @@ static int loopback_trigger(struct snd_pcm_substream *substream, int cmd)
 	case SNDRV_PCM_TRIGGER_SUSPEND:
 		spin_lock(&cable->lock);	
 		cable->pause |= stream;
-		err = loopback_timer_stop(dpcm);
+		err = cable->ops->stop(dpcm);
 		spin_unlock(&cable->lock);
 		if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
 			loopback_active_notify(dpcm);
@@ -298,7 +331,7 @@ static int loopback_trigger(struct snd_pcm_substream *substream, int cmd)
 		spin_lock(&cable->lock);
 		dpcm->last_jiffies = jiffies;
 		cable->pause &= ~stream;
-		err = loopback_timer_start(dpcm);
+		err = cable->ops->start(dpcm);
 		spin_unlock(&cable->lock);
 		if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
 			loopback_active_notify(dpcm);
@@ -329,9 +362,11 @@ static int loopback_prepare(struct snd_pcm_substream *substream)
 	struct loopback_cable *cable = dpcm->cable;
 	int err, bps, salign;
 
-	err = loopback_timer_stop_sync(dpcm);
-	if (err < 0)
-		return err;
+	if (cable->ops->stop_sync) {
+		err = cable->ops->stop_sync(dpcm);
+		if (err < 0)
+			return err;
+	}
 
 	salign = (snd_pcm_format_physical_width(runtime->format) *
 						runtime->channels) / 8;
@@ -539,6 +574,18 @@ static void loopback_timer_function(struct timer_list *t)
 	spin_unlock_irqrestore(&dpcm->cable->lock, flags);
 }
 
+static void loopback_jiffies_timer_dpcm_info(struct loopback_pcm *dpcm,
+					     struct snd_info_buffer *buffer)
+{
+	snd_iprintf(buffer, "    update_pending:\t%u\n",
+		    dpcm->period_update_pending);
+	snd_iprintf(buffer, "    irq_pos:\t\t%u\n", dpcm->irq_pos);
+	snd_iprintf(buffer, "    period_frac:\t%u\n", dpcm->period_size_frac);
+	snd_iprintf(buffer, "    last_jiffies:\t%lu (%lu)\n",
+		    dpcm->last_jiffies, jiffies);
+	snd_iprintf(buffer, "    timer_expires:\t%lu\n", dpcm->timer.expires);
+}
+
 static snd_pcm_uframes_t loopback_pointer(struct snd_pcm_substream *substream)
 {
 	struct snd_pcm_runtime *runtime = substream->runtime;
@@ -546,7 +593,8 @@ static snd_pcm_uframes_t loopback_pointer(struct snd_pcm_substream *substream)
 	snd_pcm_uframes_t pos;
 
 	spin_lock(&dpcm->cable->lock);
-	loopback_pos_update(dpcm->cable);
+	if (dpcm->cable->ops->pos_update)
+		dpcm->cable->ops->pos_update(dpcm->cable);
 	pos = dpcm->buf_pos;
 	spin_unlock(&dpcm->cable->lock);
 	return bytes_to_frames(runtime, pos);
@@ -671,12 +719,33 @@ static void free_cable(struct snd_pcm_substream *substream)
 		cable->streams[substream->stream] = NULL;
 		spin_unlock_irq(&cable->lock);
 	} else {
+		struct loopback_pcm *dpcm = substream->runtime->private_data;
+
+		if (cable->ops && cable->ops->close_cable && dpcm)
+			cable->ops->close_cable(dpcm);
 		/* free the cable */
 		loopback->cables[substream->number][dev] = NULL;
 		kfree(cable);
 	}
 }
 
+static int loopback_jiffies_timer_open(struct loopback_pcm *dpcm)
+{
+	timer_setup(&dpcm->timer, loopback_timer_function, 0);
+
+	return 0;
+}
+
+static struct loopback_ops loopback_jiffies_timer_ops = {
+	.open = loopback_jiffies_timer_open,
+	.start = loopback_timer_start,
+	.stop = loopback_timer_stop,
+	.stop_sync = loopback_timer_stop_sync,
+	.close_substream = loopback_timer_stop_sync,
+	.pos_update = loopback_pos_update,
+	.dpcm_info = loopback_jiffies_timer_dpcm_info,
+};
+
 static int loopback_open(struct snd_pcm_substream *substream)
 {
 	struct snd_pcm_runtime *runtime = substream->runtime;
@@ -694,7 +763,6 @@ static int loopback_open(struct snd_pcm_substream *substream)
 	}
 	dpcm->loopback = loopback;
 	dpcm->substream = substream;
-	timer_setup(&dpcm->timer, loopback_timer_function, 0);
 
 	cable = loopback->cables[substream->number][dev];
 	if (!cable) {
@@ -705,9 +773,17 @@ static int loopback_open(struct snd_pcm_substream *substream)
 		}
 		spin_lock_init(&cable->lock);
 		cable->hw = loopback_pcm_hardware;
+		cable->ops = &loopback_jiffies_timer_ops;
 		loopback->cables[substream->number][dev] = cable;
 	}
 	dpcm->cable = cable;
+	runtime->private_data = dpcm;
+
+	if (cable->ops->open) {
+		err = cable->ops->open(dpcm);
+		if (err < 0)
+			goto unlock;
+	}
 
 	snd_pcm_hw_constraint_integer(runtime, SNDRV_PCM_HW_PARAM_PERIODS);
 
@@ -733,7 +809,9 @@ static int loopback_open(struct snd_pcm_substream *substream)
 	if (err < 0)
 		goto unlock;
 
-	runtime->private_data = dpcm;
+	/* loopback_runtime_free() has not to be called if kfree(dpcm) was
+	 * already called here. Otherwise it will end up with a double free.
+	 */
 	runtime->private_free = loopback_runtime_free;
 	if (get_notify(dpcm))
 		runtime->hw = loopback_pcm_hardware;
@@ -757,12 +835,14 @@ static int loopback_close(struct snd_pcm_substream *substream)
 {
 	struct loopback *loopback = substream->private_data;
 	struct loopback_pcm *dpcm = substream->runtime->private_data;
+	int err = 0;
 
-	loopback_timer_stop_sync(dpcm);
+	if (dpcm->cable->ops->close_substream)
+		err = dpcm->cable->ops->close_substream(dpcm);
 	mutex_lock(&loopback->cable_lock);
 	free_cable(substream);
 	mutex_unlock(&loopback->cable_lock);
-	return 0;
+	return err;
 }
 
 static const struct snd_pcm_ops loopback_pcm_ops = {
@@ -1086,13 +1166,8 @@ static void print_dpcm_info(struct snd_info_buffer *buffer,
 	snd_iprintf(buffer, "    bytes_per_sec:\t%u\n", dpcm->pcm_bps);
 	snd_iprintf(buffer, "    sample_align:\t%u\n", dpcm->pcm_salign);
 	snd_iprintf(buffer, "    rate_shift:\t\t%u\n", dpcm->pcm_rate_shift);
-	snd_iprintf(buffer, "    update_pending:\t%u\n",
-						dpcm->period_update_pending);
-	snd_iprintf(buffer, "    irq_pos:\t\t%u\n", dpcm->irq_pos);
-	snd_iprintf(buffer, "    period_frac:\t%u\n", dpcm->period_size_frac);
-	snd_iprintf(buffer, "    last_jiffies:\t%lu (%lu)\n",
-					dpcm->last_jiffies, jiffies);
-	snd_iprintf(buffer, "    timer_expires:\t%lu\n", dpcm->timer.expires);
+	if (dpcm->cable->ops->dpcm_info)
+		dpcm->cable->ops->dpcm_info(dpcm, buffer);
 }
 
 static void print_substream_info(struct snd_info_buffer *buffer,
-- 
2.21.0


^ permalink raw reply related	[flat|nested] 26+ messages in thread

* [alsa-devel] [PATCH v4 3/7] ALSA: aloop: Use callback functions for timer specific implementations
@ 2019-11-20 11:58       ` Andrew Gabbasov
  0 siblings, 0 replies; 26+ messages in thread
From: Andrew Gabbasov @ 2019-11-20 11:58 UTC (permalink / raw)
  To: alsa-devel, linux-kernel, Jaroslav Kysela, Takashi Iwai,
	Timo Wischer, Andrew Gabbasov

From: Timo Wischer <twischer@de.adit-jv.com>

This commit only refactors the implementation. It does not change the
behaviour.
It is required to support other timers (e.g sound timer).

Signed-off-by: Timo Wischer <twischer@de.adit-jv.com>
Signed-off-by: Andrew Gabbasov <andrew_gabbasov@mentor.com>
---
 sound/drivers/aloop.c | 113 +++++++++++++++++++++++++++++++++++-------
 1 file changed, 94 insertions(+), 19 deletions(-)

diff --git a/sound/drivers/aloop.c b/sound/drivers/aloop.c
index 7919006f70a5..3bfd7c32803c 100644
--- a/sound/drivers/aloop.c
+++ b/sound/drivers/aloop.c
@@ -55,8 +55,39 @@ MODULE_PARM_DESC(pcm_notify, "Break capture when PCM format/rate/channels change
 
 #define NO_PITCH 100000
 
+struct loopback_cable;
 struct loopback_pcm;
 
+struct loopback_ops {
+	/* optional
+	 * call in loopback->cable_lock
+	 */
+	int (*open)(struct loopback_pcm *dpcm);
+	/* required
+	 * call in cable->lock
+	 */
+	int (*start)(struct loopback_pcm *dpcm);
+	/* required
+	 * call in cable->lock
+	 */
+	int (*stop)(struct loopback_pcm *dpcm);
+	/* optional */
+	int (*stop_sync)(struct loopback_pcm *dpcm);
+	/* optional */
+	int (*close_substream)(struct loopback_pcm *dpcm);
+	/* optional
+	 * call in loopback->cable_lock
+	 */
+	int (*close_cable)(struct loopback_pcm *dpcm);
+	/* optional
+	 * call in cable->lock
+	 */
+	unsigned int (*pos_update)(struct loopback_cable *cable);
+	/* optional */
+	void (*dpcm_info)(struct loopback_pcm *dpcm,
+			  struct snd_info_buffer *buffer);
+};
+
 struct loopback_cable {
 	spinlock_t lock;
 	struct loopback_pcm *streams[2];
@@ -65,6 +96,8 @@ struct loopback_cable {
 	unsigned int valid;
 	unsigned int running;
 	unsigned int pause;
+	/* timer specific */
+	struct loopback_ops *ops;
 };
 
 struct loopback_setup {
@@ -270,7 +303,7 @@ static int loopback_trigger(struct snd_pcm_substream *substream, int cmd)
 		spin_lock(&cable->lock);	
 		cable->running |= stream;
 		cable->pause &= ~stream;
-		err = loopback_timer_start(dpcm);
+		err = cable->ops->start(dpcm);
 		spin_unlock(&cable->lock);
 		if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
 			loopback_active_notify(dpcm);
@@ -279,7 +312,7 @@ static int loopback_trigger(struct snd_pcm_substream *substream, int cmd)
 		spin_lock(&cable->lock);	
 		cable->running &= ~stream;
 		cable->pause &= ~stream;
-		err = loopback_timer_stop(dpcm);
+		err = cable->ops->stop(dpcm);
 		spin_unlock(&cable->lock);
 		if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
 			loopback_active_notify(dpcm);
@@ -288,7 +321,7 @@ static int loopback_trigger(struct snd_pcm_substream *substream, int cmd)
 	case SNDRV_PCM_TRIGGER_SUSPEND:
 		spin_lock(&cable->lock);	
 		cable->pause |= stream;
-		err = loopback_timer_stop(dpcm);
+		err = cable->ops->stop(dpcm);
 		spin_unlock(&cable->lock);
 		if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
 			loopback_active_notify(dpcm);
@@ -298,7 +331,7 @@ static int loopback_trigger(struct snd_pcm_substream *substream, int cmd)
 		spin_lock(&cable->lock);
 		dpcm->last_jiffies = jiffies;
 		cable->pause &= ~stream;
-		err = loopback_timer_start(dpcm);
+		err = cable->ops->start(dpcm);
 		spin_unlock(&cable->lock);
 		if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
 			loopback_active_notify(dpcm);
@@ -329,9 +362,11 @@ static int loopback_prepare(struct snd_pcm_substream *substream)
 	struct loopback_cable *cable = dpcm->cable;
 	int err, bps, salign;
 
-	err = loopback_timer_stop_sync(dpcm);
-	if (err < 0)
-		return err;
+	if (cable->ops->stop_sync) {
+		err = cable->ops->stop_sync(dpcm);
+		if (err < 0)
+			return err;
+	}
 
 	salign = (snd_pcm_format_physical_width(runtime->format) *
 						runtime->channels) / 8;
@@ -539,6 +574,18 @@ static void loopback_timer_function(struct timer_list *t)
 	spin_unlock_irqrestore(&dpcm->cable->lock, flags);
 }
 
+static void loopback_jiffies_timer_dpcm_info(struct loopback_pcm *dpcm,
+					     struct snd_info_buffer *buffer)
+{
+	snd_iprintf(buffer, "    update_pending:\t%u\n",
+		    dpcm->period_update_pending);
+	snd_iprintf(buffer, "    irq_pos:\t\t%u\n", dpcm->irq_pos);
+	snd_iprintf(buffer, "    period_frac:\t%u\n", dpcm->period_size_frac);
+	snd_iprintf(buffer, "    last_jiffies:\t%lu (%lu)\n",
+		    dpcm->last_jiffies, jiffies);
+	snd_iprintf(buffer, "    timer_expires:\t%lu\n", dpcm->timer.expires);
+}
+
 static snd_pcm_uframes_t loopback_pointer(struct snd_pcm_substream *substream)
 {
 	struct snd_pcm_runtime *runtime = substream->runtime;
@@ -546,7 +593,8 @@ static snd_pcm_uframes_t loopback_pointer(struct snd_pcm_substream *substream)
 	snd_pcm_uframes_t pos;
 
 	spin_lock(&dpcm->cable->lock);
-	loopback_pos_update(dpcm->cable);
+	if (dpcm->cable->ops->pos_update)
+		dpcm->cable->ops->pos_update(dpcm->cable);
 	pos = dpcm->buf_pos;
 	spin_unlock(&dpcm->cable->lock);
 	return bytes_to_frames(runtime, pos);
@@ -671,12 +719,33 @@ static void free_cable(struct snd_pcm_substream *substream)
 		cable->streams[substream->stream] = NULL;
 		spin_unlock_irq(&cable->lock);
 	} else {
+		struct loopback_pcm *dpcm = substream->runtime->private_data;
+
+		if (cable->ops && cable->ops->close_cable && dpcm)
+			cable->ops->close_cable(dpcm);
 		/* free the cable */
 		loopback->cables[substream->number][dev] = NULL;
 		kfree(cable);
 	}
 }
 
+static int loopback_jiffies_timer_open(struct loopback_pcm *dpcm)
+{
+	timer_setup(&dpcm->timer, loopback_timer_function, 0);
+
+	return 0;
+}
+
+static struct loopback_ops loopback_jiffies_timer_ops = {
+	.open = loopback_jiffies_timer_open,
+	.start = loopback_timer_start,
+	.stop = loopback_timer_stop,
+	.stop_sync = loopback_timer_stop_sync,
+	.close_substream = loopback_timer_stop_sync,
+	.pos_update = loopback_pos_update,
+	.dpcm_info = loopback_jiffies_timer_dpcm_info,
+};
+
 static int loopback_open(struct snd_pcm_substream *substream)
 {
 	struct snd_pcm_runtime *runtime = substream->runtime;
@@ -694,7 +763,6 @@ static int loopback_open(struct snd_pcm_substream *substream)
 	}
 	dpcm->loopback = loopback;
 	dpcm->substream = substream;
-	timer_setup(&dpcm->timer, loopback_timer_function, 0);
 
 	cable = loopback->cables[substream->number][dev];
 	if (!cable) {
@@ -705,9 +773,17 @@ static int loopback_open(struct snd_pcm_substream *substream)
 		}
 		spin_lock_init(&cable->lock);
 		cable->hw = loopback_pcm_hardware;
+		cable->ops = &loopback_jiffies_timer_ops;
 		loopback->cables[substream->number][dev] = cable;
 	}
 	dpcm->cable = cable;
+	runtime->private_data = dpcm;
+
+	if (cable->ops->open) {
+		err = cable->ops->open(dpcm);
+		if (err < 0)
+			goto unlock;
+	}
 
 	snd_pcm_hw_constraint_integer(runtime, SNDRV_PCM_HW_PARAM_PERIODS);
 
@@ -733,7 +809,9 @@ static int loopback_open(struct snd_pcm_substream *substream)
 	if (err < 0)
 		goto unlock;
 
-	runtime->private_data = dpcm;
+	/* loopback_runtime_free() has not to be called if kfree(dpcm) was
+	 * already called here. Otherwise it will end up with a double free.
+	 */
 	runtime->private_free = loopback_runtime_free;
 	if (get_notify(dpcm))
 		runtime->hw = loopback_pcm_hardware;
@@ -757,12 +835,14 @@ static int loopback_close(struct snd_pcm_substream *substream)
 {
 	struct loopback *loopback = substream->private_data;
 	struct loopback_pcm *dpcm = substream->runtime->private_data;
+	int err = 0;
 
-	loopback_timer_stop_sync(dpcm);
+	if (dpcm->cable->ops->close_substream)
+		err = dpcm->cable->ops->close_substream(dpcm);
 	mutex_lock(&loopback->cable_lock);
 	free_cable(substream);
 	mutex_unlock(&loopback->cable_lock);
-	return 0;
+	return err;
 }
 
 static const struct snd_pcm_ops loopback_pcm_ops = {
@@ -1086,13 +1166,8 @@ static void print_dpcm_info(struct snd_info_buffer *buffer,
 	snd_iprintf(buffer, "    bytes_per_sec:\t%u\n", dpcm->pcm_bps);
 	snd_iprintf(buffer, "    sample_align:\t%u\n", dpcm->pcm_salign);
 	snd_iprintf(buffer, "    rate_shift:\t\t%u\n", dpcm->pcm_rate_shift);
-	snd_iprintf(buffer, "    update_pending:\t%u\n",
-						dpcm->period_update_pending);
-	snd_iprintf(buffer, "    irq_pos:\t\t%u\n", dpcm->irq_pos);
-	snd_iprintf(buffer, "    period_frac:\t%u\n", dpcm->period_size_frac);
-	snd_iprintf(buffer, "    last_jiffies:\t%lu (%lu)\n",
-					dpcm->last_jiffies, jiffies);
-	snd_iprintf(buffer, "    timer_expires:\t%lu\n", dpcm->timer.expires);
+	if (dpcm->cable->ops->dpcm_info)
+		dpcm->cable->ops->dpcm_info(dpcm, buffer);
 }
 
 static void print_substream_info(struct snd_info_buffer *buffer,
-- 
2.21.0

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

^ permalink raw reply related	[flat|nested] 26+ messages in thread

* [PATCH v4 4/7] ALSA: aloop: Rename all jiffies timer specific functions
  2019-11-20 11:58       ` [alsa-devel] " Andrew Gabbasov
@ 2019-11-20 11:58         ` Andrew Gabbasov
  -1 siblings, 0 replies; 26+ messages in thread
From: Andrew Gabbasov @ 2019-11-20 11:58 UTC (permalink / raw)
  To: alsa-devel, linux-kernel, Jaroslav Kysela, Takashi Iwai,
	Timo Wischer, Andrew Gabbasov

From: Timo Wischer <twischer@de.adit-jv.com>

This commit does not change the behaviour. It only separates the jiffies
timer specific implementation from the generic part.

Signed-off-by: Timo Wischer <twischer@de.adit-jv.com>
Signed-off-by: Andrew Gabbasov <andrew_gabbasov@mentor.com>
---
 sound/drivers/aloop.c | 28 +++++++++++++++-------------
 1 file changed, 15 insertions(+), 13 deletions(-)

diff --git a/sound/drivers/aloop.c b/sound/drivers/aloop.c
index 3bfd7c32803c..2f208aaa54cf 100644
--- a/sound/drivers/aloop.c
+++ b/sound/drivers/aloop.c
@@ -188,7 +188,7 @@ static inline unsigned int get_rate_shift(struct loopback_pcm *dpcm)
 }
 
 /* call in cable->lock */
-static int loopback_timer_start(struct loopback_pcm *dpcm)
+static int loopback_jiffies_timer_start(struct loopback_pcm *dpcm)
 {
 	unsigned long tick;
 	unsigned int rate_shift = get_rate_shift(dpcm);
@@ -209,7 +209,7 @@ static int loopback_timer_start(struct loopback_pcm *dpcm)
 }
 
 /* call in cable->lock */
-static inline int loopback_timer_stop(struct loopback_pcm *dpcm)
+static inline int loopback_jiffies_timer_stop(struct loopback_pcm *dpcm)
 {
 	del_timer(&dpcm->timer);
 	dpcm->timer.expires = 0;
@@ -217,7 +217,7 @@ static inline int loopback_timer_stop(struct loopback_pcm *dpcm)
 	return 0;
 }
 
-static inline int loopback_timer_stop_sync(struct loopback_pcm *dpcm)
+static inline int loopback_jiffies_timer_stop_sync(struct loopback_pcm *dpcm)
 {
 	del_timer_sync(&dpcm->timer);
 
@@ -502,7 +502,8 @@ static inline void bytepos_finish(struct loopback_pcm *dpcm,
 }
 
 /* call in cable->lock */
-static unsigned int loopback_pos_update(struct loopback_cable *cable)
+static unsigned int loopback_jiffies_timer_pos_update
+		(struct loopback_cable *cable)
 {
 	struct loopback_pcm *dpcm_play =
 			cable->streams[SNDRV_PCM_STREAM_PLAYBACK];
@@ -555,14 +556,15 @@ static unsigned int loopback_pos_update(struct loopback_cable *cable)
 	return running;
 }
 
-static void loopback_timer_function(struct timer_list *t)
+static void loopback_jiffies_timer_function(struct timer_list *t)
 {
 	struct loopback_pcm *dpcm = from_timer(dpcm, t, timer);
 	unsigned long flags;
 
 	spin_lock_irqsave(&dpcm->cable->lock, flags);
-	if (loopback_pos_update(dpcm->cable) & (1 << dpcm->substream->stream)) {
-		loopback_timer_start(dpcm);
+	if (loopback_jiffies_timer_pos_update(dpcm->cable) &
+			(1 << dpcm->substream->stream)) {
+		loopback_jiffies_timer_start(dpcm);
 		if (dpcm->period_update_pending) {
 			dpcm->period_update_pending = 0;
 			spin_unlock_irqrestore(&dpcm->cable->lock, flags);
@@ -731,18 +733,18 @@ static void free_cable(struct snd_pcm_substream *substream)
 
 static int loopback_jiffies_timer_open(struct loopback_pcm *dpcm)
 {
-	timer_setup(&dpcm->timer, loopback_timer_function, 0);
+	timer_setup(&dpcm->timer, loopback_jiffies_timer_function, 0);
 
 	return 0;
 }
 
 static struct loopback_ops loopback_jiffies_timer_ops = {
 	.open = loopback_jiffies_timer_open,
-	.start = loopback_timer_start,
-	.stop = loopback_timer_stop,
-	.stop_sync = loopback_timer_stop_sync,
-	.close_substream = loopback_timer_stop_sync,
-	.pos_update = loopback_pos_update,
+	.start = loopback_jiffies_timer_start,
+	.stop = loopback_jiffies_timer_stop,
+	.stop_sync = loopback_jiffies_timer_stop_sync,
+	.close_substream = loopback_jiffies_timer_stop_sync,
+	.pos_update = loopback_jiffies_timer_pos_update,
 	.dpcm_info = loopback_jiffies_timer_dpcm_info,
 };
 
-- 
2.21.0


^ permalink raw reply related	[flat|nested] 26+ messages in thread

* [alsa-devel] [PATCH v4 4/7] ALSA: aloop: Rename all jiffies timer specific functions
@ 2019-11-20 11:58         ` Andrew Gabbasov
  0 siblings, 0 replies; 26+ messages in thread
From: Andrew Gabbasov @ 2019-11-20 11:58 UTC (permalink / raw)
  To: alsa-devel, linux-kernel, Jaroslav Kysela, Takashi Iwai,
	Timo Wischer, Andrew Gabbasov

From: Timo Wischer <twischer@de.adit-jv.com>

This commit does not change the behaviour. It only separates the jiffies
timer specific implementation from the generic part.

Signed-off-by: Timo Wischer <twischer@de.adit-jv.com>
Signed-off-by: Andrew Gabbasov <andrew_gabbasov@mentor.com>
---
 sound/drivers/aloop.c | 28 +++++++++++++++-------------
 1 file changed, 15 insertions(+), 13 deletions(-)

diff --git a/sound/drivers/aloop.c b/sound/drivers/aloop.c
index 3bfd7c32803c..2f208aaa54cf 100644
--- a/sound/drivers/aloop.c
+++ b/sound/drivers/aloop.c
@@ -188,7 +188,7 @@ static inline unsigned int get_rate_shift(struct loopback_pcm *dpcm)
 }
 
 /* call in cable->lock */
-static int loopback_timer_start(struct loopback_pcm *dpcm)
+static int loopback_jiffies_timer_start(struct loopback_pcm *dpcm)
 {
 	unsigned long tick;
 	unsigned int rate_shift = get_rate_shift(dpcm);
@@ -209,7 +209,7 @@ static int loopback_timer_start(struct loopback_pcm *dpcm)
 }
 
 /* call in cable->lock */
-static inline int loopback_timer_stop(struct loopback_pcm *dpcm)
+static inline int loopback_jiffies_timer_stop(struct loopback_pcm *dpcm)
 {
 	del_timer(&dpcm->timer);
 	dpcm->timer.expires = 0;
@@ -217,7 +217,7 @@ static inline int loopback_timer_stop(struct loopback_pcm *dpcm)
 	return 0;
 }
 
-static inline int loopback_timer_stop_sync(struct loopback_pcm *dpcm)
+static inline int loopback_jiffies_timer_stop_sync(struct loopback_pcm *dpcm)
 {
 	del_timer_sync(&dpcm->timer);
 
@@ -502,7 +502,8 @@ static inline void bytepos_finish(struct loopback_pcm *dpcm,
 }
 
 /* call in cable->lock */
-static unsigned int loopback_pos_update(struct loopback_cable *cable)
+static unsigned int loopback_jiffies_timer_pos_update
+		(struct loopback_cable *cable)
 {
 	struct loopback_pcm *dpcm_play =
 			cable->streams[SNDRV_PCM_STREAM_PLAYBACK];
@@ -555,14 +556,15 @@ static unsigned int loopback_pos_update(struct loopback_cable *cable)
 	return running;
 }
 
-static void loopback_timer_function(struct timer_list *t)
+static void loopback_jiffies_timer_function(struct timer_list *t)
 {
 	struct loopback_pcm *dpcm = from_timer(dpcm, t, timer);
 	unsigned long flags;
 
 	spin_lock_irqsave(&dpcm->cable->lock, flags);
-	if (loopback_pos_update(dpcm->cable) & (1 << dpcm->substream->stream)) {
-		loopback_timer_start(dpcm);
+	if (loopback_jiffies_timer_pos_update(dpcm->cable) &
+			(1 << dpcm->substream->stream)) {
+		loopback_jiffies_timer_start(dpcm);
 		if (dpcm->period_update_pending) {
 			dpcm->period_update_pending = 0;
 			spin_unlock_irqrestore(&dpcm->cable->lock, flags);
@@ -731,18 +733,18 @@ static void free_cable(struct snd_pcm_substream *substream)
 
 static int loopback_jiffies_timer_open(struct loopback_pcm *dpcm)
 {
-	timer_setup(&dpcm->timer, loopback_timer_function, 0);
+	timer_setup(&dpcm->timer, loopback_jiffies_timer_function, 0);
 
 	return 0;
 }
 
 static struct loopback_ops loopback_jiffies_timer_ops = {
 	.open = loopback_jiffies_timer_open,
-	.start = loopback_timer_start,
-	.stop = loopback_timer_stop,
-	.stop_sync = loopback_timer_stop_sync,
-	.close_substream = loopback_timer_stop_sync,
-	.pos_update = loopback_pos_update,
+	.start = loopback_jiffies_timer_start,
+	.stop = loopback_jiffies_timer_stop,
+	.stop_sync = loopback_jiffies_timer_stop_sync,
+	.close_substream = loopback_jiffies_timer_stop_sync,
+	.pos_update = loopback_jiffies_timer_pos_update,
 	.dpcm_info = loopback_jiffies_timer_dpcm_info,
 };
 
-- 
2.21.0

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

^ permalink raw reply related	[flat|nested] 26+ messages in thread

* [PATCH v4 5/7] ALSA: aloop: Move CABLE_VALID_BOTH to the top of file
  2019-11-20 11:58         ` [alsa-devel] " Andrew Gabbasov
@ 2019-11-20 11:58           ` Andrew Gabbasov
  -1 siblings, 0 replies; 26+ messages in thread
From: Andrew Gabbasov @ 2019-11-20 11:58 UTC (permalink / raw)
  To: alsa-devel, linux-kernel, Jaroslav Kysela, Takashi Iwai,
	Timo Wischer, Andrew Gabbasov

From: Timo Wischer <twischer@de.adit-jv.com>

so all functions can use the same.

Signed-off-by: Timo Wischer <twischer@de.adit-jv.com>
Signed-off-by: Andrew Gabbasov <andrew_gabbasov@mentor.com>
---
 sound/drivers/aloop.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/sound/drivers/aloop.c b/sound/drivers/aloop.c
index 2f208aaa54cf..0eacaa9d7862 100644
--- a/sound/drivers/aloop.c
+++ b/sound/drivers/aloop.c
@@ -55,6 +55,10 @@ MODULE_PARM_DESC(pcm_notify, "Break capture when PCM format/rate/channels change
 
 #define NO_PITCH 100000
 
+#define CABLE_VALID_PLAYBACK	BIT(SNDRV_PCM_STREAM_PLAYBACK)
+#define CABLE_VALID_CAPTURE	BIT(SNDRV_PCM_STREAM_CAPTURE)
+#define CABLE_VALID_BOTH	(CABLE_VALID_PLAYBACK | CABLE_VALID_CAPTURE)
+
 struct loopback_cable;
 struct loopback_pcm;
 
@@ -224,10 +228,6 @@ static inline int loopback_jiffies_timer_stop_sync(struct loopback_pcm *dpcm)
 	return 0;
 }
 
-#define CABLE_VALID_PLAYBACK	(1 << SNDRV_PCM_STREAM_PLAYBACK)
-#define CABLE_VALID_CAPTURE	(1 << SNDRV_PCM_STREAM_CAPTURE)
-#define CABLE_VALID_BOTH	(CABLE_VALID_PLAYBACK|CABLE_VALID_CAPTURE)
-
 static int loopback_check_format(struct loopback_cable *cable, int stream)
 {
 	struct snd_pcm_runtime *runtime, *cruntime;
-- 
2.21.0


^ permalink raw reply related	[flat|nested] 26+ messages in thread

* [alsa-devel] [PATCH v4 5/7] ALSA: aloop: Move CABLE_VALID_BOTH to the top of file
@ 2019-11-20 11:58           ` Andrew Gabbasov
  0 siblings, 0 replies; 26+ messages in thread
From: Andrew Gabbasov @ 2019-11-20 11:58 UTC (permalink / raw)
  To: alsa-devel, linux-kernel, Jaroslav Kysela, Takashi Iwai,
	Timo Wischer, Andrew Gabbasov

From: Timo Wischer <twischer@de.adit-jv.com>

so all functions can use the same.

Signed-off-by: Timo Wischer <twischer@de.adit-jv.com>
Signed-off-by: Andrew Gabbasov <andrew_gabbasov@mentor.com>
---
 sound/drivers/aloop.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/sound/drivers/aloop.c b/sound/drivers/aloop.c
index 2f208aaa54cf..0eacaa9d7862 100644
--- a/sound/drivers/aloop.c
+++ b/sound/drivers/aloop.c
@@ -55,6 +55,10 @@ MODULE_PARM_DESC(pcm_notify, "Break capture when PCM format/rate/channels change
 
 #define NO_PITCH 100000
 
+#define CABLE_VALID_PLAYBACK	BIT(SNDRV_PCM_STREAM_PLAYBACK)
+#define CABLE_VALID_CAPTURE	BIT(SNDRV_PCM_STREAM_CAPTURE)
+#define CABLE_VALID_BOTH	(CABLE_VALID_PLAYBACK | CABLE_VALID_CAPTURE)
+
 struct loopback_cable;
 struct loopback_pcm;
 
@@ -224,10 +228,6 @@ static inline int loopback_jiffies_timer_stop_sync(struct loopback_pcm *dpcm)
 	return 0;
 }
 
-#define CABLE_VALID_PLAYBACK	(1 << SNDRV_PCM_STREAM_PLAYBACK)
-#define CABLE_VALID_CAPTURE	(1 << SNDRV_PCM_STREAM_CAPTURE)
-#define CABLE_VALID_BOTH	(CABLE_VALID_PLAYBACK|CABLE_VALID_CAPTURE)
-
 static int loopback_check_format(struct loopback_cable *cable, int stream)
 {
 	struct snd_pcm_runtime *runtime, *cruntime;
-- 
2.21.0

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

^ permalink raw reply related	[flat|nested] 26+ messages in thread

* [PATCH v4 6/7] ALSA: aloop: Support selection of snd_timer instead of jiffies
  2019-11-20 11:58           ` [alsa-devel] " Andrew Gabbasov
@ 2019-11-20 11:58             ` Andrew Gabbasov
  -1 siblings, 0 replies; 26+ messages in thread
From: Andrew Gabbasov @ 2019-11-20 11:58 UTC (permalink / raw)
  To: alsa-devel, linux-kernel, Jaroslav Kysela, Takashi Iwai,
	Timo Wischer, Andrew Gabbasov

From: Timo Wischer <twischer@de.adit-jv.com>

to do synchronous audio forwarding between hardware sound card and aloop
devices. Such an audio route could look like the following:
Sound card -> Loopback application -> ALSA loop device -> arecord

In this case the loopback device should use the sound timer of the sound
card. Without this patch the loopback application has to implement an
adaptive sample rate converter to align the different clocks of the
different ALSA devices.

The used timer can be selected by referring to a sound card, its device
and subdevice, when loading the module:
  $ modprobe snd_aloop enable=1 timer_source=[<card>[.<dev>[.<subdev>]]]
<card> is the name (id) of the sound card or a card number.
<dev> and <subdev> are device and subdevice numbers (defaults are 0).
Empty string as a value of timer_source= parameter enables previous
functionality (using jiffies timer).

Signed-off-by: Timo Wischer <twischer@de.adit-jv.com>
Signed-off-by: Andrew Gabbasov <andrew_gabbasov@mentor.com>
---
 sound/drivers/aloop.c | 477 +++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 476 insertions(+), 1 deletion(-)

diff --git a/sound/drivers/aloop.c b/sound/drivers/aloop.c
index 0eacaa9d7862..67ebbd510e1b 100644
--- a/sound/drivers/aloop.c
+++ b/sound/drivers/aloop.c
@@ -28,6 +28,7 @@
 #include <sound/pcm_params.h>
 #include <sound/info.h>
 #include <sound/initval.h>
+#include <sound/timer.h>
 
 MODULE_AUTHOR("Jaroslav Kysela <perex@perex.cz>");
 MODULE_DESCRIPTION("A loopback soundcard");
@@ -41,6 +42,7 @@ static char *id[SNDRV_CARDS] = SNDRV_DEFAULT_STR;	/* ID for this card */
 static bool enable[SNDRV_CARDS] = {1, [1 ... (SNDRV_CARDS - 1)] = 0};
 static int pcm_substreams[SNDRV_CARDS] = {[0 ... (SNDRV_CARDS - 1)] = 8};
 static int pcm_notify[SNDRV_CARDS];
+static char *timer_source[SNDRV_CARDS];
 
 module_param_array(index, int, NULL, 0444);
 MODULE_PARM_DESC(index, "Index value for loopback soundcard.");
@@ -52,6 +54,8 @@ module_param_array(pcm_substreams, int, NULL, 0444);
 MODULE_PARM_DESC(pcm_substreams, "PCM substreams # (1-8) for loopback driver.");
 module_param_array(pcm_notify, int, NULL, 0444);
 MODULE_PARM_DESC(pcm_notify, "Break capture when PCM format/rate/channels changes.");
+module_param_array(timer_source, charp, NULL, 0444);
+MODULE_PARM_DESC(timer_source, "Sound card name or number and device/subdevice number of timer to be used. Empty string for jiffies timer [default].");
 
 #define NO_PITCH 100000
 
@@ -102,6 +106,13 @@ struct loopback_cable {
 	unsigned int pause;
 	/* timer specific */
 	struct loopback_ops *ops;
+	/* If sound timer is used */
+	struct {
+		int stream;
+		struct snd_timer_id id;
+		struct tasklet_struct event_tasklet;
+		struct snd_timer_instance *instance;
+	} snd_timer;
 };
 
 struct loopback_setup {
@@ -122,6 +133,7 @@ struct loopback {
 	struct loopback_cable *cables[MAX_PCM_SUBSTREAMS][2];
 	struct snd_pcm *pcm[2];
 	struct loopback_setup setup[MAX_PCM_SUBSTREAMS][2];
+	const char *timer_source;
 };
 
 struct loopback_pcm {
@@ -145,6 +157,7 @@ struct loopback_pcm {
 	unsigned int period_size_frac;	/* period size in jiffies ticks */
 	unsigned int last_drift;
 	unsigned long last_jiffies;
+	/* If jiffies timer is used */
 	struct timer_list timer;
 };
 
@@ -212,6 +225,35 @@ static int loopback_jiffies_timer_start(struct loopback_pcm *dpcm)
 	return 0;
 }
 
+/* call in cable->lock */
+static int loopback_snd_timer_start(struct loopback_pcm *dpcm)
+{
+	struct loopback_cable *cable = dpcm->cable;
+	int err;
+
+	/* Loopback device has to use same period as timer card. Therefore
+	 * wake up for each snd_pcm_period_elapsed() call of timer card.
+	 */
+	err = snd_timer_start(cable->snd_timer.instance, 1);
+	if (err < 0) {
+		/* do not report error if trying to start but already
+		 * running. For example called by opposite substream
+		 * of the same cable
+		 */
+		if (err == -EBUSY)
+			return 0;
+
+		pcm_err(dpcm->substream->pcm,
+			"snd_timer_start(%d,%d,%d) failed with %d",
+			cable->snd_timer.id.card,
+			cable->snd_timer.id.device,
+			cable->snd_timer.id.subdevice,
+			err);
+	}
+
+	return err;
+}
+
 /* call in cable->lock */
 static inline int loopback_jiffies_timer_stop(struct loopback_pcm *dpcm)
 {
@@ -221,6 +263,29 @@ static inline int loopback_jiffies_timer_stop(struct loopback_pcm *dpcm)
 	return 0;
 }
 
+/* call in cable->lock */
+static int loopback_snd_timer_stop(struct loopback_pcm *dpcm)
+{
+	struct loopback_cable *cable = dpcm->cable;
+	int err;
+
+	/* only stop if both devices (playback and capture) are not running */
+	if (cable->running ^ cable->pause)
+		return 0;
+
+	err = snd_timer_stop(cable->snd_timer.instance);
+	if (err < 0) {
+		pcm_err(dpcm->substream->pcm,
+			"snd_timer_stop(%d,%d,%d) failed with %d",
+			cable->snd_timer.id.card,
+			cable->snd_timer.id.device,
+			cable->snd_timer.id.subdevice,
+			err);
+	}
+
+	return err;
+}
+
 static inline int loopback_jiffies_timer_stop_sync(struct loopback_pcm *dpcm)
 {
 	del_timer_sync(&dpcm->timer);
@@ -228,6 +293,30 @@ static inline int loopback_jiffies_timer_stop_sync(struct loopback_pcm *dpcm)
 	return 0;
 }
 
+/* call in loopback->cable_lock */
+static int loopback_snd_timer_close_cable(struct loopback_pcm *dpcm)
+{
+	struct loopback_cable *cable = dpcm->cable;
+
+	/* snd_timer was not opened */
+	if (!cable->snd_timer.instance)
+		return 0;
+
+	/* wait till drain tasklet has finished if requested */
+	tasklet_kill(&cable->snd_timer.event_tasklet);
+
+	/* will only be called from free_cable() when other stream was
+	 * already closed. Other stream cannot be reopened as long as
+	 * loopback->cable_lock is locked. Therefore no need to lock
+	 * cable->lock;
+	 */
+	snd_timer_close(cable->snd_timer.instance);
+	snd_timer_instance_free(cable->snd_timer.instance);
+	memset(&cable->snd_timer, 0, sizeof(cable->snd_timer));
+
+	return 0;
+}
+
 static int loopback_check_format(struct loopback_cable *cable, int stream)
 {
 	struct snd_pcm_runtime *runtime, *cruntime;
@@ -353,6 +442,13 @@ static void params_change(struct snd_pcm_substream *substream)
 	cable->hw.rate_max = runtime->rate;
 	cable->hw.channels_min = runtime->channels;
 	cable->hw.channels_max = runtime->channels;
+
+	if (cable->snd_timer.instance) {
+		cable->hw.period_bytes_min =
+				frames_to_bytes(runtime, runtime->period_size);
+		cable->hw.period_bytes_max = cable->hw.period_bytes_min;
+	}
+
 }
 
 static int loopback_prepare(struct snd_pcm_substream *substream)
@@ -576,6 +672,167 @@ static void loopback_jiffies_timer_function(struct timer_list *t)
 	spin_unlock_irqrestore(&dpcm->cable->lock, flags);
 }
 
+/* call in cable->lock */
+static int loopback_snd_timer_check_resolution(struct snd_pcm_runtime *runtime,
+					       unsigned long resolution)
+{
+	if (resolution != runtime->timer_resolution) {
+		struct loopback_pcm *dpcm = runtime->private_data;
+		struct loopback_cable *cable = dpcm->cable;
+		/* Worst case estimation of possible values for resolution
+		 * resolution <= (512 * 1024) frames / 8kHz in nsec
+		 * resolution <= 65.536.000.000 nsec
+		 *
+		 * period_size <= 65.536.000.000 nsec / 1000nsec/usec * 192kHz +
+		 *  500.000
+		 * period_size <= 12.582.912.000.000  <64bit
+		 *  / 1.000.000 usec/sec
+		 */
+		snd_pcm_uframes_t period_size_usec =
+				resolution / 1000 * runtime->rate;
+		/* round to nearest sample rate */
+		snd_pcm_uframes_t period_size =
+				(period_size_usec + 500 * 1000) / (1000 * 1000);
+
+		pcm_err(dpcm->substream->pcm,
+			"Period size (%lu frames) of loopback device is not corresponding to timer resolution (%lu nsec = %lu frames) of card timer %d,%d,%d. Use period size of %lu frames for loopback device.",
+			runtime->period_size, resolution, period_size,
+			cable->snd_timer.id.card,
+			cable->snd_timer.id.device,
+			cable->snd_timer.id.subdevice,
+			period_size);
+		return -EINVAL;
+	}
+	return 0;
+}
+
+static void loopback_snd_timer_period_elapsed(struct loopback_cable *cable,
+					      int event,
+					      unsigned long resolution)
+{
+	struct loopback_pcm *dpcm_play, *dpcm_capt;
+	struct snd_pcm_substream *substream_play, *substream_capt;
+	struct snd_pcm_runtime *valid_runtime;
+	unsigned int running, elapsed_bytes;
+	unsigned long flags;
+
+	spin_lock_irqsave(&cable->lock, flags);
+	running = cable->running ^ cable->pause;
+	/* no need to do anything if no stream is running */
+	if (!running) {
+		spin_unlock_irqrestore(&cable->lock, flags);
+		return;
+	}
+
+	dpcm_play = cable->streams[SNDRV_PCM_STREAM_PLAYBACK];
+	dpcm_capt = cable->streams[SNDRV_PCM_STREAM_CAPTURE];
+	substream_play = (running & (1 << SNDRV_PCM_STREAM_PLAYBACK)) ?
+			dpcm_play->substream : NULL;
+	substream_capt = (running & (1 << SNDRV_PCM_STREAM_CAPTURE)) ?
+			dpcm_capt->substream : NULL;
+
+	if (event == SNDRV_TIMER_EVENT_MSTOP) {
+		if (!dpcm_play ||
+		    dpcm_play->substream->runtime->status->state !=
+				SNDRV_PCM_STATE_DRAINING) {
+			spin_unlock_irqrestore(&cable->lock, flags);
+			return;
+		}
+	}
+
+	valid_runtime = (running & (1 << SNDRV_PCM_STREAM_PLAYBACK)) ?
+				dpcm_play->substream->runtime :
+				dpcm_capt->substream->runtime;
+
+	/* resolution is only valid for SNDRV_TIMER_EVENT_TICK events */
+	if (event == SNDRV_TIMER_EVENT_TICK) {
+		/* The hardware rules guarantee that playback and capture period
+		 * are the same. Therefore only one device has to be checked
+		 * here.
+		 */
+		if (loopback_snd_timer_check_resolution(valid_runtime,
+							resolution) < 0) {
+			spin_unlock_irqrestore(&cable->lock, flags);
+			if (substream_play)
+				snd_pcm_stop_xrun(substream_play);
+			if (substream_capt)
+				snd_pcm_stop_xrun(substream_capt);
+			return;
+		}
+	}
+
+	elapsed_bytes = frames_to_bytes(valid_runtime,
+					valid_runtime->period_size);
+	/* The same timer interrupt is used for playback and capture device */
+	if ((running & (1 << SNDRV_PCM_STREAM_PLAYBACK)) &&
+	    (running & (1 << SNDRV_PCM_STREAM_CAPTURE))) {
+		copy_play_buf(dpcm_play, dpcm_capt, elapsed_bytes);
+		bytepos_finish(dpcm_play, elapsed_bytes);
+		bytepos_finish(dpcm_capt, elapsed_bytes);
+	} else if (running & (1 << SNDRV_PCM_STREAM_PLAYBACK)) {
+		bytepos_finish(dpcm_play, elapsed_bytes);
+	} else if (running & (1 << SNDRV_PCM_STREAM_CAPTURE)) {
+		clear_capture_buf(dpcm_capt, elapsed_bytes);
+		bytepos_finish(dpcm_capt, elapsed_bytes);
+	}
+	spin_unlock_irqrestore(&cable->lock, flags);
+
+	if (substream_play)
+		snd_pcm_period_elapsed(substream_play);
+	if (substream_capt)
+		snd_pcm_period_elapsed(substream_capt);
+}
+
+static void loopback_snd_timer_function(struct snd_timer_instance *timeri,
+					unsigned long resolution,
+					unsigned long ticks)
+{
+	struct loopback_cable *cable = timeri->callback_data;
+
+	loopback_snd_timer_period_elapsed(cable, SNDRV_TIMER_EVENT_TICK,
+					  resolution);
+}
+
+static void loopback_snd_timer_tasklet(unsigned long arg)
+{
+	struct snd_timer_instance *timeri = (struct snd_timer_instance *)arg;
+	struct loopback_cable *cable = timeri->callback_data;
+
+	loopback_snd_timer_period_elapsed(cable, SNDRV_TIMER_EVENT_MSTOP, 0);
+}
+
+static void loopback_snd_timer_event(struct snd_timer_instance *timeri,
+				     int event,
+				     struct timespec *tstamp,
+				     unsigned long resolution)
+{
+	/* Do not lock cable->lock here because timer->lock is already hold.
+	 * There are other functions which first lock cable->lock and than
+	 * timer->lock e.g.
+	 * loopback_trigger()
+	 * spin_lock(&cable->lock)
+	 * loopback_snd_timer_start()
+	 * snd_timer_start()
+	 * spin_lock(&timer->lock)
+	 * Therefore when using the oposit order of locks here it could result
+	 * in a deadlock.
+	 */
+
+	if (event == SNDRV_TIMER_EVENT_MSTOP) {
+		struct loopback_cable *cable = timeri->callback_data;
+
+		/* sound card of the timer was stopped. Therefore there will not
+		 * be any further timer callbacks. Due to this forward audio
+		 * data from here if in draining state. When still in running
+		 * state the streaming will be aborted by the usual timeout. It
+		 * should not be aborted here because may be the timer sound
+		 * card does only a recovery and the timer is back soon.
+		 * This tasklet triggers loopback_snd_timer_tasklet()
+		 */
+		tasklet_schedule(&cable->snd_timer.event_tasklet);
+	}
+}
+
 static void loopback_jiffies_timer_dpcm_info(struct loopback_pcm *dpcm,
 					     struct snd_info_buffer *buffer)
 {
@@ -588,6 +845,20 @@ static void loopback_jiffies_timer_dpcm_info(struct loopback_pcm *dpcm,
 	snd_iprintf(buffer, "    timer_expires:\t%lu\n", dpcm->timer.expires);
 }
 
+static void loopback_snd_timer_dpcm_info(struct loopback_pcm *dpcm,
+					 struct snd_info_buffer *buffer)
+{
+	struct loopback_cable *cable = dpcm->cable;
+
+	snd_iprintf(buffer, "    sound timer:\thw:%d,%d,%d\n",
+		    cable->snd_timer.id.card,
+		    cable->snd_timer.id.device,
+		    cable->snd_timer.id.subdevice);
+	snd_iprintf(buffer, "    timer open:\t\t%s\n",
+		    (cable->snd_timer.stream == SNDRV_PCM_STREAM_CAPTURE) ?
+			    "capture" : "playback");
+}
+
 static snd_pcm_uframes_t loopback_pointer(struct snd_pcm_substream *substream)
 {
 	struct snd_pcm_runtime *runtime = substream->runtime;
@@ -706,6 +977,23 @@ static int rule_channels(struct snd_pcm_hw_params *params,
 	return snd_interval_refine(hw_param_interval(params, rule->var), &t);
 }
 
+static int rule_period_bytes(struct snd_pcm_hw_params *params,
+			     struct snd_pcm_hw_rule *rule)
+{
+	struct loopback_pcm *dpcm = rule->private;
+	struct loopback_cable *cable = dpcm->cable;
+	struct snd_interval t;
+
+	mutex_lock(&dpcm->loopback->cable_lock);
+	t.min = cable->hw.period_bytes_min;
+	t.max = cable->hw.period_bytes_max;
+	mutex_unlock(&dpcm->loopback->cable_lock);
+	t.openmin = 0;
+	t.openmax = 0;
+	t.integer = 0;
+	return snd_interval_refine(hw_param_interval(params, rule->var), &t);
+}
+
 static void free_cable(struct snd_pcm_substream *substream)
 {
 	struct loopback *loopback = substream->private_data;
@@ -748,6 +1036,163 @@ static struct loopback_ops loopback_jiffies_timer_ops = {
 	.dpcm_info = loopback_jiffies_timer_dpcm_info,
 };
 
+static int loopback_parse_timer_id(const char *str,
+				   struct snd_timer_id *tid)
+{
+	/* [<pref>:](<card name>|<card idx>)[{.,}<dev idx>[{.,}<subdev idx>]] */
+	const char * const sep_dev = ".,";
+	const char * const sep_pref = ":";
+	const char *name = str;
+	char *sep, save = '\0';
+	int card_idx = 0, dev = 0, subdev = 0;
+	int err;
+
+	sep = strpbrk(str, sep_pref);
+	if (sep)
+		name = sep + 1;
+	sep = strpbrk(name, sep_dev);
+	if (sep) {
+		save = *sep;
+		*sep = '\0';
+	}
+	err = kstrtoint(name, 0, &card_idx);
+	if (err == -EINVAL) {
+		/* Must be the name, not number */
+		for (card_idx = 0; card_idx < snd_ecards_limit; card_idx++) {
+			struct snd_card *card = snd_card_ref(card_idx);
+
+			if (card) {
+				if (!strcmp(card->id, name))
+					err = 0;
+				snd_card_unref(card);
+			}
+			if (!err)
+				break;
+		}
+	}
+	if (sep) {
+		*sep = save;
+		if (!err) {
+			char *sep2, save2 = '\0';
+
+			sep2 = strpbrk(sep + 1, sep_dev);
+			if (sep2) {
+				save2 = *sep2;
+				*sep2 = '\0';
+			}
+			err = kstrtoint(sep + 1, 0, &dev);
+			if (sep2) {
+				*sep2 = save2;
+				if (!err)
+					err = kstrtoint(sep2 + 1, 0, &subdev);
+			}
+		}
+	}
+	if (!err && tid) {
+		tid->card = card_idx;
+		tid->device = dev;
+		tid->subdevice = subdev;
+	}
+	return err;
+}
+
+/* call in loopback->cable_lock */
+static int loopback_snd_timer_open(struct loopback_pcm *dpcm)
+{
+	int err = 0;
+	struct snd_timer_id tid = {
+		.dev_class = SNDRV_TIMER_CLASS_PCM,
+		.dev_sclass = SNDRV_TIMER_SCLASS_APPLICATION,
+	};
+	struct snd_timer_instance *timeri;
+	struct loopback_cable *cable = dpcm->cable;
+
+	spin_lock_irq(&cable->lock);
+
+	/* check if timer was already opened. It is only opened once
+	 * per playback and capture subdevice (aka cable).
+	 */
+	if (cable->snd_timer.instance)
+		goto unlock;
+
+	err = loopback_parse_timer_id(dpcm->loopback->timer_source, &tid);
+	if (err < 0) {
+		pcm_err(dpcm->substream->pcm,
+			"Parsing timer source \'%s\' failed with %d",
+			dpcm->loopback->timer_source, err);
+		goto unlock;
+	}
+
+	cable->snd_timer.stream = dpcm->substream->stream;
+	cable->snd_timer.id = tid;
+
+	timeri = snd_timer_instance_new(dpcm->loopback->card->id);
+	if (!timeri) {
+		err = -ENOMEM;
+		goto unlock;
+	}
+	/* The callback has to be called from another tasklet. If
+	 * SNDRV_TIMER_IFLG_FAST is specified it will be called from the
+	 * snd_pcm_period_elapsed() call of the selected sound card.
+	 * snd_pcm_period_elapsed() helds snd_pcm_stream_lock_irqsave().
+	 * Due to our callback loopback_snd_timer_function() also calls
+	 * snd_pcm_period_elapsed() which calls snd_pcm_stream_lock_irqsave().
+	 * This would end up in a dead lock.
+	 */
+	timeri->flags |= SNDRV_TIMER_IFLG_AUTO;
+	timeri->callback = loopback_snd_timer_function;
+	timeri->callback_data = (void *)cable;
+	timeri->ccallback = loopback_snd_timer_event;
+
+	/* snd_timer_close() and snd_timer_open() should not be called with
+	 * locked spinlock because both functions can block on a mutex. The
+	 * mutex loopback->cable_lock is kept locked. Therefore snd_timer_open()
+	 * cannot be called a second time by the other device of the same cable.
+	 * Therefore the following issue cannot happen:
+	 * [proc1] Call loopback_timer_open() ->
+	 *	   Unlock cable->lock for snd_timer_close/open() call
+	 * [proc2] Call loopback_timer_open() -> snd_timer_open(),
+	 *	   snd_timer_start()
+	 * [proc1] Call snd_timer_open() and overwrite running timer
+	 *	   instance
+	 */
+	spin_unlock_irq(&cable->lock);
+	err = snd_timer_open(timeri, &cable->snd_timer.id, current->pid);
+	if (err < 0) {
+		pcm_err(dpcm->substream->pcm,
+			"snd_timer_open (%d,%d,%d) failed with %d",
+			cable->snd_timer.id.card,
+			cable->snd_timer.id.device,
+			cable->snd_timer.id.subdevice,
+			err);
+		snd_timer_instance_free(timeri);
+		return err;
+	}
+	spin_lock_irq(&cable->lock);
+
+	cable->snd_timer.instance = timeri;
+
+	/* initialise a tasklet used for draining */
+	tasklet_init(&cable->snd_timer.event_tasklet,
+		     loopback_snd_timer_tasklet, (unsigned long)timeri);
+
+unlock:
+	spin_unlock_irq(&cable->lock);
+
+	return err;
+}
+
+/* stop_sync() is not required for sound timer because it does not need to be
+ * restarted in loopback_prepare() on Xrun recovery
+ */
+static struct loopback_ops loopback_snd_timer_ops = {
+	.open = loopback_snd_timer_open,
+	.start = loopback_snd_timer_start,
+	.stop = loopback_snd_timer_stop,
+	.close_cable = loopback_snd_timer_close_cable,
+	.dpcm_info = loopback_snd_timer_dpcm_info,
+};
+
 static int loopback_open(struct snd_pcm_substream *substream)
 {
 	struct snd_pcm_runtime *runtime = substream->runtime;
@@ -775,7 +1220,10 @@ static int loopback_open(struct snd_pcm_substream *substream)
 		}
 		spin_lock_init(&cable->lock);
 		cable->hw = loopback_pcm_hardware;
-		cable->ops = &loopback_jiffies_timer_ops;
+		if (loopback->timer_source)
+			cable->ops = &loopback_snd_timer_ops;
+		else
+			cable->ops = &loopback_jiffies_timer_ops;
 		loopback->cables[substream->number][dev] = cable;
 	}
 	dpcm->cable = cable;
@@ -811,6 +1259,19 @@ static int loopback_open(struct snd_pcm_substream *substream)
 	if (err < 0)
 		goto unlock;
 
+	/* In case of sound timer the period time of both devices of the same
+	 * loop has to be the same.
+	 * This rule only takes effect if a sound timer was chosen
+	 */
+	if (cable->snd_timer.instance) {
+		err = snd_pcm_hw_rule_add(runtime, 0,
+					  SNDRV_PCM_HW_PARAM_PERIOD_BYTES,
+					  rule_period_bytes, dpcm,
+					  SNDRV_PCM_HW_PARAM_PERIOD_BYTES, -1);
+		if (err < 0)
+			goto unlock;
+	}
+
 	/* loopback_runtime_free() has not to be called if kfree(dpcm) was
 	 * already called here. Otherwise it will end up with a double free.
 	 */
@@ -1214,6 +1675,18 @@ static int loopback_proc_new(struct loopback *loopback, int cidx)
 				    print_cable_info);
 }
 
+static void loopback_set_timer_source(struct loopback *loopback,
+				      const char *value)
+{
+	if (loopback->timer_source) {
+		devm_kfree(loopback->card->dev, loopback->timer_source);
+		loopback->timer_source = NULL;
+	}
+	if (value && *value)
+		loopback->timer_source = devm_kstrdup(loopback->card->dev,
+						      value, GFP_KERNEL);
+}
+
 static int loopback_probe(struct platform_device *devptr)
 {
 	struct snd_card *card;
@@ -1233,6 +1706,8 @@ static int loopback_probe(struct platform_device *devptr)
 		pcm_substreams[dev] = MAX_PCM_SUBSTREAMS;
 	
 	loopback->card = card;
+	loopback_set_timer_source(loopback, timer_source[dev]);
+
 	mutex_init(&loopback->cable_lock);
 
 	err = loopback_pcm_new(loopback, 0, pcm_substreams[dev]);
-- 
2.21.0


^ permalink raw reply related	[flat|nested] 26+ messages in thread

* [alsa-devel] [PATCH v4 6/7] ALSA: aloop: Support selection of snd_timer instead of jiffies
@ 2019-11-20 11:58             ` Andrew Gabbasov
  0 siblings, 0 replies; 26+ messages in thread
From: Andrew Gabbasov @ 2019-11-20 11:58 UTC (permalink / raw)
  To: alsa-devel, linux-kernel, Jaroslav Kysela, Takashi Iwai,
	Timo Wischer, Andrew Gabbasov

From: Timo Wischer <twischer@de.adit-jv.com>

to do synchronous audio forwarding between hardware sound card and aloop
devices. Such an audio route could look like the following:
Sound card -> Loopback application -> ALSA loop device -> arecord

In this case the loopback device should use the sound timer of the sound
card. Without this patch the loopback application has to implement an
adaptive sample rate converter to align the different clocks of the
different ALSA devices.

The used timer can be selected by referring to a sound card, its device
and subdevice, when loading the module:
  $ modprobe snd_aloop enable=1 timer_source=[<card>[.<dev>[.<subdev>]]]
<card> is the name (id) of the sound card or a card number.
<dev> and <subdev> are device and subdevice numbers (defaults are 0).
Empty string as a value of timer_source= parameter enables previous
functionality (using jiffies timer).

Signed-off-by: Timo Wischer <twischer@de.adit-jv.com>
Signed-off-by: Andrew Gabbasov <andrew_gabbasov@mentor.com>
---
 sound/drivers/aloop.c | 477 +++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 476 insertions(+), 1 deletion(-)

diff --git a/sound/drivers/aloop.c b/sound/drivers/aloop.c
index 0eacaa9d7862..67ebbd510e1b 100644
--- a/sound/drivers/aloop.c
+++ b/sound/drivers/aloop.c
@@ -28,6 +28,7 @@
 #include <sound/pcm_params.h>
 #include <sound/info.h>
 #include <sound/initval.h>
+#include <sound/timer.h>
 
 MODULE_AUTHOR("Jaroslav Kysela <perex@perex.cz>");
 MODULE_DESCRIPTION("A loopback soundcard");
@@ -41,6 +42,7 @@ static char *id[SNDRV_CARDS] = SNDRV_DEFAULT_STR;	/* ID for this card */
 static bool enable[SNDRV_CARDS] = {1, [1 ... (SNDRV_CARDS - 1)] = 0};
 static int pcm_substreams[SNDRV_CARDS] = {[0 ... (SNDRV_CARDS - 1)] = 8};
 static int pcm_notify[SNDRV_CARDS];
+static char *timer_source[SNDRV_CARDS];
 
 module_param_array(index, int, NULL, 0444);
 MODULE_PARM_DESC(index, "Index value for loopback soundcard.");
@@ -52,6 +54,8 @@ module_param_array(pcm_substreams, int, NULL, 0444);
 MODULE_PARM_DESC(pcm_substreams, "PCM substreams # (1-8) for loopback driver.");
 module_param_array(pcm_notify, int, NULL, 0444);
 MODULE_PARM_DESC(pcm_notify, "Break capture when PCM format/rate/channels changes.");
+module_param_array(timer_source, charp, NULL, 0444);
+MODULE_PARM_DESC(timer_source, "Sound card name or number and device/subdevice number of timer to be used. Empty string for jiffies timer [default].");
 
 #define NO_PITCH 100000
 
@@ -102,6 +106,13 @@ struct loopback_cable {
 	unsigned int pause;
 	/* timer specific */
 	struct loopback_ops *ops;
+	/* If sound timer is used */
+	struct {
+		int stream;
+		struct snd_timer_id id;
+		struct tasklet_struct event_tasklet;
+		struct snd_timer_instance *instance;
+	} snd_timer;
 };
 
 struct loopback_setup {
@@ -122,6 +133,7 @@ struct loopback {
 	struct loopback_cable *cables[MAX_PCM_SUBSTREAMS][2];
 	struct snd_pcm *pcm[2];
 	struct loopback_setup setup[MAX_PCM_SUBSTREAMS][2];
+	const char *timer_source;
 };
 
 struct loopback_pcm {
@@ -145,6 +157,7 @@ struct loopback_pcm {
 	unsigned int period_size_frac;	/* period size in jiffies ticks */
 	unsigned int last_drift;
 	unsigned long last_jiffies;
+	/* If jiffies timer is used */
 	struct timer_list timer;
 };
 
@@ -212,6 +225,35 @@ static int loopback_jiffies_timer_start(struct loopback_pcm *dpcm)
 	return 0;
 }
 
+/* call in cable->lock */
+static int loopback_snd_timer_start(struct loopback_pcm *dpcm)
+{
+	struct loopback_cable *cable = dpcm->cable;
+	int err;
+
+	/* Loopback device has to use same period as timer card. Therefore
+	 * wake up for each snd_pcm_period_elapsed() call of timer card.
+	 */
+	err = snd_timer_start(cable->snd_timer.instance, 1);
+	if (err < 0) {
+		/* do not report error if trying to start but already
+		 * running. For example called by opposite substream
+		 * of the same cable
+		 */
+		if (err == -EBUSY)
+			return 0;
+
+		pcm_err(dpcm->substream->pcm,
+			"snd_timer_start(%d,%d,%d) failed with %d",
+			cable->snd_timer.id.card,
+			cable->snd_timer.id.device,
+			cable->snd_timer.id.subdevice,
+			err);
+	}
+
+	return err;
+}
+
 /* call in cable->lock */
 static inline int loopback_jiffies_timer_stop(struct loopback_pcm *dpcm)
 {
@@ -221,6 +263,29 @@ static inline int loopback_jiffies_timer_stop(struct loopback_pcm *dpcm)
 	return 0;
 }
 
+/* call in cable->lock */
+static int loopback_snd_timer_stop(struct loopback_pcm *dpcm)
+{
+	struct loopback_cable *cable = dpcm->cable;
+	int err;
+
+	/* only stop if both devices (playback and capture) are not running */
+	if (cable->running ^ cable->pause)
+		return 0;
+
+	err = snd_timer_stop(cable->snd_timer.instance);
+	if (err < 0) {
+		pcm_err(dpcm->substream->pcm,
+			"snd_timer_stop(%d,%d,%d) failed with %d",
+			cable->snd_timer.id.card,
+			cable->snd_timer.id.device,
+			cable->snd_timer.id.subdevice,
+			err);
+	}
+
+	return err;
+}
+
 static inline int loopback_jiffies_timer_stop_sync(struct loopback_pcm *dpcm)
 {
 	del_timer_sync(&dpcm->timer);
@@ -228,6 +293,30 @@ static inline int loopback_jiffies_timer_stop_sync(struct loopback_pcm *dpcm)
 	return 0;
 }
 
+/* call in loopback->cable_lock */
+static int loopback_snd_timer_close_cable(struct loopback_pcm *dpcm)
+{
+	struct loopback_cable *cable = dpcm->cable;
+
+	/* snd_timer was not opened */
+	if (!cable->snd_timer.instance)
+		return 0;
+
+	/* wait till drain tasklet has finished if requested */
+	tasklet_kill(&cable->snd_timer.event_tasklet);
+
+	/* will only be called from free_cable() when other stream was
+	 * already closed. Other stream cannot be reopened as long as
+	 * loopback->cable_lock is locked. Therefore no need to lock
+	 * cable->lock;
+	 */
+	snd_timer_close(cable->snd_timer.instance);
+	snd_timer_instance_free(cable->snd_timer.instance);
+	memset(&cable->snd_timer, 0, sizeof(cable->snd_timer));
+
+	return 0;
+}
+
 static int loopback_check_format(struct loopback_cable *cable, int stream)
 {
 	struct snd_pcm_runtime *runtime, *cruntime;
@@ -353,6 +442,13 @@ static void params_change(struct snd_pcm_substream *substream)
 	cable->hw.rate_max = runtime->rate;
 	cable->hw.channels_min = runtime->channels;
 	cable->hw.channels_max = runtime->channels;
+
+	if (cable->snd_timer.instance) {
+		cable->hw.period_bytes_min =
+				frames_to_bytes(runtime, runtime->period_size);
+		cable->hw.period_bytes_max = cable->hw.period_bytes_min;
+	}
+
 }
 
 static int loopback_prepare(struct snd_pcm_substream *substream)
@@ -576,6 +672,167 @@ static void loopback_jiffies_timer_function(struct timer_list *t)
 	spin_unlock_irqrestore(&dpcm->cable->lock, flags);
 }
 
+/* call in cable->lock */
+static int loopback_snd_timer_check_resolution(struct snd_pcm_runtime *runtime,
+					       unsigned long resolution)
+{
+	if (resolution != runtime->timer_resolution) {
+		struct loopback_pcm *dpcm = runtime->private_data;
+		struct loopback_cable *cable = dpcm->cable;
+		/* Worst case estimation of possible values for resolution
+		 * resolution <= (512 * 1024) frames / 8kHz in nsec
+		 * resolution <= 65.536.000.000 nsec
+		 *
+		 * period_size <= 65.536.000.000 nsec / 1000nsec/usec * 192kHz +
+		 *  500.000
+		 * period_size <= 12.582.912.000.000  <64bit
+		 *  / 1.000.000 usec/sec
+		 */
+		snd_pcm_uframes_t period_size_usec =
+				resolution / 1000 * runtime->rate;
+		/* round to nearest sample rate */
+		snd_pcm_uframes_t period_size =
+				(period_size_usec + 500 * 1000) / (1000 * 1000);
+
+		pcm_err(dpcm->substream->pcm,
+			"Period size (%lu frames) of loopback device is not corresponding to timer resolution (%lu nsec = %lu frames) of card timer %d,%d,%d. Use period size of %lu frames for loopback device.",
+			runtime->period_size, resolution, period_size,
+			cable->snd_timer.id.card,
+			cable->snd_timer.id.device,
+			cable->snd_timer.id.subdevice,
+			period_size);
+		return -EINVAL;
+	}
+	return 0;
+}
+
+static void loopback_snd_timer_period_elapsed(struct loopback_cable *cable,
+					      int event,
+					      unsigned long resolution)
+{
+	struct loopback_pcm *dpcm_play, *dpcm_capt;
+	struct snd_pcm_substream *substream_play, *substream_capt;
+	struct snd_pcm_runtime *valid_runtime;
+	unsigned int running, elapsed_bytes;
+	unsigned long flags;
+
+	spin_lock_irqsave(&cable->lock, flags);
+	running = cable->running ^ cable->pause;
+	/* no need to do anything if no stream is running */
+	if (!running) {
+		spin_unlock_irqrestore(&cable->lock, flags);
+		return;
+	}
+
+	dpcm_play = cable->streams[SNDRV_PCM_STREAM_PLAYBACK];
+	dpcm_capt = cable->streams[SNDRV_PCM_STREAM_CAPTURE];
+	substream_play = (running & (1 << SNDRV_PCM_STREAM_PLAYBACK)) ?
+			dpcm_play->substream : NULL;
+	substream_capt = (running & (1 << SNDRV_PCM_STREAM_CAPTURE)) ?
+			dpcm_capt->substream : NULL;
+
+	if (event == SNDRV_TIMER_EVENT_MSTOP) {
+		if (!dpcm_play ||
+		    dpcm_play->substream->runtime->status->state !=
+				SNDRV_PCM_STATE_DRAINING) {
+			spin_unlock_irqrestore(&cable->lock, flags);
+			return;
+		}
+	}
+
+	valid_runtime = (running & (1 << SNDRV_PCM_STREAM_PLAYBACK)) ?
+				dpcm_play->substream->runtime :
+				dpcm_capt->substream->runtime;
+
+	/* resolution is only valid for SNDRV_TIMER_EVENT_TICK events */
+	if (event == SNDRV_TIMER_EVENT_TICK) {
+		/* The hardware rules guarantee that playback and capture period
+		 * are the same. Therefore only one device has to be checked
+		 * here.
+		 */
+		if (loopback_snd_timer_check_resolution(valid_runtime,
+							resolution) < 0) {
+			spin_unlock_irqrestore(&cable->lock, flags);
+			if (substream_play)
+				snd_pcm_stop_xrun(substream_play);
+			if (substream_capt)
+				snd_pcm_stop_xrun(substream_capt);
+			return;
+		}
+	}
+
+	elapsed_bytes = frames_to_bytes(valid_runtime,
+					valid_runtime->period_size);
+	/* The same timer interrupt is used for playback and capture device */
+	if ((running & (1 << SNDRV_PCM_STREAM_PLAYBACK)) &&
+	    (running & (1 << SNDRV_PCM_STREAM_CAPTURE))) {
+		copy_play_buf(dpcm_play, dpcm_capt, elapsed_bytes);
+		bytepos_finish(dpcm_play, elapsed_bytes);
+		bytepos_finish(dpcm_capt, elapsed_bytes);
+	} else if (running & (1 << SNDRV_PCM_STREAM_PLAYBACK)) {
+		bytepos_finish(dpcm_play, elapsed_bytes);
+	} else if (running & (1 << SNDRV_PCM_STREAM_CAPTURE)) {
+		clear_capture_buf(dpcm_capt, elapsed_bytes);
+		bytepos_finish(dpcm_capt, elapsed_bytes);
+	}
+	spin_unlock_irqrestore(&cable->lock, flags);
+
+	if (substream_play)
+		snd_pcm_period_elapsed(substream_play);
+	if (substream_capt)
+		snd_pcm_period_elapsed(substream_capt);
+}
+
+static void loopback_snd_timer_function(struct snd_timer_instance *timeri,
+					unsigned long resolution,
+					unsigned long ticks)
+{
+	struct loopback_cable *cable = timeri->callback_data;
+
+	loopback_snd_timer_period_elapsed(cable, SNDRV_TIMER_EVENT_TICK,
+					  resolution);
+}
+
+static void loopback_snd_timer_tasklet(unsigned long arg)
+{
+	struct snd_timer_instance *timeri = (struct snd_timer_instance *)arg;
+	struct loopback_cable *cable = timeri->callback_data;
+
+	loopback_snd_timer_period_elapsed(cable, SNDRV_TIMER_EVENT_MSTOP, 0);
+}
+
+static void loopback_snd_timer_event(struct snd_timer_instance *timeri,
+				     int event,
+				     struct timespec *tstamp,
+				     unsigned long resolution)
+{
+	/* Do not lock cable->lock here because timer->lock is already hold.
+	 * There are other functions which first lock cable->lock and than
+	 * timer->lock e.g.
+	 * loopback_trigger()
+	 * spin_lock(&cable->lock)
+	 * loopback_snd_timer_start()
+	 * snd_timer_start()
+	 * spin_lock(&timer->lock)
+	 * Therefore when using the oposit order of locks here it could result
+	 * in a deadlock.
+	 */
+
+	if (event == SNDRV_TIMER_EVENT_MSTOP) {
+		struct loopback_cable *cable = timeri->callback_data;
+
+		/* sound card of the timer was stopped. Therefore there will not
+		 * be any further timer callbacks. Due to this forward audio
+		 * data from here if in draining state. When still in running
+		 * state the streaming will be aborted by the usual timeout. It
+		 * should not be aborted here because may be the timer sound
+		 * card does only a recovery and the timer is back soon.
+		 * This tasklet triggers loopback_snd_timer_tasklet()
+		 */
+		tasklet_schedule(&cable->snd_timer.event_tasklet);
+	}
+}
+
 static void loopback_jiffies_timer_dpcm_info(struct loopback_pcm *dpcm,
 					     struct snd_info_buffer *buffer)
 {
@@ -588,6 +845,20 @@ static void loopback_jiffies_timer_dpcm_info(struct loopback_pcm *dpcm,
 	snd_iprintf(buffer, "    timer_expires:\t%lu\n", dpcm->timer.expires);
 }
 
+static void loopback_snd_timer_dpcm_info(struct loopback_pcm *dpcm,
+					 struct snd_info_buffer *buffer)
+{
+	struct loopback_cable *cable = dpcm->cable;
+
+	snd_iprintf(buffer, "    sound timer:\thw:%d,%d,%d\n",
+		    cable->snd_timer.id.card,
+		    cable->snd_timer.id.device,
+		    cable->snd_timer.id.subdevice);
+	snd_iprintf(buffer, "    timer open:\t\t%s\n",
+		    (cable->snd_timer.stream == SNDRV_PCM_STREAM_CAPTURE) ?
+			    "capture" : "playback");
+}
+
 static snd_pcm_uframes_t loopback_pointer(struct snd_pcm_substream *substream)
 {
 	struct snd_pcm_runtime *runtime = substream->runtime;
@@ -706,6 +977,23 @@ static int rule_channels(struct snd_pcm_hw_params *params,
 	return snd_interval_refine(hw_param_interval(params, rule->var), &t);
 }
 
+static int rule_period_bytes(struct snd_pcm_hw_params *params,
+			     struct snd_pcm_hw_rule *rule)
+{
+	struct loopback_pcm *dpcm = rule->private;
+	struct loopback_cable *cable = dpcm->cable;
+	struct snd_interval t;
+
+	mutex_lock(&dpcm->loopback->cable_lock);
+	t.min = cable->hw.period_bytes_min;
+	t.max = cable->hw.period_bytes_max;
+	mutex_unlock(&dpcm->loopback->cable_lock);
+	t.openmin = 0;
+	t.openmax = 0;
+	t.integer = 0;
+	return snd_interval_refine(hw_param_interval(params, rule->var), &t);
+}
+
 static void free_cable(struct snd_pcm_substream *substream)
 {
 	struct loopback *loopback = substream->private_data;
@@ -748,6 +1036,163 @@ static struct loopback_ops loopback_jiffies_timer_ops = {
 	.dpcm_info = loopback_jiffies_timer_dpcm_info,
 };
 
+static int loopback_parse_timer_id(const char *str,
+				   struct snd_timer_id *tid)
+{
+	/* [<pref>:](<card name>|<card idx>)[{.,}<dev idx>[{.,}<subdev idx>]] */
+	const char * const sep_dev = ".,";
+	const char * const sep_pref = ":";
+	const char *name = str;
+	char *sep, save = '\0';
+	int card_idx = 0, dev = 0, subdev = 0;
+	int err;
+
+	sep = strpbrk(str, sep_pref);
+	if (sep)
+		name = sep + 1;
+	sep = strpbrk(name, sep_dev);
+	if (sep) {
+		save = *sep;
+		*sep = '\0';
+	}
+	err = kstrtoint(name, 0, &card_idx);
+	if (err == -EINVAL) {
+		/* Must be the name, not number */
+		for (card_idx = 0; card_idx < snd_ecards_limit; card_idx++) {
+			struct snd_card *card = snd_card_ref(card_idx);
+
+			if (card) {
+				if (!strcmp(card->id, name))
+					err = 0;
+				snd_card_unref(card);
+			}
+			if (!err)
+				break;
+		}
+	}
+	if (sep) {
+		*sep = save;
+		if (!err) {
+			char *sep2, save2 = '\0';
+
+			sep2 = strpbrk(sep + 1, sep_dev);
+			if (sep2) {
+				save2 = *sep2;
+				*sep2 = '\0';
+			}
+			err = kstrtoint(sep + 1, 0, &dev);
+			if (sep2) {
+				*sep2 = save2;
+				if (!err)
+					err = kstrtoint(sep2 + 1, 0, &subdev);
+			}
+		}
+	}
+	if (!err && tid) {
+		tid->card = card_idx;
+		tid->device = dev;
+		tid->subdevice = subdev;
+	}
+	return err;
+}
+
+/* call in loopback->cable_lock */
+static int loopback_snd_timer_open(struct loopback_pcm *dpcm)
+{
+	int err = 0;
+	struct snd_timer_id tid = {
+		.dev_class = SNDRV_TIMER_CLASS_PCM,
+		.dev_sclass = SNDRV_TIMER_SCLASS_APPLICATION,
+	};
+	struct snd_timer_instance *timeri;
+	struct loopback_cable *cable = dpcm->cable;
+
+	spin_lock_irq(&cable->lock);
+
+	/* check if timer was already opened. It is only opened once
+	 * per playback and capture subdevice (aka cable).
+	 */
+	if (cable->snd_timer.instance)
+		goto unlock;
+
+	err = loopback_parse_timer_id(dpcm->loopback->timer_source, &tid);
+	if (err < 0) {
+		pcm_err(dpcm->substream->pcm,
+			"Parsing timer source \'%s\' failed with %d",
+			dpcm->loopback->timer_source, err);
+		goto unlock;
+	}
+
+	cable->snd_timer.stream = dpcm->substream->stream;
+	cable->snd_timer.id = tid;
+
+	timeri = snd_timer_instance_new(dpcm->loopback->card->id);
+	if (!timeri) {
+		err = -ENOMEM;
+		goto unlock;
+	}
+	/* The callback has to be called from another tasklet. If
+	 * SNDRV_TIMER_IFLG_FAST is specified it will be called from the
+	 * snd_pcm_period_elapsed() call of the selected sound card.
+	 * snd_pcm_period_elapsed() helds snd_pcm_stream_lock_irqsave().
+	 * Due to our callback loopback_snd_timer_function() also calls
+	 * snd_pcm_period_elapsed() which calls snd_pcm_stream_lock_irqsave().
+	 * This would end up in a dead lock.
+	 */
+	timeri->flags |= SNDRV_TIMER_IFLG_AUTO;
+	timeri->callback = loopback_snd_timer_function;
+	timeri->callback_data = (void *)cable;
+	timeri->ccallback = loopback_snd_timer_event;
+
+	/* snd_timer_close() and snd_timer_open() should not be called with
+	 * locked spinlock because both functions can block on a mutex. The
+	 * mutex loopback->cable_lock is kept locked. Therefore snd_timer_open()
+	 * cannot be called a second time by the other device of the same cable.
+	 * Therefore the following issue cannot happen:
+	 * [proc1] Call loopback_timer_open() ->
+	 *	   Unlock cable->lock for snd_timer_close/open() call
+	 * [proc2] Call loopback_timer_open() -> snd_timer_open(),
+	 *	   snd_timer_start()
+	 * [proc1] Call snd_timer_open() and overwrite running timer
+	 *	   instance
+	 */
+	spin_unlock_irq(&cable->lock);
+	err = snd_timer_open(timeri, &cable->snd_timer.id, current->pid);
+	if (err < 0) {
+		pcm_err(dpcm->substream->pcm,
+			"snd_timer_open (%d,%d,%d) failed with %d",
+			cable->snd_timer.id.card,
+			cable->snd_timer.id.device,
+			cable->snd_timer.id.subdevice,
+			err);
+		snd_timer_instance_free(timeri);
+		return err;
+	}
+	spin_lock_irq(&cable->lock);
+
+	cable->snd_timer.instance = timeri;
+
+	/* initialise a tasklet used for draining */
+	tasklet_init(&cable->snd_timer.event_tasklet,
+		     loopback_snd_timer_tasklet, (unsigned long)timeri);
+
+unlock:
+	spin_unlock_irq(&cable->lock);
+
+	return err;
+}
+
+/* stop_sync() is not required for sound timer because it does not need to be
+ * restarted in loopback_prepare() on Xrun recovery
+ */
+static struct loopback_ops loopback_snd_timer_ops = {
+	.open = loopback_snd_timer_open,
+	.start = loopback_snd_timer_start,
+	.stop = loopback_snd_timer_stop,
+	.close_cable = loopback_snd_timer_close_cable,
+	.dpcm_info = loopback_snd_timer_dpcm_info,
+};
+
 static int loopback_open(struct snd_pcm_substream *substream)
 {
 	struct snd_pcm_runtime *runtime = substream->runtime;
@@ -775,7 +1220,10 @@ static int loopback_open(struct snd_pcm_substream *substream)
 		}
 		spin_lock_init(&cable->lock);
 		cable->hw = loopback_pcm_hardware;
-		cable->ops = &loopback_jiffies_timer_ops;
+		if (loopback->timer_source)
+			cable->ops = &loopback_snd_timer_ops;
+		else
+			cable->ops = &loopback_jiffies_timer_ops;
 		loopback->cables[substream->number][dev] = cable;
 	}
 	dpcm->cable = cable;
@@ -811,6 +1259,19 @@ static int loopback_open(struct snd_pcm_substream *substream)
 	if (err < 0)
 		goto unlock;
 
+	/* In case of sound timer the period time of both devices of the same
+	 * loop has to be the same.
+	 * This rule only takes effect if a sound timer was chosen
+	 */
+	if (cable->snd_timer.instance) {
+		err = snd_pcm_hw_rule_add(runtime, 0,
+					  SNDRV_PCM_HW_PARAM_PERIOD_BYTES,
+					  rule_period_bytes, dpcm,
+					  SNDRV_PCM_HW_PARAM_PERIOD_BYTES, -1);
+		if (err < 0)
+			goto unlock;
+	}
+
 	/* loopback_runtime_free() has not to be called if kfree(dpcm) was
 	 * already called here. Otherwise it will end up with a double free.
 	 */
@@ -1214,6 +1675,18 @@ static int loopback_proc_new(struct loopback *loopback, int cidx)
 				    print_cable_info);
 }
 
+static void loopback_set_timer_source(struct loopback *loopback,
+				      const char *value)
+{
+	if (loopback->timer_source) {
+		devm_kfree(loopback->card->dev, loopback->timer_source);
+		loopback->timer_source = NULL;
+	}
+	if (value && *value)
+		loopback->timer_source = devm_kstrdup(loopback->card->dev,
+						      value, GFP_KERNEL);
+}
+
 static int loopback_probe(struct platform_device *devptr)
 {
 	struct snd_card *card;
@@ -1233,6 +1706,8 @@ static int loopback_probe(struct platform_device *devptr)
 		pcm_substreams[dev] = MAX_PCM_SUBSTREAMS;
 	
 	loopback->card = card;
+	loopback_set_timer_source(loopback, timer_source[dev]);
+
 	mutex_init(&loopback->cable_lock);
 
 	err = loopback_pcm_new(loopback, 0, pcm_substreams[dev]);
-- 
2.21.0

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

^ permalink raw reply related	[flat|nested] 26+ messages in thread

* [PATCH v4 7/7] ALSA: aloop: Support runtime change of snd_timer via info interface
  2019-11-20 11:58             ` [alsa-devel] " Andrew Gabbasov
@ 2019-11-20 11:58               ` Andrew Gabbasov
  -1 siblings, 0 replies; 26+ messages in thread
From: Andrew Gabbasov @ 2019-11-20 11:58 UTC (permalink / raw)
  To: alsa-devel, linux-kernel, Jaroslav Kysela, Takashi Iwai,
	Timo Wischer, Andrew Gabbasov

Show and change sound card timer source with read-write info
file in proc filesystem. Initial string can still be set as
module parameter.

The timer source string value can be changed at any time,
but it is latched by PCM substream open callback (the first one
for a particular cable). At this point it is actually used, that
is the string is parsed, and the timer is looked up and opened.

The timer source is set for a loopback card (the same as initial
setting by module parameter), but every cable uses the value,
current at the moment of open.

Setting the value to empty string switches the timer to jiffies.

Signed-off-by: Andrew Gabbasov <andrew_gabbasov@mentor.com>
---
 sound/drivers/aloop.c | 37 ++++++++++++++++++++++++++++++++++---
 1 file changed, 34 insertions(+), 3 deletions(-)

diff --git a/sound/drivers/aloop.c b/sound/drivers/aloop.c
index 67ebbd510e1b..626a8b642b63 100644
--- a/sound/drivers/aloop.c
+++ b/sound/drivers/aloop.c
@@ -1666,7 +1666,7 @@ static void print_cable_info(struct snd_info_entry *entry,
 	mutex_unlock(&loopback->cable_lock);
 }
 
-static int loopback_proc_new(struct loopback *loopback, int cidx)
+static int loopback_cable_proc_new(struct loopback *loopback, int cidx)
 {
 	char name[32];
 
@@ -1687,6 +1687,36 @@ static void loopback_set_timer_source(struct loopback *loopback,
 						      value, GFP_KERNEL);
 }
 
+static void print_timer_source_info(struct snd_info_entry *entry,
+				    struct snd_info_buffer *buffer)
+{
+	struct loopback *loopback = entry->private_data;
+
+	mutex_lock(&loopback->cable_lock);
+	snd_iprintf(buffer, "%s\n",
+		    loopback->timer_source ? loopback->timer_source : "");
+	mutex_unlock(&loopback->cable_lock);
+}
+
+static void change_timer_source_info(struct snd_info_entry *entry,
+				     struct snd_info_buffer *buffer)
+{
+	struct loopback *loopback = entry->private_data;
+	char line[64];
+
+	mutex_lock(&loopback->cable_lock);
+	if (!snd_info_get_line(buffer, line, sizeof(line)))
+		loopback_set_timer_source(loopback, strim(line));
+	mutex_unlock(&loopback->cable_lock);
+}
+
+static int loopback_timer_source_proc_new(struct loopback *loopback)
+{
+	return snd_card_rw_proc_new(loopback->card, "timer_source", loopback,
+				    print_timer_source_info,
+				    change_timer_source_info);
+}
+
 static int loopback_probe(struct platform_device *devptr)
 {
 	struct snd_card *card;
@@ -1719,8 +1749,9 @@ static int loopback_probe(struct platform_device *devptr)
 	err = loopback_mixer_new(loopback, pcm_notify[dev] ? 1 : 0);
 	if (err < 0)
 		goto __nodev;
-	loopback_proc_new(loopback, 0);
-	loopback_proc_new(loopback, 1);
+	loopback_cable_proc_new(loopback, 0);
+	loopback_cable_proc_new(loopback, 1);
+	loopback_timer_source_proc_new(loopback);
 	strcpy(card->driver, "Loopback");
 	strcpy(card->shortname, "Loopback");
 	sprintf(card->longname, "Loopback %i", dev + 1);
-- 
2.21.0


^ permalink raw reply related	[flat|nested] 26+ messages in thread

* [alsa-devel] [PATCH v4 7/7] ALSA: aloop: Support runtime change of snd_timer via info interface
@ 2019-11-20 11:58               ` Andrew Gabbasov
  0 siblings, 0 replies; 26+ messages in thread
From: Andrew Gabbasov @ 2019-11-20 11:58 UTC (permalink / raw)
  To: alsa-devel, linux-kernel, Jaroslav Kysela, Takashi Iwai,
	Timo Wischer, Andrew Gabbasov

Show and change sound card timer source with read-write info
file in proc filesystem. Initial string can still be set as
module parameter.

The timer source string value can be changed at any time,
but it is latched by PCM substream open callback (the first one
for a particular cable). At this point it is actually used, that
is the string is parsed, and the timer is looked up and opened.

The timer source is set for a loopback card (the same as initial
setting by module parameter), but every cable uses the value,
current at the moment of open.

Setting the value to empty string switches the timer to jiffies.

Signed-off-by: Andrew Gabbasov <andrew_gabbasov@mentor.com>
---
 sound/drivers/aloop.c | 37 ++++++++++++++++++++++++++++++++++---
 1 file changed, 34 insertions(+), 3 deletions(-)

diff --git a/sound/drivers/aloop.c b/sound/drivers/aloop.c
index 67ebbd510e1b..626a8b642b63 100644
--- a/sound/drivers/aloop.c
+++ b/sound/drivers/aloop.c
@@ -1666,7 +1666,7 @@ static void print_cable_info(struct snd_info_entry *entry,
 	mutex_unlock(&loopback->cable_lock);
 }
 
-static int loopback_proc_new(struct loopback *loopback, int cidx)
+static int loopback_cable_proc_new(struct loopback *loopback, int cidx)
 {
 	char name[32];
 
@@ -1687,6 +1687,36 @@ static void loopback_set_timer_source(struct loopback *loopback,
 						      value, GFP_KERNEL);
 }
 
+static void print_timer_source_info(struct snd_info_entry *entry,
+				    struct snd_info_buffer *buffer)
+{
+	struct loopback *loopback = entry->private_data;
+
+	mutex_lock(&loopback->cable_lock);
+	snd_iprintf(buffer, "%s\n",
+		    loopback->timer_source ? loopback->timer_source : "");
+	mutex_unlock(&loopback->cable_lock);
+}
+
+static void change_timer_source_info(struct snd_info_entry *entry,
+				     struct snd_info_buffer *buffer)
+{
+	struct loopback *loopback = entry->private_data;
+	char line[64];
+
+	mutex_lock(&loopback->cable_lock);
+	if (!snd_info_get_line(buffer, line, sizeof(line)))
+		loopback_set_timer_source(loopback, strim(line));
+	mutex_unlock(&loopback->cable_lock);
+}
+
+static int loopback_timer_source_proc_new(struct loopback *loopback)
+{
+	return snd_card_rw_proc_new(loopback->card, "timer_source", loopback,
+				    print_timer_source_info,
+				    change_timer_source_info);
+}
+
 static int loopback_probe(struct platform_device *devptr)
 {
 	struct snd_card *card;
@@ -1719,8 +1749,9 @@ static int loopback_probe(struct platform_device *devptr)
 	err = loopback_mixer_new(loopback, pcm_notify[dev] ? 1 : 0);
 	if (err < 0)
 		goto __nodev;
-	loopback_proc_new(loopback, 0);
-	loopback_proc_new(loopback, 1);
+	loopback_cable_proc_new(loopback, 0);
+	loopback_cable_proc_new(loopback, 1);
+	loopback_timer_source_proc_new(loopback);
 	strcpy(card->driver, "Loopback");
 	strcpy(card->shortname, "Loopback");
 	sprintf(card->longname, "Loopback %i", dev + 1);
-- 
2.21.0

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

^ permalink raw reply related	[flat|nested] 26+ messages in thread

* Re: [PATCH v4 6/7] ALSA: aloop: Support selection of snd_timer instead of jiffies
  2019-11-20 11:58             ` [alsa-devel] " Andrew Gabbasov
  (?)
  (?)
@ 2019-11-20 14:33             ` Takashi Iwai
  2019-11-20 15:21                 ` [alsa-devel] " Andrew Gabbasov
  -1 siblings, 1 reply; 26+ messages in thread
From: Takashi Iwai @ 2019-11-20 14:33 UTC (permalink / raw)
  To: Andrew Gabbasov
  Cc: alsa-devel, linux-kernel, Jaroslav Kysela, Takashi Iwai, Timo Wischer

On Wed, 20 Nov 2019 12:58:55 +0100,
Andrew Gabbasov wrote:
> +/* call in loopback->cable_lock */
> +static int loopback_snd_timer_open(struct loopback_pcm *dpcm)
> +{
> +	int err = 0;
> +	struct snd_timer_id tid = {
> +		.dev_class = SNDRV_TIMER_CLASS_PCM,
> +		.dev_sclass = SNDRV_TIMER_SCLASS_APPLICATION,
> +	};
> +	struct snd_timer_instance *timeri;
> +	struct loopback_cable *cable = dpcm->cable;
> +
> +	spin_lock_irq(&cable->lock);
> +
> +	/* check if timer was already opened. It is only opened once
> +	 * per playback and capture subdevice (aka cable).
> +	 */
> +	if (cable->snd_timer.instance)
> +		goto unlock;
> +
> +	err = loopback_parse_timer_id(dpcm->loopback->timer_source, &tid);
> +	if (err < 0) {
> +		pcm_err(dpcm->substream->pcm,
> +			"Parsing timer source \'%s\' failed with %d",
> +			dpcm->loopback->timer_source, err);
> +		goto unlock;
> +	}
> +
> +	cable->snd_timer.stream = dpcm->substream->stream;
> +	cable->snd_timer.id = tid;
> +
> +	timeri = snd_timer_instance_new(dpcm->loopback->card->id);
> +	if (!timeri) {
> +		err = -ENOMEM;
> +		goto unlock;
> +	}
> +	/* The callback has to be called from another tasklet. If
> +	 * SNDRV_TIMER_IFLG_FAST is specified it will be called from the
> +	 * snd_pcm_period_elapsed() call of the selected sound card.
> +	 * snd_pcm_period_elapsed() helds snd_pcm_stream_lock_irqsave().
> +	 * Due to our callback loopback_snd_timer_function() also calls
> +	 * snd_pcm_period_elapsed() which calls snd_pcm_stream_lock_irqsave().
> +	 * This would end up in a dead lock.
> +	 */
> +	timeri->flags |= SNDRV_TIMER_IFLG_AUTO;
> +	timeri->callback = loopback_snd_timer_function;
> +	timeri->callback_data = (void *)cable;
> +	timeri->ccallback = loopback_snd_timer_event;
> +
> +	/* snd_timer_close() and snd_timer_open() should not be called with
> +	 * locked spinlock because both functions can block on a mutex. The
> +	 * mutex loopback->cable_lock is kept locked. Therefore snd_timer_open()
> +	 * cannot be called a second time by the other device of the same cable.
> +	 * Therefore the following issue cannot happen:
> +	 * [proc1] Call loopback_timer_open() ->
> +	 *	   Unlock cable->lock for snd_timer_close/open() call
> +	 * [proc2] Call loopback_timer_open() -> snd_timer_open(),
> +	 *	   snd_timer_start()
> +	 * [proc1] Call snd_timer_open() and overwrite running timer
> +	 *	   instance
> +	 */
> +	spin_unlock_irq(&cable->lock);
> +	err = snd_timer_open(timeri, &cable->snd_timer.id, current->pid);
> +	if (err < 0) {
> +		pcm_err(dpcm->substream->pcm,
> +			"snd_timer_open (%d,%d,%d) failed with %d",
> +			cable->snd_timer.id.card,
> +			cable->snd_timer.id.device,
> +			cable->snd_timer.id.subdevice,
> +			err);
> +		snd_timer_instance_free(timeri);
> +		return err;
> +	}
> +	spin_lock_irq(&cable->lock);
> +
> +	cable->snd_timer.instance = timeri;
> +
> +	/* initialise a tasklet used for draining */
> +	tasklet_init(&cable->snd_timer.event_tasklet,
> +		     loopback_snd_timer_tasklet, (unsigned long)timeri);

This has to be set before snd_timer_open().  The callback might be
called immediately after snd_timer_open().


thanks,

Takashi

^ permalink raw reply	[flat|nested] 26+ messages in thread

* RE: [PATCH v4 6/7] ALSA: aloop: Support selection of snd_timer instead of jiffies
  2019-11-20 14:33             ` [PATCH v4 6/7] ALSA: aloop: Support selection of snd_timer instead of jiffies Takashi Iwai
@ 2019-11-20 15:21                 ` Andrew Gabbasov
  0 siblings, 0 replies; 26+ messages in thread
From: Andrew Gabbasov @ 2019-11-20 15:21 UTC (permalink / raw)
  To: 'Takashi Iwai'
  Cc: alsa-devel, linux-kernel, Jaroslav Kysela, Takashi Iwai, Timo Wischer

Hello Takashi,

> -----Original Message-----
> From: Takashi Iwai [mailto:tiwai@suse.de]
> Sent: Wednesday, November 20, 2019 5:34 PM
> To: Gabbasov, Andrew
> Cc: alsa-devel@alsa-project.org; linux-kernel@vger.kernel.org; Jaroslav
> Kysela; Takashi Iwai; Timo Wischer
> Subject: Re: [PATCH v4 6/7] ALSA: aloop: Support selection of snd_timer
> instead of jiffies
> 
> On Wed, 20 Nov 2019 12:58:55 +0100,
> Andrew Gabbasov wrote:
> > +/* call in loopback->cable_lock */
> > +static int loopback_snd_timer_open(struct loopback_pcm *dpcm)
> > +{
> > +	int err = 0;
> > +	struct snd_timer_id tid = {
> > +		.dev_class = SNDRV_TIMER_CLASS_PCM,
> > +		.dev_sclass = SNDRV_TIMER_SCLASS_APPLICATION,
> > +	};
> > +	struct snd_timer_instance *timeri;
> > +	struct loopback_cable *cable = dpcm->cable;
> > +
> > +	spin_lock_irq(&cable->lock);
> > +
> > +	/* check if timer was already opened. It is only opened once
> > +	 * per playback and capture subdevice (aka cable).
> > +	 */
> > +	if (cable->snd_timer.instance)
> > +		goto unlock;
> > +
> > +	err = loopback_parse_timer_id(dpcm->loopback->timer_source, &tid);
> > +	if (err < 0) {
> > +		pcm_err(dpcm->substream->pcm,
> > +			"Parsing timer source \'%s\' failed with %d",
> > +			dpcm->loopback->timer_source, err);
> > +		goto unlock;
> > +	}
> > +
> > +	cable->snd_timer.stream = dpcm->substream->stream;
> > +	cable->snd_timer.id = tid;
> > +
> > +	timeri = snd_timer_instance_new(dpcm->loopback->card->id);
> > +	if (!timeri) {
> > +		err = -ENOMEM;
> > +		goto unlock;
> > +	}
> > +	/* The callback has to be called from another tasklet. If
> > +	 * SNDRV_TIMER_IFLG_FAST is specified it will be called from the
> > +	 * snd_pcm_period_elapsed() call of the selected sound card.
> > +	 * snd_pcm_period_elapsed() helds snd_pcm_stream_lock_irqsave().
> > +	 * Due to our callback loopback_snd_timer_function() also calls
> > +	 * snd_pcm_period_elapsed() which calls
> snd_pcm_stream_lock_irqsave().
> > +	 * This would end up in a dead lock.
> > +	 */
> > +	timeri->flags |= SNDRV_TIMER_IFLG_AUTO;
> > +	timeri->callback = loopback_snd_timer_function;
> > +	timeri->callback_data = (void *)cable;
> > +	timeri->ccallback = loopback_snd_timer_event;
> > +
> > +	/* snd_timer_close() and snd_timer_open() should not be called with
> > +	 * locked spinlock because both functions can block on a mutex. The
> > +	 * mutex loopback->cable_lock is kept locked. Therefore
> snd_timer_open()
> > +	 * cannot be called a second time by the other device of the same
> cable.
> > +	 * Therefore the following issue cannot happen:
> > +	 * [proc1] Call loopback_timer_open() ->
> > +	 *	   Unlock cable->lock for snd_timer_close/open() call
> > +	 * [proc2] Call loopback_timer_open() -> snd_timer_open(),
> > +	 *	   snd_timer_start()
> > +	 * [proc1] Call snd_timer_open() and overwrite running timer
> > +	 *	   instance
> > +	 */
> > +	spin_unlock_irq(&cable->lock);
> > +	err = snd_timer_open(timeri, &cable->snd_timer.id, current->pid);
> > +	if (err < 0) {
> > +		pcm_err(dpcm->substream->pcm,
> > +			"snd_timer_open (%d,%d,%d) failed with %d",
> > +			cable->snd_timer.id.card,
> > +			cable->snd_timer.id.device,
> > +			cable->snd_timer.id.subdevice,
> > +			err);
> > +		snd_timer_instance_free(timeri);
> > +		return err;
> > +	}
> > +	spin_lock_irq(&cable->lock);
> > +
> > +	cable->snd_timer.instance = timeri;
> > +
> > +	/* initialise a tasklet used for draining */
> > +	tasklet_init(&cable->snd_timer.event_tasklet,
> > +		     loopback_snd_timer_tasklet, (unsigned long)timeri);
> 
> This has to be set before snd_timer_open().  The callback might be
> called immediately after snd_timer_open().

This tasklet is used/scheduled only in ccallback (not regular tick
callback),
and only for SNDRV_TIMER_EVENT_MSTOP event. Can this event really happen
immediately after snd_timer_open()?

Thanks!

Best regards,
Andrew


^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [alsa-devel] [PATCH v4 6/7] ALSA: aloop: Support selection of snd_timer instead of jiffies
@ 2019-11-20 15:21                 ` Andrew Gabbasov
  0 siblings, 0 replies; 26+ messages in thread
From: Andrew Gabbasov @ 2019-11-20 15:21 UTC (permalink / raw)
  To: 'Takashi Iwai'
  Cc: Timo Wischer, alsa-devel, Takashi Iwai, linux-kernel

Hello Takashi,

> -----Original Message-----
> From: Takashi Iwai [mailto:tiwai@suse.de]
> Sent: Wednesday, November 20, 2019 5:34 PM
> To: Gabbasov, Andrew
> Cc: alsa-devel@alsa-project.org; linux-kernel@vger.kernel.org; Jaroslav
> Kysela; Takashi Iwai; Timo Wischer
> Subject: Re: [PATCH v4 6/7] ALSA: aloop: Support selection of snd_timer
> instead of jiffies
> 
> On Wed, 20 Nov 2019 12:58:55 +0100,
> Andrew Gabbasov wrote:
> > +/* call in loopback->cable_lock */
> > +static int loopback_snd_timer_open(struct loopback_pcm *dpcm)
> > +{
> > +	int err = 0;
> > +	struct snd_timer_id tid = {
> > +		.dev_class = SNDRV_TIMER_CLASS_PCM,
> > +		.dev_sclass = SNDRV_TIMER_SCLASS_APPLICATION,
> > +	};
> > +	struct snd_timer_instance *timeri;
> > +	struct loopback_cable *cable = dpcm->cable;
> > +
> > +	spin_lock_irq(&cable->lock);
> > +
> > +	/* check if timer was already opened. It is only opened once
> > +	 * per playback and capture subdevice (aka cable).
> > +	 */
> > +	if (cable->snd_timer.instance)
> > +		goto unlock;
> > +
> > +	err = loopback_parse_timer_id(dpcm->loopback->timer_source, &tid);
> > +	if (err < 0) {
> > +		pcm_err(dpcm->substream->pcm,
> > +			"Parsing timer source \'%s\' failed with %d",
> > +			dpcm->loopback->timer_source, err);
> > +		goto unlock;
> > +	}
> > +
> > +	cable->snd_timer.stream = dpcm->substream->stream;
> > +	cable->snd_timer.id = tid;
> > +
> > +	timeri = snd_timer_instance_new(dpcm->loopback->card->id);
> > +	if (!timeri) {
> > +		err = -ENOMEM;
> > +		goto unlock;
> > +	}
> > +	/* The callback has to be called from another tasklet. If
> > +	 * SNDRV_TIMER_IFLG_FAST is specified it will be called from the
> > +	 * snd_pcm_period_elapsed() call of the selected sound card.
> > +	 * snd_pcm_period_elapsed() helds snd_pcm_stream_lock_irqsave().
> > +	 * Due to our callback loopback_snd_timer_function() also calls
> > +	 * snd_pcm_period_elapsed() which calls
> snd_pcm_stream_lock_irqsave().
> > +	 * This would end up in a dead lock.
> > +	 */
> > +	timeri->flags |= SNDRV_TIMER_IFLG_AUTO;
> > +	timeri->callback = loopback_snd_timer_function;
> > +	timeri->callback_data = (void *)cable;
> > +	timeri->ccallback = loopback_snd_timer_event;
> > +
> > +	/* snd_timer_close() and snd_timer_open() should not be called with
> > +	 * locked spinlock because both functions can block on a mutex. The
> > +	 * mutex loopback->cable_lock is kept locked. Therefore
> snd_timer_open()
> > +	 * cannot be called a second time by the other device of the same
> cable.
> > +	 * Therefore the following issue cannot happen:
> > +	 * [proc1] Call loopback_timer_open() ->
> > +	 *	   Unlock cable->lock for snd_timer_close/open() call
> > +	 * [proc2] Call loopback_timer_open() -> snd_timer_open(),
> > +	 *	   snd_timer_start()
> > +	 * [proc1] Call snd_timer_open() and overwrite running timer
> > +	 *	   instance
> > +	 */
> > +	spin_unlock_irq(&cable->lock);
> > +	err = snd_timer_open(timeri, &cable->snd_timer.id, current->pid);
> > +	if (err < 0) {
> > +		pcm_err(dpcm->substream->pcm,
> > +			"snd_timer_open (%d,%d,%d) failed with %d",
> > +			cable->snd_timer.id.card,
> > +			cable->snd_timer.id.device,
> > +			cable->snd_timer.id.subdevice,
> > +			err);
> > +		snd_timer_instance_free(timeri);
> > +		return err;
> > +	}
> > +	spin_lock_irq(&cable->lock);
> > +
> > +	cable->snd_timer.instance = timeri;
> > +
> > +	/* initialise a tasklet used for draining */
> > +	tasklet_init(&cable->snd_timer.event_tasklet,
> > +		     loopback_snd_timer_tasklet, (unsigned long)timeri);
> 
> This has to be set before snd_timer_open().  The callback might be
> called immediately after snd_timer_open().

This tasklet is used/scheduled only in ccallback (not regular tick
callback),
and only for SNDRV_TIMER_EVENT_MSTOP event. Can this event really happen
immediately after snd_timer_open()?

Thanks!

Best regards,
Andrew

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH v4 6/7] ALSA: aloop: Support selection of snd_timer instead of jiffies
  2019-11-20 15:21                 ` [alsa-devel] " Andrew Gabbasov
  (?)
@ 2019-11-20 15:32                 ` Takashi Iwai
  2019-11-20 15:39                     ` [alsa-devel] " Andrew Gabbasov
  -1 siblings, 1 reply; 26+ messages in thread
From: Takashi Iwai @ 2019-11-20 15:32 UTC (permalink / raw)
  To: Andrew Gabbasov
  Cc: alsa-devel, linux-kernel, Jaroslav Kysela, Takashi Iwai, Timo Wischer

On Wed, 20 Nov 2019 16:21:36 +0100,
Andrew Gabbasov wrote:
> 
> Hello Takashi,
> 
> > -----Original Message-----
> > From: Takashi Iwai [mailto:tiwai@suse.de]
> > Sent: Wednesday, November 20, 2019 5:34 PM
> > To: Gabbasov, Andrew
> > Cc: alsa-devel@alsa-project.org; linux-kernel@vger.kernel.org; Jaroslav
> > Kysela; Takashi Iwai; Timo Wischer
> > Subject: Re: [PATCH v4 6/7] ALSA: aloop: Support selection of snd_timer
> > instead of jiffies
> > 
> > On Wed, 20 Nov 2019 12:58:55 +0100,
> > Andrew Gabbasov wrote:
> > > +/* call in loopback->cable_lock */
> > > +static int loopback_snd_timer_open(struct loopback_pcm *dpcm)
> > > +{
> > > +	int err = 0;
> > > +	struct snd_timer_id tid = {
> > > +		.dev_class = SNDRV_TIMER_CLASS_PCM,
> > > +		.dev_sclass = SNDRV_TIMER_SCLASS_APPLICATION,
> > > +	};
> > > +	struct snd_timer_instance *timeri;
> > > +	struct loopback_cable *cable = dpcm->cable;
> > > +
> > > +	spin_lock_irq(&cable->lock);
> > > +
> > > +	/* check if timer was already opened. It is only opened once
> > > +	 * per playback and capture subdevice (aka cable).
> > > +	 */
> > > +	if (cable->snd_timer.instance)
> > > +		goto unlock;
> > > +
> > > +	err = loopback_parse_timer_id(dpcm->loopback->timer_source, &tid);
> > > +	if (err < 0) {
> > > +		pcm_err(dpcm->substream->pcm,
> > > +			"Parsing timer source \'%s\' failed with %d",
> > > +			dpcm->loopback->timer_source, err);
> > > +		goto unlock;
> > > +	}
> > > +
> > > +	cable->snd_timer.stream = dpcm->substream->stream;
> > > +	cable->snd_timer.id = tid;
> > > +
> > > +	timeri = snd_timer_instance_new(dpcm->loopback->card->id);
> > > +	if (!timeri) {
> > > +		err = -ENOMEM;
> > > +		goto unlock;
> > > +	}
> > > +	/* The callback has to be called from another tasklet. If
> > > +	 * SNDRV_TIMER_IFLG_FAST is specified it will be called from the
> > > +	 * snd_pcm_period_elapsed() call of the selected sound card.
> > > +	 * snd_pcm_period_elapsed() helds snd_pcm_stream_lock_irqsave().
> > > +	 * Due to our callback loopback_snd_timer_function() also calls
> > > +	 * snd_pcm_period_elapsed() which calls
> > snd_pcm_stream_lock_irqsave().
> > > +	 * This would end up in a dead lock.
> > > +	 */
> > > +	timeri->flags |= SNDRV_TIMER_IFLG_AUTO;
> > > +	timeri->callback = loopback_snd_timer_function;
> > > +	timeri->callback_data = (void *)cable;
> > > +	timeri->ccallback = loopback_snd_timer_event;
> > > +
> > > +	/* snd_timer_close() and snd_timer_open() should not be called with
> > > +	 * locked spinlock because both functions can block on a mutex. The
> > > +	 * mutex loopback->cable_lock is kept locked. Therefore
> > snd_timer_open()
> > > +	 * cannot be called a second time by the other device of the same
> > cable.
> > > +	 * Therefore the following issue cannot happen:
> > > +	 * [proc1] Call loopback_timer_open() ->
> > > +	 *	   Unlock cable->lock for snd_timer_close/open() call
> > > +	 * [proc2] Call loopback_timer_open() -> snd_timer_open(),
> > > +	 *	   snd_timer_start()
> > > +	 * [proc1] Call snd_timer_open() and overwrite running timer
> > > +	 *	   instance
> > > +	 */
> > > +	spin_unlock_irq(&cable->lock);
> > > +	err = snd_timer_open(timeri, &cable->snd_timer.id, current->pid);
> > > +	if (err < 0) {
> > > +		pcm_err(dpcm->substream->pcm,
> > > +			"snd_timer_open (%d,%d,%d) failed with %d",
> > > +			cable->snd_timer.id.card,
> > > +			cable->snd_timer.id.device,
> > > +			cable->snd_timer.id.subdevice,
> > > +			err);
> > > +		snd_timer_instance_free(timeri);
> > > +		return err;
> > > +	}
> > > +	spin_lock_irq(&cable->lock);
> > > +
> > > +	cable->snd_timer.instance = timeri;
> > > +
> > > +	/* initialise a tasklet used for draining */
> > > +	tasklet_init(&cable->snd_timer.event_tasklet,
> > > +		     loopback_snd_timer_tasklet, (unsigned long)timeri);
> > 
> > This has to be set before snd_timer_open().  The callback might be
> > called immediately after snd_timer_open().
> 
> This tasklet is used/scheduled only in ccallback (not regular tick
> callback),
> and only for SNDRV_TIMER_EVENT_MSTOP event. Can this event really happen
> immediately after snd_timer_open()?

Why not?  The master timer can be stopped at any time, even between
these two lines.

Beware that there are fuzzer programs that can trigger such racy
things, and you're adding the code to the target that is actively
slapped by them :)


thanks,

Takashi

^ permalink raw reply	[flat|nested] 26+ messages in thread

* RE: [PATCH v4 6/7] ALSA: aloop: Support selection of snd_timer instead of jiffies
  2019-11-20 15:32                 ` Takashi Iwai
@ 2019-11-20 15:39                     ` Andrew Gabbasov
  0 siblings, 0 replies; 26+ messages in thread
From: Andrew Gabbasov @ 2019-11-20 15:39 UTC (permalink / raw)
  To: 'Takashi Iwai'
  Cc: alsa-devel, linux-kernel, Jaroslav Kysela, Takashi Iwai, Timo Wischer

Hello Takashi,

> -----Original Message-----
> From: Takashi Iwai [mailto:tiwai@suse.de]
> Sent: Wednesday, November 20, 2019 6:32 PM
> To: Gabbasov, Andrew
> Cc: alsa-devel@alsa-project.org; linux-kernel@vger.kernel.org; Jaroslav
> Kysela; Takashi Iwai; Timo Wischer
> Subject: Re: [PATCH v4 6/7] ALSA: aloop: Support selection of snd_timer
> instead of jiffies
> 
> On Wed, 20 Nov 2019 16:21:36 +0100,
> Andrew Gabbasov wrote:
> >
> > Hello Takashi,
> >
> > > -----Original Message-----
> > > From: Takashi Iwai [mailto:tiwai@suse.de]
> > > Sent: Wednesday, November 20, 2019 5:34 PM
> > > To: Gabbasov, Andrew
> > > Cc: alsa-devel@alsa-project.org; linux-kernel@vger.kernel.org;
Jaroslav
> > > Kysela; Takashi Iwai; Timo Wischer
> > > Subject: Re: [PATCH v4 6/7] ALSA: aloop: Support selection of
snd_timer
> > > instead of jiffies
> > >
> > > On Wed, 20 Nov 2019 12:58:55 +0100,
> > > Andrew Gabbasov wrote:
> > > > +/* call in loopback->cable_lock */
> > > > +static int loopback_snd_timer_open(struct loopback_pcm *dpcm)
> > > > +{
> > > > +	int err = 0;
> > > > +	struct snd_timer_id tid = {
> > > > +		.dev_class = SNDRV_TIMER_CLASS_PCM,
> > > > +		.dev_sclass = SNDRV_TIMER_SCLASS_APPLICATION,
> > > > +	};
> > > > +	struct snd_timer_instance *timeri;
> > > > +	struct loopback_cable *cable = dpcm->cable;
> > > > +
> > > > +	spin_lock_irq(&cable->lock);
> > > > +
> > > > +	/* check if timer was already opened. It is only opened once
> > > > +	 * per playback and capture subdevice (aka cable).
> > > > +	 */
> > > > +	if (cable->snd_timer.instance)
> > > > +		goto unlock;
> > > > +
> > > > +	err = loopback_parse_timer_id(dpcm->loopback->timer_source,
> &tid);
> > > > +	if (err < 0) {
> > > > +		pcm_err(dpcm->substream->pcm,
> > > > +			"Parsing timer source \'%s\' failed with
%d",
> > > > +			dpcm->loopback->timer_source, err);
> > > > +		goto unlock;
> > > > +	}
> > > > +
> > > > +	cable->snd_timer.stream = dpcm->substream->stream;
> > > > +	cable->snd_timer.id = tid;
> > > > +
> > > > +	timeri = snd_timer_instance_new(dpcm->loopback->card->id);
> > > > +	if (!timeri) {
> > > > +		err = -ENOMEM;
> > > > +		goto unlock;
> > > > +	}
> > > > +	/* The callback has to be called from another tasklet. If
> > > > +	 * SNDRV_TIMER_IFLG_FAST is specified it will be called from
> the
> > > > +	 * snd_pcm_period_elapsed() call of the selected sound card.
> > > > +	 * snd_pcm_period_elapsed() helds
> snd_pcm_stream_lock_irqsave().
> > > > +	 * Due to our callback loopback_snd_timer_function() also
> calls
> > > > +	 * snd_pcm_period_elapsed() which calls
> > > snd_pcm_stream_lock_irqsave().
> > > > +	 * This would end up in a dead lock.
> > > > +	 */
> > > > +	timeri->flags |= SNDRV_TIMER_IFLG_AUTO;
> > > > +	timeri->callback = loopback_snd_timer_function;
> > > > +	timeri->callback_data = (void *)cable;
> > > > +	timeri->ccallback = loopback_snd_timer_event;
> > > > +
> > > > +	/* snd_timer_close() and snd_timer_open() should not be
called
> with
> > > > +	 * locked spinlock because both functions can block on a
> mutex. The
> > > > +	 * mutex loopback->cable_lock is kept locked. Therefore
> > > snd_timer_open()
> > > > +	 * cannot be called a second time by the other device of the
> same
> > > cable.
> > > > +	 * Therefore the following issue cannot happen:
> > > > +	 * [proc1] Call loopback_timer_open() ->
> > > > +	 *	   Unlock cable->lock for snd_timer_close/open()
call
> > > > +	 * [proc2] Call loopback_timer_open() -> snd_timer_open(),
> > > > +	 *	   snd_timer_start()
> > > > +	 * [proc1] Call snd_timer_open() and overwrite running timer
> > > > +	 *	   instance
> > > > +	 */
> > > > +	spin_unlock_irq(&cable->lock);
> > > > +	err = snd_timer_open(timeri, &cable->snd_timer.id, current-
> >pid);
> > > > +	if (err < 0) {
> > > > +		pcm_err(dpcm->substream->pcm,
> > > > +			"snd_timer_open (%d,%d,%d) failed with %d",
> > > > +			cable->snd_timer.id.card,
> > > > +			cable->snd_timer.id.device,
> > > > +			cable->snd_timer.id.subdevice,
> > > > +			err);
> > > > +		snd_timer_instance_free(timeri);
> > > > +		return err;
> > > > +	}
> > > > +	spin_lock_irq(&cable->lock);
> > > > +
> > > > +	cable->snd_timer.instance = timeri;
> > > > +
> > > > +	/* initialise a tasklet used for draining */
> > > > +	tasklet_init(&cable->snd_timer.event_tasklet,
> > > > +		     loopback_snd_timer_tasklet, (unsigned
> long)timeri);
> > >
> > > This has to be set before snd_timer_open().  The callback might be
> > > called immediately after snd_timer_open().
> >
> > This tasklet is used/scheduled only in ccallback (not regular tick
> > callback),
> > and only for SNDRV_TIMER_EVENT_MSTOP event. Can this event really happen
> > immediately after snd_timer_open()?
> 
> Why not?  The master timer can be stopped at any time, even between
> these two lines.
> 
> Beware that there are fuzzer programs that can trigger such racy
> things, and you're adding the code to the target that is actively
> slapped by them :)

OK, got it.
I'll move this initialization to before snd_timer_open() in the next
update together with the fixes for the other issues you will find
in this version.

Thanks!

Best regards,
Andrew


^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [alsa-devel] [PATCH v4 6/7] ALSA: aloop: Support selection of snd_timer instead of jiffies
@ 2019-11-20 15:39                     ` Andrew Gabbasov
  0 siblings, 0 replies; 26+ messages in thread
From: Andrew Gabbasov @ 2019-11-20 15:39 UTC (permalink / raw)
  To: 'Takashi Iwai'
  Cc: Timo Wischer, alsa-devel, Takashi Iwai, linux-kernel

Hello Takashi,

> -----Original Message-----
> From: Takashi Iwai [mailto:tiwai@suse.de]
> Sent: Wednesday, November 20, 2019 6:32 PM
> To: Gabbasov, Andrew
> Cc: alsa-devel@alsa-project.org; linux-kernel@vger.kernel.org; Jaroslav
> Kysela; Takashi Iwai; Timo Wischer
> Subject: Re: [PATCH v4 6/7] ALSA: aloop: Support selection of snd_timer
> instead of jiffies
> 
> On Wed, 20 Nov 2019 16:21:36 +0100,
> Andrew Gabbasov wrote:
> >
> > Hello Takashi,
> >
> > > -----Original Message-----
> > > From: Takashi Iwai [mailto:tiwai@suse.de]
> > > Sent: Wednesday, November 20, 2019 5:34 PM
> > > To: Gabbasov, Andrew
> > > Cc: alsa-devel@alsa-project.org; linux-kernel@vger.kernel.org;
Jaroslav
> > > Kysela; Takashi Iwai; Timo Wischer
> > > Subject: Re: [PATCH v4 6/7] ALSA: aloop: Support selection of
snd_timer
> > > instead of jiffies
> > >
> > > On Wed, 20 Nov 2019 12:58:55 +0100,
> > > Andrew Gabbasov wrote:
> > > > +/* call in loopback->cable_lock */
> > > > +static int loopback_snd_timer_open(struct loopback_pcm *dpcm)
> > > > +{
> > > > +	int err = 0;
> > > > +	struct snd_timer_id tid = {
> > > > +		.dev_class = SNDRV_TIMER_CLASS_PCM,
> > > > +		.dev_sclass = SNDRV_TIMER_SCLASS_APPLICATION,
> > > > +	};
> > > > +	struct snd_timer_instance *timeri;
> > > > +	struct loopback_cable *cable = dpcm->cable;
> > > > +
> > > > +	spin_lock_irq(&cable->lock);
> > > > +
> > > > +	/* check if timer was already opened. It is only opened once
> > > > +	 * per playback and capture subdevice (aka cable).
> > > > +	 */
> > > > +	if (cable->snd_timer.instance)
> > > > +		goto unlock;
> > > > +
> > > > +	err = loopback_parse_timer_id(dpcm->loopback->timer_source,
> &tid);
> > > > +	if (err < 0) {
> > > > +		pcm_err(dpcm->substream->pcm,
> > > > +			"Parsing timer source \'%s\' failed with
%d",
> > > > +			dpcm->loopback->timer_source, err);
> > > > +		goto unlock;
> > > > +	}
> > > > +
> > > > +	cable->snd_timer.stream = dpcm->substream->stream;
> > > > +	cable->snd_timer.id = tid;
> > > > +
> > > > +	timeri = snd_timer_instance_new(dpcm->loopback->card->id);
> > > > +	if (!timeri) {
> > > > +		err = -ENOMEM;
> > > > +		goto unlock;
> > > > +	}
> > > > +	/* The callback has to be called from another tasklet. If
> > > > +	 * SNDRV_TIMER_IFLG_FAST is specified it will be called from
> the
> > > > +	 * snd_pcm_period_elapsed() call of the selected sound card.
> > > > +	 * snd_pcm_period_elapsed() helds
> snd_pcm_stream_lock_irqsave().
> > > > +	 * Due to our callback loopback_snd_timer_function() also
> calls
> > > > +	 * snd_pcm_period_elapsed() which calls
> > > snd_pcm_stream_lock_irqsave().
> > > > +	 * This would end up in a dead lock.
> > > > +	 */
> > > > +	timeri->flags |= SNDRV_TIMER_IFLG_AUTO;
> > > > +	timeri->callback = loopback_snd_timer_function;
> > > > +	timeri->callback_data = (void *)cable;
> > > > +	timeri->ccallback = loopback_snd_timer_event;
> > > > +
> > > > +	/* snd_timer_close() and snd_timer_open() should not be
called
> with
> > > > +	 * locked spinlock because both functions can block on a
> mutex. The
> > > > +	 * mutex loopback->cable_lock is kept locked. Therefore
> > > snd_timer_open()
> > > > +	 * cannot be called a second time by the other device of the
> same
> > > cable.
> > > > +	 * Therefore the following issue cannot happen:
> > > > +	 * [proc1] Call loopback_timer_open() ->
> > > > +	 *	   Unlock cable->lock for snd_timer_close/open()
call
> > > > +	 * [proc2] Call loopback_timer_open() -> snd_timer_open(),
> > > > +	 *	   snd_timer_start()
> > > > +	 * [proc1] Call snd_timer_open() and overwrite running timer
> > > > +	 *	   instance
> > > > +	 */
> > > > +	spin_unlock_irq(&cable->lock);
> > > > +	err = snd_timer_open(timeri, &cable->snd_timer.id, current-
> >pid);
> > > > +	if (err < 0) {
> > > > +		pcm_err(dpcm->substream->pcm,
> > > > +			"snd_timer_open (%d,%d,%d) failed with %d",
> > > > +			cable->snd_timer.id.card,
> > > > +			cable->snd_timer.id.device,
> > > > +			cable->snd_timer.id.subdevice,
> > > > +			err);
> > > > +		snd_timer_instance_free(timeri);
> > > > +		return err;
> > > > +	}
> > > > +	spin_lock_irq(&cable->lock);
> > > > +
> > > > +	cable->snd_timer.instance = timeri;
> > > > +
> > > > +	/* initialise a tasklet used for draining */
> > > > +	tasklet_init(&cable->snd_timer.event_tasklet,
> > > > +		     loopback_snd_timer_tasklet, (unsigned
> long)timeri);
> > >
> > > This has to be set before snd_timer_open().  The callback might be
> > > called immediately after snd_timer_open().
> >
> > This tasklet is used/scheduled only in ccallback (not regular tick
> > callback),
> > and only for SNDRV_TIMER_EVENT_MSTOP event. Can this event really happen
> > immediately after snd_timer_open()?
> 
> Why not?  The master timer can be stopped at any time, even between
> these two lines.
> 
> Beware that there are fuzzer programs that can trigger such racy
> things, and you're adding the code to the target that is actively
> slapped by them :)

OK, got it.
I'll move this initialization to before snd_timer_open() in the next
update together with the fixes for the other issues you will find
in this version.

Thanks!

Best regards,
Andrew

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH v4 6/7] ALSA: aloop: Support selection of snd_timer instead of jiffies
  2019-11-20 15:39                     ` [alsa-devel] " Andrew Gabbasov
  (?)
@ 2019-11-20 15:58                     ` Takashi Iwai
  2019-11-20 17:56                         ` [alsa-devel] " Andrew Gabbasov
  -1 siblings, 1 reply; 26+ messages in thread
From: Takashi Iwai @ 2019-11-20 15:58 UTC (permalink / raw)
  To: Andrew Gabbasov
  Cc: alsa-devel, linux-kernel, Jaroslav Kysela, Takashi Iwai, Timo Wischer

On Wed, 20 Nov 2019 16:39:00 +0100,
Andrew Gabbasov wrote:
> 
> Hello Takashi,
> 
> > -----Original Message-----
> > From: Takashi Iwai [mailto:tiwai@suse.de]
> > Sent: Wednesday, November 20, 2019 6:32 PM
> > To: Gabbasov, Andrew
> > Cc: alsa-devel@alsa-project.org; linux-kernel@vger.kernel.org; Jaroslav
> > Kysela; Takashi Iwai; Timo Wischer
> > Subject: Re: [PATCH v4 6/7] ALSA: aloop: Support selection of snd_timer
> > instead of jiffies
> > 
> > On Wed, 20 Nov 2019 16:21:36 +0100,
> > Andrew Gabbasov wrote:
> > >
> > > Hello Takashi,
> > >
> > > > -----Original Message-----
> > > > From: Takashi Iwai [mailto:tiwai@suse.de]
> > > > Sent: Wednesday, November 20, 2019 5:34 PM
> > > > To: Gabbasov, Andrew
> > > > Cc: alsa-devel@alsa-project.org; linux-kernel@vger.kernel.org;
> Jaroslav
> > > > Kysela; Takashi Iwai; Timo Wischer
> > > > Subject: Re: [PATCH v4 6/7] ALSA: aloop: Support selection of
> snd_timer
> > > > instead of jiffies
> > > >
> > > > On Wed, 20 Nov 2019 12:58:55 +0100,
> > > > Andrew Gabbasov wrote:
> > > > > +/* call in loopback->cable_lock */
> > > > > +static int loopback_snd_timer_open(struct loopback_pcm *dpcm)
> > > > > +{
> > > > > +	int err = 0;
> > > > > +	struct snd_timer_id tid = {
> > > > > +		.dev_class = SNDRV_TIMER_CLASS_PCM,
> > > > > +		.dev_sclass = SNDRV_TIMER_SCLASS_APPLICATION,
> > > > > +	};
> > > > > +	struct snd_timer_instance *timeri;
> > > > > +	struct loopback_cable *cable = dpcm->cable;
> > > > > +
> > > > > +	spin_lock_irq(&cable->lock);
> > > > > +
> > > > > +	/* check if timer was already opened. It is only opened once
> > > > > +	 * per playback and capture subdevice (aka cable).
> > > > > +	 */
> > > > > +	if (cable->snd_timer.instance)
> > > > > +		goto unlock;
> > > > > +
> > > > > +	err = loopback_parse_timer_id(dpcm->loopback->timer_source,
> > &tid);
> > > > > +	if (err < 0) {
> > > > > +		pcm_err(dpcm->substream->pcm,
> > > > > +			"Parsing timer source \'%s\' failed with
> %d",
> > > > > +			dpcm->loopback->timer_source, err);
> > > > > +		goto unlock;
> > > > > +	}
> > > > > +
> > > > > +	cable->snd_timer.stream = dpcm->substream->stream;
> > > > > +	cable->snd_timer.id = tid;
> > > > > +
> > > > > +	timeri = snd_timer_instance_new(dpcm->loopback->card->id);
> > > > > +	if (!timeri) {
> > > > > +		err = -ENOMEM;
> > > > > +		goto unlock;
> > > > > +	}
> > > > > +	/* The callback has to be called from another tasklet. If
> > > > > +	 * SNDRV_TIMER_IFLG_FAST is specified it will be called from
> > the
> > > > > +	 * snd_pcm_period_elapsed() call of the selected sound card.
> > > > > +	 * snd_pcm_period_elapsed() helds
> > snd_pcm_stream_lock_irqsave().
> > > > > +	 * Due to our callback loopback_snd_timer_function() also
> > calls
> > > > > +	 * snd_pcm_period_elapsed() which calls
> > > > snd_pcm_stream_lock_irqsave().
> > > > > +	 * This would end up in a dead lock.
> > > > > +	 */
> > > > > +	timeri->flags |= SNDRV_TIMER_IFLG_AUTO;
> > > > > +	timeri->callback = loopback_snd_timer_function;
> > > > > +	timeri->callback_data = (void *)cable;
> > > > > +	timeri->ccallback = loopback_snd_timer_event;
> > > > > +
> > > > > +	/* snd_timer_close() and snd_timer_open() should not be
> called
> > with
> > > > > +	 * locked spinlock because both functions can block on a
> > mutex. The
> > > > > +	 * mutex loopback->cable_lock is kept locked. Therefore
> > > > snd_timer_open()
> > > > > +	 * cannot be called a second time by the other device of the
> > same
> > > > cable.
> > > > > +	 * Therefore the following issue cannot happen:
> > > > > +	 * [proc1] Call loopback_timer_open() ->
> > > > > +	 *	   Unlock cable->lock for snd_timer_close/open()
> call
> > > > > +	 * [proc2] Call loopback_timer_open() -> snd_timer_open(),
> > > > > +	 *	   snd_timer_start()
> > > > > +	 * [proc1] Call snd_timer_open() and overwrite running timer
> > > > > +	 *	   instance
> > > > > +	 */
> > > > > +	spin_unlock_irq(&cable->lock);
> > > > > +	err = snd_timer_open(timeri, &cable->snd_timer.id, current-
> > >pid);
> > > > > +	if (err < 0) {
> > > > > +		pcm_err(dpcm->substream->pcm,
> > > > > +			"snd_timer_open (%d,%d,%d) failed with %d",
> > > > > +			cable->snd_timer.id.card,
> > > > > +			cable->snd_timer.id.device,
> > > > > +			cable->snd_timer.id.subdevice,
> > > > > +			err);
> > > > > +		snd_timer_instance_free(timeri);
> > > > > +		return err;
> > > > > +	}
> > > > > +	spin_lock_irq(&cable->lock);
> > > > > +
> > > > > +	cable->snd_timer.instance = timeri;
> > > > > +
> > > > > +	/* initialise a tasklet used for draining */
> > > > > +	tasklet_init(&cable->snd_timer.event_tasklet,
> > > > > +		     loopback_snd_timer_tasklet, (unsigned
> > long)timeri);
> > > >
> > > > This has to be set before snd_timer_open().  The callback might be
> > > > called immediately after snd_timer_open().
> > >
> > > This tasklet is used/scheduled only in ccallback (not regular tick
> > > callback),
> > > and only for SNDRV_TIMER_EVENT_MSTOP event. Can this event really happen
> > > immediately after snd_timer_open()?
> > 
> > Why not?  The master timer can be stopped at any time, even between
> > these two lines.
> > 
> > Beware that there are fuzzer programs that can trigger such racy
> > things, and you're adding the code to the target that is actively
> > slapped by them :)
> 
> OK, got it.
> I'll move this initialization to before snd_timer_open() in the next
> update together with the fixes for the other issues you will find
> in this version.

I have no other issues, so you can just resubmit only that patch,
too.


thanks,

Takashi

^ permalink raw reply	[flat|nested] 26+ messages in thread

* RE: [PATCH v4 6/7] ALSA: aloop: Support selection of snd_timer instead of jiffies
  2019-11-20 15:58                     ` Takashi Iwai
@ 2019-11-20 17:56                         ` Andrew Gabbasov
  0 siblings, 0 replies; 26+ messages in thread
From: Andrew Gabbasov @ 2019-11-20 17:56 UTC (permalink / raw)
  To: 'Takashi Iwai'
  Cc: alsa-devel, linux-kernel, Jaroslav Kysela, Takashi Iwai, Timo Wischer

Hello Takashi,

> -----Original Message-----
> From: Takashi Iwai [mailto:tiwai@suse.de]
> Sent: Wednesday, November 20, 2019 6:59 PM
> To: Gabbasov, Andrew
> Cc: alsa-devel@alsa-project.org; linux-kernel@vger.kernel.org; Jaroslav
> Kysela; Takashi Iwai; Timo Wischer
> Subject: Re: [PATCH v4 6/7] ALSA: aloop: Support selection of snd_timer
> instead of jiffies
> 
> On Wed, 20 Nov 2019 16:39:00 +0100,
> Andrew Gabbasov wrote:
> >
> > Hello Takashi,
> >
> > > -----Original Message-----
> > > From: Takashi Iwai [mailto:tiwai@suse.de]
> > > Sent: Wednesday, November 20, 2019 6:32 PM
> > > To: Gabbasov, Andrew
> > > Cc: alsa-devel@alsa-project.org; linux-kernel@vger.kernel.org;
Jaroslav
> > > Kysela; Takashi Iwai; Timo Wischer
> > > Subject: Re: [PATCH v4 6/7] ALSA: aloop: Support selection of
snd_timer
> > > instead of jiffies
> > >
> > > On Wed, 20 Nov 2019 16:21:36 +0100,
> > > Andrew Gabbasov wrote:
> > > >
> > > > Hello Takashi,
> > > >
> > > > > -----Original Message-----
> > > > > From: Takashi Iwai [mailto:tiwai@suse.de]
> > > > > Sent: Wednesday, November 20, 2019 5:34 PM
> > > > > To: Gabbasov, Andrew
> > > > > Cc: alsa-devel@alsa-project.org; linux-kernel@vger.kernel.org;
> > Jaroslav
> > > > > Kysela; Takashi Iwai; Timo Wischer
> > > > > Subject: Re: [PATCH v4 6/7] ALSA: aloop: Support selection of
> > snd_timer
> > > > > instead of jiffies
> > > > >
> > > > > On Wed, 20 Nov 2019 12:58:55 +0100,
> > > > > Andrew Gabbasov wrote:
> > > > > > +/* call in loopback->cable_lock */
> > > > > > +static int loopback_snd_timer_open(struct loopback_pcm *dpcm)
> > > > > > +{
> > > > > > +	int err = 0;
> > > > > > +	struct snd_timer_id tid = {
> > > > > > +		.dev_class = SNDRV_TIMER_CLASS_PCM,
> > > > > > +		.dev_sclass = SNDRV_TIMER_SCLASS_APPLICATION,
> > > > > > +	};
> > > > > > +	struct snd_timer_instance *timeri;
> > > > > > +	struct loopback_cable *cable = dpcm->cable;
> > > > > > +
> > > > > > +	spin_lock_irq(&cable->lock);
> > > > > > +
> > > > > > +	/* check if timer was already opened. It is only opened once
> > > > > > +	 * per playback and capture subdevice (aka cable).
> > > > > > +	 */
> > > > > > +	if (cable->snd_timer.instance)
> > > > > > +		goto unlock;
> > > > > > +
> > > > > > +	err = loopback_parse_timer_id(dpcm->loopback->timer_source,
> > > &tid);
> > > > > > +	if (err < 0) {
> > > > > > +		pcm_err(dpcm->substream->pcm,
> > > > > > +			"Parsing timer source \'%s\' failed with
> > %d",
> > > > > > +			dpcm->loopback->timer_source, err);
> > > > > > +		goto unlock;
> > > > > > +	}
> > > > > > +
> > > > > > +	cable->snd_timer.stream = dpcm->substream->stream;
> > > > > > +	cable->snd_timer.id = tid;
> > > > > > +
> > > > > > +	timeri = snd_timer_instance_new(dpcm->loopback->card->id);
> > > > > > +	if (!timeri) {
> > > > > > +		err = -ENOMEM;
> > > > > > +		goto unlock;
> > > > > > +	}
> > > > > > +	/* The callback has to be called from another tasklet. If
> > > > > > +	 * SNDRV_TIMER_IFLG_FAST is specified it will be called from
> > > the
> > > > > > +	 * snd_pcm_period_elapsed() call of the selected sound card.
> > > > > > +	 * snd_pcm_period_elapsed() helds
> > > snd_pcm_stream_lock_irqsave().
> > > > > > +	 * Due to our callback loopback_snd_timer_function() also
> > > calls
> > > > > > +	 * snd_pcm_period_elapsed() which calls
> > > > > snd_pcm_stream_lock_irqsave().
> > > > > > +	 * This would end up in a dead lock.
> > > > > > +	 */
> > > > > > +	timeri->flags |= SNDRV_TIMER_IFLG_AUTO;
> > > > > > +	timeri->callback = loopback_snd_timer_function;
> > > > > > +	timeri->callback_data = (void *)cable;
> > > > > > +	timeri->ccallback = loopback_snd_timer_event;
> > > > > > +
> > > > > > +	/* snd_timer_close() and snd_timer_open() should not be
> > called
> > > with
> > > > > > +	 * locked spinlock because both functions can block on a
> > > mutex. The
> > > > > > +	 * mutex loopback->cable_lock is kept locked. Therefore
> > > > > snd_timer_open()
> > > > > > +	 * cannot be called a second time by the other device of the
> > > same
> > > > > cable.
> > > > > > +	 * Therefore the following issue cannot happen:
> > > > > > +	 * [proc1] Call loopback_timer_open() ->
> > > > > > +	 *	   Unlock cable->lock for snd_timer_close/open()
> > call
> > > > > > +	 * [proc2] Call loopback_timer_open() -> snd_timer_open(),
> > > > > > +	 *	   snd_timer_start()
> > > > > > +	 * [proc1] Call snd_timer_open() and overwrite running timer
> > > > > > +	 *	   instance
> > > > > > +	 */
> > > > > > +	spin_unlock_irq(&cable->lock);
> > > > > > +	err = snd_timer_open(timeri, &cable->snd_timer.id, current-
> > > >pid);
> > > > > > +	if (err < 0) {
> > > > > > +		pcm_err(dpcm->substream->pcm,
> > > > > > +			"snd_timer_open (%d,%d,%d) failed with %d",
> > > > > > +			cable->snd_timer.id.card,
> > > > > > +			cable->snd_timer.id.device,
> > > > > > +			cable->snd_timer.id.subdevice,
> > > > > > +			err);
> > > > > > +		snd_timer_instance_free(timeri);
> > > > > > +		return err;
> > > > > > +	}
> > > > > > +	spin_lock_irq(&cable->lock);
> > > > > > +
> > > > > > +	cable->snd_timer.instance = timeri;
> > > > > > +
> > > > > > +	/* initialise a tasklet used for draining */
> > > > > > +	tasklet_init(&cable->snd_timer.event_tasklet,
> > > > > > +		     loopback_snd_timer_tasklet, (unsigned
> > > long)timeri);
> > > > >
> > > > > This has to be set before snd_timer_open().  The callback might be
> > > > > called immediately after snd_timer_open().
> > > >
> > > > This tasklet is used/scheduled only in ccallback (not regular tick
> > > > callback),
> > > > and only for SNDRV_TIMER_EVENT_MSTOP event. Can this event really
> happen
> > > > immediately after snd_timer_open()?
> > >
> > > Why not?  The master timer can be stopped at any time, even between
> > > these two lines.
> > >
> > > Beware that there are fuzzer programs that can trigger such racy
> > > things, and you're adding the code to the target that is actively
> > > slapped by them :)
> >
> > OK, got it.
> > I'll move this initialization to before snd_timer_open() in the next
> > update together with the fixes for the other issues you will find
> > in this version.
> 
> I have no other issues, so you can just resubmit only that patch,
> too.

I'm not sure how to correctly format resubmitting of a single patch from
a patch set, so I'm submitting the next version v5 of the whole patch set:
https://mailman.alsa-project.org/pipermail/alsa-devel/2019-November/158939.h
tml

Thanks!

Best regards,
Andrew


^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [alsa-devel] [PATCH v4 6/7] ALSA: aloop: Support selection of snd_timer instead of jiffies
@ 2019-11-20 17:56                         ` Andrew Gabbasov
  0 siblings, 0 replies; 26+ messages in thread
From: Andrew Gabbasov @ 2019-11-20 17:56 UTC (permalink / raw)
  To: 'Takashi Iwai'
  Cc: Timo Wischer, alsa-devel, Takashi Iwai, linux-kernel

Hello Takashi,

> -----Original Message-----
> From: Takashi Iwai [mailto:tiwai@suse.de]
> Sent: Wednesday, November 20, 2019 6:59 PM
> To: Gabbasov, Andrew
> Cc: alsa-devel@alsa-project.org; linux-kernel@vger.kernel.org; Jaroslav
> Kysela; Takashi Iwai; Timo Wischer
> Subject: Re: [PATCH v4 6/7] ALSA: aloop: Support selection of snd_timer
> instead of jiffies
> 
> On Wed, 20 Nov 2019 16:39:00 +0100,
> Andrew Gabbasov wrote:
> >
> > Hello Takashi,
> >
> > > -----Original Message-----
> > > From: Takashi Iwai [mailto:tiwai@suse.de]
> > > Sent: Wednesday, November 20, 2019 6:32 PM
> > > To: Gabbasov, Andrew
> > > Cc: alsa-devel@alsa-project.org; linux-kernel@vger.kernel.org;
Jaroslav
> > > Kysela; Takashi Iwai; Timo Wischer
> > > Subject: Re: [PATCH v4 6/7] ALSA: aloop: Support selection of
snd_timer
> > > instead of jiffies
> > >
> > > On Wed, 20 Nov 2019 16:21:36 +0100,
> > > Andrew Gabbasov wrote:
> > > >
> > > > Hello Takashi,
> > > >
> > > > > -----Original Message-----
> > > > > From: Takashi Iwai [mailto:tiwai@suse.de]
> > > > > Sent: Wednesday, November 20, 2019 5:34 PM
> > > > > To: Gabbasov, Andrew
> > > > > Cc: alsa-devel@alsa-project.org; linux-kernel@vger.kernel.org;
> > Jaroslav
> > > > > Kysela; Takashi Iwai; Timo Wischer
> > > > > Subject: Re: [PATCH v4 6/7] ALSA: aloop: Support selection of
> > snd_timer
> > > > > instead of jiffies
> > > > >
> > > > > On Wed, 20 Nov 2019 12:58:55 +0100,
> > > > > Andrew Gabbasov wrote:
> > > > > > +/* call in loopback->cable_lock */
> > > > > > +static int loopback_snd_timer_open(struct loopback_pcm *dpcm)
> > > > > > +{
> > > > > > +	int err = 0;
> > > > > > +	struct snd_timer_id tid = {
> > > > > > +		.dev_class = SNDRV_TIMER_CLASS_PCM,
> > > > > > +		.dev_sclass = SNDRV_TIMER_SCLASS_APPLICATION,
> > > > > > +	};
> > > > > > +	struct snd_timer_instance *timeri;
> > > > > > +	struct loopback_cable *cable = dpcm->cable;
> > > > > > +
> > > > > > +	spin_lock_irq(&cable->lock);
> > > > > > +
> > > > > > +	/* check if timer was already opened. It is only opened once
> > > > > > +	 * per playback and capture subdevice (aka cable).
> > > > > > +	 */
> > > > > > +	if (cable->snd_timer.instance)
> > > > > > +		goto unlock;
> > > > > > +
> > > > > > +	err = loopback_parse_timer_id(dpcm->loopback->timer_source,
> > > &tid);
> > > > > > +	if (err < 0) {
> > > > > > +		pcm_err(dpcm->substream->pcm,
> > > > > > +			"Parsing timer source \'%s\' failed with
> > %d",
> > > > > > +			dpcm->loopback->timer_source, err);
> > > > > > +		goto unlock;
> > > > > > +	}
> > > > > > +
> > > > > > +	cable->snd_timer.stream = dpcm->substream->stream;
> > > > > > +	cable->snd_timer.id = tid;
> > > > > > +
> > > > > > +	timeri = snd_timer_instance_new(dpcm->loopback->card->id);
> > > > > > +	if (!timeri) {
> > > > > > +		err = -ENOMEM;
> > > > > > +		goto unlock;
> > > > > > +	}
> > > > > > +	/* The callback has to be called from another tasklet. If
> > > > > > +	 * SNDRV_TIMER_IFLG_FAST is specified it will be called from
> > > the
> > > > > > +	 * snd_pcm_period_elapsed() call of the selected sound card.
> > > > > > +	 * snd_pcm_period_elapsed() helds
> > > snd_pcm_stream_lock_irqsave().
> > > > > > +	 * Due to our callback loopback_snd_timer_function() also
> > > calls
> > > > > > +	 * snd_pcm_period_elapsed() which calls
> > > > > snd_pcm_stream_lock_irqsave().
> > > > > > +	 * This would end up in a dead lock.
> > > > > > +	 */
> > > > > > +	timeri->flags |= SNDRV_TIMER_IFLG_AUTO;
> > > > > > +	timeri->callback = loopback_snd_timer_function;
> > > > > > +	timeri->callback_data = (void *)cable;
> > > > > > +	timeri->ccallback = loopback_snd_timer_event;
> > > > > > +
> > > > > > +	/* snd_timer_close() and snd_timer_open() should not be
> > called
> > > with
> > > > > > +	 * locked spinlock because both functions can block on a
> > > mutex. The
> > > > > > +	 * mutex loopback->cable_lock is kept locked. Therefore
> > > > > snd_timer_open()
> > > > > > +	 * cannot be called a second time by the other device of the
> > > same
> > > > > cable.
> > > > > > +	 * Therefore the following issue cannot happen:
> > > > > > +	 * [proc1] Call loopback_timer_open() ->
> > > > > > +	 *	   Unlock cable->lock for snd_timer_close/open()
> > call
> > > > > > +	 * [proc2] Call loopback_timer_open() -> snd_timer_open(),
> > > > > > +	 *	   snd_timer_start()
> > > > > > +	 * [proc1] Call snd_timer_open() and overwrite running timer
> > > > > > +	 *	   instance
> > > > > > +	 */
> > > > > > +	spin_unlock_irq(&cable->lock);
> > > > > > +	err = snd_timer_open(timeri, &cable->snd_timer.id, current-
> > > >pid);
> > > > > > +	if (err < 0) {
> > > > > > +		pcm_err(dpcm->substream->pcm,
> > > > > > +			"snd_timer_open (%d,%d,%d) failed with %d",
> > > > > > +			cable->snd_timer.id.card,
> > > > > > +			cable->snd_timer.id.device,
> > > > > > +			cable->snd_timer.id.subdevice,
> > > > > > +			err);
> > > > > > +		snd_timer_instance_free(timeri);
> > > > > > +		return err;
> > > > > > +	}
> > > > > > +	spin_lock_irq(&cable->lock);
> > > > > > +
> > > > > > +	cable->snd_timer.instance = timeri;
> > > > > > +
> > > > > > +	/* initialise a tasklet used for draining */
> > > > > > +	tasklet_init(&cable->snd_timer.event_tasklet,
> > > > > > +		     loopback_snd_timer_tasklet, (unsigned
> > > long)timeri);
> > > > >
> > > > > This has to be set before snd_timer_open().  The callback might be
> > > > > called immediately after snd_timer_open().
> > > >
> > > > This tasklet is used/scheduled only in ccallback (not regular tick
> > > > callback),
> > > > and only for SNDRV_TIMER_EVENT_MSTOP event. Can this event really
> happen
> > > > immediately after snd_timer_open()?
> > >
> > > Why not?  The master timer can be stopped at any time, even between
> > > these two lines.
> > >
> > > Beware that there are fuzzer programs that can trigger such racy
> > > things, and you're adding the code to the target that is actively
> > > slapped by them :)
> >
> > OK, got it.
> > I'll move this initialization to before snd_timer_open() in the next
> > update together with the fixes for the other issues you will find
> > in this version.
> 
> I have no other issues, so you can just resubmit only that patch,
> too.

I'm not sure how to correctly format resubmitting of a single patch from
a patch set, so I'm submitting the next version v5 of the whole patch set:
https://mailman.alsa-project.org/pipermail/alsa-devel/2019-November/158939.h
tml

Thanks!

Best regards,
Andrew

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH v4 6/7] ALSA: aloop: Support selection of snd_timer instead of jiffies
  2019-11-20 11:58             ` [alsa-devel] " Andrew Gabbasov
                               ` (2 preceding siblings ...)
  (?)
@ 2019-11-22 12:36             ` kbuild test robot
  -1 siblings, 0 replies; 26+ messages in thread
From: kbuild test robot @ 2019-11-22 12:36 UTC (permalink / raw)
  To: kbuild-all

[-- Attachment #1: Type: text/plain, Size: 9369 bytes --]

Hi Andrew,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on linus/master]
[also build test WARNING on v5.4-rc8]
[cannot apply to sound/for-next next-20191121]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/Andrew-Gabbasov/ALSA-aloop-Support-sound-timer-as-clock-source-instead-of-jiffies/20191122-170109
base:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 81429eb8d9ca40b0c65bb739d29fa856c5d5e958
config: arm64-allyesconfig (attached as .config)
compiler: aarch64-linux-gcc (GCC) 7.4.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        GCC_VERSION=7.4.0 make.cross ARCH=arm64 

If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

   sound//drivers/aloop.c: In function 'loopback_snd_timer_close_cable':
   sound//drivers/aloop.c:314:2: error: implicit declaration of function 'snd_timer_instance_free'; did you mean 'snd_timer_global_free'? [-Werror=implicit-function-declaration]
     snd_timer_instance_free(cable->snd_timer.instance);
     ^~~~~~~~~~~~~~~~~~~~~~~
     snd_timer_global_free
   sound//drivers/aloop.c: In function 'loopback_snd_timer_open':
   sound//drivers/aloop.c:1130:11: error: implicit declaration of function 'snd_timer_instance_new'; did you mean 'snd_timer_global_new'? [-Werror=implicit-function-declaration]
     timeri = snd_timer_instance_new(dpcm->loopback->card->id);
              ^~~~~~~~~~~~~~~~~~~~~~
              snd_timer_global_new
   sound//drivers/aloop.c:1130:9: warning: assignment makes pointer from integer without a cast [-Wint-conversion]
     timeri = snd_timer_instance_new(dpcm->loopback->card->id);
            ^
   sound//drivers/aloop.c:1161:23: error: passing argument 1 of 'snd_timer_open' from incompatible pointer type [-Werror=incompatible-pointer-types]
     err = snd_timer_open(timeri, &cable->snd_timer.id, current->pid);
                          ^~~~~~
   In file included from sound//drivers/aloop.c:31:0:
   include/sound/timer.h:121:5: note: expected 'struct snd_timer_instance **' but argument is of type 'struct snd_timer_instance *'
    int snd_timer_open(struct snd_timer_instance **ti, char *owner, struct snd_timer_id *tid, unsigned int slave_id);
        ^~~~~~~~~~~~~~
   sound//drivers/aloop.c:1161:31: error: passing argument 2 of 'snd_timer_open' from incompatible pointer type [-Werror=incompatible-pointer-types]
     err = snd_timer_open(timeri, &cable->snd_timer.id, current->pid);
                                  ^
   In file included from sound//drivers/aloop.c:31:0:
   include/sound/timer.h:121:5: note: expected 'char *' but argument is of type 'struct snd_timer_id *'
    int snd_timer_open(struct snd_timer_instance **ti, char *owner, struct snd_timer_id *tid, unsigned int slave_id);
        ^~~~~~~~~~~~~~
   In file included from include/linux/thread_info.h:21:0,
                    from arch/arm64/include/asm/preempt.h:5,
                    from include/linux/preempt.h:78,
                    from include/linux/spinlock.h:51,
                    from include/linux/seqlock.h:36,
                    from include/linux/time.h:6,
                    from include/linux/jiffies.h:9,
                    from sound//drivers/aloop.c:19:
>> arch/arm64/include/asm/current.h:24:17: warning: passing argument 3 of 'snd_timer_open' makes pointer from integer without a cast [-Wint-conversion]
    #define current get_current()
                    ^
   sound//drivers/aloop.c:1161:53: note: in expansion of macro 'current'
     err = snd_timer_open(timeri, &cable->snd_timer.id, current->pid);
                                                        ^~~~~~~
   In file included from sound//drivers/aloop.c:31:0:
   include/sound/timer.h:121:5: note: expected 'struct snd_timer_id *' but argument is of type 'pid_t {aka int}'
    int snd_timer_open(struct snd_timer_instance **ti, char *owner, struct snd_timer_id *tid, unsigned int slave_id);
        ^~~~~~~~~~~~~~
   sound//drivers/aloop.c:1161:8: error: too few arguments to function 'snd_timer_open'
     err = snd_timer_open(timeri, &cable->snd_timer.id, current->pid);
           ^~~~~~~~~~~~~~
   In file included from sound//drivers/aloop.c:31:0:
   include/sound/timer.h:121:5: note: declared here
    int snd_timer_open(struct snd_timer_instance **ti, char *owner, struct snd_timer_id *tid, unsigned int slave_id);
        ^~~~~~~~~~~~~~
   cc1: some warnings being treated as errors
--
   sound/drivers/aloop.c: In function 'loopback_snd_timer_close_cable':
   sound/drivers/aloop.c:314:2: error: implicit declaration of function 'snd_timer_instance_free'; did you mean 'snd_timer_global_free'? [-Werror=implicit-function-declaration]
     snd_timer_instance_free(cable->snd_timer.instance);
     ^~~~~~~~~~~~~~~~~~~~~~~
     snd_timer_global_free
   sound/drivers/aloop.c: In function 'loopback_snd_timer_open':
   sound/drivers/aloop.c:1130:11: error: implicit declaration of function 'snd_timer_instance_new'; did you mean 'snd_timer_global_new'? [-Werror=implicit-function-declaration]
     timeri = snd_timer_instance_new(dpcm->loopback->card->id);
              ^~~~~~~~~~~~~~~~~~~~~~
              snd_timer_global_new
   sound/drivers/aloop.c:1130:9: warning: assignment makes pointer from integer without a cast [-Wint-conversion]
     timeri = snd_timer_instance_new(dpcm->loopback->card->id);
            ^
   sound/drivers/aloop.c:1161:23: error: passing argument 1 of 'snd_timer_open' from incompatible pointer type [-Werror=incompatible-pointer-types]
     err = snd_timer_open(timeri, &cable->snd_timer.id, current->pid);
                          ^~~~~~
   In file included from sound/drivers/aloop.c:31:0:
   include/sound/timer.h:121:5: note: expected 'struct snd_timer_instance **' but argument is of type 'struct snd_timer_instance *'
    int snd_timer_open(struct snd_timer_instance **ti, char *owner, struct snd_timer_id *tid, unsigned int slave_id);
        ^~~~~~~~~~~~~~
   sound/drivers/aloop.c:1161:31: error: passing argument 2 of 'snd_timer_open' from incompatible pointer type [-Werror=incompatible-pointer-types]
     err = snd_timer_open(timeri, &cable->snd_timer.id, current->pid);
                                  ^
   In file included from sound/drivers/aloop.c:31:0:
   include/sound/timer.h:121:5: note: expected 'char *' but argument is of type 'struct snd_timer_id *'
    int snd_timer_open(struct snd_timer_instance **ti, char *owner, struct snd_timer_id *tid, unsigned int slave_id);
        ^~~~~~~~~~~~~~
   In file included from include/linux/thread_info.h:21:0,
                    from arch/arm64/include/asm/preempt.h:5,
                    from include/linux/preempt.h:78,
                    from include/linux/spinlock.h:51,
                    from include/linux/seqlock.h:36,
                    from include/linux/time.h:6,
                    from include/linux/jiffies.h:9,
                    from sound/drivers/aloop.c:19:
>> arch/arm64/include/asm/current.h:24:17: warning: passing argument 3 of 'snd_timer_open' makes pointer from integer without a cast [-Wint-conversion]
    #define current get_current()
                    ^
   sound/drivers/aloop.c:1161:53: note: in expansion of macro 'current'
     err = snd_timer_open(timeri, &cable->snd_timer.id, current->pid);
                                                        ^~~~~~~
   In file included from sound/drivers/aloop.c:31:0:
   include/sound/timer.h:121:5: note: expected 'struct snd_timer_id *' but argument is of type 'pid_t {aka int}'
    int snd_timer_open(struct snd_timer_instance **ti, char *owner, struct snd_timer_id *tid, unsigned int slave_id);
        ^~~~~~~~~~~~~~
   sound/drivers/aloop.c:1161:8: error: too few arguments to function 'snd_timer_open'
     err = snd_timer_open(timeri, &cable->snd_timer.id, current->pid);
           ^~~~~~~~~~~~~~
   In file included from sound/drivers/aloop.c:31:0:
   include/sound/timer.h:121:5: note: declared here
    int snd_timer_open(struct snd_timer_instance **ti, char *owner, struct snd_timer_id *tid, unsigned int slave_id);
        ^~~~~~~~~~~~~~
   cc1: some warnings being treated as errors

vim +/snd_timer_open +24 arch/arm64/include/asm/current.h

c02433dd6de32f Mark Rutland 2016-11-03  23  
c02433dd6de32f Mark Rutland 2016-11-03 @24  #define current get_current()
c02433dd6de32f Mark Rutland 2016-11-03  25  

:::::: The code at line 24 was first introduced by commit
:::::: c02433dd6de32f042cf3ffe476746b1115b8c096 arm64: split thread_info from task stack

:::::: TO: Mark Rutland <mark.rutland@arm.com>
:::::: CC: Catalin Marinas <catalin.marinas@arm.com>

---
0-DAY kernel test infrastructure                 Open Source Technology Center
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org Intel Corporation

[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 67239 bytes --]

^ permalink raw reply	[flat|nested] 26+ messages in thread

end of thread, other threads:[~2019-11-22 12:36 UTC | newest]

Thread overview: 26+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-11-20 11:58 [PATCH v4 0/7] ALSA: aloop: Support sound timer as clock source instead of jiffies Andrew Gabbasov
2019-11-20 11:58 ` [alsa-devel] " Andrew Gabbasov
2019-11-20 11:58 ` [PATCH v4 1/7] ALSA: aloop: Describe units of variables Andrew Gabbasov
2019-11-20 11:58   ` [alsa-devel] " Andrew Gabbasov
2019-11-20 11:58   ` [PATCH v4 2/7] ALSA: aloop: Support return of error code for timer start and stop Andrew Gabbasov
2019-11-20 11:58     ` [alsa-devel] " Andrew Gabbasov
2019-11-20 11:58     ` [PATCH v4 3/7] ALSA: aloop: Use callback functions for timer specific implementations Andrew Gabbasov
2019-11-20 11:58       ` [alsa-devel] " Andrew Gabbasov
2019-11-20 11:58       ` [PATCH v4 4/7] ALSA: aloop: Rename all jiffies timer specific functions Andrew Gabbasov
2019-11-20 11:58         ` [alsa-devel] " Andrew Gabbasov
2019-11-20 11:58         ` [PATCH v4 5/7] ALSA: aloop: Move CABLE_VALID_BOTH to the top of file Andrew Gabbasov
2019-11-20 11:58           ` [alsa-devel] " Andrew Gabbasov
2019-11-20 11:58           ` [PATCH v4 6/7] ALSA: aloop: Support selection of snd_timer instead of jiffies Andrew Gabbasov
2019-11-20 11:58             ` [alsa-devel] " Andrew Gabbasov
2019-11-20 11:58             ` [PATCH v4 7/7] ALSA: aloop: Support runtime change of snd_timer via info interface Andrew Gabbasov
2019-11-20 11:58               ` [alsa-devel] " Andrew Gabbasov
2019-11-20 14:33             ` [PATCH v4 6/7] ALSA: aloop: Support selection of snd_timer instead of jiffies Takashi Iwai
2019-11-20 15:21               ` Andrew Gabbasov
2019-11-20 15:21                 ` [alsa-devel] " Andrew Gabbasov
2019-11-20 15:32                 ` Takashi Iwai
2019-11-20 15:39                   ` Andrew Gabbasov
2019-11-20 15:39                     ` [alsa-devel] " Andrew Gabbasov
2019-11-20 15:58                     ` Takashi Iwai
2019-11-20 17:56                       ` Andrew Gabbasov
2019-11-20 17:56                         ` [alsa-devel] " Andrew Gabbasov
2019-11-22 12:36             ` kbuild test robot

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.