line6linux-devel

[Line6linux-devel] [PATCH 0/5] staging: line6: more fixes and cleanups

[<ste...@gm...>]

From: Stefan Hajnoczi <ste...@gm...>

I have been testing PulseAudio, ALSA, and jack applications with a POD HD300
because they tend to produce different call patterns.  This is useful for
exposing bugs.  This series includes the latest fixes as well as some code
cleanups.

The fixes I'd like to highlight are:

Patch 1 resolves a potential memory leak which I introduced when moving buffer
allocation into the .hw_params() pcm callback.

Patch 3 ensures that the playback buffer is used correctly irrespective of urb
completion order.  I'm not certain what guarantees we have on iso urb
completion order so this may not be a real problem but it at least simplifies
the code.

Patch 5 makes jackd work by eliminating the -EBUSY error when it tries to start
a stream.

Stefan Hajnoczi (5):
  staging: line6: fix memory leak in .hw_params()
  staging: line6: fix playback urb transfer buffer calculation
  staging: line6: eliminate useless index_out variable
  staging: line6: eliminate useless NULL checks
  staging: line6: wait for urbs in snd_line6_prepare()

 capture.c  |    7 +++++--
 pcm.c      |   14 ++++++++++++++
 pcm.h      |    5 -----
 playback.c |   12 ++++++------
 pod.c      |    6 +-----
 podhd.c    |    6 +-----
 toneport.c |    6 +-----
 variax.c   |    6 +-----
 8 files changed, 29 insertions(+), 33 deletions(-)

-- 
1.7.7.3

[Line6linux-devel] [PATCH 1/5] staging: line6: fix memory leak in .hw_params()

[<ste...@gm...>]

From: Stefan Hajnoczi <ste...@gm...>

The .hw_params() pcm callback can be invoked multiple times in a row.
Ensure that the URB data buffer is only allocated once.

Signed-off-by: Stefan Hajnoczi <ste...@gm...>
---
 capture.c  |    7 +++++--
 playback.c |    7 +++++--
 2 files changed, 10 insertions(+), 4 deletions(-)

diff --git a/capture.c b/capture.c
index d9da7ed..c7e3bfa 100644
--- a/capture.c
+++ b/capture.c
@@ -320,8 +320,11 @@ static int snd_line6_capture_hw_params(struct snd_pcm_substream *substream,
 	}
 	/* -- [FD] end */
 
-	line6pcm->buffer_in = kmalloc(LINE6_ISO_BUFFERS * LINE6_ISO_PACKETS *
-				      line6pcm->max_packet_size, GFP_KERNEL);
+	/* We may be invoked multiple times in a row so allocate once only */
+	if (!line6pcm->buffer_in)
+		line6pcm->buffer_in =
+			kmalloc(LINE6_ISO_BUFFERS * LINE6_ISO_PACKETS *
+				line6pcm->max_packet_size, GFP_KERNEL);
 
 	if (!line6pcm->buffer_in) {
 		dev_err(line6pcm->line6->ifcdev,
diff --git a/playback.c b/playback.c
index b344527..c39d1f0 100644
--- a/playback.c
+++ b/playback.c
@@ -470,8 +470,11 @@ static int snd_line6_playback_hw_params(struct snd_pcm_substream *substream,
 	}
 	/* -- [FD] end */
 
-	line6pcm->buffer_out = kmalloc(LINE6_ISO_BUFFERS * LINE6_ISO_PACKETS *
-				       line6pcm->max_packet_size, GFP_KERNEL);
+	/* We may be invoked multiple times in a row so allocate once only */
+	if (!line6pcm->buffer_out)
+		line6pcm->buffer_out =
+			kmalloc(LINE6_ISO_BUFFERS * LINE6_ISO_PACKETS *
+				line6pcm->max_packet_size, GFP_KERNEL);
 
 	if (!line6pcm->buffer_out) {
 		dev_err(line6pcm->line6->ifcdev,
-- 
1.7.7.3

[Line6linux-devel] [PATCH 2/5] staging: line6: fix playback urb transfer buffer calculation

[<ste...@gm...>]

From: Stefan Hajnoczi <ste...@gm...>

The playback urb transfer buffer calculation does not factor in
LINE6_ISO_PACKETS.  Buffer memory is organized like this in the driver:

  Buffer 0                 Buffer 1                 ...
  [Packet 0, Packet 1, ...][Packet 0, Packet 1, ...][Packet 0, ...]

However, we're lucky that LINE6_ISO_PACKETS is currently defined as 1 so
this patch does not change any behavior.  It's still worth including
this fix in case the LINE6_ISO_PACKETS value is changed in the future.

Signed-off-by: Stefan Hajnoczi <ste...@gm...>
---
 playback.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/playback.c b/playback.c
index c39d1f0..f43f55b 100644
--- a/playback.c
+++ b/playback.c
@@ -192,7 +192,7 @@ static int submit_audio_out_urb(struct snd_line6_pcm *line6pcm)
 	urb_frames = urb_size / bytes_per_frame;
 	urb_out->transfer_buffer =
 	    line6pcm->buffer_out +
-	    line6pcm->max_packet_size * line6pcm->index_out;
+	    LINE6_ISO_PACKETS * line6pcm->max_packet_size * line6pcm->index_out;
 	urb_out->transfer_buffer_length = urb_size;
 	urb_out->context = line6pcm;
 
-- 
1.7.7.3

[Line6linux-devel] [PATCH 5/5] staging: line6: wait for urbs in snd_line6_prepare()

[<ste...@gm...>]

From: Stefan Hajnoczi <ste...@gm...>

The .trigger() pcm callbacks are not allowed to block and cannot wait
until urbs have completed.  We need to ensure that stopping, preparing,
and then restarting a stream always works.

Currently the driver will sometimes return -EBUSY when restarting the
stream because urbs have not completed yet.  This can be triggered by
jackd from userspace.

The solution is to wait on urbs in the .prepare() pcm callback since
blocking is allowed in that callback.  This guarantees that all urbs are
quiesced and ready to be submitted when the start trigger callback is
invoked.

Signed-off-by: Stefan Hajnoczi <ste...@gm...>
---
 pcm.c |   14 ++++++++++++++
 1 files changed, 14 insertions(+), 0 deletions(-)

diff --git a/pcm.c b/pcm.c
index 7e3a5f0..7ca6451 100644
--- a/pcm.c
+++ b/pcm.c
@@ -491,6 +491,20 @@ int snd_line6_prepare(struct snd_pcm_substream *substream)
 {
 	struct snd_line6_pcm *line6pcm = snd_pcm_substream_chip(substream);
 
+	switch (substream->stream) {
+	case SNDRV_PCM_STREAM_PLAYBACK:
+		line6_unlink_wait_clear_audio_out_urbs(line6pcm);
+		break;
+
+	case SNDRV_PCM_STREAM_CAPTURE:
+		line6_unlink_wait_clear_audio_in_urbs(line6pcm);
+		break;
+
+	default:
+		MISSING_CASE;
+	}
+
+
 	if (!test_and_set_bit(BIT_PREPARED, &line6pcm->flags)) {
 		line6pcm->count_out = 0;
 		line6pcm->pos_out = 0;
-- 
1.7.7.3

[Line6linux-devel] [PATCH 3/5] staging: line6: eliminate useless index_out variable

[<ste...@gm...>]

From: Stefan Hajnoczi <ste...@gm...>

Playback urbs use the index_out counter to decide which part of the
playback buffer to use.  Since the urb already has a unique index in
range [0, LINE6_ISO_BUFFERS) there is no need to keep a separate
counter.

Use the urb index instead.  This also eliminates the possibility of two
urbs using the same playback buffer space if they ever complete
out-of-order for some reason.

Signed-off-by: Stefan Hajnoczi <ste...@gm...>
---
 pcm.h      |    5 -----
 playback.c |    5 +----
 2 files changed, 1 insertions(+), 9 deletions(-)

diff --git a/pcm.h b/pcm.h
index 77055b3..e41e981 100644
--- a/pcm.h
+++ b/pcm.h
@@ -149,11 +149,6 @@ struct snd_line6_pcm {
 	unsigned char *buffer_in;
 
 	/**
-		 Temporary buffer index for playback.
-	*/
-	int index_out;
-
-	/**
 		 Previously captured frame (for software monitoring).
 	*/
 	unsigned char *prev_fbuf;
diff --git a/playback.c b/playback.c
index f43f55b..add9537 100644
--- a/playback.c
+++ b/playback.c
@@ -192,13 +192,10 @@ static int submit_audio_out_urb(struct snd_line6_pcm *line6pcm)
 	urb_frames = urb_size / bytes_per_frame;
 	urb_out->transfer_buffer =
 	    line6pcm->buffer_out +
-	    LINE6_ISO_PACKETS * line6pcm->max_packet_size * line6pcm->index_out;
+	    index * LINE6_ISO_PACKETS * line6pcm->max_packet_size;
 	urb_out->transfer_buffer_length = urb_size;
 	urb_out->context = line6pcm;
 
-	if (++line6pcm->index_out == LINE6_ISO_BUFFERS)
-		line6pcm->index_out = 0;
-
 	if (test_bit(BIT_PCM_ALSA_PLAYBACK, &line6pcm->flags) &&
 	    !test_bit(BIT_PAUSE_PLAYBACK, &line6pcm->flags)) {
 		struct snd_pcm_runtime *runtime =
-- 
1.7.7.3

[Line6linux-devel] [PATCH 4/5] staging: line6: eliminate useless NULL checks

[<ste...@gm...>]

From: Stefan Hajnoczi <ste...@gm...>

The line6 driver checks struct field addresses for NULL where it does
not make sense to do so.  The struct has already been checked for NULL
and there is no value in checking the first field's address too.

Suggested-by: Dan Carpenter <dan...@or...>
Signed-off-by: Stefan Hajnoczi <ste...@gm...>
---
 pod.c      |    6 +-----
 podhd.c    |    6 +-----
 toneport.c |    6 +-----
 variax.c   |    6 +-----
 4 files changed, 4 insertions(+), 20 deletions(-)

diff --git a/pod.c b/pod.c
index d9b3021..4dadc57 100644
--- a/pod.c
+++ b/pod.c
@@ -1149,14 +1149,10 @@ static struct snd_kcontrol_new pod_control_monitor = {
 static void pod_destruct(struct usb_interface *interface)
 {
 	struct usb_line6_pod *pod = usb_get_intfdata(interface);
-	struct usb_line6 *line6;
 
 	if (pod == NULL)
 		return;
-	line6 = &pod->line6;
-	if (line6 == NULL)
-		return;
-	line6_cleanup_audio(line6);
+	line6_cleanup_audio(&pod->line6);
 
 	del_timer(&pod->startup_timer);
 	cancel_work_sync(&pod->startup_work);
diff --git a/podhd.c b/podhd.c
index 6c0f7f2..7ef4543 100644
--- a/podhd.c
+++ b/podhd.c
@@ -80,14 +80,10 @@ static struct line6_pcm_properties podhd_pcm_properties = {
 static void podhd_destruct(struct usb_interface *interface)
 {
 	struct usb_line6_podhd *podhd = usb_get_intfdata(interface);
-	struct usb_line6 *line6;
 
 	if (podhd == NULL)
 		return;
-	line6 = &podhd->line6;
-	if (line6 == NULL)
-		return;
-	line6_cleanup_audio(line6);
+	line6_cleanup_audio(&podhd->line6);
 }
 
 /*
diff --git a/toneport.c b/toneport.c
index 879e699..f310578 100644
--- a/toneport.c
+++ b/toneport.c
@@ -295,14 +295,10 @@ static struct snd_kcontrol_new toneport_control_source = {
 static void toneport_destruct(struct usb_interface *interface)
 {
 	struct usb_line6_toneport *toneport = usb_get_intfdata(interface);
-	struct usb_line6 *line6;
 
 	if (toneport == NULL)
 		return;
-	line6 = &toneport->line6;
-	if (line6 == NULL)
-		return;
-	line6_cleanup_audio(line6);
+	line6_cleanup_audio(&toneport->line6);
 }
 
 /*
diff --git a/variax.c b/variax.c
index 81241cd..d366222 100644
--- a/variax.c
+++ b/variax.c
@@ -572,14 +572,10 @@ static DEVICE_ATTR(raw2, S_IWUSR, line6_nop_read, variax_set_raw2);
 static void variax_destruct(struct usb_interface *interface)
 {
 	struct usb_line6_variax *variax = usb_get_intfdata(interface);
-	struct usb_line6 *line6;
 
 	if (variax == NULL)
 		return;
-	line6 = &variax->line6;
-	if (line6 == NULL)
-		return;
-	line6_cleanup_audio(line6);
+	line6_cleanup_audio(&variax->line6);
 
 	del_timer(&variax->startup_timer1);
 	del_timer(&variax->startup_timer2);
-- 
1.7.7.3