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.

But, Behringer F-Control Audio 202 (based on OXFW 970) has a quirk to postpone transferring isochronous packet till finish handling any asynchronous packets. In this case, this model is lazy, transfers no packets during 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 | 15 ++++++++++++++- sound/firewire/amdtp.h | 4 ++++ sound/firewire/oxfw/oxfw-stream.c | 10 ++++++++-- 3 files changed, 26 insertions(+), 3 deletions(-)

diff --git a/sound/firewire/amdtp.c b/sound/firewire/amdtp.c index e061355..6424382 100644 --- a/sound/firewire/amdtp.c +++ b/sound/firewire/amdtp.c @@ -251,7 +251,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_DATA_BLOCKS) + multiplier = 5; + + return 8 + s->syt_interval * s->data_block_quadlets * 4 * multiplier; } EXPORT_SYMBOL(amdtp_stream_get_max_payload);

@@ -687,6 +692,14 @@ static void handle_in_packet(struct amdtp_stream *s, struct snd_pcm_substream *pcm = NULL; bool lost;

+ /* Protect from buffer-over-run. */ + if (payload_quadlets > amdtp_stream_get_max_payload(s)) { + dev_info(&s->unit->device, + "Too many data blocks in a packet: %02X %02X\n", + amdtp_stream_get_max_payload(s), payload_quadlets); + goto err; + } + cip_header[0] = be32_to_cpu(buffer[0]); cip_header[1] = be32_to_cpu(buffer[1]);

diff --git a/sound/firewire/amdtp.h b/sound/firewire/amdtp.h index 8a03a91..6e06203 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_DATA_BLOCKS: Only for in-stream. The number of data blocks in an + * packet is sometimes 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_DATA_BLOCKS = 0x100, };

/** diff --git a/sound/firewire/oxfw/oxfw-stream.c b/sound/firewire/oxfw/oxfw-stream.c index e6757cd..74ec2dc 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_DATA_BLOCKS; end: return err; }

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 | 44 ++++++++++++++++++++++++++++---------------- 1 file changed, 28 insertions(+), 16 deletions(-)

diff --git a/sound/firewire/amdtp.c b/sound/firewire/amdtp.c index e3f14a8..5892a36 100644 --- a/sound/firewire/amdtp.c +++ b/sound/firewire/amdtp.c @@ -647,17 +647,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) | @@ -687,13 +686,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 = 0; + unsigned int data_block_quadlets, data_block_counter, dbc_interval; struct snd_pcm_substream *pcm = NULL; bool lost;

@@ -793,10 +791,12 @@ end: if (pcm) update_pcm_pointers(s, pcm, data_blocks);

- return; + return data_blocks; err: s->packet_index = -1; amdtp_stream_pcm_abort(s); + + return -EIO; }

static void out_stream_callback(struct fw_iso_context *context, u32 cycle, @@ -805,6 +805,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. @@ -815,7 +816,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); } @@ -826,6 +829,7 @@ static void in_stream_callback(struct fw_iso_context *context, u32 cycle, { struct amdtp_stream *s = private_data; unsigned int p, syt, packets, payload_quadlets; + unsigned int data_blocks; __be32 *buffer, *headers = header;

/* The number of packets in buffer */ @@ -837,20 +841,28 @@ static void in_stream_callback(struct fw_iso_context *context, u32 cycle,

buffer = s->buffer.packets[s->packet_index].buffer;

+ /* The number of quadlets in this packet */ + payload_quadlets = + (be32_to_cpu(headers[p]) >> ISO_DATA_LENGTH_SHIFT) / 4; + + 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, syt); + handle_out_packet(s->sync_slave, data_blocks, syt); }

- /* The number of quadlets in this packet */ - payload_quadlets = - (be32_to_cpu(headers[p]) >> ISO_DATA_LENGTH_SHIFT) / 4; - handle_in_packet(s, payload_quadlets, buffer); }

/* 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;

On May 16 2015 20:22, Takashi Sakamoto wrote:

But, Behringer F-Control Audio 202 (based on OXFW 970) has a quirk to postpone transferring isochronous packet till finish handling any asynchronous packets. In this case, this model is lazy, transfers no packets during 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.

This is an actual packet dump. We can see this model postpone transferring packets during handling asynchronous transaction.

FYI

-- Time expressed in clock-ticks of 10.172526 nSec 19657542078 CycleStart dst=0xFFFF label=0 rcode=retry_1 src=0xFFC3 offset=0xFFFFF0000200 cycle_time_data=0x58F3F024 speed=100 19657546326 Streaming length=40 tag=1 channel=0 synchronization=0 speed=400 0000: 02020072 900002E4 40FFFF8B 40000005 [...r....@...@...] 0010: 40FFFFD8 40FFFFFD 40FFFF37 40FFFFA5 [@...@...@..7@...] 0020: 40FFFE81 40FFFF2F [@...@../] 19657554363 CycleStart dst=0xFFFF label=0 rcode=retry_1 src=0xFFC3 offset=0xFFFFF0000200 cycle_time_data=0x58F40024 speed=100 19657559477 Streaming length=40 tag=1 channel=0 synchronization=0 speed=400 0000: 02020076 90000351 40FFFF1C 40FFFF95 [...v...Q@...@...] 0010: 40FFFF01 40FFFEF6 40FFFF2E 40FFFF77 [@...@...@...@..w] 0020: 40FFFEE9 40FFFF72 [@...@..r] 19657566647 CycleStart dst=0xFFFF label=0 rcode=retry_1 src=0xFFC3 offset=0xFFFFF0000200 cycle_time_data=0x58F41024 speed=100 19657570253 Streaming length=40 tag=1 channel=0 synchronization=0 speed=400 0000: 0202007A 90000244 40FFFF11 40FFFF80 [...z...D@...@...] 0010: 40FFFF2A 40FFFFA7 40FFFF40 40FFFF71 [@..*@...@..@@..q] 0020: 40FFFF0E 40FFFFB1 [@...@...] 19657578933 CycleStart dst=0xFFFF label=0 rcode=retry_1 src=0xFFC3 offset=0xFFFFF0000200 cycle_time_data=0x58F42024 speed=100 19657582987 Streaming length=40 tag=1 channel=0 synchronization=0 speed=400 0000: 0202007E 900002B4 40FFFF8C 40FFFFC5 [...~....@...@...] 0010: 40FFFF79 40FFFFBB 40FFFFDE 40FFFFE0 [@..y@...@...@...] 0020: 40FFFFF2 40000031 [@...@..1] 19657591217 CycleStart dst=0xFFFF label=0 rcode=retry_1 src=0xFFC3 offset=0xFFFFF0000200 cycle_time_data=0x58F43024 speed=100 19657595721 Streaming length=40 tag=1 channel=0 synchronization=0 speed=400 0000: 02020082 90000324 4000004C 4000004B [.......$@..L@..K] 0010: 40000054 40FFFFF9 4000004D 40FFFFFE [@..T@...@..M@...] 0020: 40000053 4000003F [@..S@..?] 19657600127 ReadReq dst=0xFFC2 label=36 rcode=retry_X src=0xFFC3 offset=0xFFFFF0000904 speed=400 ack=ack_pending 19657603503 CycleStart dst=0xFFFF label=0 rcode=retry_1 src=0xFFC3 offset=0xFFFFF0000200 cycle_time_data=0x58F44024 speed=100 19657615788 CycleStart dst=0xFFFF label=0 rcode=retry_1 src=0xFFC3 offset=0xFFFFF0000200 cycle_time_data=0x58F45024 speed=100 19657628072 CycleStart dst=0xFFFF label=0 rcode=retry_1 src=0xFFC3 offset=0xFFFFF0000200 cycle_time_data=0x58F46024 speed=100 19657633918 ReadResp dst=0xFFC3 label=36 rcode=retry_X src=0xFFC2 response=resp_complete data=0x81008042 speed=400 ack=ack_complete 19657640358 CycleStart dst=0xFFFF label=0 rcode=retry_1 src=0xFFC3 offset=0xFFFFF0000200 cycle_time_data=0x58F47024 speed=100 19657644228 Streaming length=136 tag=1 channel=0 synchronization=0 speed=400 0000: 02020086 90000257 40FFFFE9 4000003C [.......W@...@..<] 0010: 40000000 40FFFFE2 40FFFFDF 40FFFFD9 [@...@...@...@...] 0020: 40000010 40000024 40FFFF6E 4000000B [@...@..$@..n@...] 0030: 40FFFFC7 40FFFFE8 40FFFFE6 40FFFFAF [@...@...@...@...] 0040: 40FFFFE1 4000002A 40000039 4000004A [@...@..*@..9@..J] 0050: 40000055 40000043 40000091 400000C0 [@..U@..C@...@...] 0060: 40000089 40000010 40FFFFEF 40FFFFE7 [@...@...@...@...] 0070: 40000036 4000001D 40FFFFF8 40FFFFD5 [@..6@...@...@...] 0080: 4000001C 40000014 [@...@...] 19657652642 CycleStart dst=0xFFFF label=0 rcode=retry_1 src=0xFFC3 offset=0xFFFFF0000200 cycle_time_data=0x58F48024 speed=100 19657657066 Streaming length=40 tag=1 channel=0 synchronization=0 speed=400 0000: 02020096 900002F1 40FFFFC2 40FFFF9F [........@...@...] 0010: 40FFFFC1 4000000A 4000002A 40000018 [@...@...@..*@...] 0020: 40FFFFA5 40FFFFD5 [@...@...] 19657664928 CycleStart dst=0xFFFF label=0 rcode=retry_1 src=0xFFC3 offset=0xFFFFF0000200 cycle_time_data=0x58F49024 speed=100 19657670106 Streaming length=40 tag=1 channel=0 synchronization=0 speed=400 0000: 0202009A 90000380 40000044 40000053 [........@..D@..S] 0010: 40000050 40000046 40000086 4000003A [@..P@..F@...@..:] 0020: 4000006D 4000002D [@..m@..-] 19657677213 CycleStart dst=0xFFFF label=0 rcode=retry_1 src=0xFFC3 offset=0xFFFFF0000200 cycle_time_data=0x58F4A024 speed=100 19657680881 Streaming length=40 tag=1 channel=0 synchronization=0 speed=400 0000: 0202009E 90000253 4000006A 40000047 [.......S@..j@..G] 0010: 40000056 4000000A 40000074 4000001F [@..V@...@..t@...] 0020: 4000004A 4000004A [@..J@..J] 19657689497 CycleStart dst=0xFFFF label=0 rcode=retry_1 src=0xFFC3 offset=0xFFFFF0000200 cycle_time_data=0x58F4B024 speed=100 19657693615 Streaming length=40 tag=1 channel=0 synchronization=0 speed=400 0000: 020200A2 900002C4 40000030 40000014 [........@..0@...] 0010: 4000002F 4000002D 40000013 4000001A [@../@..-@...@...] 0020: 4000002F 40FFFFBE [@../@...]

Regards

Takashi Sakamoto

On May 18 2015 21:54, Takashi Iwai wrote:

At Sat, 16 May 2015 20:22:42 +0900, Takashi Sakamoto wrote:

...

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.

But, Behringer F-Control Audio 202 (based on OXFW 970) has a quirk to postpone transferring isochronous packet till finish handling any asynchronous packets. In this case, this model is lazy, transfers no packets during 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 | 15 ++++++++++++++- sound/firewire/amdtp.h | 4 ++++ sound/firewire/oxfw/oxfw-stream.c | 10 ++++++++-- 3 files changed, 26 insertions(+), 3 deletions(-)

diff --git a/sound/firewire/amdtp.c b/sound/firewire/amdtp.c index e061355..6424382 100644 --- a/sound/firewire/amdtp.c +++ b/sound/firewire/amdtp.c @@ -251,7 +251,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_DATA_BLOCKS)

• multiplier = 5;
  

• return 8 + s->syt_interval * s->data_block_quadlets * 4 * multiplier; } EXPORT_SYMBOL(amdtp_stream_get_max_payload);

@@ -687,6 +692,14 @@ static void handle_in_packet(struct amdtp_stream *s, struct snd_pcm_substream *pcm = NULL; bool lost;

• /* Protect from buffer-over-run. */
• if (payload_quadlets > amdtp_stream_get_max_payload(s)) {

• dev_info(&s->unit->device,
  

• 	 "Too many data blocks in a packet: %02X %02X\n",
  

• 	 amdtp_stream_get_max_payload(s), payload_quadlets);
  

• goto err;
  

How often may this message appear? My concern is the flood of such error message. Some messages are already with _ratelimited() for avoiding it.

When detecting such jumbo size of payload, the firewire-lib stops processing the isochronous stream packet. Therefore, the error messaging doesn't continue.

The firewire-lib also sets XRUN to the state of PCM substream. ALSA PCM applications can recover this state by calling snd_pcm_prepare() (or snd_pcm_recover()). But this operation starts new isochronous context. In this case, one message per several hundreds mili-seconds. So the error messaging doesn't continue such frequently.

For me, no floading issues occur to these codes.

By the way, I think it good to use dev_err() instead of dev_info() because drivers should not handle such devices which transfer packets with such jumbo payload. This should be reported to developers.

Additionally, amdtp_stream_get_max_payload() returns the same value during streaming. So it's no need to calculate every packet. Ideally, I should add new member to 'struct amdtp_stream' for this value, while the value should be re-calculated when stream is not running. So I want to move the code to in_stream_callback() and use stack to keep the value.

I'd like to keep this patchset pending till posting new one.

Thanks

Takashi Sakamoto