ALSA: Add driver for big Scarlett 4th Gen interfaces
Subsystems (2)
Geoffrey D. Bennett
maintainerv4 cover ALSA: Add driver for big Scarlett 4th Gen interfaces
Hi Takashi,
Thanks again for your feedback. Requested changes made.
Quoted 8 lines
Now, considering this implementation again, a fundamental question is whether we really should go to this direction or not.
Usually the driver implements an ioctl per certain limited task (except for some debugging purpose). But we open all doors fully. This gives the most flexibility, of course. OTOH, it has a significant risk that every program may screw up your device easily by sending some malicious ioctls.
That is a good point. To eliminate that risk and prevent malicious use entirely, I added a requirement in v4 of having CAP_SYS_RAWIO to use the hwdep interface. In addition, I have added validation of the flash commands to prevent the App_Gold flash segment from being accidentally erased or written to (this segment contains the backup firmware used when the App_Upgrade segment does not contain a valid firmware image). I think this way we can have both flexibility and safety.
Regards,
Geoffrey.
---
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 | 71 +++ sound/usb/Makefile | 1 + sound/usb/fcp.c | 1018 +++++++++++++++++++++++++++++++++++ sound/usb/fcp.h | 7 + sound/usb/mixer_quirks.c | 7 + sound/usb/mixer_scarlett2.c | 8 + 7 files changed, 1118 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 v4 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 | 71 +++ sound/usb/Makefile | 1 + sound/usb/fcp.c | 1018 ++++++++++++++++++++++++++++++++++++++ sound/usb/fcp.h | 7 + sound/usb/mixer_quirks.c | 7 + 6 files changed, 1110 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 +71 -0
71 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 +1018 -0
1,018 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 v4 1/2] ALSA: FCP: Add Focusrite Control Protocol driver
Quoted 13 lines · Geoffrey D. Bennett
include/uapi/sound/fcp.h new file +13
-0+/* Get protocol version */ +#define FCP_IOCTL_PVERSION _IOR('S', 0x60, int) + +/* Start the protocol */ + +/* Step 0 and step 2 responses are variable length and placed in +* resp[] one after the other. +*/ +struct fcp_init_v1 { +__u32 version; +__u16 step0_resp_size; +__u16 step2_resp_size; +__u32 init1_opcode;
include/uapi/sound/fcp.h line 48
Quoted 10 lines
include/uapi/sound/fcp.h new file +10
-0+__u32 init2_opcode; +__u8 resp[]; +} __attribute__((packed)); + +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.
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.
sound/usb/fcp.c line 662
Quoted 7 lines
sound/usb/fcp.c new file +7
-0+static int fcp_hwdep_open (struct snd_hwdep *hw,struct file *file) +{ +struct usb_mixer_interface *mixer = hw->private_data; +struct fcp_data *private = mixer->private_data; + +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)?
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.
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.
thanks,
Takashi
Geoffrey D. Bennett
maintainerPATCH v4 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