ALSA: Add driver for big Scarlett 4th Gen interfaces

PATCH Superseded Geoffrey D. Bennett 2 participants 5 messages Jan 13, 2025 – Jan 14, 2025

Subsystems (2)

Sound Focusrite control protocol/SCARLETT2 mixer drivers (Scarlett Gen 2+, Clarett, and Vocaster)

lore

Share & tools

lore

Geoffrey D. Bennett

author maintainer

2025-01-13 19:41 UTC #0 lore raw

v6 cover ALSA: Add driver for big Scarlett 4th Gen interfaces

Hi Takashi and Jaroslav,

Thank you both for your reviews. I have addressed all of your comments below:

Quoted 2 lines · Takashi Iwai

This happens only after the suspend, and this is for automatic re-initialization, right?

Correct.

Quoted 5 lines

It's a bit scary that such a low-level function itself does re-initialization, as fcp_init() will call this function again. That said, it's more straightforward if the re-initialization is checked and done in the upper level. (And here check only mixer->urb and return error (or spew WARN_ON()).

Fixed.

Quoted 6 lines

Hmm, this special is a special use of TLV in non-standard way, which needs definitely documentation. The use is no longer TLV, just some raw read/write of a bulk data for the kcontrol , after all. -
Also, I couldn't figure out what exactly this "meter_labels" stuff serves for. It's not referred from anywhere else than TLV read?

The user-space driver stores meter labels in the Level Meter control's TLV data so users can determine what each channel of the control corresponds to.

As the kernel doesn't need to interpret what's stored there, I've renamed it from meter_labels to the more generic "meter_metadata" in case future uses are discovered.

Quoted 2 lines

Quoted 1 line
[...] fcp_ioctl_cmd() [...]
You can avoid unneeded multiple cast.

Fixed.

Quoted 1 line · Jaroslav Kysela

The data format _must_ be in TLV encapsulation also for such R/W operations.

The user-space driver stores the data in the correct type-length-data format (with the length a multiple of sizeof(unsigned int)). This is similar to how sound/control.c read_user_tlv() and replace_user_tlv() operate.

Quoted 3 lines

So, a new type should be defined. Perhaps, we can define a driver specific data type, because the meter levels (and the whole extensions) seem as device specific.

Are you thinking like this?

include/uapi/sound/tlv.h +2 -0

index b99a2414b53d..965c64796b6a 100644
@@ -18,6 +18,8 @@
 #define SNDRV_CTL_TLVT_CHMAP_VAR       0x102   /* channels freely swappable */
 #define SNDRV_CTL_TLVT_CHMAP_PAIRED    0x103   /* pair-wise swappable */
 
+#define SNDRV_CTL_TLVT_FCP_METER_METADATA 0x110 /* FCP driver meter metadata */
+

[...]

I was thinking it wouldn't be needed as the kernel doesn't need to interpret the data I'm storing, and user-created ALSA controls (which this is very similar to) are free to store whatever they like in the TLV data anyway?

Thanks again,
Geoffrey.

---
Changes in v6:
- Move re-initialisation out of fcp_usb() so it's clear there's no infinite recursion
- Update theory of operation and clarify the use of meter TLV metadata
- Rename meter_labels to meter_metadata for clarity
- Remove unnecessary casts in fcp_ioctl_cmd()

---
Changes in v5:
- Remove version/union complexity from init ioctl
- Add documentation to clarify ioctl usage and big picture

---
Changes in v4:
- Use variable-length data arrays in ioctl structs instead of pointers
- Add CAP_SYS_RAWIO requirement to hwdep interface
- Add validation of flash commands to prevent accidental bricking due to erasing/writing the App_Gold segment
- Refactor URB cleanup

---
Changes in v3:
- Update ioctl structs and add ioctl_compat op to work with 32-bit userspace on 64-bit kernels
- Update driver to do all init steps so it can re-init after suspend/resume
- Add version field to init ioctl for future compatibility
- Improve error messages when unexpected response data is received

---
Changes in v2 as per Takashi's feedback:
- Use fixed-size data arrays instead of pointers in ioctl structs
- Define notify struct outside of struct fcp_dev
- Use u8/u16 types without __ prefix
- Use cleanup.h for code simplification
- Add init flag to ensure FCP_IOCTL_INIT is called before FCP_IOCTL_CMD and FCP_IOCTL_SET_METER_MAP
- Do not destroy/recreate the meter control (the number of channels is now fixed when it is created)

Geoffrey D. Bennett (2):
ALSA: FCP: Add Focusrite Control Protocol driver
ALSA: scarlett2: Add device_setup option to use FCP driver

 MAINTAINERS                 |   10 +-
 include/uapi/sound/fcp.h    |  107 ++++
 sound/usb/Makefile          |    1 +
 sound/usb/fcp.c             | 1075 +++++++++++++++++++++++++++++++++++
 sound/usb/fcp.h             |    7 +
 sound/usb/mixer_quirks.c    |    7 +
 sound/usb/mixer_scarlett2.c |    8 +
 7 files changed, 1211 insertions(+), 4 deletions(-)
 create mode 100644 include/uapi/sound/fcp.h
 create mode 100644 sound/usb/fcp.c
 create mode 100644 sound/usb/fcp.h

Signature · 1 line

2.45.0

Geoffrey D. Bennett

author maintainer

2025-01-13 19:42 UTC #1 lore raw

PATCH v6 1/2 ALSA: FCP: Add Focusrite Control Protocol driver

Add a new kernel driver for the Focusrite Control Protocol (FCP), which is used by Focusrite Scarlett 2nd Gen, 3rd Gen, 4th Gen, Clarett USB, Clarett+, and Vocaster series audio interfaces. This driver provides a user-space control interface via ALSA's hwdep subsystem.

Unlike the existing Scarlett2 driver which implements all ALSA controls in kernel space, this new FCP driver takes a different approach by providing a minimal kernel interface that allows a user-space driver to send FCP commands and receive notifications. The only control implemented in kernel space is the Level Meter, since it requires frequent polling of volatile data.

While this driver supports all interfaces that the Scarlett2 driver works with, it is initially enabled only for 4th Gen 16i16, 18i16, and 18i20 interfaces that are not supported by the Scarlett2 driver.

Signed-off-by Geoffrey D. Bennett <[email protected]>

---
 MAINTAINERS              |   10 +-
 include/uapi/sound/fcp.h |  107 ++++
 sound/usb/Makefile       |    1 +
 sound/usb/fcp.c          | 1075 ++++++++++++++++++++++++++++++++++++++
 sound/usb/fcp.h          |    7 +
 sound/usb/mixer_quirks.c |    7 +
 6 files changed, 1203 insertions(+), 4 deletions(-)
 create mode 100644 include/uapi/sound/fcp.h
 create mode 100644 sound/usb/fcp.c
 create mode 100644 sound/usb/fcp.h

MAINTAINERS +6 -4

index 6bb4ec0c162a..2fd0945cb738 100644
@@ -8868,14 +8868,16 @@ L:	[email protected]
 S:	Maintained
 F:	drivers/input/joystick/fsia6b.c
 
-FOCUSRITE SCARLETT2 MIXER DRIVER (Scarlett Gen 2+ and Clarett)
+FOCUSRITE CONTROL PROTOCOL/SCARLETT2 MIXER DRIVERS (Scarlett Gen 2+, Clarett, and Vocaster)
 M:	Geoffrey D. Bennett <[email protected]>
 L:	[email protected]
 S:	Maintained
-W:	https://github.com/geoffreybennett/scarlett-gen2
-B:	https://github.com/geoffreybennett/scarlett-gen2/issues
-T:	git https://github.com/geoffreybennett/scarlett-gen2.git
+W:	https://github.com/geoffreybennett/linux-fcp
+B:	https://github.com/geoffreybennett/linux-fcp/issues
+T:	git https://github.com/geoffreybennett/linux-fcp.git
+F:	include/uapi/sound/fcp.h
 F:	include/uapi/sound/scarlett2.h
+F:	sound/usb/fcp.c
 F:	sound/usb/mixer_scarlett2.c
 
 FORCEDETH GIGABIT ETHERNET DRIVER

include/uapi/sound/fcp.h new file +107 -0

107 lines not on this page · raw message

sound/usb/Makefile +1 -0

index 0532499dbc6d..30bd5348477b 100644
@@ -6,6 +6,7 @@
 snd-usb-audio-y := 	card.o \
 			clock.o \
 			endpoint.o \
+			fcp.o \
 			format.o \
 			helper.o \
 			implicit.o \

sound/usb/fcp.c new file +1075 -0

1,075 lines not on this page · raw message

sound/usb/fcp.h new file +7 -0

new file mode 100644
index 000000000000..d58cc7db75b1
@@ -0,0 +1,7 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef __USBAUDIO_FCP_H
+#define __USBAUDIO_FCP_H
+
+int snd_fcp_init(struct usb_mixer_interface *mixer);
+
+#endif /* __USBAUDIO_FCP_H */

sound/usb/mixer_quirks.c +7 -0

index a95ebcf4e46e..150fbb5ac7ed 100644
@@ -38,6 +38,7 @@
 #include "mixer_us16x08.h"
 #include "mixer_s1810c.h"
 #include "helper.h"
+#include "fcp.h"
 
 struct std_mono_table {
 	unsigned int unitid, control, cmask;

@@ -4033,6 +4034,12 @@ int snd_usb_mixer_apply_create_quirk(struct usb_mixer_interface *mixer)
 		err = snd_scarlett2_init(mixer);
 		break;
 
+	case USB_ID(0x1235, 0x821b): /* Focusrite Scarlett 16i16 4th Gen */
+	case USB_ID(0x1235, 0x821c): /* Focusrite Scarlett 18i16 4th Gen */
+	case USB_ID(0x1235, 0x821d): /* Focusrite Scarlett 18i20 4th Gen */
+		err = snd_fcp_init(mixer);
+		break;
+
 	case USB_ID(0x041e, 0x323b): /* Creative Sound Blaster E1 */
 		err = snd_soundblaster_e1_switch_create(mixer);
 		break;

Signature · 1 line

2.45.0

Takashi Iwai

2025-01-14 12:57 UTC #2 lore raw

Re: [PATCH v6 1/2] ALSA: FCP: Add Focusrite Control Protocol driver

Quoted 31 lines · Geoffrey D. Bennett

sound/usb/fcp.c new file +31 -0

+static int fcp_meter_ctl_get(struct snd_kcontrol *kctl,
+			     struct snd_ctl_elem_value *ucontrol)
+{
+	struct usb_mixer_elem_info *elem = kctl->private_data;
+	struct usb_mixer_interface *mixer = elem->head.mixer;
+	struct fcp_data *private = mixer->private_data;
+	int num_meter_slots, resp_size;
+	u32 *resp __free(kfree) = NULL;
+	int i, err = 0;
+
+	struct {
+		__le16 pad;
+		__le16 num_meters;
+		__le32 magic;
+	} __packed req;
+
+	guard(mutex)(&private->mutex);
+
+	num_meter_slots = private->num_meter_slots;
+	resp_size = num_meter_slots * sizeof(u32);
+
+	resp = kmalloc(resp_size, GFP_KERNEL);
+	if (!resp)
+		return -ENOMEM;
+
+	req.pad = 0;
+	req.num_meters = cpu_to_le16(num_meter_slots);
+	req.magic = cpu_to_le32(FCP_USB_METER_LEVELS_GET_MAGIC);
+	err = fcp_usb(mixer, FCP_USB_GET_METER,
+		      &req, sizeof(req), resp, resp_size);
+

sound/usb/fcp.c line 413

Quoted 10 lines

sound/usb/fcp.c new file +10 -0

+	/* reinit and retry if needed */
+	if (err == -ENODEV) {
+		err = fcp_reinit(mixer);
+		if (err < 0)
+			return err;
+		err = fcp_usb(mixer, FCP_USB_GET_METER,
+			      &req, sizeof(req), resp, resp_size);
+	} else if (err < 0) {
+		return err;
+	}

You seem to have forgotten to check the error after the second fcp_usb()?

I think fcp_reinit() can be better checked before the first fcb_usb() call, just by checking mixer->urb. i.e.

	if (!mixer->urb) {
		err = fcp_reinit(mixer);
		if (err < 0)
			return err;
	}

	err = fcp_usb(...);
	if (err < 0)
		return err;

Or change fcp_reinit() to return 0 if mixer->urbs is already set, then you can call fcp_reinit() unconditionally, too.

Quoted 4 lines

sound/usb/fcp.c new file +4 -0

+/* Execute an FCP command specified by the user */
+static int fcp_ioctl_cmd(struct usb_mixer_interface *mixer,
+			 struct fcp_cmd __user *arg)
+{

(snip)

sound/usb/fcp.c line 631

Quoted 4 lines

sound/usb/fcp.c new file +4 -0

+	/* copy response to user */
+	if (cmd.resp_size > 0)
+		if (copy_to_user(((void __user *)arg) + sizeof(cmd), data,
+				 cmd.resp_size))

This should be

		if (copy_to_user(arg->data, data, cmd.resp_size))

(snip)

sound/usb/fcp.c line 658

Quoted 10 lines

sound/usb/fcp.c new file +10 -0

+/* Set the Level Meter map and add the control */
+static int fcp_ioctl_set_meter_map(struct usb_mixer_interface *mixer,
+				   unsigned long arg)
+{
+	struct fcp_meter_map map;
+	struct fcp_data *private = mixer->private_data;
+	s16 *tmp_map __free(kfree) = NULL;
+	int err;
+
+	if (copy_from_user(&map, (void __user *)arg, sizeof(map)))

This function can also take struct fcp_meter_map __user * argument, and you can avoid the cast.

Ditto for fcp_ioctl_init().

sound/usb/fcp.c line 729

Quoted 5 lines

sound/usb/fcp.c new file +5 -0

+static int fcp_hwdep_ioctl(struct snd_hwdep *hw, struct file *file,
+			   unsigned int cmd, unsigned long arg)
+{
+	struct usb_mixer_interface *mixer = hw->private_data;
+	struct fcp_data *private = mixer->private_data;

You can assign a void __user pointer, e.g.

	void __user *argp = (void __user *)arg;

and pass it to each function, e.g.

sound/usb/fcp.c line 741

Quoted 11 lines

sound/usb/fcp.c new file +11 -0

+	guard(mutex)(&private->mutex);
+
+	switch (cmd) {
+
+	case FCP_IOCTL_PVERSION:
+		return put_user(FCP_HWDEP_VERSION,
+				(int __user *)arg) ? -EFAULT : 0;
+		break;
+
+	case FCP_IOCTL_INIT:
+		return fcp_ioctl_init(mixer, arg);

This will be:

		return fcp_ioctl_init(mixer, argp);

and make fcp_ioctl_init() receiving it like:

static int fcp_ioctl_init(struct usb_mixer_interface *mixer,
struct fcp_init __user *arg)

{
	struct fcp_init init;
	....

	if (copy_from_user(&init, arg, sizeof(init)))
		return -EFAULT;	
	....
	if (copy_to_user(arg->resp, resp, buf_size))
		return -EFAULT;

Takashi

Geoffrey D. Bennett

author maintainer

2025-01-13 19:42 UTC #3 lore raw

PATCH v6 2/2 ALSA: scarlett2: Add device_setup option to use FCP driver

In reply to Geoffrey D. Bennett

Add a new device_setup option (SCARLETT2_USE_FCP_DRIVER = 0x08) that allows users to opt in to using the new FCP driver instead of the existing scarlett2 driver for their device. This provides a way to test the new FCP driver on existing supported hardware while keeping the Scarlett2 driver as the default.

When the SCARLETT2_USE_FCP_DRIVER bit is set in device_setup, the scarlett2 driver initialisation will hand off to the FCP driver instead of proceeding with its own initialisation. The FCP driver then provides access to the device via its hwdep interface.

Signed-off-by Geoffrey D. Bennett <[email protected]>

---
 sound/usb/mixer_scarlett2.c | 8 ++++++++
 1 file changed, 8 insertions(+)

sound/usb/mixer_scarlett2.c +8 -0

index 7f595c1752a5..288d22e6a0b2 100644
@@ -166,6 +166,7 @@
 #include "helper.h"
 
 #include "mixer_scarlett2.h"
+#include "fcp.h"
 
 /* device_setup value to allow turning MSD mode back on */
 #define SCARLETT2_MSD_ENABLE 0x02

@@ -173,6 +174,9 @@
 /* device_setup value to disable this mixer driver */
 #define SCARLETT2_DISABLE 0x04
 
+/* device_setup value to use the FCP driver instead */
+#define SCARLETT2_USE_FCP_DRIVER 0x08
+
 /* some gui mixers can't handle negative ctl values */
 #define SCARLETT2_VOLUME_BIAS 127

@@ -9702,6 +9706,10 @@ int snd_scarlett2_init(struct usb_mixer_interface *mixer)
 	if (!mixer->protocol)
 		return 0;
 
+	/* check if the user wants to use the FCP driver instead */
+	if (chip->setup & SCARLETT2_USE_FCP_DRIVER)
+		return snd_fcp_init(mixer);
+
 	/* find entry in scarlett2_devices */
 	entry = get_scarlett2_device_entry(mixer);
 	if (!entry) {

Signature · 1 line

2.45.0

Takashi Iwai

2025-01-14 12:43 UTC #4 lore raw

In reply to Geoffrey D. Bennett

Quoted 15 lines · Geoffrey D. Bennett

Quoted 7 lines
Hmm, this special is a special use of TLV in non-standard way, which needs definitely documentation. The use is no longer TLV, just some raw read/write of a bulk data for the kcontrol , after all. -
Also, I couldn't figure out what exactly this "meter_labels" stuff serves for. It's not referred from anywhere else than TLV read?
The user-space driver stores meter labels in the Level Meter control's TLV data so users can determine what each channel of the control corresponds to.
As the kernel doesn't need to interpret what's stored there, I've renamed it from meter_labels to the more generic "meter_metadata" in case future uses are discovered.

(snip)

Quoted 26 lines · Jaroslav Kysela

Quoted 1 line
The data format _must_ be in TLV encapsulation also for such R/W operations.
The user-space driver stores the data in the correct type-length-data format (with the length a multiple of sizeof(unsigned int)). This is similar to how sound/control.c read_user_tlv() and replace_user_tlv() operate.
Quoted 3 lines
So, a new type should be defined. Perhaps, we can define a driver specific data type, because the meter levels (and the whole extensions) seem as device specific.
Are you thinking like this?
include/uapi/sound/tlv.h +2 -0
index b99a2414b53d..965c64796b6a 100644
@@ -18,6 +18,8 @@
 #define SNDRV_CTL_TLVT_CHMAP_VAR       0x102   /* channels freely swappable */
 #define SNDRV_CTL_TLVT_CHMAP_PAIRED    0x103   /* pair-wise swappable */
 
+#define SNDRV_CTL_TLVT_FCP_METER_METADATA 0x110 /* FCP driver meter metadata */
+
[...]

Quoted 4 lines

I was thinking it wouldn't be needed as the kernel doesn't need to interpret the data I'm storing, and user-created ALSA controls (which this is very similar to) are free to store whatever they like in the TLV data anyway?

I suppose this data is set by the configuration program just like other fcp register writes at the probe time, and the applications are supposed just to read them, instead, right?

If so, it's safer to set this via ioctl instead of TLV write. Every application is permitted to write TLV, hence everyone can write a malformed data there, too.

Judging from your comment, it's a single TLV entry? Then passing the data via ioctl should be straightforward and you can verify the data length there, too.

The TLV read can return the value set by the ioctl only after it's set up.

thanks,

Takashi