[PATCH] ASoC: core: Clean up DAPM before the card debugfs

[PATCH] ASoC: core: Clean up DAPM before the card debugfs

[Mark Brown]

Both the card and DAPM cleanups recursively delete their debugfs
directories.  Since the DAPM debugfs subdirectory for the card is
located within the card debugfs this means we end up trying to double
free the DAPM subdirectory.  Reorder the cleanup to free the card
debugfs after we've cleaned up DAPM and it has deleted its own
subdirectory.

Reported-by: Russell King - ARM Linux <linux@armlinux.org.uk>
Signed-off-by: Mark Brown <broonie@kernel.org>
---

Not tested at all yet, just about done for today.

 sound/soc/soc-core.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c
index 16369cad4803..66a33f1e4881 100644
--- a/sound/soc/soc-core.c
+++ b/sound/soc/soc-core.c
@@ -2083,14 +2083,13 @@ static int soc_cleanup_card_resources(struct snd_soc_card *card)
 	/* remove auxiliary devices */
 	soc_remove_aux_devices(card);
 
+	snd_soc_dapm_free(&card->dapm);
 	soc_cleanup_card_debugfs(card);
 
 	/* remove the card */
 	if (card->remove)
 		card->remove(card);
 
-	snd_soc_dapm_free(&card->dapm);
-
 	snd_card_free(card->snd_card);
 	return 0;
 
-- 
2.8.1

Re: [PATCH] ASoC: core: Clean up DAPM before the card debugfs

[Russell King - ARM Linux]

On Thu, Aug 18, 2016 at 07:37:29PM +0100, Mark Brown wrote:
> Both the card and DAPM cleanups recursively delete their debugfs
> directories.  Since the DAPM debugfs subdirectory for the card is
> located within the card debugfs this means we end up trying to double
> free the DAPM subdirectory.  Reorder the cleanup to free the card
> debugfs after we've cleaned up DAPM and it has deleted its own
> subdirectory.
> 
> Reported-by: Russell King - ARM Linux <linux@armlinux.org.uk>
> Signed-off-by: Mark Brown <broonie@kernel.org>
> ---
> 
> Not tested at all yet, just about done for today.

Thanks - that appears to solve the oops on rmmod of
snd-soc-omap-abe-twl6040!

Tested-by: Russell King <rmk+kernel@armlinux.org.uk>

So this allows me to test a bit more... and there's more bad news.
On re-inserting this module...

twl6040 0-004b: Unable to sync registers 0xc-0xd. -121
genirq: Flags mismatch irq 242. 00000004 (McPDM) vs. 00000004 (McPDM)
omap-mcpdm 40132000.mcpdm: Request for IRQ failed
omap-mcpdm 40132000.mcpdm: ASoC: failed to probe DAI 40132000.mcpdm: -16
omap-abe-twl6040 sound: ASoC: failed to instantiate card -16
omap-abe-twl6040 sound: snd_soc_register_card() failed: -16
omap-abe-twl6040: probe of sound failed with error -16

Looks like removing and re-inserting these modules has never been
tested. :(  And oh my...

static int omap_mcpdm_probe(struct snd_soc_dai *dai)
{
        ret = devm_request_irq(mcpdm->dev, mcpdm->irq, omap_mcpdm_irq_handler,
                                0, "McPDM", (void *)mcpdm);

static struct snd_soc_dai_driver omap_mcpdm_dai = {
        .probe = omap_mcpdm_probe,
        .remove = omap_mcpdm_remove,

Using devm_* stuff in a context where it doesn't get automatically
freed when the DAI driver is unbound, and nothing in omap_mcpdm_remove()
to undo that request.

I suspect that isn't the full story though, because of the regmap
complaint (which I've not even looked at yet.)

At the moment, omap_mcpdm_probe() should _not_ be making use of any
devm_* calls, and omap_mcpdm_remove() should be _explicitly_ releasing
everything that was claimed in omap_mcpdm_probe() because ASoC does
nothing with devres groups.  The alternative solution here is that
ASoC gains devres group support so resources claimed in the DAI
driver .probe function can be automatically released in the same way
as happens with normal driver model probe/release and component
helper bind/unbind.

Maybe we should have a test mode in the kernel where every device
goes through a bind-unbind-rebind sequence during boot to test more
of these paths...

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

Re: [PATCH] ASoC: core: Clean up DAPM before the card debugfs

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 1564 bytes --]

On Fri, Aug 19, 2016 at 12:26:42AM +0100, Russell King - ARM Linux wrote:

> So this allows me to test a bit more... and there's more bad news.
> On re-inserting this module...

> Looks like removing and re-inserting these modules has never been
> tested. :(  And oh my...

> static int omap_mcpdm_probe(struct snd_soc_dai *dai)
> {
>         ret = devm_request_irq(mcpdm->dev, mcpdm->irq, omap_mcpdm_irq_handler,
>                                 0, "McPDM", (void *)mcpdm);

> static struct snd_soc_dai_driver omap_mcpdm_dai = {
>         .probe = omap_mcpdm_probe,
>         .remove = omap_mcpdm_remove,

> Using devm_* stuff in a context where it doesn't get automatically
> freed when the DAI driver is unbound, and nothing in omap_mcpdm_remove()
> to undo that request.

Yes, that's obviously bad - and in any case we should request resources
at the device model level regardless of that.

> I suspect that isn't the full story though, because of the regmap
> complaint (which I've not even looked at yet.)

Missed that one?

> Maybe we should have a test mode in the kernel where every device
> goes through a bind-unbind-rebind sequence during boot to test more
> of these paths...

That actually came up independently the other day during the Kernel
Summit discussions, Rob Herring gave it a bit of a go but I'm not sure
if he did it with a view to upstreaming or anything.  It's definitely a
good idea, as I said on that thread I think deferred probe has been one
of the best things to ever happen to kernel error handling and this
would just extend it.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH] ASoC: core: Clean up DAPM before the card debugfs

[Russell King - ARM Linux]

On Fri, Aug 19, 2016 at 06:50:03PM +0100, Mark Brown wrote:
> On Fri, Aug 19, 2016 at 12:26:42AM +0100, Russell King - ARM Linux wrote:
> > I suspect that isn't the full story though, because of the regmap
> > complaint (which I've not even looked at yet.)
> 
> Missed that one?

I think it was something like the following, but I don't remember
off hand without re-running the build that caused it and booting it
again.

twl6040 0-004b: Unable to sync registers 0xc-0xd. -121

I got that while trying my hibernate test (which fails due to no swap)
but it shouldn't cause any other errors:

CPU1: shutdown
PM: Creating hibernation image:
PM: Need to copy 12497 pages
PM: Normal pages needed: 9337 + 1024, available pages: 54137
PM: Hibernation image created (12497 pages copied)
Enabling non-boot CPUs ...
CPU1 is up
PM: noirq thaw of devices complete after 8.209 msecs
PM: early thaw of devices complete after 5.065 msecs
PM: thaw of devices complete after 15.563 msecs
twl6040 0-004b: Unable to sync registers 0xc-0xd. -121
PM: writing image.
PM: Cannot find swap device, try swapon -a.
PM: Cannot get swap writer
PM: Basic memory bitmaps freed
Restarting tasks ... done.

That's probably not regmap's fault, but something else.  I guess TIers
need to dig into that one.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

Re: [PATCH] ASoC: core: Clean up DAPM before the card debugfs

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 491 bytes --]

On Fri, Aug 19, 2016 at 07:04:52PM +0100, Russell King - ARM Linux wrote:

> I think it was something like the following, but I don't remember
> off hand without re-running the build that caused it and booting it
> again.

> twl6040 0-004b: Unable to sync registers 0xc-0xd. -121

...

> That's probably not regmap's fault, but something else.  I guess TIers
> need to dig into that one.

Yeah, smells like an I/O error - possibly attempting to resync before
the I2C bus is up or something?

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH] ASoC: core: Clean up DAPM before the card debugfs

[Peter Ujfalusi]

On 08/19/16 21:11, Mark Brown wrote:
> On Fri, Aug 19, 2016 at 07:04:52PM +0100, Russell King - ARM Linux wrote:
> 
>> I think it was something like the following, but I don't remember
>> off hand without re-running the build that caused it and booting it
>> again.
> 
>> twl6040 0-004b: Unable to sync registers 0xc-0xd. -121
> 
> ...
> 
>> That's probably not regmap's fault, but something else.  I guess TIers
>> need to dig into that one.
> 
> Yeah, smells like an I/O error - possibly attempting to resync before
> the I2C bus is up or something?

twl6040 does not support bulk access:
https://lkml.org/lkml/2016/5/18/296

I will resend the MFD patches asap.

-- 
Péter

Re: [PATCH] ASoC: core: Clean up DAPM before the card debugfs

[Peter Ujfalusi]

On 08/19/16 02:26, Russell King - ARM Linux wrote:
> On Thu, Aug 18, 2016 at 07:37:29PM +0100, Mark Brown wrote:
>> Both the card and DAPM cleanups recursively delete their debugfs
>> directories.  Since the DAPM debugfs subdirectory for the card is
>> located within the card debugfs this means we end up trying to double
>> free the DAPM subdirectory.  Reorder the cleanup to free the card
>> debugfs after we've cleaned up DAPM and it has deleted its own
>> subdirectory.
>>
>> Reported-by: Russell King - ARM Linux <linux@armlinux.org.uk>
>> Signed-off-by: Mark Brown <broonie@kernel.org>
>> ---
>>
>> Not tested at all yet, just about done for today.
> 
> Thanks - that appears to solve the oops on rmmod of
> snd-soc-omap-abe-twl6040!
> 
> Tested-by: Russell King <rmk+kernel@armlinux.org.uk>
> 
> So this allows me to test a bit more... and there's more bad news.
> On re-inserting this module...
> 
> twl6040 0-004b: Unable to sync registers 0xc-0xd. -121
> genirq: Flags mismatch irq 242. 00000004 (McPDM) vs. 00000004 (McPDM)
> omap-mcpdm 40132000.mcpdm: Request for IRQ failed
> omap-mcpdm 40132000.mcpdm: ASoC: failed to probe DAI 40132000.mcpdm: -16
> omap-abe-twl6040 sound: ASoC: failed to instantiate card -16
> omap-abe-twl6040 sound: snd_soc_register_card() failed: -16
> omap-abe-twl6040: probe of sound failed with error -16
> 
> Looks like removing and re-inserting these modules has never been
> tested. :(  And oh my...
> 
> static int omap_mcpdm_probe(struct snd_soc_dai *dai)
> {
>         ret = devm_request_irq(mcpdm->dev, mcpdm->irq, omap_mcpdm_irq_handler,
>                                 0, "McPDM", (void *)mcpdm);
> 
> static struct snd_soc_dai_driver omap_mcpdm_dai = {
>         .probe = omap_mcpdm_probe,
>         .remove = omap_mcpdm_remove,
> 
> Using devm_* stuff in a context where it doesn't get automatically
> freed when the DAI driver is unbound, and nothing in omap_mcpdm_remove()
> to undo that request.
> 
> I suspect that isn't the full story though, because of the regmap
> complaint (which I've not even looked at yet.)
> 
> At the moment, omap_mcpdm_probe() should _not_ be making use of any
> devm_* calls, and omap_mcpdm_remove() should be _explicitly_ releasing
> everything that was claimed in omap_mcpdm_probe() because ASoC does
> nothing with devres groups.  The alternative solution here is that
> ASoC gains devres group support so resources claimed in the DAI
> driver .probe function can be automatically released in the same way
> as happens with normal driver model probe/release and component
> helper bind/unbind.
> 
> Maybe we should have a test mode in the kernel where every device
> goes through a bind-unbind-rebind sequence during boot to test more
> of these paths...

Ouch, we have only tested modules when they are all removed. I will fix it asap.


-- 
Péter

Applied "ASoC: core: Clean up DAPM before the card debugfs" to the asoc tree

[Mark Brown]

The patch

   ASoC: core: Clean up DAPM before the card debugfs

has been applied to the asoc tree at

   git://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git 

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.  

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark

>From d1e81428826221d7ff8e4d83db784d099cd232a7 Mon Sep 17 00:00:00 2001
From: Mark Brown <broonie@kernel.org>
Date: Thu, 18 Aug 2016 19:32:59 +0100
Subject: [PATCH] ASoC: core: Clean up DAPM before the card debugfs

Both the card and DAPM cleanups recursively delete their debugfs
directories.  Since the DAPM debugfs subdirectory for the card is
located within the card debugfs this means we end up trying to double
free the DAPM subdirectory.  Reorder the cleanup to free the card
debugfs after we've cleaned up DAPM and it has deleted its own
subdirectory.

Reported-by: Russell King - ARM Linux <linux@armlinux.org.uk>
Tested-by: Russell King <rmk+kernel@armlinux.org.uk>
Signed-off-by: Mark Brown <broonie@kernel.org>
---
 sound/soc/soc-core.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c
index 16369cad4803..66a33f1e4881 100644
--- a/sound/soc/soc-core.c
+++ b/sound/soc/soc-core.c
@@ -2083,14 +2083,13 @@ static int soc_cleanup_card_resources(struct snd_soc_card *card)
 	/* remove auxiliary devices */
 	soc_remove_aux_devices(card);
 
+	snd_soc_dapm_free(&card->dapm);
 	soc_cleanup_card_debugfs(card);
 
 	/* remove the card */
 	if (card->remove)
 		card->remove(card);
 
-	snd_soc_dapm_free(&card->dapm);
-
 	snd_card_free(card->snd_card);
 	return 0;
 
-- 
2.8.1