On 9/5/23 08:37, Pierre-Louis Bossart wrote: > > > On 9/1/23 08:44, Péter Ujfalusi wrote: >> >> >> On 01/09/2023 15:15, Kai Vehmanen wrote: >>> Hi, >>> >>> On Wed, 30 Aug 2023, Maarten Lankhorst wrote: >>> >>>> With the upcoming changes for i915/Xe driver relying on the >>>> -EPROBE_DEFER mechanism, we need to have a first pass of the probe >>>> which cannot be pushed to a workqueue. Introduce 2 new optional >>>> callbacks. >>> [...] >>>> diff --git a/sound/soc/sof/core.c b/sound/soc/sof/core.c >>>> index 30db685cc5f4b..54c384a5d6140 100644 >>>> --- a/sound/soc/sof/core.c >>>> +++ b/sound/soc/sof/core.c >>>> @@ -327,8 +327,6 @@ static int sof_probe_continue(struct snd_sof_dev *sdev) >>>> dsp_err: >>>> snd_sof_remove(sdev); >>>> probe_err: >>>> - sof_ops_free(sdev); >>>> - >>> >>> this seems a bit out-of-place in this patch. It seems a valid change, >>> but not really related to this patch, right? >> >> The ops needs to be preserved even if the wq fails since the patch wants >> to call snd_sof_remove_no_wq() unconditionally on remove. >> >>> We seem to have a related fix waiting to be sent to alsa-devel, by >>> Peter: >>> "ASoC: SOF: core: Only call sof_ops_free() on remove if the probe wa" >>> https://github.com/thesofproject/linux/pull/4515 >> >> I guess we can revert that in sof-dev, if this is the preferred way? >> >>> ... not yet in Mark's tree. >>> >>> Otherwise patch looks good to me. >> >> I would have not created the snd_sof_remove_no_wq() as it makes not much >> functional sense. >> It might be even better if the remove in the wq would do the >> hda_codec_i915_exit() as the module will remain in there until the user >> removes it. > > I think find all this very confusing, because there is no workqueue used > in the remove steps. The workqueue is only used ONCE during the probe. Maybe we should just remove any references to workqueues, and have probe_start (cannot run in a wq) probe (may run in a wq) remove (cannot run in a wq, needs to call cancel_work_sync() if the probe runs in a wq) remove_last (cannot run in a wq, releases all resources acquired in probe_start) Or something similar that shows the symmetry between steps and when the wq is allowed.
