[alsa-devel] [PATCH 0/5] Convert to use devm_*() for amba attached modules
When review device driver modules which attach to amba bus, found several modules are not using devm_*() apis to manage resource. As result, some drivers have memory leakage or missing iomem unmapping when rmmod module. And the code has many "goto" tags to handle different failures.
So this patch series is to convert to use devm_*() for moudules which are attached to amba bus to manage resource and get more robust and neat code.
Patch 0003 "drivers/rtc/rtc-pl031.c: Convert to use devm_*()" has been verified on 96boards Hikey. Other patches can pass building but have not really tested on hardware.
Leo Yan (5): Input: ambakmi - Convert to use devm_*() drivers/rtc/rtc-pl030.c: Convert to use devm_*() drivers/rtc/rtc-pl031.c: Convert to use devm_*() vfio: platform: Convert to use devm_*() ALSA: AACI: Convert to use devm_ioremap_resource()
drivers/input/serio/ambakmi.c | 44 +++++++---------------------- drivers/rtc/rtc-pl030.c | 49 ++++++++------------------------ drivers/rtc/rtc-pl031.c | 59 ++++++++++----------------------------- drivers/vfio/platform/vfio_amba.c | 25 ++++++----------- sound/arm/aaci.c | 21 ++++---------- 5 files changed, 51 insertions(+), 147 deletions(-)
Convert driver to use devm_*() APIs so rely on driver model core layer to manage resources. This eliminates error path boilerplate and makes code neat.
This patch also fixes the memory leakage for 'kmi->io' when remove module.
Signed-off-by: Leo Yan leo.yan@linaro.org --- drivers/input/serio/ambakmi.c | 44 ++++++++++--------------------------------- 1 file changed, 10 insertions(+), 34 deletions(-)
diff --git a/drivers/input/serio/ambakmi.c b/drivers/input/serio/ambakmi.c index c6606ca..d4814be 100644 --- a/drivers/input/serio/ambakmi.c +++ b/drivers/input/serio/ambakmi.c @@ -112,19 +112,11 @@ static int amba_kmi_probe(struct amba_device *dev, { struct amba_kmi_port *kmi; struct serio *io; - int ret; - - ret = amba_request_regions(dev, NULL); - if (ret) - return ret; - - kmi = kzalloc(sizeof(struct amba_kmi_port), GFP_KERNEL); - io = kzalloc(sizeof(struct serio), GFP_KERNEL); - if (!kmi || !io) { - ret = -ENOMEM; - goto out; - }
+ kmi = devm_kzalloc(&dev->dev, sizeof(*kmi), GFP_KERNEL); + io = devm_kzalloc(&dev->dev, sizeof(*io), GFP_KERNEL); + if (!kmi || !io) + return -ENOMEM;
io->id.type = SERIO_8042; io->write = amba_kmi_write; @@ -136,31 +128,19 @@ static int amba_kmi_probe(struct amba_device *dev, io->dev.parent = &dev->dev;
kmi->io = io; - kmi->base = ioremap(dev->res.start, resource_size(&dev->res)); - if (!kmi->base) { - ret = -ENOMEM; - goto out; - } + kmi->base = devm_ioremap_resource(&dev->dev, &dev->res); + if (IS_ERR(kmi->base)) + return PTR_ERR(kmi->base);
- kmi->clk = clk_get(&dev->dev, "KMIREFCLK"); - if (IS_ERR(kmi->clk)) { - ret = PTR_ERR(kmi->clk); - goto unmap; - } + kmi->clk = devm_clk_get(&dev->dev, "KMIREFCLK"); + if (IS_ERR(kmi->clk)) + return PTR_ERR(kmi->clk);
kmi->irq = dev->irq[0]; amba_set_drvdata(dev, kmi);
serio_register_port(kmi->io); return 0; - - unmap: - iounmap(kmi->base); - out: - kfree(kmi); - kfree(io); - amba_release_regions(dev); - return ret; }
static int amba_kmi_remove(struct amba_device *dev) @@ -168,10 +148,6 @@ static int amba_kmi_remove(struct amba_device *dev) struct amba_kmi_port *kmi = amba_get_drvdata(dev);
serio_unregister_port(kmi->io); - clk_put(kmi->clk); - iounmap(kmi->base); - kfree(kmi); - amba_release_regions(dev); return 0; }
On Sun, Mar 26, 2017 at 10:41:50PM +0800, Leo Yan wrote:
Convert driver to use devm_*() APIs so rely on driver model core layer to manage resources. This eliminates error path boilerplate and makes code neat.
This patch also fixes the memory leakage for 'kmi->io' when remove module.
No, it is not leaked, in fact, your patch introduces a use-after-free bug.
kmi->io is of type "struct serio", and this structure has an embedded "struct device". The lifetime of any structure containing a "struct device" is controlled by the lifetime of the struct device.
Once serio_register_port() has been called on it, it is up to the serio layer to free this structure.
Therefore, your patch creates a bug, and so it gets a NAK. There is no resource leakage here.
On Sun, Mar 26, 2017 at 03:50:24PM +0100, Russell King - ARM Linux wrote:
On Sun, Mar 26, 2017 at 10:41:50PM +0800, Leo Yan wrote:
Convert driver to use devm_*() APIs so rely on driver model core layer to manage resources. This eliminates error path boilerplate and makes code neat.
This patch also fixes the memory leakage for 'kmi->io' when remove module.
No, it is not leaked, in fact, your patch introduces a use-after-free bug.
kmi->io is of type "struct serio", and this structure has an embedded "struct device". The lifetime of any structure containing a "struct device" is controlled by the lifetime of the struct device.
Once serio_register_port() has been called on it, it is up to the serio layer to free this structure.
Therefore, your patch creates a bug, and so it gets a NAK. There is no resource leakage here.
Thanks for reviewing. Now I understood and please ignore this patch and patch 0005.
Thanks, Leo Yan
Convert driver to use devm_*() APIs so rely on driver model core layer to manage resources. This eliminates error path boilerplate and makes code neat.
Signed-off-by: Leo Yan leo.yan@linaro.org --- drivers/rtc/rtc-pl030.c | 49 ++++++++++++------------------------------------- 1 file changed, 12 insertions(+), 37 deletions(-)
diff --git a/drivers/rtc/rtc-pl030.c b/drivers/rtc/rtc-pl030.c index f85a1a9..372b1fd 100644 --- a/drivers/rtc/rtc-pl030.c +++ b/drivers/rtc/rtc-pl030.c @@ -102,49 +102,30 @@ static int pl030_probe(struct amba_device *dev, const struct amba_id *id) struct pl030_rtc *rtc; int ret;
- ret = amba_request_regions(dev, NULL); - if (ret) - goto err_req; - rtc = devm_kzalloc(&dev->dev, sizeof(*rtc), GFP_KERNEL); - if (!rtc) { - ret = -ENOMEM; - goto err_rtc; - } + if (!rtc) + return -ENOMEM;
- rtc->base = ioremap(dev->res.start, resource_size(&dev->res)); - if (!rtc->base) { - ret = -ENOMEM; - goto err_rtc; - } + rtc->base = devm_ioremap_resource(&dev->dev, &dev->res); + if (IS_ERR(rtc->base)) + return PTR_ERR(rtc->base);
__raw_writel(0, rtc->base + RTC_CR); __raw_writel(0, rtc->base + RTC_EOI);
amba_set_drvdata(dev, rtc);
- ret = request_irq(dev->irq[0], pl030_interrupt, 0, - "rtc-pl030", rtc); + ret = devm_request_irq(&dev->dev, dev->irq[0], pl030_interrupt, 0, + "rtc-pl030", rtc); if (ret) - goto err_irq; + return ret;
- rtc->rtc = rtc_device_register("pl030", &dev->dev, &pl030_ops, - THIS_MODULE); - if (IS_ERR(rtc->rtc)) { - ret = PTR_ERR(rtc->rtc); - goto err_reg; - } + rtc->rtc = devm_rtc_device_register(&dev->dev, "pl030", &pl030_ops, + THIS_MODULE); + if (IS_ERR(rtc->rtc)) + return PTR_ERR(rtc->rtc);
return 0; - - err_reg: - free_irq(dev->irq[0], rtc); - err_irq: - iounmap(rtc->base); - err_rtc: - amba_release_regions(dev); - err_req: - return ret; }
static int pl030_remove(struct amba_device *dev) @@ -152,12 +133,6 @@ static int pl030_remove(struct amba_device *dev) struct pl030_rtc *rtc = amba_get_drvdata(dev);
writel(0, rtc->base + RTC_CR); - - free_irq(dev->irq[0], rtc); - rtc_device_unregister(rtc->rtc); - iounmap(rtc->base); - amba_release_regions(dev); - return 0; }
On Sun, Mar 26, 2017 at 4:41 PM, Leo Yan leo.yan@linaro.org wrote:
Convert driver to use devm_*() APIs so rely on driver model core layer to manage resources. This eliminates error path boilerplate and makes code neat.
Signed-off-by: Leo Yan leo.yan@linaro.org
Acked-by: Linus Walleij linus.walleij@linaro.org
Yours, Linus Walleij
Convert driver to use devm_*() APIs so rely on driver model core layer to manage resources. This eliminates error path boilerplate and makes code neat.
Signed-off-by: Leo Yan leo.yan@linaro.org --- drivers/rtc/rtc-pl031.c | 59 +++++++++++++------------------------------------ 1 file changed, 15 insertions(+), 44 deletions(-)
diff --git a/drivers/rtc/rtc-pl031.c b/drivers/rtc/rtc-pl031.c index e1687e1..1699a17 100644 --- a/drivers/rtc/rtc-pl031.c +++ b/drivers/rtc/rtc-pl031.c @@ -304,15 +304,8 @@ static int pl031_set_alarm(struct device *dev, struct rtc_wkalrm *alarm)
static int pl031_remove(struct amba_device *adev) { - struct pl031_local *ldata = dev_get_drvdata(&adev->dev); - dev_pm_clear_wake_irq(&adev->dev); device_init_wakeup(&adev->dev, false); - free_irq(adev->irq[0], ldata); - rtc_device_unregister(ldata->rtc); - iounmap(ldata->base); - kfree(ldata); - amba_release_regions(adev);
return 0; } @@ -325,23 +318,15 @@ static int pl031_probe(struct amba_device *adev, const struct amba_id *id) struct rtc_class_ops *ops = &vendor->ops; unsigned long time, data;
- ret = amba_request_regions(adev, NULL); - if (ret) - goto err_req; + ldata = devm_kzalloc(&adev->dev, sizeof(*ldata), GFP_KERNEL); + if (!ldata) + return -ENOMEM;
- ldata = kzalloc(sizeof(struct pl031_local), GFP_KERNEL); - if (!ldata) { - ret = -ENOMEM; - goto out; - } ldata->vendor = vendor;
- ldata->base = ioremap(adev->res.start, resource_size(&adev->res)); - - if (!ldata->base) { - ret = -ENOMEM; - goto out_no_remap; - } + ldata->base = devm_ioremap_resource(&adev->dev, &adev->res); + if (IS_ERR(ldata->base)) + return PTR_ERR(ldata->base);
amba_set_drvdata(adev, ldata);
@@ -374,32 +359,18 @@ static int pl031_probe(struct amba_device *adev, const struct amba_id *id) }
device_init_wakeup(&adev->dev, true); - ldata->rtc = rtc_device_register("pl031", &adev->dev, ops, - THIS_MODULE); - if (IS_ERR(ldata->rtc)) { - ret = PTR_ERR(ldata->rtc); - goto out_no_rtc; - } + ldata->rtc = devm_rtc_device_register(&adev->dev, "pl031", ops, + THIS_MODULE); + if (IS_ERR(ldata->rtc)) + return PTR_ERR(ldata->rtc); + + ret = devm_request_irq(&adev->dev, adev->irq[0], pl031_interrupt, + vendor->irqflags, "rtc-pl031", ldata); + if (ret) + return ret;
- if (request_irq(adev->irq[0], pl031_interrupt, - vendor->irqflags, "rtc-pl031", ldata)) { - ret = -EIO; - goto out_no_irq; - } dev_pm_set_wake_irq(&adev->dev, adev->irq[0]); return 0; - -out_no_irq: - rtc_device_unregister(ldata->rtc); -out_no_rtc: - iounmap(ldata->base); -out_no_remap: - kfree(ldata); -out: - amba_release_regions(adev); -err_req: - - return ret; }
/* Operations for the original ARM version */
On Sun, Mar 26, 2017 at 4:41 PM, Leo Yan leo.yan@linaro.org wrote:
Convert driver to use devm_*() APIs so rely on driver model core layer to manage resources. This eliminates error path boilerplate and makes code neat.
Signed-off-by: Leo Yan leo.yan@linaro.org
Acked-by: Linus Walleij linus.walleij@linaro.org
Yours, Linus Walleij
Convert driver to use devm_*() APIs so rely on driver model core layer to manage resources. This eliminates error path boilerplate and makes code neat.
This patch also renames amba_id structure, the old code used some code which directly copied from other driver.
Signed-off-by: Leo Yan leo.yan@linaro.org --- drivers/vfio/platform/vfio_amba.c | 25 ++++++++----------------- 1 file changed, 8 insertions(+), 17 deletions(-)
diff --git a/drivers/vfio/platform/vfio_amba.c b/drivers/vfio/platform/vfio_amba.c index 31372fb..433db1f 100644 --- a/drivers/vfio/platform/vfio_amba.c +++ b/drivers/vfio/platform/vfio_amba.c @@ -53,15 +53,14 @@ static int vfio_amba_probe(struct amba_device *adev, const struct amba_id *id) struct vfio_platform_device *vdev; int ret;
- vdev = kzalloc(sizeof(*vdev), GFP_KERNEL); + vdev = devm_kzalloc(&adev->dev, sizeof(*vdev), GFP_KERNEL); if (!vdev) return -ENOMEM;
- vdev->name = kasprintf(GFP_KERNEL, "vfio-amba-%08x", adev->periphid); - if (!vdev->name) { - kfree(vdev); + vdev->name = devm_kasprintf(&adev->dev, GFP_KERNEL, + "vfio-amba-%08x", adev->periphid); + if (!vdev->name) return -ENOMEM; - }
vdev->opaque = (void *) adev; vdev->flags = VFIO_DEVICE_FLAGS_AMBA; @@ -71,11 +70,6 @@ static int vfio_amba_probe(struct amba_device *adev, const struct amba_id *id) vdev->reset_required = false;
ret = vfio_platform_probe_common(vdev, &adev->dev); - if (ret) { - kfree(vdev->name); - kfree(vdev); - } - return ret; }
@@ -84,25 +78,22 @@ static int vfio_amba_remove(struct amba_device *adev) struct vfio_platform_device *vdev;
vdev = vfio_platform_remove_common(&adev->dev); - if (vdev) { - kfree(vdev->name); - kfree(vdev); + if (vdev) return 0; - }
return -EINVAL; }
-static struct amba_id pl330_ids[] = { +static struct amba_id vfio_ids[] = { { 0, 0 }, };
-MODULE_DEVICE_TABLE(amba, pl330_ids); +MODULE_DEVICE_TABLE(amba, vfio_ids);
static struct amba_driver vfio_amba_driver = { .probe = vfio_amba_probe, .remove = vfio_amba_remove, - .id_table = pl330_ids, + .id_table = vfio_ids, .drv = { .name = "vfio-amba", .owner = THIS_MODULE,
Convert to use devm_ioremap_resource() in probe function, otherwise it's missed to unremap this region if probe failed or rmmod module.
Signed-off-by: Leo Yan leo.yan@linaro.org --- sound/arm/aaci.c | 21 ++++++--------------- 1 file changed, 6 insertions(+), 15 deletions(-)
diff --git a/sound/arm/aaci.c b/sound/arm/aaci.c index 4140b1b..2078568 100644 --- a/sound/arm/aaci.c +++ b/sound/arm/aaci.c @@ -990,19 +990,13 @@ static int aaci_probe(struct amba_device *dev, struct aaci *aaci; int ret, i;
- ret = amba_request_regions(dev, NULL); - if (ret) - return ret; - aaci = aaci_init_card(dev); - if (!aaci) { - ret = -ENOMEM; - goto out; - } + if (!aaci) + return -ENOMEM;
- aaci->base = ioremap(dev->res.start, resource_size(&dev->res)); - if (!aaci->base) { - ret = -ENOMEM; + aaci->base = devm_ioremap_resource(&dev->dev, &dev->res); + if (IS_ERR(aaci->base)) { + ret = PTR_ERR(aaci->base); goto out; }
@@ -1064,9 +1058,7 @@ static int aaci_probe(struct amba_device *dev, }
out: - if (aaci) - snd_card_free(aaci->card); - amba_release_regions(dev); + snd_card_free(aaci->card); return ret; }
@@ -1079,7 +1071,6 @@ static int aaci_remove(struct amba_device *dev) writel(0, aaci->base + AACI_MAINCR);
snd_card_free(card); - amba_release_regions(dev); }
return 0;
On Sun, Mar 26, 2017 at 10:41:54PM +0800, Leo Yan wrote:
Convert to use devm_ioremap_resource() in probe function, otherwise it's missed to unremap this region if probe failed or rmmod module.
Again, wrong - just because there's nothing in the cleanup paths does not mean it doesn't clean up.
static void aaci_free_card(struct snd_card *card) { struct aaci *aaci = card->private_data;
iounmap(aaci->base); }
This unmaps the mapping you claim it fails to do so. So, your patch actually _creates_ bugs that were not there before.
NAK.
On 26/03/2017 at 22:41:49 +0800, Leo Yan wrote:
When review device driver modules which attach to amba bus, found several modules are not using devm_*() apis to manage resource. As result, some drivers have memory leakage or missing iomem unmapping when rmmod module. And the code has many "goto" tags to handle different failures.
So this patch series is to convert to use devm_*() for moudules which are attached to amba bus to manage resource and get more robust and neat code.
Patch 0003 "drivers/rtc/rtc-pl031.c: Convert to use devm_*()" has been verified on 96boards Hikey. Other patches can pass building but have not really tested on hardware.
If your plan is to actually remove usage of amba_request_regions() and amba_release_regions(), you should do so in its own patch sets instead of hiding that in a useless cleanup series.
On Sun, Mar 26, 2017 at 05:20:50PM +0200, Alexandre Belloni wrote:
On 26/03/2017 at 22:41:49 +0800, Leo Yan wrote:
When review device driver modules which attach to amba bus, found several modules are not using devm_*() apis to manage resource. As result, some drivers have memory leakage or missing iomem unmapping when rmmod module. And the code has many "goto" tags to handle different failures.
So this patch series is to convert to use devm_*() for moudules which are attached to amba bus to manage resource and get more robust and neat code.
Patch 0003 "drivers/rtc/rtc-pl031.c: Convert to use devm_*()" has been verified on 96boards Hikey. Other patches can pass building but have not really tested on hardware.
If your plan is to actually remove usage of amba_request_regions() and amba_release_regions(), you should do so in its own patch sets instead of hiding that in a useless cleanup series.
Just curious, from Russell's replying for patch 0005, IIUC we cannot totally remove usage of amba_request_regions() and amba_release_regions(), there have some coner case should use amba_request_regions() + ioremap().
Does it make sense to remove most usage of amba_request_regions() and amba_release_regions() but we still keep these two functions in the kernel?
Thanks, Leo Yan
participants (4)
-
Alexandre Belloni -
Leo Yan -
Linus Walleij -
Russell King - ARM Linux