[PATCH 0/7] ALSA: firewire-lib: some fixes for tracepoints events

[PATCH 0/7] ALSA: firewire-lib: some fixes for tracepoints events

[Takashi Sakamoto]

Hi,

This kernel module, firewire-lib, adds 'amdtp_packet' tracepoints events
to Linux system. After heavy code refactoring for v5.3, some event
parameters include invalid values. This patchset is to fix them, and
the last part of my refactoring work in this development period.

Takashi Sakamoto (7):
  ALSA: firewire-lib: fix invalid length of tx packet payload for
    tracepoint events
  ALSA: firewire-lib/ficeface: fix initial value of data block counter
    for IR context with CIP_NO_HEADER
  ALSA: firewire-lib: fix initial value of data block count for IR
    context without CIP_DBC_IS_END_EVENT
  ALSA: firewire-lib: fix different data block counter between probed
    event and transferred isochronous packet
  ALSA: firewire-lib: code refactoring for error path of parser for CIP
    header
  ALSA: firewire-lib: code refactoring for post operation to data block
    counter
  ALSA: firewire-lib: code refactoring for local variables

 sound/firewire/amdtp-stream.c | 59 ++++++++++++++++++-----------------
 1 file changed, 31 insertions(+), 28 deletions(-)

-- 
2.20.1

[PATCH 1/7] ALSA: firewire-lib: fix invalid length of rx packet payload for tracepoint events

[Takashi Sakamoto]

Although CIP header is handled as context header, the length of isochronous
packet includes two quadlets for its payload. In tracepoints event the
value of payload_quadlets should includes the two quadlets. But at present
it doesn't.

This commit fixes the bug.

Fixes: b18f0cfaf16b ("ALSA: firewire-lib: use 8 byte packet header for IT context to separate CIP header from CIP payload")
Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
---
 sound/firewire/amdtp-stream.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/sound/firewire/amdtp-stream.c b/sound/firewire/amdtp-stream.c
index 91b890241840..6c9f4d026505 100644
--- a/sound/firewire/amdtp-stream.c
+++ b/sound/firewire/amdtp-stream.c
@@ -490,8 +490,12 @@ static void build_it_pkt_header(struct amdtp_stream *s, unsigned int cycle,
 				unsigned int data_blocks, unsigned int syt,
 				unsigned int index)
 {
+	unsigned int payload_length;
 	__be32 *cip_header;
 
+	payload_length = data_blocks * sizeof(__be32) * s->data_block_quadlets;
+	params->payload_length = payload_length;
+
 	if (s->flags & CIP_DBC_IS_END_EVENT) {
 		s->data_block_counter =
 				(s->data_block_counter + data_blocks) & 0xff;
@@ -501,6 +505,7 @@ static void build_it_pkt_header(struct amdtp_stream *s, unsigned int cycle,
 		cip_header = (__be32 *)params->header;
 		generate_cip_header(s, cip_header, syt);
 		params->header_length = 2 * sizeof(__be32);
+		payload_length += params->header_length;
 	} else {
 		cip_header = NULL;
 	}
@@ -510,11 +515,8 @@ static void build_it_pkt_header(struct amdtp_stream *s, unsigned int cycle,
 				(s->data_block_counter + data_blocks) & 0xff;
 	}
 
-	params->payload_length =
-			data_blocks * sizeof(__be32) * s->data_block_quadlets;
-
-	trace_amdtp_packet(s, cycle, cip_header, params->payload_length,
-			   data_blocks, index);
+	trace_amdtp_packet(s, cycle, cip_header, payload_length, data_blocks,
+			   index);
 }
 
 static int check_cip_header(struct amdtp_stream *s, const __be32 *buf,
-- 
2.20.1

[PATCH 2/7] ALSA: firewire-lib/fireface: fix initial value of data block counter for IR context with CIP_NO_HEADER

[Takashi Sakamoto]

For IR context, ALSA IEC 61883-1/6 engine uses initial value of data
block counter as UINT_MAX, to detect first isochronous packet in the
middle of packet streaming.

At present, when CIP_NO_HEADER is used (i.e. for ALSA fireface driver),
the initial value is used for tracepoints event. 0x00 should be
for the event when the initial value is UINT_MAX because isochronous
packets with CIP_NO_HEADER option has no field for data block count.

This commit fixes the bug.

Fixes: 76864868dbab ("ALSA: firewire-lib: cache next data_block_counter after probing tracepoints event for IR context")
Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
---
 sound/firewire/amdtp-stream.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/sound/firewire/amdtp-stream.c b/sound/firewire/amdtp-stream.c
index 6c9f4d026505..c8d77bb05798 100644
--- a/sound/firewire/amdtp-stream.c
+++ b/sound/firewire/amdtp-stream.c
@@ -646,8 +646,12 @@ static int parse_ir_ctx_header(struct amdtp_stream *s, unsigned int cycle,
 		err = 0;
 		*data_blocks = *payload_length / sizeof(__be32) /
 			       s->data_block_quadlets;
-		*dbc = s->data_block_counter;
 		*syt = 0;
+
+		if (s->data_block_counter != UINT_MAX)
+			*dbc = s->data_block_counter;
+		else
+			*dbc = 0;
 	}
 
 	if (err >= 0 && s->flags & CIP_DBC_IS_END_EVENT)
-- 
2.20.1

[PATCH 3/7] ALSA: firewire-lib: fix initial value of data block count for IR context without CIP_DBC_IS_END_EVENT

[Takashi Sakamoto]

For IR context, ALSA IEC 61883-1/6 engine uses initial value of data
block counter as UINT_MAX, to detect first isochronous packet in the
middle of packet streaming.

At present, when CIP_DBC_IS_END_EVENT is not used (i.e. for drivers except
for ALSA fireworks driver), the initial value is used as is for
tracepoints event. However, the engine can detect the value of dbc field
in the payload of first isochronous packet and the value should be assigned
to the event.

This commit fixes the bug.

Fixes: 76864868dbab ("ALSA: firewire-lib: cache next data_block_counter after probing tracepoints event for IR context")
Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
---
 sound/firewire/amdtp-stream.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/sound/firewire/amdtp-stream.c b/sound/firewire/amdtp-stream.c
index c8d77bb05798..c5daef7872a5 100644
--- a/sound/firewire/amdtp-stream.c
+++ b/sound/firewire/amdtp-stream.c
@@ -654,8 +654,10 @@ static int parse_ir_ctx_header(struct amdtp_stream *s, unsigned int cycle,
 			*dbc = 0;
 	}
 
-	if (err >= 0 && s->flags & CIP_DBC_IS_END_EVENT)
-		s->data_block_counter = *dbc;
+	if (err < 0)
+		return err;
+
+	s->data_block_counter = *dbc;
 
 	trace_amdtp_packet(s, cycle, cip_header, *payload_length, *data_blocks,
 			   index);
-- 
2.20.1

[PATCH 4/7] ALSA: firewire-lib: fix different data block counter between probed event and transferred isochronous packet

[Takashi Sakamoto]

For IT context, tracepoints event is probed after calculating next data
block counter. This brings difference of data block counter between
the probed event and actual isochronous packet.

This commit fixes it.

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

diff --git a/sound/firewire/amdtp-stream.c b/sound/firewire/amdtp-stream.c
index c5daef7872a5..24cc8ce51e01 100644
--- a/sound/firewire/amdtp-stream.c
+++ b/sound/firewire/amdtp-stream.c
@@ -510,13 +510,13 @@ static void build_it_pkt_header(struct amdtp_stream *s, unsigned int cycle,
 		cip_header = NULL;
 	}
 
+	trace_amdtp_packet(s, cycle, cip_header, payload_length, data_blocks,
+			   index);
+
 	if (!(s->flags & CIP_DBC_IS_END_EVENT)) {
 		s->data_block_counter =
 				(s->data_block_counter + data_blocks) & 0xff;
 	}
-
-	trace_amdtp_packet(s, cycle, cip_header, payload_length, data_blocks,
-			   index);
 }
 
 static int check_cip_header(struct amdtp_stream *s, const __be32 *buf,
-- 
2.20.1

[PATCH 5/7] ALSA: firewire-lib: code refactoring for error path of parser for CIP header

[Takashi Sakamoto]

When a parser for CIP header returns -EAGAIN, no extra care is needed
to probe tracepoints event.

This commit adds code refactoring for the error path.

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

diff --git a/sound/firewire/amdtp-stream.c b/sound/firewire/amdtp-stream.c
index 24cc8ce51e01..8c4564a560f6 100644
--- a/sound/firewire/amdtp-stream.c
+++ b/sound/firewire/amdtp-stream.c
@@ -635,12 +635,8 @@ static int parse_ir_ctx_header(struct amdtp_stream *s, unsigned int cycle,
 		cip_header = ctx_header + 2;
 		err = check_cip_header(s, cip_header, *payload_length,
 				       data_blocks, dbc, syt);
-		if (err < 0) {
-			if (err != -EAGAIN)
-				return err;
-
-			*data_blocks = 0;
-		}
+		if (err < 0)
+			return err;
 	} else {
 		cip_header = NULL;
 		err = 0;
@@ -654,9 +650,6 @@ static int parse_ir_ctx_header(struct amdtp_stream *s, unsigned int cycle,
 			*dbc = 0;
 	}
 
-	if (err < 0)
-		return err;
-
 	s->data_block_counter = *dbc;
 
 	trace_amdtp_packet(s, cycle, cip_header, *payload_length, *data_blocks,
-- 
2.20.1

[PATCH 6/7] ALSA: firewire-lib: code refactoring for post operation to data block counter

[Takashi Sakamoto]

As a result of former commits, post operation to data block count for
cases without CIP_DBC_IS_END_EVENT can be done just with
data_block_counter member of amdtp_stream structure.

This commit adds code refactoring to obsolete local variable for
data block counter.

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

diff --git a/sound/firewire/amdtp-stream.c b/sound/firewire/amdtp-stream.c
index 8c4564a560f6..81af191627db 100644
--- a/sound/firewire/amdtp-stream.c
+++ b/sound/firewire/amdtp-stream.c
@@ -616,9 +616,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 *dbc,
-			       unsigned int *syt, unsigned int index)
+			       unsigned int *data_blocks, unsigned int *syt,
+			       unsigned int index)
 {
+	unsigned int dbc;
 	const __be32 *cip_header;
 	int err;
 
@@ -634,7 +635,7 @@ 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)
 			return err;
 	} else {
@@ -645,12 +646,12 @@ static int parse_ir_ctx_header(struct amdtp_stream *s, unsigned int cycle,
 		*syt = 0;
 
 		if (s->data_block_counter != UINT_MAX)
-			*dbc = s->data_block_counter;
+			dbc = s->data_block_counter;
 		else
-			*dbc = 0;
+			dbc = 0;
 	}
 
-	s->data_block_counter = *dbc;
+	s->data_block_counter = dbc;
 
 	trace_amdtp_packet(s, cycle, cip_header, *payload_length, *data_blocks,
 			   index);
@@ -758,7 +759,6 @@ static void in_stream_callback(struct fw_iso_context *context, u32 tstamp,
 		u32 cycle;
 		unsigned int payload_length;
 		unsigned int data_blocks;
-		unsigned int dbc;
 		unsigned int syt;
 		__be32 *buffer;
 		unsigned int pcm_frames = 0;
@@ -768,7 +768,7 @@ 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_blocks, &dbc, &syt, i);
+					  &data_blocks, &syt, i);
 		if (err < 0 && err != -EAGAIN)
 			break;
 
@@ -778,8 +778,8 @@ static void in_stream_callback(struct fw_iso_context *context, u32 tstamp,
 							    data_blocks, &syt);
 
 			if (!(s->flags & CIP_DBC_IS_END_EVENT)) {
-				s->data_block_counter =
-						(dbc + data_blocks) & 0xff;
+				s->data_block_counter += data_blocks;
+				s->data_block_counter &= 0xff;
 			}
 		}
 
-- 
2.20.1

[PATCH 7/7] ALSA: firewire-lib: code refactoring for local variables

[Takashi Sakamoto]

It's better to use int type for loop index. For consistency, the name
of local variable for the number of data block should be plural.

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

diff --git a/sound/firewire/amdtp-stream.c b/sound/firewire/amdtp-stream.c
index 81af191627db..51f97df81dbf 100644
--- a/sound/firewire/amdtp-stream.c
+++ b/sound/firewire/amdtp-stream.c
@@ -700,7 +700,8 @@ static void out_stream_callback(struct fw_iso_context *context, u32 tstamp,
 {
 	struct amdtp_stream *s = private_data;
 	const __be32 *ctx_header = header;
-	unsigned int i, packets = header_length / sizeof(*ctx_header);
+	unsigned int packets = header_length / sizeof(*ctx_header);
+	int i;
 
 	if (s->packet_index < 0)
 		return;
@@ -708,7 +709,7 @@ static void out_stream_callback(struct fw_iso_context *context, u32 tstamp,
 	for (i = 0; i < packets; ++i) {
 		u32 cycle;
 		unsigned int syt;
-		unsigned int data_block;
+		unsigned int data_blocks;
 		__be32 *buffer;
 		unsigned int pcm_frames;
 		struct {
@@ -719,12 +720,13 @@ static void out_stream_callback(struct fw_iso_context *context, u32 tstamp,
 
 		cycle = compute_it_cycle(*ctx_header);
 		syt = calculate_syt(s, cycle);
-		data_block = calculate_data_blocks(s, syt);
+		data_blocks = calculate_data_blocks(s, syt);
 		buffer = s->buffer.packets[s->packet_index].buffer;
-		pcm_frames = s->process_data_blocks(s, buffer, data_block, &syt);
+		pcm_frames = s->process_data_blocks(s, buffer, data_blocks,
+						    &syt);
 
-		build_it_pkt_header(s, cycle, &template.params, data_block, syt,
-				    i);
+		build_it_pkt_header(s, cycle, &template.params, data_blocks,
+				    syt, i);
 
 		if (queue_out_packet(s, &template.params) < 0) {
 			cancel_stream(s);
-- 
2.20.1

Re: [PATCH 0/7] ALSA: firewire-lib: some fixes for tracepoints events

[Takashi Iwai]

On Sun, 07 Jul 2019 14:07:52 +0200,
Takashi Sakamoto wrote:
> 
> Hi,
> 
> This kernel module, firewire-lib, adds 'amdtp_packet' tracepoints events
> to Linux system. After heavy code refactoring for v5.3, some event
> parameters include invalid values. This patchset is to fix them, and
> the last part of my refactoring work in this development period.
> 
> Takashi Sakamoto (7):
>   ALSA: firewire-lib: fix invalid length of tx packet payload for
>     tracepoint events
>   ALSA: firewire-lib/ficeface: fix initial value of data block counter
>     for IR context with CIP_NO_HEADER
>   ALSA: firewire-lib: fix initial value of data block count for IR
>     context without CIP_DBC_IS_END_EVENT
>   ALSA: firewire-lib: fix different data block counter between probed
>     event and transferred isochronous packet
>   ALSA: firewire-lib: code refactoring for error path of parser for CIP
>     header
>   ALSA: firewire-lib: code refactoring for post operation to data block
>     counter
>   ALSA: firewire-lib: code refactoring for local variables

Applied all seven patches.  Thanks.


Takashi