[v3,0/2] ALSA: Add driver for big Scarlett 4th Gen interfaces

Message

Geoffrey D. Bennett Jan. 2, 2025, 10:38 p.m. UTC

Hi Takashi,

Static buffers in the ioctl structs didn't seem to be the right way to
go, so I followed the instructions in
Documentation/drivers-api/ioctl.rst and the ioctls now work with
32-bit userspace on 64-bit kernels.

I added suspend/resume handling and all the suspend/resume cases that
I tried now work too.

Regards,
Geoffrey.

---
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             | 979 ++++++++++++++++++++++++++++++++++++
 sound/usb/fcp.h             |   7 +
 sound/usb/mixer_quirks.c    |   7 +
 sound/usb/mixer_scarlett2.c |   8 +
 7 files changed, 1079 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

Comments

Takashi Iwai Jan. 3, 2025, 5:07 p.m. UTC | #1

On Thu, 02 Jan 2025 23:38:07 +0100,
Geoffrey D. Bennett wrote:
> 
> Hi Takashi,
> 
> Static buffers in the ioctl structs didn't seem to be the right way to
> go, so I followed the instructions in
> Documentation/drivers-api/ioctl.rst and the ioctls now work with
> 32-bit userspace on 64-bit kernels.

Actually it's rather trivial if you use a variable length array and
passes the header to the ioctl struct.  e.g.

struct fcp_cmd {
	__u16 size;
	__u16 resp_size;
	u8 data[];
};

then you can simply do copy_from_user() from the data field.  And
write back to the data field again for the response if resp_size is
non-zero, too.

The above can be used for most of your needed I/O in general, I
suppose.

> I added suspend/resume handling and all the suspend/resume cases that
> I tried now work too.

Thanks.  The code used for suspend is same for the fcp_private_free(),
and they can be factored out.


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.

So one another possible way would be to provide more tailored ioctls
that are specific to each task.  Or we may introduce some sanity
checks of the possible registers or whatever parameters instead of
accepting all as is.  Of course those would decrease the flexibility;
when some new feature is needed, you'd need to adjust the kernel side
as well.  That's the cost for safety.

I'm not sure about this design decision yet.


thanks,

Takashi