[alsa-devel] [PATCH] ASoC: imx-spdif: Use module_init() to handle platform_device_register()

[Mark Brown]

On Mon, Dec 16, 2013 at 06:36:09PM +0800, Nicolin Chen wrote:

> 1) When modprobing modules in order: snd-soc-fsl-spdif -> snd-soc-imx-spdif
> -> snd-soc-spdif-tx/rx, we will fail to create imx-spdif card and dai link
> unless we rmmod snd-soc-imx-spdif and modprobe it again due to inappropriate
> condition of platform_driver_unregister() in probe().

The crucial bit of information here is what the "inappropriate
condition" is - what is this trying to fix?  The code doesn't look
obviously wrong though it does rely on the platform device registration
taking effect immediately to actually probe which is a bit of an
assumption and is probably the thing that messes up the first time
around as the S/PDIF CODEC drivers probably aren't loaded when the card
is trying to probe.  This probably causes us to hit a case where
deferred probe stalls as it's not making any progress as one of the
deferred devices essentially has a dependency on itself.

What should work right now is for the module to ensure that the S/PDIF
CODEC drivers are loaded before it is by linking to some symbol from
there.  This is a total hack though.  Nicer would be for the machine
driver to either directly register S/PDIF DAIs (rather than devices that
then register the DAIs) or to create a card subdevice in parallel with
the S/PDIF ones and hook the card registration off that.

> 2) After "imx-spdif sound-spdif.17: dit-hifi <-> 2004000.spdif mapping ok",
> doing rmmod the imx-spdif would cause kernel Oops:
> ------------[ cut here ]------------
> WARNING: CPU: 0 PID: 1301 at /home/rmk/git/linux-rmk/fs/sysfs/dir.c:915 sysfs_hash_and_remove+0x84/0x90()

Please don't paste entire backtraces into commit messages, they're hard
to read and very long - this is over 100 lines long and doesn't have
anything like that much information.  Either edit them to pull out the
important details or (better yet) explain in words what's going wrong.

I *suspect* this is triggered by removing the CODECs prior to removing
the card which should work but doesn't entirely (we lock modules in to
prevent it triggering normally unless you go and really try by using
unregistration).  Reverting back to normal card registration and doing
that prior to unregistering the CODEC devices ought to fix the unload
case at least.

> Ideally, we should bypass the device_unregister() if getting EPROBE_DEFER,

This won't work, it'll cause the next probe to fail as the driver tries
to reregister an already registered device.  I'm also not sure why you'd
want to do that...

> +static int __init imx_spdif_init(void)
> +{
> +	int ret;
> +
> +	txdev = platform_device_register_simple("spdif-dit", -1, NULL, 0);
> +	if (IS_ERR(txdev))
> +		return PTR_ERR(txdev);
> +
> +	rxdev = platform_device_register_simple("spdif-dir", -1, NULL, 0);
> +	if (IS_ERR(rxdev)) {
> +		ret = PTR_ERR(rxdev);
> +		goto err;
> +	}

This is going to cause these devices to be loaded on unrelated systems
if they have the module compiled in or it gets loaded for some other
reason - we shouldn't do that, it'll cause errors in their registration
paths which may stop them working at all.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://mailman.alsa-project.org/pipermail/alsa-devel/attachments/20131216/f7f8459c/attachment.sig>