[PATCH 0/5] Convert to use devm_*() for amba attached modules

[PATCH 0/5] Convert to use devm_*() for amba attached modules

[Leo Yan]

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(-)

-- 
2.7.4

[PATCH 1/5] Input: ambakmi - Convert to use devm_*()

[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.

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;
 }
 
-- 
2.7.4

[PATCH 1/5] Input: ambakmi - Convert to use devm_*()

[Russell King - ARM Linux]

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.

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

[PATCH 1/5] Input: ambakmi - Convert to use devm_*()

[Leo Yan]

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

[PATCH 2/5] drivers/rtc/rtc-pl030.c: Convert to use devm_*()

[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;
 }
 
-- 
2.7.4

[PATCH 2/5] drivers/rtc/rtc-pl030.c: Convert to use devm_*()

[Linus Walleij]

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

[PATCH 3/5] drivers/rtc/rtc-pl031.c: Convert to use devm_*()

[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-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 */
-- 
2.7.4

[PATCH 3/5] drivers/rtc/rtc-pl031.c: Convert to use devm_*()

[Linus Walleij]

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

[PATCH 4/5] vfio: platform: Convert to use devm_*()

[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.

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,
-- 
2.7.4

[PATCH 4/5] vfio: platform: Convert to use devm_*()

[Auger Eric]

Hi Leo,

On 26/03/2017 16:41, 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 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>
Looks good to me
Reviewed-by: Eric Auger <eric.auger@redhat.com>

May be interesting to go further converting as well the vfio-platform
driver but this can be done later on.

Thanks

Eric


> ---
>  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,
> 

[PATCH 4/5] vfio: platform: Convert to use devm_*()

[Leo Yan]

On Sun, Apr 02, 2017 at 04:45:28PM +0200, Auger Eric wrote:
> Hi Leo,
> 
> On 26/03/2017 16:41, 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 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>
> Looks good to me
> Reviewed-by: Eric Auger <eric.auger@redhat.com>

Thanks for reviewing, Eric.

> May be interesting to go further converting as well the vfio-platform
> driver but this can be done later on.

Looked a bit for vfio-platform driver and I'm not sure if need some
converting within function vfio_platform_probe_common(). So I'd like
to leave this to who really understand the subsystem :)

[...]

Thanks,
Leo Yan

[PATCH 5/5] ALSA: AACI: Convert to use devm_ioremap_resource()

[Leo Yan]

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;
-- 
2.7.4

[PATCH 5/5] ALSA: AACI: Convert to use devm_ioremap_resource()

[Russell King - ARM Linux]

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.

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

[PATCH 0/5] Convert to use devm_*() for amba attached modules

[Alexandre Belloni]

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.

-- 
Alexandre Belloni, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[PATCH 0/5] Convert to use devm_*() for amba attached modules

[Leo Yan]

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