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

[sashiko-bot@kernel.org]

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