On 20/01/2023 19:55, Pierre-Louis Bossart wrote:
On 1/19/23 09:35, Richard Fitzgerald wrote:
On 19/1/23 14:48, Pierre-Louis Bossart wrote:
+static int cs42l42_sdw_dai_startup(struct snd_pcm_substream
*substream,
+ struct snd_soc_dai *dai)
+{
+ struct cs42l42_private *cs42l42 =
snd_soc_component_get_drvdata(dai->component);
+
+ if (!cs42l42->init_done)
+ return -ENODEV;
Can this happen? IIRC the ASoC framework would use
pm_runtime_resume_and_get() before .startup, which would guarantee that
the device is initialized, no?
Yes, this can happen. Because of the way that the SoundWire enumeration
was implemented in the core code, it isn't a probe event so we cannot
call snd_soc_register_component() on enumeration because -EPROBE_DEFER
wouldn't be handled. So the snd_soc_register_component() must be called
from probe(). This leaves a limbo situation where we've registered the
driver but in fact don't yet have any hardware. ALSA/ASoC doesn't know
that we've registered before we are functional so they are happy to
go ahead and try to use the soundcard. If for some reason the hardware
failed to enumerate we can get here without having enumerated.
Humm, yes, but you've also made the regmap cache-only, so is there
really a problem?
It's true that normally we go past these stages in cache-only, but that
is because normally (non-Soundwire) we already initialized the hardware
to good state during probe().
If we just carry on when it hasn't enumerated and we haven't initialized
it yet, who knows what will happen if it enumerates some time later.
We could just ignore it and see if anyone has a problem but for the sake
of a couple of lines of code I feel like I'd rather check for it.
FWIW I don't see a startup callback in any other codec driver. It may be
wrong but it's also a sign that this isn't a problem we've seen so far
on existing Intel-based platforms.
It's nicer to do the check in startup() because then the application
open() will fail cleanly. We could delay until prepare - which is the
point we really need the hardware to be accessible - and hope the
hardware enumerated and initialized by that time. But that's not so
nice from the app point of view.
Another way to avoid problems is to rely on the codec component .probe
to check if the SoundWire device is initialized before registering a card.
I just tried with a system where the ACPI info exposes a codec which is
not connected, it fails nicely. That avoids the pitfalls of creating a
card which isn't functional since all dependencies are not met.
[ 64.616530] snd_soc_sof_sdw:mc_probe: sof_sdw sof_sdw: Entry
[ 64.616549] snd_soc_sof_sdw:log_quirks: sof_sdw sof_sdw: quirk
SOF_SDW_PCH_DMIC enabled
[ 64.616559] snd_soc_sof_sdw:sof_card_dai_links_create: sof_sdw
sof_sdw: sdw 2, ssp 0, dmic 2, hdmi 0
[ 64.616587] snd_soc_sof_sdw:init_dai_link: sof_sdw sof_sdw: create
dai link SDW0-Playback, id 0
[ 64.616600] snd_soc_sof_sdw:init_dai_link: sof_sdw sof_sdw: create
dai link SDW0-Capture, id 1
[ 64.616607] snd_soc_sof_sdw:init_dai_link: sof_sdw sof_sdw: create
dai link dmic01, id 2
[ 64.616614] snd_soc_sof_sdw:init_dai_link: sof_sdw sof_sdw: create
dai link dmic16k, id 3
[ 69.757115] rt5682 sdw:0:025d:5682:00: Initialization not complete,
timed out
[ 69.757128] rt5682 sdw:0:025d:5682:00: ASoC: error at
snd_soc_component_probe on sdw:0:025d:5682:00: -110
[ 69.757224] sof_sdw sof_sdw: ASoC: failed to instantiate card -110
[ 69.757734] sof_sdw sof_sdw: snd_soc_register_card failed -110
see
https://elixir.bootlin.com/linux/latest/source/sound/soc/codecs/rt5682.c#L2927
I think this is compatible with the device model and bind/unbind, but it
could be improved with the removal of the wait if we had a way to return
-EPROBEDEFER, and have a mechanism to force the deferred probe work to
be triggered when a device actually shows up. It's a generic problem
that the probe cannot always be a synchronous function but may complete
'later'.
I see what you've done in your patch, but I had already experimented
with this idea and found that the wait_for_completion() can deadlock the
Soundwire core.
