Alsa-Devel Archive on lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/5 v2] ALSA: firewire-lib: purge restriction of synchronization for non-blocking mode
@ 2015-05-22 14:00 Takashi Sakamoto
  2015-05-22 14:00 ` [PATCH 1/5] ALSA: firewire-lib: add buffer-over-run protection at receiving more data blocks than expected Takashi Sakamoto
                   ` (6 more replies)
  0 siblings, 7 replies; 8+ messages in thread
From: Takashi Sakamoto @ 2015-05-22 14:00 UTC (permalink / raw)
  To: clemens, tiwai; +Cc: alsa-devel, linux1394-devel, ffado-devel

This patchset renews my previous one.

[alsa-devel] [PATCH 0/4] firewire-lib: purge restriction of synchronization for non-blocking mode
http://mailman.alsa-project.org/pipermail/alsa-devel/2015-May/092086.html

Changes:
 * Use stack to keep calculation result per callback of isochronous receive context.
 * Fix my stupid mistake to compare byte count to quadlet count. (oh...)
 * Rename flag to proper name.
 * Add 4th patch newly for better reading.


Takashi Sakamoto (5):
  ALSA: firewire-lib: add buffer-over-run protection at receiving more
    data blocks than expected
  ALSA: firewire-lib: simplify function to calculate the number of data
    blocks
  ALSA: firewire-lib: pass the number of data blocks in incoming packets
    to outgoing packets
  ALSA: firewire-lib: set streaming error outside of packetization
  ALSA: firewire-lib: remove restriction for non-blocking mode

 sound/firewire/amdtp.c            | 151 ++++++++++++++++++++++++--------------
 sound/firewire/amdtp.h            |   4 +
 sound/firewire/oxfw/oxfw-stream.c |  10 ++-
 3 files changed, 106 insertions(+), 59 deletions(-)

-- 
2.1.4


------------------------------------------------------------------------------
One dashboard for servers and applications across Physical-Virtual-Cloud 
Widest out-of-the-box monitoring support with 50+ applications
Performance metrics, stats and reports that give you Actionable Insights
Deep dive visibility with transaction tracing using APM Insight.
http://ad.doubleclick.net/ddm/clk/290420510;117567292;y

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

* [PATCH 1/5] ALSA: firewire-lib: add buffer-over-run protection at receiving more data blocks than expected
  2015-05-22 14:00 [PATCH 0/5 v2] ALSA: firewire-lib: purge restriction of synchronization for non-blocking mode Takashi Sakamoto
@ 2015-05-22 14:00 ` Takashi Sakamoto
  2015-05-22 14:00 ` [PATCH 2/5] ALSA: firewire-lib: simplify function to calculate the number of data blocks Takashi Sakamoto
                   ` (5 subsequent siblings)
  6 siblings, 0 replies; 8+ messages in thread
From: Takashi Sakamoto @ 2015-05-22 14:00 UTC (permalink / raw)
  To: clemens, tiwai; +Cc: alsa-devel, linux1394-devel, ffado-devel

In IEC 61883-6, the number of data blocks in a packet is limited up to
the value of SYT_INTERVAL. Current implementation is compliant to the
limitation, while it can cause buffer-over-run when the value of dbs
field in received packet is illegally large.

This commit adds a validator to detect such illegal packets to prevent
the buffer-over-run. Actually, the buffer is aligned to the size of memory
 page, thus this issue hardly causes system errors due to the room to page
alignment, as long as a few packets includes such jumbo payload; i.e.
a packet to several received packets.

Here, Behringer F-Control Audio 202 (based on OXFW 960) has a quirk to
postpone transferring isochronous packet till finish handling any
asynchronous packets. In this case, this model is lazy, transfers no
packets according to several cycle-start packets. After finishing, this
model pushes required data in next isochronous packet. As a result, the
packet include more data blocks than IEC 61883-6 defines.

To continue to support this model, this commit adds a new flag to extend
the length of calculated payload. This flag allows the size of payload
5 times as large as IEC 61883-6 defines. As a result, packets from this
model passed the validator successfully.

Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
---
 sound/firewire/amdtp.c            | 21 +++++++++++++++++++--
 sound/firewire/amdtp.h            |  4 ++++
 sound/firewire/oxfw/oxfw-stream.c | 10 ++++++++--
 3 files changed, 31 insertions(+), 4 deletions(-)

diff --git a/sound/firewire/amdtp.c b/sound/firewire/amdtp.c
index d882ca5..4eb8dc9 100644
--- a/sound/firewire/amdtp.c
+++ b/sound/firewire/amdtp.c
@@ -255,7 +255,12 @@ EXPORT_SYMBOL(amdtp_stream_set_parameters);
  */
 unsigned int amdtp_stream_get_max_payload(struct amdtp_stream *s)
 {
-	return 8 + s->syt_interval * s->data_block_quadlets * 4;
+	unsigned int multiplier = 1;
+
+	if (s->flags & CIP_JUMBO_PAYLOAD)
+		multiplier = 5;
+
+	return 8 + s->syt_interval * s->data_block_quadlets * 4 * multiplier;
 }
 EXPORT_SYMBOL(amdtp_stream_get_max_payload);
 
@@ -811,12 +816,16 @@ static void in_stream_callback(struct fw_iso_context *context, u32 cycle,
 			       void *private_data)
 {
 	struct amdtp_stream *s = private_data;
-	unsigned int p, syt, packets, payload_quadlets;
+	unsigned int p, syt, packets;
+	unsigned int payload_quadlets, max_payload_quadlets;
 	__be32 *buffer, *headers = header;
 
 	/* The number of packets in buffer */
 	packets = header_length / IN_PACKET_HEADER_SIZE;
 
+	/* For buffer-over-run prevention. */
+	max_payload_quadlets = amdtp_stream_get_max_payload(s) / 4;
+
 	for (p = 0; p < packets; p++) {
 		if (s->packet_index < 0)
 			break;
@@ -832,6 +841,14 @@ static void in_stream_callback(struct fw_iso_context *context, u32 cycle,
 		/* The number of quadlets in this packet */
 		payload_quadlets =
 			(be32_to_cpu(headers[p]) >> ISO_DATA_LENGTH_SHIFT) / 4;
+		if (payload_quadlets > max_payload_quadlets) {
+			dev_err(&s->unit->device,
+				"Detect jumbo payload: %02x %02x\n",
+				payload_quadlets, max_payload_quadlets);
+			s->packet_index = -1;
+			break;
+		}
+
 		handle_in_packet(s, payload_quadlets, buffer);
 	}
 
diff --git a/sound/firewire/amdtp.h b/sound/firewire/amdtp.h
index 8a03a91..26b9093 100644
--- a/sound/firewire/amdtp.h
+++ b/sound/firewire/amdtp.h
@@ -29,6 +29,9 @@
  *	packet is not continuous from an initial value.
  * @CIP_EMPTY_HAS_WRONG_DBC: Only for in-stream. The value of dbc in empty
  *	packet is wrong but the others are correct.
+ * @CIP_JUMBO_PAYLOAD: Only for in-stream. The number of data blocks in an
+ *	packet is larger than IEC 61883-6 defines. Current implementation
+ *	allows 5 times as large as IEC 61883-6 defines.
  */
 enum cip_flags {
 	CIP_NONBLOCKING		= 0x00,
@@ -40,6 +43,7 @@ enum cip_flags {
 	CIP_SKIP_DBC_ZERO_CHECK	= 0x20,
 	CIP_SKIP_INIT_DBC_CHECK	= 0x40,
 	CIP_EMPTY_HAS_WRONG_DBC	= 0x80,
+	CIP_JUMBO_PAYLOAD	= 0x100,
 };
 
 /**
diff --git a/sound/firewire/oxfw/oxfw-stream.c b/sound/firewire/oxfw/oxfw-stream.c
index e6757cd..873d40f 100644
--- a/sound/firewire/oxfw/oxfw-stream.c
+++ b/sound/firewire/oxfw/oxfw-stream.c
@@ -232,9 +232,15 @@ int snd_oxfw_stream_init_simplex(struct snd_oxfw *oxfw,
 		goto end;
 	}
 
-	/* OXFW starts to transmit packets with non-zero dbc. */
+	/*
+	 * OXFW starts to transmit packets with non-zero dbc.
+	 * OXFW postpone transferring packets till handling any asynchronous
+	 * packets. As a result, next isochronous packet includes more data
+	 * blocks than IEC 61883-6 defines.
+	 */
 	if (stream == &oxfw->tx_stream)
-		oxfw->tx_stream.flags |= CIP_SKIP_INIT_DBC_CHECK;
+		oxfw->tx_stream.flags |= CIP_SKIP_INIT_DBC_CHECK |
+					 CIP_JUMBO_PAYLOAD;
 end:
 	return err;
 }
-- 
2.1.4


------------------------------------------------------------------------------
One dashboard for servers and applications across Physical-Virtual-Cloud 
Widest out-of-the-box monitoring support with 50+ applications
Performance metrics, stats and reports that give you Actionable Insights
Deep dive visibility with transaction tracing using APM Insight.
http://ad.doubleclick.net/ddm/clk/290420510;117567292;y

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

* [PATCH 2/5] ALSA: firewire-lib: simplify function to calculate the number of data blocks
  2015-05-22 14:00 [PATCH 0/5 v2] ALSA: firewire-lib: purge restriction of synchronization for non-blocking mode Takashi Sakamoto
  2015-05-22 14:00 ` [PATCH 1/5] ALSA: firewire-lib: add buffer-over-run protection at receiving more data blocks than expected Takashi Sakamoto
@ 2015-05-22 14:00 ` Takashi Sakamoto
  2015-05-22 14:00 ` [PATCH 3/5] ALSA: firewire-lib: pass the number of data blocks in incoming packets to outgoing packets Takashi Sakamoto
                   ` (4 subsequent siblings)
  6 siblings, 0 replies; 8+ messages in thread
From: Takashi Sakamoto @ 2015-05-22 14:00 UTC (permalink / raw)
  To: clemens, tiwai; +Cc: alsa-devel, linux1394-devel, ffado-devel

This function is called according to conditions between the value of
syt and streaming mode(blocking or non-blocking).

To simplify caller's work, this commit push these conditions to the
function.

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

diff --git a/sound/firewire/amdtp.c b/sound/firewire/amdtp.c
index 4eb8dc9..ab75ed9 100644
--- a/sound/firewire/amdtp.c
+++ b/sound/firewire/amdtp.c
@@ -325,17 +325,25 @@ void amdtp_stream_pcm_prepare(struct amdtp_stream *s)
 }
 EXPORT_SYMBOL(amdtp_stream_pcm_prepare);
 
-static unsigned int calculate_data_blocks(struct amdtp_stream *s)
+static unsigned int calculate_data_blocks(struct amdtp_stream *s,
+					  unsigned int syt)
 {
 	unsigned int phase, data_blocks;
 
-	if (s->flags & CIP_BLOCKING)
-		data_blocks = s->syt_interval;
-	else if (!cip_sfc_is_base_44100(s->sfc)) {
-		/* Sample_rate / 8000 is an integer, and precomputed. */
-		data_blocks = s->data_block_state;
+	/* Blocking mode. */
+	if (s->flags & CIP_BLOCKING) {
+		/* This module generate empty packet for 'no data'. */
+		if (syt == CIP_SYT_NO_INFO)
+			data_blocks = 0;
+		else
+			data_blocks = s->syt_interval;
+	/* Non-blocking mode. */
 	} else {
-		phase = s->data_block_state;
+		if (!cip_sfc_is_base_44100(s->sfc)) {
+			/* Sample_rate / 8000 is an integer, and precomputed. */
+			data_blocks = s->data_block_state;
+		} else {
+			phase = s->data_block_state;
 
 		/*
 		 * This calculates the number of data blocks per packet so that
@@ -345,16 +353,17 @@ static unsigned int calculate_data_blocks(struct amdtp_stream *s)
 		 *    as possible in the sequence (to prevent underruns of the
 		 *    device's buffer).
 		 */
-		if (s->sfc == CIP_SFC_44100)
-			/* 6 6 5 6 5 6 5 ... */
-			data_blocks = 5 + ((phase & 1) ^
-					   (phase == 0 || phase >= 40));
-		else
-			/* 12 11 11 11 11 ... or 23 22 22 22 22 ... */
-			data_blocks = 11 * (s->sfc >> 1) + (phase == 0);
-		if (++phase >= (80 >> (s->sfc >> 1)))
-			phase = 0;
-		s->data_block_state = phase;
+			if (s->sfc == CIP_SFC_44100)
+				/* 6 6 5 6 5 6 5 ... */
+				data_blocks = 5 + ((phase & 1) ^
+						   (phase == 0 || phase >= 40));
+			else
+				/* 12 11 11 11 11 ... or 23 22 22 22 22 ... */
+				data_blocks = 11 * (s->sfc >> 1) + (phase == 0);
+			if (++phase >= (80 >> (s->sfc >> 1)))
+				phase = 0;
+			s->data_block_state = phase;
+		}
 	}
 
 	return data_blocks;
@@ -651,11 +660,7 @@ static void handle_out_packet(struct amdtp_stream *s, unsigned int syt)
 	if (s->packet_index < 0)
 		return;
 
-	/* this module generate empty packet for 'no data' */
-	if (!(s->flags & CIP_BLOCKING) || (syt != CIP_SYT_NO_INFO))
-		data_blocks = calculate_data_blocks(s);
-	else
-		data_blocks = 0;
+	data_blocks = calculate_data_blocks(s, syt);
 
 	buffer = s->buffer.packets[s->packet_index].buffer;
 	buffer[0] = cpu_to_be32(ACCESS_ONCE(s->source_node_id_field) |
-- 
2.1.4


------------------------------------------------------------------------------
One dashboard for servers and applications across Physical-Virtual-Cloud 
Widest out-of-the-box monitoring support with 50+ applications
Performance metrics, stats and reports that give you Actionable Insights
Deep dive visibility with transaction tracing using APM Insight.
http://ad.doubleclick.net/ddm/clk/290420510;117567292;y

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

* [PATCH 3/5] ALSA: firewire-lib: pass the number of data blocks in incoming packets to outgoing packets
  2015-05-22 14:00 [PATCH 0/5 v2] ALSA: firewire-lib: purge restriction of synchronization for non-blocking mode Takashi Sakamoto
  2015-05-22 14:00 ` [PATCH 1/5] ALSA: firewire-lib: add buffer-over-run protection at receiving more data blocks than expected Takashi Sakamoto
  2015-05-22 14:00 ` [PATCH 2/5] ALSA: firewire-lib: simplify function to calculate the number of data blocks Takashi Sakamoto
@ 2015-05-22 14:00 ` Takashi Sakamoto
  2015-05-22 14:00 ` [PATCH 4/5] ALSA: firewire-lib: set streaming error outside of packetization Takashi Sakamoto
                   ` (3 subsequent siblings)
  6 siblings, 0 replies; 8+ messages in thread
From: Takashi Sakamoto @ 2015-05-22 14:00 UTC (permalink / raw)
  To: clemens, tiwai; +Cc: alsa-devel, linux1394-devel, ffado-devel

Current implementation reuses the value of syt field in incoming packet to
outgoing packet for full duplex timestamp synchronization, while the number
of data blocks in outgoing packets refers to hard-coded table and the
synchronization cannot be applied to non-blocking stream.

This commit passes the number of data blocks from incoming packet
processing to outgoing packet processing for the synchronization. For
normal mode, isochronous callback handler is changed to generate the values
of syt and data blocks.

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

diff --git a/sound/firewire/amdtp.c b/sound/firewire/amdtp.c
index ab75ed9..c5034e7 100644
--- a/sound/firewire/amdtp.c
+++ b/sound/firewire/amdtp.c
@@ -651,17 +651,16 @@ static inline int queue_in_packet(struct amdtp_stream *s)
 			    amdtp_stream_get_max_payload(s), false);
 }
 
-static void handle_out_packet(struct amdtp_stream *s, unsigned int syt)
+static void handle_out_packet(struct amdtp_stream *s, unsigned int data_blocks,
+			      unsigned int syt)
 {
 	__be32 *buffer;
-	unsigned int data_blocks, payload_length;
+	unsigned int payload_length;
 	struct snd_pcm_substream *pcm;
 
 	if (s->packet_index < 0)
 		return;
 
-	data_blocks = calculate_data_blocks(s, syt);
-
 	buffer = s->buffer.packets[s->packet_index].buffer;
 	buffer[0] = cpu_to_be32(ACCESS_ONCE(s->source_node_id_field) |
 				(s->data_block_quadlets << AMDTP_DBS_SHIFT) |
@@ -691,13 +690,12 @@ static void handle_out_packet(struct amdtp_stream *s, unsigned int syt)
 		update_pcm_pointers(s, pcm, data_blocks);
 }
 
-static void handle_in_packet(struct amdtp_stream *s,
-			     unsigned int payload_quadlets,
-			     __be32 *buffer)
+static int handle_in_packet(struct amdtp_stream *s,
+			    unsigned int payload_quadlets, __be32 *buffer)
 {
 	u32 cip_header[2];
-	unsigned int data_blocks, data_block_quadlets, data_block_counter,
-		     dbc_interval;
+	unsigned int data_blocks;
+	unsigned int data_block_quadlets, data_block_counter, dbc_interval;
 	struct snd_pcm_substream *pcm = NULL;
 	bool lost;
 
@@ -714,6 +712,7 @@ static void handle_in_packet(struct amdtp_stream *s,
 		dev_info_ratelimited(&s->unit->device,
 				"Invalid CIP header for AMDTP: %08X:%08X\n",
 				cip_header[0], cip_header[1]);
+		data_blocks = 0;
 		goto end;
 	}
 
@@ -730,7 +729,7 @@ static void handle_in_packet(struct amdtp_stream *s,
 			dev_info_ratelimited(&s->unit->device,
 				"Detect invalid value in dbs field: %08X\n",
 				cip_header[0]);
-			goto err;
+			return -EIO;
 		}
 		if (s->flags & CIP_WRONG_DBS)
 			data_block_quadlets = s->data_block_quadlets;
@@ -763,7 +762,7 @@ static void handle_in_packet(struct amdtp_stream *s,
 		dev_info(&s->unit->device,
 			 "Detect discontinuity of CIP: %02X %02X\n",
 			 s->data_block_counter, data_block_counter);
-		goto err;
+		return -EIO;
 	}
 
 	if (data_blocks > 0) {
@@ -784,15 +783,12 @@ static void handle_in_packet(struct amdtp_stream *s,
 				(data_block_counter + data_blocks) & 0xff;
 end:
 	if (queue_in_packet(s) < 0)
-		goto err;
+		return -EIO;
 
 	if (pcm)
 		update_pcm_pointers(s, pcm, data_blocks);
 
-	return;
-err:
-	s->packet_index = -1;
-	amdtp_stream_pcm_abort(s);
+	return data_blocks;
 }
 
 static void out_stream_callback(struct fw_iso_context *context, u32 cycle,
@@ -801,6 +797,7 @@ static void out_stream_callback(struct fw_iso_context *context, u32 cycle,
 {
 	struct amdtp_stream *s = private_data;
 	unsigned int i, syt, packets = header_length / 4;
+	unsigned int data_blocks;
 
 	/*
 	 * Compute the cycle of the last queued packet.
@@ -811,7 +808,9 @@ static void out_stream_callback(struct fw_iso_context *context, u32 cycle,
 
 	for (i = 0; i < packets; ++i) {
 		syt = calculate_syt(s, ++cycle);
-		handle_out_packet(s, syt);
+		data_blocks = calculate_data_blocks(s, syt);
+
+		handle_out_packet(s, data_blocks, syt);
 	}
 	fw_iso_context_queue_flush(s->context);
 }
@@ -823,6 +822,7 @@ static void in_stream_callback(struct fw_iso_context *context, u32 cycle,
 	struct amdtp_stream *s = private_data;
 	unsigned int p, syt, packets;
 	unsigned int payload_quadlets, max_payload_quadlets;
+	unsigned int data_blocks;
 	__be32 *buffer, *headers = header;
 
 	/* The number of packets in buffer */
@@ -837,12 +837,6 @@ static void in_stream_callback(struct fw_iso_context *context, u32 cycle,
 
 		buffer = s->buffer.packets[s->packet_index].buffer;
 
-		/* Process sync slave stream */
-		if (s->sync_slave && s->sync_slave->callbacked) {
-			syt = be32_to_cpu(buffer[1]) & CIP_SYT_MASK;
-			handle_out_packet(s->sync_slave, syt);
-		}
-
 		/* The number of quadlets in this packet */
 		payload_quadlets =
 			(be32_to_cpu(headers[p]) >> ISO_DATA_LENGTH_SHIFT) / 4;
@@ -854,11 +848,23 @@ static void in_stream_callback(struct fw_iso_context *context, u32 cycle,
 			break;
 		}
 
-		handle_in_packet(s, payload_quadlets, buffer);
+		data_blocks = handle_in_packet(s, payload_quadlets, buffer);
+		if (data_blocks < 0) {
+			s->packet_index = -1;
+			break;
+		}
+
+		/* Process sync slave stream */
+		if (s->sync_slave && s->sync_slave->callbacked) {
+			syt = be32_to_cpu(buffer[1]) & CIP_SYT_MASK;
+			handle_out_packet(s->sync_slave, data_blocks, syt);
+		}
 	}
 
 	/* Queueing error or detecting discontinuity */
 	if (s->packet_index < 0) {
+		amdtp_stream_pcm_abort(s);
+
 		/* Abort sync slave. */
 		if (s->sync_slave) {
 			s->sync_slave->packet_index = -1;
-- 
2.1.4


------------------------------------------------------------------------------
One dashboard for servers and applications across Physical-Virtual-Cloud 
Widest out-of-the-box monitoring support with 50+ applications
Performance metrics, stats and reports that give you Actionable Insights
Deep dive visibility with transaction tracing using APM Insight.
http://ad.doubleclick.net/ddm/clk/290420510;117567292;y

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

* [PATCH 4/5] ALSA: firewire-lib: set streaming error outside of packetization
  2015-05-22 14:00 [PATCH 0/5 v2] ALSA: firewire-lib: purge restriction of synchronization for non-blocking mode Takashi Sakamoto
                   ` (2 preceding siblings ...)
  2015-05-22 14:00 ` [PATCH 3/5] ALSA: firewire-lib: pass the number of data blocks in incoming packets to outgoing packets Takashi Sakamoto
@ 2015-05-22 14:00 ` Takashi Sakamoto
  2015-05-22 14:00 ` [PATCH 5/5] ALSA: firewire-lib: remove restriction for non-blocking mode Takashi Sakamoto
                   ` (2 subsequent siblings)
  6 siblings, 0 replies; 8+ messages in thread
From: Takashi Sakamoto @ 2015-05-22 14:00 UTC (permalink / raw)
  To: clemens, tiwai; +Cc: alsa-devel, linux1394-devel, ffado-devel

In previous commit, error handling for incoming packet processing is
outside of packetization. This is nice for reading the codes.

This commit applies this idea for outgoing packet processing, too.

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

diff --git a/sound/firewire/amdtp.c b/sound/firewire/amdtp.c
index c5034e7..5c0e3ce 100644
--- a/sound/firewire/amdtp.c
+++ b/sound/firewire/amdtp.c
@@ -651,16 +651,13 @@ static inline int queue_in_packet(struct amdtp_stream *s)
 			    amdtp_stream_get_max_payload(s), false);
 }
 
-static void handle_out_packet(struct amdtp_stream *s, unsigned int data_blocks,
-			      unsigned int syt)
+static int handle_out_packet(struct amdtp_stream *s, unsigned int data_blocks,
+			     unsigned int syt)
 {
 	__be32 *buffer;
 	unsigned int payload_length;
 	struct snd_pcm_substream *pcm;
 
-	if (s->packet_index < 0)
-		return;
-
 	buffer = s->buffer.packets[s->packet_index].buffer;
 	buffer[0] = cpu_to_be32(ACCESS_ONCE(s->source_node_id_field) |
 				(s->data_block_quadlets << AMDTP_DBS_SHIFT) |
@@ -680,14 +677,14 @@ static void handle_out_packet(struct amdtp_stream *s, unsigned int data_blocks,
 	s->data_block_counter = (s->data_block_counter + data_blocks) & 0xff;
 
 	payload_length = 8 + data_blocks * 4 * s->data_block_quadlets;
-	if (queue_out_packet(s, payload_length, false) < 0) {
-		s->packet_index = -1;
-		amdtp_stream_pcm_abort(s);
-		return;
-	}
+	if (queue_out_packet(s, payload_length, false) < 0)
+		return -EIO;
 
 	if (pcm)
 		update_pcm_pointers(s, pcm, data_blocks);
+
+	/* No need to return the number of handled data blocks. */
+	return 0;
 }
 
 static int handle_in_packet(struct amdtp_stream *s,
@@ -799,6 +796,9 @@ static void out_stream_callback(struct fw_iso_context *context, u32 cycle,
 	unsigned int i, syt, packets = header_length / 4;
 	unsigned int data_blocks;
 
+	if (s->packet_index < 0)
+		return;
+
 	/*
 	 * Compute the cycle of the last queued packet.
 	 * (We need only the four lowest bits for the SYT, so we can ignore
@@ -810,8 +810,13 @@ static void out_stream_callback(struct fw_iso_context *context, u32 cycle,
 		syt = calculate_syt(s, ++cycle);
 		data_blocks = calculate_data_blocks(s, syt);
 
-		handle_out_packet(s, data_blocks, syt);
+		if (handle_out_packet(s, data_blocks, syt) < 0) {
+			s->packet_index = -1;
+			amdtp_stream_pcm_abort(s);
+			return;
+		}
 	}
+
 	fw_iso_context_queue_flush(s->context);
 }
 
@@ -825,6 +830,9 @@ static void in_stream_callback(struct fw_iso_context *context, u32 cycle,
 	unsigned int data_blocks;
 	__be32 *buffer, *headers = header;
 
+	if (s->packet_index < 0)
+		return;
+
 	/* The number of packets in buffer */
 	packets = header_length / IN_PACKET_HEADER_SIZE;
 
@@ -832,9 +840,6 @@ static void in_stream_callback(struct fw_iso_context *context, u32 cycle,
 	max_payload_quadlets = amdtp_stream_get_max_payload(s) / 4;
 
 	for (p = 0; p < packets; p++) {
-		if (s->packet_index < 0)
-			break;
-
 		buffer = s->buffer.packets[s->packet_index].buffer;
 
 		/* The number of quadlets in this packet */
@@ -857,7 +862,11 @@ static void in_stream_callback(struct fw_iso_context *context, u32 cycle,
 		/* Process sync slave stream */
 		if (s->sync_slave && s->sync_slave->callbacked) {
 			syt = be32_to_cpu(buffer[1]) & CIP_SYT_MASK;
-			handle_out_packet(s->sync_slave, data_blocks, syt);
+			if (handle_out_packet(s->sync_slave,
+					      data_blocks, syt) < 0) {
+				s->packet_index = -1;
+				break;
+			}
 		}
 	}
 
-- 
2.1.4


------------------------------------------------------------------------------
One dashboard for servers and applications across Physical-Virtual-Cloud 
Widest out-of-the-box monitoring support with 50+ applications
Performance metrics, stats and reports that give you Actionable Insights
Deep dive visibility with transaction tracing using APM Insight.
http://ad.doubleclick.net/ddm/clk/290420510;117567292;y

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

* [PATCH 5/5] ALSA: firewire-lib: remove restriction for non-blocking mode
  2015-05-22 14:00 [PATCH 0/5 v2] ALSA: firewire-lib: purge restriction of synchronization for non-blocking mode Takashi Sakamoto
                   ` (3 preceding siblings ...)
  2015-05-22 14:00 ` [PATCH 4/5] ALSA: firewire-lib: set streaming error outside of packetization Takashi Sakamoto
@ 2015-05-22 14:00 ` Takashi Sakamoto
  2015-05-23  7:16 ` [PATCH 0/5 v2] ALSA: firewire-lib: purge restriction of synchronization " Takashi Iwai
  2015-05-25 15:12 ` Takashi Sakamoto
  6 siblings, 0 replies; 8+ messages in thread
From: Takashi Sakamoto @ 2015-05-22 14:00 UTC (permalink / raw)
  To: clemens, tiwai; +Cc: alsa-devel, linux1394-devel, ffado-devel

Former patches allow non-blocking streams to synchronize with timestamp.
This patch removes the restriction.

Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
---
 sound/firewire/amdtp.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/sound/firewire/amdtp.c b/sound/firewire/amdtp.c
index 5c0e3ce..bed8e10 100644
--- a/sound/firewire/amdtp.c
+++ b/sound/firewire/amdtp.c
@@ -913,7 +913,7 @@ static void amdtp_stream_first_callback(struct fw_iso_context *context,
 
 	if (s->direction == AMDTP_IN_STREAM)
 		context->callback.sc = in_stream_callback;
-	else if ((s->flags & CIP_BLOCKING) && (s->flags & CIP_SYNC_TO_DEVICE))
+	else if (s->flags & CIP_SYNC_TO_DEVICE)
 		context->callback.sc = slave_stream_callback;
 	else
 		context->callback.sc = out_stream_callback;
-- 
2.1.4


------------------------------------------------------------------------------
One dashboard for servers and applications across Physical-Virtual-Cloud 
Widest out-of-the-box monitoring support with 50+ applications
Performance metrics, stats and reports that give you Actionable Insights
Deep dive visibility with transaction tracing using APM Insight.
http://ad.doubleclick.net/ddm/clk/290420510;117567292;y

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

* Re: [PATCH 0/5 v2] ALSA: firewire-lib: purge restriction of synchronization for non-blocking mode
  2015-05-22 14:00 [PATCH 0/5 v2] ALSA: firewire-lib: purge restriction of synchronization for non-blocking mode Takashi Sakamoto
                   ` (4 preceding siblings ...)
  2015-05-22 14:00 ` [PATCH 5/5] ALSA: firewire-lib: remove restriction for non-blocking mode Takashi Sakamoto
@ 2015-05-23  7:16 ` Takashi Iwai
  2015-05-25 15:12 ` Takashi Sakamoto
  6 siblings, 0 replies; 8+ messages in thread
From: Takashi Iwai @ 2015-05-23  7:16 UTC (permalink / raw)
  To: Takashi Sakamoto; +Cc: alsa-devel, linux1394-devel, ffado-devel

At Fri, 22 May 2015 23:00:49 +0900,
Takashi Sakamoto wrote:
> 
> This patchset renews my previous one.
> 
> [alsa-devel] [PATCH 0/4] firewire-lib: purge restriction of synchronization for non-blocking mode
> http://mailman.alsa-project.org/pipermail/alsa-devel/2015-May/092086.html
> 
> Changes:
>  * Use stack to keep calculation result per callback of isochronous receive context.
>  * Fix my stupid mistake to compare byte count to quadlet count. (oh...)
>  * Rename flag to proper name.
>  * Add 4th patch newly for better reading.

Applied all five patches now.


thanks,

Takashi


> 
> 
> Takashi Sakamoto (5):
>   ALSA: firewire-lib: add buffer-over-run protection at receiving more
>     data blocks than expected
>   ALSA: firewire-lib: simplify function to calculate the number of data
>     blocks
>   ALSA: firewire-lib: pass the number of data blocks in incoming packets
>     to outgoing packets
>   ALSA: firewire-lib: set streaming error outside of packetization
>   ALSA: firewire-lib: remove restriction for non-blocking mode
> 
>  sound/firewire/amdtp.c            | 151 ++++++++++++++++++++++++--------------
>  sound/firewire/amdtp.h            |   4 +
>  sound/firewire/oxfw/oxfw-stream.c |  10 ++-
>  3 files changed, 106 insertions(+), 59 deletions(-)
> 
> -- 
> 2.1.4
> 

------------------------------------------------------------------------------
One dashboard for servers and applications across Physical-Virtual-Cloud 
Widest out-of-the-box monitoring support with 50+ applications
Performance metrics, stats and reports that give you Actionable Insights
Deep dive visibility with transaction tracing using APM Insight.
http://ad.doubleclick.net/ddm/clk/290420510;117567292;y

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

* Re: [PATCH 0/5 v2] ALSA: firewire-lib: purge restriction of synchronization for non-blocking mode
  2015-05-22 14:00 [PATCH 0/5 v2] ALSA: firewire-lib: purge restriction of synchronization for non-blocking mode Takashi Sakamoto
                   ` (5 preceding siblings ...)
  2015-05-23  7:16 ` [PATCH 0/5 v2] ALSA: firewire-lib: purge restriction of synchronization " Takashi Iwai
@ 2015-05-25 15:12 ` Takashi Sakamoto
  6 siblings, 0 replies; 8+ messages in thread
From: Takashi Sakamoto @ 2015-05-25 15:12 UTC (permalink / raw)
  To: clemens, tiwai; +Cc: alsa-devel, linux1394-devel, ffado-devel

On May 22 2015 23:00, Takashi Sakamoto wrote:
> This patchset renews my previous one.
> 
> [alsa-devel] [PATCH 0/4] firewire-lib: purge restriction of synchronization for non-blocking mode
> http://mailman.alsa-project.org/pipermail/alsa-devel/2015-May/092086.html
> 
> Changes:
>  * Use stack to keep calculation result per callback of isochronous receive context.
>  * Fix my stupid mistake to compare byte count to quadlet count. (oh...)
>  * Rename flag to proper name.
>  * Add 4th patch newly for better reading.
> 
> 
> Takashi Sakamoto (5):
>   ALSA: firewire-lib: add buffer-over-run protection at receiving more
>     data blocks than expected
>   ALSA: firewire-lib: simplify function to calculate the number of data
>     blocks
>   ALSA: firewire-lib: pass the number of data blocks in incoming packets
>     to outgoing packets
>   ALSA: firewire-lib: set streaming error outside of packetization
>   ALSA: firewire-lib: remove restriction for non-blocking mode
> 
>  sound/firewire/amdtp.c            | 151 ++++++++++++++++++++++++--------------
>  sound/firewire/amdtp.h            |   4 +
>  sound/firewire/oxfw/oxfw-stream.c |  10 ++-
>  3 files changed, 106 insertions(+), 59 deletions(-)

I realized that 3rd and 4th patches cause unexpected execution under
compiler optimization. As a result, when setting error state to incoming
stream, this module can cause invalid paging request.

I guess that 'struct amdtp_stream.packet_index' is set with minus value
and it's used to refer to packet buffer (struct
amdtp_stream.buffer.packets). Then a pointer of the buffer refers to
invalid page.

I tested -O1 and -O2 options for gcc. In both cases,
"handle_in_packet()" is merged into "in_stream_callback()", as I
expected. While, the error condition may not be handled as my
expectation. Especially under -O2, even if 'handle_in_packet()' returns
minus value, the loop in 'in_stream_callback()' doesn't finished till
all of packets are handled. In this case, when detecting packet
discontinuity, several error messages occur against my intension (one
iteration).

Briefly, I move 's->packet_index = -1' from 'in_stream_callback()' to
'handle_in_packet()' and the stack seems to work as I expected. But I'm
not so good at compiler optimization, so need a bit time to fix this issue..

Sorry to post patchset including bugs...


Regards

Takashi Sakamoto

------------------------------------------------------------------------------
One dashboard for servers and applications across Physical-Virtual-Cloud 
Widest out-of-the-box monitoring support with 50+ applications
Performance metrics, stats and reports that give you Actionable Insights
Deep dive visibility with transaction tracing using APM Insight.
http://ad.doubleclick.net/ddm/clk/290420510;117567292;y

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

end of thread, other threads:[~2015-05-25 15:12 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2015-05-22 14:00 [PATCH 0/5 v2] ALSA: firewire-lib: purge restriction of synchronization for non-blocking mode Takashi Sakamoto
2015-05-22 14:00 ` [PATCH 1/5] ALSA: firewire-lib: add buffer-over-run protection at receiving more data blocks than expected Takashi Sakamoto
2015-05-22 14:00 ` [PATCH 2/5] ALSA: firewire-lib: simplify function to calculate the number of data blocks Takashi Sakamoto
2015-05-22 14:00 ` [PATCH 3/5] ALSA: firewire-lib: pass the number of data blocks in incoming packets to outgoing packets Takashi Sakamoto
2015-05-22 14:00 ` [PATCH 4/5] ALSA: firewire-lib: set streaming error outside of packetization Takashi Sakamoto
2015-05-22 14:00 ` [PATCH 5/5] ALSA: firewire-lib: remove restriction for non-blocking mode Takashi Sakamoto
2015-05-23  7:16 ` [PATCH 0/5 v2] ALSA: firewire-lib: purge restriction of synchronization " Takashi Iwai
2015-05-25 15:12 ` Takashi Sakamoto

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox