Re: [PATCH v7 4/8] ALSA: usb: babyfacepro: add routing flags and varispeed pitch

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:
- [High] ALSA: usb: babyfacepro: bf_state_apply_flags() unmutes AN1/2 inputs on probe
- [High] ALSA: usb: babyfacepro: bf_ms_put() restores wrong crosspoints to AN1/2
- [High] ALSA: usb: babyfacepro: DIM and Phones master/mute states clobber each other
- [Medium] ALSA: usb: babyfacepro: preamp flag commits clear Boost ref level state
- [Medium] ALSA: usb: babyfacepro: bf_loopback_put() omits notify on cleared outputs
- [Low] ALSA: usb: babyfacepro: missing enum bounds and long checks in flag puts
- [Low] ALSA: usb: babyfacepro: unused dim_kctl pointer in babyface_create_flags()
--

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

ALSA: usb: babyfacepro: add routing flags and varispeed pitch

This commit adds ALSA mixer controls and state restoration for loopback,
AN 1>2, sample clock source, optical output format, AN1/2 input link, MS
processor, DIM, stereo width, FX send, and varispeed pitch.

> diff --git a/sound/usb/babyfacepro/babyfacepro-ctl.c b/sound/usb/babyfacepro/babyfacepro-ctl.c
> index 60604242be883..39584ec719b23 100644
> --- a/sound/usb/babyfacepro/babyfacepro-ctl.c
> +++ b/sound/usb/babyfacepro/babyfacepro-ctl.c

[ ... ]

> @@ -377,6 +381,11 @@ static int bf_master_put(struct snd_kcontrol *kctl,
>  
>  	chip->master[out][0] = l;
>  	chip->master[out][1] = r;
> +	/* A Phones change while DIM is engaged re-bases the restore point. */
> +	if (chip->dim && out == 1) {
> +		chip->dim_saved[0] = l;
> +		chip->dim_saved[1] = r;
> +	}
>  	ret = 1;
>  out:
>  	mutex_unlock(&chip->mutex);

[Severity: High]
Does changing the Phones master volume while DIM is engaged cancel the DIM
attenuation on the hardware?

In bf_master_put(), when chip->dim is true and out == 1, the code updates
chip->dim_saved[0..1], but only after writing wire_l and wire_r (the full
undimmed volume) to BF_REG_MASTER_8 + 2 * out and BF_REG_MASTER_16 + 2 * out.
That drives the Phones output to the undimmed level while chip->dim remains
true.

Similarly, in bf_mute_put(), unmuting out == 1 while chip->dim is true writes
chip->master[1][0..1] to the Phones master registers instead of the dimmed
BF_MASTER_MINUS20_8 and BF_MASTER_MINUS20_16 levels.

> @@ -1280,3 +1289,699 @@ int babyface_create_preamp(struct snd_usb_babyface *chip)

[ ... ]

> +static int bf_pitch_put(struct snd_kcontrol *kctl,
> +			struct snd_ctl_elem_value *ucontrol)
> +{
> +	struct snd_usb_babyface *chip = snd_kcontrol_chip(kctl);
> +	int p = ucontrol->value.integer.value[0];
> +	int ret = 0;
> +
> +	if (p < -50 || p > 50)
> +		return -EINVAL;

[Severity: Low]
Can ucontrol->value.integer.value[0] wrap when narrowed from long to int here
(and in bf_width_put() below) before the range check, unlike bf_master_put()
and bf_fx_send_put() which validate the long value prior to narrowing?

[ ... ]

> +static int bf_loopback_put(struct snd_kcontrol *kctl,
> +			   struct snd_ctl_elem_value *ucontrol)
> +{
> +	struct snd_usb_babyface *chip = snd_kcontrol_chip(kctl);
> +	int out = kctl->private_value;
> +	bool on = ucontrol->value.integer.value[0];
> +	int ret = 0;
> +
> +	mutex_lock(&chip->mutex);
> +	if (on == chip->loopback[out])
> +		goto out;
> +	ret = bf_loopback_write_map(chip, out, on);
> +	if (ret < 0)
> +		goto out;
> +	/* Single-active model (TotalMix writes one pair at 0x0001, the
> +	 * rest 0x0000): toggling one output clears the others.
> +	 */
> +	memset(chip->loopback, 0, sizeof(chip->loopback));
> +	chip->loopback[out] = on;
> +	ret = 1;
> +out:
> +	mutex_unlock(&chip->mutex);
> +	return ret;
> +}

[Severity: Medium]
When enabling Loopback Switch on one output clears a previously active
chip->loopback[] entry via memset(), should this notify user space with
snd_ctl_notify() for the cleared control (similar to how bf_trim_put()
notifies its sibling control)?

Also, babyface_create_flags() registers Loopback Switch using bf_mute_info
(which declares count = 2 channels), whereas bf_loopback_put() only inspects
ucontrol->value.integer.value[0]. Should Loopback Switch use bf_switch_info
(count = 1) or handle both channels?

[ ... ]

> +static int bf_an12_put(struct snd_kcontrol *kctl,
> +		       struct snd_ctl_elem_value *ucontrol)
> +{
> +	struct snd_usb_babyface *chip = snd_kcontrol_chip(kctl);
> +	bool an12 = ucontrol->value.integer.value[0];
> +	u16 v;
> +	int ret = 0;
> +
> +	mutex_lock(&chip->mutex);
> +	if (an12 == chip->an12)
> +		goto out;
> +	v = (chip->linked ? 0x0400 : 0x0000) | (an12 ? 0x1000 : 0x0000);
> +	ret = bf_vendor_write(chip, BF_REQ_PREAMP, v, 0x1000);
> +	if (ret < 0)
> +		goto out;
> +	ret = bf_vendor_write(chip, BF_REQ_PREAMP_COMMIT, 0x0000, 0x0000);

[Severity: Medium]
Does sending BF_REQ_PREAMP_COMMIT with a hardcoded 0x0000 here (as well as in
bf_link_put() and bf_state_apply_flags()) clear the Instrument 3/4 Boost
reference level on the hardware?

In bf_preamp_state_write(), Boost is distinguished from -10dBV by passing
wValue = 0x0003 instead of 0x0000 to BF_REQ_PREAMP_COMMIT:

    u16 commit = chip->ref_level == BF_REF_LEVEL_BOOST ? 0x0003 : 0x0000;

When chip->ref_level == BF_REF_LEVEL_BOOST, toggling AN 1>2 Switch or AN1/2
Link Switch, or running bf_state_apply_flags() after bf_preamp_state_write(),
sends 0x0000 and drops the hardware back to -10dBV while chip->ref_level still
reports Boost.

[ ... ]

> +static int bf_clock_put(struct snd_kcontrol *kctl,
> +			struct snd_ctl_elem_value *ucontrol)
> +{
> +	struct snd_usb_babyface *chip = snd_kcontrol_chip(kctl);
> +	bool optical = ucontrol->value.enumerated.item[0] != 0;
> +	int ret = 0;

[ ... ]

> +static int bf_optical_put(struct snd_kcontrol *kctl,
> +			  struct snd_ctl_elem_value *ucontrol)
> +{
> +	struct snd_usb_babyface *chip = snd_kcontrol_chip(kctl);
> +	bool spdif = ucontrol->value.enumerated.item[0] != 0;
> +	int ret = 0;

[Severity: Low]
Should bf_clock_put() and bf_optical_put() reject ucontrol->value.enumerated.item[0] >= 2
with -EINVAL when CONFIG_SND_CTL_INPUT_VALIDATION is not enabled, matching
bf_reflevel_put()?

[ ... ]

> +static int bf_ms_put(struct snd_kcontrol *kctl,
> +		     struct snd_ctl_elem_value *ucontrol)
> +{
> +	struct snd_usb_babyface *chip = snd_kcontrol_chip(kctl);
> +	bool on = ucontrol->value.integer.value[0];
> +	int ret = 0;
> +
> +	mutex_lock(&chip->mutex);
> +	if (on == chip->ms_proc)
> +		goto out;
> +	if (on) {
> +		ret = bf_vendor_write(chip, BF_REQ_CROSSPOINT, 0x0000, 0x0035);
> +		if (ret < 0)
> +			goto out;
> +		ret = bf_vendor_write(chip, BF_REQ_CROSSPOINT, 0x0000, 0x004f);
> +		if (ret < 0)
> +			goto out;
> +		ret = bf_vendor_write(chip, BF_REQ_CROSSPOINT, 0x0000, 0x0001);
> +		if (ret < 0)
> +			goto out;
> +		ret = bf_vendor_write(chip, BF_REQ_CROSSPOINT, 0x0000, 0x001b);
> +		if (ret < 0)
> +			goto out;
> +	} else {
> +		ret = bf_vendor_write(chip, BF_REQ_CROSSPOINT,
> +				      chip->xpoint[1][1][0], 0x0001);
> +		if (ret < 0)
> +			goto out;
> +		ret = bf_vendor_write(chip, BF_REQ_CROSSPOINT,
> +				      chip->xpoint[1][1][0], 0x0035);
> +		if (ret < 0)
> +			goto out;
> +		ret = bf_vendor_write(chip, BF_REQ_CROSSPOINT,
> +				      chip->xpoint[1][1][1], 0x001b);
> +		if (ret < 0)
> +			goto out;
> +		ret = bf_vendor_write(chip, BF_REQ_CROSSPOINT,
> +				      chip->xpoint[1][1][1], 0x004f);
> +		if (ret < 0)
> +			goto out;
> +	}

[Severity: High]
When disengaging MS Processor Switch (on == false), does this write output 1's
(PH3/4) cached AN2 crosspoints into output 0's (AN1/2) registers?

Registers 0x0001 (L) and 0x001b (R) belong to output pair 0 (AN1/2), whose
cached AN2 levels are in chip->xpoint[0][1][0] and chip->xpoint[0][1][1],
whereas chip->xpoint[1][1][0..1] holds output pair 1 (PH3/4, registers 0x0035
and 0x004f). Writing chip->xpoint[1][1][0..1] to 0x0001 and 0x001b overwrites
output 0's AN2 routing with output 1's fader values and leaves hardware out of
sync with chip->xpoint[0][1].

[ ... ]

> +static int bf_dim_apply(struct snd_usb_babyface *chip, bool on)
> +{
> +	u16 flag;
> +	int ret;
> +
> +	lockdep_assert_held(&chip->mutex);
> +	if (on) {
> +		chip->dim_saved[0] = chip->master[1][0];
> +		chip->dim_saved[1] = chip->master[1][1];
> +		ret = bf_vendor_write(chip, BF_REQ_GAIN, BF_MASTER_MINUS20_8,
> +				      BF_REG_MASTER_8 + 2 * 1);
> +		if (ret < 0)
> +			return ret;
> +		ret = bf_vendor_write(chip, BF_REQ_GAIN, BF_MASTER_MINUS20_8,
> +				      BF_REG_MASTER_8 + 2 * 1 + 1);
> +		if (ret < 0)
> +			return ret;
> +		flag = bf_flag_cycle[chip->flag_cnt];
> +		chip->flag_cnt = (chip->flag_cnt + 1) & 3;
> +		ret = bf_vendor_write(chip, BF_REQ_CROSSPOINT,
> +				      BF_MASTER_MINUS20_16,
> +				      (BF_REG_MASTER_16 + 2 * 1) | flag);
> +		if (ret < 0)
> +			return ret;
> +		ret = bf_vendor_write(chip, BF_REQ_CROSSPOINT,
> +				      BF_MASTER_MINUS20_16,
> +				      (BF_REG_MASTER_16 + 2 * 1 + 1) | flag);
> +		if (ret < 0)
> +			return ret;
> +		ret = bf_vendor_write(chip, BF_REQ_PREAMP, 0x2000, 0x2000);
> +		if (ret < 0)
> +			return ret;
> +	} else {
> +		ret = bf_vendor_write(chip, BF_REQ_GAIN,
> +				      bf_master_reg8(chip->dim_saved[0]),
> +				      BF_REG_MASTER_8 + 2 * 1);
> +		if (ret < 0)
> +			return ret;
> +		ret = bf_vendor_write(chip, BF_REQ_GAIN,
> +				      bf_master_reg8(chip->dim_saved[1]),
> +				      BF_REG_MASTER_8 + 2 * 1 + 1);
> +		if (ret < 0)
> +			return ret;
> +		flag = bf_flag_cycle[chip->flag_cnt];
> +		chip->flag_cnt = (chip->flag_cnt + 1) & 3;
> +		ret = bf_vendor_write(chip, BF_REQ_CROSSPOINT,
> +				      bf_master_reg16(chip->dim_saved[0]),
> +				      (BF_REG_MASTER_16 + 2 * 1) | flag);
> +		if (ret < 0)
> +			return ret;
> +		ret = bf_vendor_write(chip, BF_REQ_CROSSPOINT,
> +				      bf_master_reg16(chip->dim_saved[1]),
> +				      (BF_REG_MASTER_16 + 2 * 1 + 1) | flag);
> +		if (ret < 0)
> +			return ret;
> +		ret = bf_vendor_write(chip, BF_REQ_PREAMP, 0x0000, 0x2000);
> +		if (ret < 0)
> +			return ret;
> +	}
> +	chip->dim = on;
> +	return 0;
> +}

[Severity: High]
What happens in bf_dim_apply() (and in the chip->dim restore block in
bf_state_apply_flags()) if the Phones output is muted (chip->muted[1] == true)?

Both paths write non-muted gain values (BF_MASTER_MINUS20_8 /
BF_MASTER_MINUS20_16 on engage, or chip->dim_saved[0..1] on release) to the
Phones master registers without checking chip->muted[1]. If Phones was muted,
toggling Dim Switch or running bf_state_apply_flags() unmutes the headphones
on the hardware while chip->muted[1] remains true.

[ ... ]

> +	kctl = snd_ctl_new1(&(struct snd_kcontrol_new){
> +		.iface = SNDRV_CTL_ELEM_IFACE_MIXER,
> +		.name = "Dim Switch",
> +		.info = bf_switch_info,
> +		.get = bf_dim_get,
> +		.put = bf_dim_put,
> +	}, chip);
> +	chip->dim_kctl = kctl;
> +	err = snd_ctl_add(chip->card, kctl);
> +	if (err < 0)
> +		return err;

[Severity: Low]
This isn't a bug, but chip->dim_kctl is assigned here in
babyface_create_flags() and never read anywhere in the driver (a later patch
adds a separate chip->dim_press_kctl for the front-panel button). Can
dim_kctl be dropped from struct snd_usb_babyface?

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

[ ... ]

> @@ -300,14 +317,111 @@ int babyface_restore_state(struct snd_usb_babyface *chip)
>  	return bf_pitch_write(chip, chip->pitch);
>  }
>  
> -/* Re-apply the per-input state that sits on top of the crosspoints
> - * (phase invert, stereo split, trim) after a state restore.  The write
> - * patterns mirror the corresponding _put() handlers.  Caller holds
> - * chip->mutex.
> +/* Re-apply the state that sits on top of the masters and crosspoints
> + * (the routing flags, DIM, phase invert, stereo split, trim) after a
> + * state restore.  The write patterns mirror the corresponding _put()
> + * handlers.  Caller holds chip->mutex.
>   */
>  int bf_state_apply_flags(struct snd_usb_babyface *chip)
>  {
> -	int ret;
> +	int out, ret, on_out = -1;
> +	u16 l, r;
> +
> +	/* Loopback: the full 30-channel map from the cached state (the
> +	 * single-active invariant keeps at most one pair at 0x0001).
> +	 */
> +	for (out = 0; out < 6; out++) {
> +		if (chip->loopback[out]) {
> +			on_out = out;
> +			break;
> +		}
> +	}
> +	ret = bf_loopback_write_map(chip, on_out, on_out >= 0);
> +	if (ret < 0)
> +		return ret;
> +
> +	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;
> +
> +	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;

[Severity: High]
Does running bf_state_apply_flags() unconditionally unmute hardware inputs AN1
and AN2 into the main outputs (AN1/2) on probe and state restore?

In babyface_probe(), babyface_write_default_mixer() intentionally writes 0 to
all hardware input crosspoints (src < BF_SRC_PB1, which includes AN1 at
0x0000/0x001a and AN2 at 0x0001/0x001b on output 0) and caches 0 in
chip->xpoint[0][0..1][0..1] so live inputs are not routed out of the box.

Immediately afterward, babyface_probe() calls bf_state_apply_flags(). Even
when chip->width == 0, this block computes l = 0x1000 and r = 0x1000 and
writes them to registers 0x0000, 0x001a, 0x0001, and 0x001b, routing AN1 and
AN2 at 0 dB into output 0 while chip->xpoint[0][0..1][0..1] remains 0. Because
the cache still holds 0, a subsequent alsactl restore or mixer write of 0 to
those crosspoints hits the early return in bf_xpoint_put() and cannot mute
them.

Also, when chip->width is non-zero, bf_width_put() writes both the low map
(0x0000, 0x001a, 0x0001, 0x001b) and the block-0 map (0x0034, 0x004e, 0x0035,
0x004f), whereas bf_state_apply_flags() only writes the low map. Should the
width writes in bf_state_apply_flags() be guarded by if (chip->width) and
include both maps?

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