[PATCH 0/4] firewire-lib: purge restriction of synchronization for non-blocking mode

[PATCH 0/4] firewire-lib: purge restriction of synchronization for non-blocking mode

[Takashi Sakamoto]

This patchset allows non-blocking streams to use timestamp synchronization.

In full duplex synchronization mode, current implementation pass the value
of 'syt' field of CIP header in incoming packets to outgoing packet
processing, while the number of data blocks is not passed. Therefore,
this mode is allowed just to blocking streams, because of a lack of
validator for the number which actual devices transfers. As long as the
number is suspicious, it's not reused to outgoing packet processing.

To allow the synchronization mode to non-blocking streams, this patchset:
 * adds a validator for the number of data blocks in incoming packets
 * adds a flag still to support illegal models (i.e. Behringer FCA202)
 * pass the evaluated number from incoming packet processing to outgoing
 * remove the restriction


Takashi Sakamoto (4):
  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: remove restriction for non-blocking mode

 sound/firewire/amdtp.c            | 108 ++++++++++++++++++++++++--------------
 sound/firewire/amdtp.h            |   4 ++
 sound/firewire/oxfw/oxfw-stream.c |  10 +++-
 3 files changed, 81 insertions(+), 41 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

[PATCH 1/4] ALSA: firewire-lib: add buffer-over-run protection at receiving more data blocks than expected

[Takashi Sakamoto]

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;
 }
-- 
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

Behringer FCA 202 packet dump (Re: [PATCH 1/4] ALSA: firewire-lib: add buffer-over-run protection at receiving more data blocks than expected)

[Takashi Sakamoto]

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

Re: [PATCH 1/4] ALSA: firewire-lib: add buffer-over-run protection at receiving more data blocks than expected

[Takashi Iwai]

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.


Takashi

Re: [PATCH 1/4] ALSA: firewire-lib: add buffer-over-run protection at receiving more data blocks than expected

[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

------------------------------------------------------------------------------
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

Re: [PATCH 1/4] ALSA: firewire-lib: add buffer-over-run protection at receiving more data blocks than expected

[Takashi Iwai]

At Tue, 19 May 2015 09:25:56 +0900,
Takashi Sakamoto wrote:
> 
> 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.

Fair enough.

> 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.

Either dev_err() or dev_warn() would be suitable, yes.

> 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.

OK.  I'm going to apply this series once when I see no one objecting.


thanks,

Takashi

[PATCH 2/4] ALSA: firewire-lib: simplify function to calculate the number of data blocks

[Takashi Sakamoto]

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 6424382..e3f14a8 100644
--- a/sound/firewire/amdtp.c
+++ b/sound/firewire/amdtp.c
@@ -321,17 +321,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
@@ -341,16 +349,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;
@@ -647,11 +656,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

[PATCH 3/4] ALSA: firewire-lib: pass the number of data blocks in incoming packets to outgoing packets

[Takashi Sakamoto]

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

[PATCH 4/4] ALSA: firewire-lib: remove restriction for non-blocking mode

[Takashi Sakamoto]

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 5892a36..c5bdb62 100644
--- a/sound/firewire/amdtp.c
+++ b/sound/firewire/amdtp.c
@@ -902,7 +902,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