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