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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] ALSA: usb: babyfacepro: bf_state_apply_flags() clobbers AN1/2 crosspoints on resume
- [Medium] ALSA: usb: babyfacepro: bf_state_apply_flags() unmutes Phones on resume
- [Medium] ALSA: usb: babyfacepro: bf_state_apply_flags() resets Boost ref level on resume
--

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

ALSA: usb: babyfacepro: add S3 suspend/resume

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

> diff --git a/sound/usb/babyfacepro/babyfacepro.c b/sound/usb/babyfacepro/babyfacepro.c
> index ed4555c711064..e0eb0ce9b92f3 100644
> --- a/sound/usb/babyfacepro/babyfacepro.c
> +++ b/sound/usb/babyfacepro/babyfacepro.c

[ ... ]

> @@ -390,10 +400,8 @@ int bf_state_apply_flags(struct snd_usb_babyface *chip)
>  	}
>  
>  	/* Re-apply an engaged DIM (the fixed -20 dB Phones pair + flag).  The
> -	 * level DIM releases back to is not persisted: it always tracks the
> -	 * Phones master while DIM is engaged, so rebuild it here - otherwise a
> -	 * DIM released after a re-probe would drive Phones to
> -	 * silence (dim_saved still zero on the fresh chip).
> +	 * level DIM releases back to tracks the Phones master while DIM is
> +	 * engaged.
>  	 */
>  	if (chip->dim) {
>  		chip->dim_saved[0] = chip->master[1][0];

[Severity: Medium]
What happens on resume when both chip->dim and chip->muted[1] are active?

In babyface_resume(), babyface_restore_state() calls bf_apply_masters(),
which checks chip->muted[out] and writes the muted level (BF_MASTER_MUTE /
0x0000) to the hardware master registers:

sound/usb/babyfacepro/babyfacepro-ctl.c:bf_apply_masters() {
	for (out = 0; out < 6; out++) {
		u16 l = chip->muted[out] ? 0 : chip->master[out][0];
		u16 r = chip->muted[out] ? 0 : chip->master[out][1];
		...
}

Immediately afterwards, babyface_resume() calls bf_state_apply_flags(), where
the chip->dim block unconditionally writes 0xcb (-20 dB) to
BF_REG_MASTER_8 + 2 * 1 (and + 1) and 0x0333 to BF_REG_MASTER_16 + 2 * 1
(and + 1) without checking chip->muted[1]:

sound/usb/babyfacepro/babyfacepro.c:bf_state_apply_flags() {
	if (chip->dim) {
		chip->dim_saved[0] = chip->master[1][0];
		chip->dim_saved[1] = chip->master[1][1];
		ret = bf_vendor_write(chip, BF_REQ_GAIN, 0xcb,
				      BF_REG_MASTER_8 + 2 * 1);
		...
		ret = bf_vendor_write(chip, BF_REQ_CROSSPOINT, 0x0333,
				      (BF_REG_MASTER_16 + 2 * 1) |
				      bf_flag_cycle[chip->flag_cnt]);
		...
}

Won't this overwrite the muted state on the hardware headphone output (PH3/4)
with -20 dB across a suspend/resume cycle while chip->muted[1] remains true?

[ ... ]

> @@ -1788,6 +1798,63 @@ 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)
> +{

[ ... ]

> +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.
> +	 */
> +	mutex_lock(&chip->mutex);
> +	err = usb_set_interface(chip->dev, BF_IFACE, chip->alt);
> +	if (err < 0)
> +		goto out;
> +	err = bf_cold_init(chip);
> +	if (err < 0)
> +		goto out;
> +	err = babyface_restore_state(chip);
> +	if (err < 0)
> +		goto out;
> +	err = bf_state_apply_flags(chip);

[Severity: High]
Does calling bf_state_apply_flags() right after babyface_restore_state() in
babyface_resume() clobber the restored AN1 -> AN1/2 and AN2 -> AN1/2
crosspoint faders?

During babyface_restore_state(), the crosspoint loop writes all cached
chip->xpoint[out][src] values to the hardware, including out = 0 (AN1/2) and
src = 0..1 (AN1 and AN2 at registers 0x0000, 0x001a, 0x0001, and 0x001b):

sound/usb/babyfacepro/babyfacepro.c:babyface_restore_state() {
	for (out = 0; out < 6; out++) {
		for (src = 0; src < 14; src++) {
			ret = bf_xpoint_write(chip, out, src,
					      chip->xpoint[out][src][0],
					      chip->xpoint[out][src][1]);
			...
}

Then bf_state_apply_flags() runs. Unlike ms_proc, phase, split, and trim
(which only touch the shared crosspoint registers when their respective state
is non-zero), the chip->width calculation in bf_state_apply_flags() executes
unconditionally:

sound/usb/babyfacepro/babyfacepro.c:bf_state_apply_flags() {
	l = (u16)(((0x2000 * (100 + chip->width) / 2) + 50) / 100);
	r = 0x2000 - l;
	ret = bf_vendor_write(chip, BF_REQ_CROSSPOINT, l, 0x0000);
	if (ret < 0)
		return ret;
	ret = bf_vendor_write(chip, BF_REQ_CROSSPOINT, r, 0x001a);
	if (ret < 0)
		return ret;
	ret = bf_vendor_write(chip, BF_REQ_CROSSPOINT, r, 0x0001);
	if (ret < 0)
		return ret;
	ret = bf_vendor_write(chip, BF_REQ_CROSSPOINT, l, 0x001b);
	if (ret < 0)
		return ret;
}

When chip->width is 0 (the default), this writes 0x1000 to registers 0x0000,
0x001a, 0x0001, and 0x001b, overwriting whatever chip->xpoint[0][0] and
chip->xpoint[0][1] values babyface_restore_state() just restored and routing
live microphone inputs AN1 and AN2 into the main AN1/2 output even when they
were muted in chip->xpoint[0][0..1]. Because chip->xpoint[0][0..1] in the
host cache still holds the old values (such as 0), subsequent bf_xpoint_put()
calls with the same value will also see no change against the cache and return
early without updating the hardware. (Also, when chip->width is non-zero,
bf_width_put() writes both 0x0000/0x001a/0x0001/0x001b and
0x0034/0x004e/0x0035/0x004f, whereas bf_state_apply_flags() only writes the
first four.)

[Severity: Medium]
Could bf_state_apply_flags() also reset the Instrument Ref Level from Boost
back to -10dBV during babyface_resume()?

In babyface_restore_state(), bf_preamp_state_write() restores the preamp
state and commits BF_REQ_PREAMP_COMMIT with 0x0003 when chip->ref_level is
BF_REF_LEVEL_BOOST:

sound/usb/babyfacepro/babyfacepro-ctl.c:bf_preamp_state_write() {
	ret = bf_vendor_write(chip, BF_REQ_PREAMP, chip->preamp, BF_REG_PREAMP);
	if (ret < 0)
		return ret;
	return bf_vendor_write(chip, BF_REQ_PREAMP_COMMIT,
			       chip->ref_level == BF_REF_LEVEL_BOOST ?
			       0x0003 : 0x0000, 0x0000);
}

However, bf_state_apply_flags() then writes the linked/an12 flags and follows
up with a hardcoded 0x0000 commit to BF_REQ_PREAMP_COMMIT:

sound/usb/babyfacepro/babyfacepro.c:bf_state_apply_flags() {
	ret = bf_vendor_write(chip, BF_REQ_PREAMP,
			      (chip->linked ? 0x0400 : 0x0000) |
			      (chip->an12 ? 0x1000 : 0x0000), 0x1000);
	if (ret < 0)
		return ret;
	ret = bf_vendor_write(chip, BF_REQ_PREAMP_COMMIT, 0x0000, 0x0000);
	if (ret < 0)
		return ret;
}

As noted in bf_preamp_state_write(), sending 0x0000 in BF_REQ_PREAMP_COMMIT
causes Boost to degrade to -10dBV while chip->ref_level still reports
BF_REF_LEVEL_BOOST. Should bf_state_apply_flags() pass
chip->ref_level == BF_REF_LEVEL_BOOST ? 0x0003 : 0x0000 in that commit write
as well?

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