Re: [PATCH v2 3/3] arm64: dts: rockchip: restore RK3308 codec detection resources

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] ASoC: codecs: rk3308: dangling dac_analog and component pointers on unbind
- [Medium] ASoC: codecs: rk3308: rk3308_codec_adc_event() overwrites MIC mute state
- [Medium] ASoC: codecs: rk3308: leaked devm_kmemdup() widget array on component probe
--

commit a099049aa9bf21b04f0558d74e37572e81c21ec8
Author: Xing Zheng <zhengxing@rock-chips.com>

ASoC: codecs: rk3308: restore audio and headphone support

This commit restores the vendor analog enable/disable sequences, reference
current ramps, headphone detection, and output routing for the RK3308 audio
codec across revisions A, B, and C.

> diff --git a/sound/soc/codecs/rk3308_codec.c b/sound/soc/codecs/rk3308_codec.c
> index a3d41cd8150d0..1894e1a93fbf8 100644
> --- a/sound/soc/codecs/rk3308_codec.c
> +++ b/sound/soc/codecs/rk3308_codec.c

[ ... ]

> +static int rk3308_codec_adc_event(struct snd_soc_dapm_widget *w,
> +				  struct snd_kcontrol *kcontrol, int event)
> +{
> +	struct snd_soc_component *component = snd_soc_dapm_to_component(w->dapm);
> +	struct rk3308_codec_priv *rk3308 = snd_soc_component_get_drvdata(component);
> +	unsigned int grp = w->shift;
> +	unsigned int zerocross;
> +
> +	if (event == SND_SOC_DAPM_PRE_PMU) {
> +		regmap_update_bits(rk3308->regmap, RK3308_ADC_ANA_CON07(grp),
> +				   RK3308_ADC_CH1_IN_SEL_MSK | RK3308_ADC_CH2_IN_SEL_MSK,
> +				   RK3308_ADC_CH1_IN_MIC | RK3308_ADC_CH2_IN_MIC);
> +		regmap_set_bits(rk3308->regmap, RK3308_ADC_ANA_CON00(grp),
> +				RK3308_ADC_CH1_MIC_UNMUTE | RK3308_ADC_CH2_MIC_UNMUTE);

[Severity: Medium]
Does rk3308_codec_adc_event() overwrite the user-configured microphone mute
state when capture starts?

Bits 3 (RK3308_ADC_CH1_MIC_UNMUTE) and 7 (RK3308_ADC_CH2_MIC_UNMUTE) of
RK3308_ADC_ANA_CON00(grp) directly back the MIC1..8 Capture Switch controls
in rk3308_codec_controls:

	SOC_SINGLE("MIC1 Capture Switch", RK3308_ADC_ANA_CON00(0), 3, 1, 0),
	SOC_SINGLE("MIC2 Capture Switch", RK3308_ADC_ANA_CON00(0), 7, 1, 0),

And rk3308_codec_initialize() already sets the initial unmuted default after
codec reset:

	/* Match the vendor analog defaults; controls remain adjustable. */
	for (grp = 0; grp < ADC_LR_GROUP_MAX; grp++)
		regmap_set_bits(rk3308->regmap, RK3308_ADC_ANA_CON00(grp),
				BIT(3) | BIT(7));

If userspace mutes a microphone channel via MIC1..8 Capture Switch while
capture is idle and then starts a capture stream, DAPM powers up the
ADC_CURRENT_EN12..78 supply widgets and runs rk3308_codec_adc_event() with
SND_SOC_DAPM_PRE_PMU, which unconditionally sets RK3308_ADC_CH1_MIC_UNMUTE |
RK3308_ADC_CH2_MIC_UNMUTE and clobbers the mute setting.

[ ... ]

> +static int rk3308_codec_add_widgets(struct snd_soc_component *component)
> +{
> +	struct snd_soc_dapm_widget *widgets;
> +	int i, grp, ret;
> +
> +	widgets = devm_kmemdup(component->dev, rk3308_codec_dapm_widgets,
> +			       sizeof(rk3308_codec_dapm_widgets), GFP_KERNEL);
> +	if (!widgets)
> +		return -ENOMEM;

[ ... ]

> +	ret = snd_soc_dapm_new_controls(snd_soc_component_to_dapm(component),
> +					widgets, ARRAY_SIZE(rk3308_codec_dapm_widgets));
> +	if (ret)
> +		return ret;

[Severity: Medium]
Does rk3308_codec_add_widgets() leak the widgets template array across
component probe cycles?

snd_soc_dapm_new_controls() -> snd_soc_dapm_new_control_unlocked() ->
dapm_cnew_widget() duplicates each widget individually with kmemdup() and
does not retain a pointer to the input widgets array after returning.

Because rk3308_codec_add_widgets() runs from the component probe callback
rk3308_codec_probe() and allocates widgets with devm_kmemdup() on the
underlying platform device (component->dev), the temporary array remains
allocated for the lifetime of the platform device and allocates another copy
on every sound card unbind/rebind or deferred probe cycle. Could this use
kmemdup() and kfree() (or __free(kfree)) instead?

[ ... ]

>  static int rk3308_codec_probe(struct snd_soc_component *component)
>  {
>  	struct rk3308_codec_priv *rk3308 = snd_soc_component_get_drvdata(component);
> +	int ret;
>  
> +	ret = rk3308_codec_add_widgets(component);
> +	if (ret)
> +		return ret;
>  	rk3308->component = component;
> +	rk3308->dac_output = 11;
> +	rk3308->output_routes_mask = 3;
>  
>  	rk3308_codec_reset(component);
>  	rk3308_codec_initialize(rk3308);
>  
> -	return 0;
> +	return rk3308_codec_add_mic_boost_controls(component);
>  }

[ ... ]

> +static void rk3308_codec_remove(struct snd_soc_component *component)
> +{
> +	struct rk3308_codec_priv *rk3308 = snd_soc_component_get_drvdata(component);
> +
> +	rk3308_codec_set_jack(component, NULL, NULL);
> +	mutex_lock(&rk3308->io_lock);
> +	rk3308->component = NULL;
> +	mutex_unlock(&rk3308->io_lock);
> +}

[Severity: High]
Can rk3308->dac_analog and rk3308->component become dangling pointers across
sound card unbind/rebind or component probe failure?

In rk3308_codec_dac_event(), the driver caches the component's DAC_CURRENT_EN
DAPM widget pointer:

	rk3308->dac_analog = w;

When the sound card is unbound, snd_soc_component_remove() calls
rk3308_codec_remove() and then snd_soc_dapm_free(), which frees all DAPM
widgets belonging to the component. Neither rk3308_codec_remove() nor
rk3308_codec_probe() clears rk3308->dac_analog. If the card is rebound and
rk3308_codec_output_event() runs before rk3308_codec_dac_event(), it will
see a non-NULL rk3308->dac_analog and dereference the freed widget:

	if (!rk3308->dac_analog)
		return 0;
	...
	if (event == SND_SOC_DAPM_PRE_PMD && !rk3308->dac_analog->new_power)
		return 0;

Additionally, in rk3308_codec_probe(), rk3308->component is set before
calling rk3308_codec_add_mic_boost_controls(component). If that call fails,
snd_soc_component_probe() aborts before marking the component as probed, so
rk3308_codec_remove() is not called during error cleanup and
rk3308->component is left pointing to an unprobed component.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261101044149.2018744-1-zhengxing@rock-chips.com?part=3