Re: [PATCH] ASoC: cs42l43: Move shutter IRQ handling into a worker thread

[Takashi Iwai <tiwai@xxxxxxx>]

On Mon, 29 Jul 2024 17:59:32 +0200,
Charles Keepax wrote:
> 
> The microphone/speaker privacy shutter ALSA control handlers need to
> call pm_runtime_resume, since the hardware needs to be powered up to
> check the hardware state of the shutter. The IRQ handler for the
> shutters also needs to notify the ALSA control to inform user-space
> the shutters updated. However this leads to a mutex inversion, between
> the sdw_dev_lock and the controls_rwsem.

That's bad, how does the mutex inversion look like?  Generally
speaking, a call of snd_ctl_notify() from an irq handler is a very
standard procedure, and it should work without too much workaround.


thanks,

Takashi

> 
> To avoid this mutex inversion add a work item for handling the shutter
> IRQ and move the notifies into that.
> 
> Signed-off-by: Charles Keepax <ckeepax@xxxxxxxxxxxxxxxxxxxxx>
> ---
>  sound/soc/codecs/cs42l43.c | 43 +++++++++++++++++++++++++++-----------
>  sound/soc/codecs/cs42l43.h |  3 +++
>  2 files changed, 34 insertions(+), 12 deletions(-)
> 
> diff --git a/sound/soc/codecs/cs42l43.c b/sound/soc/codecs/cs42l43.c
> index 92674314227c4..74cb396ec0576 100644
> --- a/sound/soc/codecs/cs42l43.c
> +++ b/sound/soc/codecs/cs42l43.c
> @@ -249,9 +249,10 @@ CS42L43_IRQ_COMPLETE(spkr_startup)
>  CS42L43_IRQ_COMPLETE(spkl_startup)
>  CS42L43_IRQ_COMPLETE(load_detect)
>  
> -static irqreturn_t cs42l43_mic_shutter(int irq, void *data)
> +static void cs42l43_mic_shutter_work(struct work_struct *work)
>  {
> -	struct cs42l43_codec *priv = data;
> +	struct cs42l43_codec *priv = container_of(work, struct cs42l43_codec,
> +						  mic_shutter_work);
>  	static const char * const controls[] = {
>  		"Decimator 1 Switch",
>  		"Decimator 2 Switch",
> @@ -263,32 +264,47 @@ static irqreturn_t cs42l43_mic_shutter(int irq, void *data)
>  	dev_dbg(priv->dev, "Microphone shutter changed\n");
>  
>  	if (!priv->component)
> -		return IRQ_NONE;
> +		return;
>  
>  	for (i = 0; i < ARRAY_SIZE(controls); i++) {
> -		ret = snd_soc_component_notify_control(priv->component,
> -						       controls[i]);
> +		ret = snd_soc_component_notify_control(priv->component, controls[i]);
>  		if (ret)
> -			return IRQ_NONE;
> +			dev_err(priv->dev, "Failed to notify control %s: %d\n",
> +				controls[i], ret);
>  	}
> +}
> +
> +static irqreturn_t cs42l43_mic_shutter(int irq, void *data)
> +{
> +	struct cs42l43_codec *priv = data;
> +
> +	queue_work(system_wq, &priv->mic_shutter_work);
>  
>  	return IRQ_HANDLED;
>  }
>  
> -static irqreturn_t cs42l43_spk_shutter(int irq, void *data)
> +static void cs42l43_spk_shutter_work(struct work_struct *work)
>  {
> -	struct cs42l43_codec *priv = data;
> +	struct cs42l43_codec *priv = container_of(work, struct cs42l43_codec,
> +						  mic_shutter_work);
> +	const char * const control = "Speaker Digital Switch";
>  	int ret;
>  
>  	dev_dbg(priv->dev, "Speaker shutter changed\n");
>  
>  	if (!priv->component)
> -		return IRQ_NONE;
> +		return;
>  
> -	ret = snd_soc_component_notify_control(priv->component,
> -					       "Speaker Digital Switch");
> +	ret = snd_soc_component_notify_control(priv->component, control);
>  	if (ret)
> -		return IRQ_NONE;
> +		dev_err(priv->dev, "Failed to notify control %s: %d\n", control, ret);
> +}
> +
> +static irqreturn_t cs42l43_spk_shutter(int irq, void *data)
> +{
> +	struct cs42l43_codec *priv = data;
> +
> +	queue_work(system_wq, &priv->spk_shutter_work);
>  
>  	return IRQ_HANDLED;
>  }
> @@ -2280,6 +2296,9 @@ static int cs42l43_codec_probe(struct platform_device *pdev)
>  	INIT_WORK(&priv->button_release_work, cs42l43_button_release_work);
>  	INIT_WORK(&priv->hp_ilimit_work, cs42l43_hp_ilimit_work);
>  
> +	INIT_WORK(&priv->mic_shutter_work, cs42l43_mic_shutter_work);
> +	INIT_WORK(&priv->spk_shutter_work, cs42l43_spk_shutter_work);
> +
>  	pm_runtime_set_autosuspend_delay(priv->dev, 100);
>  	pm_runtime_use_autosuspend(priv->dev);
>  	pm_runtime_set_active(priv->dev);
> diff --git a/sound/soc/codecs/cs42l43.h b/sound/soc/codecs/cs42l43.h
> index 9924c13e1eb53..5592aab5c8042 100644
> --- a/sound/soc/codecs/cs42l43.h
> +++ b/sound/soc/codecs/cs42l43.h
> @@ -100,6 +100,9 @@ struct cs42l43_codec {
>  	struct delayed_work hp_ilimit_clear_work;
>  	bool hp_ilimited;
>  	int hp_ilimit_count;
> +
> +	struct work_struct mic_shutter_work;
> +	struct work_struct spk_shutter_work;
>  };
>  
>  #if IS_REACHABLE(CONFIG_SND_SOC_CS42L43_SDW)
> -- 
> 2.39.2
> 
>