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

Message

Geoffrey D. Bennett Dec. 29, 2024, 10:42 p.m. UTC

Hi Takashi,

Thanks for your feedback on the first version. Appreciate your
comments. I implemented fixes for almost all; I've got a couple of
questions, and I found one thing to fix.

> > +/* Do FCP step 0 */
> > +struct fcp_step0 {
> > +	void     *data;
> > +	uint16_t  size;
> > +};
> > +#define FCP_IOCTL_INIT _IOWR('S', 0x64, struct fcp_step0)
> 
> An ioctl with a pointer has to be handled carefully because you must
> convert it for 32bit compat code.  IOW, it's better to be avoided if
> possible.

For v2, I have changed the structs to have fixed-size data arrays
instead of a pointer, e.g.:

 /* Do FCP step 0 */
 struct fcp_step0 {
-       void     *data;
-       uint16_t  size;
+       uint16_t size;
+       uint8_t  data[FCP_MAX_DATA_SIZE];
 };

However, this approach makes struct fcp_cmd over 2KB which seems
excessive/a lot of copying for each ioctl call?

https://www.kernel.org/doc/html/v6.12/driver-api/ioctl.html suggests:

> 32-bit compat mode
> [...]
> As long as all the rules for data structures are followed, this is
> as easy as setting the .compat_ioctl pointer to a helper function
> such as compat_ptr_ioctl() or blkdev_compat_ptr_ioctl().
> [...]
> Pointers have the same problem, in addition to requiring the use of
> compat_ptr(). The best workaround is to use __u64 in place of
> pointers, which requires a cast to uintptr_t in user space, and the
> use of u64_to_user_ptr() in the kernel to convert it back into a
> user pointer.

Between these two options (fixed arrays vs u64_to_user_ptr), which
would you recommend? Or is there a third approach you had in mind?

> [...other comments noted and addressed...]

> Last but not least, any need for suspend/resume and disconnect
> handling?

I have tested on a laptop, playing audio, getting meter data 20x per
second:

1. suspend then resume
2. suspend, unplug, replug, resume
3. suspend, unplug, power off interface, power on interface, replug,
   resume

Results from testing:
- Cases 1 and 2: Driver remained connected and functioned correctly
- Case 3: Driver remained connected (to my surprise) but the sequence
  numbers were out of sync; I will work on a fix for that.

For disconnect handling, I've disconnected the device (software reset,
USB disconnect, and power off) during testing countless times, and
have not seen any issues. Is there something in particular I should
be looking for?

Thanks again for your feedback.

Regards,
Geoffrey.

---
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    |  65 +++
 sound/usb/Makefile          |   1 +
 sound/usb/fcp.c             | 837 ++++++++++++++++++++++++++++++++++++
 sound/usb/fcp.h             |   7 +
 sound/usb/mixer_quirks.c    |   7 +
 sound/usb/mixer_scarlett2.c |   8 +
 7 files changed, 931 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