Re: [PATCH v8 5/8] ALSA: usb: babyfacepro: add S3 suspend/resume

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] ALSA: usb: babyfacepro: missing snd_power_change_state() in suspend and resume
- [Low] ALSA: usb: babyfacepro: babyface_suspend() walks private card->devices list
--

commit f0c674e7a86798c445b9cc47638dc398955e5a17
Author: Ismaïl Bahloul <i.bahloul01@gmail.com>

ALSA: usb: babyfacepro: add S3 suspend/resume

This commit adds system suspend, resume, and reset_resume callbacks to the
RME Babyface Pro USB audio driver. On suspend, it suspends PCM substreams,
kills active URBs, and cancels stream work; on resume, it re-runs cold
initialization and restores cached mixer state and flags.

> diff --git a/sound/usb/babyfacepro/babyfacepro.c b/sound/usb/babyfacepro/babyfacepro.c
> index 12cfd7fea3315..06ef1a97d27a4 100644
> --- a/sound/usb/babyfacepro/babyfacepro.c
> +++ b/sound/usb/babyfacepro/babyfacepro.c

[ ... ]

> @@ -1752,6 +1764,62 @@ static void babyface_disconnect(struct usb_interface *intf)
>  	snd_card_free_when_closed(chip->card);
>  }
>  
> +static int babyface_suspend(struct usb_interface *intf, pm_message_t message)
> +{
> +	struct snd_usb_babyface *chip = usb_get_intfdata(intf);
> +
> +	struct snd_device *sdev;
> +
> +	if (!chip)
> +		return 0;
> +	list_for_each_entry(sdev, &chip->card->devices, list) {
> +		if (sdev->type == SNDRV_DEV_PCM)
> +			snd_pcm_suspend_all(sdev->device_data);
> +	}

[Severity: Low]
This isn't a bug, but why walk the internal chip->card->devices list here?

card->devices and struct snd_device are internal to sound/core/device.c and
are not meant to be traversed directly by drivers. In addition, the commit
message notes:

  On suspend, let the PCM core suspend the substreams (applications get
  -ESTRPIPE and restart)

With USB autosuspend disabled, the PCM core already registers pcm_dev_pm_ops
(do_pcm_suspend() in sound/core/pcm.c) on the child PCM device, which calls
snd_pcm_suspend_all() before the parent USB interface suspend callback runs.
If the driver ever needs to reference its PCM instance directly, could it
store the struct snd_pcm pointer returned by snd_pcm_new() in struct
snd_usb_babyface instead of iterating over chip->card->devices?

> +	/* The URB handlers queue the work: kill them first. */
> +	scoped_guard(mutex, &chip->mutex) {
> +		if (chip->streaming)
> +			babyface_stream_kill(chip);
> +	}
> +	cancel_work_sync(&chip->stream_work);
> +	return 0;
> +}
> +
> +static int babyface_resume(struct usb_interface *intf)
> +{
> +	struct snd_usb_babyface *chip = usb_get_intfdata(intf);
> +	int err;
> +
> +	if (!chip)
> +		return 0;
> +
> +	/* The device lost its state across the suspend; re-run the cold
> +	 * init and re-apply the cached mixer state.  Suspended PCM
> +	 * substreams are woken by the core - apps get -ESTRPIPE and
> +	 * restart (prepare starts a new session).
> +	 *
> +	 * This is also the reset_resume handler.  Some hosts reset the
> +	 * device on resume; that needs nothing more than the cold init
> +	 * already does here, and without the handler the USB core unbinds
> +	 * and re-probes the driver instead: the card goes away, open PCMs
> +	 * fail, and udev's alsactl restore then loads the mixer state
> +	 * stored at the last shutdown over the current one.
> +	 */
> +	guard(mutex)(&chip->mutex);
> +	err = usb_set_interface(chip->dev, BF_IFACE, chip->alt);
> +	if (err < 0)
> +		return err;
> +	err = bf_cold_init(chip);
> +	if (err < 0)
> +		return err;
> +	err = babyface_restore_state(chip);
> +	if (err < 0)
> +		return err;
> +	err = bf_state_apply_flags(chip);
> +
> +	return err;
> +}

[Severity: Medium]
Should babyface_suspend() and babyface_resume() update the card power state
via snd_power_change_state()?

In ALSA core (sound/core/pcm_native.c and sound/core/control.c), entry
points such as snd_pcm_common_ioctl(), snd_pcm_release(),
snd_ctl_elem_read_user(), snd_ctl_elem_write_user(),
snd_ctl_elem_info_user(), and snd_ctl_tlv_ioctl() call snd_power_wait() or
snd_power_ref_and_wait(), which only block while snd_power_get_state(card)
is not SNDRV_CTL_POWER_D0.

Because babyface_suspend() never sets SNDRV_CTL_POWER_D3hot and
babyface_resume() never restores SNDRV_CTL_POWER_D0, chip->card->power_state
stays at SNDRV_CTL_POWER_D0 across the entire suspend and resume cycle.
Couldn't a user-space audio server woken by snd_pcm_suspend_all() (receiving
-ESTRPIPE) or issuing SNDRV_PCM_IOCTL_PREPARE or control writes enter
babyface_pcm_prepare() or mixer callbacks while the interface is suspending,
suspended, or before babyface_resume() finishes restoring hardware registers?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261105151553.223722-1-i.bahloul01@gmail.com?part=5