linux-kernel - Re: [PATCH v4 4/7] ASoC: codecs: wcd937x: add basic controls

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <c2eb1eb0-2555-6293-999e-ed394e84c946@quicinc.com>
Date: Wed, 22 May 2024 16:49:37 +0530
From: Mohammad Rafi Shaik <quic_mohs@...cinc.com>
To: Mark Brown <broonie@...nel.org>
CC: Srinivas Kandagatla <srinivas.kandagatla@...aro.org>,
        Banajit Goswami
	<bgoswami@...cinc.com>,
        Liam Girdwood <lgirdwood@...il.com>, Rob Herring
	<robh@...nel.org>,
        Krzysztof Kozlowski <krzk+dt@...nel.org>,
        Conor Dooley
	<conor+dt@...nel.org>, Jaroslav Kysela <perex@...ex.cz>,
        Takashi Iwai
	<tiwai@...e.com>, <alsa-devel@...a-project.org>,
        <linux-arm-msm@...r.kernel.org>, <linux-sound@...r.kernel.org>,
        <devicetree@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
        <quic_rohkumar@...cinc.com>, <quic_pkumpatl@...cinc.com>
Subject: Re: [PATCH v4 4/7] ASoC: codecs: wcd937x: add basic controls

On 5/16/2024 5:28 PM, Mark Brown wrote:
> On Thu, May 16, 2024 at 10:17:58AM +0530, Mohammad Rafi Shaik wrote:
> 
>> +static int wcd937x_rx_hph_mode_put(struct snd_kcontrol *kcontrol,
>> +				   struct snd_ctl_elem_value *ucontrol)
>> +{
>> +	struct snd_soc_component *component =
>> +				snd_soc_kcontrol_component(kcontrol);
>> +	struct wcd937x_priv *wcd937x = snd_soc_component_get_drvdata(component);
>> +	u32 mode_val;
>> +

Thanks for the review.

>> +	mode_val = ucontrol->value.enumerated.item[0];
>> +	if (!mode_val) {
>> +		dev_warn(component->dev, "Invalid HPH Mode, default to class_AB\n");
>> +		mode_val = CLS_AB;
> 
> This should be silent (or return an error) otherwise people can DoS the
> logs by just spamming in invalid values.
> 

okay,

I will remove logs.

>> +	}
>> +
>> +	wcd937x->hph_mode = mode_val;
> 
> I would expect there's more validation needed here, this will blindly
> assign any non-zero mode.  Please run the mixer-test selftests on a card
> with this device in it and show the results on future submissions, this
> will detect this and other issues for you.
> 
> Several of the other controls look like they're also missing validation.
> 

I'll add missing validations for all controls.

>> +static int wcd937x_set_swr_port(struct snd_kcontrol *kcontrol,
>> +				struct snd_ctl_elem_value *ucontrol)
>> +{
>> +	struct soc_mixer_control *mixer = (struct soc_mixer_control *)kcontrol->private_value;
>> +	struct snd_soc_component *comp = snd_soc_kcontrol_component(kcontrol);
>> +	struct wcd937x_priv *wcd937x = snd_soc_component_get_drvdata(comp);
>> +	struct wcd937x_sdw_priv *wcd;
>> +	int dai_id = mixer->shift;
>> +	int ch_idx = mixer->reg;
>> +	int portidx;
>> +	bool enable;
>> +
>> +	wcd = wcd937x->sdw_priv[dai_id];
>> +
>> +	portidx = wcd->ch_info[ch_idx].port_num;
>> +
>> +	enable = !!ucontrol->value.integer.value[0];
>> +
>> +	wcd->port_enable[portidx] = enable;
>> +	wcd937x_connect_port(wcd, portidx, ch_idx, enable);
>> +
>> +	return 1;
>> +}
> 
> This unconditionally reports that the value changed so will generate
> spurious events.
>> +
>> +static const char * const rx_hph_mode_mux_text[] = {
>> +	"CLS_H_INVALID", "CLS_H_HIFI", "CLS_H_LP", "CLS_AB", "CLS_H_LOHIFI",
>> +	"CLS_H_ULP", "CLS_AB_HIFI",
>> +};
> 
> It would be more idiomatic to write these in a more human readable form.

i guess this change is fine.

I took the reference from wcd938x and wcd939x.

https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git/tree/sound/soc/codecs/wcd938x.c#n1693

https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git/tree/sound/soc/codecs/wcd939x.c#n1635

> 
>> +static const char * const wcd937x_ear_pa_gain_text[] = {
>> +	"G_6_DB", "G_4P5_DB", "G_3_DB", "G_1P5_DB", "G_0_DB",
>> +	"G_M1P5_DB", "G_M3_DB", "G_M4P5_DB",
>> +	"G_M6_DB", "G_7P5_DB", "G_M9_DB",
>> +	"G_M10P5_DB", "G_M12_DB", "G_M13P5_DB",
>> +	"G_M15_DB", "G_M16P5_DB", "G_M18_DB",
>> +};
> 
> Why is this an enum and not TLV information?

I'll check and add TLV instead enum.

Thanks & Regards,
Rafi