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@xxxxxxxxxxxxx> >> 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@xxxxxxxxxxxxxxxx >> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel > _______________________________________________ Alsa-devel mailing list Alsa-devel@xxxxxxxxxxxxxxxx http://mailman.alsa-project.org/mailman/listinfo/alsa-devel