[PATCH 0/2] ALSA: firewire-lib: fix miss detection from MIDI conformant data channel in AM824 format for IR context

[PATCH 0/2] ALSA: firewire-lib: fix miss detection from MIDI conformant data channel in AM824 format for IR context

[Takashi Sakamoto]

Hi,

This patchset is a fix for a case that AM824 format is used with
non-blocking transmission method in IEC 61883-1/6.

The recent changes for ALSA IEC 61883-1/6 engine brings a bug to miss
received MIDI messages in the case due to wrong data block counter for
calculation of multiplexed MIDI stream index. This bug mainly affects
ALSA oxfw driver.

Takashi Sakamoto (2):
  ALSA: firewire-lib: cache next data_block_counter after probing
    tracepoints event for IR context
  ALSA: firewire-lib: fix to process MIDI conformant data channel for
    AM824 format

 sound/firewire/amdtp-stream.c | 58 +++++++++++++++++++----------------
 1 file changed, 32 insertions(+), 26 deletions(-)

-- 
2.20.1

[PATCH 1/2] ALSA: firewire-lib: cache next data_block_counter after probing tracepoints event for IR context

[Takashi Sakamoto]

For debugging purpose, ALSA IEC 61883-1/6 engine has tracepoints event.
In current implementation, next data block counter is stored as current
data block counter before probing the event for IR isoc context. It's not
good to check current packet parameter.

This commit changes to assign the next data block counter after probing
the event.

Besides, Fireworks devices has a quirk to transfer isoc packet with
data block counter for the last data block. For this quirk, the
assignment is done before calling data block processing layer.

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

diff --git a/sound/firewire/amdtp-stream.c b/sound/firewire/amdtp-stream.c
index 3aef6a78a188..b341bd86605e 100644
--- a/sound/firewire/amdtp-stream.c
+++ b/sound/firewire/amdtp-stream.c
@@ -519,13 +519,13 @@ static void build_it_pkt_header(struct amdtp_stream *s, unsigned int cycle,
 
 static int check_cip_header(struct amdtp_stream *s, const __be32 *buf,
 			    unsigned int payload_length,
-			    unsigned int *data_blocks, unsigned int *syt)
+			    unsigned int *data_blocks, unsigned int *dbc,
+			    unsigned int *syt)
 {
 	u32 cip_header[2];
 	unsigned int sph;
 	unsigned int fmt;
 	unsigned int fdf;
-	unsigned int data_block_counter;
 	bool lost;
 
 	cip_header[0] = be32_to_cpu(buf[0]);
@@ -577,17 +577,17 @@ static int check_cip_header(struct amdtp_stream *s, const __be32 *buf,
 	}
 
 	/* Check data block counter continuity */
-	data_block_counter = cip_header[0] & CIP_DBC_MASK;
+	*dbc = cip_header[0] & CIP_DBC_MASK;
 	if (*data_blocks == 0 && (s->flags & CIP_EMPTY_HAS_WRONG_DBC) &&
 	    s->data_block_counter != UINT_MAX)
-		data_block_counter = s->data_block_counter;
+		*dbc = s->data_block_counter;
 
 	if (((s->flags & CIP_SKIP_DBC_ZERO_CHECK) &&
-	     data_block_counter == s->ctx_data.tx.first_dbc) ||
+	     *dbc == s->ctx_data.tx.first_dbc) ||
 	    s->data_block_counter == UINT_MAX) {
 		lost = false;
 	} else if (!(s->flags & CIP_DBC_IS_END_EVENT)) {
-		lost = data_block_counter != s->data_block_counter;
+		lost = *dbc != s->data_block_counter;
 	} else {
 		unsigned int dbc_interval;
 
@@ -596,26 +596,18 @@ static int check_cip_header(struct amdtp_stream *s, const __be32 *buf,
 		else
 			dbc_interval = *data_blocks;
 
-		lost = data_block_counter !=
-		       ((s->data_block_counter + dbc_interval) & 0xff);
+		lost = *dbc != ((s->data_block_counter + dbc_interval) & 0xff);
 	}
 
 	if (lost) {
 		dev_err(&s->unit->device,
 			"Detect discontinuity of CIP: %02X %02X\n",
-			s->data_block_counter, data_block_counter);
+			s->data_block_counter, *dbc);
 		return -EIO;
 	}
 
 	*syt = cip_header[1] & CIP_SYT_MASK;
 
-	if (s->flags & CIP_DBC_IS_END_EVENT) {
-		s->data_block_counter = data_block_counter;
-	} else {
-		s->data_block_counter =
-				(data_block_counter + *data_blocks) & 0xff;
-	}
-
 	return 0;
 }
 
@@ -626,6 +618,7 @@ static int parse_ir_ctx_header(struct amdtp_stream *s, unsigned int cycle,
 			       unsigned int *syt, unsigned int index)
 {
 	const __be32 *cip_header;
+	unsigned int dbc;
 	int err;
 
 	*payload_length = be32_to_cpu(ctx_header[0]) >> ISO_DATA_LENGTH_SHIFT;
@@ -640,22 +633,33 @@ static int parse_ir_ctx_header(struct amdtp_stream *s, unsigned int cycle,
 	if (!(s->flags & CIP_NO_HEADER)) {
 		cip_header = ctx_header + 2;
 		err = check_cip_header(s, cip_header, *payload_length,
-				       data_blocks, syt);
-		if (err < 0)
-			return err;
+				       data_blocks, &dbc, syt);
+		if (err < 0) {
+			if (err != -EAGAIN)
+				return err;
+
+			*data_blocks = 0;
+			dbc = s->data_block_counter;
+		}
 	} else {
 		cip_header = NULL;
+		err = 0;
 		*data_blocks = *payload_length / sizeof(__be32) /
 			       s->data_block_quadlets;
+		dbc = s->data_block_counter;
 		*syt = 0;
-		s->data_block_counter =
-				(s->data_block_counter + *data_blocks) & 0xff;
 	}
 
+	if (err >= 0 && s->flags & CIP_DBC_IS_END_EVENT)
+		s->data_block_counter = dbc;
+
 	trace_amdtp_packet(s, cycle, cip_header, *payload_length, *data_blocks,
 			   index);
 
-	return 0;
+	if (err >= 0 && !(s->flags & CIP_DBC_IS_END_EVENT))
+		s->data_block_counter = (dbc + *data_blocks) & 0xff;
+
+	return err;
 }
 
 // In CYCLE_TIMER register of IEEE 1394, 7 bits are used to represent second. On
-- 
2.20.1

[PATCH 2/2] ALSA: firewire-lib: fix to process MIDI conformant data channel for AM824 format

[Takashi Sakamoto]

In IEC 61883-6, 8 MIDI data streams are multiplexed into single MIDI
conformant data channel. The index of stream is calculated by modulo 8
of the value of data block counter. Therefore data block processing
layer requires valid value of data block counter.

In recent changes of ALSA IEC 61883-1/6 engine, the value of data block
counter is changed before calling data block processing layer. This
brings miss detection of MIDI messages in non-blocking transmission
method is used.

This commit fixes the bug by changing chached data block counter after
calling data block processing layer.

Fixes: e335425b6596 ("ALSA: firewire-lib: split helper function to check incoming CIP header")
Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
---
 sound/firewire/amdtp-stream.c | 26 ++++++++++++++------------
 1 file changed, 14 insertions(+), 12 deletions(-)

diff --git a/sound/firewire/amdtp-stream.c b/sound/firewire/amdtp-stream.c
index b341bd86605e..91b890241840 100644
--- a/sound/firewire/amdtp-stream.c
+++ b/sound/firewire/amdtp-stream.c
@@ -614,11 +614,10 @@ static int check_cip_header(struct amdtp_stream *s, const __be32 *buf,
 static int parse_ir_ctx_header(struct amdtp_stream *s, unsigned int cycle,
 			       const __be32 *ctx_header,
 			       unsigned int *payload_length,
-			       unsigned int *data_blocks,
+			       unsigned int *data_blocks, unsigned int *dbc,
 			       unsigned int *syt, unsigned int index)
 {
 	const __be32 *cip_header;
-	unsigned int dbc;
 	int err;
 
 	*payload_length = be32_to_cpu(ctx_header[0]) >> ISO_DATA_LENGTH_SHIFT;
@@ -633,32 +632,28 @@ static int parse_ir_ctx_header(struct amdtp_stream *s, unsigned int cycle,
 	if (!(s->flags & CIP_NO_HEADER)) {
 		cip_header = ctx_header + 2;
 		err = check_cip_header(s, cip_header, *payload_length,
-				       data_blocks, &dbc, syt);
+				       data_blocks, dbc, syt);
 		if (err < 0) {
 			if (err != -EAGAIN)
 				return err;
 
 			*data_blocks = 0;
-			dbc = s->data_block_counter;
 		}
 	} else {
 		cip_header = NULL;
 		err = 0;
 		*data_blocks = *payload_length / sizeof(__be32) /
 			       s->data_block_quadlets;
-		dbc = s->data_block_counter;
+		*dbc = s->data_block_counter;
 		*syt = 0;
 	}
 
 	if (err >= 0 && s->flags & CIP_DBC_IS_END_EVENT)
-		s->data_block_counter = dbc;
+		s->data_block_counter = *dbc;
 
 	trace_amdtp_packet(s, cycle, cip_header, *payload_length, *data_blocks,
 			   index);
 
-	if (err >= 0 && !(s->flags & CIP_DBC_IS_END_EVENT))
-		s->data_block_counter = (dbc + *data_blocks) & 0xff;
-
 	return err;
 }
 
@@ -761,7 +756,8 @@ static void in_stream_callback(struct fw_iso_context *context, u32 tstamp,
 	for (i = 0; i < packets; i++) {
 		u32 cycle;
 		unsigned int payload_length;
-		unsigned int data_block;
+		unsigned int data_blocks;
+		unsigned int dbc;
 		unsigned int syt;
 		__be32 *buffer;
 		unsigned int pcm_frames = 0;
@@ -771,13 +767,19 @@ static void in_stream_callback(struct fw_iso_context *context, u32 tstamp,
 
 		cycle = compute_cycle_count(ctx_header[1]);
 		err = parse_ir_ctx_header(s, cycle, ctx_header, &payload_length,
-					  &data_block, &syt, i);
+					  &data_blocks, &dbc, &syt, i);
 		if (err < 0 && err != -EAGAIN)
 			break;
+
 		if (err >= 0) {
 			buffer = s->buffer.packets[s->packet_index].buffer;
 			pcm_frames = s->process_data_blocks(s, buffer,
-							    data_block, &syt);
+							    data_blocks, &syt);
+
+			if (!(s->flags & CIP_DBC_IS_END_EVENT)) {
+				s->data_block_counter =
+						(dbc + data_blocks) & 0xff;
+			}
 		}
 
 		if (queue_in_packet(s, &params) < 0)
-- 
2.20.1

Re: [PATCH 1/2] ALSA: firewire-lib: cache next data_block_counter after probing tracepoints event for IR context

[Takashi Iwai]

On Fri, 28 Jun 2019 07:53:30 +0200,
Takashi Sakamoto wrote:
> 
> For debugging purpose, ALSA IEC 61883-1/6 engine has tracepoints event.
> In current implementation, next data block counter is stored as current
> data block counter before probing the event for IR isoc context. It's not
> good to check current packet parameter.
> 
> This commit changes to assign the next data block counter after probing
> the event.
> 
> Besides, Fireworks devices has a quirk to transfer isoc packet with
> data block counter for the last data block. For this quirk, the
> assignment is done before calling data block processing layer.
> 
> Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>

Thanks, applied now.


Takashi

Re: [PATCH 2/2] ALSA: firewire-lib: fix to process MIDI conformant data channel for AM824 format

[Takashi Iwai]

On Fri, 28 Jun 2019 07:53:31 +0200,
Takashi Sakamoto wrote:
> 
> In IEC 61883-6, 8 MIDI data streams are multiplexed into single MIDI
> conformant data channel. The index of stream is calculated by modulo 8
> of the value of data block counter. Therefore data block processing
> layer requires valid value of data block counter.
> 
> In recent changes of ALSA IEC 61883-1/6 engine, the value of data block
> counter is changed before calling data block processing layer. This
> brings miss detection of MIDI messages in non-blocking transmission
> method is used.
> 
> This commit fixes the bug by changing chached data block counter after
> calling data block processing layer.
> 
> Fixes: e335425b6596 ("ALSA: firewire-lib: split helper function to check incoming CIP header")
> Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>

Thanks, applied now.


Takashi