ALSA: Add driver for big Scarlett 4th Gen interfaces
Subsystems (2)
Geoffrey D. Bennett
maintainerv5 cover ALSA: Add driver for big Scarlett 4th Gen interfaces
Hi Takashi,
Thank you again for your feedback.
Quoted 2 lines · Takashi Iwai
On Sat, 04 Jan 2025 18:12:54 +0100,
Geoffrey D. Bennett wrote:
[...]
Quoted 11 lines
Review
Quoted 6 lines
(quoted diff) +6
-0+union fcp_init { +>--__u32 version; +>--struct fcp_init_v1 v1; +}; + +#define FCP_IOCTL_INIT _IOWR('S', 0x64, union fcp_init)-
I don't think this version and union are needed.
If you need a different way of initialization, you can provide simply a new ioctl, instead of keeping the same ioctl and handles something conditionally in a complex way.
Noted and fixed.
Quoted 5 lines
Also, in which situation should the init ioctl be performed? And in which situation it shouldn't (must not) be performed? Currently the driver seems allowing to run it at any time. e.g. what happens if the init ioctl is called while you're playing or recording streams? The same question is applied to the
fcp_cmd, too.
The init must be performed at least once before any commands will work, but can be safely performed again at any time, including during playback/recording. Based on my testing, it seems that the control interface is completely separate from the audio/isochronous transfers. I've updated the comments to explicitly note that the init ioctl can be called at any time.
fcp_cmd is expected to be called at any time as well.
Quoted 7 lines
Quoted 2 lines
+>--if (!capable(CAP_SYS_RAWIO))
+>-->-------return-EPERM;-
So now you limit the hwdep operation essentially only to root user. Is it intended, right? That is, the hwdep should be accessed only at probe time for configuring the stuff (preferably some service or udev stuff)?
Correct. During development it was convenient to not have that restriction, but as you point out there are some risks that way, and the user-space side of the driver is intended to be automatically started by udev/systemd, which can run it with CAP_SYS_RAWIO.
FYI, the complete end-to-end setup from kernel driver to user-space driver (fcp-server from the fcp-support project) (automatically loaded by udev/systemd) to user-space control app (alsamixer, amixer, or alsa-scarlett-gui) is working well for me and a few other testers.
The ALSA controls created by the user-space driver are nearly indistinguishable from those created by the existing scarlett2 driver. All the expected features (mixing, routing, gain/volume, levels, firmware updates, etc.) are working well. The current public code for the user-space driver matches v1 of the kernel side that I previously submitted. Once we get the ioctls finalised then I'll push the corresponding user-space side.
Quoted 4 lines
Maybe it'd be helpful to give some "big picture" to indicate how things should work. Also, the more proper documentation should be provided for the hwdep interface, too. The kernel doc in comments should be fine, too.
Noted and fixed.
Quoted 3 lines
Last but not least, I don't think we need to split to two patches. The second patch merely adds the setup hook, and this can be folded into the main patch.
I'm not sure I agree with combining the patches. Here's my reasoning:
Patch 1 adds the new FCP driver which supports the new 4th Gen 16i16/18i16/18i20 interfaces. This patch is complete on its own and these interfaces don't require patch 2.
Patch 2 is optional and adds the ability (but not the default) to use the new FCP driver with existing hardware (that is already supported by the scarlett2 driver). This capability is added through a module parameter in the scarlett2 driver and is logically separate from the FCP driver.
But I'm okay to combine them if you still think that would be better.
Regards,
Geoffrey.
---
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 | 1044 +++++++++++++++++++++++++++++++++++ sound/usb/fcp.h | 7 + sound/usb/mixer_quirks.c | 7 + sound/usb/mixer_scarlett2.c | 8 + 7 files changed, 1180 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
maintainerPATCH v5 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.
--- MAINTAINERS | 10 +- include/uapi/sound/fcp.h | 107 ++++ sound/usb/Makefile | 1 + sound/usb/fcp.c | 1044 ++++++++++++++++++++++++++++++++++++++ sound/usb/fcp.h | 7 + sound/usb/mixer_quirks.c | 7 + 6 files changed, 1172 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
@@ -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
@@ -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 +1044 -0
1,044 lines not on this page · raw message
sound/usb/fcp.h new file +7 -0
@@ -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
@@ -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
Re: [PATCH v5 1/2] ALSA: FCP: Add Focusrite Control Protocol driver
Quoted 20 lines · Geoffrey D. Bennett
sound/usb/fcp.c new file +19
-0+/* Send an FCP command and get the response */ +static int fcp_usb (struct usb_mixer_interface *mixer,u32 opcode, +const void *req_data,u16 req_size, +void *resp_data,u16 resp_size) +{ +struct fcp_data *private = mixer->private_data; +struct usb_device *dev = mixer->chip->dev; + struct fcp_usb_packet *req __free(kfree) = NULL; + struct fcp_usb_packet *resp __free(kfree) = NULL; +size_t req_buf_size =struct_size (req, data, req_size); +size_t resp_buf_size =struct_size (resp, data, resp_size); +int retries =0 ; +const int max_retries =5 ; +int err; + +if (!mixer->urb) { + void *step0_resp __free(kfree) = NULL; + void *step2_resp __free(kfree) = NULL; +
sound/usb/fcp.c line 194
Quoted 10 lines
sound/usb/fcp.c new file +10
-0+ step0_resp =kmalloc (private->step0_resp_size,GFP_KERNEL ); +if (!step0_resp) +return -ENOMEM ; + step2_resp =kmalloc (private->step2_resp_size,GFP_KERNEL ); +if (!step2_resp) +return -ENOMEM ; + err =fcp_init (mixer, step0_resp, step2_resp); +if (err <0 ) +return err; + }
This happens only after the suspend, and this is for automatic re-initialization, right?
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()).
Quoted 38 lines
sound/usb/fcp.c new file +38
-0+static int fcp_meter_tlv_callback (struct snd_kcontrol *kctl, +int op_flag,unsigned int size, +unsigned int __user *tlv) +{ +struct usb_mixer_elem_info *elem = kctl->private_data; +struct usb_mixer_interface *mixer = elem->head.mixer; +struct fcp_data *private = mixer->private_data; + +if (op_flag ==SNDRV_CTL_TLV_OP_READ ) { +if (private->meter_labels_size ==0 ) +return 0 ; + +if (size > private->meter_labels_size) + size = private->meter_labels_size; + +if (copy_to_user (tlv, private->meter_labels, size)) +return -EFAULT ; + +return size; + } + +if (op_flag ==SNDRV_CTL_TLV_OP_WRITE ) { +kfree (private->meter_labels); + private->meter_labels =NULL ; + private->meter_labels_size =0 ; + +if (size ==0 ) +return 0 ; + +if (size >4096 ) +return -EINVAL ; + + private->meter_labels =kmalloc (size,GFP_KERNEL ); +if (!private->meter_labels) +return -ENOMEM ; + +if (copy_from_user (private->meter_labels, tlv, size)) { +kfree (private->meter_labels);
sound/usb/fcp.c line 452
Quoted 10 lines
sound/usb/fcp.c new file +10
-0+ private->meter_labels =NULL ; +return -EFAULT ; + } + + private->meter_labels_size = size; +return 0 ; + } + +return -EINVAL ; +}
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?
sound/usb/fcp.c line 566
Quoted 10 lines
sound/usb/fcp.c new file +10
-0+/* Execute an FCP command specified by the user */ +static int fcp_ioctl_cmd (struct usb_mixer_interface *mixer,unsigned long arg) +{ +struct fcp_cmd cmd; +int err, buf_size; + void *data __free(kfree) = NULL; + +/* get opcode and request/response size */ +if (copy_from_user (&cmd, (void __user *)arg,sizeof (cmd))) +return -EFAULT ;
You can avoid unneeded multiple cast. e.g. make a void * pointer in the caller side cast from arg, then pass it to each function. Each function may have an argument with the explicit type pointer, here would be "struct fcp_cmd *arg". Then the rest can be just "arg" here as the address, and...
sound/usb/fcp.c line 584
Quoted 4 lines
sound/usb/fcp.c new file +4
-0+/* copy request from user */ +if (cmd.req_size >0 ) +if (copy_from_user (data, ((void __user *)arg) +sizeof (cmd), + cmd.req_size))
"arg.data" here instead of the error-prone cast + offset.
thanks,
Takashi
Jaroslav Kysela
maintainerRe: [PATCH v5 1/2] ALSA: FCP: Add Focusrite Control Protocol driver
Quoted 44 lines · Takashi Iwai
Quoted 44 lines
sound/usb/fcp.c new file +44
-0+static int fcp_meter_tlv_callback (struct snd_kcontrol *kctl, +int op_flag,unsigned int size, +unsigned int __user *tlv) +{ +struct usb_mixer_elem_info *elem = kctl->private_data; +struct usb_mixer_interface *mixer = elem->head.mixer; +struct fcp_data *private = mixer->private_data; + +if (op_flag ==SNDRV_CTL_TLV_OP_READ ) { +if (private->meter_labels_size ==0 ) +return 0 ; + +if (size > private->meter_labels_size) + size = private->meter_labels_size; + +if (copy_to_user (tlv, private->meter_labels, size)) +return -EFAULT ; + +return size; + } + +if (op_flag ==SNDRV_CTL_TLV_OP_WRITE ) { +kfree (private->meter_labels); + private->meter_labels =NULL ; + private->meter_labels_size =0 ; + +if (size ==0 ) +return 0 ; + +if (size >4096 ) +return -EINVAL ; + + private->meter_labels =kmalloc (size,GFP_KERNEL ); +if (!private->meter_labels) +return -ENOMEM ; + +if (copy_from_user (private->meter_labels, tlv, size)) { +kfree (private->meter_labels); + private->meter_labels =NULL ; +return -EFAULT ; + } + + private->meter_labels_size = size; +return 0 ;
Quoted 8 lines
sound/usb/fcp.c line 221
Quoted 4 lines
sound/usb/fcp.c new file +4
-0+ } + +return -EINVAL ; +}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.
The data format _must_ be in TLV encapsulation also for such R/W operations. 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.
Jaroslav
Signature · 2 lines
Jaroslav Kysela <[email protected]> Linux Sound Maintainer; ALSA Project; Red Hat, Inc.
Geoffrey D. Bennett
maintainerPATCH v5 2/2 ALSA: scarlett2: Add device_setup option to use FCP driver
In reply to Geoffrey D. BennettAdd 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.
--- sound/usb/mixer_scarlett2.c | 8 ++++++++ 1 file changed, 8 insertions(+)
sound/usb/mixer_scarlett2.c +8 -0
@@ -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