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@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
