[alsa-devel] [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 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;
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...
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.
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.
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?
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.
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.
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;
participants (3)
-
Mark Brown -
Peter Ujfalusi -
Russell King - ARM Linux