Re: [PATCH v2 9/9] ASoC: tegra_sgtl5000: fix platform name vs. of_node assignement

[Marcel Ziswiler]

On Wed, 2018-10-17 at 13:32 +0100, Jon Hunter wrote:
> On 16/10/2018 11:47, Marcel Ziswiler wrote:
> > From: Marcel Ziswiler <marcel.ziswiler@xxxxxxxxxxx>
> > 
> > This fixes the following error as seen post commit daecf46ee0e5
> > ("ASoC: soc-core: use snd_soc_dai_link_component for platform"):
> > 
> > tegra-snd-sgtl5000 sound: ASoC: Both platform name/of_node are set
> > for
> >  sgtl5000
> > tegra-snd-sgtl5000 sound: ASoC: failed to init link sgtl5000
> > tegra-snd-sgtl5000 sound: snd_soc_register_card failed (-22)
> > tegra-snd-sgtl5000: probe of sound failed with error -22
> > 
> > Signed-off-by: Marcel Ziswiler <marcel.ziswiler@xxxxxxxxxxx>
> > 
> > ---
> > 
> > Changes in v2: New patch
> > 
> >  sound/soc/tegra/tegra_sgtl5000.c | 5 +++++
> >  1 file changed, 5 insertions(+)
> > 
> > diff --git a/sound/soc/tegra/tegra_sgtl5000.c
> > b/sound/soc/tegra/tegra_sgtl5000.c
> > index 901457da25ec..eb702925cac3 100644
> > --- a/sound/soc/tegra/tegra_sgtl5000.c
> > +++ b/sound/soc/tegra/tegra_sgtl5000.c
> > @@ -168,6 +168,11 @@ static int tegra_sgtl5000_driver_probe(struct
> > platform_device *pdev)
> >  	return 0;
> >  
> >  err_fini_utils:
> > +	if (tegra_sgtl5000_dai.platform) {
> > +		devm_kfree(&pdev->dev,
> > tegra_sgtl5000_dai.platform);
> > +		tegra_sgtl5000_dai.platform = NULL;
> > +	}
> > +
> >  	tegra_asoc_utils_fini(&machine->util_data);
> >  err_put_cpu_of_node:
> >  	of_node_put(tegra_sgtl5000_dai.cpu_of_node);
> > 
> 
> Where is the above allocated?

snd_soc_init_platform() in sound/soc/soc-core.c

> I don't see it allocated in this driver
> AFAICT. If it is not then it does not seem right to free something
> that
> we have not allocated in this driver. I would have assumed it was
> allocated by snd_soc_init_platform() in which case it should not be
> necessary to free because that function uses devm_kzalloc().

That is kind of what I assumed as well.

> What am I missing here?

That is actually a very very good question. Unfortunately, since above
mentioned commit which is part of the bigger multi-platform (or
whatever one may call it) rework done by folks on CC things start
falling apart.

I should maybe rather have phrased this one as an RFC.

Some facts from my humble investigation so far:

- The issue does not exhibit itself on Apalis/Colibri T30 where
probably the order of things being initialised is slightly different.

- Bisecting points to the above mentioned commit being to blame.
However there is no way to just revert that commit as it is part of the
bigger multi-platform rework.

- Somehow it has to do with probe deferral. Basically, platform gets
allocated in snd_soc_init_platform() but due to GPIO/I2C whatever not
being ready the SGTL5000 codec aka dai_link can not yet be found and
therefore it probe defers as follows:

[    2.166517] tegra30-i2s 70301200.i2s: DMA channels sourced from
device 70300000.ahub
[    2.176043] tegra-snd-sgtl5000 sound: ASoC: CODEC DAI sgtl5000 not
registered
[    2.183241] tegra-snd-sgtl5000 sound: snd_soc_register_card failed
(-517)

- Somewhere thereafter platform seems to get stumped onto (e.g. its
name rather than being null now is bogus. Unfortunately, it is not just
the name as clearing just that did not really help (sgtl5000 codec gets
instantiated but trying to play audio always returned -EINVAL).

- The second time around snd_soc_init_platform() re-uses previously
allocated platform now corrupt triggering a check in soc-core
concerning platform name and of_node both being set (as noted in above
commit message).

Some questions:

- How exactly are devm allocations supposed to work concerning probe
deferrals?

- Does or should the platform get cleared during a probe deferral
cycle?

- If so, why does that not work?

- Or is some special implicit probe deferral handling missing in soc-
core?

I'm happy to try more things and/or provide more debugging output if
needed. Just let me know.

Thanks Jon!

> Cheers
> Jon

Cheers

Marcel