[alsa-devel] [RFC][PATCH] ALSA: firewire-lib: support Linux tracing to dump a part of packet data
When audio and music units have some quirks in their sequence of packet, it's really hard for non-owners to identify the quirks. To handle the quirks, developers need a dump for sequence of packets at least, however it's difficult to users who have no knowledge and equipment for this purpose.
This commit adds tracepoints for this situation. When users encounter the issue, they can dump a part of packet data via Linux tracing framework as long as using drivers in ALSA firewire stack.
This commit adds 'snd-firewire-lib' subsystem with 'in_packet' and 'out_packet' events. In the events, the content of CIP headers, the number of included quadlets and the index of packet managed by this module are recorded per packet.
This is a sample:
$ trace-cmd record -e snd_firewire_lib:out_packet -e snd_firewire_lib:in_packet /sys/kernel/tracing/events/snd_firewire_lib/out_packet/filter /sys/kernel/tracing/events/snd_firewire_lib/in_packet/filter Hit Ctrl^C to stop recording ^C $ trace-cmd report trace.dat ... <idle>-0 [002] 40398.221702: out_packet: 00070008 9001427a: 058: 31 <idle>-0 [002] 40398.221703: in_packet: 01020010 9001ffff: 002: 15 <idle>-0 [002] 40398.221703: out_packet: 00070010 9001ffff: 002: 32 <idle>-0 [002] 40398.223679: in_packet: 010b0010 900157e4: 090: 16 <idle>-0 [002] 40398.223681: out_packet: 00070010 900157e4: 058: 33 <idle>-0 [002] 40398.223681: in_packet: 010b0018 9001714f: 090: 17 ...
One line represent one packet. The legend for the last four fields is: - The first quadlet of CIP header - The second quadlet of CIP header - The number of included quadlets - The index of packet inner buffer maintained by this module
Currently, when detecting packet discontinuity, this module stops packet streaming. This is reasonable to packet streaming implementation. However, to identify the quirks, packet streaming should continue to dump enough sequence of packet. This commit adds a condition statement and a marker for this purpose. --- sound/firewire/Makefile | 3 ++ sound/firewire/amdtp-stream-trace.h | 68 +++++++++++++++++++++++++++++++++++++ sound/firewire/amdtp-stream.c | 17 +++++++++- 3 files changed, 87 insertions(+), 1 deletion(-) create mode 100644 sound/firewire/amdtp-stream-trace.h
diff --git a/sound/firewire/Makefile b/sound/firewire/Makefile index 003c090..0ee1fb1 100644 --- a/sound/firewire/Makefile +++ b/sound/firewire/Makefile @@ -1,3 +1,6 @@ +# To find a header included by define_trace.h. +CFLAGS_amdtp-stream.o := -I$(src) + snd-firewire-lib-objs := lib.o iso-resources.o packets-buffer.o \ fcp.o cmp.o amdtp-stream.o amdtp-am824.o snd-isight-objs := isight.o diff --git a/sound/firewire/amdtp-stream-trace.h b/sound/firewire/amdtp-stream-trace.h new file mode 100644 index 0000000..316d495 --- /dev/null +++ b/sound/firewire/amdtp-stream-trace.h @@ -0,0 +1,68 @@ +/* + * amdtp-stream-trace.h - Linux tracing application for ALSA AMDTP engine + * + * Copyright (c) 2016 Takashi Sakamoto + * Licensed under the terms of the GNU General Public License, version 2. + */ + +#undef TRACE_SYSTEM +#define TRACE_SYSTEM snd_firewire_lib + +#if !defined(_AMDTP_STREAM_TRACE_H) || defined(TRACE_HEADER_MULTI_READ) +#define _AMDTP_STREAM_TRACE_H + +#include <linux/tracepoint.h> + +TRACE_EVENT(in_packet, + TP_PROTO(u32 cip_header0, u32 cip_header1, unsigned int payload_quadlets, unsigned int index), + TP_ARGS(cip_header0, cip_header1, payload_quadlets, index), + TP_STRUCT__entry( + __field(u32, cip_header0) + __field(u32, cip_header1) + __field(unsigned int, payload_quadlets) + __field(unsigned int, index) + ), + TP_fast_assign( + __entry->cip_header0 = cip_header0; + __entry->cip_header1 = cip_header1; + __entry->payload_quadlets = payload_quadlets; + __entry->index = index; + ), + TP_printk( + "%08x %08x: %03u: %02u", + __entry->cip_header0, + __entry->cip_header1, + __entry->payload_quadlets, + __entry->index) +); + +TRACE_EVENT(out_packet, + TP_PROTO(u32 cip_header0, u32 cip_header1, unsigned int payload_quadlets, unsigned int index), + TP_ARGS(cip_header0, cip_header1, payload_quadlets, index), + TP_STRUCT__entry( + __field(u32, cip_header0) + __field(u32, cip_header1) + __field(unsigned int, payload_quadlets) + __field(unsigned int, index) + ), + TP_fast_assign( + __entry->cip_header0 = cip_header0; + __entry->cip_header1 = cip_header1; + __entry->payload_quadlets = payload_quadlets; + __entry->index = index; + ), + TP_printk( + "%08x %08x: %03u: %02u", + __entry->cip_header0, + __entry->cip_header1, + __entry->payload_quadlets, + __entry->index) +); + +#endif + +#undef TRACE_INCLUDE_PATH +#define TRACE_INCLUDE_PATH . +#undef TRACE_INCLUDE_FILE +#define TRACE_INCLUDE_FILE amdtp-stream-trace +#include <trace/define_trace.h> diff --git a/sound/firewire/amdtp-stream.c b/sound/firewire/amdtp-stream.c index ed29026..152375f 100644 --- a/sound/firewire/amdtp-stream.c +++ b/sound/firewire/amdtp-stream.c @@ -15,6 +15,10 @@ #include <sound/pcm_params.h> #include "amdtp-stream.h"
+/* Always support tracepoints enabled by Linux tracing subsystem. */ +#define CREATE_TRACE_POINTS +#include "amdtp-stream-trace.h" + #define TICKS_PER_CYCLE 3072 #define CYCLES_PER_SECOND 8000 #define TICKS_PER_SECOND (TICKS_PER_CYCLE * CYCLES_PER_SECOND) @@ -426,6 +430,10 @@ static int 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; + + trace_out_packet(be32_to_cpu(buffer[0]), be32_to_cpu(buffer[1]), + payload_length / 4, s->packet_index); + if (queue_out_packet(s, payload_length, false) < 0) return -EIO;
@@ -451,6 +459,9 @@ static int handle_in_packet(struct amdtp_stream *s, cip_header[0] = be32_to_cpu(buffer[0]); cip_header[1] = be32_to_cpu(buffer[1]);
+ trace_in_packet(cip_header[0], cip_header[1], payload_quadlets, + s->packet_index); + /* * This module supports 'Two-quadlet CIP header with SYT field'. * For convenience, also check FMT field is AM824 or not. @@ -523,7 +534,11 @@ static int handle_in_packet(struct amdtp_stream *s, dev_err(&s->unit->device, "Detect discontinuity of CIP: %02X %02X\n", s->data_block_counter, data_block_counter); - return -EIO; + if (!trace_in_packet_enabled()) + return -EIO; + + /* To identifying this situation. */ + trace_in_packet(0xffffffff, 0xffffffff, 999, 99); }
pcm_frames = s->process_data_blocks(s, buffer + 2, *data_blocks, &syt);
On Sun, 27 Mar 2016 14:18:06 +0200, Takashi Sakamoto wrote:
When audio and music units have some quirks in their sequence of packet, it's really hard for non-owners to identify the quirks. To handle the quirks, developers need a dump for sequence of packets at least, however it's difficult to users who have no knowledge and equipment for this purpose.
This commit adds tracepoints for this situation. When users encounter the issue, they can dump a part of packet data via Linux tracing framework as long as using drivers in ALSA firewire stack.
This commit adds 'snd-firewire-lib' subsystem with 'in_packet' and 'out_packet' events. In the events, the content of CIP headers, the number of included quadlets and the index of packet managed by this module are recorded per packet.
This is a sample:
$ trace-cmd record -e snd_firewire_lib:out_packet -e snd_firewire_lib:in_packet /sys/kernel/tracing/events/snd_firewire_lib/out_packet/filter /sys/kernel/tracing/events/snd_firewire_lib/in_packet/filter Hit Ctrl^C to stop recording ^C $ trace-cmd report trace.dat ... <idle>-0 [002] 40398.221702: out_packet: 00070008 9001427a: 058: 31 <idle>-0 [002] 40398.221703: in_packet: 01020010 9001ffff: 002: 15 <idle>-0 [002] 40398.221703: out_packet: 00070010 9001ffff: 002: 32 <idle>-0 [002] 40398.223679: in_packet: 010b0010 900157e4: 090: 16 <idle>-0 [002] 40398.223681: out_packet: 00070010 900157e4: 058: 33 <idle>-0 [002] 40398.223681: in_packet: 010b0018 9001714f: 090: 17 ...
One line represent one packet. The legend for the last four fields is:
- The first quadlet of CIP header
- The second quadlet of CIP header
- The number of included quadlets
- The index of packet inner buffer maintained by this module
Currently, when detecting packet discontinuity, this module stops packet streaming. This is reasonable to packet streaming implementation. However, to identify the quirks, packet streaming should continue to dump enough sequence of packet. This commit adds a condition statement and a marker for this purpose.
The packet tracing is nice, and I don't see any reason against it. But changing the driver behavior depending on the trace state is unusual, rather a thing to be avoided in general.
thanks,
Takashi
Hi,
On Mar 29 2016 22:54, Takashi Iwai wrote:
On Sun, 27 Mar 2016 14:18:06 +0200, Takashi Sakamoto wrote:
When audio and music units have some quirks in their sequence of packet, it's really hard for non-owners to identify the quirks. To handle the quirks, developers need a dump for sequence of packets at least, however it's difficult to users who have no knowledge and equipment for this purpose.
This commit adds tracepoints for this situation. When users encounter the issue, they can dump a part of packet data via Linux tracing framework as long as using drivers in ALSA firewire stack.
This commit adds 'snd-firewire-lib' subsystem with 'in_packet' and 'out_packet' events. In the events, the content of CIP headers, the number of included quadlets and the index of packet managed by this module are recorded per packet.
This is a sample:
$ trace-cmd record -e snd_firewire_lib:out_packet -e snd_firewire_lib:in_packet /sys/kernel/tracing/events/snd_firewire_lib/out_packet/filter /sys/kernel/tracing/events/snd_firewire_lib/in_packet/filter Hit Ctrl^C to stop recording ^C $ trace-cmd report trace.dat ... <idle>-0 [002] 40398.221702: out_packet: 00070008 9001427a: 058: 31 <idle>-0 [002] 40398.221703: in_packet: 01020010 9001ffff: 002: 15 <idle>-0 [002] 40398.221703: out_packet: 00070010 9001ffff: 002: 32 <idle>-0 [002] 40398.223679: in_packet: 010b0010 900157e4: 090: 16 <idle>-0 [002] 40398.223681: out_packet: 00070010 900157e4: 058: 33 <idle>-0 [002] 40398.223681: in_packet: 010b0018 9001714f: 090: 17 ...
One line represent one packet. The legend for the last four fields is:
- The first quadlet of CIP header
- The second quadlet of CIP header
- The number of included quadlets
- The index of packet inner buffer maintained by this module
Currently, when detecting packet discontinuity, this module stops packet streaming. This is reasonable to packet streaming implementation. However, to identify the quirks, packet streaming should continue to dump enough sequence of packet. This commit adds a condition statement and a marker for this purpose.
The packet tracing is nice, and I don't see any reason against it. But changing the driver behavior depending on the trace state is unusual, rather a thing to be avoided in general.
See already-identified quirks: - 13f3a46d2a6c('ALSA: oxfw: add Mackie Onyx Satellite quirk to inform wrong value in 'dbs' field in tx CIP header') - 18f5ed365d3f('ALSA: fireworks/firewire-lib: add support for recent firmware quirk') - c4d860a0d256('ALSA: bebob: loosen up severity of checking continuity for BeBoB v3 quirk') - 9d59124cacf5('ALSA: bebob/firewire-lib: Add a quirk of wrong dbc in empty packet for M-Audio special Firewire series') - d9cd0065c8a4('ALSA: fireworks/firewire-lib: Add a quirk for fixed interval of reported dbc') - c8bdf49b9935('ALSA: fireworks/firewire-lib: Add a quirk for the meaning of dbc')
To investigate quirks similar to these, the sequence of packet before/after detecting discontinuity helps us. It's a reason to change the behaviour against the general issue.
Regards
Takashi Sakamoto
On Wed, 30 Mar 2016 02:47:06 +0200, Takashi Sakamoto wrote:
Hi,
On Mar 29 2016 22:54, Takashi Iwai wrote:
On Sun, 27 Mar 2016 14:18:06 +0200, Takashi Sakamoto wrote:
When audio and music units have some quirks in their sequence of packet, it's really hard for non-owners to identify the quirks. To handle the quirks, developers need a dump for sequence of packets at least, however it's difficult to users who have no knowledge and equipment for this purpose.
This commit adds tracepoints for this situation. When users encounter the issue, they can dump a part of packet data via Linux tracing framework as long as using drivers in ALSA firewire stack.
This commit adds 'snd-firewire-lib' subsystem with 'in_packet' and 'out_packet' events. In the events, the content of CIP headers, the number of included quadlets and the index of packet managed by this module are recorded per packet.
This is a sample:
$ trace-cmd record -e snd_firewire_lib:out_packet -e snd_firewire_lib:in_packet /sys/kernel/tracing/events/snd_firewire_lib/out_packet/filter /sys/kernel/tracing/events/snd_firewire_lib/in_packet/filter Hit Ctrl^C to stop recording ^C $ trace-cmd report trace.dat ... <idle>-0 [002] 40398.221702: out_packet: 00070008 9001427a: 058: 31 <idle>-0 [002] 40398.221703: in_packet: 01020010 9001ffff: 002: 15 <idle>-0 [002] 40398.221703: out_packet: 00070010 9001ffff: 002: 32 <idle>-0 [002] 40398.223679: in_packet: 010b0010 900157e4: 090: 16 <idle>-0 [002] 40398.223681: out_packet: 00070010 900157e4: 058: 33 <idle>-0 [002] 40398.223681: in_packet: 010b0018 9001714f: 090: 17 ...
One line represent one packet. The legend for the last four fields is:
- The first quadlet of CIP header
- The second quadlet of CIP header
- The number of included quadlets
- The index of packet inner buffer maintained by this module
Currently, when detecting packet discontinuity, this module stops packet streaming. This is reasonable to packet streaming implementation. However, to identify the quirks, packet streaming should continue to dump enough sequence of packet. This commit adds a condition statement and a marker for this purpose.
The packet tracing is nice, and I don't see any reason against it. But changing the driver behavior depending on the trace state is unusual, rather a thing to be avoided in general.
See already-identified quirks:
- 13f3a46d2a6c('ALSA: oxfw: add Mackie Onyx Satellite quirk to inform
wrong value in 'dbs' field in tx CIP header')
- 18f5ed365d3f('ALSA: fireworks/firewire-lib: add support for recent
firmware quirk')
- c4d860a0d256('ALSA: bebob: loosen up severity of checking continuity
for BeBoB v3 quirk')
- 9d59124cacf5('ALSA: bebob/firewire-lib: Add a quirk of wrong dbc in
empty packet for M-Audio special Firewire series')
- d9cd0065c8a4('ALSA: fireworks/firewire-lib: Add a quirk for fixed
interval of reported dbc')
- c8bdf49b9935('ALSA: fireworks/firewire-lib: Add a quirk for the
meaning of dbc')
To investigate quirks similar to these, the sequence of packet before/after detecting discontinuity helps us. It's a reason to change the behaviour against the general issue.
Yeah, I know the reason. But my point is that tracing is only for showing the state of the code flow, and it's not a debug flag to change the code flow itself. It's unusual that the driver behavior depends on the tracing state.
That being said, you should show that this driver behavior change won't affect any other device behavior, i.e. it won't be any cause of heisenbugs.
thanks,
Takashi
Hi,
On 2016年03月30日 14:27, Takashi Iwai wrote:
On Wed, 30 Mar 2016 02:47:06 +0200, Takashi Sakamoto wrote:
Hi,
On Mar 29 2016 22:54, Takashi Iwai wrote:
On Sun, 27 Mar 2016 14:18:06 +0200, Takashi Sakamoto wrote:
When audio and music units have some quirks in their sequence of packet, it's really hard for non-owners to identify the quirks. To handle the quirks, developers need a dump for sequence of packets at least, however it's difficult to users who have no knowledge and equipment for this purpose.
This commit adds tracepoints for this situation. When users encounter the issue, they can dump a part of packet data via Linux tracing framework as long as using drivers in ALSA firewire stack.
This commit adds 'snd-firewire-lib' subsystem with 'in_packet' and 'out_packet' events. In the events, the content of CIP headers, the number of included quadlets and the index of packet managed by this module are recorded per packet.
This is a sample:
$ trace-cmd record -e snd_firewire_lib:out_packet -e snd_firewire_lib:in_packet /sys/kernel/tracing/events/snd_firewire_lib/out_packet/filter /sys/kernel/tracing/events/snd_firewire_lib/in_packet/filter Hit Ctrl^C to stop recording ^C $ trace-cmd report trace.dat ... <idle>-0 [002] 40398.221702: out_packet: 00070008 9001427a: 058: 31 <idle>-0 [002] 40398.221703: in_packet: 01020010 9001ffff: 002: 15 <idle>-0 [002] 40398.221703: out_packet: 00070010 9001ffff: 002: 32 <idle>-0 [002] 40398.223679: in_packet: 010b0010 900157e4: 090: 16 <idle>-0 [002] 40398.223681: out_packet: 00070010 900157e4: 058: 33 <idle>-0 [002] 40398.223681: in_packet: 010b0018 9001714f: 090: 17 ...
One line represent one packet. The legend for the last four fields is:
- The first quadlet of CIP header
- The second quadlet of CIP header
- The number of included quadlets
- The index of packet inner buffer maintained by this module
Currently, when detecting packet discontinuity, this module stops packet streaming. This is reasonable to packet streaming implementation. However, to identify the quirks, packet streaming should continue to dump enough sequence of packet. This commit adds a condition statement and a marker for this purpose.
The packet tracing is nice, and I don't see any reason against it. But changing the driver behavior depending on the trace state is unusual, rather a thing to be avoided in general.
See already-identified quirks:
- 13f3a46d2a6c('ALSA: oxfw: add Mackie Onyx Satellite quirk to inform
wrong value in 'dbs' field in tx CIP header')
- 18f5ed365d3f('ALSA: fireworks/firewire-lib: add support for recent
firmware quirk')
- c4d860a0d256('ALSA: bebob: loosen up severity of checking continuity
for BeBoB v3 quirk')
- 9d59124cacf5('ALSA: bebob/firewire-lib: Add a quirk of wrong dbc in
empty packet for M-Audio special Firewire series')
- d9cd0065c8a4('ALSA: fireworks/firewire-lib: Add a quirk for fixed
interval of reported dbc')
- c8bdf49b9935('ALSA: fireworks/firewire-lib: Add a quirk for the
meaning of dbc')
To investigate quirks similar to these, the sequence of packet before/after detecting discontinuity helps us. It's a reason to change the behaviour against the general issue.
Yeah, I know the reason. But my point is that tracing is only for showing the state of the code flow, and it's not a debug flag to change the code flow itself. It's unusual that the driver behavior depends on the tracing state.
That being said, you should show that this driver behavior change won't affect any other device behavior, i.e. it won't be any cause of heisenbugs.
No uncertainty there is, even if the return statement is skipped. For me, it has been one of ways to gather packet dumps (with printk).
(But near future, Königsberg 1931, by myself.)
Regards
Takashi Sakamoto
On 27.03.2016 14:18, Takashi Sakamoto wrote:
/* * This module supports 'Two-quadlet CIP header with SYT field'. * For convenience, also check FMT field is AM824 or not. @@ -523,7 +534,11 @@ static int handle_in_packet(struct amdtp_stream *s, dev_err(&s->unit->device, "Detect discontinuity of CIP: %02X %02X\n", s->data_block_counter, data_block_counter);
return -EIO;
if (!trace_in_packet_enabled())return -EIO;/* To identifying this situation. */trace_in_packet(0xffffffff, 0xffffffff, 999, 99);
This looks wrong. Tracing should not change the path taken.
cheers, daniel
participants (3)
-
Daniel Wagner -
Takashi Iwai -
Takashi Sakamoto