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

Groups > linux.kernel > #1562934 > unrolled thread

[PATCH 0/8] PowerPC-NVRAM: Fine-tuning for some function implementations

Back to article view | Back to linux.kernel

Contents

  [PATCH 0/8] PowerPC-NVRAM: Fine-tuning for some function  implementations SF Markus Elfring <elfring@users.sourceforge.net> - 2017-01-19 18:00 +0100
    [PATCH 7/8] powerpc/nvram: Improve size determinations in three  functions SF Markus Elfring <elfring@users.sourceforge.net> - 2017-01-19 18:10 +0100
    [PATCH 3/8] powerpc/nvram: Move an assignment for the variable "ret"  in dev_nvram_write() SF Markus Elfring <elfring@users.sourceforge.net> - 2017-01-19 18:10 +0100
      Re: [PATCH 3/8] powerpc/nvram: Move an assignment for the variable  "ret" in dev_nvram_write() Tyrel Datwyler <tyreld@linux.vnet.ibm.com> - 2017-01-20 01:30 +0100
        Re: powerpc/nvram: Move an assignment for the variable "ret" in  dev_nvram_write() SF Markus Elfring <elfring@users.sourceforge.net> - 2017-01-20 08:10 +0100
          Re: powerpc/nvram: Move an assignment for the variable "ret" in  dev_nvram_write() Tyrel Datwyler <tyreld@linux.vnet.ibm.com> - 2017-01-20 22:00 +0100
    [PATCH 6/8] powerpc/nvram: Delete three error messages for a failed  memory allocation SF Markus Elfring <elfring@users.sourceforge.net> - 2017-01-19 18:10 +0100
    [PATCH 8/8] powerpc/nvram: Move an assignment for the variable "err"  in nvram_scan_partitions() SF Markus Elfring <elfring@users.sourceforge.net> - 2017-01-19 18:40 +0100
    [PATCH 5/8] powerpc/nvram: Return directly after a failed kmalloc()  in dev_nvram_read() SF Markus Elfring <elfring@users.sourceforge.net> - 2017-01-19 21:50 +0100

#1562934 — [PATCH 0/8] PowerPC-NVRAM: Fine-tuning for some function implementations

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Thu, 19 Jan 2017 17:41:23 +0100

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

Markus Elfring (8):
  Return directly after a failed parameter validation in dev_nvram_write()
  Return directly after a failed kmalloc() in dev_nvram_write()
  Move an assignment for the variable "ret" in dev_nvram_write()
  Return directly after a failed parameter validation in dev_nvram_read()
  Return directly after a failed kmalloc() in dev_nvram_read()
  Delete three error messages for a failed memory allocation
  Improve size determinations in three functions
  Move an assignment for the variable "err" in nvram_scan_partitions()

 arch/powerpc/kernel/nvram_64.c | 63 +++++++++++++++---------------------------
 1 file changed, 22 insertions(+), 41 deletions(-)

-- 
2.11.0

[toc] | [next] | [standalone]

#1562940 — [PATCH 7/8] powerpc/nvram: Improve size determinations in three functions

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Thu, 19 Jan 2017 17:15:30 +0100

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

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 arch/powerpc/kernel/nvram_64.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/arch/powerpc/kernel/nvram_64.c b/arch/powerpc/kernel/nvram_64.c
index 7af1baaaf01b..ed54147e3c60 100644
--- a/arch/powerpc/kernel/nvram_64.c
+++ b/arch/powerpc/kernel/nvram_64.c
@@ -207,8 +207,7 @@ int nvram_write_os_partition(struct nvram_os_partition *part,
 
 	tmp_index = part->index;
 
-	rc = ppc_md.nvram_write((char *)&info, sizeof(struct err_log_info),
-				&tmp_index);
+	rc = ppc_md.nvram_write((char *)&info, sizeof(info), &tmp_index);
 	if (rc <= 0) {
 		pr_err("%s: Failed nvram_write (%d)\n", __func__, rc);
 		return rc;
@@ -244,9 +243,7 @@ int nvram_read_partition(struct nvram_os_partition *part, char *buff,
 	tmp_index = part->index;
 
 	if (part->os_partition) {
-		rc = ppc_md.nvram_read((char *)&info,
-					sizeof(struct err_log_info),
-					&tmp_index);
+		rc = ppc_md.nvram_read((char *)&info, sizeof(info), &tmp_index);
 		if (rc <= 0) {
 			pr_err("%s: Failed nvram_read (%d)\n", __func__, rc);
 			return rc;
@@ -1175,7 +1172,7 @@ int __init nvram_scan_partitions(void)
 			       "detected: 0-length partition\n");
 			goto out;
 		}
-		tmp_part = kmalloc(sizeof(struct nvram_partition), GFP_KERNEL);
+		tmp_part = kmalloc(sizeof(*tmp_part), GFP_KERNEL);
 		err = -ENOMEM;
 		if (!tmp_part)
 			goto out;
-- 
2.11.0

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

#1562944 — [PATCH 3/8] powerpc/nvram: Move an assignment for the variable "ret" in dev_nvram_write()

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Thu, 19 Jan 2017 15:55:36 +0100

A local variable was set to an error code before a concrete error situation
was detected. Thus move the corresponding assignment into an if branch
to indicate a software failure there.

This issue was detected by using the Coccinelle software.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 arch/powerpc/kernel/nvram_64.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/arch/powerpc/kernel/nvram_64.c b/arch/powerpc/kernel/nvram_64.c
index cf839adf3aa7..dc90a0e9ad65 100644
--- a/arch/powerpc/kernel/nvram_64.c
+++ b/arch/powerpc/kernel/nvram_64.c
@@ -806,9 +806,10 @@ static ssize_t dev_nvram_write(struct file *file, const char __user *buf,
 	if (!tmp)
 		return -ENOMEM;
 
-	ret = -EFAULT;
-	if (copy_from_user(tmp, buf, count))
+	if (copy_from_user(tmp, buf, count)) {
+		ret = -EFAULT;
 		goto out;
+	}
 
 	ret = ppc_md.nvram_write(tmp, count, ppos);
 
-- 
2.11.0

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

#1563219 — Re: [PATCH 3/8] powerpc/nvram: Move an assignment for the variable "ret" in dev_nvram_write()

On 01/19/2017 08:56 AM, SF Markus Elfring wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Thu, 19 Jan 2017 15:55:36 +0100
> 
> A local variable was set to an error code before a concrete error situation
> was detected. Thus move the corresponding assignment into an if branch
> to indicate a software failure there.
> 
> This issue was detected by using the Coccinelle software.
> 
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>  arch/powerpc/kernel/nvram_64.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/powerpc/kernel/nvram_64.c b/arch/powerpc/kernel/nvram_64.c
> index cf839adf3aa7..dc90a0e9ad65 100644
> --- a/arch/powerpc/kernel/nvram_64.c
> +++ b/arch/powerpc/kernel/nvram_64.c
> @@ -806,9 +806,10 @@ static ssize_t dev_nvram_write(struct file *file, const char __user *buf,
>  	if (!tmp)
>  		return -ENOMEM;
> 
> -	ret = -EFAULT;
> -	if (copy_from_user(tmp, buf, count))
> +	if (copy_from_user(tmp, buf, count)) {
> +		ret = -EFAULT;
>  		goto out;
> +	}
>
>  	ret = ppc_md.nvram_write(tmp, count, ppos);
> 

I think you really could have squashed patches 1-3 into a single patch
that returns directly after any failure. After this 3rd patch this is
now the only spot that branches to the "out" label. At this point you
might as well remove that label and move the kfree(tmp) call up and
return directly after the failure and at the nvram_write() call site
doing away completely with the "ret" variable.

Something like the following patch:

diff --git a/arch/powerpc/kernel/nvram_64.c b/arch/powerpc/kernel/nvram_64.c
index d5e2b83..eadb55c 100644
--- a/arch/powerpc/kernel/nvram_64.c
+++ b/arch/powerpc/kernel/nvram_64.c
@@ -789,37 +789,29 @@ static ssize_t dev_nvram_read(struct file *file,
char __user *buf,
 static ssize_t dev_nvram_write(struct file *file, const char __user *buf,
                          size_t count, loff_t *ppos)
 {
-       ssize_t ret;
-       char *tmp = NULL;
+       char *tmp;
        ssize_t size;

-       ret = -ENODEV;
        if (!ppc_md.nvram_size)
-               goto out;
+               return -ENODEV;

-       ret = 0;
        size = ppc_md.nvram_size();
        if (*ppos >= size || size < 0)
-               goto out;
+               return 0;

        count = min_t(size_t, count, size - *ppos);
        count = min(count, PAGE_SIZE);

-       ret = -ENOMEM;
        tmp = kmalloc(count, GFP_KERNEL);
        if (!tmp)
-               goto out;
-
-       ret = -EFAULT;
-       if (copy_from_user(tmp, buf, count))
-               goto out;
-
-       ret = ppc_md.nvram_write(tmp, count, ppos);
+               return -ENOMEM;

-out:
-       kfree(tmp);
-       return ret;
+       if (copy_from_user(tmp, buf, count)) {
+               kfree(tmp);
+               return -EFAULT;
+       }

+       return ppc_md.nvram_write(tmp, count, ppos);
 }

 static long dev_nvram_ioctl(struct file *file, unsigned int cmd,

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

#1563334 — Re: powerpc/nvram: Move an assignment for the variable "ret" in dev_nvram_write()

> I think you really could have squashed patches 1-3 into a single patch
> that returns directly after any failure.

Thanks for your constructive feedback.

I have got software development concerns around such patch squashing.


> At this point you might as well remove that label and move the kfree(tmp) call up
> and return directly after the failure and at the nvram_write() call site
> doing away completely with the "ret" variable.

Your idea might look nice at first glance. But I would interpret the previous
implementation of the discussed function in the way that the memory which was
dynamically allocated here should always (not only in the failure case) be released
before returning here.

Would you really like to change the life time for this “temporary” data item?

Regards,
Markus

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

#1563914 — Re: powerpc/nvram: Move an assignment for the variable "ret" in dev_nvram_write()

On 01/19/2017 11:08 PM, SF Markus Elfring wrote:
>> I think you really could have squashed patches 1-3 into a single patch
>> that returns directly after any failure.
> 
> Thanks for your constructive feedback.
> 
> I have got software development concerns around such patch squashing.
> 
> 
>> At this point you might as well remove that label and move the kfree(tmp) call up
>> and return directly after the failure and at the nvram_write() call site
>> doing away completely with the "ret" variable.
> 
> Your idea might look nice at first glance. But I would interpret the previous
> implementation of the discussed function in the way that the memory which was
> dynamically allocated here should always (not only in the failure case) be released
> before returning here.

You are correct. I did muck that part up. However, I do still believe it
is cleaner to squash your three patches together. There is no functional
change here and it is clearer in a single patch that you are modifying
the function to return directly in the simple error cases.

-Tyrel

> 
> Would you really like to change the life time for this “temporary” data item?
> 
> Regards,
> Markus
> 

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

#1562946 — [PATCH 6/8] powerpc/nvram: Delete three error messages for a failed memory allocation

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Thu, 19 Jan 2017 16:56:46 +0100

The script "checkpatch.pl" pointed information out like the following.

WARNING: Possible unnecessary 'out of memory' message

Thus fix affected source code places.

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

diff --git a/arch/powerpc/kernel/nvram_64.c b/arch/powerpc/kernel/nvram_64.c
index 68b970bcf2fc..7af1baaaf01b 100644
--- a/arch/powerpc/kernel/nvram_64.c
+++ b/arch/powerpc/kernel/nvram_64.c
@@ -1040,10 +1040,8 @@ loff_t __init nvram_create_partition(const char *name, int sig,
 	
 	/* Create our OS partition */
 	new_part = kmalloc(sizeof(*new_part), GFP_KERNEL);
-	if (!new_part) {
-		pr_err("%s: kmalloc failed\n", __func__);
+	if (!new_part)
 		return -ENOMEM;
-	}
 
 	new_part->index = free_part->index;
 	new_part->header.signature = sig;
@@ -1145,10 +1143,8 @@ int __init nvram_scan_partitions(void)
 	total_size = ppc_md.nvram_size();
 	
 	header = kmalloc(NVRAM_HEADER_LEN, GFP_KERNEL);
-	if (!header) {
-		printk(KERN_ERR "nvram_scan_partitions: Failed kmalloc\n");
+	if (!header)
 		return -ENOMEM;
-	}
 
 	while (cur_index < total_size) {
 
@@ -1181,10 +1177,8 @@ int __init nvram_scan_partitions(void)
 		}
 		tmp_part = kmalloc(sizeof(struct nvram_partition), GFP_KERNEL);
 		err = -ENOMEM;
-		if (!tmp_part) {
-			printk(KERN_ERR "nvram_scan_partitions: kmalloc failed\n");
+		if (!tmp_part)
 			goto out;
-		}
 		
 		memcpy(&tmp_part->header, &phead, NVRAM_HEADER_LEN);
 		tmp_part->index = cur_index;
-- 
2.11.0

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

#1562967 — [PATCH 8/8] powerpc/nvram: Move an assignment for the variable "err" in nvram_scan_partitions()

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Thu, 19 Jan 2017 17:27:37 +0100

A local variable was set to an error code before a concrete error situation
was detected. Thus move the corresponding assignment into an if branch
to indicate a software failure there.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 arch/powerpc/kernel/nvram_64.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/arch/powerpc/kernel/nvram_64.c b/arch/powerpc/kernel/nvram_64.c
index ed54147e3c60..5172115c4ef1 100644
--- a/arch/powerpc/kernel/nvram_64.c
+++ b/arch/powerpc/kernel/nvram_64.c
@@ -1173,9 +1173,10 @@ int __init nvram_scan_partitions(void)
 			goto out;
 		}
 		tmp_part = kmalloc(sizeof(*tmp_part), GFP_KERNEL);
-		err = -ENOMEM;
-		if (!tmp_part)
+		if (!tmp_part) {
+			err = -ENOMEM;
 			goto out;
+		}
 		
 		memcpy(&tmp_part->header, &phead, NVRAM_HEADER_LEN);
 		tmp_part->index = cur_index;
-- 
2.11.0

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

#1563074 — [PATCH 5/8] powerpc/nvram: Return directly after a failed kmalloc() in dev_nvram_read()

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Thu, 19 Jan 2017 16:50:31 +0100

Return directly after a call of the function "kmalloc" failed here.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 arch/powerpc/kernel/nvram_64.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/arch/powerpc/kernel/nvram_64.c b/arch/powerpc/kernel/nvram_64.c
index 463551589b97..68b970bcf2fc 100644
--- a/arch/powerpc/kernel/nvram_64.c
+++ b/arch/powerpc/kernel/nvram_64.c
@@ -762,10 +762,8 @@ static ssize_t dev_nvram_read(struct file *file, char __user *buf,
 	count = min(count, PAGE_SIZE);
 
 	tmp = kmalloc(count, GFP_KERNEL);
-	if (!tmp) {
-		ret = -ENOMEM;
-		goto out;
-	}
+	if (!tmp)
+		return -ENOMEM;
 
 	ret = ppc_md.nvram_read(tmp, count, ppos);
 	if (ret <= 0)
-- 
2.11.0

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web