Re: [PATCH v7 7/8] ALSA: usb: babyfacepro: add the hardware DSP EQ

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Medium] ALSA: usb: babyfacepro: 64-bit signed division in bf_eq_lc_freq_raw()
- [Medium] ALSA: usb: babyfacepro: bf_eq_band_words() treats Off band type as High Shelf
- [Medium] ALSA: usb: babyfacepro: bf_eq_defaults() leaves slope_db uninitialized at 0
- [Medium] ALSA: usb: babyfacepro: bf_eq_update_strip() ignores bf_eq_write_strip() error
--

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

ALSA: usb: babyfacepro: add the hardware DSP EQ

This commit adds hardware DSP parametric EQ support (3 bands plus low-cut
 across 4 analog input strips) for the RME Babyface Pro. It implements fixed-
point Q27 CORDIC biquad coefficient calculation, bulk endpoint 0x0A block
uploads, and 15 ALSA mixer controls per strip.

> diff --git a/sound/usb/babyfacepro/babyfacepro-ctl.c b/sound/usb/babyfacepro/babyfacepro-ctl.c
> index cf41093dcbcaf..0606bcc609a47 100644
> --- a/sound/usb/babyfacepro/babyfacepro-ctl.c
> +++ b/sound/usb/babyfacepro/babyfacepro-ctl.c
[ ... ]
> +void bf_eq_band_words(s32 *w, int type, s32 freq_hz, s32 q100,
> +		      s32 gain_x10, s32 fs)
> +{
> +	s64 f = freq_hz;
> +	s64 w0, c, s, u, su, cu, alpha, A, sq;
> +	s64 b0, b1, b2, a0, a1, a2;
> +	s64 pi = 0x1921FB54;	/* pi, Q27 */
> +	s64 hpi = 0xC90FDAA;	/* pi/2, Q27 */
> +	s64 t;
> +	int both = 0, cflip = 0;
> +
> +	if (gain_x10 == 0 || q100 <= 0) {
> +		/* Inactive band: identity words (also guards the alpha
> +		 * division below against the default Q=0 the controls start
> +		 * with - a user setting gain before Q used to hit a kernel
> +		 * divide-by-zero oops).
> +		 */
> +		w[0] = 0;
> +		w[1] = 0;
> +		w[2] = 0;
> +		w[3] = 0;
> +		/* w[4] is the shared scale bf_eq_update_strip() reads when the
> +		 * band has a type and a gain but no Q yet (Q defaults to 0).
> +		 * Leaving it unwritten uploaded a garbage shared scale.
> +		 */
> +		w[4] = BF_EQ_Q27;
> +		return;
> +	}
[ ... ]
> +	if (type == 1) {
> +		s64 ta = (alpha * A + (1 << 26)) >> 27;
> +
> +		b0 = BF_EQ_Q27 + ta;
> +		b1 = -2 * c;
> +		b2 = BF_EQ_Q27 - ta;
> +		a0 = BF_EQ_Q27 + div64_s64(alpha * BF_EQ_Q27 + A / 2, A);
> +		a1 = -2 * c;
> +		a2 = BF_EQ_Q27 - div64_s64(alpha * BF_EQ_Q27 + A / 2, A);
> +	} else {
> +		s64 ap1 = A + BF_EQ_Q27;
> +		s64 am1 = A - BF_EQ_Q27;
> +		s64 cp0 = (am1 * c + (1 << 26)) >> 27;	/* (A-1).c */
> +		s64 cp1 = (ap1 * c + (1 << 26)) >> 27;	/* (A+1).c */
> +		s64 ab = (2 * sq * alpha + (1 << 26)) >> 27;
> +
> +		if (type == 2) {	/* low shelf */
> +			b0 = (A * (ap1 - cp0 + ab) + (1 << 26)) >> 27;
> +			b1 = (2 * A * (am1 - cp1) + (1 << 26)) >> 27;
> +			b2 = (A * (ap1 - cp0 - ab) + (1 << 26)) >> 27;
> +			a0 = ap1 + cp0 + ab;
> +			a1 = -2 * (am1 + cp1);
> +			a2 = ap1 + cp0 - ab;
> +		} else {		/* high shelf */
> +			b0 = (A * (ap1 + cp0 + ab) + (1 << 26)) >> 27;
> +			b1 = (-2 * A * (am1 + cp1) + (1 << 26)) >> 27;
> +			b2 = (A * (ap1 + cp0 - ab) + (1 << 26)) >> 27;
> +			a0 = ap1 - cp0 + ab;
> +			a1 = -2 * (am1 - cp1);
> +			a2 = ap1 - cp0 - ab;
> +		}
> +	}

[Severity: Medium]
What happens in bf_eq_band_words() when a band has its type set to 0 ("Off")
while gain_x10 is non-zero?

In bf_eq_defaults(), band_q[] is initialized to 100 while band_type[] starts
at 0 ("Off"). In bf_eq_update_strip(), bf_eq_band_words() is called for all
three bands and w[0..3] are unconditionally copied into e->words[band][0..3]:

	for (band = 0; band < 3; band++) {
		s32 w[5];

		bf_eq_band_words(w, e->band_type[band], e->band_freq[band],
				 e->band_q[band], e->band_gain[band], fs);
		for (i = 0; i < 4; i++)
			e->words[band][i] = w[i];
		if (e->band_type[band] && e->band_gain[band])
			last_c4 = w[4];	/* shared scale: the last band */
	}

Because the early return in bf_eq_band_words() only checks
(gain_x10 == 0 || q100 <= 0) and does not check (type <= 0), setting a
non-zero gain on a band whose type is "Off" (or switching an active band's
type back to "Off" without zeroing its gain) falls through to the else branch
for type == 3 and computes high-shelf coefficients. Those coefficients are
then stored in e->words[band][0..3] without updating last_c4, uploading an
unscaled high-shelf biquad to the hardware DSP instead of the identity
filter.

Should the inactive-band check in bf_eq_band_words() also guard against
type <= 0?

[ ... ]
> +/* The 0x38 low-cut frequency word: round(K.f'.(11656)/(11656+f')) with
> + * K = 11508, f' = f x slope-compensation factor (cap_eq9 fit, 0.003%;
> + * the slope factor keeps the composite -3 dB point constant).
> + */
> +static u32 bf_eq_lc_freq_raw(s32 freq_hz, s32 slope_db)
> +{
> +	s64 f, word;
> +
> +	if (freq_hz <= 0)
> +		return BF_EQ_LC_OFF;
> +	f = freq_hz;
> +	switch (slope_db) {
> +	case 6:
> +		f = f * 15267 / 10000;
> +		break;
> +
> +	case 18:
> +		f = f * 8061 / 10000;
> +		break;
> +
> +	case 24:
> +		f = f * 6977 / 10000;
> +		break;
> +	}
> +	word = (11508 * f * 11656 + (11656 + f) / 2) / (11656 + f);
> +	return (u32)word;
> +}

[Severity: Medium]
Will the plain "/" divisions on s64 variables in bf_eq_lc_freq_raw() cause a
link failure (__divdi3 undefined) on 32-bit architectures?

SND_USB_BABYFACE_PRO has no 64BIT dependency in sound/usb/Kconfig. Here, f
and word are s64, and both the constant divisions by 10000 and the division
by the runtime s64 expression (11656 + f) use the "/" operator directly
rather than div_s64() or div64_s64().

[ ... ]
> +static void bf_eq_update_strip(struct snd_usb_babyface *chip, int strip)
> +{
> +	struct bf_eq_channel *e = &chip->eq[strip];
> +	s32 fs = chip->rate ? chip->rate : 48000;
> +	s32 last_c4 = BF_EQ_Q27;
> +	int band, i;
> +
> +	lockdep_assert_held(&chip->mutex);
> +
> +	for (band = 0; band < 3; band++) {
> +		s32 w[5];
> +
> +		bf_eq_band_words(w, e->band_type[band], e->band_freq[band],
> +				 e->band_q[band], e->band_gain[band], fs);
> +		for (i = 0; i < 4; i++)
> +			e->words[band][i] = w[i];
> +		if (e->band_type[band] && e->band_gain[band])
> +			last_c4 = w[4];	/* shared scale: the last band */
> +	}
> +	e->shared = last_c4;
> +	e->lc_raw = bf_eq_lc_freq_raw(e->lc_hz, e->slope_db);
> +	e->slope = bf_eq_lc_slope_byte(e->slope_db);
> +	bf_eq_write_strip(chip, strip);
> +}
> +
> +/* Recompute + re-upload all four strips (rate change). Caller must
> + * hold chip->mutex - bf_eq_update_strip()/bf_eq_write_strip() are
> + * lock-free by convention (see bf_eq_put()) and the only caller,
> + * babyface_pcm_hw_params(), already holds the lock across the rate
> + * change; locking here too self-deadlocked it (hung-task: "blocked
> + * on a mutex likely owned by" itself, hit via regress.sh's rate
> + * sweep).
> + */
> +void bf_eq_reupload(struct snd_usb_babyface *chip)
> +{
> +	int strip;
> +
> +	for (strip = 0; strip < 4; strip++)
> +		bf_eq_update_strip(chip, strip);
> +}
[ ... ]
> +static int bf_eq_put(struct snd_kcontrol *kctl,
> +		     struct snd_ctl_elem_value *ucontrol)
> +{
[ ... ]
> +	mutex_lock(&chip->mutex);
> +	if (param == 0) {
> +		if (e->on != !!nv) {
> +			e->on = !!nv;
> +			bf_eq_update_strip(chip, strip);
> +			ret = 1;
> +		}
> +	} else if (v && *v != nv) {
> +		*v = nv;
> +		bf_eq_update_strip(chip, strip);
> +		ret = 1;
> +	}
> +	mutex_unlock(&chip->mutex);
> +	return ret;
> +}

[Severity: Medium]
Should bf_eq_update_strip() and bf_eq_reupload() propagate the error returned
by bf_eq_write_strip()?

When kmemdup() or usb_bulk_msg() fails in bf_eq_upload(), bf_eq_write_strip()
returns a negative errno, but bf_eq_update_strip() discards it. In
bf_eq_put(), e->on or *v is updated before calling bf_eq_update_strip() and
ret = 1 is returned unconditionally. If the bulk transfer fails, the cached
state in chip->eq[strip] becomes out of sync with the hardware, and retrying
the same control write from userspace will see *v == nv and do nothing.
Likewise, babyface_resume() calls bf_eq_reupload() without checking if
restoring the DSP state succeeded.

> +
> +/* The Q controls declare 5..1000 (Q x 100) and bf_eq_put() rejects
> + * anything below 5, so a zeroed band_q would read back out of range -
> + * alsactl then fails to restore it (EINVAL) - and could reach the
> + * coefficient math.  Seed the neutral Q 1.0 on every strip at probe,
> + * before any saved state is applied so a restore still overrides it.
> + */
> +void bf_eq_defaults(struct snd_usb_babyface *chip)
> +{
> +	int strip, band;
> +
> +	for (strip = 0; strip < 4; strip++)
> +		for (band = 0; band < 3; band++)
> +			chip->eq[strip].band_q[band] = 100;
> +}

[Severity: Medium]
Does bf_eq_defaults() also need to initialize chip->eq[strip].slope_db to 6?

At probe, chip->eq[strip].slope_db is 0. In bf_eq_get() for param 14 ("EQ Low
Cut Slope"):

	case 14:
		v = &e->slope_db;
		break;
	...
	} else if (param == 14) {
		s32 slope = v ? *v : 6;

		ucontrol->value.enumerated.item[0] =
			slope >= 24 ? 3 : slope >= 18 ? 2 : slope >= 12 ? 1 : 0;

Because v is non-NULL, slope evaluates to *v (0), which bf_eq_get() reports to
userspace as enum item 0 ("6 dB/oct").

However, if the user enables EQ and sets "EQ Low Cut Freq" > 0 without
changing "EQ Low Cut Slope" from its reported "6 dB/oct" setting,
bf_eq_update_strip() passes e->slope_db == 0 to bf_eq_lc_slope_byte(0), which
returns 0 (low-cut disabled) instead of 0x01, and to
bf_eq_lc_freq_raw(e->lc_hz, 0), which skips the case 6 frequency compensation
factor. In addition, writing enum item 0 via bf_eq_put() maps 0 to 6, sees
*v (0) != nv (6), and triggers a bulk upload and a control change notification
even though the control was already reporting item 0.

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