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

[Takashi Iwai <tiwai@suse.de>]

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?

> 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?

> 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.


thanks,

Takashi