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

["Geoffrey D. Bennett" <g@xxxxx>]

Hi Takashi,

On Fri, Nov 24, 2023 at 02:39:26PM +0100, Takashi Iwai wrote:
> On Sun, 19 Nov 2023 18:35:27 +0100,
> Geoffrey D. Bennett wrote:
> > 
> > Hi Jaroslav, Takashi,
> > 
> > I took your feedback onboard about not providing generic access to the
> > scarlett2_usb() function from user-space.
> > 
> > After a few iterations, I've come up with this hwdep interface to
> > support reset-to-factory-defaults, reset-to-factory-firmware, and
> > firmware-update in a safe way:
> > 
> > -----8<-----8<-----8<-----8<-----8<-----8<-----8<-----8<-----8<-----
> > 
> > /* Get protocol version */
> > #define SCARLETT2_IOCTL_PVERSION _IOR('S', 0x60, int)
> > 
> > /* Reboot */
> > #define SCARLETT2_IOCTL_REBOOT _IO('S', 0x61)
> > 
> > /* Select flash segment */
> > #define SCARLETT2_SEGMENT_ID_SETTINGS 0
> > #define SCARLETT2_SEGMENT_ID_FIRMWARE 1
> > #define SCARLETT2_SEGMENT_ID_COUNT 2
> > 
> > #define SCARLETT2_IOCTL_SELECT_FLASH_SEGMENT _IOW('S', 0x62, int)
> > 
> > /* Erase selected flash segment */
> > #define SCARLETT2_IOCTL_ERASE_FLASH_SEGMENT _IO('S', 0x63)
> > 
> > /* Get selected flash segment erase progress
> >  * 1 through to num_blocks, or 255 for complete
> >  */
> > struct scarlett2_flash_segment_erase_progress {
> >         unsigned char progress;
> >         unsigned char num_blocks;
> > };
> > #define SCARLETT2_IOCTL_GET_ERASE_PROGRESS \
> >         _IOR('S', 0x64, struct scarlett2_flash_segment_erase_progress)
> > 
> > -----8<-----8<-----8<-----8<-----8<-----8<-----8<-----8<-----8<-----
> > 
> > Does that look reasonable to you?
> > 
> > Broadly, it's used like this:
> > 
> > Reset to factory default configuration:
> > 
> > - ioctl select_flash_segment SCARLETT2_SEGMENT_ID_SETTINGS
> > - ioctl erase_flash_segment
> > - ioctl get_erase_progress (optional)
> 
> So the erase operation is asynchronous?  This sounds a bit dangerous.
> Will the driver block further conflicting operations until the erase
> finishes?

Yes it is asynchronous. I've made it so that it's not dangerous by
locking out any conflicting operations:
- Mixer operations that require device access return EBUSY
- The hwdep is marked as exclusive so other processes can't use it
- Subsequent hwdep operations (if get_erase_progress wasn't called)
  will block until the erase is complete

> > Erase firmware (reverts to factory firmware which is stored in a
> > different flash segment, inaccessible from these ioctls):
> > 
> > - ioctl select_flash_segment SCARLETT2_SEGMENT_ID_FIRMWARE
> > - ioctl erase_flash_segment
> > - ioctl get_erase_progress (optional)
> > 
> > Upload new firmware:
> > 
> > - write() <- a bunch of these, only permitted after the previous erase
> >   step was completed
> 
> The write op must accept partial writes, and it becomes cumbersome.
> Can it be a one-shot ioctl, too?

I considered one-shot ioctls, but as the erase & write operations take
some seconds, then it is not possible to provide feedback to the
end-user while the erase & write operations happen.

> > On completion:
> > 
> > - ioctl reboot
> > 
> > To confirm that this interface is sufficient, I have implemented it in
> > the scarlett2 driver and written a user-space utility which can
> > perform all the above operations.
> > 
> > I will clean up the implementation a bit and then submit for review;
> > just wanted to share the interface first in case you have any comments
> > at this point.
> 
> IMO, from the user POV, it's easier to have per-purpose ioctls,
> instead of combining multiple ioctl sequences.  Of course, it won't
> scale too much, but for the limited number of operations, it's
> clearer.
> 
> That is, we can provide just a few ioctls for reset-to-factory,
> reset-to-something-else, and update.
> 
> But, if you need asynchronous operations inevitably by some reason,
> it's a different story, though.

Just to provide progress feedback to the end-user.

I've written the CLI tool using the proposed ioctl interface, and it
works nicely:

https://github.com/geoffreybennett/scarlett2

[g@fedora ~]$ time scarlett2 update
Selected device Scarlett 4th Gen Solo
Found firmware version 2115 for Scarlett 4th Gen Solo:
  /usr/lib/firmware/scarlett2/scarlett2-1235-8218-2115.bin
Updating Scarlett 4th Gen Solo from firmware version 1974 to 2115
Resetting configuration to factory default...
Erase progress: Done!
Erasing upgrade firmware...
Erase progress: Done!
Firmware write progress: Done!
Rebooting interface...

real    0m5.919s
user    0m0.007s
sys     0m0.034s

The user experience would not be as nice with one-shot ioctls. And
using ioctls which block for a long time would make using them from
the GUI https://github.com/geoffreybennett/alsa-scarlett-gui/ rather
awkward. None of the other operations on the interface block for an
appreciable amount of time.

I've got a first draft of firmware update and Scarlett 4th Gen support
that I am sharing with others to test now. It's 48 commits, divided
into:
- 5 commits to add extra checks that are missing
- 5 commits for firmware management
- 20 commits refactoring the existing driver to allow Scarlett 4th Gen
  support to be added
- 18 commits adding the support (although the underlying Gen 4
  protocol is the same as the other series, there are many new
  different types of controls)

I've put those commits on this branch:
https://github.com/geoffreybennett/scarlett-gen2/tree/scarlett-gen4

Do you want me to share all 48 commits on the mailing list at once? Or
maybe just the first 5+5 commits for now and the rest after I get some
feedback from others?

Thanks,
Geoffrey.