| Message ID | 5731EB36.3070706@sakamocchi.jp (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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. 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
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 >
On Wed, 11 May 2016 00:56:39 +0200, Takashi Sakamoto wrote: > > 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. I guess you are confused. atomic ops can be safely used in any contexts. (I suggested atomic_t in the previous mail, but actually a lighter primitive for such a case is cmpxchg() and its variants. In anyway, all are found in atomic.h.) In your code, you never call spin_lock() but only spin_trylock(). That is, there is no part really spinning, but it's used only as a flag. So, it is equivalent with doing something like: if (cmpxchg(flag, 1)) /* concurrently running? */ return; do_something; cmpxchg(flag, 0); in both callbacks. Takashi > 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 > > >
On May 11 2016 14:35, Takashi Iwai wrote: > On Wed, 11 May 2016 00:56:39 +0200, > Takashi Sakamoto wrote: >> >> 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. > > I guess you are confused. atomic ops can be safely used in any > contexts. (I suggested atomic_t in the previous mail, but actually a > lighter primitive for such a case is cmpxchg() and its variants. In > anyway, all are found in atomic.h.) Yes. In this morning, I stupidly confused 'atomic_t' and 'struct mutex', sorry. (My brain might be still half awake...) > In your code, you never call spin_lock() but only spin_trylock(). > That is, there is no part really spinning, but it's used only as a > flag. So, it is equivalent with doing something like: > > if (cmpxchg(flag, 1)) /* concurrently running? */ > return; > do_something; > cmpxchg(flag, 0); > > in both callbacks. Yep. It's suitable in my case to use atomic_t. Well, I realize that this second patch is meaningless for its aim. I overlooked that 'tasklet_disable()' is called in 'ohci_flush_iso_completions()' against software IRQ of IR/IT context. http://git.kernel.org/cgit/linux/kernel/git/tiwai/sound.git/tree/drivers/firewire/ohci.c#n3477 The race condition between the software irq context and the process context (calling 'fw_iso_context_flush_completions()') is already intervened. So the packet processing is never executed in several contexts in a time. Now I'm back to the first patch. Thanks Takashi Sakamoto >> 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
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;