On 9/12/23 02:10, Péter Ujfalusi wrote: > > > On 12/09/2023 03:25, Pierre-Louis Bossart wrote: >> >> >>> What we have atm: >>> snd_sof_probe - might be called from wq >>> snd_sof_remove - might be called from wq (cleans up the snd_sof_probe >>> step) >> >> I don't think it's correct, snd_sof_remove cannot be called from a wq. >> The device core knows nothing about workqueues. > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/sound/soc/sof/core.c#n328 > > it is called on the error path of sof_probe_continue(), which can be run > in a workque. > >>> We want a callbacks for hardware/device probing, right, split the >>> snd_sof_probe (and remove) to be able to support a sane level of >>> deferred probing support. >>> >>> With that in mind: >>> snd_sof_device_probe - Not called from wq (to handle deferred probing) >>> snd_sof_probe - might be called from wq >>> >>> snd_sof_remove - might be called from wq (cleans up the snd_sof_probe >>> step) >>> snd_sof_device_remove - Not called from wq (to up the >>> snd_sof_device_probe step) >>> >>> Naming option: s/device/hardware >> >> I like the 'device' hint since it's directly related to the device (or >> subsystem) callbacks. >> >>> However, I think the snd_sof_device_remove itself is redundant and we >>> might not need it at all as in case we have wq and there is a failure in >>> there we do want to release resources as much as possible. The module >>> will be kept loaded (no deferred handling in wq) and that might block >>> PM, other devices to behave correctly. Iow, if the wq has failure we >>> should do a cleanup to the best effort to reach a level like the driver >>> is not even loaded. >> >> If we have a failure in a workqueue used for probe, then we have to >> clean-up everything since nothing in the device core will do so for us. > > Yes, this makes the snd_sof_device_remove() redundant or at least the > definition of it is no longer a mirror of snd_sof_device_probe(): > > snd_sof_device_remove - might be called from wq (cleans up the > snd_sof_device_probe step) > > Any failure in sof_probe_continue() should execute the > snd_sof_device_remove(), snd_sof_remove() is only involved after the > snd_sof_probe() have returned with success. > > I think this makes actually makes sense and it is well defined. > On module remove we need to take into account the case when we have > failed in wq similarly as we do currently (the resources have been freed > up already). Agree, I stand corrected, thanks Peter.
