[PATCH 0/7] iSCSI-target: Fine-tuning for three function implementations

[PATCH 0/7] iSCSI-target: Fine-tuning for three function implementations

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sat, 12 Dec 2015 15:25:20 +0100

Some update suggestions were taken into account from static
source code analysis.

Markus Elfring (7):
  Use a variable initialisation in iscsi_set_default_param() directly
  Less checks in iscsi_set_default_param() after error detection
  Delete an unnecessary variable initialisation in iscsi_create_default_params()
  Make a variable initialisation a bit more obvious in iscsi_create_default_params()
  Rename a jump label in iscsi_create_default_params()
  Delete unnecessary variable initialisations in iscsi_check_valuelist_for_support()
  Make two variable initialisations a bit more obvious in iscsi_check_valuelist_for_support()

 drivers/target/iscsi/iscsi_target_parameters.c | 100 ++++++++++++-------------
 1 file changed, 47 insertions(+), 53 deletions(-)

-- 
2.6.3

[PATCH 1/7] iscsi-target: Use a variable initialisation in iscsi_set_default_param() directly

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sat, 12 Dec 2015 11:36:02 +0100

Omit the unnecessary setting to a null pointer for the variable "param"
at the beginning of the function "iscsi_set_default_param"
because it can be directly initialized with the return value
from the function "kzalloc".

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/target/iscsi/iscsi_target_parameters.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/target/iscsi/iscsi_target_parameters.c b/drivers/target/iscsi/iscsi_target_parameters.c
index 3a1f9a7..0a8bd3f 100644
--- a/drivers/target/iscsi/iscsi_target_parameters.c
+++ b/drivers/target/iscsi/iscsi_target_parameters.c
@@ -127,9 +127,8 @@ static struct iscsi_param *iscsi_set_default_param(struct iscsi_param_list *para
 		char *name, char *value, u8 phase, u8 scope, u8 sender,
 		u16 type_range, u8 use)
 {
-	struct iscsi_param *param = NULL;
+	struct iscsi_param *param = kzalloc(sizeof(*param), GFP_KERNEL);
 
-	param = kzalloc(sizeof(struct iscsi_param), GFP_KERNEL);
 	if (!param) {
 		pr_err("Unable to allocate memory for parameter.\n");
 		goto out;
-- 
2.6.3

Re: [PATCH 1/7] iscsi-target: Use a variable initialisation in iscsi_set_default_param() directly

[Dan Carpenter]

On Sat, Dec 12, 2015 at 03:34:50PM +0100, SF Markus Elfring wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Sat, 12 Dec 2015 11:36:02 +0100
> 
> Omit the unnecessary setting to a null pointer for the variable "param"
> at the beginning of the function "iscsi_set_default_param"
> because it can be directly initialized with the return value
> from the function "kzalloc".
> 
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>  drivers/target/iscsi/iscsi_target_parameters.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/target/iscsi/iscsi_target_parameters.c b/drivers/target/iscsi/iscsi_target_parameters.c
> index 3a1f9a7..0a8bd3f 100644
> --- a/drivers/target/iscsi/iscsi_target_parameters.c
> +++ b/drivers/target/iscsi/iscsi_target_parameters.c
> @@ -127,9 +127,8 @@ static struct iscsi_param *iscsi_set_default_param(struct iscsi_param_list *para
>  		char *name, char *value, u8 phase, u8 scope, u8 sender,
>  		u16 type_range, u8 use)
>  {
> -	struct iscsi_param *param = NULL;
> +	struct iscsi_param *param = kzalloc(sizeof(*param), GFP_KERNEL);
>  
> -	param = kzalloc(sizeof(struct iscsi_param), GFP_KERNEL);
>  	if (!param) {
>  		pr_err("Unable to allocate memory for parameter.\n");
>  		goto out;

It's better to just get rid of the initialization but leave the
kzalloc() as-is for two reasons.

1)  Initializer code normally contains more bugs per line than other
    code.  I am thinking about dereferencing pointers before checking
    for NULL or not checking the allocation for failure.

2)  It puts a blank line between the allocation and the check for
    failure.  It's like a new paragraph.  The allocation and the check
    should be next to each other.

regards,
dan carpenter

Re: [PATCH 1/7] iscsi-target: Use a variable initialisation in iscsi_set_default_param() directly

[SF Markus Elfring]

>> @@ -127,9 +127,8 @@ static struct iscsi_param *iscsi_set_default_param(struct iscsi_param_list *para
>>  		char *name, char *value, u8 phase, u8 scope, u8 sender,
>>  		u16 type_range, u8 use)
>>  {
>> -	struct iscsi_param *param = NULL;
>> +	struct iscsi_param *param = kzalloc(sizeof(*param), GFP_KERNEL);
>>  
>> -	param = kzalloc(sizeof(struct iscsi_param), GFP_KERNEL);
>>  	if (!param) {
>>  		pr_err("Unable to allocate memory for parameter.\n");
>>  		goto out;
> 
> It's better to just get rid of the initialization but leave the
> kzalloc() as-is for two reasons.
> 
> 1)  Initializer code normally contains more bugs per line than other
>     code.  I am thinking about dereferencing pointers before checking
>     for NULL or not checking the allocation for failure.

I can follow your concerns a bit.


> 2)  It puts a blank line between the allocation and the check for failure.

Is there a target conflict between "convenient" variable initialisation
in the declaration section and the function outline that seems to be checked
by the script "checkpatch.pl" to some degree while corresponding preferences
or recommendations are not mentioned in the document "CodingStyle"?


>     It's like a new paragraph.

I do not see the separation in a strict way so far.


>     The allocation and the check should be next to each other.

I find that these actions are still close enough in the discussed use case.

Regards,
Markus

Re: [PATCH 1/7] iscsi-target: Use a variable initialisation in iscsi_set_default_param() directly

[Johannes Thumshirn]

On Sat, Dec 12, 2015 at 10:49:40PM +0300, Dan Carpenter wrote:
> On Sat, Dec 12, 2015 at 03:34:50PM +0100, SF Markus Elfring wrote:
> > From: Markus Elfring <elfring@users.sourceforge.net>
> > Date: Sat, 12 Dec 2015 11:36:02 +0100
> > 
> > Omit the unnecessary setting to a null pointer for the variable "param"
> > at the beginning of the function "iscsi_set_default_param"
> > because it can be directly initialized with the return value
> > from the function "kzalloc".
> > 
> > Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> > ---
> >  drivers/target/iscsi/iscsi_target_parameters.c | 3 +--
> >  1 file changed, 1 insertion(+), 2 deletions(-)
> > 
> > diff --git a/drivers/target/iscsi/iscsi_target_parameters.c b/drivers/target/iscsi/iscsi_target_parameters.c
> > index 3a1f9a7..0a8bd3f 100644
> > --- a/drivers/target/iscsi/iscsi_target_parameters.c
> > +++ b/drivers/target/iscsi/iscsi_target_parameters.c
> > @@ -127,9 +127,8 @@ static struct iscsi_param *iscsi_set_default_param(struct iscsi_param_list *para
> >  		char *name, char *value, u8 phase, u8 scope, u8 sender,
> >  		u16 type_range, u8 use)
> >  {
> > -	struct iscsi_param *param = NULL;
> > +	struct iscsi_param *param = kzalloc(sizeof(*param), GFP_KERNEL);
> >  
> > -	param = kzalloc(sizeof(struct iscsi_param), GFP_KERNEL);
> >  	if (!param) {
> >  		pr_err("Unable to allocate memory for parameter.\n");
> >  		goto out;
> 
> It's better to just get rid of the initialization but leave the
> kzalloc() as-is for two reasons.
> 
> 1)  Initializer code normally contains more bugs per line than other
>     code.  I am thinking about dereferencing pointers before checking
>     for NULL or not checking the allocation for failure.
> 
> 2)  It puts a blank line between the allocation and the check for
>     failure.  It's like a new paragraph.  The allocation and the check
>     should be next to each other.

I agree with Dan here. Please don't do it.

@@ -127,9 +127,8 @@ static struct iscsi_param *iscsi_set_default_param(struct iscsi_param_list *para
 		char *name, char *value, u8 phase, u8 scope, u8 sender,
 		u16 type_range, u8 use)
 {
-	struct iscsi_param *param = NULL;
+	struct iscsi_param *param;
 
	param = kzalloc(sizeof(struct iscsi_param), GFP_KERNEL);
 	if (!param) {
 		pr_err("Unable to allocate memory for parameter.\n");


This way it would be _far_ more readable. IMHO one should have a 1 action per
line of code style and only assign values in at declaration time if really 
necessary.

But what is the benefit from this? Is it fixing a (hypothetical) bug? I somehow
fail to see it.

Thanks,
	Johannes

-- 
Johannes Thumshirn                                          Storage
jthumshirn@suse.de                                +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850

Re: [PATCH 1/7] iscsi-target: Use a variable initialisation in iscsi_set_default_param() directly

[SF Markus Elfring]

> @@ -127,9 +127,8 @@ static struct iscsi_param *iscsi_set_default_param(struct iscsi_param_list *para
>  		char *name, char *value, u8 phase, u8 scope, u8 sender,
>  		u16 type_range, u8 use)
>  {
> -	struct iscsi_param *param = NULL;
> +	struct iscsi_param *param;
>  
> 	param = kzalloc(sizeof(struct iscsi_param), GFP_KERNEL);
>  	if (!param) {
>  		pr_err("Unable to allocate memory for parameter.\n");
> 
> 
> This way it would be _far_ more readable.

I guess that there are some opinions available for this implementation detail.


> IMHO one should have a 1 action per line of code style

How often do you care for such style issues?


> and only assign values in at declaration time if really necessary.

Which is or might become the official Linux coding style recommendation
for this aspect?


> But what is the benefit from this? Is it fixing a (hypothetical) bug?

I find the shown null pointer initialisation just needless.

Regards,
Markus

[PATCH 2/7] iscsi-target: Less checks in iscsi_set_default_param() after error detection

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sat, 12 Dec 2015 12:50:10 +0100

This issue was detected by using the Coccinelle software.

A sanity check would be performed by the iscsi_set_default_param() function
even if it is known already that the passed variable contained
a null pointer.

* This implementation detail could be improved by adjustments
  for jump targets according to the Linux coding style convention.

* Let us return directly if a call of the function "kzalloc" failed.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/target/iscsi/iscsi_target_parameters.c | 21 ++++++++++-----------
 1 file changed, 10 insertions(+), 11 deletions(-)

diff --git a/drivers/target/iscsi/iscsi_target_parameters.c b/drivers/target/iscsi/iscsi_target_parameters.c
index 0a8bd3f..15b2618 100644
--- a/drivers/target/iscsi/iscsi_target_parameters.c
+++ b/drivers/target/iscsi/iscsi_target_parameters.c
@@ -131,20 +131,20 @@ static struct iscsi_param *iscsi_set_default_param(struct iscsi_param_list *para
 
 	if (!param) {
 		pr_err("Unable to allocate memory for parameter.\n");
-		goto out;
+		return NULL;
 	}
 	INIT_LIST_HEAD(&param->p_list);
 
 	param->name = kstrdup(name, GFP_KERNEL);
 	if (!param->name) {
 		pr_err("Unable to allocate memory for parameter name.\n");
-		goto out;
+		goto free_param;
 	}
 
 	param->value = kstrdup(value, GFP_KERNEL);
 	if (!param->value) {
 		pr_err("Unable to allocate memory for parameter value.\n");
-		goto out;
+		goto free_name;
 	}
 
 	param->phase		= phase;
@@ -182,18 +182,17 @@ static struct iscsi_param *iscsi_set_default_param(struct iscsi_param_list *para
 	default:
 		pr_err("Unknown type_range 0x%02x\n",
 				param->type_range);
-		goto out;
+		goto free_value;
 	}
 	list_add_tail(&param->p_list, &param_list->param_list);
 
 	return param;
-out:
-	if (param) {
-		kfree(param->value);
-		kfree(param->name);
-		kfree(param);
-	}
-
+free_value:
+	kfree(param->value);
+free_name:
+	kfree(param->name);
+free_param:
+	kfree(param);
 	return NULL;
 }
 
-- 
2.6.3

[PATCH 3/7] iscsi-target: Delete an unnecessary variable initialisation in iscsi_create_default_params()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sat, 12 Dec 2015 13:20:08 +0100

The variable "param" will eventually be set to an appropriate pointer
from a call of the iscsi_set_default_param() function.
Thus let us omit the explicit initialisation at the beginning.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/target/iscsi/iscsi_target_parameters.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/target/iscsi/iscsi_target_parameters.c b/drivers/target/iscsi/iscsi_target_parameters.c
index 15b2618..e0b173d 100644
--- a/drivers/target/iscsi/iscsi_target_parameters.c
+++ b/drivers/target/iscsi/iscsi_target_parameters.c
@@ -199,7 +199,7 @@ free_param:
 /* #warning Add extension keys */
 int iscsi_create_default_params(struct iscsi_param_list **param_list_ptr)
 {
-	struct iscsi_param *param = NULL;
+	struct iscsi_param *param;
 	struct iscsi_param_list *pl;
 
 	pl = kzalloc(sizeof(struct iscsi_param_list), GFP_KERNEL);
-- 
2.6.3

[PATCH 4/7] iscsi-target: Make a variable initialisation a bit more obvious in iscsi_create_default_params()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sat, 12 Dec 2015 13:44:06 +0100

The variable "pl" was declared and immediately assigned a return value
from a function call in a separate statement.

* Let us express the desired variable initialisation directly.

* Avoid the repetition of the data type specification for the
  involved memory allocation according to the Linux coding
  style convention.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/target/iscsi/iscsi_target_parameters.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/target/iscsi/iscsi_target_parameters.c b/drivers/target/iscsi/iscsi_target_parameters.c
index e0b173d..3f3842f 100644
--- a/drivers/target/iscsi/iscsi_target_parameters.c
+++ b/drivers/target/iscsi/iscsi_target_parameters.c
@@ -200,9 +200,8 @@ free_param:
 int iscsi_create_default_params(struct iscsi_param_list **param_list_ptr)
 {
 	struct iscsi_param *param;
-	struct iscsi_param_list *pl;
+	struct iscsi_param_list *pl = kzalloc(sizeof(*pl), GFP_KERNEL);
 
-	pl = kzalloc(sizeof(struct iscsi_param_list), GFP_KERNEL);
 	if (!pl) {
 		pr_err("Unable to allocate memory for"
 				" struct iscsi_param_list.\n");
-- 
2.6.3

Re: [PATCH 4/7] iscsi-target: Make a variable initialisation a bit more obvious in iscsi_create_default_params()

[Julia Lawall]

On Sat, 12 Dec 2015, SF Markus Elfring wrote:

> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Sat, 12 Dec 2015 13:44:06 +0100
>
> The variable "pl" was declared and immediately assigned a return value
> from a function call in a separate statement.
>
> * Let us express the desired variable initialisation directly.
>
> * Avoid the repetition of the data type specification for the
>   involved memory allocation according to the Linux coding
>   style convention.
>
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>  drivers/target/iscsi/iscsi_target_parameters.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/drivers/target/iscsi/iscsi_target_parameters.c b/drivers/target/iscsi/iscsi_target_parameters.c
> index e0b173d..3f3842f 100644
> --- a/drivers/target/iscsi/iscsi_target_parameters.c
> +++ b/drivers/target/iscsi/iscsi_target_parameters.c
> @@ -200,9 +200,8 @@ free_param:
>  int iscsi_create_default_params(struct iscsi_param_list **param_list_ptr)
>  {
>  	struct iscsi_param *param;
> -	struct iscsi_param_list *pl;
> +	struct iscsi_param_list *pl = kzalloc(sizeof(*pl), GFP_KERNEL);
>
> -	pl = kzalloc(sizeof(struct iscsi_param_list), GFP_KERNEL);

I don't see the benefit of this change, and the pattern assignment ->
failure test becomes more obscure.

julia

>  	if (!pl) {
>  		pr_err("Unable to allocate memory for"
>  				" struct iscsi_param_list.\n");
> --
> 2.6.3
>
> --
> To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Re: [PATCH 4/7] iscsi-target: Make a variable initialisation a bit more obvious in iscsi_create_default_params()

[SF Markus Elfring]

>> @@ -200,9 +200,8 @@ free_param:
>>  int iscsi_create_default_params(struct iscsi_param_list **param_list_ptr)
>>  {
>>  	struct iscsi_param *param;
>> -	struct iscsi_param_list *pl;
>> +	struct iscsi_param_list *pl = kzalloc(sizeof(*pl), GFP_KERNEL);
>>
>> -	pl = kzalloc(sizeof(struct iscsi_param_list), GFP_KERNEL);
> 
> I don't see the benefit of this change, and the pattern assignment ->
> failure test becomes more obscure.

Are there any more software developers who prefer to specify
such a variable initialisation on a single line?

Does the proposed small source code reduction matter for you?

Regards,
Markus

[PATCH 5/7] iscsi-target: Rename a jump label in iscsi_create_default_params()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sat, 12 Dec 2015 14:12:50 +0100

This issue was detected by using the Coccinelle software.

Choose a jump label according to the current Linux coding style convention.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/target/iscsi/iscsi_target_parameters.c | 64 +++++++++++++-------------
 1 file changed, 32 insertions(+), 32 deletions(-)

diff --git a/drivers/target/iscsi/iscsi_target_parameters.c b/drivers/target/iscsi/iscsi_target_parameters.c
index 3f3842f..29ecf29 100644
--- a/drivers/target/iscsi/iscsi_target_parameters.c
+++ b/drivers/target/iscsi/iscsi_target_parameters.c
@@ -225,185 +225,185 @@ int iscsi_create_default_params(struct iscsi_param_list **param_list_ptr)
 			PHASE_SECURITY, SCOPE_CONNECTION_ONLY, SENDER_BOTH,
 			TYPERANGE_AUTH, USE_INITIAL_ONLY);
 	if (!param)
-		goto out;
+		goto release_list;
 
 	param = iscsi_set_default_param(pl, HEADERDIGEST, INITIAL_HEADERDIGEST,
 			PHASE_OPERATIONAL, SCOPE_CONNECTION_ONLY, SENDER_BOTH,
 			TYPERANGE_DIGEST, USE_INITIAL_ONLY);
 	if (!param)
-		goto out;
+		goto release_list;
 
 	param = iscsi_set_default_param(pl, DATADIGEST, INITIAL_DATADIGEST,
 			PHASE_OPERATIONAL, SCOPE_CONNECTION_ONLY, SENDER_BOTH,
 			TYPERANGE_DIGEST, USE_INITIAL_ONLY);
 	if (!param)
-		goto out;
+		goto release_list;
 
 	param = iscsi_set_default_param(pl, MAXCONNECTIONS,
 			INITIAL_MAXCONNECTIONS, PHASE_OPERATIONAL,
 			SCOPE_SESSION_WIDE, SENDER_BOTH,
 			TYPERANGE_1_TO_65535, USE_LEADING_ONLY);
 	if (!param)
-		goto out;
+		goto release_list;
 
 	param = iscsi_set_default_param(pl, SENDTARGETS, INITIAL_SENDTARGETS,
 			PHASE_FFP0, SCOPE_SESSION_WIDE, SENDER_INITIATOR,
 			TYPERANGE_UTF8, 0);
 	if (!param)
-		goto out;
+		goto release_list;
 
 	param = iscsi_set_default_param(pl, TARGETNAME, INITIAL_TARGETNAME,
 			PHASE_DECLARATIVE, SCOPE_SESSION_WIDE, SENDER_BOTH,
 			TYPERANGE_ISCSINAME, USE_ALL);
 	if (!param)
-		goto out;
+		goto release_list;
 
 	param = iscsi_set_default_param(pl, INITIATORNAME,
 			INITIAL_INITIATORNAME, PHASE_DECLARATIVE,
 			SCOPE_SESSION_WIDE, SENDER_INITIATOR,
 			TYPERANGE_ISCSINAME, USE_INITIAL_ONLY);
 	if (!param)
-		goto out;
+		goto release_list;
 
 	param = iscsi_set_default_param(pl, TARGETALIAS, INITIAL_TARGETALIAS,
 			PHASE_DECLARATIVE, SCOPE_SESSION_WIDE, SENDER_TARGET,
 			TYPERANGE_UTF8, USE_ALL);
 	if (!param)
-		goto out;
+		goto release_list;
 
 	param = iscsi_set_default_param(pl, INITIATORALIAS,
 			INITIAL_INITIATORALIAS, PHASE_DECLARATIVE,
 			SCOPE_SESSION_WIDE, SENDER_INITIATOR, TYPERANGE_UTF8,
 			USE_ALL);
 	if (!param)
-		goto out;
+		goto release_list;
 
 	param = iscsi_set_default_param(pl, TARGETADDRESS,
 			INITIAL_TARGETADDRESS, PHASE_DECLARATIVE,
 			SCOPE_SESSION_WIDE, SENDER_TARGET,
 			TYPERANGE_TARGETADDRESS, USE_ALL);
 	if (!param)
-		goto out;
+		goto release_list;
 
 	param = iscsi_set_default_param(pl, TARGETPORTALGROUPTAG,
 			INITIAL_TARGETPORTALGROUPTAG,
 			PHASE_DECLARATIVE, SCOPE_SESSION_WIDE, SENDER_TARGET,
 			TYPERANGE_0_TO_65535, USE_INITIAL_ONLY);
 	if (!param)
-		goto out;
+		goto release_list;
 
 	param = iscsi_set_default_param(pl, INITIALR2T, INITIAL_INITIALR2T,
 			PHASE_OPERATIONAL, SCOPE_SESSION_WIDE, SENDER_BOTH,
 			TYPERANGE_BOOL_OR, USE_LEADING_ONLY);
 	if (!param)
-		goto out;
+		goto release_list;
 
 	param = iscsi_set_default_param(pl, IMMEDIATEDATA,
 			INITIAL_IMMEDIATEDATA, PHASE_OPERATIONAL,
 			SCOPE_SESSION_WIDE, SENDER_BOTH, TYPERANGE_BOOL_AND,
 			USE_LEADING_ONLY);
 	if (!param)
-		goto out;
+		goto release_list;
 
 	param = iscsi_set_default_param(pl, MAXXMITDATASEGMENTLENGTH,
 			INITIAL_MAXXMITDATASEGMENTLENGTH,
 			PHASE_OPERATIONAL, SCOPE_CONNECTION_ONLY, SENDER_BOTH,
 			TYPERANGE_512_TO_16777215, USE_ALL);
 	if (!param)
-		goto out;
+		goto release_list;
 
 	param = iscsi_set_default_param(pl, MAXRECVDATASEGMENTLENGTH,
 			INITIAL_MAXRECVDATASEGMENTLENGTH,
 			PHASE_OPERATIONAL, SCOPE_CONNECTION_ONLY, SENDER_BOTH,
 			TYPERANGE_512_TO_16777215, USE_ALL);
 	if (!param)
-		goto out;
+		goto release_list;
 
 	param = iscsi_set_default_param(pl, MAXBURSTLENGTH,
 			INITIAL_MAXBURSTLENGTH, PHASE_OPERATIONAL,
 			SCOPE_SESSION_WIDE, SENDER_BOTH,
 			TYPERANGE_512_TO_16777215, USE_LEADING_ONLY);
 	if (!param)
-		goto out;
+		goto release_list;
 
 	param = iscsi_set_default_param(pl, FIRSTBURSTLENGTH,
 			INITIAL_FIRSTBURSTLENGTH,
 			PHASE_OPERATIONAL, SCOPE_SESSION_WIDE, SENDER_BOTH,
 			TYPERANGE_512_TO_16777215, USE_LEADING_ONLY);
 	if (!param)
-		goto out;
+		goto release_list;
 
 	param = iscsi_set_default_param(pl, DEFAULTTIME2WAIT,
 			INITIAL_DEFAULTTIME2WAIT,
 			PHASE_OPERATIONAL, SCOPE_SESSION_WIDE, SENDER_BOTH,
 			TYPERANGE_0_TO_3600, USE_LEADING_ONLY);
 	if (!param)
-		goto out;
+		goto release_list;
 
 	param = iscsi_set_default_param(pl, DEFAULTTIME2RETAIN,
 			INITIAL_DEFAULTTIME2RETAIN,
 			PHASE_OPERATIONAL, SCOPE_SESSION_WIDE, SENDER_BOTH,
 			TYPERANGE_0_TO_3600, USE_LEADING_ONLY);
 	if (!param)
-		goto out;
+		goto release_list;
 
 	param = iscsi_set_default_param(pl, MAXOUTSTANDINGR2T,
 			INITIAL_MAXOUTSTANDINGR2T,
 			PHASE_OPERATIONAL, SCOPE_SESSION_WIDE, SENDER_BOTH,
 			TYPERANGE_1_TO_65535, USE_LEADING_ONLY);
 	if (!param)
-		goto out;
+		goto release_list;
 
 	param = iscsi_set_default_param(pl, DATAPDUINORDER,
 			INITIAL_DATAPDUINORDER, PHASE_OPERATIONAL,
 			SCOPE_SESSION_WIDE, SENDER_BOTH, TYPERANGE_BOOL_OR,
 			USE_LEADING_ONLY);
 	if (!param)
-		goto out;
+		goto release_list;
 
 	param = iscsi_set_default_param(pl, DATASEQUENCEINORDER,
 			INITIAL_DATASEQUENCEINORDER,
 			PHASE_OPERATIONAL, SCOPE_SESSION_WIDE, SENDER_BOTH,
 			TYPERANGE_BOOL_OR, USE_LEADING_ONLY);
 	if (!param)
-		goto out;
+		goto release_list;
 
 	param = iscsi_set_default_param(pl, ERRORRECOVERYLEVEL,
 			INITIAL_ERRORRECOVERYLEVEL,
 			PHASE_OPERATIONAL, SCOPE_SESSION_WIDE, SENDER_BOTH,
 			TYPERANGE_0_TO_2, USE_LEADING_ONLY);
 	if (!param)
-		goto out;
+		goto release_list;
 
 	param = iscsi_set_default_param(pl, SESSIONTYPE, INITIAL_SESSIONTYPE,
 			PHASE_DECLARATIVE, SCOPE_SESSION_WIDE, SENDER_INITIATOR,
 			TYPERANGE_SESSIONTYPE, USE_LEADING_ONLY);
 	if (!param)
-		goto out;
+		goto release_list;
 
 	param = iscsi_set_default_param(pl, IFMARKER, INITIAL_IFMARKER,
 			PHASE_OPERATIONAL, SCOPE_CONNECTION_ONLY, SENDER_BOTH,
 			TYPERANGE_BOOL_AND, USE_INITIAL_ONLY);
 	if (!param)
-		goto out;
+		goto release_list;
 
 	param = iscsi_set_default_param(pl, OFMARKER, INITIAL_OFMARKER,
 			PHASE_OPERATIONAL, SCOPE_CONNECTION_ONLY, SENDER_BOTH,
 			TYPERANGE_BOOL_AND, USE_INITIAL_ONLY);
 	if (!param)
-		goto out;
+		goto release_list;
 
 	param = iscsi_set_default_param(pl, IFMARKINT, INITIAL_IFMARKINT,
 			PHASE_OPERATIONAL, SCOPE_CONNECTION_ONLY, SENDER_BOTH,
 			TYPERANGE_UTF8, USE_INITIAL_ONLY);
 	if (!param)
-		goto out;
+		goto release_list;
 
 	param = iscsi_set_default_param(pl, OFMARKINT, INITIAL_OFMARKINT,
 			PHASE_OPERATIONAL, SCOPE_CONNECTION_ONLY, SENDER_BOTH,
 			TYPERANGE_UTF8, USE_INITIAL_ONLY);
 	if (!param)
-		goto out;
+		goto release_list;
 
 	/*
 	 * Extra parameters for ISER from RFC-5046
@@ -412,25 +412,25 @@ int iscsi_create_default_params(struct iscsi_param_list **param_list_ptr)
 			PHASE_OPERATIONAL, SCOPE_SESSION_WIDE, SENDER_BOTH,
 			TYPERANGE_BOOL_AND, USE_LEADING_ONLY);
 	if (!param)
-		goto out;
+		goto release_list;
 
 	param = iscsi_set_default_param(pl, INITIATORRECVDATASEGMENTLENGTH,
 			INITIAL_INITIATORRECVDATASEGMENTLENGTH,
 			PHASE_OPERATIONAL, SCOPE_CONNECTION_ONLY, SENDER_BOTH,
 			TYPERANGE_512_TO_16777215, USE_ALL);
 	if (!param)
-		goto out;
+		goto release_list;
 
 	param = iscsi_set_default_param(pl, TARGETRECVDATASEGMENTLENGTH,
 			INITIAL_TARGETRECVDATASEGMENTLENGTH,
 			PHASE_OPERATIONAL, SCOPE_CONNECTION_ONLY, SENDER_BOTH,
 			TYPERANGE_512_TO_16777215, USE_ALL);
 	if (!param)
-		goto out;
+		goto release_list;
 
 	*param_list_ptr = pl;
 	return 0;
-out:
+release_list:
 	iscsi_release_param_list(pl);
 	return -1;
 }
-- 
2.6.3

[PATCH 6/7] iscsi-target: Delete unnecessary variable initialisations in iscsi_check_valuelist_for_support()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sat, 12 Dec 2015 14:34:26 +0100

The variables "tmp1" and "tmp2" will eventually be set to appropriate
pointers from a call of the strchr() function.
Thus let us omit the explicit initialisation at the beginning.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/target/iscsi/iscsi_target_parameters.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/target/iscsi/iscsi_target_parameters.c b/drivers/target/iscsi/iscsi_target_parameters.c
index 29ecf29..53e3345 100644
--- a/drivers/target/iscsi/iscsi_target_parameters.c
+++ b/drivers/target/iscsi/iscsi_target_parameters.c
@@ -920,7 +920,7 @@ static char *iscsi_check_valuelist_for_support(
 	struct iscsi_param *param,
 	char *value)
 {
-	char *tmp1 = NULL, *tmp2 = NULL;
+	char *tmp1, *tmp2;
 	char *acceptor_values = NULL, *proposer_values = NULL;
 
 	acceptor_values = param->value;
-- 
2.6.3

[PATCH 7/7] iscsi-target: Make two variable initialisations a bit more obvious in iscsi_check_valuelist_for_support()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sat, 12 Dec 2015 15:04:57 +0100

The variable "acceptor_values" and "proposer_values" were initialized
by null pointers and immediately assigned values from input parameters
by separate statements.
Let us express the desired variable initialisations directly.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/target/iscsi/iscsi_target_parameters.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/target/iscsi/iscsi_target_parameters.c b/drivers/target/iscsi/iscsi_target_parameters.c
index 53e3345..fb6fd34 100644
--- a/drivers/target/iscsi/iscsi_target_parameters.c
+++ b/drivers/target/iscsi/iscsi_target_parameters.c
@@ -921,10 +921,7 @@ static char *iscsi_check_valuelist_for_support(
 	char *value)
 {
 	char *tmp1, *tmp2;
-	char *acceptor_values = NULL, *proposer_values = NULL;
-
-	acceptor_values = param->value;
-	proposer_values = value;
+	char *acceptor_values = param->value, *proposer_values = value;
 
 	do {
 		if (!proposer_values)
-- 
2.6.3

Re: [PATCH 7/7] iscsi-target: Make two variable initialisations a bit more obvious in iscsi_check_valuelist_for_support()

[walter harms]

Am 12.12.2015 15:45, schrieb SF Markus Elfring:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Sat, 12 Dec 2015 15:04:57 +0100
> 
> The variable "acceptor_values" and "proposer_values" were initialized
> by null pointers and immediately assigned values from input parameters
> by separate statements.
> Let us express the desired variable initialisations directly.
> 
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>  drivers/target/iscsi/iscsi_target_parameters.c | 5 +----
>  1 file changed, 1 insertion(+), 4 deletions(-)
> 
> diff --git a/drivers/target/iscsi/iscsi_target_parameters.c b/drivers/target/iscsi/iscsi_target_parameters.c
> index 53e3345..fb6fd34 100644
> --- a/drivers/target/iscsi/iscsi_target_parameters.c
> +++ b/drivers/target/iscsi/iscsi_target_parameters.c
> @@ -921,10 +921,7 @@ static char *iscsi_check_valuelist_for_support(
>  	char *value)
>  {
>  	char *tmp1, *tmp2;
> -	char *acceptor_values = NULL, *proposer_values = NULL;
> -
> -	acceptor_values = param->value;
> -	proposer_values = value;
> +	char *acceptor_values = param->value, *proposer_values = value;
>  

I do not thing that this is a good idea,
i find the first version more readable
but you are right the NULL can be removed.

just my 2 cents,

re,
 wh



>  	do {
>  		if (!proposer_values)

[PATCH 0/5] block-cciss: Fine-tuning for two function implementations

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Thu, 18 Aug 2016 11:40:04 +0200

Some update suggestions were taken into account
from static source code analysis.

Markus Elfring (5):
  Use memdup_user()
  Less function calls after error detection
  Delete unnecessary initialisations
  Move an assignment for the variable "sg_used"
  Replace three kzalloc() calls by kcalloc()

 drivers/block/cciss.c | 66 ++++++++++++++++++++++++---------------------------
 1 file changed, 31 insertions(+), 35 deletions(-)

-- 
2.9.3

[PATCH 1/5] block-cciss: Use memdup_user() rather than duplicating its implementation

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Wed, 17 Aug 2016 22:10:29 +0200

* Reuse existing functionality from memdup_user() instead of keeping
  duplicate source code.

  This issue was detected by using the Coccinelle software.

* Return directly if this copy operation failed.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/block/cciss.c | 12 +++---------
 1 file changed, 3 insertions(+), 9 deletions(-)

diff --git a/drivers/block/cciss.c b/drivers/block/cciss.c
index db9d6bb..e044342 100644
--- a/drivers/block/cciss.c
+++ b/drivers/block/cciss.c
@@ -1587,15 +1587,9 @@ static int cciss_bigpassthru(ctlr_info_t *h, void __user *argp)
 		return -EINVAL;
 	if (!capable(CAP_SYS_RAWIO))
 		return -EPERM;
-	ioc = kmalloc(sizeof(*ioc), GFP_KERNEL);
-	if (!ioc) {
-		status = -ENOMEM;
-		goto cleanup1;
-	}
-	if (copy_from_user(ioc, argp, sizeof(*ioc))) {
-		status = -EFAULT;
-		goto cleanup1;
-	}
+	ioc = memdup_user(argp, sizeof(*ioc));
+	if (IS_ERR(ioc))
+		return PTR_ERR(ioc);
 	if ((ioc->buf_size < 1) &&
 	    (ioc->Request.Type.Direction != XFER_NONE)) {
 		status = -EINVAL;
-- 
2.9.3

[PATCH 2/5] block-cciss: Less function calls in cciss_bigpassthru() after error detection

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Wed, 17 Aug 2016 22:39:31 +0200

The kfree() function was called in a few cases by the
cciss_bigpassthru() function during error handling
even if a passed variable contained a null pointer.

Adjust jump targets according to the Linux coding style convention.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/block/cciss.c | 40 ++++++++++++++++++++--------------------
 1 file changed, 20 insertions(+), 20 deletions(-)

diff --git a/drivers/block/cciss.c b/drivers/block/cciss.c
index e044342..43ac632 100644
--- a/drivers/block/cciss.c
+++ b/drivers/block/cciss.c
@@ -1593,26 +1593,26 @@ static int cciss_bigpassthru(ctlr_info_t *h, void __user *argp)
 	if ((ioc->buf_size < 1) &&
 	    (ioc->Request.Type.Direction != XFER_NONE)) {
 		status = -EINVAL;
-		goto cleanup1;
+		goto free_ioc;
 	}
 	/* Check kmalloc limits  using all SGs */
 	if (ioc->malloc_size > MAX_KMALLOC_SIZE) {
 		status = -EINVAL;
-		goto cleanup1;
+		goto free_ioc;
 	}
 	if (ioc->buf_size > ioc->malloc_size * MAXSGENTRIES) {
 		status = -EINVAL;
-		goto cleanup1;
-	}
-	buff = kzalloc(MAXSGENTRIES * sizeof(char *), GFP_KERNEL);
-	if (!buff) {
-		status = -ENOMEM;
-		goto cleanup1;
+		goto free_ioc;
 	}
 	buff_size = kmalloc(MAXSGENTRIES * sizeof(int), GFP_KERNEL);
 	if (!buff_size) {
 		status = -ENOMEM;
-		goto cleanup1;
+		goto free_ioc;
+	}
+	buff = kzalloc(MAXSGENTRIES * sizeof(char *), GFP_KERNEL);
+	if (!buff) {
+		status = -ENOMEM;
+		goto free_size;
 	}
 	left = ioc->buf_size;
 	data_ptr = ioc->buf;
@@ -1622,12 +1622,12 @@ static int cciss_bigpassthru(ctlr_info_t *h, void __user *argp)
 		buff[sg_used] = kmalloc(sz, GFP_KERNEL);
 		if (buff[sg_used] == NULL) {
 			status = -ENOMEM;
-			goto cleanup1;
+			goto free_buffer;
 		}
 		if (ioc->Request.Type.Direction == XFER_WRITE) {
 			if (copy_from_user(buff[sg_used], data_ptr, sz)) {
 				status = -EFAULT;
-				goto cleanup1;
+				goto free_buffer;
 			}
 		} else {
 			memset(buff[sg_used], 0, sz);
@@ -1639,7 +1639,7 @@ static int cciss_bigpassthru(ctlr_info_t *h, void __user *argp)
 	c = cmd_special_alloc(h);
 	if (!c) {
 		status = -ENOMEM;
-		goto cleanup1;
+		goto free_buffer;
 	}
 	c->cmd_type = CMD_IOCTL_PEND;
 	c->Header.ReplyQueue = 0;
@@ -1674,7 +1674,7 @@ static int cciss_bigpassthru(ctlr_info_t *h, void __user *argp)
 	if (copy_to_user(argp, ioc, sizeof(*ioc))) {
 		cmd_special_free(h, c);
 		status = -EFAULT;
-		goto cleanup1;
+		goto free_buffer;
 	}
 	if (ioc->Request.Type.Direction == XFER_READ) {
 		/* Copy the data out of the buffer we created */
@@ -1683,20 +1683,20 @@ static int cciss_bigpassthru(ctlr_info_t *h, void __user *argp)
 			if (copy_to_user(ptr, buff[i], buff_size[i])) {
 				cmd_special_free(h, c);
 				status = -EFAULT;
-				goto cleanup1;
+				goto free_buffer;
 			}
 			ptr += buff_size[i];
 		}
 	}
 	cmd_special_free(h, c);
 	status = 0;
-cleanup1:
-	if (buff) {
-		for (i = 0; i < sg_used; i++)
-			kfree(buff[i]);
-		kfree(buff);
-	}
+free_buffer:
+	for (i = 0; i < sg_used; i++)
+		kfree(buff[i]);
+	kfree(buff);
+free_size:
 	kfree(buff_size);
+free_ioc:
 	kfree(ioc);
 	return status;
 }
-- 
2.9.3

[PATCH 3/5] block-cciss: Delete unnecessary initialisations in cciss_bigpassthru()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Wed, 17 Aug 2016 22:55:51 +0200

Three local variables will be set to an appropriate value a bit later.
Thus omit the explicit initialisation at the beginning.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/block/cciss.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/block/cciss.c b/drivers/block/cciss.c
index 43ac632..10e1b0a 100644
--- a/drivers/block/cciss.c
+++ b/drivers/block/cciss.c
@@ -1572,11 +1572,11 @@ static int cciss_bigpassthru(ctlr_info_t *h, void __user *argp)
 {
 	BIG_IOCTL_Command_struct *ioc;
 	CommandList_struct *c;
-	unsigned char **buff = NULL;
-	int *buff_size = NULL;
+	unsigned char **buff;
+	int *buff_size;
 	u64bit temp64;
 	BYTE sg_used = 0;
-	int status = 0;
+	int status;
 	int i;
 	DECLARE_COMPLETION_ONSTACK(wait);
 	__u32 left;
-- 
2.9.3

[PATCH 4/5] block-cciss: Move an assignment for the variable "sg_used" in cciss_bigpassthru()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Wed, 17 Aug 2016 23:04:46 +0200

Move the assignment for the local variable "sg_used" behind the source code
for some memory allocations by this function.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/block/cciss.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/block/cciss.c b/drivers/block/cciss.c
index 10e1b0a..b08bfb7 100644
--- a/drivers/block/cciss.c
+++ b/drivers/block/cciss.c
@@ -1575,7 +1575,7 @@ static int cciss_bigpassthru(ctlr_info_t *h, void __user *argp)
 	unsigned char **buff;
 	int *buff_size;
 	u64bit temp64;
-	BYTE sg_used = 0;
+	BYTE sg_used;
 	int status;
 	int i;
 	DECLARE_COMPLETION_ONSTACK(wait);
@@ -1616,6 +1616,7 @@ static int cciss_bigpassthru(ctlr_info_t *h, void __user *argp)
 	}
 	left = ioc->buf_size;
 	data_ptr = ioc->buf;
+	sg_used = 0;
 	while (left) {
 		sz = (left > ioc->malloc_size) ? ioc->malloc_size : left;
 		buff_size[sg_used] = sz;
-- 
2.9.3

[PATCH 5/5] block-cciss: Replace three kzalloc() calls by kcalloc()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Thu, 18 Aug 2016 11:26:18 +0200

* The script "checkpatch.pl" can point information out like the following.

  WARNING: Prefer kcalloc over kzalloc with multiply

  Thus fix the affected source code places.

* Replace the specification of data structures by pointer dereferences
  to make the corresponding size determination a bit safer according to
  the Linux coding style convention.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/block/cciss.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/block/cciss.c b/drivers/block/cciss.c
index b08bfb7..3502a3a 100644
--- a/drivers/block/cciss.c
+++ b/drivers/block/cciss.c
@@ -1604,12 +1604,12 @@ static int cciss_bigpassthru(ctlr_info_t *h, void __user *argp)
 		status = -EINVAL;
 		goto free_ioc;
 	}
-	buff_size = kmalloc(MAXSGENTRIES * sizeof(int), GFP_KERNEL);
+	buff_size = kcalloc(MAXSGENTRIES, sizeof(*buff_size), GFP_KERNEL);
 	if (!buff_size) {
 		status = -ENOMEM;
 		goto free_ioc;
 	}
-	buff = kzalloc(MAXSGENTRIES * sizeof(char *), GFP_KERNEL);
+	buff = kcalloc(MAXSGENTRIES, sizeof(*buff), GFP_KERNEL);
 	if (!buff) {
 		status = -ENOMEM;
 		goto free_size;
@@ -4838,8 +4838,9 @@ static int cciss_allocate_scatterlists(ctlr_info_t *h)
 	int i;
 
 	/* zero it, so that on free we need not know how many were alloc'ed */
-	h->scatter_list = kzalloc(h->max_commands *
-				sizeof(struct scatterlist *), GFP_KERNEL);
+	h->scatter_list = kcalloc(h->max_commands,
+				  sizeof(*h->scatter_list),
+				  GFP_KERNEL);
 	if (!h->scatter_list)
 		return -ENOMEM;
 
-- 
2.9.3

Re: [PATCH 0/5] block-cciss: Fine-tuning for two function implementations

[SF Markus Elfring]

> Date: Thu, 18 Aug 2016 11:40:04 +0200
> 
> Some update suggestions were taken into account
> from static source code analysis.
> 
> Markus Elfring (5):
>   Use memdup_user()
>   Less function calls after error detection
>   Delete unnecessary initialisations
>   Move an assignment for the variable "sg_used"
>   Replace three kzalloc() calls by kcalloc()
> 
>  drivers/block/cciss.c | 66 ++++++++++++++++++++++++---------------------------
>  1 file changed, 31 insertions(+), 35 deletions(-)

How will the clarification be continued for the shown change possibilities?

Regards,
Markus

[PATCH 0/7] aacraid: Fine-tuning for a few functions

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sun, 21 Aug 2016 09:03:21 +0200

Several update suggestions were taken into account
from static source code analysis.

Markus Elfring (7):
  Use memdup_user() rather than duplicating its implementation
  One function call less in aac_send_raw_srb() after error detection
  Delete unnecessary initialisations in aac_send_raw_srb()
  Delete unnecessary braces
  Add spaces after control flow keywords
  Improve determination of a few sizes
  Apply another recommendation from "checkpatch.pl"

 drivers/scsi/aacraid/commctrl.c | 140 +++++++++++++++++++---------------------
 1 file changed, 67 insertions(+), 73 deletions(-)

-- 
2.9.3

[PATCH 1/7] aacraid: Use memdup_user() rather than duplicating its implementation

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sat, 20 Aug 2016 20:05:24 +0200

Reuse existing functionality from memdup_user() instead of keeping
duplicate source code.

This issue was detected by using the Coccinelle software.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/scsi/aacraid/commctrl.c | 12 +++---------
 1 file changed, 3 insertions(+), 9 deletions(-)

diff --git a/drivers/scsi/aacraid/commctrl.c b/drivers/scsi/aacraid/commctrl.c
index 5648b71..1af3084 100644
--- a/drivers/scsi/aacraid/commctrl.c
+++ b/drivers/scsi/aacraid/commctrl.c
@@ -526,15 +526,9 @@ static int aac_send_raw_srb(struct aac_dev* dev, void __user * arg)
 		goto cleanup;
 	}
 
-	user_srbcmd = kmalloc(fibsize, GFP_KERNEL);
-	if (!user_srbcmd) {
-		dprintk((KERN_DEBUG"aacraid: Could not make a copy of the srb\n"));
-		rcode = -ENOMEM;
-		goto cleanup;
-	}
-	if(copy_from_user(user_srbcmd, user_srb,fibsize)){
-		dprintk((KERN_DEBUG"aacraid: Could not copy srb from user\n"));
-		rcode = -EFAULT;
+	user_srbcmd = memdup_user(user_srb, fibsize);
+	if (IS_ERR(user_srbcmd)) {
+		rcode = PTR_ERR(user_srbcmd);
 		goto cleanup;
 	}
 
-- 
2.9.3

RE: [PATCH 1/7] aacraid: Use memdup_user() rather than duplicating its implementation

[David Carroll]

> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Sat, 20 Aug 2016 20:05:24 +0200
> 
> Reuse existing functionality from memdup_user() instead of keeping duplicate
> source code.
> 
> This issue was detected by using the Coccinelle software.
> 
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>  drivers/scsi/aacraid/commctrl.c | 12 +++---------
>  1 file changed, 3 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/scsi/aacraid/commctrl.c b/drivers/scsi/aacraid/commctrl.c
> index 5648b71..1af3084 100644
> --- a/drivers/scsi/aacraid/commctrl.c
> +++ b/drivers/scsi/aacraid/commctrl.c
> @@ -526,15 +526,9 @@ static int aac_send_raw_srb(struct aac_dev* dev, void
> __user * arg)
>                 goto cleanup;
>         }
> 
> -       user_srbcmd = kmalloc(fibsize, GFP_KERNEL);
> -       if (!user_srbcmd) {
> -               dprintk((KERN_DEBUG"aacraid: Could not make a copy of the srb\n"));
> -               rcode = -ENOMEM;
> -               goto cleanup;
> -       }
> -       if(copy_from_user(user_srbcmd, user_srb,fibsize)){
> -               dprintk((KERN_DEBUG"aacraid: Could not copy srb from user\n"));
> -               rcode = -EFAULT;
> +       user_srbcmd = memdup_user(user_srb, fibsize);
> +       if (IS_ERR(user_srbcmd)) {
> +               rcode = PTR_ERR(user_srbcmd);
>                 goto cleanup;
>         }
> 
> --

Hi Markus,

Patch 2/7 should precede Patch 1/7, as falling into kfree() would not look pretty.

Thanks, -Dave

Re: [PATCH 1/7] aacraid: Use memdup_user() rather than duplicating its implementation

[SF Markus Elfring]

>> @@ -526,15 +526,9 @@ static int aac_send_raw_srb(struct aac_dev* dev, void
>> __user * arg)
>>                 goto cleanup;
>>         }
>>
>> -       user_srbcmd = kmalloc(fibsize, GFP_KERNEL);
>> -       if (!user_srbcmd) {
>> -               dprintk((KERN_DEBUG"aacraid: Could not make a copy of the srb\n"));
>> -               rcode = -ENOMEM;
>> -               goto cleanup;
>> -       }
>> -       if(copy_from_user(user_srbcmd, user_srb,fibsize)){
>> -               dprintk((KERN_DEBUG"aacraid: Could not copy srb from user\n"));
>> -               rcode = -EFAULT;
>> +       user_srbcmd = memdup_user(user_srb, fibsize);
>> +       if (IS_ERR(user_srbcmd)) {
>> +               rcode = PTR_ERR(user_srbcmd);
>>                 goto cleanup;
>>         }
>>
>> --
> 
> Hi Markus,
> 
> Patch 2/7 should precede Patch 1/7, as falling into kfree() would not look pretty.

Do you eventually prefer that this source code adjustment should be combined with
the update suggestion "[2/7] aacraid: One function call less in aac_send_raw_srb()
after error detection" in a single update step?

Regards,
Markus

RE: [PATCH 1/7] aacraid: Use memdup_user() rather than duplicating its implementation

[David Carroll]

> >
> > Hi Markus,
> >
> > Patch 2/7 should precede Patch 1/7, as falling into kfree() would not look
> pretty.
> 
> Do you eventually prefer that this source code adjustment should be combined
> with the update suggestion "[2/7] aacraid: One function call less in
> aac_send_raw_srb() after error detection" in a single update step?
> 

Hi Markus,

My primary objective in this would be the ability to bisect ... The secondary would be one issue/patch. I think my preference would be to swap patches 1 and 2.

Thanks, -Dave

[PATCH 2/7] aacraid: One function call less in aac_send_raw_srb() after error detection

[SF Markus Elfring]

>From e8187662ee30aab709a260c72fb86c51673f8e0d Mon Sep 17 00:00:00 2001
From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sat, 20 Aug 2016 20:40:47 +0200
Subject: [PATCH 2/7] aacraid: One function call less in aac_send_raw_srb()
 after error detection

The kfree() function was called in a few cases by the
aac_send_raw_srb() function during error handling
even if the variable "user_srbcmd" contained eventually
an inappropriate pointer value.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/scsi/aacraid/commctrl.c | 49 ++++++++++++++++++++---------------------
 1 file changed, 24 insertions(+), 25 deletions(-)

diff --git a/drivers/scsi/aacraid/commctrl.c b/drivers/scsi/aacraid/commctrl.c
index 1af3084..6dcdf91 100644
--- a/drivers/scsi/aacraid/commctrl.c
+++ b/drivers/scsi/aacraid/commctrl.c
@@ -517,19 +517,19 @@ static int aac_send_raw_srb(struct aac_dev* dev, void __user * arg)
 	if(copy_from_user(&fibsize, &user_srb->count,sizeof(u32))){
 		dprintk((KERN_DEBUG"aacraid: Could not copy data size from user\n"));
 		rcode = -EFAULT;
-		goto cleanup;
+		goto free_sg_list;
 	}
 
 	if ((fibsize < (sizeof(struct user_aac_srb) - sizeof(struct user_sgentry))) ||
 	    (fibsize > (dev->max_fib_size - sizeof(struct aac_fibhdr)))) {
 		rcode = -EINVAL;
-		goto cleanup;
+		goto free_sg_list;
 	}
 
 	user_srbcmd = memdup_user(user_srb, fibsize);
 	if (IS_ERR(user_srbcmd)) {
 		rcode = PTR_ERR(user_srbcmd);
-		goto cleanup;
+		goto free_sg_list;
 	}
 
 	user_reply = arg+fibsize;
@@ -564,7 +564,7 @@ static int aac_send_raw_srb(struct aac_dev* dev, void __user * arg)
 		dprintk((KERN_DEBUG"aacraid: too many sg entries %d\n",
 		  le32_to_cpu(srbcmd->sg.count)));
 		rcode = -EINVAL;
-		goto cleanup;
+		goto free_user_srbcmd;
 	}
 	actual_fibsize = sizeof(struct aac_srb) - sizeof(struct sgentry) +
 		((user_srbcmd->sg.count & 0xff) * sizeof(struct sgentry));
@@ -580,12 +580,12 @@ static int aac_send_raw_srb(struct aac_dev* dev, void __user * arg)
 		  sizeof(struct aac_srb), sizeof(struct sgentry),
 		  sizeof(struct sgentry64), fibsize));
 		rcode = -EINVAL;
-		goto cleanup;
+		goto free_user_srbcmd;
 	}
 	if ((data_dir == DMA_NONE) && user_srbcmd->sg.count) {
 		dprintk((KERN_DEBUG"aacraid: SG with no direction specified in Raw SRB command\n"));
 		rcode = -EINVAL;
-		goto cleanup;
+		goto free_user_srbcmd;
 	}
 	byte_count = 0;
 	if (dev->adapter_info.options & AAC_OPT_SGMAP_HOST64) {
@@ -606,7 +606,7 @@ static int aac_send_raw_srb(struct aac_dev* dev, void __user * arg)
 				      (dev->scsi_host_ptr->max_sectors << 9) :
 				      65536)) {
 					rcode = -EINVAL;
-					goto cleanup;
+					goto free_user_srbcmd;
 				}
 				/* Does this really need to be GFP_DMA? */
 				p = kmalloc(upsg->sg[i].count,GFP_KERNEL|__GFP_DMA);
@@ -614,7 +614,7 @@ static int aac_send_raw_srb(struct aac_dev* dev, void __user * arg)
 					dprintk((KERN_DEBUG"aacraid: Could not allocate SG buffer - size = %d buffer number %d of %d\n",
 					  upsg->sg[i].count,i,upsg->count));
 					rcode = -ENOMEM;
-					goto cleanup;
+					goto free_user_srbcmd;
 				}
 				addr = (u64)upsg->sg[i].addr[0];
 				addr += ((u64)upsg->sg[i].addr[1]) << 32;
@@ -626,7 +626,7 @@ static int aac_send_raw_srb(struct aac_dev* dev, void __user * arg)
 					if(copy_from_user(p,sg_user[i],upsg->sg[i].count)){
 						dprintk((KERN_DEBUG"aacraid: Could not copy sg data from user\n"));
 						rcode = -EFAULT;
-						goto cleanup;
+						goto free_user_srbcmd;
 					}
 				}
 				addr = pci_map_single(dev->pdev, p, upsg->sg[i].count, data_dir);
@@ -644,7 +644,7 @@ static int aac_send_raw_srb(struct aac_dev* dev, void __user * arg)
 			if (!usg) {
 				dprintk((KERN_DEBUG"aacraid: Allocation error in Raw SRB command\n"));
 				rcode = -ENOMEM;
-				goto cleanup;
+				goto free_user_srbcmd;
 			}
 			actual_fibsize = actual_fibsize64;
 
@@ -658,7 +658,7 @@ static int aac_send_raw_srb(struct aac_dev* dev, void __user * arg)
 				      65536)) {
 					kfree(usg);
 					rcode = -EINVAL;
-					goto cleanup;
+					goto free_user_srbcmd;
 				}
 				/* Does this really need to be GFP_DMA? */
 				p = kmalloc(usg->sg[i].count,GFP_KERNEL|__GFP_DMA);
@@ -667,7 +667,7 @@ static int aac_send_raw_srb(struct aac_dev* dev, void __user * arg)
 					  usg->sg[i].count,i,usg->count));
 					kfree(usg);
 					rcode = -ENOMEM;
-					goto cleanup;
+					goto free_user_srbcmd;
 				}
 				sg_user[i] = (void __user *)(uintptr_t)usg->sg[i].addr;
 				sg_list[i] = p; // save so we can clean up later
@@ -678,7 +678,7 @@ static int aac_send_raw_srb(struct aac_dev* dev, void __user * arg)
 						kfree (usg);
 						dprintk((KERN_DEBUG"aacraid: Could not copy sg data from user\n"));
 						rcode = -EFAULT;
-						goto cleanup;
+						goto free_user_srbcmd;
 					}
 				}
 				addr = pci_map_single(dev->pdev, p, usg->sg[i].count, data_dir);
@@ -711,7 +711,7 @@ static int aac_send_raw_srb(struct aac_dev* dev, void __user * arg)
 				      (dev->scsi_host_ptr->max_sectors << 9) :
 				      65536)) {
 					rcode = -EINVAL;
-					goto cleanup;
+					goto free_user_srbcmd;
 				}
 				/* Does this really need to be GFP_DMA? */
 				p = kmalloc(usg->sg[i].count,GFP_KERNEL|__GFP_DMA);
@@ -719,7 +719,7 @@ static int aac_send_raw_srb(struct aac_dev* dev, void __user * arg)
 					dprintk((KERN_DEBUG"aacraid: Could not allocate SG buffer - size = %d buffer number %d of %d\n",
 					  usg->sg[i].count,i,usg->count));
 					rcode = -ENOMEM;
-					goto cleanup;
+					goto free_user_srbcmd;
 				}
 				addr = (u64)usg->sg[i].addr[0];
 				addr += ((u64)usg->sg[i].addr[1]) << 32;
@@ -731,7 +731,7 @@ static int aac_send_raw_srb(struct aac_dev* dev, void __user * arg)
 					if(copy_from_user(p,sg_user[i],usg->sg[i].count)){
 						dprintk((KERN_DEBUG"aacraid: Could not copy sg data from user\n"));
 						rcode = -EFAULT;
-						goto cleanup;
+						goto free_user_srbcmd;
 					}
 				}
 				addr = pci_map_single(dev->pdev, p, usg->sg[i].count, data_dir);
@@ -750,14 +750,14 @@ static int aac_send_raw_srb(struct aac_dev* dev, void __user * arg)
 				      (dev->scsi_host_ptr->max_sectors << 9) :
 				      65536)) {
 					rcode = -EINVAL;
-					goto cleanup;
+					goto free_user_srbcmd;
 				}
 				p = kmalloc(upsg->sg[i].count, GFP_KERNEL);
 				if (!p) {
 					dprintk((KERN_DEBUG"aacraid: Could not allocate SG buffer - size = %d buffer number %d of %d\n",
 					  upsg->sg[i].count, i, upsg->count));
 					rcode = -ENOMEM;
-					goto cleanup;
+					goto free_user_srbcmd;
 				}
 				sg_user[i] = (void __user *)(uintptr_t)upsg->sg[i].addr;
 				sg_list[i] = p; // save so we can clean up later
@@ -768,7 +768,7 @@ static int aac_send_raw_srb(struct aac_dev* dev, void __user * arg)
 							upsg->sg[i].count)) {
 						dprintk((KERN_DEBUG"aacraid: Could not copy sg data from user\n"));
 						rcode = -EFAULT;
-						goto cleanup;
+						goto free_user_srbcmd;
 					}
 				}
 				addr = pci_map_single(dev->pdev, p,
@@ -788,13 +788,13 @@ static int aac_send_raw_srb(struct aac_dev* dev, void __user * arg)
 	}
 	if (status == -ERESTARTSYS) {
 		rcode = -ERESTARTSYS;
-		goto cleanup;
+		goto free_user_srbcmd;
 	}
 
 	if (status != 0){
 		dprintk((KERN_DEBUG"aacraid: Could not send raw srb fib to hba\n"));
 		rcode = -ENXIO;
-		goto cleanup;
+		goto free_user_srbcmd;
 	}
 
 	if (flags & SRB_DataIn) {
@@ -806,7 +806,7 @@ static int aac_send_raw_srb(struct aac_dev* dev, void __user * arg)
 			if(copy_to_user(sg_user[i], sg_list[i], byte_count)){
 				dprintk((KERN_DEBUG"aacraid: Could not copy sg data to user\n"));
 				rcode = -EFAULT;
-				goto cleanup;
+				goto free_user_srbcmd;
 
 			}
 		}
@@ -816,11 +816,10 @@ static int aac_send_raw_srb(struct aac_dev* dev, void __user * arg)
 	if(copy_to_user(user_reply,reply,sizeof(struct aac_srb_reply))){
 		dprintk((KERN_DEBUG"aacraid: Could not copy reply to user\n"));
 		rcode = -EFAULT;
-		goto cleanup;
 	}
-
-cleanup:
+free_user_srbcmd:
 	kfree(user_srbcmd);
+free_sg_list:
 	for(i=0; i <= sg_indx; i++){
 		kfree(sg_list[i]);
 	}
-- 
2.9.3

[PATCH 3/7] aacraid: Delete unnecessary initialisations in aac_send_raw_srb()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sat, 20 Aug 2016 21:25:20 +0200

Six local variables will be set to an appropriate value a bit later.
Thus omit the explicit initialisation at the beginning.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/scsi/aacraid/commctrl.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/scsi/aacraid/commctrl.c b/drivers/scsi/aacraid/commctrl.c
index 6dcdf91..49a664f 100644
--- a/drivers/scsi/aacraid/commctrl.c
+++ b/drivers/scsi/aacraid/commctrl.c
@@ -476,20 +476,20 @@ static int aac_send_raw_srb(struct aac_dev* dev, void __user * arg)
 {
 	struct fib* srbfib;
 	int status;
-	struct aac_srb *srbcmd = NULL;
-	struct user_aac_srb *user_srbcmd = NULL;
+	struct aac_srb *srbcmd;
+	struct user_aac_srb *user_srbcmd;
 	struct user_aac_srb __user *user_srb = arg;
 	struct aac_srb_reply __user *user_reply;
 	struct aac_srb_reply* reply;
-	u32 fibsize = 0;
-	u32 flags = 0;
+	u32 fibsize;
+	u32 flags;
 	s32 rcode = 0;
 	u32 data_dir;
 	void __user *sg_user[32];
 	void *sg_list[32];
 	u32 sg_indx = 0;
-	u32 byte_count = 0;
-	u32 actual_fibsize64, actual_fibsize = 0;
+	u32 byte_count;
+	u32 actual_fibsize64, actual_fibsize;
 	int i;
 
 
-- 
2.9.3

[PATCH 4/7] aacraid: Delete unnecessary braces

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sun, 21 Aug 2016 07:07:08 +0200

Do not use curly brackets at some source code places
where a single statement should be sufficient.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/scsi/aacraid/commctrl.c | 18 ++++++------------
 1 file changed, 6 insertions(+), 12 deletions(-)

diff --git a/drivers/scsi/aacraid/commctrl.c b/drivers/scsi/aacraid/commctrl.c
index 49a664f..9f4ddb0 100644
--- a/drivers/scsi/aacraid/commctrl.c
+++ b/drivers/scsi/aacraid/commctrl.c
@@ -66,13 +66,11 @@ static int ioctl_send_fib(struct aac_dev * dev, void __user *arg)
 	unsigned int size, osize;
 	int retval;
 
-	if (dev->in_reset) {
+	if (dev->in_reset)
 		return -EBUSY;
-	}
 	fibptr = aac_fib_alloc(dev);
-	if(fibptr == NULL) {
+	if (!fibptr)
 		return -ENOMEM;
-	}
 
 	kfib = fibptr->hw_fib_va;
 	/*
@@ -138,9 +136,8 @@ static int ioctl_send_fib(struct aac_dev * dev, void __user *arg)
 		retval = aac_fib_send(le16_to_cpu(kfib->header.Command), fibptr,
 				le16_to_cpu(kfib->header.Size) , FsaNormal,
 				1, 1, NULL, NULL);
-		if (retval) {
+		if (retval)
 			goto cleanup;
-		}
 		if (aac_fib_complete(fibptr) != 0) {
 			retval = -EINVAL;
 			goto cleanup;
@@ -228,12 +225,10 @@ static int open_getadapter_fib(struct aac_dev * dev, void __user *arg)
 		}
 		list_add_tail(&fibctx->next, &dev->fib_list);
 		spin_unlock_irqrestore(&dev->fib_lock, flags);
-		if (copy_to_user(arg, &fibctx->unique,
-						sizeof(fibctx->unique))) {
+		if (copy_to_user(arg, &fibctx->unique, sizeof(fibctx->unique)))
 			status = -EFAULT;
-		} else {
+		else
 			status = 0;
-		}
 	}
 	return status;
 }
@@ -820,9 +815,8 @@ static int aac_send_raw_srb(struct aac_dev* dev, void __user * arg)
 free_user_srbcmd:
 	kfree(user_srbcmd);
 free_sg_list:
-	for(i=0; i <= sg_indx; i++){
+	for (i = 0; i <= sg_indx; i++)
 		kfree(sg_list[i]);
-	}
 	if (rcode != -ERESTARTSYS) {
 		aac_fib_complete(srbfib);
 		aac_fib_free(srbfib);
-- 
2.9.3

[PATCH 5/7] aacraid: Add spaces after control flow keywords

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sun, 21 Aug 2016 07:10:43 +0200

Keywords which belong to the category "control flow" in the C programming
language should be followed by a space character according to
the Linux coding style convention.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/scsi/aacraid/commctrl.c | 41 ++++++++++++++++++++++++-----------------
 1 file changed, 24 insertions(+), 17 deletions(-)

diff --git a/drivers/scsi/aacraid/commctrl.c b/drivers/scsi/aacraid/commctrl.c
index 9f4ddb0..cda03f0 100644
--- a/drivers/scsi/aacraid/commctrl.c
+++ b/drivers/scsi/aacraid/commctrl.c
@@ -251,7 +251,7 @@ static int next_getadapter_fib(struct aac_dev * dev, void __user *arg)
 	struct list_head * entry;
 	unsigned long flags;
 
-	if(copy_from_user((void *)&f, arg, sizeof(struct fib_ioctl)))
+	if (copy_from_user((void *)&f, arg, sizeof(struct fib_ioctl)))
 		return -EFAULT;
 	/*
 	 *	Verify that the HANDLE passed in was a valid AdapterFibContext
@@ -280,7 +280,7 @@ static int next_getadapter_fib(struct aac_dev * dev, void __user *arg)
 		return -EINVAL;
 	}
 
-	if((fibctx->type != FSAFS_NTC_GET_ADAPTER_FIB_CONTEXT) ||
+	if ((fibctx->type != FSAFS_NTC_GET_ADAPTER_FIB_CONTEXT) ||
 		 (fibctx->size != sizeof(struct aac_fib_context))) {
 		spin_unlock_irqrestore(&dev->fib_lock, flags);
 		dprintk ((KERN_INFO "Fib Context corrupt?\n"));
@@ -327,7 +327,7 @@ return_fib:
 			ssleep(1);
 		}
 		if (f.wait) {
-			if(down_interruptible(&fibctx->wait_sem) < 0) {
+			if (down_interruptible(&fibctx->wait_sem) < 0) {
 				status = -ERESTARTSYS;
 			} else {
 				/* Lock again and retry */
@@ -404,7 +404,7 @@ static int close_getadapter_fib(struct aac_dev * dev, void __user *arg)
 	entry = dev->fib_list.next;
 	fibctx = NULL;
 
-	while(entry != &dev->fib_list) {
+	while (entry != &dev->fib_list) {
 		fibctx = list_entry(entry, struct aac_fib_context, next);
 		/*
 		 *	Extract the fibctx from the input parameters
@@ -418,7 +418,7 @@ static int close_getadapter_fib(struct aac_dev * dev, void __user *arg)
 	if (!fibctx)
 		return 0; /* Already gone */
 
-	if((fibctx->type != FSAFS_NTC_GET_ADAPTER_FIB_CONTEXT) ||
+	if ((fibctx->type != FSAFS_NTC_GET_ADAPTER_FIB_CONTEXT) ||
 		 (fibctx->size != sizeof(struct aac_fib_context)))
 		return -EINVAL;
 	spin_lock_irqsave(&dev->fib_lock, flags);
@@ -509,7 +509,7 @@ static int aac_send_raw_srb(struct aac_dev* dev, void __user * arg)
 	srbcmd = (struct aac_srb*) fib_data(srbfib);
 
 	memset(sg_list, 0, sizeof(sg_list)); /* cleanup may take issue */
-	if(copy_from_user(&fibsize, &user_srb->count,sizeof(u32))){
+	if (copy_from_user(&fibsize, &user_srb->count, sizeof(u32))) {
 		dprintk((KERN_DEBUG"aacraid: Could not copy data size from user\n"));
 		rcode = -EFAULT;
 		goto free_sg_list;
@@ -605,7 +605,7 @@ static int aac_send_raw_srb(struct aac_dev* dev, void __user * arg)
 				}
 				/* Does this really need to be GFP_DMA? */
 				p = kmalloc(upsg->sg[i].count,GFP_KERNEL|__GFP_DMA);
-				if(!p) {
+				if (!p) {
 					dprintk((KERN_DEBUG"aacraid: Could not allocate SG buffer - size = %d buffer number %d of %d\n",
 					  upsg->sg[i].count,i,upsg->count));
 					rcode = -ENOMEM;
@@ -618,7 +618,9 @@ static int aac_send_raw_srb(struct aac_dev* dev, void __user * arg)
 				sg_indx = i;
 
 				if (flags & SRB_DataOut) {
-					if(copy_from_user(p,sg_user[i],upsg->sg[i].count)){
+					if (copy_from_user(p,
+							   sg_user[i],
+							   upsg->sg[i].count)) {
 						dprintk((KERN_DEBUG"aacraid: Could not copy sg data from user\n"));
 						rcode = -EFAULT;
 						goto free_user_srbcmd;
@@ -657,7 +659,7 @@ static int aac_send_raw_srb(struct aac_dev* dev, void __user * arg)
 				}
 				/* Does this really need to be GFP_DMA? */
 				p = kmalloc(usg->sg[i].count,GFP_KERNEL|__GFP_DMA);
-				if(!p) {
+				if (!p) {
 					dprintk((KERN_DEBUG "aacraid: Could not allocate SG buffer - size = %d buffer number %d of %d\n",
 					  usg->sg[i].count,i,usg->count));
 					kfree(usg);
@@ -669,7 +671,9 @@ static int aac_send_raw_srb(struct aac_dev* dev, void __user * arg)
 				sg_indx = i;
 
 				if (flags & SRB_DataOut) {
-					if(copy_from_user(p,sg_user[i],upsg->sg[i].count)){
+					if (copy_from_user(p,
+							   sg_user[i],
+							   upsg->sg[i].count)) {
 						kfree (usg);
 						dprintk((KERN_DEBUG"aacraid: Could not copy sg data from user\n"));
 						rcode = -EFAULT;
@@ -710,7 +714,7 @@ static int aac_send_raw_srb(struct aac_dev* dev, void __user * arg)
 				}
 				/* Does this really need to be GFP_DMA? */
 				p = kmalloc(usg->sg[i].count,GFP_KERNEL|__GFP_DMA);
-				if(!p) {
+				if (!p) {
 					dprintk((KERN_DEBUG"aacraid: Could not allocate SG buffer - size = %d buffer number %d of %d\n",
 					  usg->sg[i].count,i,usg->count));
 					rcode = -ENOMEM;
@@ -723,7 +727,9 @@ static int aac_send_raw_srb(struct aac_dev* dev, void __user * arg)
 				sg_indx = i;
 
 				if (flags & SRB_DataOut) {
-					if(copy_from_user(p,sg_user[i],usg->sg[i].count)){
+					if (copy_from_user(p,
+							   sg_user[i],
+							   usg->sg[i].count)) {
 						dprintk((KERN_DEBUG"aacraid: Could not copy sg data from user\n"));
 						rcode = -EFAULT;
 						goto free_user_srbcmd;
@@ -759,8 +765,9 @@ static int aac_send_raw_srb(struct aac_dev* dev, void __user * arg)
 				sg_indx = i;
 
 				if (flags & SRB_DataOut) {
-					if(copy_from_user(p, sg_user[i],
-							upsg->sg[i].count)) {
+					if (copy_from_user(p,
+							   sg_user[i],
+							   upsg->sg[i].count)) {
 						dprintk((KERN_DEBUG"aacraid: Could not copy sg data from user\n"));
 						rcode = -EFAULT;
 						goto free_user_srbcmd;
@@ -793,12 +800,12 @@ static int aac_send_raw_srb(struct aac_dev* dev, void __user * arg)
 	}
 
 	if (flags & SRB_DataIn) {
-		for(i = 0 ; i <= sg_indx; i++){
+		for (i = 0 ; i <= sg_indx; i++) {
 			byte_count = le32_to_cpu(
 			  (dev->adapter_info.options & AAC_OPT_SGMAP_HOST64)
 			      ? ((struct sgmap64*)&srbcmd->sg)->sg[i].count
 			      : srbcmd->sg.sg[i].count);
-			if(copy_to_user(sg_user[i], sg_list[i], byte_count)){
+			if (copy_to_user(sg_user[i], sg_list[i], byte_count)) {
 				dprintk((KERN_DEBUG"aacraid: Could not copy sg data to user\n"));
 				rcode = -EFAULT;
 				goto free_user_srbcmd;
@@ -808,7 +815,7 @@ static int aac_send_raw_srb(struct aac_dev* dev, void __user * arg)
 	}
 
 	reply = (struct aac_srb_reply *) fib_data(srbfib);
-	if(copy_to_user(user_reply,reply,sizeof(struct aac_srb_reply))){
+	if (copy_to_user(user_reply, reply, sizeof(struct aac_srb_reply))) {
 		dprintk((KERN_DEBUG"aacraid: Could not copy reply to user\n"));
 		rcode = -EFAULT;
 	}
-- 
2.9.3

[PATCH 6/7] aacraid: Improve determination of a few sizes

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sun, 21 Aug 2016 08:04:48 +0200

Replace the specification of data structures by references for variables
as the parameter for the operator "sizeof" to make the corresponding size
determination a bit safer according to the Linux coding style convention.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/scsi/aacraid/commctrl.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/scsi/aacraid/commctrl.c b/drivers/scsi/aacraid/commctrl.c
index cda03f0..d2029db 100644
--- a/drivers/scsi/aacraid/commctrl.c
+++ b/drivers/scsi/aacraid/commctrl.c
@@ -177,7 +177,7 @@ static int open_getadapter_fib(struct aac_dev * dev, void __user *arg)
 	struct aac_fib_context * fibctx;
 	int status;
 
-	fibctx = kmalloc(sizeof(struct aac_fib_context), GFP_KERNEL);
+	fibctx = kmalloc(sizeof(*fibctx), GFP_KERNEL);
 	if (fibctx == NULL) {
 		status = -ENOMEM;
 	} else {
@@ -186,7 +186,7 @@ static int open_getadapter_fib(struct aac_dev * dev, void __user *arg)
 		struct aac_fib_context * context;
 
 		fibctx->type = FSAFS_NTC_GET_ADAPTER_FIB_CONTEXT;
-		fibctx->size = sizeof(struct aac_fib_context);
+		fibctx->size = sizeof(*fibctx);
 		/*
 		 *	Yes yes, I know this could be an index, but we have a
 		 * better guarantee of uniqueness for the locked loop below.
@@ -251,7 +251,7 @@ static int next_getadapter_fib(struct aac_dev * dev, void __user *arg)
 	struct list_head * entry;
 	unsigned long flags;
 
-	if (copy_from_user((void *)&f, arg, sizeof(struct fib_ioctl)))
+	if (copy_from_user(&f, arg, sizeof(f)))
 		return -EFAULT;
 	/*
 	 *	Verify that the HANDLE passed in was a valid AdapterFibContext
@@ -509,7 +509,7 @@ static int aac_send_raw_srb(struct aac_dev* dev, void __user * arg)
 	srbcmd = (struct aac_srb*) fib_data(srbfib);
 
 	memset(sg_list, 0, sizeof(sg_list)); /* cleanup may take issue */
-	if (copy_from_user(&fibsize, &user_srb->count, sizeof(u32))) {
+	if (copy_from_user(&fibsize, &user_srb->count, sizeof(fibsize))) {
 		dprintk((KERN_DEBUG"aacraid: Could not copy data size from user\n"));
 		rcode = -EFAULT;
 		goto free_sg_list;
-- 
2.9.3

[PATCH 7/7] aacraid: Apply another recommendation from "checkpatch.pl"

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sun, 21 Aug 2016 08:23:25 +0200

The script "checkpatch.pl" can point out that assignments should usually
not be performed within condition checks.
Thus move the assignment for the variable "srbfib" to a separate statement.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/scsi/aacraid/commctrl.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/aacraid/commctrl.c b/drivers/scsi/aacraid/commctrl.c
index d2029db..7e6c76d 100644
--- a/drivers/scsi/aacraid/commctrl.c
+++ b/drivers/scsi/aacraid/commctrl.c
@@ -499,9 +499,9 @@ static int aac_send_raw_srb(struct aac_dev* dev, void __user * arg)
 	/*
 	 *	Allocate and initialize a Fib then setup a SRB command
 	 */
-	if (!(srbfib = aac_fib_alloc(dev))) {
+	srbfib = aac_fib_alloc(dev);
+	if (!srbfib)
 		return -ENOMEM;
-	}
 	aac_fib_init(srbfib);
 	/* raw_srb FIB is not FastResponseCapable */
 	srbfib->hw_fib_va->header.XferState &= ~cpu_to_le32(FastResponseCapable);
-- 
2.9.3

[PATCH] megaraid_sas: Use memdup_user() rather than duplicating its implementation

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sun, 21 Aug 2016 10:39:04 +0200

Reuse existing functionality from memdup_user() instead of keeping
duplicate source code.

This issue was detected by using the Coccinelle software.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/scsi/megaraid/megaraid_sas_base.c | 11 +++--------
 1 file changed, 3 insertions(+), 8 deletions(-)

diff --git a/drivers/scsi/megaraid/megaraid_sas_base.c b/drivers/scsi/megaraid/megaraid_sas_base.c
index c1ed25a..9a2fe4e 100644
--- a/drivers/scsi/megaraid/megaraid_sas_base.c
+++ b/drivers/scsi/megaraid/megaraid_sas_base.c
@@ -6711,14 +6711,9 @@ static int megasas_mgmt_ioctl_fw(struct file *file, unsigned long arg)
 	unsigned long flags;
 	u32 wait_time = MEGASAS_RESET_WAIT_TIME;
 
-	ioc = kmalloc(sizeof(*ioc), GFP_KERNEL);
-	if (!ioc)
-		return -ENOMEM;
-
-	if (copy_from_user(ioc, user_ioc, sizeof(*ioc))) {
-		error = -EFAULT;
-		goto out_kfree_ioc;
-	}
+	ioc = memdup_user(user_ioc, sizeof(*ioc));
+	if (IS_ERR(ioc))
+		return PTR_ERR(ioc);
 
 	instance = megasas_lookup_instance(ioc->host_no);
 	if (!instance) {
-- 
2.9.3

RE: [PATCH] megaraid_sas: Use memdup_user() rather than duplicating its implementation

[Sumit Saxena]

>-----Original Message-----
>From: SF Markus Elfring [mailto:elfring@users.sourceforge.net]
>Sent: Sunday, August 21, 2016 2:19 PM
>To: linux-scsi@vger.kernel.org; megaraidlinux.pdl@avagotech.com; James E.
>J.
>Bottomley; Kashyap Desai; Martin K. Petersen; Sumit Saxena; Uday Lingala
>Cc: LKML; kernel-janitors@vger.kernel.org; Julia Lawall
>Subject: [PATCH] megaraid_sas: Use memdup_user() rather than duplicating
>its
>implementation
>
>From: Markus Elfring <elfring@users.sourceforge.net>
>Date: Sun, 21 Aug 2016 10:39:04 +0200
>
>Reuse existing functionality from memdup_user() instead of keeping
>duplicate
>source code.
>
>This issue was detected by using the Coccinelle software.
>
>Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
>---
> drivers/scsi/megaraid/megaraid_sas_base.c | 11 +++--------
> 1 file changed, 3 insertions(+), 8 deletions(-)
>
>diff --git a/drivers/scsi/megaraid/megaraid_sas_base.c
>b/drivers/scsi/megaraid/megaraid_sas_base.c
>index c1ed25a..9a2fe4e 100644
>--- a/drivers/scsi/megaraid/megaraid_sas_base.c
>+++ b/drivers/scsi/megaraid/megaraid_sas_base.c
>@@ -6711,14 +6711,9 @@ static int megasas_mgmt_ioctl_fw(struct file *file,
>unsigned long arg)
> 	unsigned long flags;
> 	u32 wait_time = MEGASAS_RESET_WAIT_TIME;
>
>-	ioc = kmalloc(sizeof(*ioc), GFP_KERNEL);
>-	if (!ioc)
>-		return -ENOMEM;
>-
>-	if (copy_from_user(ioc, user_ioc, sizeof(*ioc))) {
>-		error = -EFAULT;
>-		goto out_kfree_ioc;
>-	}
>+	ioc = memdup_user(user_ioc, sizeof(*ioc));
>+	if (IS_ERR(ioc))
>+		return PTR_ERR(ioc);
>
> 	instance = megasas_lookup_instance(ioc->host_no);
> 	if (!instance) {

Acked by: Sumit Saxena <sumit.saxena@broadcom.com>

>--
>2.9.3

Re: [PATCH] megaraid_sas: Use memdup_user() rather than duplicating its implementation

[Martin K. Petersen]

>>>>> "SF" == SF Markus Elfring <elfring@users.sourceforge.net> writes:

SF> Reuse existing functionality from memdup_user() instead of keeping
SF> duplicate source code.

Applied to 4.9/scsi-queue.

-- 
Martin K. Petersen	Oracle Linux Engineering