[00/11] ALSA: scarlett2: Firmware Upgrade and Error Handling Improvements

Message

Geoffrey D. Bennett Dec. 19, 2023, 5:36 p.m. UTC

Hi Takashi,

11 patches to improve error handling and add firmware upgrade support
to the scarlett2 driver. The notes below incorporate your feedback &
include answers to your questions in response to my previous RFC v2
email.

Patch 1 adds GitHub links for the repo where I share this code before
submission here, and for issue reporting.

Patches 2-5 add missing error/bounds checks.

Patch 6 adds a missing lock.

Patches 7-11 add support for firmware upgrades.

The usage of the hwdep interface is as per my previous proposal:

- ioctl pversion to check the protocol version
- ioctl select_flash_segment SCARLETT2_SEGMENT_ID_SETTINGS
- ioctl erase_flash_segment
- ioctl get_erase_progress (optional)
- ioctl select_flash_segment SCARLETT2_SEGMENT_ID_FIRMWARE
- ioctl erase_flash_segment
- ioctl get_erase_progress (optional)
- write() the firmware
- ioctl reboot

Notes on the hwdep interface:

- Conflicting mixer operations are denied with EBUSY.

- The hwdep interface is marked as exclusive so there can't be
  conflicting hwdep operations.

- Invalid sequences of operations (e.g. erase before select, write
  before erase) return an error.

- The erase operation is asynchronous, and the progress can optionally
  be monitored by calling get_erase_progress.

- If the erase progress is not monitored then subsequent hwdep
  operations will wait until the erase is complete.

- The write operation is synchronous, but <1KB can be written per
  call, and it returns very quickly. On the user-side it looks like
  this with error checking omitted:

  while (offset < len)
    offset += snd_hwdep_write(hwdep, buf + offset, len - offset);

- By using a subset of the firmware-upgrade steps, other useful
  high-level operations can be performed: reset-to-factory-defaults,
  reset-to-factory-firmware, or just reboot.

- I considered combining the select and erase operations, but that
  would prevent a future read-flash operation if that ever become
  possible.

- I considered separate one-shot ioctls for reset-to-factory, etc.,
  but that prevents providing progress feedback.

- In case the purpose of the other firmware segments is discovered in
  the future, this implementation would be able to support
  erase/write/read of them with minimal changes.

Here is a user-space implementation of all of the above:
https://github.com/geoffreybennett/scarlett2

This has been tested by me on every supported Scarlett 2nd, 3rd, 4th
Gen and Clarett+ device, and tested by a handful of others on their
Scarlett 4th Gen devices.

Thanks,
Geoffrey.

Geoffrey D. Bennett (11):
  ALSA: scarlett2: Update maintainer info
  ALSA: scarlett2: Add missing error check to scarlett2_config_save()
  ALSA: scarlett2: Add missing error check to scarlett2_usb_set_config()
  ALSA: scarlett2: Add missing error checks to *_ctl_get()
  ALSA: scarlett2: Add clamp() in scarlett2_mixer_ctl_put()
  ALSA: scarlett2: Add missing mutex lock around get meter levels
  ALSA: scarlett2: Add #defines for firmware upgrade
  ALSA: scarlett2: Retrieve useful flash segment numbers
  ALSA: scarlett2: Add skeleton hwdep/ioctl interface
  ALSA: scarlett2: Add ioctl commands to erase flash segments
  ALSA: scarlett2: Add support for uploading new firmware

 MAINTAINERS                    |   7 +-
 include/uapi/sound/scarlett2.h |  54 ++
 sound/usb/mixer_scarlett2.c    | 914 ++++++++++++++++++++++++++++++---
 3 files changed, 901 insertions(+), 74 deletions(-)
 create mode 100644 include/uapi/sound/scarlett2.h

Comments

Takashi Iwai Dec. 29, 2023, 3:08 p.m. UTC | #1

On Tue, 19 Dec 2023 18:36:04 +0100,
Geoffrey D. Bennett wrote:
> 
> Hi Takashi,
> 
> 11 patches to improve error handling and add firmware upgrade support
> to the scarlett2 driver. The notes below incorporate your feedback &
> include answers to your questions in response to my previous RFC v2
> email.
> 
> Patch 1 adds GitHub links for the repo where I share this code before
> submission here, and for issue reporting.
> 
> Patches 2-5 add missing error/bounds checks.
> 
> Patch 6 adds a missing lock.
> 
> Patches 7-11 add support for firmware upgrades.
> 
> The usage of the hwdep interface is as per my previous proposal:
> 
> - ioctl pversion to check the protocol version
> - ioctl select_flash_segment SCARLETT2_SEGMENT_ID_SETTINGS
> - ioctl erase_flash_segment
> - ioctl get_erase_progress (optional)
> - ioctl select_flash_segment SCARLETT2_SEGMENT_ID_FIRMWARE
> - ioctl erase_flash_segment
> - ioctl get_erase_progress (optional)
> - write() the firmware
> - ioctl reboot
> 
> Notes on the hwdep interface:
> 
> - Conflicting mixer operations are denied with EBUSY.
> 
> - The hwdep interface is marked as exclusive so there can't be
>   conflicting hwdep operations.
> 
> - Invalid sequences of operations (e.g. erase before select, write
>   before erase) return an error.
> 
> - The erase operation is asynchronous, and the progress can optionally
>   be monitored by calling get_erase_progress.
> 
> - If the erase progress is not monitored then subsequent hwdep
>   operations will wait until the erase is complete.
> 
> - The write operation is synchronous, but <1KB can be written per
>   call, and it returns very quickly. On the user-side it looks like
>   this with error checking omitted:
> 
>   while (offset < len)
>     offset += snd_hwdep_write(hwdep, buf + offset, len - offset);
> 
> - By using a subset of the firmware-upgrade steps, other useful
>   high-level operations can be performed: reset-to-factory-defaults,
>   reset-to-factory-firmware, or just reboot.
> 
> - I considered combining the select and erase operations, but that
>   would prevent a future read-flash operation if that ever become
>   possible.
> 
> - I considered separate one-shot ioctls for reset-to-factory, etc.,
>   but that prevents providing progress feedback.
> 
> - In case the purpose of the other firmware segments is discovered in
>   the future, this implementation would be able to support
>   erase/write/read of them with minimal changes.
> 
> Here is a user-space implementation of all of the above:
> https://github.com/geoffreybennett/scarlett2
> 
> This has been tested by me on every supported Scarlett 2nd, 3rd, 4th
> Gen and Clarett+ device, and tested by a handful of others on their
> Scarlett 4th Gen devices.
> 
> Thanks,
> Geoffrey.
> 
> Geoffrey D. Bennett (11):
>   ALSA: scarlett2: Update maintainer info
>   ALSA: scarlett2: Add missing error check to scarlett2_config_save()
>   ALSA: scarlett2: Add missing error check to scarlett2_usb_set_config()
>   ALSA: scarlett2: Add missing error checks to *_ctl_get()
>   ALSA: scarlett2: Add clamp() in scarlett2_mixer_ctl_put()
>   ALSA: scarlett2: Add missing mutex lock around get meter levels
>   ALSA: scarlett2: Add #defines for firmware upgrade
>   ALSA: scarlett2: Retrieve useful flash segment numbers
>   ALSA: scarlett2: Add skeleton hwdep/ioctl interface
>   ALSA: scarlett2: Add ioctl commands to erase flash segments
>   ALSA: scarlett2: Add support for uploading new firmware

Now all patches have been merged to topic/scarlett2 branch (merged to
for-next branch for 6.8).


thanks,

Takashi