Re: [PATCH v2] ASoC: cs35l56: Use devres to destroy workqueue

[Christophe JAILLET <christophe.jaillet@wanadoo.fr>]

Le 04/05/2026 à 17:10, Richard Fitzgerald a écrit :
> In cs35l56_dsp_init() use devm_add_action_or_reset() to add a devres
> cleanup function that flushes and destroys the workqueue. This replaces
> manually calling destroy_workqueue().
> 
> The error path in cs35l56_common_probe() did not call destroy_workqueue().
> Using devres keeps the destroy_workqueue() automatically ordered relative
> to all the other devres-managed cleanup.
> 
> The call to destroy_workqueue() in cs35l56_remove() has been changed to
> flush_workqueue(), as pointed out by Sashiko:
> https://sashiko.dev/#/patchset/20260504110743.3341869-1-rf%40opensource.cirrus.com
> 
> The use of devres cleanup was suggested by https://sashiko.dev to avoid a
> small cleanup inversion window if destroy_workqueue() is called in the
> error path of cs35l56_common_probe().
> 
> Fixes: e49611252900 ("ASoC: cs35l56: Add driver for Cirrus Logic CS35L56")
> Closes: https://sashiko.dev/#/patchset/20260501103002.2843735-1-rf%40opensource.cirrus.com
> Signed-off-by: Richard Fitzgerald <rf@opensource.cirrus.com>
> ---
> Change in V2:
> - Call flush_workqueue() in cs35l56_remove().
> 
>   sound/soc/codecs/cs35l56.c | 16 +++++++++++++++-
>   1 file changed, 15 insertions(+), 1 deletion(-)
> 
> diff --git a/sound/soc/codecs/cs35l56.c b/sound/soc/codecs/cs35l56.c
> index 378017fcea10..26fa94c98775 100644
> --- a/sound/soc/codecs/cs35l56.c
> +++ b/sound/soc/codecs/cs35l56.c
> @@ -1627,6 +1627,14 @@ static int cs35l56_control_add_nop(struct wm_adsp *dsp, struct cs_dsp_coeff_ctl
>   	return 0;
>   }
>   
> +static void cs35l56_dsp_workqueue_destroy(void *data)
> +{
> +	struct workqueue_struct *wq = data;
> +
> +	flush_workqueue(wq);

I don't think that an explicit flush_workqueue() is needed here.

destroy_workqueue() already does the job ( destroy_workqueue() --> 
drain_workqueue() --> __flush_workqueue())

> +	destroy_workqueue(wq);
> +}
> +
>   static int cs35l56_dsp_init(struct cs35l56_private *cs35l56)
>   {
>   	struct wm_adsp *dsp;
> @@ -1636,6 +1644,12 @@ static int cs35l56_dsp_init(struct cs35l56_private *cs35l56)
>   	if (!cs35l56->dsp_wq)
>   		return -ENOMEM;
>   
> +	ret = devm_add_action_or_reset(cs35l56->base.dev,
> +				       cs35l56_dsp_workqueue_destroy,

so, I guess that destroy_workqueue() could even be called directly 
without the need of cs35l56_dsp_workqueue_destroy(). Not sure it is cleaner.

CJ

> +				       cs35l56->dsp_wq);
> +	if (ret)
> +		return ret;
> +
>   	INIT_WORK(&cs35l56->dsp_work, cs35l56_dsp_work);
>   
>   	dsp = &cs35l56->dsp;
> @@ -2066,7 +2080,7 @@ void cs35l56_remove(struct cs35l56_private *cs35l56)
>   	if (cs35l56->base.irq)
>   		devm_free_irq(cs35l56->base.dev, cs35l56->base.irq, &cs35l56->base);
>   
> -	destroy_workqueue(cs35l56->dsp_wq);
> +	flush_workqueue(cs35l56->dsp_wq);
>   
>   	pm_runtime_dont_use_autosuspend(cs35l56->base.dev);
>   	pm_runtime_suspend(cs35l56->base.dev);