| Message ID | 1431775365-25211-2-git-send-email-o-takashi@sakamocchi.jp (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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
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
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
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; }
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(-)