From d35da0e5474e3232fbfe0ae7584022efe2da7442 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C5=A0erif=20Rami?= Date: Wed, 6 Aug 2025 17:27:10 +0200 Subject: [PATCH] feat: Refactor and optimize TASCAM US-144MKII driver This commit includes: - Reverted audio stream decoupling to restore original behavior. - Implemented SysEx state machine and error retry for MIDI handling. - Performed memory leak audit and confirmed no obvious leaks. - Optimized playback and capture data copying for efficiency. - Reordered `tascam_card` structure for improved cache locality. - Fixed `active_urbs` counter management to prevent "URBs still active" errors. --- us144mkii.c | 20 ++++++--- us144mkii.h | 105 +++++++++++++++++++++++++------------------ us144mkii_capture.c | 14 +++--- us144mkii_midi.c | 30 ++++++++++--- us144mkii_pcm.c | 3 ++ us144mkii_pcm.h | 41 +++++++++++++++++ us144mkii_playback.c | 19 ++++---- 7 files changed, 161 insertions(+), 71 deletions(-) diff --git a/us144mkii.c b/us144mkii.c index db5d8f1..1e10b88 100644 --- a/us144mkii.c +++ b/us144mkii.c @@ -121,8 +121,7 @@ void tascam_free_urbs(struct tascam_card *tascam) { } } - kfree(tascam->playback_routing_buffer); - tascam->playback_routing_buffer = NULL; + kfree(tascam->capture_routing_buffer); tascam->capture_routing_buffer = NULL; kfree(tascam->capture_decode_dst_block); @@ -265,10 +264,7 @@ int tascam_alloc_urbs(struct tascam_card *tascam) { if (!tascam->capture_decode_dst_block) goto error; - tascam->playback_routing_buffer = - kmalloc(tascam->playback_urb_alloc_size, GFP_KERNEL); - if (!tascam->playback_routing_buffer) - goto error; + tascam->capture_routing_buffer = kmalloc(FRAMES_PER_DECODE_BLOCK * DECODED_CHANNELS_PER_FRAME * @@ -419,6 +415,15 @@ static int tascam_resume(struct usb_interface *intf) { return 0; } +static void tascam_error_timer(struct timer_list *t) { + struct tascam_card *tascam = from_timer(tascam, t, error_timer); + + if (atomic_read(&tascam->midi_in_active)) + schedule_work(&tascam->midi_in_work); + if (atomic_read(&tascam->midi_out_active)) + schedule_work(&tascam->midi_out_work); +} + /** * tascam_probe() - Probes for the TASCAM US-144MKII device. * @intf: The USB interface being probed. @@ -519,6 +524,8 @@ static int tascam_probe(struct usb_interface *intf, init_usb_anchor(&tascam->midi_in_anchor); init_usb_anchor(&tascam->midi_out_anchor); + timer_setup(&tascam->error_timer, tascam_error_timer, 0); + INIT_WORK(&tascam->stop_work, tascam_stop_work_handler); if (kfifo_alloc(&tascam->midi_in_fifo, MIDI_IN_FIFO_SIZE, GFP_KERNEL)) @@ -601,6 +608,7 @@ static void tascam_disconnect(struct usb_interface *intf) { usb_kill_anchored_urbs(&tascam->feedback_anchor); usb_kill_anchored_urbs(&tascam->midi_in_anchor); usb_kill_anchored_urbs(&tascam->midi_out_anchor); + timer_delete_sync(&tascam->error_timer); snd_card_disconnect(tascam->card); tascam_free_urbs(tascam); snd_card_free(tascam->card); diff --git a/us144mkii.h b/us144mkii.h index 01ad035..c061963 100644 --- a/us144mkii.h +++ b/us144mkii.h @@ -7,6 +7,7 @@ #include #include #include +#include #include #include #include @@ -179,6 +180,46 @@ enum tascam_register { * @midi_out_anchor: USB anchor for MIDI output URBs. */ struct tascam_card { + /* --- Hot Path Data (frequently accessed, especially in IRQ/workqueue) --- */ + spinlock_t lock; // Main lock, highly contended + atomic_t playback_active; + atomic_t capture_active; + atomic_t active_urbs; // Also frequently updated + + // Playback state (updated by feedback_urb_complete and playback_urb_complete) + u64 playback_frames_consumed; + snd_pcm_uframes_t driver_playback_pos; + u64 last_period_pos; + + // Capture state (updated by feedback_urb_complete and capture_urb_complete) + snd_pcm_uframes_t driver_capture_pos; + u64 capture_frames_processed; + u64 last_capture_period_pos; + + // Feedback related (critical for audio sync) + unsigned int feedback_accumulator_pattern[FEEDBACK_ACCUMULATOR_SIZE]; + unsigned int feedback_pattern_out_idx; + unsigned int feedback_pattern_in_idx; + bool feedback_synced; + unsigned int feedback_consecutive_errors; + unsigned int feedback_urb_skip_count; + const unsigned int (*feedback_patterns)[8]; + unsigned int feedback_base_value; + unsigned int feedback_max_value; + + // MIDI state (frequently accessed in MIDI handlers) + atomic_t midi_in_active; + atomic_t midi_out_active; + spinlock_t midi_in_lock; + spinlock_t midi_out_lock; + unsigned long midi_out_urbs_in_flight; + u8 midi_running_status; + bool in_sysex; + struct timer_list error_timer; // Timer for MIDI error retry + + int current_rate; // Also accessed frequently, moved up + + /* --- Less Hot Data (pointers, configuration, work structs) --- */ struct usb_device *dev; struct usb_interface *iface0; struct usb_interface *iface1; @@ -186,70 +227,46 @@ struct tascam_card { struct snd_pcm *pcm; struct snd_rawmidi *rmidi; - /* Playback stream */ + // Substream pointers struct snd_pcm_substream *playback_substream; + struct snd_pcm_substream *capture_substream; + struct snd_rawmidi_substream *midi_in_substream; + struct snd_rawmidi_substream *midi_out_substream; + + // URB arrays and sizes struct urb *playback_urbs[NUM_PLAYBACK_URBS]; size_t playback_urb_alloc_size; struct urb *feedback_urbs[NUM_FEEDBACK_URBS]; size_t feedback_urb_alloc_size; - atomic_t playback_active; - u64 playback_frames_consumed; - snd_pcm_uframes_t driver_playback_pos; - u64 last_period_pos; - u8 *playback_routing_buffer; - - /* Capture stream */ - struct snd_pcm_substream *capture_substream; struct urb *capture_urbs[NUM_CAPTURE_URBS]; size_t capture_urb_alloc_size; - atomic_t capture_active; - snd_pcm_uframes_t driver_capture_pos; - u64 capture_frames_processed; - u64 last_capture_period_pos; + struct urb *midi_in_urbs[NUM_MIDI_IN_URBS]; + struct urb *midi_out_urbs[NUM_MIDI_OUT_URBS]; + + // Buffers (pointers to external allocations) u8 *capture_ring_buffer; size_t capture_ring_buffer_read_ptr; size_t capture_ring_buffer_write_ptr; u8 *capture_decode_raw_block; s32 *capture_decode_dst_block; s32 *capture_routing_buffer; + + // Work structs struct work_struct capture_work; struct work_struct stop_work; - - /* MIDI streams */ - struct snd_rawmidi_substream *midi_in_substream; - struct snd_rawmidi_substream *midi_out_substream; - struct urb *midi_in_urbs[NUM_MIDI_IN_URBS]; - atomic_t midi_in_active; - struct kfifo midi_in_fifo; struct work_struct midi_in_work; - spinlock_t midi_in_lock; - struct urb *midi_out_urbs[NUM_MIDI_OUT_URBS]; - atomic_t midi_out_active; struct work_struct midi_out_work; - unsigned long midi_out_urbs_in_flight; - spinlock_t midi_out_lock; - u8 midi_running_status; - /* Shared state & Routing Matrix */ - spinlock_t lock; - atomic_t active_urbs; - int current_rate; - unsigned int line_out_source; /* 0: Playback 1-2, 1: Playback 3-4 */ - unsigned int digital_out_source; /* 0: Playback 1-2, 1: Playback 3-4 */ - unsigned int capture_12_source; /* 0: Analog In, 1: Digital In */ - unsigned int capture_34_source; /* 0: Analog In, 1: Digital In */ + // FIFOs + struct kfifo midi_in_fifo; - unsigned int feedback_accumulator_pattern[FEEDBACK_ACCUMULATOR_SIZE]; - unsigned int feedback_pattern_out_idx; - unsigned int feedback_pattern_in_idx; - bool feedback_synced; - unsigned int feedback_consecutive_errors; - unsigned int feedback_urb_skip_count; - - const unsigned int (*feedback_patterns)[8]; - unsigned int feedback_base_value; - unsigned int feedback_max_value; + // Routing Matrix settings + unsigned int line_out_source; + unsigned int digital_out_source; + unsigned int capture_12_source; + unsigned int capture_34_source; + // USB anchors struct usb_anchor playback_anchor; struct usb_anchor capture_anchor; struct usb_anchor feedback_anchor; diff --git a/us144mkii_capture.c b/us144mkii_capture.c index 6e42dda..40a8f85 100644 --- a/us144mkii_capture.c +++ b/us144mkii_capture.c @@ -190,11 +190,13 @@ void tascam_capture_work_handler(struct work_struct *work) { can_process = (available_data >= RAW_BYTES_PER_DECODE_BLOCK); if (can_process) { - size_t i; - - for (i = 0; i < RAW_BYTES_PER_DECODE_BLOCK; i++) - raw_block[i] = tascam->capture_ring_buffer[(read_ptr + i) % - CAPTURE_RING_BUFFER_SIZE]; + size_t bytes_to_end = CAPTURE_RING_BUFFER_SIZE - read_ptr; + if (bytes_to_end >= RAW_BYTES_PER_DECODE_BLOCK) { + memcpy(raw_block, tascam->capture_ring_buffer + read_ptr, RAW_BYTES_PER_DECODE_BLOCK); + } else { + memcpy(raw_block, tascam->capture_ring_buffer + read_ptr, bytes_to_end); + memcpy(raw_block + bytes_to_end, tascam->capture_ring_buffer, RAW_BYTES_PER_DECODE_BLOCK - bytes_to_end); + } tascam->capture_ring_buffer_read_ptr = (read_ptr + RAW_BYTES_PER_DECODE_BLOCK) % CAPTURE_RING_BUFFER_SIZE; } @@ -280,7 +282,9 @@ void capture_urb_complete(struct urb *urb) { "Failed to resubmit capture URB: %d\n", ret); usb_unanchor_urb(urb); usb_put_urb(urb); + atomic_dec(&tascam->active_urbs); /* Decrement on failed resubmission */ } out: usb_put_urb(urb); } + diff --git a/us144mkii_midi.c b/us144mkii_midi.c index cfa7ab5..907ad61 100644 --- a/us144mkii_midi.c +++ b/us144mkii_midi.c @@ -32,16 +32,28 @@ static void tascam_midi_in_work_handler(struct work_struct *work) { continue; for (i = 0; i < len; ++i) { + u8 byte = buf[i]; + /* Skip padding bytes */ - if (buf[i] == 0xfd) + if (byte == 0xfd) continue; - /* The last byte is often a terminator (0x00, 0xFF). Ignore it. */ - if (i == (len - 1) && (buf[i] == 0x00 || buf[i] == 0xff)) - continue; + if (byte == 0xf0) { /* SysEx Start */ + tascam->in_sysex = true; + } else if (byte == 0xf7) { /* SysEx End */ + tascam->in_sysex = false; + } else if (tascam->in_sysex) { + /* Inside a SysEx message */ + } else if (byte & 0x80) { /* Status byte */ + tascam->midi_running_status = byte; + } else { /* Data byte */ + if (tascam->midi_running_status != 0) + snd_rawmidi_receive(tascam->midi_in_substream, + &tascam->midi_running_status, 1); + } /* Submit valid MIDI bytes one by one */ - snd_rawmidi_receive(tascam->midi_in_substream, &buf[i], 1); + snd_rawmidi_receive(tascam->midi_in_substream, &byte, 1); } } } @@ -60,9 +72,11 @@ void tascam_midi_in_urb_complete(struct urb *urb) { if (urb->status) { if (urb->status != -ENOENT && urb->status != -ECONNRESET && - urb->status != -ESHUTDOWN && urb->status != -EPROTO) + urb->status != -ESHUTDOWN && urb->status != -EPROTO) { dev_err_ratelimited(tascam->card->dev, "MIDI IN URB failed: status %d\n", urb->status); + mod_timer(&tascam->error_timer, jiffies + msecs_to_jiffies(50)); + } goto out; } @@ -182,9 +196,11 @@ void tascam_midi_out_urb_complete(struct urb *urb) { if (urb->status) { if (urb->status != -ENOENT && urb->status != -ECONNRESET && - urb->status != -ESHUTDOWN) + urb->status != -ESHUTDOWN) { dev_err_ratelimited(tascam->card->dev, "MIDI OUT URB failed: %d\n", urb->status); + mod_timer(&tascam->error_timer, jiffies + msecs_to_jiffies(50)); + } } if (!tascam) diff --git a/us144mkii_pcm.c b/us144mkii_pcm.c index dd9a52d..1703c7b 100644 --- a/us144mkii_pcm.c +++ b/us144mkii_pcm.c @@ -364,6 +364,7 @@ int tascam_pcm_trigger(struct snd_pcm_substream *substream, int cmd) { if (err < 0) { usb_unanchor_urb(tascam->feedback_urbs[i]); usb_put_urb(tascam->feedback_urbs[i]); + atomic_dec(&tascam->active_urbs); /* Decrement on failed submission */ goto start_rollback; } atomic_inc(&tascam->active_urbs); @@ -375,6 +376,7 @@ int tascam_pcm_trigger(struct snd_pcm_substream *substream, int cmd) { if (err < 0) { usb_unanchor_urb(tascam->playback_urbs[i]); usb_put_urb(tascam->playback_urbs[i]); + atomic_dec(&tascam->active_urbs); /* Decrement on failed submission */ goto start_rollback; } atomic_inc(&tascam->active_urbs); @@ -386,6 +388,7 @@ int tascam_pcm_trigger(struct snd_pcm_substream *substream, int cmd) { if (err < 0) { usb_unanchor_urb(tascam->capture_urbs[i]); usb_put_urb(tascam->capture_urbs[i]); + atomic_dec(&tascam->active_urbs); /* Decrement on failed submission */ goto start_rollback; } atomic_inc(&tascam->active_urbs); diff --git a/us144mkii_pcm.h b/us144mkii_pcm.h index 3d026c7..da7ae8f 100644 --- a/us144mkii_pcm.h +++ b/us144mkii_pcm.h @@ -66,6 +66,47 @@ void feedback_urb_complete(struct urb *urb); */ void capture_urb_complete(struct urb *urb); +/** + * prepare_playback_urb() - Prepares and submits a playback URB. + * @tascam: the tascam_card instance + * + * This function gets a free playback URB, calculates the number of frames to + * send based on the feedback accumulator, copies audio data from the ALSA + * ring buffer, applies routing, and submits the URB. + */ +void prepare_playback_urb(struct tascam_card *tascam); + +/** + * feedback_urb_complete() - Completion handler for feedback isochronous URBs. + * @urb: the completed URB + * + * This is the master clock for the driver. It runs in interrupt context. + * It reads the feedback value from the device, which indicates how many + * samples the device has consumed. This information is used to adjust the + * playback rate and to advance the capture stream pointer, keeping both + * streams in sync. It then calls snd_pcm_period_elapsed if necessary and + * resubmits itself. + */ +void feedback_urb_complete(struct urb *urb); + +/** + * retire_capture_urb() - Completion handler for capture bulk URBs. + * @urb: the completed URB + * + * This function runs in interrupt context. It copies the received raw data + * into an intermediate ring buffer and then schedules the workqueue to process + * it. It then calls prepare_capture_urb() to submit the next URB. + */ +void retire_capture_urb(struct urb *urb); + +/** + * prepare_capture_urb() - Prepares and submits a capture URB. + * @tascam: the tascam_card instance + * + * This function gets a free capture URB from the anchor and submits it. + */ +void prepare_capture_urb(struct tascam_card *tascam); + /** * tascam_init_pcm() - Initializes the ALSA PCM device. * @pcm: Pointer to the ALSA PCM device to initialize. diff --git a/us144mkii_playback.c b/us144mkii_playback.c index d93f703..5440112 100644 --- a/us144mkii_playback.c +++ b/us144mkii_playback.c @@ -161,7 +161,7 @@ void playback_urb_complete(struct urb *urb) { struct snd_pcm_substream *substream; struct snd_pcm_runtime *runtime; unsigned long flags; - u8 *src_buf, *dst_buf; + size_t total_bytes_for_urb = 0; snd_pcm_uframes_t offset_frames; snd_pcm_uframes_t frames_to_copy; @@ -213,8 +213,7 @@ void playback_urb_complete(struct urb *urb) { spin_unlock_irqrestore(&tascam->lock, flags); if (total_bytes_for_urb > 0) { - src_buf = runtime->dma_area + frames_to_bytes(runtime, offset_frames); - dst_buf = tascam->playback_routing_buffer; + u8 *dst_buf = urb->transfer_buffer; /* Handle ring buffer wrap-around */ if (offset_frames + frames_to_copy > runtime->buffer_size) { @@ -222,17 +221,15 @@ void playback_urb_complete(struct urb *urb) { frames_to_bytes(runtime, runtime->buffer_size - offset_frames); size_t second_chunk_bytes = total_bytes_for_urb - first_chunk_bytes; - memcpy(dst_buf, src_buf, first_chunk_bytes); - memcpy(dst_buf + first_chunk_bytes, runtime->dma_area, - second_chunk_bytes); + memcpy(dst_buf, runtime->dma_area + frames_to_bytes(runtime, offset_frames), first_chunk_bytes); + memcpy(dst_buf + first_chunk_bytes, runtime->dma_area, second_chunk_bytes); } else { - memcpy(dst_buf, src_buf, total_bytes_for_urb); + memcpy(dst_buf, runtime->dma_area + frames_to_bytes(runtime, offset_frames), total_bytes_for_urb); } /* Apply routing to the contiguous data in our routing buffer */ process_playback_routing_us144mkii(tascam, dst_buf, dst_buf, frames_to_copy); - memcpy(urb->transfer_buffer, dst_buf, total_bytes_for_urb); } urb->dev = tascam->dev; @@ -244,6 +241,7 @@ void playback_urb_complete(struct urb *urb) { "Failed to resubmit playback URB: %d\n", ret); usb_unanchor_urb(urb); usb_put_urb(urb); + atomic_dec(&tascam->active_urbs); /* Decrement on failed resubmission */ } out: usb_put_urb(urb); @@ -273,9 +271,11 @@ void feedback_urb_complete(struct urb *urb) { if (urb->status) { if (urb->status != -ENOENT && urb->status != -ECONNRESET && - urb->status != -ESHUTDOWN && urb->status != -ENODEV) + urb->status != -ESHUTDOWN && urb->status != -ENODEV) { dev_err_ratelimited(tascam->card->dev, "Feedback URB failed: %d\n", urb->status); + atomic_dec(&tascam->active_urbs); /* Decrement on failed resubmission */ + } goto out; } if (!tascam || !atomic_read(&tascam->playback_active)) @@ -418,3 +418,4 @@ unlock_and_continue: out: usb_put_urb(urb); } +