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

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

[Tristan Madani]

From: Tristan Madani <tristan@talencesecurity.com>

The cleanup in rt5659_remove() does not reset hs_jack, jack_type, or
the component pointer, and does not mark the regmap cache dirty after
the codec reset.  On card rebind this leaves a dangling hs_jack pointer
and causes regmap_update_bits() to skip re-programming jack-detection
registers because the cache still holds the pre-reset values.

Only clear headset detection state via rt5659_headset_detect() for
JD3-mode jacks.  In HDA header mode the SAR, 4BTN_IL, and IL_IRQ
registers are set up once during I2C probe by
rt5659_intel_hd_header_probe_setup() and are not re-enabled on card
rebind, so clearing them in remove would leave mic detection disabled
after the next bind.

Reset the HDA IRQ polarity bits in RT5659_IRQ_CTRL_1 and
RT5659_IRQ_CTRL_2 to their unplugged defaults before marking the
regmap cache dirty.  Without this, regcache_sync() on rebind restores
plugged-in polarity while hda_hp_plugged and hda_mic_plugged are
false, so the polarity-flip logic in
rt5659_jack_detect_intel_hd_header() never fires and subsequent
plug/unplug events do not generate an interrupt.

If the I2C device unbinds without a card bound, rt5659_remove() is
never called and the IRQ keeps queuing jack_detect_work until devres
frees the private data.  Register a devm action to disable the IRQ
and cancel the delayed work on any I2C teardown path.  Guard
disable_irq() in rt5659_i2c_shutdown() with the irq_disabled flag
to avoid an unbalanced disable when rt5659_remove() already ran.

Store the I2C client pointer in rt5659_priv so all paths can
reference it directly.

Fixes: d3cb2de2479b ("ASoC: rt5659: add rt5659 codec driver")
Cc: stable@vger.kernel.org
Signed-off-by: Tristan Madani <tristan@talencesecurity.com>
---
Changes in v2:
  - Only call rt5659_headset_detect() for JD3-mode jacks; HDA header
    mode registers are set during I2C probe and not re-enabled on rebind
  - Reset HDA IRQ polarity bits to unplugged defaults before
    regcache_mark_dirty() to prevent stale polarity on rebind

 sound/soc/codecs/rt5659.c | 47 ++++++++++++++++++++++++++++++++++++---
 sound/soc/codecs/rt5659.h |  1 +
 2 files changed, 45 insertions(+), 3 deletions(-)

diff --git a/sound/soc/codecs/rt5659.c b/sound/soc/codecs/rt5659.c
index 68f7d5673e190..b28804fa15806 100644
--- a/sound/soc/codecs/rt5659.c
+++ b/sound/soc/codecs/rt5659.c
@@ -3660,6 +3660,16 @@ static int rt5659_set_bias_level(struct snd_soc_component *component,
 	return 0;
 }
 
+static void rt5659_i2c_disable_irq_and_cancel_work(void *data)
+{
+	struct rt5659_priv *rt5659 = data;
+
+	if (rt5659->irq_acquired && !rt5659->irq_disabled)
+		disable_irq(rt5659->i2c->irq);
+	if (rt5659->jack_detect_work_inited)
+		cancel_delayed_work_sync(&rt5659->jack_detect_work);
+}
+
 static int rt5659_probe(struct snd_soc_component *component)
 {
 	struct snd_soc_dapm_context *dapm = snd_soc_component_to_dapm(component);
@@ -3667,8 +3677,10 @@ static int rt5659_probe(struct snd_soc_component *component)
 
 	rt5659->component = component;
 
+	regcache_sync(rt5659->regmap);
+
 	if (rt5659->irq_disabled) {
-		enable_irq(to_i2c_client(component->dev)->irq);
+		enable_irq(rt5659->i2c->irq);
 		rt5659->irq_disabled = false;
 	}
 
@@ -3689,17 +3701,34 @@ 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->pdata.jd_src == RT5659_JD3)
+		rt5659_headset_detect(component, 0);
+
+	rt5659->hs_jack = NULL;
+	rt5659->jack_type = 0;
+	rt5659->hda_hp_plugged = false;
+	rt5659->hda_mic_plugged = false;
+
+	if (rt5659->pdata.jd_src == RT5659_JD_HDA_HEADER) {
+		regmap_update_bits(rt5659->regmap, RT5659_IRQ_CTRL_1,
+				   0x10, 0x10);
+		regmap_update_bits(rt5659->regmap, RT5659_IRQ_CTRL_2,
+				   0x2, 0x0);
+	}
+
+	rt5659->component = NULL;
+
 	regmap_write(rt5659->regmap, RT5659_RESET, 0);
+	regcache_mark_dirty(rt5659->regmap);
 }
 
 #ifdef CONFIG_PM
@@ -4328,6 +4357,13 @@ static int rt5659_i2c_probe(struct i2c_client *i2c)
 				   RT5659_GP1_PIN_MASK, RT5659_GP1_PIN_IRQ);
 	}
 
+	rt5659->i2c = i2c;
+	ret = devm_add_action_or_reset(&i2c->dev,
+				      rt5659_i2c_disable_irq_and_cancel_work,
+				      rt5659);
+	if (ret)
+		return ret;
+
 	return devm_snd_soc_register_component(&i2c->dev,
 			&soc_component_dev_rt5659,
 			rt5659_dai, ARRAY_SIZE(rt5659_dai));
@@ -4337,6 +4373,11 @@ static void rt5659_i2c_shutdown(struct i2c_client *client)
 {
 	struct rt5659_priv *rt5659 = i2c_get_clientdata(client);
 
+	if (rt5659->irq_acquired && !rt5659->irq_disabled)
+		disable_irq(client->irq);
+	if (rt5659->jack_detect_work_inited)
+		cancel_delayed_work_sync(&rt5659->jack_detect_work);
+
 	regmap_write(rt5659->regmap, RT5659_RESET, 0);
 }
 
diff --git a/sound/soc/codecs/rt5659.h b/sound/soc/codecs/rt5659.h
index e10b3844501b9..1118e6ab1887d 100644
--- a/sound/soc/codecs/rt5659.h
+++ b/sound/soc/codecs/rt5659.h
@@ -1793,6 +1793,7 @@ struct rt5659_priv {
 	struct snd_soc_component *component;
 	struct rt5659_platform_data pdata;
 	struct regmap *regmap;
+	struct i2c_client *i2c;
 	struct gpio_desc *gpiod_ldo1_en;
 	struct gpio_desc *gpiod_reset;
 	struct snd_soc_jack *hs_jack;
-- 
2.47.3

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Low] ASoC: rt5659: missing irq_disabled update and duplicated
  teardown logic
--

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

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

This commit resets jack detection state, HDA IRQ polarity bits, and the
regmap cache in rt5659_remove(), and registers a devm action to disable the
IRQ and cancel jack_detect_work during I2C teardown.

> diff --git a/sound/soc/codecs/rt5659.c b/sound/soc/codecs/rt5659.c
> index 68f7d5673e190..b28804fa15806 100644
> --- a/sound/soc/codecs/rt5659.c
> +++ b/sound/soc/codecs/rt5659.c
> @@ -3660,6 +3660,16 @@ static int rt5659_set_bias_level(struct snd_soc_component *component,
>  	return 0;
>  }
>  
> +static void rt5659_i2c_disable_irq_and_cancel_work(void *data)
> +{
> +	struct rt5659_priv *rt5659 = data;
> +
> +	if (rt5659->irq_acquired && !rt5659->irq_disabled)
> +		disable_irq(rt5659->i2c->irq);
> +	if (rt5659->jack_detect_work_inited)
> +		cancel_delayed_work_sync(&rt5659->jack_detect_work);
> +}

[ ... ]

> @@ -3689,17 +3701,34 @@ 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);

[ ... ]

> @@ -4337,6 +4373,11 @@ static void rt5659_i2c_shutdown(struct i2c_client *client)
>  {
>  	struct rt5659_priv *rt5659 = i2c_get_clientdata(client);
>  
> +	if (rt5659->irq_acquired && !rt5659->irq_disabled)
> +		disable_irq(client->irq);
> +	if (rt5659->jack_detect_work_inited)
> +		cancel_delayed_work_sync(&rt5659->jack_detect_work);
> +
>  	regmap_write(rt5659->regmap, RT5659_RESET, 0);
>  }

[Severity: Low]
Should rt5659_i2c_disable_irq_and_cancel_work() set
rt5659->irq_disabled = true and be called from both rt5659_remove() and
rt5659_i2c_shutdown()?

Currently neither rt5659_i2c_disable_irq_and_cancel_work() nor
rt5659_i2c_shutdown() sets rt5659->irq_disabled = true, and rt5659_remove()
does not check !rt5659->irq_disabled.  If rt5659_i2c_shutdown() runs before
an unbind or device removal, rt5659_remove() or the devm action will call
disable_irq() a second time on the already-disabled IRQ, and the teardown
sequence is duplicated in all three places.

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