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

[Christophe JAILLET]

Le 05/05/2026 à 11:46, Richard Fitzgerald a écrit :
> On 4/5/26 17:46, Christophe JAILLET wrote:
> > 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@xxxxxxxxxxxxxxxxxxxxx>
> > > ---
> > > 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
>
> Ok, I will change to manually destroying the ASoC component in the
> driver remove(), instead of using devm_snd_soc_register_component().
> That would make it safe to directly call destroy_workqueue() from
> driver remove().

Hi,

That's not my point.
Using devm_add_action_or_reset() here looks safe to me.

What I mean is that maybe using:
	ret = devm_add_action_or_reset(cs35l56->base.dev,
				       destroy_workqueue,

*if* it compiles, could save some extra lines of code, if desired.

CJ