* [PATCH RFC v2] ALSA: scarlett2: Add ioctls for user-space access [not found] ` <87edhtn0r2.wl-tiwai@suse.de> @ 2023-11-19 17:35 ` Geoffrey D. Bennett 2023-11-24 13:39 ` Takashi Iwai 0 siblings, 1 reply; 3+ messages in thread From: Geoffrey D. Bennett @ 2023-11-19 17:35 UTC (permalink / raw) To: Takashi Iwai; +Cc: Jaroslav Kysela, Takashi Iwai, alsa-devel, linux-sound 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) 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 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. Thanks, Geoffrey. ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH RFC v2] ALSA: scarlett2: Add ioctls for user-space access 2023-11-19 17:35 ` [PATCH RFC v2] ALSA: scarlett2: Add ioctls for user-space access Geoffrey D. Bennett @ 2023-11-24 13:39 ` Takashi Iwai 2023-11-27 18:32 ` Geoffrey D. Bennett 0 siblings, 1 reply; 3+ messages in thread From: Takashi Iwai @ 2023-11-24 13:39 UTC (permalink / raw) To: Geoffrey D. Bennett Cc: Jaroslav Kysela, Takashi Iwai, alsa-devel, linux-sound 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 ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH RFC v2] ALSA: scarlett2: Add ioctls for user-space access 2023-11-24 13:39 ` Takashi Iwai @ 2023-11-27 18:32 ` Geoffrey D. Bennett 0 siblings, 0 replies; 3+ messages in thread From: Geoffrey D. Bennett @ 2023-11-27 18:32 UTC (permalink / raw) To: Takashi Iwai; +Cc: Jaroslav Kysela, Takashi Iwai, alsa-devel, linux-sound 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. ^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2023-11-27 18:32 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <ZSqehHhedJQY9h/1@m.b4.vu>
[not found] ` <76c1526d-78be-92d2-cf2b-148278394575@perex.cz>
[not found] ` <ZS0tajzKr68CZ5uA@m.b4.vu>
[not found] ` <123242ed-c343-dab8-fed1-9f5d2da44d7a@perex.cz>
[not found] ` <ZS1asqF0cXRUzBwb@m.b4.vu>
[not found] ` <87edhtn0r2.wl-tiwai@suse.de>
2023-11-19 17:35 ` [PATCH RFC v2] ALSA: scarlett2: Add ioctls for user-space access Geoffrey D. Bennett
2023-11-24 13:39 ` Takashi Iwai
2023-11-27 18:32 ` Geoffrey D. Bennett
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox