Re: [PATCH v3] staging: ccree: fix boolreturn.cocci warning
On Thursday 19 October 2017 02:24 AM, Tobin C. Harding wrote:
> Hi Suniel,
>
> Well done with you continued versions. I am being particularly nit picky here
> but since we are
> striving for perfection I'm sure will humour me. If English is not your first
> language please
> forgive me for picking you up on language subtleties.
Hi Tobin,
First of all, I thank you very much for the reviews, to be honest I enjoyed the
process. Yes all of
us, here we are striving for perfection. I am always open to take suggestions
from the community to
improve things which I am working on and there by improving myself. Yeah
English is not my first
language, but all my education was done in English, no issues there.
>
> On Wed, Oct 18, 2017 at 12:11:55PM +0530, suni...@techveda.org wrote:
>> From: Suniel Mahesh
>>
>> This fixes the following coccinelle warning:
>> WARNING: return of 0/1 in function 'ssi_is_hw_key' with return type bool.
>
> This should be a description of the problem, so saying _why_ there is a
> problem or _what_ is wrong
> with the code currently that warrants a patch. Sometimes while describing the
> problem you may
> include descriptions of the solution especially it is not immediately obvious
> why your proposed
> solution fixes the issue being explained. As an extra we shouldn't ever say
> 'This patch ...' or
> 'This does xyz'.
>
>> return "false" instead of 0.
>
> Perfect, this is in imperative mood. Spot on!
>
>> Signed-off-by: Suniel Mahesh
>> ---
>> Changes for v3:
>> - Changed the commit log even more to give an accurate
>> description of the changeset as suggested by Toby C.Harding.
>
> My name is Tobin :)
how did I blind myself, my bad, will be careful and avoid such mistakes moving
forward.
>
>> ---
>> Changes for v2:
>> - Changed the commit log to give a more accurate description
>> of the changeset as suggested by Toby C.Harding.
>> ---
>> Note:
>> - Patch was built(ARCH=arm) on latest linux-next.
>> - No build issues reported, however it was not
>> tested on real hardware.
>> - Please discard this changeset, if this is not
>> helping the code look better.
>> ---
>> drivers/staging/ccree/ssi_cipher.h | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/staging/ccree/ssi_cipher.h
>> b/drivers/staging/ccree/ssi_cipher.h
>> index c9a83df..f499962 100644
>> --- a/drivers/staging/ccree/ssi_cipher.h
>> +++ b/drivers/staging/ccree/ssi_cipher.h
>> @@ -75,7 +75,7 @@ struct arm_hw_key_info {
>>
>> static inline bool ssi_is_hw_key(struct crypto_tfm *tfm)
>> {
>> -return 0;
>> +return false;
>> }
>>
>> #endif /* CRYPTO_TFM_REQ_HW_KEY */
>> --
>> 1.9.1
>>
>
> For what it's worth, Reviewed-by: Tobin C. Harding
>
> As stated I am being particularly 'nit picky', the commit log is _probably_
> good enough to be
> merged, I am not a maintainer though so it's not really anything to do with
> me. I do know however
> that sometimes patches go to the bottom of Greg's list if they have
> comments/suggestions. I mention
> this only so you learn more about the process and to help you with
> successfully getting you patches
> merged. Keep up the work!
Thanks once again Tobin, I love feedback and that's how we can make this world
a better workplace.
Suniel
>
> Good luck,
> Tobin.
>
Re: [PATCH] staging: ccree: local variable "dev" not required
On Thursday 05 October 2017 11:30 PM, Joe Perches wrote: > On Thu, 2017-10-05 at 10:07 +0300, Gilad Ben-Yossef wrote: >> On Wed, Oct 4, 2017 at 10:12 PM, wrote: >>> There is no need to create a local pointer variable "dev" and >>> pass it various API's, instead use plat_dev which is enumerated >>> by platform core on successful probe. > [] >> I'm sorry but I don't understand what is the problem you are trying to solve >> or >> what is the improvement suggested. Hi Gilad and all, Actually this patch is not a major improvement nor trying to solve some thing. As struct device is a member of struct platform_device, I thought why can't we use plat_dev (pointer to struct platform_device) through out the code. May be I was trying to make code look compact, may be I am wrong. >> >> Why is having a local variable undesirable? I think having it makes >> the code more readable, not less. > > IMO: The local variable is _not_ undesirable. > It does make the code more readable and > shortens line lengths too. > Yes, as you guys pointed out, we can use local variables. It is definitely making the code more readable and shortening line lengths. Please drop the patch, if this not helping the code look better. Thanks Suniel
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
>
>
Re: [PATCH 1/3] staging: ccree: Replace kzalloc with devm_kzalloc
On Monday 17 July 2017 06:03 PM, Greg KH wrote:
> On Sat, Jul 15, 2017 at 01:21:54PM +0530, suni...@techveda.org wrote:
>> From: Suniel Mahesh
>>
>> It is recommended to use managed function devm_kzalloc, which
>> simplifies driver cleanup paths and driver code.
>> This patch does the following:
>> (a) replace kzalloc with devm_kzalloc.
>> (b) drop kfree(), because memory allocated with devm_kzalloc() is
>> automatically freed on driver detach, otherwise it leads to a double
>> free.
>> (c) remove unnecessary blank lines.
>>
>> Signed-off-by: Suniel Mahesh
>> ---
>> Note:
>> - Patch was tested and built(ARCH=arm) on next-20170714.
>> No build issues reported, however it was not tested on
>> real hardware.
>> ---
>> drivers/staging/ccree/ssi_driver.c | 10 --
>> 1 file changed, 4 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/staging/ccree/ssi_driver.c
>> b/drivers/staging/ccree/ssi_driver.c
>> index 78709b92..f231ecf 100644
>> --- a/drivers/staging/ccree/ssi_driver.c
>> +++ b/drivers/staging/ccree/ssi_driver.c
>> @@ -224,13 +224,15 @@ static int init_cc_resources(struct platform_device
>> *plat_dev)
>> struct resource *req_mem_cc_regs = NULL;
>> void __iomem *cc_base = NULL;
>> bool irq_registered = false;
>> -struct ssi_drvdata *new_drvdata = kzalloc(sizeof(struct ssi_drvdata),
>> GFP_KERNEL);
>> +struct ssi_drvdata *new_drvdata;
>> struct device *dev = &plat_dev->dev;
>> struct device_node *np = dev->of_node;
>> u32 signature_val;
>> int rc = 0;
>>
>> -if (unlikely(!new_drvdata)) {
>> +new_drvdata = devm_kzalloc(&plat_dev->dev, sizeof(struct ssi_drvdata),
>
> sizeof(*new_drvdata), right?
>
Yes, sending v2 in a while. Thanks for the review
suniel
> thanks,
>
> greg k-h
>
