* [PATCH v3] staging: bcm2835-audio: Release resources on module_exit()
@ 2018-03-21 19:24 ` Andy Shevchenko
0 siblings, 0 replies; 42+ messages in thread
From: Andy Shevchenko @ 2018-03-21 19:24 UTC (permalink / raw)
To: linux-arm-kernel
On Wed, Mar 21, 2018 at 8:48 PM, Kirill Marinushkin
<k.marinushkin@gmail.com> 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 <andy.shevchenko@gmail.com>
P.S. I didn't get why do you need an empty ->remove() stub. Doesn't
work without it?
> Signed-off-by: Kirill Marinushkin <k.marinushkin@gmail.com>
> Cc: Eric Anholt <eric@anholt.net>
> Cc: Stefan Wahren <stefan.wahren@i2se.com>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Florian Fainelli <f.fainelli@gmail.com>
> Cc: Ray Jui <rjui@broadcom.com>
> Cc: Scott Branden <sbranden@broadcom.com>
> Cc: bcm-kernel-feedback-list at broadcom.com
> Cc: Michael Zoran <mzoran@crowfest.net>
> Cc: linux-rpi-kernel at lists.infradead.org
> Cc: linux-arm-kernel at lists.infradead.org
> Cc: devel at driverdev.osuosl.org
> Cc: linux-kernel at vger.kernel.org
> ---
> .../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
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH v3] staging: bcm2835-audio: Release resources on module_exit()
2018-03-21 19:24 ` Andy Shevchenko
@ 2018-03-21 19:51 ` Kirill Marinushkin
-1 siblings, 0 replies; 42+ messages in thread
From: Kirill Marinushkin @ 2018-03-21 19:51 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Stefan Wahren, devel, Florian Fainelli, Scott Branden, Ray Jui,
Linux Kernel Mailing List, Eric Anholt, Michael Zoran,
bcm-kernel-feedback-list, linux-rpi-kernel, Greg Kroah-Hartman,
linux-arm Mailing List
On 03/21/18 20:24, Andy Shevchenko wrote:
> On Wed, Mar 21, 2018 at 8:48 PM, Kirill Marinushkin
> <k.marinushkin@gmail.com> 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 <andy.shevchenko@gmail.com>
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 <k.marinushkin@gmail.com>
>> Cc: Eric Anholt <eric@anholt.net>
>> Cc: Stefan Wahren <stefan.wahren@i2se.com>
>> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>> Cc: Florian Fainelli <f.fainelli@gmail.com>
>> Cc: Ray Jui <rjui@broadcom.com>
>> Cc: Scott Branden <sbranden@broadcom.com>
>> Cc: bcm-kernel-feedback-list@broadcom.com
>> Cc: Michael Zoran <mzoran@crowfest.net>
>> Cc: linux-rpi-kernel@lists.infradead.org
>> Cc: linux-arm-kernel@lists.infradead.org
>> Cc: devel@driverdev.osuosl.org
>> Cc: linux-kernel@vger.kernel.org
>> ---
>> .../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
>>
>
>
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
^ permalink raw reply [flat|nested] 42+ messages in thread
* [PATCH v3] staging: bcm2835-audio: Release resources on module_exit()
@ 2018-03-21 19:51 ` Kirill Marinushkin
0 siblings, 0 replies; 42+ messages in thread
From: Kirill Marinushkin @ 2018-03-21 19:51 UTC (permalink / raw)
To: linux-arm-kernel
On 03/21/18 20:24, Andy Shevchenko wrote:
> On Wed, Mar 21, 2018 at 8:48 PM, Kirill Marinushkin
> <k.marinushkin@gmail.com> 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 <andy.shevchenko@gmail.com>
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 <k.marinushkin@gmail.com>
>> Cc: Eric Anholt <eric@anholt.net>
>> Cc: Stefan Wahren <stefan.wahren@i2se.com>
>> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>> Cc: Florian Fainelli <f.fainelli@gmail.com>
>> Cc: Ray Jui <rjui@broadcom.com>
>> Cc: Scott Branden <sbranden@broadcom.com>
>> Cc: bcm-kernel-feedback-list at broadcom.com
>> Cc: Michael Zoran <mzoran@crowfest.net>
>> Cc: linux-rpi-kernel at lists.infradead.org
>> Cc: linux-arm-kernel at lists.infradead.org
>> Cc: devel at driverdev.osuosl.org
>> Cc: linux-kernel at vger.kernel.org
>> ---
>> .../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
>>
>
>
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH v3] staging: bcm2835-audio: Release resources on module_exit()
2018-03-21 19:24 ` Andy Shevchenko
@ 2018-03-22 21:21 ` Kirill Marinushkin
-1 siblings, 0 replies; 42+ messages in thread
From: Kirill Marinushkin @ 2018-03-22 21:21 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Stefan Wahren, devel, Florian Fainelli, Scott Branden, Ray Jui,
Linux Kernel Mailing List, Eric Anholt, Michael Zoran,
bcm-kernel-feedback-list, linux-rpi-kernel, Greg Kroah-Hartman,
linux-arm Mailing List
On 03/21/18 20:24, Andy Shevchenko wrote:
> On Wed, Mar 21, 2018 at 8:48 PM, Kirill Marinushkin
> <k.marinushkin@gmail.com> 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 <andy.shevchenko@gmail.com>
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 <k.marinushkin@gmail.com>
>> Cc: Eric Anholt <eric@anholt.net>
>> Cc: Stefan Wahren <stefan.wahren@i2se.com>
>> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>> Cc: Florian Fainelli <f.fainelli@gmail.com>
>> Cc: Ray Jui <rjui@broadcom.com>
>> Cc: Scott Branden <sbranden@broadcom.com>
>> Cc: bcm-kernel-feedback-list@broadcom.com
>> Cc: Michael Zoran <mzoran@crowfest.net>
>> Cc: linux-rpi-kernel@lists.infradead.org
>> Cc: linux-arm-kernel@lists.infradead.org
>> Cc: devel@driverdev.osuosl.org
>> Cc: linux-kernel@vger.kernel.org
>> ---
>> .../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
>>
>
>
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
^ permalink raw reply [flat|nested] 42+ messages in thread
* [PATCH v3] staging: bcm2835-audio: Release resources on module_exit()
@ 2018-03-22 21:21 ` Kirill Marinushkin
0 siblings, 0 replies; 42+ messages in thread
From: Kirill Marinushkin @ 2018-03-22 21:21 UTC (permalink / raw)
To: linux-arm-kernel
On 03/21/18 20:24, Andy Shevchenko wrote:
> On Wed, Mar 21, 2018 at 8:48 PM, Kirill Marinushkin
> <k.marinushkin@gmail.com> 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 <andy.shevchenko@gmail.com>
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 <k.marinushkin@gmail.com>
>> Cc: Eric Anholt <eric@anholt.net>
>> Cc: Stefan Wahren <stefan.wahren@i2se.com>
>> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>> Cc: Florian Fainelli <f.fainelli@gmail.com>
>> Cc: Ray Jui <rjui@broadcom.com>
>> Cc: Scott Branden <sbranden@broadcom.com>
>> Cc: bcm-kernel-feedback-list at broadcom.com
>> Cc: Michael Zoran <mzoran@crowfest.net>
>> Cc: linux-rpi-kernel at lists.infradead.org
>> Cc: linux-arm-kernel at lists.infradead.org
>> Cc: devel at driverdev.osuosl.org
>> Cc: linux-kernel at vger.kernel.org
>> ---
>> .../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
>>
>
>
^ permalink raw reply [flat|nested] 42+ messages in thread
* [PATCH v4] staging: bcm2835-audio: Release resources on module_exit()
2018-03-22 21:21 ` Kirill Marinushkin
@ 2018-03-22 21:37 ` Kirill Marinushkin
-1 siblings, 0 replies; 42+ messages in thread
From: Kirill Marinushkin @ 2018-03-22 21:37 UTC (permalink / raw)
To: Andy Shevchenko, Greg Kroah-Hartman, Eric Anholt, Stefan Wahren,
Florian Fainelli, Ray Jui, Scott Branden,
bcm-kernel-feedback-list, Michael Zoran
Cc: Kirill Marinushkin, linux-rpi-kernel, linux-arm-kernel, devel,
linux-kernel
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 <k.marinushkin@gmail.com>
Cc: Eric Anholt <eric@anholt.net>
Cc: Stefan Wahren <stefan.wahren@i2se.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Florian Fainelli <f.fainelli@gmail.com>
Cc: Ray Jui <rjui@broadcom.com>
Cc: Scott Branden <sbranden@broadcom.com>
Cc: bcm-kernel-feedback-list@broadcom.com
Cc: Michael Zoran <mzoran@crowfest.net>
Cc: Andy Shevchenko <andy.shevchenko@gmail.com>
Cc: linux-rpi-kernel@lists.infradead.org
Cc: linux-arm-kernel@lists.infradead.org
Cc: devel@driverdev.osuosl.org
Cc: linux-kernel@vger.kernel.org
---
.../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
^ permalink raw reply related [flat|nested] 42+ messages in thread
* [PATCH v4] staging: bcm2835-audio: Release resources on module_exit()
@ 2018-03-22 21:37 ` Kirill Marinushkin
0 siblings, 0 replies; 42+ messages in thread
From: Kirill Marinushkin @ 2018-03-22 21:37 UTC (permalink / raw)
To: linux-arm-kernel
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 <k.marinushkin@gmail.com>
Cc: Eric Anholt <eric@anholt.net>
Cc: Stefan Wahren <stefan.wahren@i2se.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Florian Fainelli <f.fainelli@gmail.com>
Cc: Ray Jui <rjui@broadcom.com>
Cc: Scott Branden <sbranden@broadcom.com>
Cc: bcm-kernel-feedback-list at broadcom.com
Cc: Michael Zoran <mzoran@crowfest.net>
Cc: Andy Shevchenko <andy.shevchenko@gmail.com>
Cc: linux-rpi-kernel at lists.infradead.org
Cc: linux-arm-kernel at lists.infradead.org
Cc: devel at driverdev.osuosl.org
Cc: linux-kernel at vger.kernel.org
---
.../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
^ permalink raw reply related [flat|nested] 42+ messages in thread
* Re: [PATCH v4] staging: bcm2835-audio: Release resources on module_exit()
2018-03-22 21:37 ` Kirill Marinushkin
@ 2018-03-23 16:23 ` Andy Shevchenko
-1 siblings, 0 replies; 42+ messages in thread
From: Andy Shevchenko @ 2018-03-23 16:23 UTC (permalink / raw)
To: Kirill Marinushkin
Cc: Stefan Wahren, devel, Florian Fainelli, Scott Branden,
Greg Kroah-Hartman, Linux Kernel Mailing List, Eric Anholt,
Michael Zoran, bcm-kernel-feedback-list, linux-rpi-kernel,
Ray Jui, linux-arm Mailing List
On Thu, Mar 22, 2018 at 11:37 PM, Kirill Marinushkin
<k.marinushkin@gmail.com> 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 <andy.shevchenko@gmail.com>
--
With Best Regards,
Andy Shevchenko
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
^ permalink raw reply [flat|nested] 42+ messages in thread
* [PATCH v4] staging: bcm2835-audio: Release resources on module_exit()
@ 2018-03-23 16:23 ` Andy Shevchenko
0 siblings, 0 replies; 42+ messages in thread
From: Andy Shevchenko @ 2018-03-23 16:23 UTC (permalink / raw)
To: linux-arm-kernel
On Thu, Mar 22, 2018 at 11:37 PM, Kirill Marinushkin
<k.marinushkin@gmail.com> 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 <andy.shevchenko@gmail.com>
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH v4] staging: bcm2835-audio: Release resources on module_exit()
2018-03-23 16:23 ` Andy Shevchenko
@ 2018-03-23 19:22 ` Kirill Marinushkin
-1 siblings, 0 replies; 42+ messages in thread
From: Kirill Marinushkin @ 2018-03-23 19:22 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Stefan Wahren, devel, Florian Fainelli, Scott Branden,
Greg Kroah-Hartman, Linux Kernel Mailing List, Eric Anholt,
Michael Zoran, bcm-kernel-feedback-list, linux-rpi-kernel,
Ray Jui, linux-arm Mailing List
On 03/23/18 17:23, Andy Shevchenko wrote:
> On Thu, Mar 22, 2018 at 11:37 PM, Kirill Marinushkin
> <k.marinushkin@gmail.com> 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 <andy.shevchenko@gmail.com>
>
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
^ permalink raw reply [flat|nested] 42+ messages in thread
* [PATCH v4] staging: bcm2835-audio: Release resources on module_exit()
@ 2018-03-23 19:22 ` Kirill Marinushkin
0 siblings, 0 replies; 42+ messages in thread
From: Kirill Marinushkin @ 2018-03-23 19:22 UTC (permalink / raw)
To: linux-arm-kernel
On 03/23/18 17:23, Andy Shevchenko wrote:
> On Thu, Mar 22, 2018 at 11:37 PM, Kirill Marinushkin
> <k.marinushkin@gmail.com> 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 <andy.shevchenko@gmail.com>
>
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH v4] staging: bcm2835-audio: Release resources on module_exit()
2018-03-23 19:22 ` Kirill Marinushkin
@ 2018-03-25 10:33 ` Andy Shevchenko
-1 siblings, 0 replies; 42+ messages in thread
From: Andy Shevchenko @ 2018-03-25 10:33 UTC (permalink / raw)
To: Kirill Marinushkin
Cc: Stefan Wahren, devel, Florian Fainelli, Scott Branden,
Greg Kroah-Hartman, Linux Kernel Mailing List, Eric Anholt,
Michael Zoran, bcm-kernel-feedback-list, linux-rpi-kernel,
Ray Jui, linux-arm Mailing List
On Fri, Mar 23, 2018 at 9:22 PM, Kirill Marinushkin
<k.marinushkin@gmail.com> wrote:
> On 03/23/18 17:23, Andy Shevchenko wrote:
>> After addressing above, FWIW,
>>
>> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
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
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
^ permalink raw reply [flat|nested] 42+ messages in thread
* [PATCH v4] staging: bcm2835-audio: Release resources on module_exit()
@ 2018-03-25 10:33 ` Andy Shevchenko
0 siblings, 0 replies; 42+ messages in thread
From: Andy Shevchenko @ 2018-03-25 10:33 UTC (permalink / raw)
To: linux-arm-kernel
On Fri, Mar 23, 2018 at 9:22 PM, Kirill Marinushkin
<k.marinushkin@gmail.com> wrote:
> On 03/23/18 17:23, Andy Shevchenko wrote:
>> After addressing above, FWIW,
>>
>> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
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
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH v4] staging: bcm2835-audio: Release resources on module_exit()
2018-03-25 10:33 ` Andy Shevchenko
@ 2018-03-25 10:44 ` Kirill Marinushkin
-1 siblings, 0 replies; 42+ messages in thread
From: Kirill Marinushkin @ 2018-03-25 10:44 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Stefan Wahren, devel, Florian Fainelli, Scott Branden,
Greg Kroah-Hartman, Linux Kernel Mailing List, Eric Anholt,
Michael Zoran, bcm-kernel-feedback-list, linux-rpi-kernel,
Ray Jui, linux-arm Mailing List
On 03/25/18 12:33, Andy Shevchenko wrote:
> On Fri, Mar 23, 2018 at 9:22 PM, Kirill Marinushkin
> <k.marinushkin@gmail.com> wrote:
>> On 03/23/18 17:23, Andy Shevchenko wrote:
>>> After addressing above, FWIW,
>>>
>>> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> 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
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
^ permalink raw reply [flat|nested] 42+ messages in thread
* [PATCH v4] staging: bcm2835-audio: Release resources on module_exit()
@ 2018-03-25 10:44 ` Kirill Marinushkin
0 siblings, 0 replies; 42+ messages in thread
From: Kirill Marinushkin @ 2018-03-25 10:44 UTC (permalink / raw)
To: linux-arm-kernel
On 03/25/18 12:33, Andy Shevchenko wrote:
> On Fri, Mar 23, 2018 at 9:22 PM, Kirill Marinushkin
> <k.marinushkin@gmail.com> wrote:
>> On 03/23/18 17:23, Andy Shevchenko wrote:
>>> After addressing above, FWIW,
>>>
>>> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> 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
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH v4] staging: bcm2835-audio: Release resources on module_exit()
2018-03-25 10:44 ` Kirill Marinushkin
@ 2018-03-25 13:03 ` Andy Shevchenko
-1 siblings, 0 replies; 42+ messages in thread
From: Andy Shevchenko @ 2018-03-25 13:03 UTC (permalink / raw)
To: Kirill Marinushkin
Cc: Greg Kroah-Hartman, Eric Anholt, Stefan Wahren, Florian Fainelli,
Ray Jui, Scott Branden, bcm-kernel-feedback-list, Michael Zoran,
linux-rpi-kernel, linux-arm Mailing List, devel,
Linux Kernel Mailing List
On Sun, Mar 25, 2018 at 1:44 PM, Kirill Marinushkin
<k.marinushkin@gmail.com> wrote:
> On 03/25/18 12:33, Andy Shevchenko wrote:
>> On Fri, Mar 23, 2018 at 9:22 PM, Kirill Marinushkin
>> <k.marinushkin@gmail.com> wrote:
>>> On 03/23/18 17:23, Andy Shevchenko wrote:
>>>> After addressing above, FWIW,
>>>>
>>>> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
>> 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
^ permalink raw reply [flat|nested] 42+ messages in thread
* [PATCH v4] staging: bcm2835-audio: Release resources on module_exit()
@ 2018-03-25 13:03 ` Andy Shevchenko
0 siblings, 0 replies; 42+ messages in thread
From: Andy Shevchenko @ 2018-03-25 13:03 UTC (permalink / raw)
To: linux-arm-kernel
On Sun, Mar 25, 2018 at 1:44 PM, Kirill Marinushkin
<k.marinushkin@gmail.com> wrote:
> On 03/25/18 12:33, Andy Shevchenko wrote:
>> On Fri, Mar 23, 2018 at 9:22 PM, Kirill Marinushkin
>> <k.marinushkin@gmail.com> wrote:
>>> On 03/23/18 17:23, Andy Shevchenko wrote:
>>>> After addressing above, FWIW,
>>>>
>>>> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
>> 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
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH v4] staging: bcm2835-audio: Release resources on module_exit()
2018-03-25 13:03 ` Andy Shevchenko
@ 2018-03-25 13:54 ` Kirill Marinushkin
-1 siblings, 0 replies; 42+ messages in thread
From: Kirill Marinushkin @ 2018-03-25 13:54 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Greg Kroah-Hartman, Eric Anholt, Stefan Wahren, Florian Fainelli,
Ray Jui, Scott Branden, bcm-kernel-feedback-list, Michael Zoran,
linux-rpi-kernel, linux-arm Mailing List, devel,
Linux Kernel Mailing List
On 03/25/18 15:03, Andy Shevchenko wrote:
> On Sun, Mar 25, 2018 at 1:44 PM, Kirill Marinushkin
> <k.marinushkin@gmail.com> wrote:
>> On 03/25/18 12:33, Andy Shevchenko wrote:
>>> On Fri, Mar 23, 2018 at 9:22 PM, Kirill Marinushkin
>>> <k.marinushkin@gmail.com> wrote:
>>>> On 03/23/18 17:23, Andy Shevchenko wrote:
>>>>> After addressing above, FWIW,
>>>>>
>>>>> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
>>> 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!
^ permalink raw reply [flat|nested] 42+ messages in thread
* [PATCH v4] staging: bcm2835-audio: Release resources on module_exit()
@ 2018-03-25 13:54 ` Kirill Marinushkin
0 siblings, 0 replies; 42+ messages in thread
From: Kirill Marinushkin @ 2018-03-25 13:54 UTC (permalink / raw)
To: linux-arm-kernel
On 03/25/18 15:03, Andy Shevchenko wrote:
> On Sun, Mar 25, 2018 at 1:44 PM, Kirill Marinushkin
> <k.marinushkin@gmail.com> wrote:
>> On 03/25/18 12:33, Andy Shevchenko wrote:
>>> On Fri, Mar 23, 2018 at 9:22 PM, Kirill Marinushkin
>>> <k.marinushkin@gmail.com> wrote:
>>>> On 03/23/18 17:23, Andy Shevchenko wrote:
>>>>> After addressing above, FWIW,
>>>>>
>>>>> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
>>> 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!
^ permalink raw reply [flat|nested] 42+ messages in thread
* [PATCH v5] staging: bcm2835-audio: Release resources on module_exit()
2018-03-23 16:23 ` Andy Shevchenko
@ 2018-03-23 19:32 ` Kirill Marinushkin
-1 siblings, 0 replies; 42+ messages in thread
From: Kirill Marinushkin @ 2018-03-23 19:32 UTC (permalink / raw)
To: Andy Shevchenko, Greg Kroah-Hartman, Eric Anholt, Stefan Wahren,
Florian Fainelli, Ray Jui, Scott Branden,
bcm-kernel-feedback-list, Michael Zoran
Cc: linux-arm-kernel, devel, linux-rpi-kernel, Kirill Marinushkin,
linux-kernel
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 <k.marinushkin@gmail.com>
Cc: Eric Anholt <eric@anholt.net>
Cc: Stefan Wahren <stefan.wahren@i2se.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Florian Fainelli <f.fainelli@gmail.com>
Cc: Ray Jui <rjui@broadcom.com>
Cc: Scott Branden <sbranden@broadcom.com>
Cc: bcm-kernel-feedback-list@broadcom.com
Cc: Michael Zoran <mzoran@crowfest.net>
Cc: Andy Shevchenko <andy.shevchenko@gmail.com>
Cc: linux-rpi-kernel@lists.infradead.org
Cc: linux-arm-kernel@lists.infradead.org
Cc: devel@driverdev.osuosl.org
Cc: linux-kernel@vger.kernel.org
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
---
.../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
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
^ permalink raw reply related [flat|nested] 42+ messages in thread
* [PATCH v5] staging: bcm2835-audio: Release resources on module_exit()
@ 2018-03-23 19:32 ` Kirill Marinushkin
0 siblings, 0 replies; 42+ messages in thread
From: Kirill Marinushkin @ 2018-03-23 19:32 UTC (permalink / raw)
To: linux-arm-kernel
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 <k.marinushkin@gmail.com>
Cc: Eric Anholt <eric@anholt.net>
Cc: Stefan Wahren <stefan.wahren@i2se.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Florian Fainelli <f.fainelli@gmail.com>
Cc: Ray Jui <rjui@broadcom.com>
Cc: Scott Branden <sbranden@broadcom.com>
Cc: bcm-kernel-feedback-list at broadcom.com
Cc: Michael Zoran <mzoran@crowfest.net>
Cc: Andy Shevchenko <andy.shevchenko@gmail.com>
Cc: linux-rpi-kernel at lists.infradead.org
Cc: linux-arm-kernel at lists.infradead.org
Cc: devel at driverdev.osuosl.org
Cc: linux-kernel at vger.kernel.org
---
.../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
^ permalink raw reply related [flat|nested] 42+ messages in thread
* Re: [PATCH v5] staging: bcm2835-audio: Release resources on module_exit()
2018-03-23 19:32 ` Kirill Marinushkin
@ 2018-03-25 10:33 ` Andy Shevchenko
-1 siblings, 0 replies; 42+ messages in thread
From: Andy Shevchenko @ 2018-03-25 10:33 UTC (permalink / raw)
To: Kirill Marinushkin
Cc: Stefan Wahren, devel, Florian Fainelli, Scott Branden,
Greg Kroah-Hartman, Linux Kernel Mailing List, Eric Anholt,
Michael Zoran, bcm-kernel-feedback-list, linux-rpi-kernel,
Ray Jui, linux-arm Mailing List
On Fri, Mar 23, 2018 at 9:32 PM, Kirill Marinushkin
<k.marinushkin@gmail.com> 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 <andy.shevchenko@gmail.com>
> Signed-off-by: Kirill Marinushkin <k.marinushkin@gmail.com>
> Cc: Eric Anholt <eric@anholt.net>
> Cc: Stefan Wahren <stefan.wahren@i2se.com>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Florian Fainelli <f.fainelli@gmail.com>
> Cc: Ray Jui <rjui@broadcom.com>
> Cc: Scott Branden <sbranden@broadcom.com>
> Cc: bcm-kernel-feedback-list@broadcom.com
> Cc: Michael Zoran <mzoran@crowfest.net>
> Cc: Andy Shevchenko <andy.shevchenko@gmail.com>
> Cc: linux-rpi-kernel@lists.infradead.org
> Cc: linux-arm-kernel@lists.infradead.org
> Cc: devel@driverdev.osuosl.org
> Cc: linux-kernel@vger.kernel.org
> ---
> .../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
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
^ permalink raw reply [flat|nested] 42+ messages in thread
* [PATCH v5] staging: bcm2835-audio: Release resources on module_exit()
@ 2018-03-25 10:33 ` Andy Shevchenko
0 siblings, 0 replies; 42+ messages in thread
From: Andy Shevchenko @ 2018-03-25 10:33 UTC (permalink / raw)
To: linux-arm-kernel
On Fri, Mar 23, 2018 at 9:32 PM, Kirill Marinushkin
<k.marinushkin@gmail.com> 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 <andy.shevchenko@gmail.com>
> Signed-off-by: Kirill Marinushkin <k.marinushkin@gmail.com>
> Cc: Eric Anholt <eric@anholt.net>
> Cc: Stefan Wahren <stefan.wahren@i2se.com>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Florian Fainelli <f.fainelli@gmail.com>
> Cc: Ray Jui <rjui@broadcom.com>
> Cc: Scott Branden <sbranden@broadcom.com>
> Cc: bcm-kernel-feedback-list at broadcom.com
> Cc: Michael Zoran <mzoran@crowfest.net>
> Cc: Andy Shevchenko <andy.shevchenko@gmail.com>
> Cc: linux-rpi-kernel at lists.infradead.org
> Cc: linux-arm-kernel at lists.infradead.org
> Cc: devel at driverdev.osuosl.org
> Cc: linux-kernel at vger.kernel.org
> ---
> .../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
^ permalink raw reply [flat|nested] 42+ messages in thread