[ASoC] Control access issues

[plbossart]

Reported by @tiwai

A bug report in openSUSE bugzilla below
http://bugzilla.opensuse.org/show_bug.cgi?id=1176200
showed a constant kernel error like:

[ 17.545593] sof_sdw sof_sdw: control 2:0:0:PGA2.0 2 Master Capture Switch:0: value out of range 65536 (0/1) at count 0
[ 17.545597] sof_sdw sof_sdw: control 2:0:0:PGA2.0 2 Master Capture Switch:0: access overflow

by accessing via alsactl. This is caught by CONFIG_SND_CTL_VALIDATION=y.

FWIW, the test system was Dell XPS 17 and with topic/dev-sof-rebase on top of v5.9-rc4.

[plbossart]

Confirmed with topic/sof-dev and thesofproject/kconfig#41

[   15.256711] sof_sdw sof_sdw: control 2:0:0:EQIIR5.0 eqiir_coef_5:0: invalid count 1024
[   15.257305] sof_sdw sof_sdw: control 2:0:0:EQIIR5.0 eqiir_coef_5:0: invalid count 1024
[   15.257788] sof_sdw sof_sdw: control 2:0:0:EQIIR5.0 eqiir_coef_5:0: invalid count 1024
[   15.258287] sof_sdw sof_sdw: control 2:0:0:EQIIR5.0 eqiir_coef_5:0: invalid count 1024
[   15.258718] sof_sdw sof_sdw: control 2:0:0:EQIIR5.0 eqiir_coef_5:0: invalid count 1024
[   15.478450] sof_sdw sof_sdw: control 2:0:0:EQIIR5.0 eqiir_coef_5:0: invalid count 1024
[   15.482394] sof_sdw sof_sdw: control 2:0:0:EQIIR5.0 eqiir_coef_5:0: invalid count 1024
[   15.486264] sof_sdw sof_sdw: control 2:0:0:EQIIR5.0 eqiir_coef_5:0: invalid count 1024
[   15.490854] sof_sdw sof_sdw: control 2:0:0:EQIIR5.0 eqiir_coef_5:0: invalid count 1024
[   15.572108] sof_sdw sof_sdw: control 2:0:0:EQIIR5.0 eqiir_coef_5:0: invalid count 1024
[   15.989799] sof_sdw sof_sdw: control 2:0:0:EQIIR5.0 eqiir_coef_5:0: invalid count 1024
[   15.990468] sof_sdw sof_sdw: control 2:0:0:EQIIR5.0 eqiir_coef_5:0: invalid count 1024
[   16.009891] sof_sdw sof_sdw: control 2:0:0:EQIIR5.0 eqiir_coef_5:0: invalid count 1024
[   16.010027] sof_sdw sof_sdw: control 2:0:0:EQIIR5.0 eqiir_coef_5:0: invalid count 1024
[   16.013913] sof_sdw sof_sdw: control 2:0:0:EQIIR5.0 eqiir_coef_5:0: invalid count 1024
[   16.014041] sof_sdw sof_sdw: control 2:0:0:EQIIR5.0 eqiir_coef_5:0: invalid count 1024
[   16.014154] sof_sdw sof_sdw: control 2:0:0:EQIIR5.0 eqiir_coef_5:0: invalid count 1024
[   16.014272] sof_sdw sof_sdw: control 2:0:0:EQIIR5.0 eqiir_coef_5:0: invalid count 1024
[   16.014379] sof_sdw sof_sdw: control 2:0:0:EQIIR5.0 eqiir_coef_5:0: invalid count 1024
[   16.014488] sof_sdw sof_sdw: control 2:0:0:EQIIR5.0 eqiir_coef_5:0: invalid count 1024
[   16.014593] sof_sdw sof_sdw: control 2:0:0:EQIIR5.0 eqiir_coef_5:0: invalid count 1024

[plbossart]

It gets worse:

root:~# alsamixer
cannot load mixer controls: Invalid argument

[plbossart]

and worse:

root:~# amixer -Dhw:0 controls
numid=31,iface=CARD,name='HDMI/DP,pcm=5 Jack'
numid=37,iface=CARD,name='HDMI/DP,pcm=6 Jack'
numid=43,iface=CARD,name='HDMI/DP,pcm=7 Jack'
numid=28,iface=CARD,name='Headphone Jack'
numid=29,iface=CARD,name='Headset Mic Jack'
numid=26,iface=MIXER,name='Headphone Switch'
numid=32,iface=MIXER,name='IEC958 Playback Con Mask'
numid=38,iface=MIXER,name='IEC958 Playback Con Mask',index=1
numid=44,iface=MIXER,name='IEC958 Playback Con Mask',index=2
numid=33,iface=MIXER,name='IEC958 Playback Pro Mask'
numid=39,iface=MIXER,name='IEC958 Playback Pro Mask',index=1
numid=45,iface=MIXER,name='IEC958 Playback Pro Mask',index=2
numid=34,iface=MIXER,name='IEC958 Playback Default'
numid=40,iface=MIXER,name='IEC958 Playback Default',index=1
numid=46,iface=MIXER,name='IEC958 Playback Default',index=2
numid=35,iface=MIXER,name='IEC958 Playback Switch'
numid=41,iface=MIXER,name='IEC958 Playback Switch',index=1
numid=47,iface=MIXER,name='IEC958 Playback Switch',index=2
amixer: Control hw:0 snd_hctl_elem_info error: Invalid argument

[plbossart]

@kv2019i @ranj063 @juimonen @bardliao @lgirdwood: does this ring a bell? I don't think I've ever enabled this CONFIG_SND_CTL_VALIDATION option.

[plbossart]

Looks like a complete mess to me. We've managed to confuse regular BYTES controls with EXT_BYTES ones.

git grep SOF_TPLG_KCTL_BYTES_ID
include/uapi/sound/sof/tokens.h:#define SOF_TPLG_KCTL_BYTES_ID  258
sound/soc/sof/topology.c:       {SOF_TPLG_KCTL_BYTES_ID, snd_sof_bytes_get, snd_sof_bytes_put},
sound/soc/sof/topology.c:       {SOF_TPLG_KCTL_BYTES_ID, snd_sof_bytes_ext_get, snd_sof_bytes_ext_put},

/* bind a kcontrol to it's IO handlers */
static int soc_tplg_kcontrol_bind_io(struct snd_soc_tplg_ctl_hdr *hdr,
	struct snd_kcontrol_new *k,
	const struct soc_tplg *tplg)
{
	const struct snd_soc_tplg_kcontrol_ops *ops;
	const struct snd_soc_tplg_bytes_ext_ops *ext_ops;
	int num_ops, i;

	if (le32_to_cpu(hdr->ops.info) == SND_SOC_TPLG_CTL_BYTES
		&& k->iface & SNDRV_CTL_ELEM_IFACE_MIXER
		&& k->access & SNDRV_CTL_ELEM_ACCESS_TLV_READWRITE
		&& k->access & SNDRV_CTL_ELEM_ACCESS_TLV_CALLBACK) {
		struct soc_bytes_ext *sbe;
		struct snd_soc_tplg_bytes_control *be;

		sbe = (struct soc_bytes_ext *)k->private_value;
		be = container_of(hdr, struct snd_soc_tplg_bytes_control, hdr);

		/* TLV bytes controls need standard kcontrol info handler,
		 * TLV callback and extended put/get handlers.
		 */
		k->info = snd_soc_bytes_info_ext;
		k->tlv.c = snd_soc_bytes_tlv_callback;

		ext_ops = tplg->bytes_ext_ops;
		num_ops = tplg->bytes_ext_ops_count;

not clear to me if this option ever worked with any topology-based ASoC driver?

[plbossart]

@tiwai I have a fix in PR #2432 for some of the size check issues but I am unable to reproduce the initial problem with the master capture switch. I don't even see how it's possible to set a -1 value for a boolean?

[tiwai]

@tiwai I have a fix in PR #2432 for some of the size check issues but I am unable to reproduce the initial problem with the master capture switch. I don't even see how it's possible to set a -1 value for a boolean?

Sorry for the late follow up, now I'm back from vacation and catching up.

AFAIK, the buggy control is "PGA2.0 2 Master Capture Switch", and we need to check which actual get callback is used for this control. The error indicates that value.integer.value[0] is set wrongly after the get callback was called.

BTW, you didn't have to check about CONFIG_SND_CTL_VALIDATION. The SNDRV_CTL_ELEM_ACCESS_SKIP_CHECK value itself depends on the kconfig, and if it's disabled, the *_SKIP_CHECK is 0, i.e. the compiler would optimize out in anyway.
An extra IS_ENABLED() check doesn't hurt, of course, though.

[plbossart]

welcome back @tiwai!
I don't know what you meant in the initial bug report with "accessing via alsactl". Can we reproduce the error with regular amixer commands? What's so specific about alsactl?

[tiwai]

I don't think there is anything special in alsactl. It just reads or writes the all control elements sequentially at storing and restoring, and the same error must be triggered by other mixer applications. Maybe it's just a matter which element was accessed, and alsactl hits easily because the control has a smaller numid.

[plbossart]

when I tried to set the value of a switch to 65536 as in your bug report amixer politely declined to do so. I wonder if we are missing a range check somewhere.

[tiwai]

I don't think that's an error at writing the value, but it's at reading. Maybe reading from the very beginning, uninitialized state?

[plbossart]

oh, read access. I completely missed this. It's perfectly possible that we have races between alsactl and topology.

@ranj063 @juimonen that goes back to my 2-yr old question: how are topology controls initialized...

[ranj063]

oh, read access. I completely missed this. It's perfectly possible that we have races between alsactl and topology.

@ranj063 @juimonen that goes back to my 2-yr old question: how are topology controls initialized...

@plbossart I think we might have a bug in the FW with the get() command related to the switch. After topology loading is done, we cache all kcontrol values by reading back the values from the FW and I see that even for the switch, we read the current volume gain which is set to MAX. Let me submit a fix.

[ranj063]

@plbossart I dont have a device to test this, could you please confirm if it resolves the issue on your CML SDW device?

[plbossart]

I don't have a sequence to test this either, i don't use alsactl (or I don't have control on it). I'd need the help of @tiwai to define a 'manual' sequence to test this.

[tiwai]

I also don't know. It's a reporter's machine, and this could be from an action by any program, not necessarily alsactl.
In theory, "amixer contents" should trigger the similar effect.