Re: [PATCH] ASoC: rt5659: fix teardown order and stale state across card rebind

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Medium] ASoC: rt5659: rt5659_remove() breaks HDA header mic detect on
  rebind
- [Medium] ASoC: rt5659: rt5659_remove() desynchronizes HDA IRQ polarity
  cache
--

commit b7c86ad3bd52c1cf96525f2f34febf94cfef4a2d
Author: Tristan Madani <tristan@talencesecurity.com>

ASoC: rt5659: fix teardown order and stale state across card rebind

This commit resets component and jack state on removal, marks and resyncs
the regmap cache across card rebind, and registers a devm action to
disable the IRQ and cancel jack detection work on I2C teardown.

> diff --git a/sound/soc/codecs/rt5659.c b/sound/soc/codecs/rt5659.c
> index 68f7d5673e190..14111f40384e5 100644
> --- a/sound/soc/codecs/rt5659.c
> +++ b/sound/soc/codecs/rt5659.c

[ ... ]

> @@ -3689,17 +3701,26 @@ static int rt5659_probe(struct snd_soc_component *component)
>  static void rt5659_remove(struct snd_soc_component *component)
>  {
>  	struct rt5659_priv *rt5659 = snd_soc_component_get_drvdata(component);
> -	struct i2c_client *i2c = to_i2c_client(component->dev);
>  
>  	if (rt5659->irq_acquired) {
> -		disable_irq(i2c->irq);
> +		disable_irq(rt5659->i2c->irq);
>  		rt5659->irq_disabled = true;
>  	}
>  
>  	if (rt5659->jack_detect_work_inited)
>  		cancel_delayed_work_sync(&rt5659->jack_detect_work);
>  
> +	if (rt5659->jack_type)
> +		rt5659_headset_detect(component, 0);

[Severity: Medium]
Should this call to rt5659_headset_detect() be guarded by checking that
rt5659->pdata.jd_src is RT5659_JD3?

When jd_src is RT5659_JD_HDA_HEADER, rt5659_probe() skips registering
rt5659_particular_dapm_widgets ("Mic Det Power"), and
rt5659_intel_hd_header_probe_setup() enables RT5659_4BTN_IL_EN and
RT5659_IL_IRQ_EN once during I2C probe.  If both headphone and microphone
are plugged into the HDA header on removal, rt5659->jack_type equals
SND_JACK_HEADSET, so rt5659_headset_detect(component, 0) logs an unknown
DAPM pin error for "Mic Det Power" and calls
rt5659_enable_push_button_irq(component, false), clearing
RT5659_4BTN_IL_EN and RT5659_IL_IRQ_EN in the regmap cache.  On card
rebind, regcache_sync() writes those cleared bits to the codec without
re-running rt5659_intel_hd_header_probe_setup(), leaving HDA header
microphone detection disabled.

> +
> +	rt5659->hs_jack = NULL;
> +	rt5659->jack_type = 0;
> +	rt5659->hda_hp_plugged = false;
> +	rt5659->hda_mic_plugged = false;
> +	rt5659->component = NULL;
> +
>  	regmap_write(rt5659->regmap, RT5659_RESET, 0);
> +	regcache_mark_dirty(rt5659->regmap);
>  }

[Severity: Medium]
Does resetting hda_hp_plugged and hda_mic_plugged to false without
updating RT5659_IRQ_CTRL_1 and RT5659_IRQ_CTRL_2 leave the cached HDA IRQ
polarity inverted after rebind?

In rt5659_jack_detect_intel_hd_header(), plugging in a jack flips the IRQ
polarity bits in RT5659_IRQ_CTRL_1 (bit 0x10 to 0x0) and
RT5659_IRQ_CTRL_2 (bit 0x2 to 0x2):

sound/soc/codecs/rt5659.c:rt5659_jack_detect_intel_hd_header() {
	...
	if (hp_flag != rt5659->hda_hp_plugged) {
		rt5659->hda_hp_plugged = hp_flag;

		if (hp_flag) {
			regmap_update_bits(rt5659->regmap, RT5659_IRQ_CTRL_1,
				0x10, 0x0);
			rt5659->jack_type |= SND_JACK_HEADPHONE;
		} else {
			regmap_update_bits(rt5659->regmap, RT5659_IRQ_CTRL_1,
				0x10, 0x10);
	...
}

Because the regmap cache retains the plugged-in polarity across
RT5659_RESET, regcache_sync() in rt5659_probe() restores that plugged-in
polarity to the hardware while hda_hp_plugged and hda_mic_plugged are
false.  If a jack is unplugged while the card is unbound,
hp_flag != rt5659->hda_hp_plugged (or mic_flag != rt5659->hda_mic_plugged)
evaluates to false, so the polarity is never restored to the unplugged
state and subsequent plug insertions will not generate an interrupt.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261107162341.1494240-1-tristmd@gmail.com?part=1