[PATCH] staging: most: hdm-dim2: Use devm_ functions

[PATCH] staging: most: hdm-dim2: Use devm_ functions

[Amitoj Kaur Chawla]

Devm_ functions allocate memory that is released when a driver
detaches. Replace kzalloc, ioremap and request_irq with their devm_*
counterparts in the probe function of a platform device which were
originally freed in the remove function.

Also, unnecessary labels have been removed and header file
linux/device.h has been added to ensure devm_* routine declarations
are unambiguously available.

Signed-off-by: Amitoj Kaur Chawla <amitoj1606@gmail.com>
---
 drivers/staging/most/hdm-dim2/dim2_hdm.c | 57 +++++++-------------------------
 1 file changed, 12 insertions(+), 45 deletions(-)

diff --git a/drivers/staging/most/hdm-dim2/dim2_hdm.c b/drivers/staging/most/hdm-dim2/dim2_hdm.c
index 6296aa5..759f2cc 100644
--- a/drivers/staging/most/hdm-dim2/dim2_hdm.c
+++ b/drivers/staging/most/hdm-dim2/dim2_hdm.c
@@ -24,6 +24,7 @@
 #include <linux/dma-mapping.h>
 #include <linux/sched.h>
 #include <linux/kthread.h>
+#include <linux/device.h>
 
 #include <mostcore.h>
 #include <networking.h>
@@ -736,58 +737,41 @@ static int dim2_probe(struct platform_device *pdev)
 	int ret, i;
 	struct kobject *kobj;
 
-	dev = kzalloc(sizeof(*dev), GFP_KERNEL);
+	dev = devm_kzalloc(&pdev->dev, sizeof(*dev), GFP_KERNEL);
 	if (!dev)
 		return -ENOMEM;
 
 	dev->atx_idx = -1;
 
-	platform_set_drvdata(pdev, dev);
 #if defined(ENABLE_HDM_TEST)
 	test_dev = dev;
 #else
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-	if (!res) {
-		pr_err("no memory region defined\n");
-		ret = -ENOENT;
-		goto err_free_dev;
-	}
-
-	if (!request_mem_region(res->start, resource_size(res), pdev->name)) {
-		pr_err("failed to request mem region\n");
-		ret = -EBUSY;
-		goto err_free_dev;
-	}
-
-	dev->io_base = ioremap(res->start, resource_size(res));
-	if (!dev->io_base) {
-		pr_err("failed to ioremap\n");
-		ret = -ENOMEM;
-		goto err_release_mem;
-	}
+	dev->io_base = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(dev->io_base))
+		return PTR_ERR(dev->io_base);
 
 	ret = platform_get_irq(pdev, 0);
 	if (ret < 0) {
 		pr_err("failed to get irq\n");
-		goto err_unmap_io;
+		return ret;
 	}
 	dev->irq_ahb0 = ret;
 
-	ret = request_irq(dev->irq_ahb0, dim2_ahb_isr, 0, "mlb_ahb0", dev);
+	ret = devm_request_irq(&pdev->dev, dev->irq_ahb0, dim2_ahb_isr, 0,
+			       "mlb_ahb0", dev);
 	if (ret) {
 		pr_err("failed to request IRQ: %d, err: %d\n",
 		       dev->irq_ahb0, ret);
-		goto err_unmap_io;
+		return ret;
 	}
 #endif
 	init_waitqueue_head(&dev->netinfo_waitq);
 	dev->deliver_netinfo = 0;
 	dev->netinfo_task = kthread_run(&deliver_netinfo_thread, (void *)dev,
 					"dim2_netinfo");
-	if (IS_ERR(dev->netinfo_task)) {
-		ret = PTR_ERR(dev->netinfo_task);
-		goto err_free_irq;
-	}
+	if (IS_ERR(dev->netinfo_task))
+		return PTR_ERR(dev->netinfo_task);
 
 	for (i = 0; i < DMA_CHANNELS; i++) {
 		struct most_channel_capability *cap = dev->capabilities + i;
@@ -847,6 +831,7 @@ static int dim2_probe(struct platform_device *pdev)
 		goto err_destroy_bus;
 	}
 
+	platform_set_drvdata(pdev, dev);
 	return 0;
 
 err_destroy_bus:
@@ -855,16 +840,6 @@ err_unreg_iface:
 	most_deregister_interface(&dev->most_iface);
 err_stop_thread:
 	kthread_stop(dev->netinfo_task);
-err_free_irq:
-#if !defined(ENABLE_HDM_TEST)
-	free_irq(dev->irq_ahb0, dev);
-err_unmap_io:
-	iounmap(dev->io_base);
-err_release_mem:
-	release_mem_region(res->start, resource_size(res));
-err_free_dev:
-#endif
-	kfree(dev);
 
 	return ret;
 }
@@ -878,7 +853,6 @@ err_free_dev:
 static int dim2_remove(struct platform_device *pdev)
 {
 	struct dim2_hdm *dev = platform_get_drvdata(pdev);
-	struct resource *res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	struct dim2_platform_data *pdata = pdev->dev.platform_data;
 	unsigned long flags;
 
@@ -892,13 +866,6 @@ static int dim2_remove(struct platform_device *pdev)
 	dim2_sysfs_destroy(&dev->bus);
 	most_deregister_interface(&dev->most_iface);
 	kthread_stop(dev->netinfo_task);
-#if !defined(ENABLE_HDM_TEST)
-	free_irq(dev->irq_ahb0, dev);
-	iounmap(dev->io_base);
-	release_mem_region(res->start, resource_size(res));
-#endif
-	kfree(dev);
-	platform_set_drvdata(pdev, NULL);
 
 	/*
 	 * break link to local platform_device_id struct
-- 
1.9.1

Re: [Outreachy kernel] [PATCH] staging: most: hdm-dim2: Use devm_ functions

[Greg KH]

On Sat, Feb 13, 2016 at 11:18:01PM +0530, Amitoj Kaur Chawla wrote:
> Devm_ functions allocate memory that is released when a driver
> detaches. Replace kzalloc, ioremap and request_irq with their devm_*
> counterparts in the probe function of a platform device which were
> originally freed in the remove function.
> 
> Also, unnecessary labels have been removed and header file
> linux/device.h has been added to ensure devm_* routine declarations
> are unambiguously available.

That's a lot to just do in one patch :(

Do only one conversion at a time, and be _VERY_ careful about the
request_irq() change, that has some tricky side-affects if you don't
watch out.  If you don't know about the side affects, don't make the
change please.

So please break this up into different patches, and no need for the
device.h change, that's not needed at all.

thanks,

greg k-h

Re: [Outreachy kernel] [PATCH] staging: most: hdm-dim2: Use devm_ functions

[Amitoj Kaur Chawla]

On Mon, Feb 15, 2016 at 6:22 AM, Greg KH <gregkh@linuxfoundation.org> wrote:
>
> That's a lot to just do in one patch :(
>
> Do only one conversion at a time, and be _VERY_ careful about the
> request_irq() change, that has some tricky side-affects if you don't
> watch out.  If you don't know about the side affects, don't make the
> change please.
>
> So please break this up into different patches, and no need for the
> device.h change, that's not needed at all.

Okay, thanks for the feedback.

Will send v2 as a patchset.

Amitoj

Re: [Outreachy kernel] [PATCH] staging: most: hdm-dim2: Use devm_ functions

[Julia Lawall]

On Sun, 14 Feb 2016, Greg KH wrote:

> On Sat, Feb 13, 2016 at 11:18:01PM +0530, Amitoj Kaur Chawla wrote:
> > Devm_ functions allocate memory that is released when a driver
> > detaches. Replace kzalloc, ioremap and request_irq with their devm_*
> > counterparts in the probe function of a platform device which were
> > originally freed in the remove function.
> > 
> > Also, unnecessary labels have been removed and header file
> > linux/device.h has been added to ensure devm_* routine declarations
> > are unambiguously available.
> 
> That's a lot to just do in one patch :(
> 
> Do only one conversion at a time, and be _VERY_ careful about the
> request_irq() change, that has some tricky side-affects if you don't
> watch out.  If you don't know about the side affects, don't make the
> change please.

The issue with request_irq is that everything that is allocated before it 
has to be devm_allocated as well, to ensure that everything the interrupt 
handler depends on is freed after the interrupt is freed.  I think that 
that is the case here, but please check it.

To break it up into multiple patches, you would have to put the change on 
the first allocation in the first patch, the second allocation in the 
second patch, etc, to keep a working system after each patch.  The devm 
mechanism frees things in the opposite order in which they are allocated, 
ie just like the order in which the frees normally appear in the error 
handling code at the end of the probe function.

julia

> So please break this up into different patches, and no need for the
> device.h change, that's not needed at all.
> 
> thanks,
> 
> greg k-h
> 
> -- 
> You received this message because you are subscribed to the Google Groups "outreachy-kernel" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to outreachy-kernel+unsubscribe@googlegroups.com.
> To post to this group, send email to outreachy-kernel@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/20160215005209.GA19167%40kroah.com.
> For more options, visit https://groups.google.com/d/optout.
> 

Re: [Outreachy kernel] [PATCH] staging: most: hdm-dim2: Use devm_ functions

[Amitoj Kaur Chawla]

On Mon, Feb 15, 2016 at 11:59 AM, Julia Lawall <julia.lawall@lip6.fr> wrote:
>>
>> Do only one conversion at a time, and be _VERY_ careful about the
>> request_irq() change, that has some tricky side-affects if you don't
>> watch out.  If you don't know about the side affects, don't make the
>> change please.
>
> The issue with request_irq is that everything that is allocated before it
> has to be devm_allocated as well, to ensure that everything the interrupt
> handler depends on is freed after the interrupt is freed.  I think that
> that is the case here, but please check it.
>
> To break it up into multiple patches, you would have to put the change on
> the first allocation in the first patch, the second allocation in the
> second patch, etc, to keep a working system after each patch.  The devm
> mechanism frees things in the opposite order in which they are allocated,
> ie just like the order in which the frees normally appear in the error
> handling code at the end of the probe function.
>
> julia

So to break it up in patches I would :

Patch 1: Switch from kzalloc to devm_kzalloc and remove the relevant
frees, in this case kfree at the end of the probe function and the
remove function
Patch 2: Switch from ioremap and request_mem_region to
devm_ioremap_resource and remove iounmap release_mem_region from probe
and remove functions.
Patch 3: Switch from request_irq to devm_request_irq and remove
iounmap and free_irq from probe and remove functions.

Doubt as to Patch 3 and remove iounmap in the request_irq conversion
since the code currently looks like this:

        ret = request_irq(dev->irq_ahb0, dim2_ahb_isr, 0, "mlb_ahb0", dev);
        if (ret) {
                pr_err("failed to request IRQ: %d, err: %d\n",
                       dev->irq_ahb0, ret);
                goto err_unmap_io;
        }
...
        err_unmap_io:
               iounmap(dev->io_base);