Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1698018 > unrolled thread

[PATCH 2/3] staging: ccree: Convert to devm_ioremap_resource for map, unmap

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH 2/3] staging: ccree: Convert to devm_ioremap_resource for map, unmap Gilad Ben-Yossef <gilad@benyossef.com> - 2017-07-27 16:30 +0200
    Re: [PATCH 2/3] staging: ccree: Convert to devm_ioremap_resource for  map, unmap Dan Carpenter <dan.carpenter@oracle.com> - 2017-07-27 21:50 +0200
      Re: [PATCH 2/3] staging: ccree: Convert to devm_ioremap_resource for  map, unmap Suniel Mahesh <sunil.m@techveda.org> - 2017-07-28 06:30 +0200
        Re: [PATCH 2/3] staging: ccree: Convert to devm_ioremap_resource for  map, unmap Dan Carpenter <dan.carpenter@oracle.com> - 2017-07-28 10:50 +0200
      Re: [PATCH 2/3] staging: ccree: Convert to devm_ioremap_resource for  map, unmap Gilad Ben-Yossef <gilad@benyossef.com> - 2017-07-30 18:20 +0200

#1698018 — [PATCH 2/3] staging: ccree: Convert to devm_ioremap_resource for map, unmap

From: Suniel Mahesh <sunil.m@techveda.org>

It is recommended to use managed function devm_ioremap_resource(),
which simplifies driver cleanup paths and driver code.
This patch does the following:
(a) replace request_mem_region(), ioremap() and corresponding error
handling with devm_ioremap_resource().
(b) remove struct resource pointer(res_mem) in struct ssi_drvdata as it
seems redundant, use struct resource pointer which is defined locally and
adjust return value of platform_get_resource() accordingly.
(c) release_mem_region() and iounmap() are dropped, since devm_ioremap_
resource() releases and unmaps mem region on driver detach.
(d) adjust log messages accordingly and remove any blank lines.

Signed-off-by: Suniel Mahesh <sunil.m@techveda.org>
[gby: rebase on top of latest coding style fixes changes]
Acked-by: Gilad Ben-Yossef <gilad@benyossef.com>
---
 drivers/staging/ccree/ssi_driver.c | 60 ++++++++++----------------------------
 drivers/staging/ccree/ssi_driver.h |  1 -
 2 files changed, 15 insertions(+), 46 deletions(-)

diff --git a/drivers/staging/ccree/ssi_driver.c b/drivers/staging/ccree/ssi_driver.c
index 97dfc2c..603eb03 100644
--- a/drivers/staging/ccree/ssi_driver.c
+++ b/drivers/staging/ccree/ssi_driver.c
@@ -246,35 +246,21 @@ static int init_cc_resources(struct platform_device *plat_dev)
 	dev_set_drvdata(&plat_dev->dev, new_drvdata);
 	/* Get device resources */
 	/* First CC registers space */
-	new_drvdata->res_mem = platform_get_resource(plat_dev, IORESOURCE_MEM, 0);
-	if (unlikely(!new_drvdata->res_mem)) {
-		SSI_LOG_ERR("Failed getting IO memory resource\n");
-		rc = -ENODEV;
-		goto init_cc_res_err;
-	}
-	SSI_LOG_DEBUG("Got MEM resource (%s): start=%pad end=%pad\n",
-		      new_drvdata->res_mem->name,
-		      new_drvdata->res_mem->start,
-		      new_drvdata->res_mem->end);
+	req_mem_cc_regs = platform_get_resource(plat_dev, IORESOURCE_MEM, 0);
 	/* Map registers space */
-	req_mem_cc_regs = request_mem_region(new_drvdata->res_mem->start, resource_size(new_drvdata->res_mem), "arm_cc7x_regs");
-	if (unlikely(!req_mem_cc_regs)) {
-		SSI_LOG_ERR("Couldn't allocate registers memory region at 0x%08X\n",
-			    (unsigned int)new_drvdata->res_mem->start);
-		rc = -EBUSY;
-		goto init_cc_res_err;
-	}
-	cc_base = ioremap(new_drvdata->res_mem->start, resource_size(new_drvdata->res_mem));
-	if (unlikely(!cc_base)) {
-		SSI_LOG_ERR("ioremap[CC](0x%08X,0x%08X) failed\n",
-			    (unsigned int)new_drvdata->res_mem->start,
-			    (unsigned int)resource_size(new_drvdata->res_mem));
-		rc = -ENOMEM;
+	new_drvdata->cc_base = devm_ioremap_resource(&plat_dev->dev,
+						     req_mem_cc_regs);
+	if (IS_ERR(new_drvdata->cc_base)) {
+		rc = PTR_ERR(new_drvdata->cc_base);
 		goto init_cc_res_err;
 	}
-	SSI_LOG_DEBUG("CC registers mapped from %pa to 0x%p\n", &new_drvdata->res_mem->start, cc_base);
-	new_drvdata->cc_base = cc_base;
-
+	SSI_LOG_DEBUG("Got MEM resource (%s): start=%pad end=%pad\n",
+		      req_mem_cc_regs->name,
+		      req_mem_cc_regs->start,
+		      req_mem_cc_regs->end);
+	SSI_LOG_DEBUG("CC registers mapped from %pa to 0x%p\n",
+		      &req_mem_cc_regs->start, new_drvdata->cc_base);
+	cc_base = new_drvdata->cc_base;
 	/* Then IRQ */
 	new_drvdata->res_irq = platform_get_resource(plat_dev, IORESOURCE_IRQ, 0);
 	if (unlikely(!new_drvdata->res_irq)) {
@@ -424,17 +410,9 @@ static int init_cc_resources(struct platform_device *plat_dev)
 #ifdef ENABLE_CC_SYSFS
 		ssi_sysfs_fini();
 #endif
-
-		if (req_mem_cc_regs) {
-			if (irq_registered) {
-				free_irq(new_drvdata->res_irq->start, new_drvdata);
-				new_drvdata->res_irq = NULL;
-				iounmap(cc_base);
-				new_drvdata->cc_base = NULL;
-			}
-			release_mem_region(new_drvdata->res_mem->start,
-					   resource_size(new_drvdata->res_mem));
-			new_drvdata->res_mem = NULL;
+		if (irq_registered) {
+			free_irq(new_drvdata->res_irq->start, new_drvdata);
+			new_drvdata->res_irq = NULL;
 		}
 		dev_set_drvdata(&plat_dev->dev, NULL);
 	}
@@ -470,14 +448,6 @@ static void cleanup_cc_resources(struct platform_device *plat_dev)
 	cc_clk_off(drvdata);
 	free_irq(drvdata->res_irq->start, drvdata);
 	drvdata->res_irq = NULL;
-
-	if (drvdata->cc_base) {
-		iounmap(drvdata->cc_base);
-		release_mem_region(drvdata->res_mem->start,
-				   resource_size(drvdata->res_mem));
-		drvdata->cc_base = NULL;
-		drvdata->res_mem = NULL;
-	}
 	dev_set_drvdata(&plat_dev->dev, NULL);
 }
 
diff --git a/drivers/staging/ccree/ssi_driver.h b/drivers/staging/ccree/ssi_driver.h
index b6ad89a..518c0bf 100644
--- a/drivers/staging/ccree/ssi_driver.h
+++ b/drivers/staging/ccree/ssi_driver.h
@@ -128,7 +128,6 @@ struct ssi_crypto_req {
  * @fw_ver:	SeP loaded firmware version
  */
 struct ssi_drvdata {
-	struct resource *res_mem;
 	struct resource *res_irq;
 	void __iomem *cc_base;
 	unsigned int irq;
-- 
2.1.4

[toc] | [next] | [standalone]

#1698281 — Re: [PATCH 2/3] staging: ccree: Convert to devm_ioremap_resource for map, unmap

On Thu, Jul 27, 2017 at 05:27:33PM +0300, Gilad Ben-Yossef wrote:
> +	new_drvdata->cc_base = devm_ioremap_resource(&plat_dev->dev,
> +						     req_mem_cc_regs);
> +	if (IS_ERR(new_drvdata->cc_base)) {
> +		rc = PTR_ERR(new_drvdata->cc_base);
>  		goto init_cc_res_err;
                ^^^^^^^^^^^^^^^^^^^^
(This code was in the original and not introduced by the patch.)

Ideally, the goto name should say what the goto does.  In this case it
does everything.  Unfortunately trying to do everything is very
complicated so obviously the error handling is going to be full of bugs.

The first thing the error handling does is:

	ssi_aead_free(new_drvdata);

But this function assumes that if new_drvdata->aead_handle is non-NULL
then that means we have called:

	INIT_LIST_HEAD(&aead_handle->aead_list);

That assumption is false if the aead_handle->sram_workspace_addr
allocation fails.  It can't actually fail in the current code...  So
that's good, I suppose.  Reviewing this code is really hard, because I
have to jump back and forth through several functions in different
files.

Moving on two the second error handling function:

	ssi_hash_free(new_drvdata);

This one has basically the same assumption that if ->hash_handle is
allocated that means we called:

	INIT_LIST_HEAD(&hash_handle->hash_list);

That assumption is not true if ssi_hash_init_sram_digest_consts(drvdata);
fails.  That function can fail in real life.  Except the the error
handling in ssi_hash_alloc() sets ->hash_handle to NULL.  So the bug is
just a leak and not a crashing bug.

I've reviewed the first two lines of the error handling just to give a
feel for how complicated "do everything" style error handling is to
review.

The better way to do error handling is:
1) Only free things which have been allocated.
2) The unwind code should mirror the wind up code.
3) Every allocation function should have a free function.
4) Label names should tell you what the goto does.
5) Use direct returns and literals where possible.
6) Generally it's better to keep the error path and the success path
   separate.
7) Do error handling as opposed to success handling.

	one = alloc();
	if (!one)
		return -ENOMEM;
	if (foo) {
		two = alloc();
		if (!two) {
			ret = -ENOMEM;
			goto free_one;
		}
	}
	three = alloc();
	if (!three) {
		ret = -ENOMEM;
		goto free_two;
	}
	...

	return 0;

free_two:
	if (foo)
		free(two);
free_one:
	free(one);

	return ret;

This style of error handling is easier to review.  You only need to
remember the most recent thing that you have allocated.  You can tell
from the goto that it frees it so you don't have to scroll to the
bottom of the function or jump to a different file.

regards,
dan carpenter

[toc] | [prev] | [next] | [standalone]

#1698456 — Re: [PATCH 2/3] staging: ccree: Convert to devm_ioremap_resource for map, unmap

On Friday 28 July 2017 01:18 AM, Dan Carpenter wrote:
> On Thu, Jul 27, 2017 at 05:27:33PM +0300, Gilad Ben-Yossef wrote:
>> +	new_drvdata->cc_base = devm_ioremap_resource(&plat_dev->dev,
>> +						     req_mem_cc_regs);
>> +	if (IS_ERR(new_drvdata->cc_base)) {
>> +		rc = PTR_ERR(new_drvdata->cc_base);
>>  		goto init_cc_res_err;
>                 ^^^^^^^^^^^^^^^^^^^^
> (This code was in the original and not introduced by the patch.)

Hi Dan, the above lines of code were not in the original except 
"goto init_cc_res_err;" which was doing the error handling at different
places.

This patch has added those first three lines. I was constantly checking the latest
linux-next and staging-testing / next git trees. 
 
> 
> Ideally, the goto name should say what the goto does.  In this case it
> does everything.  Unfortunately trying to do everything is very
> complicated so obviously the error handling is going to be full of bugs.
> 
> The first thing the error handling does is:
> 
> 	ssi_aead_free(new_drvdata);
> 
> But this function assumes that if new_drvdata->aead_handle is non-NULL
> then that means we have called:
> 
> 	INIT_LIST_HEAD(&aead_handle->aead_list);
> 
> That assumption is false if the aead_handle->sram_workspace_addr
> allocation fails.  It can't actually fail in the current code...  So
> that's good, I suppose.  Reviewing this code is really hard, because I
> have to jump back and forth through several functions in different
> files.
> 
> Moving on two the second error handling function:
> 
> 	ssi_hash_free(new_drvdata);
> 
> This one has basically the same assumption that if ->hash_handle is
> allocated that means we called:
> 
> 	INIT_LIST_HEAD(&hash_handle->hash_list);
> 
> That assumption is not true if ssi_hash_init_sram_digest_consts(drvdata);
> fails.  That function can fail in real life.  Except the the error
> handling in ssi_hash_alloc() sets ->hash_handle to NULL.  So the bug is
> just a leak and not a crashing bug.
> 
> I've reviewed the first two lines of the error handling just to give a
> feel for how complicated "do everything" style error handling is to
> review.
> 
> The better way to do error handling is:
> 1) Only free things which have been allocated.
> 2) The unwind code should mirror the wind up code.
> 3) Every allocation function should have a free function.
> 4) Label names should tell you what the goto does.
> 5) Use direct returns and literals where possible.
> 6) Generally it's better to keep the error path and the success path
>    separate.
> 7) Do error handling as opposed to success handling.
> 
> 	one = alloc();
> 	if (!one)
> 		return -ENOMEM;
> 	if (foo) {
> 		two = alloc();
> 		if (!two) {
> 			ret = -ENOMEM;
> 			goto free_one;
> 		}
> 	}
> 	three = alloc();
> 	if (!three) {
> 		ret = -ENOMEM;
> 		goto free_two;
> 	}
> 	...
> 
> 	return 0;
> 
> free_two:
> 	if (foo)
> 		free(two);
> free_one:
> 	free(one);
> 
> 	return ret;
> 
> This style of error handling is easier to review.  You only need to
> remember the most recent thing that you have allocated.  You can tell
> from the goto that it frees it so you don't have to scroll to the
> bottom of the function or jump to a different file.

I understand, its make sense, may be we should come up with something 
better and simpler.

Thanks
Suniel

> 
> regards,
> dan carpenter
> 
> 

[toc] | [prev] | [next] | [standalone]

#1698596 — Re: [PATCH 2/3] staging: ccree: Convert to devm_ioremap_resource for map, unmap

On Fri, Jul 28, 2017 at 09:59:41AM +0530, Suniel Mahesh wrote:
> On Friday 28 July 2017 01:18 AM, Dan Carpenter wrote:
> > On Thu, Jul 27, 2017 at 05:27:33PM +0300, Gilad Ben-Yossef wrote:
> >> +	new_drvdata->cc_base = devm_ioremap_resource(&plat_dev->dev,
> >> +						     req_mem_cc_regs);
> >> +	if (IS_ERR(new_drvdata->cc_base)) {
> >> +		rc = PTR_ERR(new_drvdata->cc_base);
> >>  		goto init_cc_res_err;
> >                 ^^^^^^^^^^^^^^^^^^^^
> > (This code was in the original and not introduced by the patch.)
> 
> Hi Dan, the above lines of code were not in the original except 
> "goto init_cc_res_err;" which was doing the error handling at different
> places.
> 

Yes, yes.  I wasn't commenting on the patch just the existing code.

The patch is fine.

regards,
dan carpenter

[toc] | [prev] | [next] | [standalone]

#1699553 — Re: [PATCH 2/3] staging: ccree: Convert to devm_ioremap_resource for map, unmap

On Thu, Jul 27, 2017 at 10:48 PM, Dan Carpenter
<dan.carpenter@oracle.com> wrote:
> On Thu, Jul 27, 2017 at 05:27:33PM +0300, Gilad Ben-Yossef wrote:
>> +     new_drvdata->cc_base = devm_ioremap_resource(&plat_dev->dev,
>> +                                                  req_mem_cc_regs);
>> +     if (IS_ERR(new_drvdata->cc_base)) {
>> +             rc = PTR_ERR(new_drvdata->cc_base);
>>               goto init_cc_res_err;
>                 ^^^^^^^^^^^^^^^^^^^^
> (This code was in the original and not introduced by the patch.)
>
> Ideally, the goto name should say what the goto does.  In this case it
> does everything.  Unfortunately trying to do everything is very
> complicated so obviously the error handling is going to be full of bugs.

Thank you. This review is most helpful. I've added a patch to address
the specific
issues you've raised.

I also see some additional places that can get similar treatment to
both what you
suggested as well as what Suneil has done. I'll address those in
follow up patches.

Gilad



-- 
Gilad Ben-Yossef
Chief Coffee Drinker

"If you take a class in large-scale robotics, can you end up in a
situation where the homework eats your dog?"
 -- Jean-Baptiste Queru

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web