2018-03-05 06:48:13

by Kirill Marinushkin

Subject: [PATCH] staging: bcm2835-audio: Release resources on module_exit()

In the current implementation, `rmmod snd_bcm2835` does not release
resources properly. It causes an oops when trying to list sound devices.

This commit fixes it.

Steps to reproduce:

~~~~
$ rmmod snd_bcm2835
$ aplay -L
[ 138.648130] Unable to handle kernel paging request at virtual address 7f1343c0
[ 138.660415] pgd = ad8f0000
[ 138.665567] [7f1343c0] *pgd=3864c811, *pte=00000000, *ppte=00000000
[ 138.674887] Internal error: Oops: 7 [#1] SMP ARM
[ 138.683571] Modules linked in: sha256_generic cfg80211 rfkill snd_pcm snd_timer
snd fixed uio_pdrv_genirq uio ip_tables x_tables ipv6 [last unloaded: snd_bcm2835
]
[ 138.706594] CPU: 3 PID: 463 Comm: aplay Tainted: G WC 4.15.0-rc1-v
7+ #6
[ 138.719833] Hardware name: BCM2835
[ 138.726016] task: b877ac00 task.stack: aebec000
[ 138.733408] PC is at try_module_get+0x38/0x24c
[ 138.740813] LR is at snd_ctl_open+0x58/0x194 [snd]
[ 138.748485] pc : [<801c4d5c>] lr : [<7f0e6b2c>] psr: 20000013
[ 138.757709] sp : aebedd60 ip : aebedd88 fp : aebedd84
[ 138.765884] r10: 00000000 r9 : 00000004 r8 : 7f0ed440
[ 138.774040] r7 : b7e469b0 r6 : 7f0e6b2c r5 : afd91900 r4 : 7f1343c0
[ 138.783571] r3 : aebec000 r2 : 00000001 r1 : b877ac00 r0 : 7f1343c0
[ 138.793084] Flags: nzCv IRQs on FIQs on Mode SVC_32 ISA ARM Segment user
[ 138.803300] Control: 10c5387d Table: 2d8f006a DAC: 00000055
[ 138.812064] Process aplay (pid: 463, stack limit = 0xaebec210)
[ 138.820868] Stack: (0xaebedd60 to 0xaebee000)
[ 138.828207] dd60: 00000000 b848d000 afd91900 00000000 b7e469b0 7f0ed440 aebedda4 aebedd88
[ 138.842371] dd80: 7f0e6b2c 801c4d30 afd91900 7f0ea4dc 00000000 b7e469b0 aebeddcc aebedda8
[ 138.856611] dda0: 7f0e250c 7f0e6ae0 7f0e2464 b8478ec0 b7e469b0 afd91900 7f0ea388 00000000
[ 138.870864] ddc0: aebeddf4 aebeddd0 802ce590 7f0e2470 8090ab64 afd91900 afd91900 b7e469b0
[ 138.885301] dde0: afd91908 802ce4e4 aebede1c aebeddf8 802c57b4 802ce4f0 afd91900 aebedea8
[ 138.900110] de00: b7fa4c00 00000000 00000000 00000004 aebede3c aebede20 802c6ba8 802c56b4
[ 138.915260] de20: aebedea8 00000000 aebedf5c 00000000 aebedea4 aebede40 802d9a68 802c6b58
[ 138.930661] de40: b874ddd0 00000000 00000000 00000001 00000041 00000000 afd91900 aebede70
[ 138.946402] de60: 00000000 00000000 00000002 b7e469b0 b8a87610 b8d6ab80 801852f8 00080000
[ 138.962314] de80: aebedf5c aebedea8 00000001 80108464 aebec000 00000000 aebedf4c aebedea8
[ 138.978414] dea0: 802dacd4 802d970c b8a87610 b8d6ab80 a7982bc6 00000009 af363019 b9231480
[ 138.994617] dec0: 00000000 b8c038a0 b7e469b0 00000101 00000002 00000238 00000000 00000000
[ 139.010823] dee0: 00000000 aebedee8 00080000 0000000f aebedf3c aebedf00 802ed7e4 80843f94
[ 139.027025] df00: 00000003 00080000 b9231490 b9231480 00000000 00080000 af363000 00000000
[ 139.043229] df20: 00000005 00000002 ffffff9c 00000000 00080000 ffffff9c af363000 00000003
[ 139.059430] df40: aebedf94 aebedf50 802c6f70 802dac70 aebec000 00000000 00000001 00000000
[ 139.075629] df60: 00020000 00000004 00000100 00000001 7ebe577c 0002e038 00000000 00000005
[ 139.091828] df80: 80108464 aebec000 aebedfa4 aebedf98 802c7060 802c6e6c 00000000 aebedfa8
[ 139.108025] dfa0: 801082c0 802c7040 7ebe577c 0002e038 7ebe577c 00080000 00000b98 e81c8400
[ 139.124222] dfc0: 7ebe577c 0002e038 00000000 00000005 7ebe57e4 00a20af8 7ebe57f0 76f87394
[ 139.140419] dfe0: 00000000 7ebe55c4 76ec88e8 76df1d9c 60000010 7ebe577c 00000000 00000000
[ 139.156715] [<801c4d5c>] (try_module_get) from [<7f0e6b2c>] (snd_ctl_open+0x58/0x194 [snd])
[ 139.173222] [<7f0e6b2c>] (snd_ctl_open [snd]) from [<7f0e250c>] (snd_open+0xa8/0x14c [snd])
[ 139.189683] [<7f0e250c>] (snd_open [snd]) from [<802ce590>] (chrdev_open+0xac/0x188)
[ 139.205465] [<802ce590>] (chrdev_open) from [<802c57b4>] (do_dentry_open+0x10c/0x314)
[ 139.221347] [<802c57b4>] (do_dentry_open) from [<802c6ba8>] (vfs_open+0x5c/0x88)
[ 139.236788] [<802c6ba8>] (vfs_open) from [<802d9a68>] (path_openat+0x368/0x944)
[ 139.248270] [<802d9a68>] (path_openat) from [<802dacd4>] (do_filp_open+0x70/0xc4)
[ 139.263731] [<802dacd4>] (do_filp_open) from [<802c6f70>] (do_sys_open+0x110/0x1d4)
[ 139.279378] [<802c6f70>] (do_sys_open) from [<802c7060>] (SyS_open+0x2c/0x30)
[ 139.290647] [<802c7060>] (SyS_open) from [<801082c0>] (ret_fast_syscall+0x0/0x28)
[ 139.306021] Code: e3c3303f e5932004 e2822001 e5832004 (e5943000)
[ 139.316265] ---[ end trace 7f3f7f6193b663ed ]---
[ 139.324956] note: aplay[463] exited with preempt_count 1
~~~~

Signed-off-by: Kirill Marinushkin <[email protected]>
Cc: Eric Anholt <[email protected]>
Cc: Stefan Wahren <[email protected]>
Cc: Greg Kroah-Hartman <[email protected]>
Cc: Florian Fainelli <[email protected]>
Cc: Ray Jui <[email protected]>
Cc: Scott Branden <[email protected]>
Cc: [email protected]
Cc: Michael Zoran <[email protected]>
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
---
.../staging/vc04_services/bcm2835-audio/bcm2835.c | 37 +++++++++++-----------
1 file changed, 19 insertions(+), 18 deletions(-)

diff --git a/drivers/staging/vc04_services/bcm2835-audio/bcm2835.c b/drivers/staging/vc04_services/bcm2835-audio/bcm2835.c
index 8f2d508183b2..01187df7d85f 100644
--- a/drivers/staging/vc04_services/bcm2835-audio/bcm2835.c
+++ b/drivers/staging/vc04_services/bcm2835-audio/bcm2835.c
@@ -36,6 +36,11 @@ MODULE_PARM_DESC(enable_compat_alsa,
static void snd_devm_unregister_child(struct device *dev, void *res)
{
struct device *childdev = *(struct device **)res;
+ struct snd_card *snd_card =
+ (struct snd_card *)dev_get_drvdata(childdev);
+
+ snd_card_free(snd_card);
+ dev_set_drvdata(childdev, NULL);

device_unregister(childdev);
}
@@ -61,6 +66,11 @@ static int snd_devm_add_child(struct device *dev, struct device *child)
return 0;
}

+static void snd_devm_release(struct device *dev)
+{
+ /* avoid warnings when releasing the resources */
+}
+
static struct device *
snd_create_device(struct device *parent,
struct device_driver *driver,
@@ -76,6 +86,7 @@ snd_create_device(struct device *parent,
device_initialize(device);
device->parent = parent;
device->driver = driver;
+ device->release = &snd_devm_release;

dev_set_name(device, "%s", name);

@@ -130,31 +141,14 @@ static int snd_bcm2835_create(struct snd_card *card,
return 0;
}

-static void snd_devm_card_free(struct device *dev, void *res)
-{
- struct snd_card *snd_card = *(struct snd_card **)res;
-
- snd_card_free(snd_card);
-}
-
static struct snd_card *snd_devm_card_new(struct device *dev)
{
- struct snd_card **dr;
struct snd_card *card;
int ret;

- dr = devres_alloc(snd_devm_card_free, sizeof(*dr), GFP_KERNEL);
- if (!dr)
- return ERR_PTR(-ENOMEM);
-
ret = snd_card_new(dev, -1, NULL, THIS_MODULE, 0, &card);
- if (ret) {
- devres_free(dr);
+ if (ret)
return ERR_PTR(ret);
- }
-
- *dr = card;
- devres_add(dev, dr);

return card;
}
@@ -414,6 +408,12 @@ static int snd_bcm2835_alsa_probe_dt(struct platform_device *pdev)
return 0;
}

+static int snd_bcm2835_alsa_remove(struct platform_device *pdev)
+{
+ /* trigger snd_devm_unregister_child() */
+ return 0;
+}
+
#ifdef CONFIG_PM

static int snd_bcm2835_alsa_suspend(struct platform_device *pdev,
@@ -437,6 +437,7 @@ MODULE_DEVICE_TABLE(of, snd_bcm2835_of_match_table);

static struct platform_driver bcm2835_alsa0_driver = {
.probe = snd_bcm2835_alsa_probe_dt,
+ .remove = snd_bcm2835_alsa_remove,
#ifdef CONFIG_PM
.suspend = snd_bcm2835_alsa_suspend,
.resume = snd_bcm2835_alsa_resume,
--
2.13.6

2018-03-06 12:16:13

by Greg Kroah-Hartman

Subject: Re: [PATCH] staging: bcm2835-audio: Release resources on module_exit()

On Mon, Mar 05, 2018 at 06:52:54AM +0100, Kirill Marinushkin wrote:
> In the current implementation, `rmmod snd_bcm2835` does not release
> resources properly. It causes an oops when trying to list sound devices.
>
> This commit fixes it.
>
> Steps to reproduce:
>
> ~~~~
> $ rmmod snd_bcm2835
> $ aplay -L
> [ 138.648130] Unable to handle kernel paging request at virtual address 7f1343c0
> [ 138.660415] pgd = ad8f0000
> [ 138.665567] [7f1343c0] *pgd=3864c811, *pte=00000000, *ppte=00000000
> [ 138.674887] Internal error: Oops: 7 [#1] SMP ARM
> [ 138.683571] Modules linked in: sha256_generic cfg80211 rfkill snd_pcm snd_timer
> snd fixed uio_pdrv_genirq uio ip_tables x_tables ipv6 [last unloaded: snd_bcm2835
> ]
> [ 138.706594] CPU: 3 PID: 463 Comm: aplay Tainted: G WC 4.15.0-rc1-v
> 7+ #6
> [ 138.719833] Hardware name: BCM2835
> [ 138.726016] task: b877ac00 task.stack: aebec000
> [ 138.733408] PC is at try_module_get+0x38/0x24c
> [ 138.740813] LR is at snd_ctl_open+0x58/0x194 [snd]
> [ 138.748485] pc : [<801c4d5c>] lr : [<7f0e6b2c>] psr: 20000013
> [ 138.757709] sp : aebedd60 ip : aebedd88 fp : aebedd84
> [ 138.765884] r10: 00000000 r9 : 00000004 r8 : 7f0ed440
> [ 138.774040] r7 : b7e469b0 r6 : 7f0e6b2c r5 : afd91900 r4 : 7f1343c0
> [ 138.783571] r3 : aebec000 r2 : 00000001 r1 : b877ac00 r0 : 7f1343c0
> [ 138.793084] Flags: nzCv IRQs on FIQs on Mode SVC_32 ISA ARM Segment user
> [ 138.803300] Control: 10c5387d Table: 2d8f006a DAC: 00000055
> [ 138.812064] Process aplay (pid: 463, stack limit = 0xaebec210)
> [ 138.820868] Stack: (0xaebedd60 to 0xaebee000)
> [ 138.828207] dd60: 00000000 b848d000 afd91900 00000000 b7e469b0 7f0ed440 aebedda4 aebedd88
> [ 138.842371] dd80: 7f0e6b2c 801c4d30 afd91900 7f0ea4dc 00000000 b7e469b0 aebeddcc aebedda8
> [ 138.856611] dda0: 7f0e250c 7f0e6ae0 7f0e2464 b8478ec0 b7e469b0 afd91900 7f0ea388 00000000
> [ 138.870864] ddc0: aebeddf4 aebeddd0 802ce590 7f0e2470 8090ab64 afd91900 afd91900 b7e469b0
> [ 138.885301] dde0: afd91908 802ce4e4 aebede1c aebeddf8 802c57b4 802ce4f0 afd91900 aebedea8
> [ 138.900110] de00: b7fa4c00 00000000 00000000 00000004 aebede3c aebede20 802c6ba8 802c56b4
> [ 138.915260] de20: aebedea8 00000000 aebedf5c 00000000 aebedea4 aebede40 802d9a68 802c6b58
> [ 138.930661] de40: b874ddd0 00000000 00000000 00000001 00000041 00000000 afd91900 aebede70
> [ 138.946402] de60: 00000000 00000000 00000002 b7e469b0 b8a87610 b8d6ab80 801852f8 00080000
> [ 138.962314] de80: aebedf5c aebedea8 00000001 80108464 aebec000 00000000 aebedf4c aebedea8
> [ 138.978414] dea0: 802dacd4 802d970c b8a87610 b8d6ab80 a7982bc6 00000009 af363019 b9231480
> [ 138.994617] dec0: 00000000 b8c038a0 b7e469b0 00000101 00000002 00000238 00000000 00000000
> [ 139.010823] dee0: 00000000 aebedee8 00080000 0000000f aebedf3c aebedf00 802ed7e4 80843f94
> [ 139.027025] df00: 00000003 00080000 b9231490 b9231480 00000000 00080000 af363000 00000000
> [ 139.043229] df20: 00000005 00000002 ffffff9c 00000000 00080000 ffffff9c af363000 00000003
> [ 139.059430] df40: aebedf94 aebedf50 802c6f70 802dac70 aebec000 00000000 00000001 00000000
> [ 139.075629] df60: 00020000 00000004 00000100 00000001 7ebe577c 0002e038 00000000 00000005
> [ 139.091828] df80: 80108464 aebec000 aebedfa4 aebedf98 802c7060 802c6e6c 00000000 aebedfa8
> [ 139.108025] dfa0: 801082c0 802c7040 7ebe577c 0002e038 7ebe577c 00080000 00000b98 e81c8400
> [ 139.124222] dfc0: 7ebe577c 0002e038 00000000 00000005 7ebe57e4 00a20af8 7ebe57f0 76f87394
> [ 139.140419] dfe0: 00000000 7ebe55c4 76ec88e8 76df1d9c 60000010 7ebe577c 00000000 00000000
> [ 139.156715] [<801c4d5c>] (try_module_get) from [<7f0e6b2c>] (snd_ctl_open+0x58/0x194 [snd])
> [ 139.173222] [<7f0e6b2c>] (snd_ctl_open [snd]) from [<7f0e250c>] (snd_open+0xa8/0x14c [snd])
> [ 139.189683] [<7f0e250c>] (snd_open [snd]) from [<802ce590>] (chrdev_open+0xac/0x188)
> [ 139.205465] [<802ce590>] (chrdev_open) from [<802c57b4>] (do_dentry_open+0x10c/0x314)
> [ 139.221347] [<802c57b4>] (do_dentry_open) from [<802c6ba8>] (vfs_open+0x5c/0x88)
> [ 139.236788] [<802c6ba8>] (vfs_open) from [<802d9a68>] (path_openat+0x368/0x944)
> [ 139.248270] [<802d9a68>] (path_openat) from [<802dacd4>] (do_filp_open+0x70/0xc4)
> [ 139.263731] [<802dacd4>] (do_filp_open) from [<802c6f70>] (do_sys_open+0x110/0x1d4)
> [ 139.279378] [<802c6f70>] (do_sys_open) from [<802c7060>] (SyS_open+0x2c/0x30)
> [ 139.290647] [<802c7060>] (SyS_open) from [<801082c0>] (ret_fast_syscall+0x0/0x28)
> [ 139.306021] Code: e3c3303f e5932004 e2822001 e5832004 (e5943000)
> [ 139.316265] ---[ end trace 7f3f7f6193b663ed ]---
> [ 139.324956] note: aplay[463] exited with preempt_count 1
> ~~~~
>
> Signed-off-by: Kirill Marinushkin <[email protected]>
> Cc: Eric Anholt <[email protected]>
> Cc: Stefan Wahren <[email protected]>
> Cc: Greg Kroah-Hartman <[email protected]>
> Cc: Florian Fainelli <[email protected]>
> Cc: Ray Jui <[email protected]>
> Cc: Scott Branden <[email protected]>
> Cc: [email protected]
> Cc: Michael Zoran <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> Cc: [email protected]
> Cc: [email protected]
> ---
> .../staging/vc04_services/bcm2835-audio/bcm2835.c | 37 +++++++++++-----------
> 1 file changed, 19 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/staging/vc04_services/bcm2835-audio/bcm2835.c b/drivers/staging/vc04_services/bcm2835-audio/bcm2835.c
> index 8f2d508183b2..01187df7d85f 100644
> --- a/drivers/staging/vc04_services/bcm2835-audio/bcm2835.c
> +++ b/drivers/staging/vc04_services/bcm2835-audio/bcm2835.c
> @@ -36,6 +36,11 @@ MODULE_PARM_DESC(enable_compat_alsa,
> static void snd_devm_unregister_child(struct device *dev, void *res)
> {
> struct device *childdev = *(struct device **)res;
> + struct snd_card *snd_card =
> + (struct snd_card *)dev_get_drvdata(childdev);
> +
> + snd_card_free(snd_card);
> + dev_set_drvdata(childdev, NULL);
>
> device_unregister(childdev);
> }
> @@ -61,6 +66,11 @@ static int snd_devm_add_child(struct device *dev, struct device *child)
> return 0;
> }
>
> +static void snd_devm_release(struct device *dev)
> +{
> + /* avoid warnings when releasing the resources */
> +}

As per the in-kernel documentation, I get to make fun of you for trying
to "fool" the kernel here :(

Please please please do not do stuff like this. There is a good reason
the kernel is warning you that you need to do something "real" here. I
didn't put that logic in the kernel for no good reason, so don't try to
"shut it up" by being sneaky and trying to fool it.

Properly clean up your memory here, that is what is needed.

thanks,

greg k-h

2018-03-06 17:43:05

by Kirill Marinushkin

Subject: Re: [PATCH] staging: bcm2835-audio: Release resources on module_exit()

On 03/06/18 13:14, Greg Kroah-Hartman wrote:
> On Mon, Mar 05, 2018 at 06:52:54AM +0100, Kirill Marinushkin wrote:
>> In the current implementation, `rmmod snd_bcm2835` does not release
>> resources properly. It causes an oops when trying to list sound devices.
>>
>> This commit fixes it.
>>
>> Steps to reproduce:
>>
>> ~~~~
>> $ rmmod snd_bcm2835
>> $ aplay -L
>> [ 138.648130] Unable to handle kernel paging request at virtual address 7f1343c0
>> [ 138.660415] pgd = ad8f0000
>> [ 138.665567] [7f1343c0] *pgd=3864c811, *pte=00000000, *ppte=00000000
>> [ 138.674887] Internal error: Oops: 7 [#1] SMP ARM
>> [ 138.683571] Modules linked in: sha256_generic cfg80211 rfkill snd_pcm snd_timer
>> snd fixed uio_pdrv_genirq uio ip_tables x_tables ipv6 [last unloaded: snd_bcm2835
>> ]
>> [ 138.706594] CPU: 3 PID: 463 Comm: aplay Tainted: G WC 4.15.0-rc1-v
>> 7+ #6
>> [ 138.719833] Hardware name: BCM2835
>> [ 138.726016] task: b877ac00 task.stack: aebec000
>> [ 138.733408] PC is at try_module_get+0x38/0x24c
>> [ 138.740813] LR is at snd_ctl_open+0x58/0x194 [snd]
>> [ 138.748485] pc : [<801c4d5c>] lr : [<7f0e6b2c>] psr: 20000013
>> [ 138.757709] sp : aebedd60 ip : aebedd88 fp : aebedd84
>> [ 138.765884] r10: 00000000 r9 : 00000004 r8 : 7f0ed440
>> [ 138.774040] r7 : b7e469b0 r6 : 7f0e6b2c r5 : afd91900 r4 : 7f1343c0
>> [ 138.783571] r3 : aebec000 r2 : 00000001 r1 : b877ac00 r0 : 7f1343c0
>> [ 138.793084] Flags: nzCv IRQs on FIQs on Mode SVC_32 ISA ARM Segment user
>> [ 138.803300] Control: 10c5387d Table: 2d8f006a DAC: 00000055
>> [ 138.812064] Process aplay (pid: 463, stack limit = 0xaebec210)
>> [ 138.820868] Stack: (0xaebedd60 to 0xaebee000)
>> [ 138.828207] dd60: 00000000 b848d000 afd91900 00000000 b7e469b0 7f0ed440 aebedda4 aebedd88
>> [ 138.842371] dd80: 7f0e6b2c 801c4d30 afd91900 7f0ea4dc 00000000 b7e469b0 aebeddcc aebedda8
>> [ 138.856611] dda0: 7f0e250c 7f0e6ae0 7f0e2464 b8478ec0 b7e469b0 afd91900 7f0ea388 00000000
>> [ 138.870864] ddc0: aebeddf4 aebeddd0 802ce590 7f0e2470 8090ab64 afd91900 afd91900 b7e469b0
>> [ 138.885301] dde0: afd91908 802ce4e4 aebede1c aebeddf8 802c57b4 802ce4f0 afd91900 aebedea8
>> [ 138.900110] de00: b7fa4c00 00000000 00000000 00000004 aebede3c aebede20 802c6ba8 802c56b4
>> [ 138.915260] de20: aebedea8 00000000 aebedf5c 00000000 aebedea4 aebede40 802d9a68 802c6b58
>> [ 138.930661] de40: b874ddd0 00000000 00000000 00000001 00000041 00000000 afd91900 aebede70
>> [ 138.946402] de60: 00000000 00000000 00000002 b7e469b0 b8a87610 b8d6ab80 801852f8 00080000
>> [ 138.962314] de80: aebedf5c aebedea8 00000001 80108464 aebec000 00000000 aebedf4c aebedea8
>> [ 138.978414] dea0: 802dacd4 802d970c b8a87610 b8d6ab80 a7982bc6 00000009 af363019 b9231480
>> [ 138.994617] dec0: 00000000 b8c038a0 b7e469b0 00000101 00000002 00000238 00000000 00000000
>> [ 139.010823] dee0: 00000000 aebedee8 00080000 0000000f aebedf3c aebedf00 802ed7e4 80843f94
>> [ 139.027025] df00: 00000003 00080000 b9231490 b9231480 00000000 00080000 af363000 00000000
>> [ 139.043229] df20: 00000005 00000002 ffffff9c 00000000 00080000 ffffff9c af363000 00000003
>> [ 139.059430] df40: aebedf94 aebedf50 802c6f70 802dac70 aebec000 00000000 00000001 00000000
>> [ 139.075629] df60: 00020000 00000004 00000100 00000001 7ebe577c 0002e038 00000000 00000005
>> [ 139.091828] df80: 80108464 aebec000 aebedfa4 aebedf98 802c7060 802c6e6c 00000000 aebedfa8
>> [ 139.108025] dfa0: 801082c0 802c7040 7ebe577c 0002e038 7ebe577c 00080000 00000b98 e81c8400
>> [ 139.124222] dfc0: 7ebe577c 0002e038 00000000 00000005 7ebe57e4 00a20af8 7ebe57f0 76f87394
>> [ 139.140419] dfe0: 00000000 7ebe55c4 76ec88e8 76df1d9c 60000010 7ebe577c 00000000 00000000
>> [ 139.156715] [<801c4d5c>] (try_module_get) from [<7f0e6b2c>] (snd_ctl_open+0x58/0x194 [snd])
>> [ 139.173222] [<7f0e6b2c>] (snd_ctl_open [snd]) from [<7f0e250c>] (snd_open+0xa8/0x14c [snd])
>> [ 139.189683] [<7f0e250c>] (snd_open [snd]) from [<802ce590>] (chrdev_open+0xac/0x188)
>> [ 139.205465] [<802ce590>] (chrdev_open) from [<802c57b4>] (do_dentry_open+0x10c/0x314)
>> [ 139.221347] [<802c57b4>] (do_dentry_open) from [<802c6ba8>] (vfs_open+0x5c/0x88)
>> [ 139.236788] [<802c6ba8>] (vfs_open) from [<802d9a68>] (path_openat+0x368/0x944)
>> [ 139.248270] [<802d9a68>] (path_openat) from [<802dacd4>] (do_filp_open+0x70/0xc4)
>> [ 139.263731] [<802dacd4>] (do_filp_open) from [<802c6f70>] (do_sys_open+0x110/0x1d4)
>> [ 139.279378] [<802c6f70>] (do_sys_open) from [<802c7060>] (SyS_open+0x2c/0x30)
>> [ 139.290647] [<802c7060>] (SyS_open) from [<801082c0>] (ret_fast_syscall+0x0/0x28)
>> [ 139.306021] Code: e3c3303f e5932004 e2822001 e5832004 (e5943000)
>> [ 139.316265] ---[ end trace 7f3f7f6193b663ed ]---
>> [ 139.324956] note: aplay[463] exited with preempt_count 1
>> ~~~~
>>
>> Signed-off-by: Kirill Marinushkin <[email protected]>
>> Cc: Eric Anholt <[email protected]>
>> Cc: Stefan Wahren <[email protected]>
>> Cc: Greg Kroah-Hartman <[email protected]>
>> Cc: Florian Fainelli <[email protected]>
>> Cc: Ray Jui <[email protected]>
>> Cc: Scott Branden <[email protected]>
>> Cc: [email protected]
>> Cc: Michael Zoran <[email protected]>
>> Cc: [email protected]
>> Cc: [email protected]
>> Cc: [email protected]
>> Cc: [email protected]
>> ---
>> .../staging/vc04_services/bcm2835-audio/bcm2835.c | 37 +++++++++++-----------
>> 1 file changed, 19 insertions(+), 18 deletions(-)
>>
>> diff --git a/drivers/staging/vc04_services/bcm2835-audio/bcm2835.c b/drivers/staging/vc04_services/bcm2835-audio/bcm2835.c
>> index 8f2d508183b2..01187df7d85f 100644
>> --- a/drivers/staging/vc04_services/bcm2835-audio/bcm2835.c
>> +++ b/drivers/staging/vc04_services/bcm2835-audio/bcm2835.c
>> @@ -36,6 +36,11 @@ MODULE_PARM_DESC(enable_compat_alsa,
>> static void snd_devm_unregister_child(struct device *dev, void *res)
>> {
>> struct device *childdev = *(struct device **)res;
>> + struct snd_card *snd_card =
>> + (struct snd_card *)dev_get_drvdata(childdev);
>> +
>> + snd_card_free(snd_card);
>> + dev_set_drvdata(childdev, NULL);
>>
>> device_unregister(childdev);
>> }
>> @@ -61,6 +66,11 @@ static int snd_devm_add_child(struct device *dev, struct device *child)
>> return 0;
>> }
>>
>> +static void snd_devm_release(struct device *dev)
>> +{
>> + /* avoid warnings when releasing the resources */
>> +}
> As per the in-kernel documentation, I get to make fun of you for trying
> to "fool" the kernel here :(
>
> Please please please do not do stuff like this. There is a good reason
> the kernel is warning you that you need to do something "real" here. I
> didn't put that logic in the kernel for no good reason, so don't try to
> "shut it up" by being sneaky and trying to fool it.
>
> Properly clean up your memory here, that is what is needed.
>
> thanks,
>
> greg k-h

Hello Greg,

Thank you for the clarification. I didn't realize the problem, but now I understand
and agree, that my implementation is not correct.

I apologize for this misunderstanding. I will release the resources in a proper way,
and I will send the cleaner solution as a patch v2.

Best Regards,
Kirill

2018-03-13 19:35:25

by Kirill Marinushkin

Subject: [PATCH v2] staging: bcm2835-audio: Release resources on module_exit()

In the current implementation, `rmmod snd_bcm2835` does not release
resources properly. It causes an oops when trying to list sound devices.

This commit fixes it.

The details WRT allocation / free are described below.

Device structure WRT allocation:

pdev
\childdev[]
\card
\chip
\pcm
\ctl

Allocation / register sequence:

* childdev: devm_kzalloc - freed during driver detach
* childdev: device_initialize - freed during device_unregister
* pdev: devres_alloc - freed during driver detach
* childdev: device_add - removed during device_unregister
* pdev, childdev: devres_add - freed during driver detach
* card: snd_card_new - freed during snd_card_free
* chip: kzalloc - freed during kfree
* card, chip: snd_device_new - freed during snd_device_free
* chip: new_pcm - TODO: free pcm
* chip: new_ctl - TODO: free ctl
* card: snd_card_register - unregistered during snd_card_free

Free / unregister sequence:

* card: snd_card_free
* card, chip: snd_device_free
* childdev: device_unregister
* chip: kfree

Steps to reproduce the issue before this commit:

~~~~
$ rmmod snd_bcm2835
$ aplay -L
[ 138.648130] Unable to handle kernel paging request at virtual address 7f1343c0
[ 138.660415] pgd = ad8f0000
[ 138.665567] [7f1343c0] *pgd=3864c811, *pte=00000000, *ppte=00000000
[ 138.674887] Internal error: Oops: 7 [#1] SMP ARM
[ 138.683571] Modules linked in: sha256_generic cfg80211 rfkill snd_pcm snd_timer
snd fixed uio_pdrv_genirq uio ip_tables x_tables ipv6 [last unloaded: snd_bcm2835
]
[ 138.706594] CPU: 3 PID: 463 Comm: aplay Tainted: G WC 4.15.0-rc1-v
7+ #6
[ 138.719833] Hardware name: BCM2835
[ 138.726016] task: b877ac00 task.stack: aebec000
[ 138.733408] PC is at try_module_get+0x38/0x24c
[ 138.740813] LR is at snd_ctl_open+0x58/0x194 [snd]
[ 138.748485] pc : [<801c4d5c>] lr : [<7f0e6b2c>] psr: 20000013
[ 138.757709] sp : aebedd60 ip : aebedd88 fp : aebedd84
[ 138.765884] r10: 00000000 r9 : 00000004 r8 : 7f0ed440
[ 138.774040] r7 : b7e469b0 r6 : 7f0e6b2c r5 : afd91900 r4 : 7f1343c0
[ 138.783571] r3 : aebec000 r2 : 00000001 r1 : b877ac00 r0 : 7f1343c0
[ 138.793084] Flags: nzCv IRQs on FIQs on Mode SVC_32 ISA ARM Segment user
[ 138.803300] Control: 10c5387d Table: 2d8f006a DAC: 00000055
[ 138.812064] Process aplay (pid: 463, stack limit = 0xaebec210)
[ 138.820868] Stack: (0xaebedd60 to 0xaebee000)
[ 138.828207] dd60: 00000000 b848d000 afd91900 00000000 b7e469b0 7f0ed440 aebedda4 aebedd88
[ 138.842371] dd80: 7f0e6b2c 801c4d30 afd91900 7f0ea4dc 00000000 b7e469b0 aebeddcc aebedda8
[ 138.856611] dda0: 7f0e250c 7f0e6ae0 7f0e2464 b8478ec0 b7e469b0 afd91900 7f0ea388 00000000
[ 138.870864] ddc0: aebeddf4 aebeddd0 802ce590 7f0e2470 8090ab64 afd91900 afd91900 b7e469b0
[ 138.885301] dde0: afd91908 802ce4e4 aebede1c aebeddf8 802c57b4 802ce4f0 afd91900 aebedea8
[ 138.900110] de00: b7fa4c00 00000000 00000000 00000004 aebede3c aebede20 802c6ba8 802c56b4
[ 138.915260] de20: aebedea8 00000000 aebedf5c 00000000 aebedea4 aebede40 802d9a68 802c6b58
[ 138.930661] de40: b874ddd0 00000000 00000000 00000001 00000041 00000000 afd91900 aebede70
[ 138.946402] de60: 00000000 00000000 00000002 b7e469b0 b8a87610 b8d6ab80 801852f8 00080000
[ 138.962314] de80: aebedf5c aebedea8 00000001 80108464 aebec000 00000000 aebedf4c aebedea8
[ 138.978414] dea0: 802dacd4 802d970c b8a87610 b8d6ab80 a7982bc6 00000009 af363019 b9231480
[ 138.994617] dec0: 00000000 b8c038a0 b7e469b0 00000101 00000002 00000238 00000000 00000000
[ 139.010823] dee0: 00000000 aebedee8 00080000 0000000f aebedf3c aebedf00 802ed7e4 80843f94
[ 139.027025] df00: 00000003 00080000 b9231490 b9231480 00000000 00080000 af363000 00000000
[ 139.043229] df20: 00000005 00000002 ffffff9c 00000000 00080000 ffffff9c af363000 00000003
[ 139.059430] df40: aebedf94 aebedf50 802c6f70 802dac70 aebec000 00000000 00000001 00000000
[ 139.075629] df60: 00020000 00000004 00000100 00000001 7ebe577c 0002e038 00000000 00000005
[ 139.091828] df80: 80108464 aebec000 aebedfa4 aebedf98 802c7060 802c6e6c 00000000 aebedfa8
[ 139.108025] dfa0: 801082c0 802c7040 7ebe577c 0002e038 7ebe577c 00080000 00000b98 e81c8400
[ 139.124222] dfc0: 7ebe577c 0002e038 00000000 00000005 7ebe57e4 00a20af8 7ebe57f0 76f87394
[ 139.140419] dfe0: 00000000 7ebe55c4 76ec88e8 76df1d9c 60000010 7ebe577c 00000000 00000000
[ 139.156715] [<801c4d5c>] (try_module_get) from [<7f0e6b2c>] (snd_ctl_open+0x58/0x194 [snd])
[ 139.173222] [<7f0e6b2c>] (snd_ctl_open [snd]) from [<7f0e250c>] (snd_open+0xa8/0x14c [snd])
[ 139.189683] [<7f0e250c>] (snd_open [snd]) from [<802ce590>] (chrdev_open+0xac/0x188)
[ 139.205465] [<802ce590>] (chrdev_open) from [<802c57b4>] (do_dentry_open+0x10c/0x314)
[ 139.221347] [<802c57b4>] (do_dentry_open) from [<802c6ba8>] (vfs_open+0x5c/0x88)
[ 139.236788] [<802c6ba8>] (vfs_open) from [<802d9a68>] (path_openat+0x368/0x944)
[ 139.248270] [<802d9a68>] (path_openat) from [<802dacd4>] (do_filp_open+0x70/0xc4)
[ 139.263731] [<802dacd4>] (do_filp_open) from [<802c6f70>] (do_sys_open+0x110/0x1d4)
[ 139.279378] [<802c6f70>] (do_sys_open) from [<802c7060>] (SyS_open+0x2c/0x30)
[ 139.290647] [<802c7060>] (SyS_open) from [<801082c0>] (ret_fast_syscall+0x0/0x28)
[ 139.306021] Code: e3c3303f e5932004 e2822001 e5832004 (e5943000)
[ 139.316265] ---[ end trace 7f3f7f6193b663ed ]---
[ 139.324956] note: aplay[463] exited with preempt_count 1
~~~~

Signed-off-by: Kirill Marinushkin <[email protected]>
Cc: Eric Anholt <[email protected]>
Cc: Stefan Wahren <[email protected]>
Cc: Greg Kroah-Hartman <[email protected]>
Cc: Florian Fainelli <[email protected]>
Cc: Ray Jui <[email protected]>
Cc: Scott Branden <[email protected]>
Cc: [email protected]
Cc: Michael Zoran <[email protected]>
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
---
.../staging/vc04_services/bcm2835-audio/bcm2835.c | 58 ++++++++++++----------
1 file changed, 31 insertions(+), 27 deletions(-)

diff --git a/drivers/staging/vc04_services/bcm2835-audio/bcm2835.c b/drivers/staging/vc04_services/bcm2835-audio/bcm2835.c
index 8f2d508183b2..477cf4798a6e 100644
--- a/drivers/staging/vc04_services/bcm2835-audio/bcm2835.c
+++ b/drivers/staging/vc04_services/bcm2835-audio/bcm2835.c
@@ -36,6 +36,11 @@ MODULE_PARM_DESC(enable_compat_alsa,
static void snd_devm_unregister_child(struct device *dev, void *res)
{
struct device *childdev = *(struct device **)res;
+ struct bcm2835_chip *chip = dev_get_drvdata(childdev);
+ struct snd_card *card = chip->card;
+
+ snd_card_free(card);
+ dev_set_drvdata(childdev, NULL);

device_unregister(childdev);
}
@@ -61,6 +66,13 @@ static int snd_devm_add_child(struct device *dev, struct device *child)
return 0;
}

+static void snd_devm_release(struct device *dev)
+{
+ struct bcm2835_chip *chip = dev_get_drvdata(dev);
+
+ kfree(chip);
+}
+
static struct device *
snd_create_device(struct device *parent,
struct device_driver *driver,
@@ -76,6 +88,7 @@ snd_create_device(struct device *parent,
device_initialize(device);
device->parent = parent;
device->driver = driver;
+ device->release = snd_devm_release;

dev_set_name(device, "%s", name);

@@ -86,18 +99,19 @@ snd_create_device(struct device *parent,
return device;
}

-static int snd_bcm2835_free(struct bcm2835_chip *chip)
-{
- kfree(chip);
- return 0;
-}
-
/* component-destructor
* (see "Management of Cards and Components")
*/
static int snd_bcm2835_dev_free(struct snd_device *device)
{
- return snd_bcm2835_free(device->device_data);
+ struct bcm2835_chip *chip = device->device_data;
+ struct snd_card *card = chip->card;
+
+ /* TODO: free pcm, ctl */
+
+ snd_device_free(card, chip);
+
+ return 0;
}

/* chip-specific constructor
@@ -122,7 +136,7 @@ static int snd_bcm2835_create(struct snd_card *card,

err = snd_device_new(card, SNDRV_DEV_LOWLEVEL, chip, &ops);
if (err) {
- snd_bcm2835_free(chip);
+ kfree(chip);
return err;
}

@@ -130,31 +144,14 @@ static int snd_bcm2835_create(struct snd_card *card,
return 0;
}

-static void snd_devm_card_free(struct device *dev, void *res)
-{
- struct snd_card *snd_card = *(struct snd_card **)res;
-
- snd_card_free(snd_card);
-}
-
static struct snd_card *snd_devm_card_new(struct device *dev)
{
- struct snd_card **dr;
struct snd_card *card;
int ret;

- dr = devres_alloc(snd_devm_card_free, sizeof(*dr), GFP_KERNEL);
- if (!dr)
- return ERR_PTR(-ENOMEM);
-
ret = snd_card_new(dev, -1, NULL, THIS_MODULE, 0, &card);
- if (ret) {
- devres_free(dr);
+ if (ret)
return ERR_PTR(ret);
- }
-
- *dr = card;
- devres_add(dev, dr);

return card;
}
@@ -313,7 +310,7 @@ static int snd_add_child_device(struct device *device,
return err;
}

- dev_set_drvdata(child, card);
+ dev_set_drvdata(child, chip);
dev_info(child, "card created with %d channels\n", numchans);

return 0;
@@ -414,6 +411,12 @@ static int snd_bcm2835_alsa_probe_dt(struct platform_device *pdev)
return 0;
}

+static int snd_bcm2835_alsa_remove(struct platform_device *pdev)
+{
+ /* trigger snd_devm_unregister_child() */
+ return 0;
+}
+
#ifdef CONFIG_PM

static int snd_bcm2835_alsa_suspend(struct platform_device *pdev,
@@ -437,6 +440,7 @@ MODULE_DEVICE_TABLE(of, snd_bcm2835_of_match_table);

static struct platform_driver bcm2835_alsa0_driver = {
.probe = snd_bcm2835_alsa_probe_dt,
+ .remove = snd_bcm2835_alsa_remove,
#ifdef CONFIG_PM
.suspend = snd_bcm2835_alsa_suspend,
.resume = snd_bcm2835_alsa_resume,
--
2.13.6

2018-03-13 21:25:14

by Andy Shevchenko

Subject: Re: [PATCH v2] staging: bcm2835-audio: Release resources on module_exit()

On Tue, Mar 13, 2018 at 9:34 PM, Kirill Marinushkin
<[email protected]> wrote:
> In the current implementation, `rmmod snd_bcm2835` does not release
> resources properly. It causes an oops when trying to list sound devices.
>
> This commit fixes it.

Nice catch!

See my comments below.

> static void snd_devm_unregister_child(struct device *dev, void *res)
> {
> struct device *childdev = *(struct device **)res;
> + struct bcm2835_chip *chip = dev_get_drvdata(childdev);
> + struct snd_card *card = chip->card;
> +
> + snd_card_free(card);

> + dev_set_drvdata(childdev, NULL);

AFAIU this is done by device core.

> device_unregister(childdev);
> }

> +static void snd_devm_release(struct device *dev)
> +{
> + struct bcm2835_chip *chip = dev_get_drvdata(dev);
> +
> + kfree(chip);
> +}


> /* chip-specific constructor
> @@ -122,7 +136,7 @@ static int snd_bcm2835_create(struct snd_card *card,
>
> err = snd_device_new(card, SNDRV_DEV_LOWLEVEL, chip, &ops);
> if (err) {
> - snd_bcm2835_free(chip);
> + kfree(chip);

Do you call device_register() inside snd_device_new()?
In this case you might need put_device() here instead of simple kfree().

> return err;
> }

--
With Best Regards,
Andy Shevchenko

2018-03-15 05:58:05

by Kirill Marinushkin

Subject: Re: [PATCH v2] staging: bcm2835-audio: Release resources on module_exit()

On 03/13/18 22:23, Andy Shevchenko wrote:
> On Tue, Mar 13, 2018 at 9:34 PM, Kirill Marinushkin
> <[email protected]> wrote:
>> In the current implementation, `rmmod snd_bcm2835` does not release
>> resources properly. It causes an oops when trying to list sound devices.
>>
>> This commit fixes it.
> Nice catch!
>
> See my comments below.
>
>> static void snd_devm_unregister_child(struct device *dev, void *res)
>> {
>> struct device *childdev = *(struct device **)res;
>> + struct bcm2835_chip *chip = dev_get_drvdata(childdev);
>> + struct snd_card *card = chip->card;
>> +
>> + snd_card_free(card);
>> + dev_set_drvdata(childdev, NULL);
> AFAIU this is done by device core.

Maybe you are right. But I don't know, which function in the device core does it.
It is safe to have this line. So, I suggest to keep it.

>
>> device_unregister(childdev);
>> }
>> +static void snd_devm_release(struct device *dev)
>> +{
>> + struct bcm2835_chip *chip = dev_get_drvdata(dev);
>> +
>> + kfree(chip);
>> +}
>
>> /* chip-specific constructor
>> @@ -122,7 +136,7 @@ static int snd_bcm2835_create(struct snd_card *card,
>>
>> err = snd_device_new(card, SNDRV_DEV_LOWLEVEL, chip, &ops);
>> if (err) {
>> - snd_bcm2835_free(chip);
>> + kfree(chip);
> Do you call device_register() inside snd_device_new()?
> In this case you might need put_device() here instead of simple kfree().

No, from what I see, device_register() does not happen inside snd_device_new().

>> return err;
>> }

2018-03-21 15:05:43

by Andy Shevchenko

Subject: Re: [PATCH v2] staging: bcm2835-audio: Release resources on module_exit()

On Thu, Mar 15, 2018 at 7:57 AM, Kirill Marinushkin
<[email protected]> wrote:
> On 03/13/18 22:23, Andy Shevchenko wrote:
>> On Tue, Mar 13, 2018 at 9:34 PM, Kirill Marinushkin
>> <[email protected]> wrote:
>>> In the current implementation, `rmmod snd_bcm2835` does not release
>>> resources properly. It causes an oops when trying to list sound devices.
>>>
>>> This commit fixes it.
>> Nice catch!
>>
>> See my comments below.
>>
>>> static void snd_devm_unregister_child(struct device *dev, void *res)
>>> {
>>> struct device *childdev = *(struct device **)res;
>>> + struct bcm2835_chip *chip = dev_get_drvdata(childdev);
>>> + struct snd_card *card = chip->card;
>>> +
>>> + snd_card_free(card);
>>> + dev_set_drvdata(childdev, NULL);
>> AFAIU this is done by device core.
>
> Maybe you are right. But I don't know, which function in the device core does it.
> It is safe to have this line. So, I suggest to keep it.

Please, remove.
If you don't know, perhaps you need to spend more time on doing homework?

% git grep -n -w dev_set_drvdata -- drivers/base/dd.c
drivers/base/dd.c:469: dev_set_drvdata(dev, NULL);
drivers/base/dd.c:499: dev_set_drvdata(dev, NULL);
drivers/base/dd.c:902: dev_set_drvdata(dev, NULL);

Last one is the point of your interest.

--
With Best Regards,
Andy Shevchenko

2018-03-21 16:58:49

by Kirill Marinushkin

Subject: Re: [PATCH v2] staging: bcm2835-audio: Release resources on module_exit()

On 03/21/18 16:03, Andy Shevchenko wrote:
> On Thu, Mar 15, 2018 at 7:57 AM, Kirill Marinushkin
> <[email protected]> wrote:
>> On 03/13/18 22:23, Andy Shevchenko wrote:
>>> On Tue, Mar 13, 2018 at 9:34 PM, Kirill Marinushkin
>>> <[email protected]> wrote:
>>>> In the current implementation, `rmmod snd_bcm2835` does not release
>>>> resources properly. It causes an oops when trying to list sound devices.
>>>>
>>>> This commit fixes it.
>>> Nice catch!
>>>
>>> See my comments below.
>>>
>>>> static void snd_devm_unregister_child(struct device *dev, void *res)
>>>> {
>>>> struct device *childdev = *(struct device **)res;
>>>> + struct bcm2835_chip *chip = dev_get_drvdata(childdev);
>>>> + struct snd_card *card = chip->card;
>>>> +
>>>> + snd_card_free(card);
>>>> + dev_set_drvdata(childdev, NULL);
>>> AFAIU this is done by device core.
>> Maybe you are right. But I don't know, which function in the device core does it.
>> It is safe to have this line. So, I suggest to keep it.
> Please, remove.
> If you don't know, perhaps you need to spend more time on doing homework?

You are right, I actually should pay more attention to this detail.
I will remove this line and send as a patch v3

> % git grep -n -w dev_set_drvdata -- drivers/base/dd.c
> drivers/base/dd.c:469: dev_set_drvdata(dev, NULL);
> drivers/base/dd.c:499: dev_set_drvdata(dev, NULL);
> drivers/base/dd.c:902: dev_set_drvdata(dev, NULL);
>
> Last one is the point of your interest.
>

2018-03-21 18:50:15

by Kirill Marinushkin

Subject: [PATCH v3] staging: bcm2835-audio: Release resources on module_exit()

In the current implementation, `rmmod snd_bcm2835` does not release
resources properly. It causes an oops when trying to list sound devices.

This commit fixes it.

The details WRT allocation / free are described below.

Device structure WRT allocation:

pdev
\childdev[]
\card
\chip
\pcm
\ctl

Allocation / register sequence:

* childdev: devm_kzalloc - freed during driver detach
* childdev: device_initialize - freed during device_unregister
* pdev: devres_alloc - freed during driver detach
* childdev: device_add - removed during device_unregister
* pdev, childdev: devres_add - freed during driver detach
* card: snd_card_new - freed during snd_card_free
* chip: kzalloc - freed during kfree
* card, chip: snd_device_new - freed during snd_device_free
* chip: new_pcm - TODO: free pcm
* chip: new_ctl - TODO: free ctl
* card: snd_card_register - unregistered during snd_card_free

Free / unregister sequence:

* card: snd_card_free
* card, chip: snd_device_free
* childdev: device_unregister
* chip: kfree

Steps to reproduce the issue before this commit:

~~~~
$ rmmod snd_bcm2835
$ aplay -L
[ 138.648130] Unable to handle kernel paging request at virtual address 7f1343c0
[ 138.660415] pgd = ad8f0000
[ 138.665567] [7f1343c0] *pgd=3864c811, *pte=00000000, *ppte=00000000
[ 138.674887] Internal error: Oops: 7 [#1] SMP ARM
[ 138.683571] Modules linked in: sha256_generic cfg80211 rfkill snd_pcm snd_timer
snd fixed uio_pdrv_genirq uio ip_tables x_tables ipv6 [last unloaded: snd_bcm2835
]
[ 138.706594] CPU: 3 PID: 463 Comm: aplay Tainted: G WC 4.15.0-rc1-v
7+ #6
[ 138.719833] Hardware name: BCM2835
[ 138.726016] task: b877ac00 task.stack: aebec000
[ 138.733408] PC is at try_module_get+0x38/0x24c
[ 138.740813] LR is at snd_ctl_open+0x58/0x194 [snd]
[ 138.748485] pc : [<801c4d5c>] lr : [<7f0e6b2c>] psr: 20000013
[ 138.757709] sp : aebedd60 ip : aebedd88 fp : aebedd84
[ 138.765884] r10: 00000000 r9 : 00000004 r8 : 7f0ed440
[ 138.774040] r7 : b7e469b0 r6 : 7f0e6b2c r5 : afd91900 r4 : 7f1343c0
[ 138.783571] r3 : aebec000 r2 : 00000001 r1 : b877ac00 r0 : 7f1343c0
[ 138.793084] Flags: nzCv IRQs on FIQs on Mode SVC_32 ISA ARM Segment user
[ 138.803300] Control: 10c5387d Table: 2d8f006a DAC: 00000055
[ 138.812064] Process aplay (pid: 463, stack limit = 0xaebec210)
[ 138.820868] Stack: (0xaebedd60 to 0xaebee000)
[ 138.828207] dd60: 00000000 b848d000 afd91900 00000000 b7e469b0 7f0ed440 aebedda4 aebedd88
[ 138.842371] dd80: 7f0e6b2c 801c4d30 afd91900 7f0ea4dc 00000000 b7e469b0 aebeddcc aebedda8
[ 138.856611] dda0: 7f0e250c 7f0e6ae0 7f0e2464 b8478ec0 b7e469b0 afd91900 7f0ea388 00000000
[ 138.870864] ddc0: aebeddf4 aebeddd0 802ce590 7f0e2470 8090ab64 afd91900 afd91900 b7e469b0
[ 138.885301] dde0: afd91908 802ce4e4 aebede1c aebeddf8 802c57b4 802ce4f0 afd91900 aebedea8
[ 138.900110] de00: b7fa4c00 00000000 00000000 00000004 aebede3c aebede20 802c6ba8 802c56b4
[ 138.915260] de20: aebedea8 00000000 aebedf5c 00000000 aebedea4 aebede40 802d9a68 802c6b58
[ 138.930661] de40: b874ddd0 00000000 00000000 00000001 00000041 00000000 afd91900 aebede70
[ 138.946402] de60: 00000000 00000000 00000002 b7e469b0 b8a87610 b8d6ab80 801852f8 00080000
[ 138.962314] de80: aebedf5c aebedea8 00000001 80108464 aebec000 00000000 aebedf4c aebedea8
[ 138.978414] dea0: 802dacd4 802d970c b8a87610 b8d6ab80 a7982bc6 00000009 af363019 b9231480
[ 138.994617] dec0: 00000000 b8c038a0 b7e469b0 00000101 00000002 00000238 00000000 00000000
[ 139.010823] dee0: 00000000 aebedee8 00080000 0000000f aebedf3c aebedf00 802ed7e4 80843f94
[ 139.027025] df00: 00000003 00080000 b9231490 b9231480 00000000 00080000 af363000 00000000
[ 139.043229] df20: 00000005 00000002 ffffff9c 00000000 00080000 ffffff9c af363000 00000003
[ 139.059430] df40: aebedf94 aebedf50 802c6f70 802dac70 aebec000 00000000 00000001 00000000
[ 139.075629] df60: 00020000 00000004 00000100 00000001 7ebe577c 0002e038 00000000 00000005
[ 139.091828] df80: 80108464 aebec000 aebedfa4 aebedf98 802c7060 802c6e6c 00000000 aebedfa8
[ 139.108025] dfa0: 801082c0 802c7040 7ebe577c 0002e038 7ebe577c 00080000 00000b98 e81c8400
[ 139.124222] dfc0: 7ebe577c 0002e038 00000000 00000005 7ebe57e4 00a20af8 7ebe57f0 76f87394
[ 139.140419] dfe0: 00000000 7ebe55c4 76ec88e8 76df1d9c 60000010 7ebe577c 00000000 00000000
[ 139.156715] [<801c4d5c>] (try_module_get) from [<7f0e6b2c>] (snd_ctl_open+0x58/0x194 [snd])
[ 139.173222] [<7f0e6b2c>] (snd_ctl_open [snd]) from [<7f0e250c>] (snd_open+0xa8/0x14c [snd])
[ 139.189683] [<7f0e250c>] (snd_open [snd]) from [<802ce590>] (chrdev_open+0xac/0x188)
[ 139.205465] [<802ce590>] (chrdev_open) from [<802c57b4>] (do_dentry_open+0x10c/0x314)
[ 139.221347] [<802c57b4>] (do_dentry_open) from [<802c6ba8>] (vfs_open+0x5c/0x88)
[ 139.236788] [<802c6ba8>] (vfs_open) from [<802d9a68>] (path_openat+0x368/0x944)
[ 139.248270] [<802d9a68>] (path_openat) from [<802dacd4>] (do_filp_open+0x70/0xc4)
[ 139.263731] [<802dacd4>] (do_filp_open) from [<802c6f70>] (do_sys_open+0x110/0x1d4)
[ 139.279378] [<802c6f70>] (do_sys_open) from [<802c7060>] (SyS_open+0x2c/0x30)
[ 139.290647] [<802c7060>] (SyS_open) from [<801082c0>] (ret_fast_syscall+0x0/0x28)
[ 139.306021] Code: e3c3303f e5932004 e2822001 e5832004 (e5943000)
[ 139.316265] ---[ end trace 7f3f7f6193b663ed ]---
[ 139.324956] note: aplay[463] exited with preempt_count 1
~~~~

Signed-off-by: Kirill Marinushkin <[email protected]>
Cc: Eric Anholt <[email protected]>
Cc: Stefan Wahren <[email protected]>
Cc: Greg Kroah-Hartman <[email protected]>
Cc: Florian Fainelli <[email protected]>
Cc: Ray Jui <[email protected]>
Cc: Scott Branden <[email protected]>
Cc: [email protected]
Cc: Michael Zoran <[email protected]>
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
---
.../staging/vc04_services/bcm2835-audio/bcm2835.c | 57 ++++++++++++----------
1 file changed, 30 insertions(+), 27 deletions(-)

diff --git a/drivers/staging/vc04_services/bcm2835-audio/bcm2835.c b/drivers/staging/vc04_services/bcm2835-audio/bcm2835.c
index 8f2d508183b2..125efc55ecb9 100644
--- a/drivers/staging/vc04_services/bcm2835-audio/bcm2835.c
+++ b/drivers/staging/vc04_services/bcm2835-audio/bcm2835.c
@@ -36,6 +36,10 @@ MODULE_PARM_DESC(enable_compat_alsa,
static void snd_devm_unregister_child(struct device *dev, void *res)
{
struct device *childdev = *(struct device **)res;
+ struct bcm2835_chip *chip = dev_get_drvdata(childdev);
+ struct snd_card *card = chip->card;
+
+ snd_card_free(card);

device_unregister(childdev);
}
@@ -61,6 +65,13 @@ static int snd_devm_add_child(struct device *dev, struct device *child)
return 0;
}

+static void snd_devm_release(struct device *dev)
+{
+ struct bcm2835_chip *chip = dev_get_drvdata(dev);
+
+ kfree(chip);
+}
+
static struct device *
snd_create_device(struct device *parent,
struct device_driver *driver,
@@ -76,6 +87,7 @@ snd_create_device(struct device *parent,
device_initialize(device);
device->parent = parent;
device->driver = driver;
+ device->release = snd_devm_release;

dev_set_name(device, "%s", name);

@@ -86,18 +98,19 @@ snd_create_device(struct device *parent,
return device;
}

-static int snd_bcm2835_free(struct bcm2835_chip *chip)
-{
- kfree(chip);
- return 0;
-}
-
/* component-destructor
* (see "Management of Cards and Components")
*/
static int snd_bcm2835_dev_free(struct snd_device *device)
{
- return snd_bcm2835_free(device->device_data);
+ struct bcm2835_chip *chip = device->device_data;
+ struct snd_card *card = chip->card;
+
+ /* TODO: free pcm, ctl */
+
+ snd_device_free(card, chip);
+
+ return 0;
}

/* chip-specific constructor
@@ -122,7 +135,7 @@ static int snd_bcm2835_create(struct snd_card *card,

err = snd_device_new(card, SNDRV_DEV_LOWLEVEL, chip, &ops);
if (err) {
- snd_bcm2835_free(chip);
+ kfree(chip);
return err;
}

@@ -130,31 +143,14 @@ static int snd_bcm2835_create(struct snd_card *card,
return 0;
}

-static void snd_devm_card_free(struct device *dev, void *res)
-{
- struct snd_card *snd_card = *(struct snd_card **)res;
-
- snd_card_free(snd_card);
-}
-
static struct snd_card *snd_devm_card_new(struct device *dev)
{
- struct snd_card **dr;
struct snd_card *card;
int ret;

- dr = devres_alloc(snd_devm_card_free, sizeof(*dr), GFP_KERNEL);
- if (!dr)
- return ERR_PTR(-ENOMEM);
-
ret = snd_card_new(dev, -1, NULL, THIS_MODULE, 0, &card);
- if (ret) {
- devres_free(dr);
+ if (ret)
return ERR_PTR(ret);
- }
-
- *dr = card;
- devres_add(dev, dr);

return card;
}
@@ -313,7 +309,7 @@ static int snd_add_child_device(struct device *device,
return err;
}

- dev_set_drvdata(child, card);
+ dev_set_drvdata(child, chip);
dev_info(child, "card created with %d channels\n", numchans);

return 0;
@@ -414,6 +410,12 @@ static int snd_bcm2835_alsa_probe_dt(struct platform_device *pdev)
return 0;
}

+static int snd_bcm2835_alsa_remove(struct platform_device *pdev)
+{
+ /* trigger snd_devm_unregister_child() */
+ return 0;
+}
+
#ifdef CONFIG_PM

static int snd_bcm2835_alsa_suspend(struct platform_device *pdev,
@@ -437,6 +439,7 @@ MODULE_DEVICE_TABLE(of, snd_bcm2835_of_match_table);

static struct platform_driver bcm2835_alsa0_driver = {
.probe = snd_bcm2835_alsa_probe_dt,
+ .remove = snd_bcm2835_alsa_remove,
#ifdef CONFIG_PM
.suspend = snd_bcm2835_alsa_suspend,
.resume = snd_bcm2835_alsa_resume,
--
2.13.6

2018-03-21 19:27:50

by Andy Shevchenko

Subject: Re: [PATCH v3] staging: bcm2835-audio: Release resources on module_exit()

On Wed, Mar 21, 2018 at 8:48 PM, Kirill Marinushkin
<[email protected]> wrote:
> In the current implementation, `rmmod snd_bcm2835` does not release
> resources properly. It causes an oops when trying to list sound devices.
>
> This commit fixes it.
>
> The details WRT allocation / free are described below.
>
> Device structure WRT allocation:
>
> pdev
> \childdev[]
> \card
> \chip
> \pcm
> \ctl
>
> Allocation / register sequence:
>
> * childdev: devm_kzalloc - freed during driver detach
> * childdev: device_initialize - freed during device_unregister
> * pdev: devres_alloc - freed during driver detach
> * childdev: device_add - removed during device_unregister
> * pdev, childdev: devres_add - freed during driver detach
> * card: snd_card_new - freed during snd_card_free
> * chip: kzalloc - freed during kfree
> * card, chip: snd_device_new - freed during snd_device_free
> * chip: new_pcm - TODO: free pcm
> * chip: new_ctl - TODO: free ctl
> * card: snd_card_register - unregistered during snd_card_free
>
> Free / unregister sequence:
>
> * card: snd_card_free
> * card, chip: snd_device_free
> * childdev: device_unregister
> * chip: kfree
>
> Steps to reproduce the issue before this commit:
>
> ~~~~
> $ rmmod snd_bcm2835
> $ aplay -L
> [ 138.648130] Unable to handle kernel paging request at virtual address 7f1343c0
> [ 138.660415] pgd = ad8f0000
> [ 138.665567] [7f1343c0] *pgd=3864c811, *pte=00000000, *ppte=00000000
> [ 138.674887] Internal error: Oops: 7 [#1] SMP ARM
> [ 138.683571] Modules linked in: sha256_generic cfg80211 rfkill snd_pcm snd_timer
> snd fixed uio_pdrv_genirq uio ip_tables x_tables ipv6 [last unloaded: snd_bcm2835
> ]
> [ 138.706594] CPU: 3 PID: 463 Comm: aplay Tainted: G WC 4.15.0-rc1-v
> 7+ #6
> [ 138.719833] Hardware name: BCM2835
> [ 138.726016] task: b877ac00 task.stack: aebec000
> [ 138.733408] PC is at try_module_get+0x38/0x24c
> [ 138.740813] LR is at snd_ctl_open+0x58/0x194 [snd]
> [ 138.748485] pc : [<801c4d5c>] lr : [<7f0e6b2c>] psr: 20000013
> [ 138.757709] sp : aebedd60 ip : aebedd88 fp : aebedd84
> [ 138.765884] r10: 00000000 r9 : 00000004 r8 : 7f0ed440
> [ 138.774040] r7 : b7e469b0 r6 : 7f0e6b2c r5 : afd91900 r4 : 7f1343c0
> [ 138.783571] r3 : aebec000 r2 : 00000001 r1 : b877ac00 r0 : 7f1343c0
> [ 138.793084] Flags: nzCv IRQs on FIQs on Mode SVC_32 ISA ARM Segment user
> [ 138.803300] Control: 10c5387d Table: 2d8f006a DAC: 00000055
> [ 138.812064] Process aplay (pid: 463, stack limit = 0xaebec210)
> [ 138.820868] Stack: (0xaebedd60 to 0xaebee000)
> [ 138.828207] dd60: 00000000 b848d000 afd91900 00000000 b7e469b0 7f0ed440 aebedda4 aebedd88
> [ 138.842371] dd80: 7f0e6b2c 801c4d30 afd91900 7f0ea4dc 00000000 b7e469b0 aebeddcc aebedda8
> [ 138.856611] dda0: 7f0e250c 7f0e6ae0 7f0e2464 b8478ec0 b7e469b0 afd91900 7f0ea388 00000000
> [ 138.870864] ddc0: aebeddf4 aebeddd0 802ce590 7f0e2470 8090ab64 afd91900 afd91900 b7e469b0
> [ 138.885301] dde0: afd91908 802ce4e4 aebede1c aebeddf8 802c57b4 802ce4f0 afd91900 aebedea8
> [ 138.900110] de00: b7fa4c00 00000000 00000000 00000004 aebede3c aebede20 802c6ba8 802c56b4
> [ 138.915260] de20: aebedea8 00000000 aebedf5c 00000000 aebedea4 aebede40 802d9a68 802c6b58
> [ 138.930661] de40: b874ddd0 00000000 00000000 00000001 00000041 00000000 afd91900 aebede70
> [ 138.946402] de60: 00000000 00000000 00000002 b7e469b0 b8a87610 b8d6ab80 801852f8 00080000
> [ 138.962314] de80: aebedf5c aebedea8 00000001 80108464 aebec000 00000000 aebedf4c aebedea8
> [ 138.978414] dea0: 802dacd4 802d970c b8a87610 b8d6ab80 a7982bc6 00000009 af363019 b9231480
> [ 138.994617] dec0: 00000000 b8c038a0 b7e469b0 00000101 00000002 00000238 00000000 00000000
> [ 139.010823] dee0: 00000000 aebedee8 00080000 0000000f aebedf3c aebedf00 802ed7e4 80843f94
> [ 139.027025] df00: 00000003 00080000 b9231490 b9231480 00000000 00080000 af363000 00000000
> [ 139.043229] df20: 00000005 00000002 ffffff9c 00000000 00080000 ffffff9c af363000 00000003
> [ 139.059430] df40: aebedf94 aebedf50 802c6f70 802dac70 aebec000 00000000 00000001 00000000
> [ 139.075629] df60: 00020000 00000004 00000100 00000001 7ebe577c 0002e038 00000000 00000005
> [ 139.091828] df80: 80108464 aebec000 aebedfa4 aebedf98 802c7060 802c6e6c 00000000 aebedfa8
> [ 139.108025] dfa0: 801082c0 802c7040 7ebe577c 0002e038 7ebe577c 00080000 00000b98 e81c8400
> [ 139.124222] dfc0: 7ebe577c 0002e038 00000000 00000005 7ebe57e4 00a20af8 7ebe57f0 76f87394
> [ 139.140419] dfe0: 00000000 7ebe55c4 76ec88e8 76df1d9c 60000010 7ebe577c 00000000 00000000
> [ 139.156715] [<801c4d5c>] (try_module_get) from [<7f0e6b2c>] (snd_ctl_open+0x58/0x194 [snd])
> [ 139.173222] [<7f0e6b2c>] (snd_ctl_open [snd]) from [<7f0e250c>] (snd_open+0xa8/0x14c [snd])
> [ 139.189683] [<7f0e250c>] (snd_open [snd]) from [<802ce590>] (chrdev_open+0xac/0x188)
> [ 139.205465] [<802ce590>] (chrdev_open) from [<802c57b4>] (do_dentry_open+0x10c/0x314)
> [ 139.221347] [<802c57b4>] (do_dentry_open) from [<802c6ba8>] (vfs_open+0x5c/0x88)
> [ 139.236788] [<802c6ba8>] (vfs_open) from [<802d9a68>] (path_openat+0x368/0x944)
> [ 139.248270] [<802d9a68>] (path_openat) from [<802dacd4>] (do_filp_open+0x70/0xc4)
> [ 139.263731] [<802dacd4>] (do_filp_open) from [<802c6f70>] (do_sys_open+0x110/0x1d4)
> [ 139.279378] [<802c6f70>] (do_sys_open) from [<802c7060>] (SyS_open+0x2c/0x30)
> [ 139.290647] [<802c7060>] (SyS_open) from [<801082c0>] (ret_fast_syscall+0x0/0x28)
> [ 139.306021] Code: e3c3303f e5932004 e2822001 e5832004 (e5943000)
> [ 139.316265] ---[ end trace 7f3f7f6193b663ed ]---
> [ 139.324956] note: aplay[463] exited with preempt_count 1
> ~~~~
>

Sorry, just noticed that devm part doesn't make sense after your patch
and misleading.
So, after addressing it, FWIW,
Reviewed-by: Andy Shevchenko <[email protected]>

P.S. I didn't get why do you need an empty ->remove() stub. Doesn't
work without it?

> Signed-off-by: Kirill Marinushkin <[email protected]>
> Cc: Eric Anholt <[email protected]>
> Cc: Stefan Wahren <[email protected]>
> Cc: Greg Kroah-Hartman <[email protected]>
> Cc: Florian Fainelli <[email protected]>
> Cc: Ray Jui <[email protected]>
> Cc: Scott Branden <[email protected]>
> Cc: [email protected]
> Cc: Michael Zoran <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> Cc: [email protected]
> Cc: [email protected]
> ---
> .../staging/vc04_services/bcm2835-audio/bcm2835.c | 57 ++++++++++++----------
> 1 file changed, 30 insertions(+), 27 deletions(-)
>
> diff --git a/drivers/staging/vc04_services/bcm2835-audio/bcm2835.c b/drivers/staging/vc04_services/bcm2835-audio/bcm2835.c
> index 8f2d508183b2..125efc55ecb9 100644
> --- a/drivers/staging/vc04_services/bcm2835-audio/bcm2835.c
> +++ b/drivers/staging/vc04_services/bcm2835-audio/bcm2835.c
> @@ -36,6 +36,10 @@ MODULE_PARM_DESC(enable_compat_alsa,
> static void snd_devm_unregister_child(struct device *dev, void *res)
> {
> struct device *childdev = *(struct device **)res;
> + struct bcm2835_chip *chip = dev_get_drvdata(childdev);
> + struct snd_card *card = chip->card;
> +
> + snd_card_free(card);
>
> device_unregister(childdev);
> }
> @@ -61,6 +65,13 @@ static int snd_devm_add_child(struct device *dev, struct device *child)
> return 0;
> }
>
> +static void snd_devm_release(struct device *dev)
> +{
> + struct bcm2835_chip *chip = dev_get_drvdata(dev);
> +
> + kfree(chip);
> +}
> +
> static struct device *
> snd_create_device(struct device *parent,
> struct device_driver *driver,
> @@ -76,6 +87,7 @@ snd_create_device(struct device *parent,
> device_initialize(device);
> device->parent = parent;
> device->driver = driver;
> + device->release = snd_devm_release;
>
> dev_set_name(device, "%s", name);
>
> @@ -86,18 +98,19 @@ snd_create_device(struct device *parent,
> return device;
> }
>
> -static int snd_bcm2835_free(struct bcm2835_chip *chip)
> -{
> - kfree(chip);
> - return 0;
> -}
> -
> /* component-destructor
> * (see "Management of Cards and Components")
> */
> static int snd_bcm2835_dev_free(struct snd_device *device)
> {
> - return snd_bcm2835_free(device->device_data);
> + struct bcm2835_chip *chip = device->device_data;
> + struct snd_card *card = chip->card;
> +
> + /* TODO: free pcm, ctl */
> +
> + snd_device_free(card, chip);
> +
> + return 0;
> }
>
> /* chip-specific constructor
> @@ -122,7 +135,7 @@ static int snd_bcm2835_create(struct snd_card *card,
>
> err = snd_device_new(card, SNDRV_DEV_LOWLEVEL, chip, &ops);
> if (err) {
> - snd_bcm2835_free(chip);
> + kfree(chip);
> return err;
> }
>
> @@ -130,31 +143,14 @@ static int snd_bcm2835_create(struct snd_card *card,
> return 0;
> }
>
> -static void snd_devm_card_free(struct device *dev, void *res)
> -{
> - struct snd_card *snd_card = *(struct snd_card **)res;
> -
> - snd_card_free(snd_card);
> -}
> -
> static struct snd_card *snd_devm_card_new(struct device *dev)
> {
> - struct snd_card **dr;
> struct snd_card *card;
> int ret;
>
> - dr = devres_alloc(snd_devm_card_free, sizeof(*dr), GFP_KERNEL);
> - if (!dr)
> - return ERR_PTR(-ENOMEM);
> -
> ret = snd_card_new(dev, -1, NULL, THIS_MODULE, 0, &card);
> - if (ret) {
> - devres_free(dr);
> + if (ret)
> return ERR_PTR(ret);
> - }
> -
> - *dr = card;
> - devres_add(dev, dr);
>
> return card;
> }
> @@ -313,7 +309,7 @@ static int snd_add_child_device(struct device *device,
> return err;
> }
>
> - dev_set_drvdata(child, card);
> + dev_set_drvdata(child, chip);
> dev_info(child, "card created with %d channels\n", numchans);
>
> return 0;
> @@ -414,6 +410,12 @@ static int snd_bcm2835_alsa_probe_dt(struct platform_device *pdev)
> return 0;
> }
>
> +static int snd_bcm2835_alsa_remove(struct platform_device *pdev)
> +{
> + /* trigger snd_devm_unregister_child() */
> + return 0;
> +}
> +
> #ifdef CONFIG_PM
>
> static int snd_bcm2835_alsa_suspend(struct platform_device *pdev,
> @@ -437,6 +439,7 @@ MODULE_DEVICE_TABLE(of, snd_bcm2835_of_match_table);
>
> static struct platform_driver bcm2835_alsa0_driver = {
> .probe = snd_bcm2835_alsa_probe_dt,
> + .remove = snd_bcm2835_alsa_remove,
> #ifdef CONFIG_PM
> .suspend = snd_bcm2835_alsa_suspend,
> .resume = snd_bcm2835_alsa_resume,
> --
> 2.13.6
>



--
With Best Regards,
Andy Shevchenko

2018-03-21 19:52:53

by Kirill Marinushkin

Subject: Re: [PATCH v3] staging: bcm2835-audio: Release resources on module_exit()

On 03/21/18 20:24, Andy Shevchenko wrote:
> On Wed, Mar 21, 2018 at 8:48 PM, Kirill Marinushkin
> <[email protected]> wrote:
>> In the current implementation, `rmmod snd_bcm2835` does not release
>> resources properly. It causes an oops when trying to list sound devices.
>>
>> This commit fixes it.
>>
>> The details WRT allocation / free are described below.
>>
>> Device structure WRT allocation:
>>
>> pdev
>> \childdev[]
>> \card
>> \chip
>> \pcm
>> \ctl
>>
>> Allocation / register sequence:
>>
>> * childdev: devm_kzalloc - freed during driver detach
>> * childdev: device_initialize - freed during device_unregister
>> * pdev: devres_alloc - freed during driver detach
>> * childdev: device_add - removed during device_unregister
>> * pdev, childdev: devres_add - freed during driver detach
>> * card: snd_card_new - freed during snd_card_free
>> * chip: kzalloc - freed during kfree
>> * card, chip: snd_device_new - freed during snd_device_free
>> * chip: new_pcm - TODO: free pcm
>> * chip: new_ctl - TODO: free ctl
>> * card: snd_card_register - unregistered during snd_card_free
>>
>> Free / unregister sequence:
>>
>> * card: snd_card_free
>> * card, chip: snd_device_free
>> * childdev: device_unregister
>> * chip: kfree
>>
>> Steps to reproduce the issue before this commit:
>>
>> ~~~~
>> $ rmmod snd_bcm2835
>> $ aplay -L
>> [ 138.648130] Unable to handle kernel paging request at virtual address 7f1343c0
>> [ 138.660415] pgd = ad8f0000
>> [ 138.665567] [7f1343c0] *pgd=3864c811, *pte=00000000, *ppte=00000000
>> [ 138.674887] Internal error: Oops: 7 [#1] SMP ARM
>> [ 138.683571] Modules linked in: sha256_generic cfg80211 rfkill snd_pcm snd_timer
>> snd fixed uio_pdrv_genirq uio ip_tables x_tables ipv6 [last unloaded: snd_bcm2835
>> ]
>> [ 138.706594] CPU: 3 PID: 463 Comm: aplay Tainted: G WC 4.15.0-rc1-v
>> 7+ #6
>> [ 138.719833] Hardware name: BCM2835
>> [ 138.726016] task: b877ac00 task.stack: aebec000
>> [ 138.733408] PC is at try_module_get+0x38/0x24c
>> [ 138.740813] LR is at snd_ctl_open+0x58/0x194 [snd]
>> [ 138.748485] pc : [<801c4d5c>] lr : [<7f0e6b2c>] psr: 20000013
>> [ 138.757709] sp : aebedd60 ip : aebedd88 fp : aebedd84
>> [ 138.765884] r10: 00000000 r9 : 00000004 r8 : 7f0ed440
>> [ 138.774040] r7 : b7e469b0 r6 : 7f0e6b2c r5 : afd91900 r4 : 7f1343c0
>> [ 138.783571] r3 : aebec000 r2 : 00000001 r1 : b877ac00 r0 : 7f1343c0
>> [ 138.793084] Flags: nzCv IRQs on FIQs on Mode SVC_32 ISA ARM Segment user
>> [ 138.803300] Control: 10c5387d Table: 2d8f006a DAC: 00000055
>> [ 138.812064] Process aplay (pid: 463, stack limit = 0xaebec210)
>> [ 138.820868] Stack: (0xaebedd60 to 0xaebee000)
>> [ 138.828207] dd60: 00000000 b848d000 afd91900 00000000 b7e469b0 7f0ed440 aebedda4 aebedd88
>> [ 138.842371] dd80: 7f0e6b2c 801c4d30 afd91900 7f0ea4dc 00000000 b7e469b0 aebeddcc aebedda8
>> [ 138.856611] dda0: 7f0e250c 7f0e6ae0 7f0e2464 b8478ec0 b7e469b0 afd91900 7f0ea388 00000000
>> [ 138.870864] ddc0: aebeddf4 aebeddd0 802ce590 7f0e2470 8090ab64 afd91900 afd91900 b7e469b0
>> [ 138.885301] dde0: afd91908 802ce4e4 aebede1c aebeddf8 802c57b4 802ce4f0 afd91900 aebedea8
>> [ 138.900110] de00: b7fa4c00 00000000 00000000 00000004 aebede3c aebede20 802c6ba8 802c56b4
>> [ 138.915260] de20: aebedea8 00000000 aebedf5c 00000000 aebedea4 aebede40 802d9a68 802c6b58
>> [ 138.930661] de40: b874ddd0 00000000 00000000 00000001 00000041 00000000 afd91900 aebede70
>> [ 138.946402] de60: 00000000 00000000 00000002 b7e469b0 b8a87610 b8d6ab80 801852f8 00080000
>> [ 138.962314] de80: aebedf5c aebedea8 00000001 80108464 aebec000 00000000 aebedf4c aebedea8
>> [ 138.978414] dea0: 802dacd4 802d970c b8a87610 b8d6ab80 a7982bc6 00000009 af363019 b9231480
>> [ 138.994617] dec0: 00000000 b8c038a0 b7e469b0 00000101 00000002 00000238 00000000 00000000
>> [ 139.010823] dee0: 00000000 aebedee8 00080000 0000000f aebedf3c aebedf00 802ed7e4 80843f94
>> [ 139.027025] df00: 00000003 00080000 b9231490 b9231480 00000000 00080000 af363000 00000000
>> [ 139.043229] df20: 00000005 00000002 ffffff9c 00000000 00080000 ffffff9c af363000 00000003
>> [ 139.059430] df40: aebedf94 aebedf50 802c6f70 802dac70 aebec000 00000000 00000001 00000000
>> [ 139.075629] df60: 00020000 00000004 00000100 00000001 7ebe577c 0002e038 00000000 00000005
>> [ 139.091828] df80: 80108464 aebec000 aebedfa4 aebedf98 802c7060 802c6e6c 00000000 aebedfa8
>> [ 139.108025] dfa0: 801082c0 802c7040 7ebe577c 0002e038 7ebe577c 00080000 00000b98 e81c8400
>> [ 139.124222] dfc0: 7ebe577c 0002e038 00000000 00000005 7ebe57e4 00a20af8 7ebe57f0 76f87394
>> [ 139.140419] dfe0: 00000000 7ebe55c4 76ec88e8 76df1d9c 60000010 7ebe577c 00000000 00000000
>> [ 139.156715] [<801c4d5c>] (try_module_get) from [<7f0e6b2c>] (snd_ctl_open+0x58/0x194 [snd])
>> [ 139.173222] [<7f0e6b2c>] (snd_ctl_open [snd]) from [<7f0e250c>] (snd_open+0xa8/0x14c [snd])
>> [ 139.189683] [<7f0e250c>] (snd_open [snd]) from [<802ce590>] (chrdev_open+0xac/0x188)
>> [ 139.205465] [<802ce590>] (chrdev_open) from [<802c57b4>] (do_dentry_open+0x10c/0x314)
>> [ 139.221347] [<802c57b4>] (do_dentry_open) from [<802c6ba8>] (vfs_open+0x5c/0x88)
>> [ 139.236788] [<802c6ba8>] (vfs_open) from [<802d9a68>] (path_openat+0x368/0x944)
>> [ 139.248270] [<802d9a68>] (path_openat) from [<802dacd4>] (do_filp_open+0x70/0xc4)
>> [ 139.263731] [<802dacd4>] (do_filp_open) from [<802c6f70>] (do_sys_open+0x110/0x1d4)
>> [ 139.279378] [<802c6f70>] (do_sys_open) from [<802c7060>] (SyS_open+0x2c/0x30)
>> [ 139.290647] [<802c7060>] (SyS_open) from [<801082c0>] (ret_fast_syscall+0x0/0x28)
>> [ 139.306021] Code: e3c3303f e5932004 e2822001 e5832004 (e5943000)
>> [ 139.316265] ---[ end trace 7f3f7f6193b663ed ]---
>> [ 139.324956] note: aplay[463] exited with preempt_count 1
>> ~~~~
>>
> Sorry, just noticed that devm part doesn't make sense after your patch
> and misleading.
> So, after addressing it, FWIW,
> Reviewed-by: Andy Shevchenko <[email protected]>

I will check about devm

>
> P.S. I didn't get why do you need an empty ->remove() stub. Doesn't
> work without it?

In the v1 of this patch, it didn't. But now, when the probing is improved since then, I will check this also

>
>> Signed-off-by: Kirill Marinushkin <[email protected]>
>> Cc: Eric Anholt <[email protected]>
>> Cc: Stefan Wahren <[email protected]>
>> Cc: Greg Kroah-Hartman <[email protected]>
>> Cc: Florian Fainelli <[email protected]>
>> Cc: Ray Jui <[email protected]>
>> Cc: Scott Branden <[email protected]>
>> Cc: [email protected]
>> Cc: Michael Zoran <[email protected]>
>> Cc: [email protected]
>> Cc: [email protected]
>> Cc: [email protected]
>> Cc: [email protected]
>> ---
>> .../staging/vc04_services/bcm2835-audio/bcm2835.c | 57 ++++++++++++----------
>> 1 file changed, 30 insertions(+), 27 deletions(-)
>>
>> diff --git a/drivers/staging/vc04_services/bcm2835-audio/bcm2835.c b/drivers/staging/vc04_services/bcm2835-audio/bcm2835.c
>> index 8f2d508183b2..125efc55ecb9 100644
>> --- a/drivers/staging/vc04_services/bcm2835-audio/bcm2835.c
>> +++ b/drivers/staging/vc04_services/bcm2835-audio/bcm2835.c
>> @@ -36,6 +36,10 @@ MODULE_PARM_DESC(enable_compat_alsa,
>> static void snd_devm_unregister_child(struct device *dev, void *res)
>> {
>> struct device *childdev = *(struct device **)res;
>> + struct bcm2835_chip *chip = dev_get_drvdata(childdev);
>> + struct snd_card *card = chip->card;
>> +
>> + snd_card_free(card);
>>
>> device_unregister(childdev);
>> }
>> @@ -61,6 +65,13 @@ static int snd_devm_add_child(struct device *dev, struct device *child)
>> return 0;
>> }
>>
>> +static void snd_devm_release(struct device *dev)
>> +{
>> + struct bcm2835_chip *chip = dev_get_drvdata(dev);
>> +
>> + kfree(chip);
>> +}
>> +
>> static struct device *
>> snd_create_device(struct device *parent,
>> struct device_driver *driver,
>> @@ -76,6 +87,7 @@ snd_create_device(struct device *parent,
>> device_initialize(device);
>> device->parent = parent;
>> device->driver = driver;
>> + device->release = snd_devm_release;
>>
>> dev_set_name(device, "%s", name);
>>
>> @@ -86,18 +98,19 @@ snd_create_device(struct device *parent,
>> return device;
>> }
>>
>> -static int snd_bcm2835_free(struct bcm2835_chip *chip)
>> -{
>> - kfree(chip);
>> - return 0;
>> -}
>> -
>> /* component-destructor
>> * (see "Management of Cards and Components")
>> */
>> static int snd_bcm2835_dev_free(struct snd_device *device)
>> {
>> - return snd_bcm2835_free(device->device_data);
>> + struct bcm2835_chip *chip = device->device_data;
>> + struct snd_card *card = chip->card;
>> +
>> + /* TODO: free pcm, ctl */
>> +
>> + snd_device_free(card, chip);
>> +
>> + return 0;
>> }
>>
>> /* chip-specific constructor
>> @@ -122,7 +135,7 @@ static int snd_bcm2835_create(struct snd_card *card,
>>
>> err = snd_device_new(card, SNDRV_DEV_LOWLEVEL, chip, &ops);
>> if (err) {
>> - snd_bcm2835_free(chip);
>> + kfree(chip);
>> return err;
>> }
>>
>> @@ -130,31 +143,14 @@ static int snd_bcm2835_create(struct snd_card *card,
>> return 0;
>> }
>>
>> -static void snd_devm_card_free(struct device *dev, void *res)
>> -{
>> - struct snd_card *snd_card = *(struct snd_card **)res;
>> -
>> - snd_card_free(snd_card);
>> -}
>> -
>> static struct snd_card *snd_devm_card_new(struct device *dev)
>> {
>> - struct snd_card **dr;
>> struct snd_card *card;
>> int ret;
>>
>> - dr = devres_alloc(snd_devm_card_free, sizeof(*dr), GFP_KERNEL);
>> - if (!dr)
>> - return ERR_PTR(-ENOMEM);
>> -
>> ret = snd_card_new(dev, -1, NULL, THIS_MODULE, 0, &card);
>> - if (ret) {
>> - devres_free(dr);
>> + if (ret)
>> return ERR_PTR(ret);
>> - }
>> -
>> - *dr = card;
>> - devres_add(dev, dr);
>>
>> return card;
>> }
>> @@ -313,7 +309,7 @@ static int snd_add_child_device(struct device *device,
>> return err;
>> }
>>
>> - dev_set_drvdata(child, card);
>> + dev_set_drvdata(child, chip);
>> dev_info(child, "card created with %d channels\n", numchans);
>>
>> return 0;
>> @@ -414,6 +410,12 @@ static int snd_bcm2835_alsa_probe_dt(struct platform_device *pdev)
>> return 0;
>> }
>>
>> +static int snd_bcm2835_alsa_remove(struct platform_device *pdev)
>> +{
>> + /* trigger snd_devm_unregister_child() */
>> + return 0;
>> +}
>> +
>> #ifdef CONFIG_PM
>>
>> static int snd_bcm2835_alsa_suspend(struct platform_device *pdev,
>> @@ -437,6 +439,7 @@ MODULE_DEVICE_TABLE(of, snd_bcm2835_of_match_table);
>>
>> static struct platform_driver bcm2835_alsa0_driver = {
>> .probe = snd_bcm2835_alsa_probe_dt,
>> + .remove = snd_bcm2835_alsa_remove,
>> #ifdef CONFIG_PM
>> .suspend = snd_bcm2835_alsa_suspend,
>> .resume = snd_bcm2835_alsa_resume,
>> --
>> 2.13.6
>>
>
>

2018-03-22 21:21:49

by Kirill Marinushkin

Subject: Re: [PATCH v3] staging: bcm2835-audio: Release resources on module_exit()

On 03/21/18 20:24, Andy Shevchenko wrote:
> On Wed, Mar 21, 2018 at 8:48 PM, Kirill Marinushkin
> <[email protected]> wrote:
>> In the current implementation, `rmmod snd_bcm2835` does not release
>> resources properly. It causes an oops when trying to list sound devices.
>>
>> This commit fixes it.
>>
>> The details WRT allocation / free are described below.
>>
>> Device structure WRT allocation:
>>
>> pdev
>> \childdev[]
>> \card
>> \chip
>> \pcm
>> \ctl
>>
>> Allocation / register sequence:
>>
>> * childdev: devm_kzalloc - freed during driver detach
>> * childdev: device_initialize - freed during device_unregister
>> * pdev: devres_alloc - freed during driver detach
>> * childdev: device_add - removed during device_unregister
>> * pdev, childdev: devres_add - freed during driver detach
>> * card: snd_card_new - freed during snd_card_free
>> * chip: kzalloc - freed during kfree
>> * card, chip: snd_device_new - freed during snd_device_free
>> * chip: new_pcm - TODO: free pcm
>> * chip: new_ctl - TODO: free ctl
>> * card: snd_card_register - unregistered during snd_card_free
>>
>> Free / unregister sequence:
>>
>> * card: snd_card_free
>> * card, chip: snd_device_free
>> * childdev: device_unregister
>> * chip: kfree
>>
>> Steps to reproduce the issue before this commit:
>>
>> ~~~~
>> $ rmmod snd_bcm2835
>> $ aplay -L
>> [ 138.648130] Unable to handle kernel paging request at virtual address 7f1343c0
>> [ 138.660415] pgd = ad8f0000
>> [ 138.665567] [7f1343c0] *pgd=3864c811, *pte=00000000, *ppte=00000000
>> [ 138.674887] Internal error: Oops: 7 [#1] SMP ARM
>> [ 138.683571] Modules linked in: sha256_generic cfg80211 rfkill snd_pcm snd_timer
>> snd fixed uio_pdrv_genirq uio ip_tables x_tables ipv6 [last unloaded: snd_bcm2835
>> ]
>> [ 138.706594] CPU: 3 PID: 463 Comm: aplay Tainted: G WC 4.15.0-rc1-v
>> 7+ #6
>> [ 138.719833] Hardware name: BCM2835
>> [ 138.726016] task: b877ac00 task.stack: aebec000
>> [ 138.733408] PC is at try_module_get+0x38/0x24c
>> [ 138.740813] LR is at snd_ctl_open+0x58/0x194 [snd]
>> [ 138.748485] pc : [<801c4d5c>] lr : [<7f0e6b2c>] psr: 20000013
>> [ 138.757709] sp : aebedd60 ip : aebedd88 fp : aebedd84
>> [ 138.765884] r10: 00000000 r9 : 00000004 r8 : 7f0ed440
>> [ 138.774040] r7 : b7e469b0 r6 : 7f0e6b2c r5 : afd91900 r4 : 7f1343c0
>> [ 138.783571] r3 : aebec000 r2 : 00000001 r1 : b877ac00 r0 : 7f1343c0
>> [ 138.793084] Flags: nzCv IRQs on FIQs on Mode SVC_32 ISA ARM Segment user
>> [ 138.803300] Control: 10c5387d Table: 2d8f006a DAC: 00000055
>> [ 138.812064] Process aplay (pid: 463, stack limit = 0xaebec210)
>> [ 138.820868] Stack: (0xaebedd60 to 0xaebee000)
>> [ 138.828207] dd60: 00000000 b848d000 afd91900 00000000 b7e469b0 7f0ed440 aebedda4 aebedd88
>> [ 138.842371] dd80: 7f0e6b2c 801c4d30 afd91900 7f0ea4dc 00000000 b7e469b0 aebeddcc aebedda8
>> [ 138.856611] dda0: 7f0e250c 7f0e6ae0 7f0e2464 b8478ec0 b7e469b0 afd91900 7f0ea388 00000000
>> [ 138.870864] ddc0: aebeddf4 aebeddd0 802ce590 7f0e2470 8090ab64 afd91900 afd91900 b7e469b0
>> [ 138.885301] dde0: afd91908 802ce4e4 aebede1c aebeddf8 802c57b4 802ce4f0 afd91900 aebedea8
>> [ 138.900110] de00: b7fa4c00 00000000 00000000 00000004 aebede3c aebede20 802c6ba8 802c56b4
>> [ 138.915260] de20: aebedea8 00000000 aebedf5c 00000000 aebedea4 aebede40 802d9a68 802c6b58
>> [ 138.930661] de40: b874ddd0 00000000 00000000 00000001 00000041 00000000 afd91900 aebede70
>> [ 138.946402] de60: 00000000 00000000 00000002 b7e469b0 b8a87610 b8d6ab80 801852f8 00080000
>> [ 138.962314] de80: aebedf5c aebedea8 00000001 80108464 aebec000 00000000 aebedf4c aebedea8
>> [ 138.978414] dea0: 802dacd4 802d970c b8a87610 b8d6ab80 a7982bc6 00000009 af363019 b9231480
>> [ 138.994617] dec0: 00000000 b8c038a0 b7e469b0 00000101 00000002 00000238 00000000 00000000
>> [ 139.010823] dee0: 00000000 aebedee8 00080000 0000000f aebedf3c aebedf00 802ed7e4 80843f94
>> [ 139.027025] df00: 00000003 00080000 b9231490 b9231480 00000000 00080000 af363000 00000000
>> [ 139.043229] df20: 00000005 00000002 ffffff9c 00000000 00080000 ffffff9c af363000 00000003
>> [ 139.059430] df40: aebedf94 aebedf50 802c6f70 802dac70 aebec000 00000000 00000001 00000000
>> [ 139.075629] df60: 00020000 00000004 00000100 00000001 7ebe577c 0002e038 00000000 00000005
>> [ 139.091828] df80: 80108464 aebec000 aebedfa4 aebedf98 802c7060 802c6e6c 00000000 aebedfa8
>> [ 139.108025] dfa0: 801082c0 802c7040 7ebe577c 0002e038 7ebe577c 00080000 00000b98 e81c8400
>> [ 139.124222] dfc0: 7ebe577c 0002e038 00000000 00000005 7ebe57e4 00a20af8 7ebe57f0 76f87394
>> [ 139.140419] dfe0: 00000000 7ebe55c4 76ec88e8 76df1d9c 60000010 7ebe577c 00000000 00000000
>> [ 139.156715] [<801c4d5c>] (try_module_get) from [<7f0e6b2c>] (snd_ctl_open+0x58/0x194 [snd])
>> [ 139.173222] [<7f0e6b2c>] (snd_ctl_open [snd]) from [<7f0e250c>] (snd_open+0xa8/0x14c [snd])
>> [ 139.189683] [<7f0e250c>] (snd_open [snd]) from [<802ce590>] (chrdev_open+0xac/0x188)
>> [ 139.205465] [<802ce590>] (chrdev_open) from [<802c57b4>] (do_dentry_open+0x10c/0x314)
>> [ 139.221347] [<802c57b4>] (do_dentry_open) from [<802c6ba8>] (vfs_open+0x5c/0x88)
>> [ 139.236788] [<802c6ba8>] (vfs_open) from [<802d9a68>] (path_openat+0x368/0x944)
>> [ 139.248270] [<802d9a68>] (path_openat) from [<802dacd4>] (do_filp_open+0x70/0xc4)
>> [ 139.263731] [<802dacd4>] (do_filp_open) from [<802c6f70>] (do_sys_open+0x110/0x1d4)
>> [ 139.279378] [<802c6f70>] (do_sys_open) from [<802c7060>] (SyS_open+0x2c/0x30)
>> [ 139.290647] [<802c7060>] (SyS_open) from [<801082c0>] (ret_fast_syscall+0x0/0x28)
>> [ 139.306021] Code: e3c3303f e5932004 e2822001 e5832004 (e5943000)
>> [ 139.316265] ---[ end trace 7f3f7f6193b663ed ]---
>> [ 139.324956] note: aplay[463] exited with preempt_count 1
>> ~~~~
>>
> Sorry, just noticed that devm part doesn't make sense after your patch
> and misleading.
> So, after addressing it, FWIW,
> Reviewed-by: Andy Shevchenko <[email protected]>

I checked that actually devm makes sense and works as expected.
I don't call snd_devm_release() manually, it is done by the devm resource management.

So, the devm part does it's job correctly, that's why I will keep it as it is.

Below is the backtrace, which demonstrates that devm does a proper automatic resource management.

~~~~
[  207.569694] [<7f12f04c>] (snd_devm_release [snd_bcm2835]) from [<805b5198>] (device_release+0x3c/0xa0)
[  207.586486] [<805b5198>] (device_release) from [<8082abe4>] (kobject_put+0x94/0xf0)
[  207.601569] [<8082abe4>] (kobject_put) from [<805b66a8>] (device_unregister+0x2c/0x30)
[  207.616940] [<805b66a8>] (device_unregister) from [<7f12f09c>] (snd_devm_unregister_child+0x4c/0x5c [snd_bcm2835])
[  207.634965] [<7f12f09c>] (snd_devm_unregister_child [snd_bcm2835]) from [<805bda58>] (release_nodes+0x120/0x1e8)
[  207.652930] [<805bda58>] (release_nodes) from [<805be55c>] (devres_release_all+0x40/0x5c)
[  207.668825] [<805be55c>] (devres_release_all) from [<805baaf8>] (device_release_driver_internal+0x170/0x200)
[  207.686573] [<805baaf8>] (device_release_driver_internal) from [<805babf4>] (driver_detach+0x48/0x7c)
[  207.703887] [<805babf4>] (driver_detach) from [<805b9b88>] (bus_remove_driver+0x5c/0xb0)
[  207.720063] [<805b9b88>] (bus_remove_driver) from [<805bb07c>] (driver_unregister+0x38/0x58)
[  207.736660] [<805bb07c>] (driver_unregister) from [<805bc2f4>] (platform_driver_unregister+0x1c/0x20)
[  207.754083] [<805bc2f4>] (platform_driver_unregister) from [<7f12f6fc>] (bcm2835_alsa_device_exit+0x1c/0x24 [snd_bcm2835])
[  207.773395] [<7f12f6fc>] (bcm2835_alsa_device_exit [snd_bcm2835]) from [<801c72b0>] (SyS_delete_module+0x17c/0x1d4)
[  207.792066] [<801c72b0>] (SyS_delete_module) from [<801082c0>] (ret_fast_syscall+0x0/0x28)
~~~~

> P.S. I didn't get why do you need an empty ->remove() stub. Doesn't
> work without it?

I checked: it really works fine without the empty remove() function.
I will get rid of it, and send the result as a patch v4.

>
>> Signed-off-by: Kirill Marinushkin <[email protected]>
>> Cc: Eric Anholt <[email protected]>
>> Cc: Stefan Wahren <[email protected]>
>> Cc: Greg Kroah-Hartman <[email protected]>
>> Cc: Florian Fainelli <[email protected]>
>> Cc: Ray Jui <[email protected]>
>> Cc: Scott Branden <[email protected]>
>> Cc: [email protected]
>> Cc: Michael Zoran <[email protected]>
>> Cc: [email protected]
>> Cc: [email protected]
>> Cc: [email protected]
>> Cc: [email protected]
>> ---
>> .../staging/vc04_services/bcm2835-audio/bcm2835.c | 57 ++++++++++++----------
>> 1 file changed, 30 insertions(+), 27 deletions(-)
>>
>> diff --git a/drivers/staging/vc04_services/bcm2835-audio/bcm2835.c b/drivers/staging/vc04_services/bcm2835-audio/bcm2835.c
>> index 8f2d508183b2..125efc55ecb9 100644
>> --- a/drivers/staging/vc04_services/bcm2835-audio/bcm2835.c
>> +++ b/drivers/staging/vc04_services/bcm2835-audio/bcm2835.c
>> @@ -36,6 +36,10 @@ MODULE_PARM_DESC(enable_compat_alsa,
>> static void snd_devm_unregister_child(struct device *dev, void *res)
>> {
>> struct device *childdev = *(struct device **)res;
>> + struct bcm2835_chip *chip = dev_get_drvdata(childdev);
>> + struct snd_card *card = chip->card;
>> +
>> + snd_card_free(card);
>>
>> device_unregister(childdev);
>> }
>> @@ -61,6 +65,13 @@ static int snd_devm_add_child(struct device *dev, struct device *child)
>> return 0;
>> }
>>
>> +static void snd_devm_release(struct device *dev)
>> +{
>> + struct bcm2835_chip *chip = dev_get_drvdata(dev);
>> +
>> + kfree(chip);
>> +}
>> +
>> static struct device *
>> snd_create_device(struct device *parent,
>> struct device_driver *driver,
>> @@ -76,6 +87,7 @@ snd_create_device(struct device *parent,
>> device_initialize(device);
>> device->parent = parent;
>> device->driver = driver;
>> + device->release = snd_devm_release;
>>
>> dev_set_name(device, "%s", name);
>>
>> @@ -86,18 +98,19 @@ snd_create_device(struct device *parent,
>> return device;
>> }
>>
>> -static int snd_bcm2835_free(struct bcm2835_chip *chip)
>> -{
>> - kfree(chip);
>> - return 0;
>> -}
>> -
>> /* component-destructor
>> * (see "Management of Cards and Components")
>> */
>> static int snd_bcm2835_dev_free(struct snd_device *device)
>> {
>> - return snd_bcm2835_free(device->device_data);
>> + struct bcm2835_chip *chip = device->device_data;
>> + struct snd_card *card = chip->card;
>> +
>> + /* TODO: free pcm, ctl */
>> +
>> + snd_device_free(card, chip);
>> +
>> + return 0;
>> }
>>
>> /* chip-specific constructor
>> @@ -122,7 +135,7 @@ static int snd_bcm2835_create(struct snd_card *card,
>>
>> err = snd_device_new(card, SNDRV_DEV_LOWLEVEL, chip, &ops);
>> if (err) {
>> - snd_bcm2835_free(chip);
>> + kfree(chip);
>> return err;
>> }
>>
>> @@ -130,31 +143,14 @@ static int snd_bcm2835_create(struct snd_card *card,
>> return 0;
>> }
>>
>> -static void snd_devm_card_free(struct device *dev, void *res)
>> -{
>> - struct snd_card *snd_card = *(struct snd_card **)res;
>> -
>> - snd_card_free(snd_card);
>> -}
>> -
>> static struct snd_card *snd_devm_card_new(struct device *dev)
>> {
>> - struct snd_card **dr;
>> struct snd_card *card;
>> int ret;
>>
>> - dr = devres_alloc(snd_devm_card_free, sizeof(*dr), GFP_KERNEL);
>> - if (!dr)
>> - return ERR_PTR(-ENOMEM);
>> -
>> ret = snd_card_new(dev, -1, NULL, THIS_MODULE, 0, &card);
>> - if (ret) {
>> - devres_free(dr);
>> + if (ret)
>> return ERR_PTR(ret);
>> - }
>> -
>> - *dr = card;
>> - devres_add(dev, dr);
>>
>> return card;
>> }
>> @@ -313,7 +309,7 @@ static int snd_add_child_device(struct device *device,
>> return err;
>> }
>>
>> - dev_set_drvdata(child, card);
>> + dev_set_drvdata(child, chip);
>> dev_info(child, "card created with %d channels\n", numchans);
>>
>> return 0;
>> @@ -414,6 +410,12 @@ static int snd_bcm2835_alsa_probe_dt(struct platform_device *pdev)
>> return 0;
>> }
>>
>> +static int snd_bcm2835_alsa_remove(struct platform_device *pdev)
>> +{
>> + /* trigger snd_devm_unregister_child() */
>> + return 0;
>> +}
>> +
>> #ifdef CONFIG_PM
>>
>> static int snd_bcm2835_alsa_suspend(struct platform_device *pdev,
>> @@ -437,6 +439,7 @@ MODULE_DEVICE_TABLE(of, snd_bcm2835_of_match_table);
>>
>> static struct platform_driver bcm2835_alsa0_driver = {
>> .probe = snd_bcm2835_alsa_probe_dt,
>> + .remove = snd_bcm2835_alsa_remove,
>> #ifdef CONFIG_PM
>> .suspend = snd_bcm2835_alsa_suspend,
>> .resume = snd_bcm2835_alsa_resume,
>> --
>> 2.13.6
>>
>
>

2018-03-22 21:38:18

by Kirill Marinushkin

Subject: [PATCH v4] staging: bcm2835-audio: Release resources on module_exit()

In the current implementation, `rmmod snd_bcm2835` does not release
resources properly. It causes an oops when trying to list sound devices.

This commit fixes it.

The details WRT allocation / free are described below.

Device structure WRT allocation:

pdev
\childdev[]
\card
\chip
\pcm
\ctl

Allocation / register sequence:

* childdev: devm_kzalloc - freed during driver detach
* childdev: device_initialize - freed during device_unregister
* pdev: devres_alloc - freed during driver detach
* childdev: device_add - removed during device_unregister
* pdev, childdev: devres_add - freed during driver detach
* card: snd_card_new - freed during snd_card_free
* chip: kzalloc - freed during kfree
* card, chip: snd_device_new - freed during snd_device_free
* chip: new_pcm - TODO: free pcm
* chip: new_ctl - TODO: free ctl
* card: snd_card_register - unregistered during snd_card_free

Free / unregister sequence:

* card: snd_card_free
* card, chip: snd_device_free
* childdev: device_unregister
* chip: kfree

Steps to reproduce the issue before this commit:

~~~~
$ rmmod snd_bcm2835
$ aplay -L
[ 138.648130] Unable to handle kernel paging request at virtual address 7f1343c0
[ 138.660415] pgd = ad8f0000
[ 138.665567] [7f1343c0] *pgd=3864c811, *pte=00000000, *ppte=00000000
[ 138.674887] Internal error: Oops: 7 [#1] SMP ARM
[ 138.683571] Modules linked in: sha256_generic cfg80211 rfkill snd_pcm snd_timer
snd fixed uio_pdrv_genirq uio ip_tables x_tables ipv6 [last unloaded: snd_bcm2835
]
[ 138.706594] CPU: 3 PID: 463 Comm: aplay Tainted: G WC 4.15.0-rc1-v
7+ #6
[ 138.719833] Hardware name: BCM2835
[ 138.726016] task: b877ac00 task.stack: aebec000
[ 138.733408] PC is at try_module_get+0x38/0x24c
[ 138.740813] LR is at snd_ctl_open+0x58/0x194 [snd]
[ 138.748485] pc : [<801c4d5c>] lr : [<7f0e6b2c>] psr: 20000013
[ 138.757709] sp : aebedd60 ip : aebedd88 fp : aebedd84
[ 138.765884] r10: 00000000 r9 : 00000004 r8 : 7f0ed440
[ 138.774040] r7 : b7e469b0 r6 : 7f0e6b2c r5 : afd91900 r4 : 7f1343c0
[ 138.783571] r3 : aebec000 r2 : 00000001 r1 : b877ac00 r0 : 7f1343c0
[ 138.793084] Flags: nzCv IRQs on FIQs on Mode SVC_32 ISA ARM Segment user
[ 138.803300] Control: 10c5387d Table: 2d8f006a DAC: 00000055
[ 138.812064] Process aplay (pid: 463, stack limit = 0xaebec210)
[ 138.820868] Stack: (0xaebedd60 to 0xaebee000)
[ 138.828207] dd60: 00000000 b848d000 afd91900 00000000 b7e469b0 7f0ed440 aebedda4 aebedd88
[ 138.842371] dd80: 7f0e6b2c 801c4d30 afd91900 7f0ea4dc 00000000 b7e469b0 aebeddcc aebedda8
[ 138.856611] dda0: 7f0e250c 7f0e6ae0 7f0e2464 b8478ec0 b7e469b0 afd91900 7f0ea388 00000000
[ 138.870864] ddc0: aebeddf4 aebeddd0 802ce590 7f0e2470 8090ab64 afd91900 afd91900 b7e469b0
[ 138.885301] dde0: afd91908 802ce4e4 aebede1c aebeddf8 802c57b4 802ce4f0 afd91900 aebedea8
[ 138.900110] de00: b7fa4c00 00000000 00000000 00000004 aebede3c aebede20 802c6ba8 802c56b4
[ 138.915260] de20: aebedea8 00000000 aebedf5c 00000000 aebedea4 aebede40 802d9a68 802c6b58
[ 138.930661] de40: b874ddd0 00000000 00000000 00000001 00000041 00000000 afd91900 aebede70
[ 138.946402] de60: 00000000 00000000 00000002 b7e469b0 b8a87610 b8d6ab80 801852f8 00080000
[ 138.962314] de80: aebedf5c aebedea8 00000001 80108464 aebec000 00000000 aebedf4c aebedea8
[ 138.978414] dea0: 802dacd4 802d970c b8a87610 b8d6ab80 a7982bc6 00000009 af363019 b9231480
[ 138.994617] dec0: 00000000 b8c038a0 b7e469b0 00000101 00000002 00000238 00000000 00000000
[ 139.010823] dee0: 00000000 aebedee8 00080000 0000000f aebedf3c aebedf00 802ed7e4 80843f94
[ 139.027025] df00: 00000003 00080000 b9231490 b9231480 00000000 00080000 af363000 00000000
[ 139.043229] df20: 00000005 00000002 ffffff9c 00000000 00080000 ffffff9c af363000 00000003
[ 139.059430] df40: aebedf94 aebedf50 802c6f70 802dac70 aebec000 00000000 00000001 00000000
[ 139.075629] df60: 00020000 00000004 00000100 00000001 7ebe577c 0002e038 00000000 00000005
[ 139.091828] df80: 80108464 aebec000 aebedfa4 aebedf98 802c7060 802c6e6c 00000000 aebedfa8
[ 139.108025] dfa0: 801082c0 802c7040 7ebe577c 0002e038 7ebe577c 00080000 00000b98 e81c8400
[ 139.124222] dfc0: 7ebe577c 0002e038 00000000 00000005 7ebe57e4 00a20af8 7ebe57f0 76f87394
[ 139.140419] dfe0: 00000000 7ebe55c4 76ec88e8 76df1d9c 60000010 7ebe577c 00000000 00000000
[ 139.156715] [<801c4d5c>] (try_module_get) from [<7f0e6b2c>] (snd_ctl_open+0x58/0x194 [snd])
[ 139.173222] [<7f0e6b2c>] (snd_ctl_open [snd]) from [<7f0e250c>] (snd_open+0xa8/0x14c [snd])
[ 139.189683] [<7f0e250c>] (snd_open [snd]) from [<802ce590>] (chrdev_open+0xac/0x188)
[ 139.205465] [<802ce590>] (chrdev_open) from [<802c57b4>] (do_dentry_open+0x10c/0x314)
[ 139.221347] [<802c57b4>] (do_dentry_open) from [<802c6ba8>] (vfs_open+0x5c/0x88)
[ 139.236788] [<802c6ba8>] (vfs_open) from [<802d9a68>] (path_openat+0x368/0x944)
[ 139.248270] [<802d9a68>] (path_openat) from [<802dacd4>] (do_filp_open+0x70/0xc4)
[ 139.263731] [<802dacd4>] (do_filp_open) from [<802c6f70>] (do_sys_open+0x110/0x1d4)
[ 139.279378] [<802c6f70>] (do_sys_open) from [<802c7060>] (SyS_open+0x2c/0x30)
[ 139.290647] [<802c7060>] (SyS_open) from [<801082c0>] (ret_fast_syscall+0x0/0x28)
[ 139.306021] Code: e3c3303f e5932004 e2822001 e5832004 (e5943000)
[ 139.316265] ---[ end trace 7f3f7f6193b663ed ]---
[ 139.324956] note: aplay[463] exited with preempt_count 1
~~~~

Signed-off-by: Kirill Marinushkin <[email protected]>
Cc: Eric Anholt <[email protected]>
Cc: Stefan Wahren <[email protected]>
Cc: Greg Kroah-Hartman <[email protected]>
Cc: Florian Fainelli <[email protected]>
Cc: Ray Jui <[email protected]>
Cc: Scott Branden <[email protected]>
Cc: [email protected]
Cc: Michael Zoran <[email protected]>
Cc: Andy Shevchenko <[email protected]>
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
---
.../staging/vc04_services/bcm2835-audio/bcm2835.c | 50 ++++++++++------------
1 file changed, 23 insertions(+), 27 deletions(-)

diff --git a/drivers/staging/vc04_services/bcm2835-audio/bcm2835.c b/drivers/staging/vc04_services/bcm2835-audio/bcm2835.c
index 8f2d508183b2..b224e0764ba6 100644
--- a/drivers/staging/vc04_services/bcm2835-audio/bcm2835.c
+++ b/drivers/staging/vc04_services/bcm2835-audio/bcm2835.c
@@ -36,6 +36,10 @@ MODULE_PARM_DESC(enable_compat_alsa,
static void snd_devm_unregister_child(struct device *dev, void *res)
{
struct device *childdev = *(struct device **)res;
+ struct bcm2835_chip *chip = dev_get_drvdata(childdev);
+ struct snd_card *card = chip->card;
+
+ snd_card_free(card);

device_unregister(childdev);
}
@@ -61,6 +65,13 @@ static int snd_devm_add_child(struct device *dev, struct device *child)
return 0;
}

+static void snd_devm_release(struct device *dev)
+{
+ struct bcm2835_chip *chip = dev_get_drvdata(dev);
+
+ kfree(chip);
+}
+
static struct device *
snd_create_device(struct device *parent,
struct device_driver *driver,
@@ -76,6 +87,7 @@ snd_create_device(struct device *parent,
device_initialize(device);
device->parent = parent;
device->driver = driver;
+ device->release = snd_devm_release;

dev_set_name(device, "%s", name);

@@ -86,18 +98,19 @@ snd_create_device(struct device *parent,
return device;
}

-static int snd_bcm2835_free(struct bcm2835_chip *chip)
-{
- kfree(chip);
- return 0;
-}
-
/* component-destructor
* (see "Management of Cards and Components")
*/
static int snd_bcm2835_dev_free(struct snd_device *device)
{
- return snd_bcm2835_free(device->device_data);
+ struct bcm2835_chip *chip = device->device_data;
+ struct snd_card *card = chip->card;
+
+ /* TODO: free pcm, ctl */
+
+ snd_device_free(card, chip);
+
+ return 0;
}

/* chip-specific constructor
@@ -122,7 +135,7 @@ static int snd_bcm2835_create(struct snd_card *card,

err = snd_device_new(card, SNDRV_DEV_LOWLEVEL, chip, &ops);
if (err) {
- snd_bcm2835_free(chip);
+ kfree(chip);
return err;
}

@@ -130,31 +143,14 @@ static int snd_bcm2835_create(struct snd_card *card,
return 0;
}

-static void snd_devm_card_free(struct device *dev, void *res)
-{
- struct snd_card *snd_card = *(struct snd_card **)res;
-
- snd_card_free(snd_card);
-}
-
static struct snd_card *snd_devm_card_new(struct device *dev)
{
- struct snd_card **dr;
struct snd_card *card;
int ret;

- dr = devres_alloc(snd_devm_card_free, sizeof(*dr), GFP_KERNEL);
- if (!dr)
- return ERR_PTR(-ENOMEM);
-
ret = snd_card_new(dev, -1, NULL, THIS_MODULE, 0, &card);
- if (ret) {
- devres_free(dr);
+ if (ret)
return ERR_PTR(ret);
- }
-
- *dr = card;
- devres_add(dev, dr);

return card;
}
@@ -313,7 +309,7 @@ static int snd_add_child_device(struct device *device,
return err;
}

- dev_set_drvdata(child, card);
+ dev_set_drvdata(child, chip);
dev_info(child, "card created with %d channels\n", numchans);

return 0;
--
2.13.6

2018-03-23 16:25:13

by Andy Shevchenko

Subject: Re: [PATCH v4] staging: bcm2835-audio: Release resources on module_exit()

On Thu, Mar 22, 2018 at 11:37 PM, Kirill Marinushkin
<[email protected]> wrote:


> +static void snd_devm_release(struct device *dev)
> +{
> + struct bcm2835_chip *chip = dev_get_drvdata(dev);
> +
> + kfree(chip);
> +}

> + device->release = snd_devm_release;

This is not devm function, so, it would be rather called
snd_bcm2835_release().

> static struct snd_card *snd_devm_card_new(struct device *dev)
> {
> struct snd_card *card;
> int ret;
>
> ret = snd_card_new(dev, -1, NULL, THIS_MODULE, 0, &card);
> + if (ret)
> return ERR_PTR(ret);
>
> return card;
> }

Same here. You removed anything related to devm here, so, please make
function name consistent.

After addressing above, FWIW,

Reviewed-by: Andy Shevchenko <[email protected]>

--
With Best Regards,
Andy Shevchenko

2018-03-23 19:22:41

by Kirill Marinushkin

Subject: Re: [PATCH v4] staging: bcm2835-audio: Release resources on module_exit()

On 03/23/18 17:23, Andy Shevchenko wrote:
> On Thu, Mar 22, 2018 at 11:37 PM, Kirill Marinushkin
> <[email protected]> wrote:
>
>
>> +static void snd_devm_release(struct device *dev)
>> +{
>> + struct bcm2835_chip *chip = dev_get_drvdata(dev);
>> +
>> + kfree(chip);
>> +}
>> + device->release = snd_devm_release;
> This is not devm function, so, it would be rather called
> snd_bcm2835_release().

Ah, you mean "devm" in the names. Now I got it.
Agree, I will rename it to "snd_bcm2835_release()"

>
>> static struct snd_card *snd_devm_card_new(struct device *dev)
>> {
>> struct snd_card *card;
>> int ret;
>>
>> ret = snd_card_new(dev, -1, NULL, THIS_MODULE, 0, &card);
>> + if (ret)
>> return ERR_PTR(ret);
>>
>> return card;
>> }
> Same here. You removed anything related to devm here, so, please make
> function name consistent.

Good, I will rename it to "snd_bcm2835_card_new()"

> After addressing above, FWIW,
>
> Reviewed-by: Andy Shevchenko <[email protected]>
>

2018-03-23 19:34:04

by Kirill Marinushkin

Subject: [PATCH v5] staging: bcm2835-audio: Release resources on module_exit()

In the current implementation, `rmmod snd_bcm2835` does not release
resources properly. It causes an oops when trying to list sound devices.

This commit fixes it.

The details WRT allocation / free are described below.

Device structure WRT allocation:

pdev
\childdev[]
\card
\chip
\pcm
\ctl

Allocation / register sequence:

* childdev: devm_kzalloc - freed during driver detach
* childdev: device_initialize - freed during device_unregister
* pdev: devres_alloc - freed during driver detach
* childdev: device_add - removed during device_unregister
* pdev, childdev: devres_add - freed during driver detach
* card: snd_card_new - freed during snd_card_free
* chip: kzalloc - freed during kfree
* card, chip: snd_device_new - freed during snd_device_free
* chip: new_pcm - TODO: free pcm
* chip: new_ctl - TODO: free ctl
* card: snd_card_register - unregistered during snd_card_free

Free / unregister sequence:

* card: snd_card_free
* card, chip: snd_device_free
* childdev: device_unregister
* chip: kfree

Steps to reproduce the issue before this commit:

~~~~
$ rmmod snd_bcm2835
$ aplay -L
[ 138.648130] Unable to handle kernel paging request at virtual address 7f1343c0
[ 138.660415] pgd = ad8f0000
[ 138.665567] [7f1343c0] *pgd=3864c811, *pte=00000000, *ppte=00000000
[ 138.674887] Internal error: Oops: 7 [#1] SMP ARM
[ 138.683571] Modules linked in: sha256_generic cfg80211 rfkill snd_pcm snd_timer
snd fixed uio_pdrv_genirq uio ip_tables x_tables ipv6 [last unloaded: snd_bcm2835
]
[ 138.706594] CPU: 3 PID: 463 Comm: aplay Tainted: G WC 4.15.0-rc1-v
7+ #6
[ 138.719833] Hardware name: BCM2835
[ 138.726016] task: b877ac00 task.stack: aebec000
[ 138.733408] PC is at try_module_get+0x38/0x24c
[ 138.740813] LR is at snd_ctl_open+0x58/0x194 [snd]
[ 138.748485] pc : [<801c4d5c>] lr : [<7f0e6b2c>] psr: 20000013
[ 138.757709] sp : aebedd60 ip : aebedd88 fp : aebedd84
[ 138.765884] r10: 00000000 r9 : 00000004 r8 : 7f0ed440
[ 138.774040] r7 : b7e469b0 r6 : 7f0e6b2c r5 : afd91900 r4 : 7f1343c0
[ 138.783571] r3 : aebec000 r2 : 00000001 r1 : b877ac00 r0 : 7f1343c0
[ 138.793084] Flags: nzCv IRQs on FIQs on Mode SVC_32 ISA ARM Segment user
[ 138.803300] Control: 10c5387d Table: 2d8f006a DAC: 00000055
[ 138.812064] Process aplay (pid: 463, stack limit = 0xaebec210)
[ 138.820868] Stack: (0xaebedd60 to 0xaebee000)
[ 138.828207] dd60: 00000000 b848d000 afd91900 00000000 b7e469b0 7f0ed440 aebedda4 aebedd88
[ 138.842371] dd80: 7f0e6b2c 801c4d30 afd91900 7f0ea4dc 00000000 b7e469b0 aebeddcc aebedda8
[ 138.856611] dda0: 7f0e250c 7f0e6ae0 7f0e2464 b8478ec0 b7e469b0 afd91900 7f0ea388 00000000
[ 138.870864] ddc0: aebeddf4 aebeddd0 802ce590 7f0e2470 8090ab64 afd91900 afd91900 b7e469b0
[ 138.885301] dde0: afd91908 802ce4e4 aebede1c aebeddf8 802c57b4 802ce4f0 afd91900 aebedea8
[ 138.900110] de00: b7fa4c00 00000000 00000000 00000004 aebede3c aebede20 802c6ba8 802c56b4
[ 138.915260] de20: aebedea8 00000000 aebedf5c 00000000 aebedea4 aebede40 802d9a68 802c6b58
[ 138.930661] de40: b874ddd0 00000000 00000000 00000001 00000041 00000000 afd91900 aebede70
[ 138.946402] de60: 00000000 00000000 00000002 b7e469b0 b8a87610 b8d6ab80 801852f8 00080000
[ 138.962314] de80: aebedf5c aebedea8 00000001 80108464 aebec000 00000000 aebedf4c aebedea8
[ 138.978414] dea0: 802dacd4 802d970c b8a87610 b8d6ab80 a7982bc6 00000009 af363019 b9231480
[ 138.994617] dec0: 00000000 b8c038a0 b7e469b0 00000101 00000002 00000238 00000000 00000000
[ 139.010823] dee0: 00000000 aebedee8 00080000 0000000f aebedf3c aebedf00 802ed7e4 80843f94
[ 139.027025] df00: 00000003 00080000 b9231490 b9231480 00000000 00080000 af363000 00000000
[ 139.043229] df20: 00000005 00000002 ffffff9c 00000000 00080000 ffffff9c af363000 00000003
[ 139.059430] df40: aebedf94 aebedf50 802c6f70 802dac70 aebec000 00000000 00000001 00000000
[ 139.075629] df60: 00020000 00000004 00000100 00000001 7ebe577c 0002e038 00000000 00000005
[ 139.091828] df80: 80108464 aebec000 aebedfa4 aebedf98 802c7060 802c6e6c 00000000 aebedfa8
[ 139.108025] dfa0: 801082c0 802c7040 7ebe577c 0002e038 7ebe577c 00080000 00000b98 e81c8400
[ 139.124222] dfc0: 7ebe577c 0002e038 00000000 00000005 7ebe57e4 00a20af8 7ebe57f0 76f87394
[ 139.140419] dfe0: 00000000 7ebe55c4 76ec88e8 76df1d9c 60000010 7ebe577c 00000000 00000000
[ 139.156715] [<801c4d5c>] (try_module_get) from [<7f0e6b2c>] (snd_ctl_open+0x58/0x194 [snd])
[ 139.173222] [<7f0e6b2c>] (snd_ctl_open [snd]) from [<7f0e250c>] (snd_open+0xa8/0x14c [snd])
[ 139.189683] [<7f0e250c>] (snd_open [snd]) from [<802ce590>] (chrdev_open+0xac/0x188)
[ 139.205465] [<802ce590>] (chrdev_open) from [<802c57b4>] (do_dentry_open+0x10c/0x314)
[ 139.221347] [<802c57b4>] (do_dentry_open) from [<802c6ba8>] (vfs_open+0x5c/0x88)
[ 139.236788] [<802c6ba8>] (vfs_open) from [<802d9a68>] (path_openat+0x368/0x944)
[ 139.248270] [<802d9a68>] (path_openat) from [<802dacd4>] (do_filp_open+0x70/0xc4)
[ 139.263731] [<802dacd4>] (do_filp_open) from [<802c6f70>] (do_sys_open+0x110/0x1d4)
[ 139.279378] [<802c6f70>] (do_sys_open) from [<802c7060>] (SyS_open+0x2c/0x30)
[ 139.290647] [<802c7060>] (SyS_open) from [<801082c0>] (ret_fast_syscall+0x0/0x28)
[ 139.306021] Code: e3c3303f e5932004 e2822001 e5832004 (e5943000)
[ 139.316265] ---[ end trace 7f3f7f6193b663ed ]---
[ 139.324956] note: aplay[463] exited with preempt_count 1
~~~~

Signed-off-by: Kirill Marinushkin <[email protected]>
Cc: Eric Anholt <[email protected]>
Cc: Stefan Wahren <[email protected]>
Cc: Greg Kroah-Hartman <[email protected]>
Cc: Florian Fainelli <[email protected]>
Cc: Ray Jui <[email protected]>
Cc: Scott Branden <[email protected]>
Cc: [email protected]
Cc: Michael Zoran <[email protected]>
Cc: Andy Shevchenko <[email protected]>
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
---
.../staging/vc04_services/bcm2835-audio/bcm2835.c | 54 ++++++++++------------
1 file changed, 25 insertions(+), 29 deletions(-)

diff --git a/drivers/staging/vc04_services/bcm2835-audio/bcm2835.c b/drivers/staging/vc04_services/bcm2835-audio/bcm2835.c
index 8f2d508183b2..9030d71a3d0b 100644
--- a/drivers/staging/vc04_services/bcm2835-audio/bcm2835.c
+++ b/drivers/staging/vc04_services/bcm2835-audio/bcm2835.c
@@ -36,6 +36,10 @@ MODULE_PARM_DESC(enable_compat_alsa,
static void snd_devm_unregister_child(struct device *dev, void *res)
{
struct device *childdev = *(struct device **)res;
+ struct bcm2835_chip *chip = dev_get_drvdata(childdev);
+ struct snd_card *card = chip->card;
+
+ snd_card_free(card);

device_unregister(childdev);
}
@@ -61,6 +65,13 @@ static int snd_devm_add_child(struct device *dev, struct device *child)
return 0;
}

+static void snd_bcm2835_release(struct device *dev)
+{
+ struct bcm2835_chip *chip = dev_get_drvdata(dev);
+
+ kfree(chip);
+}
+
static struct device *
snd_create_device(struct device *parent,
struct device_driver *driver,
@@ -76,6 +87,7 @@ snd_create_device(struct device *parent,
device_initialize(device);
device->parent = parent;
device->driver = driver;
+ device->release = snd_bcm2835_release;

dev_set_name(device, "%s", name);

@@ -86,18 +98,19 @@ snd_create_device(struct device *parent,
return device;
}

-static int snd_bcm2835_free(struct bcm2835_chip *chip)
-{
- kfree(chip);
- return 0;
-}
-
/* component-destructor
* (see "Management of Cards and Components")
*/
static int snd_bcm2835_dev_free(struct snd_device *device)
{
- return snd_bcm2835_free(device->device_data);
+ struct bcm2835_chip *chip = device->device_data;
+ struct snd_card *card = chip->card;
+
+ /* TODO: free pcm, ctl */
+
+ snd_device_free(card, chip);
+
+ return 0;
}

/* chip-specific constructor
@@ -122,7 +135,7 @@ static int snd_bcm2835_create(struct snd_card *card,

err = snd_device_new(card, SNDRV_DEV_LOWLEVEL, chip, &ops);
if (err) {
- snd_bcm2835_free(chip);
+ kfree(chip);
return err;
}

@@ -130,31 +143,14 @@ static int snd_bcm2835_create(struct snd_card *card,
return 0;
}

-static void snd_devm_card_free(struct device *dev, void *res)
+static struct snd_card *snd_bcm2835_card_new(struct device *dev)
{
- struct snd_card *snd_card = *(struct snd_card **)res;
-
- snd_card_free(snd_card);
-}
-
-static struct snd_card *snd_devm_card_new(struct device *dev)
-{
- struct snd_card **dr;
struct snd_card *card;
int ret;

- dr = devres_alloc(snd_devm_card_free, sizeof(*dr), GFP_KERNEL);
- if (!dr)
- return ERR_PTR(-ENOMEM);
-
ret = snd_card_new(dev, -1, NULL, THIS_MODULE, 0, &card);
- if (ret) {
- devres_free(dr);
+ if (ret)
return ERR_PTR(ret);
- }
-
- *dr = card;
- devres_add(dev, dr);

return card;
}
@@ -271,7 +267,7 @@ static int snd_add_child_device(struct device *device,
return PTR_ERR(child);
}

- card = snd_devm_card_new(child);
+ card = snd_bcm2835_card_new(child);
if (IS_ERR(card)) {
dev_err(child, "Failed to create card");
return PTR_ERR(card);
@@ -313,7 +309,7 @@ static int snd_add_child_device(struct device *device,
return err;
}

- dev_set_drvdata(child, card);
+ dev_set_drvdata(child, chip);
dev_info(child, "card created with %d channels\n", numchans);

return 0;
--
2.13.6

2018-03-25 10:34:25

by Andy Shevchenko

Subject: Re: [PATCH v4] staging: bcm2835-audio: Release resources on module_exit()

On Fri, Mar 23, 2018 at 9:22 PM, Kirill Marinushkin
<[email protected]> wrote:
> On 03/23/18 17:23, Andy Shevchenko wrote:

>> After addressing above, FWIW,
>>
>> Reviewed-by: Andy Shevchenko <[email protected]>

Seems you missed my tag in new version.
When someone gives you a tag and you are going to send a new version
(w/o drastic changes), it's your responsibility to append it

I will send a new mail with it, so, this time no need to resend. Just
keep it for the future contributions.

--
With Best Regards,
Andy Shevchenko

2018-03-25 10:35:11

by Andy Shevchenko

Subject: Re: [PATCH v5] staging: bcm2835-audio: Release resources on module_exit()

On Fri, Mar 23, 2018 at 9:32 PM, Kirill Marinushkin
<[email protected]> wrote:
> In the current implementation, `rmmod snd_bcm2835` does not release
> resources properly. It causes an oops when trying to list sound devices.
>
> This commit fixes it.
>
> The details WRT allocation / free are described below.
>
> Device structure WRT allocation:
>
> pdev
> \childdev[]
> \card
> \chip
> \pcm
> \ctl
>
> Allocation / register sequence:
>
> * childdev: devm_kzalloc - freed during driver detach
> * childdev: device_initialize - freed during device_unregister
> * pdev: devres_alloc - freed during driver detach
> * childdev: device_add - removed during device_unregister
> * pdev, childdev: devres_add - freed during driver detach
> * card: snd_card_new - freed during snd_card_free
> * chip: kzalloc - freed during kfree
> * card, chip: snd_device_new - freed during snd_device_free
> * chip: new_pcm - TODO: free pcm
> * chip: new_ctl - TODO: free ctl
> * card: snd_card_register - unregistered during snd_card_free
>
> Free / unregister sequence:
>
> * card: snd_card_free
> * card, chip: snd_device_free
> * childdev: device_unregister
> * chip: kfree
>
> Steps to reproduce the issue before this commit:
>
> ~~~~
> $ rmmod snd_bcm2835
> $ aplay -L
> [ 138.648130] Unable to handle kernel paging request at virtual address 7f1343c0
> [ 138.660415] pgd = ad8f0000
> [ 138.665567] [7f1343c0] *pgd=3864c811, *pte=00000000, *ppte=00000000
> [ 138.674887] Internal error: Oops: 7 [#1] SMP ARM
> [ 138.683571] Modules linked in: sha256_generic cfg80211 rfkill snd_pcm snd_timer
> snd fixed uio_pdrv_genirq uio ip_tables x_tables ipv6 [last unloaded: snd_bcm2835
> ]
> [ 138.706594] CPU: 3 PID: 463 Comm: aplay Tainted: G WC 4.15.0-rc1-v
> 7+ #6
> [ 138.719833] Hardware name: BCM2835
> [ 138.726016] task: b877ac00 task.stack: aebec000
> [ 138.733408] PC is at try_module_get+0x38/0x24c
> [ 138.740813] LR is at snd_ctl_open+0x58/0x194 [snd]
> [ 138.748485] pc : [<801c4d5c>] lr : [<7f0e6b2c>] psr: 20000013
> [ 138.757709] sp : aebedd60 ip : aebedd88 fp : aebedd84
> [ 138.765884] r10: 00000000 r9 : 00000004 r8 : 7f0ed440
> [ 138.774040] r7 : b7e469b0 r6 : 7f0e6b2c r5 : afd91900 r4 : 7f1343c0
> [ 138.783571] r3 : aebec000 r2 : 00000001 r1 : b877ac00 r0 : 7f1343c0
> [ 138.793084] Flags: nzCv IRQs on FIQs on Mode SVC_32 ISA ARM Segment user
> [ 138.803300] Control: 10c5387d Table: 2d8f006a DAC: 00000055
> [ 138.812064] Process aplay (pid: 463, stack limit = 0xaebec210)
> [ 138.820868] Stack: (0xaebedd60 to 0xaebee000)
> [ 138.828207] dd60: 00000000 b848d000 afd91900 00000000 b7e469b0 7f0ed440 aebedda4 aebedd88
> [ 138.842371] dd80: 7f0e6b2c 801c4d30 afd91900 7f0ea4dc 00000000 b7e469b0 aebeddcc aebedda8
> [ 138.856611] dda0: 7f0e250c 7f0e6ae0 7f0e2464 b8478ec0 b7e469b0 afd91900 7f0ea388 00000000
> [ 138.870864] ddc0: aebeddf4 aebeddd0 802ce590 7f0e2470 8090ab64 afd91900 afd91900 b7e469b0
> [ 138.885301] dde0: afd91908 802ce4e4 aebede1c aebeddf8 802c57b4 802ce4f0 afd91900 aebedea8
> [ 138.900110] de00: b7fa4c00 00000000 00000000 00000004 aebede3c aebede20 802c6ba8 802c56b4
> [ 138.915260] de20: aebedea8 00000000 aebedf5c 00000000 aebedea4 aebede40 802d9a68 802c6b58
> [ 138.930661] de40: b874ddd0 00000000 00000000 00000001 00000041 00000000 afd91900 aebede70
> [ 138.946402] de60: 00000000 00000000 00000002 b7e469b0 b8a87610 b8d6ab80 801852f8 00080000
> [ 138.962314] de80: aebedf5c aebedea8 00000001 80108464 aebec000 00000000 aebedf4c aebedea8
> [ 138.978414] dea0: 802dacd4 802d970c b8a87610 b8d6ab80 a7982bc6 00000009 af363019 b9231480
> [ 138.994617] dec0: 00000000 b8c038a0 b7e469b0 00000101 00000002 00000238 00000000 00000000
> [ 139.010823] dee0: 00000000 aebedee8 00080000 0000000f aebedf3c aebedf00 802ed7e4 80843f94
> [ 139.027025] df00: 00000003 00080000 b9231490 b9231480 00000000 00080000 af363000 00000000
> [ 139.043229] df20: 00000005 00000002 ffffff9c 00000000 00080000 ffffff9c af363000 00000003
> [ 139.059430] df40: aebedf94 aebedf50 802c6f70 802dac70 aebec000 00000000 00000001 00000000
> [ 139.075629] df60: 00020000 00000004 00000100 00000001 7ebe577c 0002e038 00000000 00000005
> [ 139.091828] df80: 80108464 aebec000 aebedfa4 aebedf98 802c7060 802c6e6c 00000000 aebedfa8
> [ 139.108025] dfa0: 801082c0 802c7040 7ebe577c 0002e038 7ebe577c 00080000 00000b98 e81c8400
> [ 139.124222] dfc0: 7ebe577c 0002e038 00000000 00000005 7ebe57e4 00a20af8 7ebe57f0 76f87394
> [ 139.140419] dfe0: 00000000 7ebe55c4 76ec88e8 76df1d9c 60000010 7ebe577c 00000000 00000000
> [ 139.156715] [<801c4d5c>] (try_module_get) from [<7f0e6b2c>] (snd_ctl_open+0x58/0x194 [snd])
> [ 139.173222] [<7f0e6b2c>] (snd_ctl_open [snd]) from [<7f0e250c>] (snd_open+0xa8/0x14c [snd])
> [ 139.189683] [<7f0e250c>] (snd_open [snd]) from [<802ce590>] (chrdev_open+0xac/0x188)
> [ 139.205465] [<802ce590>] (chrdev_open) from [<802c57b4>] (do_dentry_open+0x10c/0x314)
> [ 139.221347] [<802c57b4>] (do_dentry_open) from [<802c6ba8>] (vfs_open+0x5c/0x88)
> [ 139.236788] [<802c6ba8>] (vfs_open) from [<802d9a68>] (path_openat+0x368/0x944)
> [ 139.248270] [<802d9a68>] (path_openat) from [<802dacd4>] (do_filp_open+0x70/0xc4)
> [ 139.263731] [<802dacd4>] (do_filp_open) from [<802c6f70>] (do_sys_open+0x110/0x1d4)
> [ 139.279378] [<802c6f70>] (do_sys_open) from [<802c7060>] (SyS_open+0x2c/0x30)
> [ 139.290647] [<802c7060>] (SyS_open) from [<801082c0>] (ret_fast_syscall+0x0/0x28)
> [ 139.306021] Code: e3c3303f e5932004 e2822001 e5832004 (e5943000)
> [ 139.316265] ---[ end trace 7f3f7f6193b663ed ]---
> [ 139.324956] note: aplay[463] exited with preempt_count 1
> ~~~~
>

FWIW,
Reviewed-by: Andy Shevchenko <[email protected]>

> Signed-off-by: Kirill Marinushkin <[email protected]>
> Cc: Eric Anholt <[email protected]>
> Cc: Stefan Wahren <[email protected]>
> Cc: Greg Kroah-Hartman <[email protected]>
> Cc: Florian Fainelli <[email protected]>
> Cc: Ray Jui <[email protected]>
> Cc: Scott Branden <[email protected]>
> Cc: [email protected]
> Cc: Michael Zoran <[email protected]>
> Cc: Andy Shevchenko <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> Cc: [email protected]
> Cc: [email protected]
> ---
> .../staging/vc04_services/bcm2835-audio/bcm2835.c | 54 ++++++++++------------
> 1 file changed, 25 insertions(+), 29 deletions(-)
>
> diff --git a/drivers/staging/vc04_services/bcm2835-audio/bcm2835.c b/drivers/staging/vc04_services/bcm2835-audio/bcm2835.c
> index 8f2d508183b2..9030d71a3d0b 100644
> --- a/drivers/staging/vc04_services/bcm2835-audio/bcm2835.c
> +++ b/drivers/staging/vc04_services/bcm2835-audio/bcm2835.c
> @@ -36,6 +36,10 @@ MODULE_PARM_DESC(enable_compat_alsa,
> static void snd_devm_unregister_child(struct device *dev, void *res)
> {
> struct device *childdev = *(struct device **)res;
> + struct bcm2835_chip *chip = dev_get_drvdata(childdev);
> + struct snd_card *card = chip->card;
> +
> + snd_card_free(card);
>
> device_unregister(childdev);
> }
> @@ -61,6 +65,13 @@ static int snd_devm_add_child(struct device *dev, struct device *child)
> return 0;
> }
>
> +static void snd_bcm2835_release(struct device *dev)
> +{
> + struct bcm2835_chip *chip = dev_get_drvdata(dev);
> +
> + kfree(chip);
> +}
> +
> static struct device *
> snd_create_device(struct device *parent,
> struct device_driver *driver,
> @@ -76,6 +87,7 @@ snd_create_device(struct device *parent,
> device_initialize(device);
> device->parent = parent;
> device->driver = driver;
> + device->release = snd_bcm2835_release;
>
> dev_set_name(device, "%s", name);
>
> @@ -86,18 +98,19 @@ snd_create_device(struct device *parent,
> return device;
> }
>
> -static int snd_bcm2835_free(struct bcm2835_chip *chip)
> -{
> - kfree(chip);
> - return 0;
> -}
> -
> /* component-destructor
> * (see "Management of Cards and Components")
> */
> static int snd_bcm2835_dev_free(struct snd_device *device)
> {
> - return snd_bcm2835_free(device->device_data);
> + struct bcm2835_chip *chip = device->device_data;
> + struct snd_card *card = chip->card;
> +
> + /* TODO: free pcm, ctl */
> +
> + snd_device_free(card, chip);
> +
> + return 0;
> }
>
> /* chip-specific constructor
> @@ -122,7 +135,7 @@ static int snd_bcm2835_create(struct snd_card *card,
>
> err = snd_device_new(card, SNDRV_DEV_LOWLEVEL, chip, &ops);
> if (err) {
> - snd_bcm2835_free(chip);
> + kfree(chip);
> return err;
> }
>
> @@ -130,31 +143,14 @@ static int snd_bcm2835_create(struct snd_card *card,
> return 0;
> }
>
> -static void snd_devm_card_free(struct device *dev, void *res)
> +static struct snd_card *snd_bcm2835_card_new(struct device *dev)
> {
> - struct snd_card *snd_card = *(struct snd_card **)res;
> -
> - snd_card_free(snd_card);
> -}
> -
> -static struct snd_card *snd_devm_card_new(struct device *dev)
> -{
> - struct snd_card **dr;
> struct snd_card *card;
> int ret;
>
> - dr = devres_alloc(snd_devm_card_free, sizeof(*dr), GFP_KERNEL);
> - if (!dr)
> - return ERR_PTR(-ENOMEM);
> -
> ret = snd_card_new(dev, -1, NULL, THIS_MODULE, 0, &card);
> - if (ret) {
> - devres_free(dr);
> + if (ret)
> return ERR_PTR(ret);
> - }
> -
> - *dr = card;
> - devres_add(dev, dr);
>
> return card;
> }
> @@ -271,7 +267,7 @@ static int snd_add_child_device(struct device *device,
> return PTR_ERR(child);
> }
>
> - card = snd_devm_card_new(child);
> + card = snd_bcm2835_card_new(child);
> if (IS_ERR(card)) {
> dev_err(child, "Failed to create card");
> return PTR_ERR(card);
> @@ -313,7 +309,7 @@ static int snd_add_child_device(struct device *device,
> return err;
> }
>
> - dev_set_drvdata(child, card);
> + dev_set_drvdata(child, chip);
> dev_info(child, "card created with %d channels\n", numchans);
>
> return 0;
> --
> 2.13.6
>



--
With Best Regards,
Andy Shevchenko

2018-03-25 10:46:07

by Kirill Marinushkin

Subject: Re: [PATCH v4] staging: bcm2835-audio: Release resources on module_exit()

On 03/25/18 12:33, Andy Shevchenko wrote:
> On Fri, Mar 23, 2018 at 9:22 PM, Kirill Marinushkin
> <[email protected]> wrote:
>> On 03/23/18 17:23, Andy Shevchenko wrote:
>>> After addressing above, FWIW,
>>>
>>> Reviewed-by: Andy Shevchenko <[email protected]>
> Seems you missed my tag in new version.
> When someone gives you a tag and you are going to send a new version
> (w/o drastic changes), it's your responsibility to append it
>
> I will send a new mail with it, so, this time no need to resend. Just
> keep it for the future contributions.
>

What is a "tag"?
I added in-reply-to this email, I added you as CC.
Isn't it enough to keep track of the versioning within a mailing list?

Could you please clarify: what "tag" should I usually attach in addition?

Best Regards,
Kirill

2018-03-25 13:04:58

by Andy Shevchenko

Subject: Re: [PATCH v4] staging: bcm2835-audio: Release resources on module_exit()

On Sun, Mar 25, 2018 at 1:44 PM, Kirill Marinushkin
<[email protected]> wrote:
> On 03/25/18 12:33, Andy Shevchenko wrote:
>> On Fri, Mar 23, 2018 at 9:22 PM, Kirill Marinushkin
>> <[email protected]> wrote:
>>> On 03/23/18 17:23, Andy Shevchenko wrote:
>>>> After addressing above, FWIW,
>>>>
>>>> Reviewed-by: Andy Shevchenko <[email protected]>
>> Seems you missed my tag in new version.
>> When someone gives you a tag and you are going to send a new version
>> (w/o drastic changes), it's your responsibility to append it
>>
>> I will send a new mail with it, so, this time no need to resend. Just
>> keep it for the future contributions.
>>
>
> What is a "tag"?
> I added in-reply-to this email, I added you as CC.
> Isn't it enough to keep track of the versioning within a mailing list?
>
> Could you please clarify: what "tag" should I usually attach in addition?

Section 13) in Submitting Patches [1] explains that.

[1]: https://elixir.bootlin.com/linux/v4.16-rc6/source/Documentation/process/submitting-patches.rst

--
With Best Regards,
Andy Shevchenko

2018-03-25 13:55:38

by Kirill Marinushkin

Subject: Re: [PATCH v4] staging: bcm2835-audio: Release resources on module_exit()

On 03/25/18 15:03, Andy Shevchenko wrote:
> On Sun, Mar 25, 2018 at 1:44 PM, Kirill Marinushkin
> <[email protected]> wrote:
>> On 03/25/18 12:33, Andy Shevchenko wrote:
>>> On Fri, Mar 23, 2018 at 9:22 PM, Kirill Marinushkin
>>> <[email protected]> wrote:
>>>> On 03/23/18 17:23, Andy Shevchenko wrote:
>>>>> After addressing above, FWIW,
>>>>>
>>>>> Reviewed-by: Andy Shevchenko <[email protected]>
>>> Seems you missed my tag in new version.
>>> When someone gives you a tag and you are going to send a new version
>>> (w/o drastic changes), it's your responsibility to append it
>>>
>>> I will send a new mail with it, so, this time no need to resend. Just
>>> keep it for the future contributions.
>>>
>> What is a "tag"?
>> I added in-reply-to this email, I added you as CC.
>> Isn't it enough to keep track of the versioning within a mailing list?
>>
>> Could you please clarify: what "tag" should I usually attach in addition?
> Section 13) in Submitting Patches [1] explains that.
>
> [1]: https://elixir.bootlin.com/linux/v4.16-rc6/source/Documentation/process/submitting-patches.rst
>

Thanks!