From: Takashi Iwai <tiwai@suse.de>
To: Frank van de Pol <fvdpol@gmail.com>
Cc: tiwai@suse.de, perex@perex.cz, corbet@lwn.net,
khan@linuxfoundation.org, rdunlap@infradead.org,
linux-sound@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v4 0/1] ALSA: usb: Add support for Reloop Jockey 3 DJ controllers
Date: Mon, 28 Sep 2026 18:25:05 +0200 [thread overview]
Message-ID: <875wzp4a3i.wl-tiwai@suse.de> (raw)
In-Reply-To: <20260925021915.78909-1-fvdpol@gmail.com>
On Fri, 25 Sep 2026 04:19:13 +0200,
Frank van de Pol wrote:
>
> Hi Takashi, Jaroslav, and the ALSA community,
>
> Apologies for the long gap since v3. This is not simply v3 with the review
> comments addressed -- I went back to the reverse engineering, and a fair
> amount of what came out of it changed the driver structurally.
>
> The patch has grown accordingly: v3 was 8 files and 1613 insertions, this is
> 17 and 7326. Most of that is not new logic: of the roughly 5700 added
> lines, about 2100 are comments, 1660 are KUnit tests and generated test
> vectors for the codec, 249 are the new jockey3.rst documentation, and only
> about 1300 are executable code. The rest is blank lines and Kconfig
> plumbing.
>
> The relatively large number of comments comes from the deliberate decision
> to include what I learned from the protocol analysis with the relevant
> section of code. The parts that most need explaining (the bit-plane wire
> format, device initialization, why the URBs run for the device's whole
> lifetime, the locking order etc.) cannot be verified by reading the code
> alone. ploytec_codec.c is over half comment for that reason: the DOC: block
> deriving the format from the board's I2S and DMA geometry is effectively the
> specification the optimized variants implement.
>
> Previous version:
> v3: https://lore.kernel.org/all/20260622011131.1748298-1-fvdpol@gmail.com/
>
> Changes v3 -> v4:
>
> - Reworked the locking architecture around an explicit hierarchy: a
> process-context rate_mutex outermost, then IRQ-safe leaf spinlocks for
> playback, capture and MIDI that are never nested in one another.
> Documented in jockey3.c.
>
> - Fixed the capture stall after a sample-rate change, eliminating the USB
> device reset that recovery used to require. The reset rate per rate
> change went from 19.3% to zero, with a 98k clean streak on arm64 and 61k
> on x86_64.
>
> - Rewrote the bit-plane codec: a portable reference implementation plus
> 32-bit and 64-bit SWAR variants selected at compile time, called through a
> batch API at the driver's real batch sizes. Against the same machine's
> reference build, the optimized path measures 11.0x encode / 7.8x decode on
> x86_64 and 6.9x / 6.1x on arm64. KUnit tests validate all variants against
> an independently derived model of the wire format.
>
> - Added URB coalescing. The driver used to submit one 512-byte packet per
> URB, fixing the completion rate at 9923/s at 44.1 kHz and 21600/s at 96
> kHz for as long as the device was plugged in, used or not. N packets per
> URB (N a power of two, 1 to 8) are now chosen per PCM open from the
> requested period size.
>
> - Fixed a cold-boot initialization race. Straight after power-on the device
> accepts the entire init sequence, reports success on every transfer, takes
> playback samples, but its audio engine never starts. Bisected over 100
> cold boots to between 144 and 156 ms after enumeration; the driver now
> waits 250 ms before its first control transfer, close to the 297-308 ms
> the Windows driver waits.
>
> - Added a URB liveness watchdog. Every error path hangs off a URB
> completion, so a device that stops completing URBs produces no error, no
> xrun and no log line. A work item now reports either direction silent for
> 20 ms and enters the recovery ladder on a stall onset.
>
> - Hardened handling of an unresponsive device: an EP0 error-class predicate
> aborts the handshake before usb_set_interface() is reached, since the USB
> core disables an interface's endpoints as its first action and does not
> re-enable them on the failure return.
>
> - Added Documentation/sound/cards/jockey3.rst
>
> - Addressed the v3 review comments.
>
>
> Testing:
>
> Validated with an automated hardware-in-the-loop framework written for the
> purpose: probe and unbind endurance, PCM cycling, every rate in both
> directions, period and buffer boundary matrices, duplex, xrun injection,
> rate-change soaks, suspend/resume, USB disconnects via hub port power
> switching, and cold-boot cycling through a network-controlled mains switch.
> Some of the figures above come from runs of tens of thousands of iterations.
>
> Exercised on real hardware on x86_64, i386, arm64 (Raspberry Pi 4) and
> armv6/armhf (Raspberry Pi 1B -- functional but tight at 88.2/96 kHz), with
> x86_64 and arm64 also run under a KASAN + lockdep debug kernel. The codec
> KUnit suite additionally tested under UML and QEMU on i386, arm, arm64,
> riscv and s390 (test big/little endianness).
>
>
> Two things I would rather state than have found:
>
> - The mitigation stack around the rate-change stall is now practically
> dormant since the rate-change improvement. I kept it as cheap insurance;
> happy to remove it.
>
> - The 250 ms settling delay in jockey3_initialize() runs on the
> USB hub thread. I can move initialization off the probe path if you
> prefer, though that introduces complexity to avoid races.
>
>
> I look forward to your review/feedback.
>
> Best regards,
> Frank
>
> Frank van de Pol (1):
> ALSA: usb: Add support for Reloop Jockey 3 DJ controllers
>
> Documentation/sound/cards/index.rst | 1 +
> Documentation/sound/cards/jockey3.rst | 249 +
> MAINTAINERS | 8 +
> sound/usb/Kconfig | 1 +
> sound/usb/Makefile | 1 +
> sound/usb/jockey3/.kunitconfig | 13 +
> sound/usb/jockey3/Kconfig | 60 +
> sound/usb/jockey3/Makefile | 7 +
> sound/usb/jockey3/jockey3.c | 4060 +++++++++++++++++
> sound/usb/jockey3/ploytec_codec.c | 656 +++
> sound/usb/jockey3/ploytec_codec.h | 44 +
> sound/usb/jockey3/ploytec_codec_kunit.c | 871 ++++
> .../usb/jockey3/ploytec_codec_test_vectors.h | 793 ++++
> sound/usb/jockey3/ploytec_midi.c | 83 +
> sound/usb/jockey3/ploytec_midi.h | 36 +
> sound/usb/jockey3/ploytec_proto.c | 365 ++
> sound/usb/jockey3/ploytec_proto.h | 78 +
> 17 files changed, 7326 insertions(+)
> create mode 100644 Documentation/sound/cards/jockey3.rst
> create mode 100644 sound/usb/jockey3/.kunitconfig
> create mode 100644 sound/usb/jockey3/Kconfig
> create mode 100644 sound/usb/jockey3/Makefile
> create mode 100644 sound/usb/jockey3/jockey3.c
> create mode 100644 sound/usb/jockey3/ploytec_codec.c
> create mode 100644 sound/usb/jockey3/ploytec_codec.h
> create mode 100644 sound/usb/jockey3/ploytec_codec_kunit.c
> create mode 100644 sound/usb/jockey3/ploytec_codec_test_vectors.h
> create mode 100644 sound/usb/jockey3/ploytec_midi.c
> create mode 100644 sound/usb/jockey3/ploytec_midi.h
> create mode 100644 sound/usb/jockey3/ploytec_proto.c
> create mode 100644 sound/usb/jockey3/ploytec_proto.h
Thanks for the update. I think overall the code would be OK, but
the problem is that it's a way too big single patch to digest.
Could you try to instruct LLM for splitting logically for easier
reviews? It could be like starting with the core probe skeleton,
polytec core stuff, basic PCM loops, the mixer support, the MIDI
support, kunit, documentation, something else I forgot, etc.
Takashi
next prev parent reply other threads:[~2026-09-28 16:25 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
2026-09-25 2:19 Frank van de Pol
2026-09-25 2:19 ` [PATCH v4 1/1] " Frank van de Pol
2026-09-28 16:25 ` Takashi Iwai [this message]
2026-09-28 16:34 ` [PATCH v4 0/1] " Takashi Iwai
2026-09-30 1:05 ` Frank van de Pol
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=875wzp4a3i.wl-tiwai@suse.de \
--to=tiwai@suse.de \
--cc=corbet@lwn.net \
--cc=fvdpol@gmail.com \
--cc=khan@linuxfoundation.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-sound@vger.kernel.org \
--cc=perex@perex.cz \
--cc=rdunlap@infradead.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®