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

-- 
2.1.4

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

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

> 

>

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