Re: [PATCH RFC] ALSA: scarlett2: Add ioctls for user-space access

[Takashi Iwai <tiwai@suse.de>]

On Mon, 16 Oct 2023 17:45:54 +0200,
Geoffrey D. Bennett wrote:
> 
> On Mon, Oct 16, 2023 at 03:28:11PM +0200, Jaroslav Kysela wrote:
> > On 16. 10. 23 14:32, Geoffrey D. Bennett wrote:
> > > On Mon, Oct 16, 2023 at 09:04:21AM +0200, Jaroslav Kysela wrote:
> > > > On 14. 10. 23 15:58, Geoffrey D. Bennett wrote:
> > > > > In order to support functions such as firmware upgrade from
> > > > > user-space, add ioctls for submitting arbitrary proprietary requests
> > > > > through scarlett2_usb() and requesting/releasing exclusive access.
> > > > > ---
> > > > > 
> > > > > Hi Takashi,
> > > > > 
> > > > > I recently figured how to update the firmware on Scarlett Gen 2+
> > > > > devices. I think the best way to implement this is with an ioctl
> > > > > giving access to the scarlett2_usb() function from user-space, plus
> > > > > two ioctls to request/release exclusive access.
> > > > > 
> > > > > Does something like this seem reasonable?
> > > > 
> > > > Maybe you can use libusb for this job without an additional kernel
> > > > interface. It allows to detach the USB kernel driver and attach it again
> > > > when the job is complete.
> > > 
> > > Hi Jaroslav,
> > > 
> > > I considered using libusb (I used it during initial development of the
> > > driver), and if the only purpose of the ioctl would be for firmware
> > > updates then it would be reasonable to detach the kernel driver for
> > > that. However...
> > > 
> > > Beyond just being able to do firmware operations, that ioctl would
> > > also allow access to all of the configuration space using cmd =
> > > SCARLETT2_USB_GET_DATA and SCARLETT2_USB_SET_DATA. I think this would
> > > be the cleanest way to allow implementing non-mixer related
> > > functionality in user-space, such as reading the current firmware
> > > version, reading/updating the device name and channel names, and
> > > updating the software configuration space for Focusrite Control
> > > compatibility to name a few. These sorts of applications need to be
> > > able to make these proprietary requests through the scarlett2 driver
> > > to avoid disrupting it (or disrupting audio).
> > 
> > Thank you for this bigger picture. But except the firmware upgrade, all
> > those functions seem to be implementable in a more abstract way using
> > standard control API. Note that we can assign the controls also to card
> > (e.g. SNDRV_CTL_ELEM_IFACE_CARD) to classify them as non-mixer.
> 
> Hi Jaroslav,
> 
> By "more abstract way", you mean to have a control for every parameter
> which could be read or written? I can see that working for things like
> firmware version, device name, and channel name, but I think it would
> be pretty awful for the software configuration space that Focusrite
> Control uses, and bloat the driver quite a bit for what seems to me to
> be something more suited to user-land implementation.
> 
> (an aside, will alsactl store/restore SNDRV_CTL_ELEM_TYPE_BYTES?)
> 
> Or am I misunderstanding and you mean there is already a way (like
> SNDRV_CTL_IOCTL_TLV_COMMAND?) to send commands & get responses?
> 
> By "pretty awful"/bloat, a bit more explanation:
> 
> (1) Part of the NVRAM that can be accessed refers to the hardware
> controls which the driver allows the user to read/write (such as
> dim/mute/volume/level/pad/air/phantom/etc.)
> 
> (2) Part of the NVRAM is used by the Focusrite Control software to
> store the state of the interface in a higher-level way. It does not
> support all features of the hardware (e.g. routing is quite
> restricted).
> 
> It's not possible to represent all functions of (1) inside (2), so
> when I developed the driver I ignored (2) and implemented all features
> of (1). It doesn't make sense to implement (2) in the kernel as that
> would double the number of controls, but what would make sense would
> be a user-space application that implements read/write of (2) with a
> UI that restricts the user to what can be represented in (2).
> 
> For example: in (1), routing is all mono vs. in (2) routing is
> stereo-only, channels can be paired together, and there are
> balance/pan controls. I feel strongly this is the sort of thing where
> the kernel provides the low-level (1) hardware interface, and a
> user-space app provides a higher-level (2) interface.
> 
> It'd be nice if the app could store this data on the device itself
> like Focusrite Control does. And perhaps it could even do this in a
> non-Focusrite Control compatible way (for additional functionality
> when compatibility is not desired). But those options are not feasible
> if there is no access to read/write NVRAM arbitrarily.
> 
> That's why I came up with the proposed ioctl interface to the
> scarlett2_usb() function. That will allow user-space to access all
> applicable functions:
> 
> - reboot
> - get flash info
> - get flash segment info
> - erase flash segment
> - get erase segment progress
> - write flash segment
> - read NVRAM
> - write NVRAM
> 
> through a common interface, without disconnecting the kernel driver,
> and without adding specific support for a bunch of things that are not
> applicable to the hardware controls (1).

I caught a flu and am still in sick leave since the last week, so
this is just a short followup from my side.

First off, I don't think it appropriate to expose a generic register
access via (hwdep) ioctl as your RFC patch.  Then it allows to screw
up the hardware too easily by sending random bytes.  It may result in
a severe defect, too.

If you have some proper "features" to be implemented as the driver
functionality, they can be provided via ioctl, though; but then each
ioctl must serve for only a single purpose.

OTOH, if you need a handy register access for debugging, providing a
debugfs interface could be a solution, too.  It's safer than ioctl
(as it's not allowed for every user unlike ioctl), and it can be
disabled on a production system.


thanks,

Takashi