Hi,
On May 11 2016 00:03, Takashi Iwai wrote:
On Tue, 10 May 2016 16:07:50 +0200, Takashi Sakamoto wrote:
Hi Clemens,
On 2016年05月07日 21:46, Takashi Sakamoto wrote:
Could I request your comment to this patch? It is to solve a race condition, but the condition is quite rare. Practically, it might have a less meanings, except for better program.
And I think there's another race condition against processing each packets by calling out/in_stream_callback(), but I cannot observe the race. Software IRQ contexts of IR/IT contexts and process contexts are under the race condition, however I can see no problems related to it in my several trials in multi-core machine. I have no idea about the reason that packet sequence is processed correctly between the software IRQ contexts and the process contexts without any lock primitives. Do you have some ideas about it?
I wrote an additional patch for this race issue. Would you please read this, too?
It's just a flag indicating of a busy task, right? If so, it doesn't have to be a spinlock, but a simple atomic_t.
This function is called in both of software IRQ context and process context. Thus, atomic_t causes kernel hungs in software IRQ context, because We cannot call kernel APIs which call process scheduler in the context.
For supplemental information, please refer to a patch which I posted just now. Unfortunately, alsa-project.org doesn't blast the message as of now... You can also see the message in an archive of ffado-devel. https://sourceforge.net/p/ffado/mailman/message/35078341/
Thanks
Takashi Sakamoto
thanks,
Takashi
Regards
----- 8< -----
From d2090cac868e718227596dbda31ea6333b72009c Mon Sep 17 00:00:00 2001 From: Takashi Sakamoto o-takashi@sakamocchi.jp Date: Tue, 10 May 2016 22:02:05 +0900 Subject: [PATCH] firewire-lib: add locking for packet processing
When packet streaming starts, packet processing is done in software IRQ context of 1394 OHCI IR/IT contexts. This is a typical way. On the other hand, process context of PCM application can also process packets in a path to handle PCM frames. This is for better PCM pointer granularity. The two execution context causes race condition against packet processing.
When the race occurs, it's enough that just one of these two contexts handles packet processing, because actual time dominates packet queueing.
This commit adds spin lock to manage the race condition. When the race occurs, second context returns immediately from critical section. Thus, it has little overhead.
sound/firewire/amdtp-stream.c | 39 ++++++++++++++++++++++++++++++--------- sound/firewire/amdtp-stream.h | 3 +++ 2 files changed, 33 insertions(+), 9 deletions(-)
diff --git a/sound/firewire/amdtp-stream.c b/sound/firewire/amdtp-stream.c index 92d5a16..80d5887 100644 --- a/sound/firewire/amdtp-stream.c +++ b/sound/firewire/amdtp-stream.c @@ -601,6 +601,14 @@ static void out_stream_callback(struct fw_iso_context *context, u32 tstamp, if (s->packet_index < 0) return;
/*
* It's enough for queued packets to be handled by process context of* PCM application or software IRQ context of 1394 OHCI IT context in a* time.*/if (!spin_trylock(&s->lock_packetization))
return;cycle = compute_cycle_count(tstamp);
/* Align to actual cycle count for the last packet. */
@@ -608,14 +616,18 @@ static void out_stream_callback(struct fw_iso_context *context, u32 tstamp,
for (i = 0; i < packets; ++i) { cycle = increment_cycle_count(cycle, 1);
if (handle_out_packet(s, cycle, i) < 0) {s->packet_index = -1;amdtp_stream_pcm_abort(s);return;}
if (handle_out_packet(s, cycle, i) < 0)break;
- fw_iso_context_queue_flush(s->context);
- if (i == packets) {
fw_iso_context_queue_flush(s->context);- } else {
s->packet_index = -1;amdtp_stream_pcm_abort(s);- }
- spin_unlock(&s->lock_packetization);
}
static void in_stream_callback(struct fw_iso_context *context, u32 tstamp, @@ -631,6 +643,14 @@ static void in_stream_callback(struct fw_iso_context *context, u32 tstamp, if (s->packet_index < 0) return;
- /*
* It's enough for queued packets to be handled by process context of* PCM application or software IRQ context of 1394 OHCI IR context in a* time.*/- if (!spin_trylock(&s->lock_packetization))
return;- /* The number of packets in buffer */ packets = header_length / IN_PACKET_HEADER_SIZE;
@@ -660,13 +680,14 @@ static void in_stream_callback(struct fw_iso_context *context, u32 tstamp, }
/* Queueing error or detecting invalid payload. */
- if (i < packets) {
- if (i == packets) {
fw_iso_context_queue_flush(s->context);- } else { s->packet_index = -1; amdtp_stream_pcm_abort(s);
return;}
fw_iso_context_queue_flush(s->context);
- spin_unlock(&s->lock_packetization);
}
/* this is executed one time */ diff --git a/sound/firewire/amdtp-stream.h b/sound/firewire/amdtp-stream.h index c1bc7fa..6be5feb 100644 --- a/sound/firewire/amdtp-stream.h +++ b/sound/firewire/amdtp-stream.h @@ -102,6 +102,9 @@ struct amdtp_stream { struct iso_packets_buffer buffer; int packet_index;
- /* Packet processing can run in one context at a time. */
- spinlock_t lock_packetization;
- /* For CIP headers. */ unsigned int source_node_id_field; unsigned int data_block_quadlets;
-- 2.7.4 _______________________________________________ Alsa-devel mailing list Alsa-devel@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/alsa-devel