Re: [RFC] ALSA: usb-audio: missing PM guards in device mixer paths

[Takashi Iwai <tiwai@suse.de>]

On Tue, 25 Aug 2026 01:03:02 +0200,
Will Porter wrote:
> 
> Hi,
> 
> I found user-triggered USB transfers without the snd_usb_lock guard in six
> snd-usb-audio mixer files. The affected paths include ALSA controls in all six
> files. mixer_scarlett2.c and fcp.c also have unguarded hwdep operations.
> 
> I would appreciate guidance on three questions:
> 
>   * Should the control callbacks take snd_usb_lock and preserve their current
>     fresh-read behavior, even when polling prevents runtime suspend?
>   * How much of each synchronous hwdep operation should the shutdown and PM
>     reference cover, especially around userspace copies and protocol waits?
>   * Should Scarlett2 flash erase hold a runtime-PM reference until completion,
>     or should runtime suspend return -EBUSY while erase is active?
> 
> I checked sound.git for-next at e72d5659a260. Representative control paths
> that reach the device are:
> 
>   mixer_s1810c.c     snd_s1810c_switch_get
>                        -> snd_s1810c_get_switch_state
>                        -> snd_sc1810c_get_status_field -> snd_usb_ctl_msg
>   mixer_us16x08.c    snd_us16x08_meter_get
>                        -> snd_us16x08_send_urb/snd_us16x08_recv_urb
>                        -> snd_usb_ctl_msg
>   mixer_scarlett.c   scarlett_ctl_meter_get -> snd_usb_ctl_msg
>   mixer_scarlett2.c  scarlett2_meter_ctl_get
>                        -> scarlett2_usb_get_meter_levels -> scarlett2_usb
>                        -> scarlett2_usb_tx -> snd_usb_ctl_msg
>   mixer_quirks.c     snd_rme_digiface_rate_get
>                        -> snd_rme_digiface_get_status_val
>                        -> snd_rme_digiface_read_status -> snd_usb_ctl_msg
>   fcp.c              fcp_meter_ctl_get -> fcp_usb -> fcp_usb_tx
>                        -> snd_usb_ctl_msg
> 
> I scoped this audit to user-triggered control and hwdep paths in the
> snd-usb-audio mixer files. Other sound/usb subdrivers are outside this RFC.
> 
> The RME Digiface get and put callbacks in mixer_quirks.c share this omission.
> Most other direct control transfers in that file already take the guard.
> 
> The Scarlett2 hwdep read and write callbacks call scarlett2_usb(). Several
> ioctls also reach that transport. These include reboot, erase, erase-progress,
> and segment selection when an erase is pending. scarlett2_hwdep_open() can
> also poll erase progress. The FCP FCP_IOCTL_INIT and FCP_IOCTL_CMD operations
> transfer data through snd_usb_ctl_msg(). The other FCP hwdep operations do not
> transfer USB data.
> 
> The Studio 1810c, Scarlett2, and FCP drivers use private mutexes to serialize
> their protocols. mixer_us16x08.c uses chip->mutex. Those locks do not acquire
> a runtime-PM reference. The Scarlett Gen 1 meter and RME Digiface helpers do
> not use a protocol mutex around their single control requests.
> 
> The common mixer paths do take the PM guard. get_ctl_value_v1() and the v2
> path in mixer.c use CLASS(snd_usb_lock, pm)(chip) before snd_usb_ctl_msg().
> Many mixer_quirks.c callbacks do the same. The Forte callbacks in
> mixer_scarlett.c use the older explicit snd_usb_lock_shutdown() form, but the
> Scarlett Gen 1 meter callback does not.
> 
> Failure mechanism
> -----------------
> 
> When the USB device enters runtime suspend, usb_suspend_both() clears
> udev->can_submit. An unguarded callback then reaches snd_usb_ctl_msg() without
> calling snd_usb_autoresume(). The submission reaches usb_hcd_link_urb_to_ep(),
> which returns -EHOSTUNREACH for !urb->dev->can_submit.
> 
> The visible result depends on the driver:
> 
>   * Studio 1810c control reads return the transfer error. Its write callback
>     can instead report no change after a failed transfer.
>   * The Scarlett Gen 1 meter returns the transfer error.
>   * The RME Digiface callbacks return the transfer error and log it. Its
>     VOLATILE status controls can repeat that log on each poll.
>   * The Scarlett2 and FCP transports log the transfer error and convert it to
>     -EINVAL. Their VOLATILE meter controls can repeat that log on each poll.
>   * The TASCAM meter callback ignores both send and receive results. It parses
>     a zero-filled receive buffer, advances its four-step polling sequence, and
>     returns the persistent meter store as a successful read. Because the zero
>     buffer has no valid packet signature, this usually returns stale values.
> 
> usb_audio_driver supports autosuspend. I did not find the affected USB IDs in
> the entries that set QUIRK_FLAG_DISABLE_AUTOSUSPEND. Practical reachability
> still depends on the complete USB device and its userspace. A sibling
> interface or a polling process can keep the device active.
> 
> Hardware reproduction of the mechanism
> ---------------------------------------
> 
> I do not own hardware for these in-tree paths. I reproduced the mechanism
> with an out-of-tree Audient iD14 MkI mixer extension that had the same missing
> guard. I read an unguarded device-specific control and a guarded mixer.c
> control in one runtime-suspended window:
> 
>   unguarded Audient control -> "No route to host", no value,
>                                device remained suspended
>   guarded mixer.c control   -> value returned, device became active
> 
> After I added CLASS(snd_usb_lock, pm)(chip) to the Audient callback, that
> callback returned a value and resumed the device. This test demonstrates the
> PM mechanism. It does not test an affected in-tree device.
> 
> The iD14 did not reach runtime suspend in its normal desktop configuration.
> Its HID interface exposes the monitor knob as a mouse, and the desktop keeps
> that input node open. usbhid then holds a runtime-PM reference for the USB
> device. PipeWire also polls the control device. For the test, I stopped the
> userspace audio stack, unbound the HID interface, and set power/control to
> auto. Whether an equivalent masking condition exists on the affected devices
> is unknown.
> 
> Prior design history
> --------------------
> 
> Commit 47ab15459382 ("ALSA: usb-audio: Avoid nested autoresume calls")
> introduced snd_usb_lock_shutdown(). Its stated purposes include autoresume and
> delaying disconnect cleanup until concurrent operations finish. The current
> guard is the automatic-cleanup form of that helper.
> 
> During review of the original FCP driver, Takashi asked whether its hwdep and
> control paths needed suspend, resume, and disconnect handling:
> 
>   https://lore.kernel.org/linux-sound/87jzbidfs5.wl-tiwai@suse.de/
> 
> The v2 response reported system-suspend and disconnect tests while polling
> meters 20 times per second:
> 
>   https://lore.kernel.org/linux-sound/cover.1735495639.git.g@b4.vu/
> 
> Later revisions added private_suspend cleanup for the notification URB. I did
> not find discussion of runtime-autosuspend references for the synchronous
> control transfers, or shutdown accounting for the callbacks that issue them.
> 
> Guard scope
> -----------
> 
> The likely control-path fix is:
> 
> 	CLASS(snd_usb_lock, pm)(chip);
> 	if (pm.err < 0)
> 		return -EIO;
> 
> The PM guard should precede the protocol mutex. This order matches the common
> mixer paths and avoids a lock-order inversion if autoresume invokes a mixer
> resume hook. Each driver still needs a lock-order audit. Callbacks that only
> read a software shadow do not need the guard.
> 
> Existing code provides a policy precedent for meter and status reads. The
> VOLATILE RME class-compliant status controls were added by commit d39f1d68fe1d
> ("ALSA: usb-audio: Add custom mixer status quirks for RME CC devices"). Those
> controls take the guard around fresh device reads. Following that precedent,
> the affected callbacks should preserve their current fresh-data behavior and
> wake a suspended device. A polling mixer GUI can therefore keep the device
> active. Cached behavior would require a separate cache and update design.
> Omitting the guard does not provide valid cached behavior.
> 
> The hwdep paths need shutdown and PM accounting from the first use of
> disconnect-sensitive driver state through the complete device transaction.
> Input data may be copied before taking the guard only if that work does not
> dereference private mixer data. A locally buffered response may be copied out
> after releasing it. Otherwise, the reference must cover the userspace copy.
> It must also remain held across protocol sleeps and command-response waits.
> 
> Scarlett2 flash erase is asynchronous across file operations. USB PM guidance
> for asynchronous output holds a PM reference from submission until the output
> queue drains:
> 
>   https://docs.kernel.org/driver-api/usb/power-management.html
> 
> By analogy, runtime PM should remain referenced from erase submission through
> completion. Alternatively, the runtime-suspend path should return -EBUSY while
> an erase is active. A scoped guard in one ioctl does not cover that interval.
> A runtime-PM reference does not prevent system suspend, which remains a
> separate policy question. I have not reduced the hwdep changes to patches
> because this operation boundary needs review first.
> 
> The missing usage_count reference also means usb_audio_disconnect() does not
> wait for these transfers through snd_refcount_sync(). Other ALSA lifetime
> rules may protect some control or hwdep paths. I have not completed that
> lifetime audit, so I am not making a memory-safety claim here.
> 
> If this scope and guard placement look correct, I can prepare patches for the
> six files. I can compile-test them, but I cannot test them on the affected
> hardware. Tests from the device owners would be valuable.
> 
> Assisted-by: Claude:claude-opus-5
> Assisted-by: Antigravity:gemini-3.1-pro-high
> Assisted-by: Codex:gpt-5.6-sol
> 
> Thanks,
> Will Porter

In general, the code accessing the hardware via usb_ctl_msg() should
be covered by snd_usb_lock.

In the case of scarlett2, protecting scarlett2_usb() alone would cover
most cases, I guess.


thanks,

Takashi