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

From: Markus Elfring <redacted>
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

From: Markus Elfring <redacted>
Date: Thu, 19 Jan 2017 15:22:56 +0100

* Return directly after an inappropriate input parameter was detected.

* Delete an initialisation for the variable "tmp" at the beginning
  and an assignment for the variable "ret" which became unnecessary
  with this refactoring.

Signed-off-by: Markus Elfring <redacted>
---
 arch/powerpc/kernel/nvram_64.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/arch/powerpc/kernel/nvram_64.c b/arch/powerpc/kernel/nvram_64.c
index 34d2c595de23..37d08b95c3f0 100644
--- a/arch/powerpc/kernel/nvram_64.c
+++ b/arch/powerpc/kernel/nvram_64.c

@@ -790,17 +790,15 @@ 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);

-- 
2.11.0

From: Markus Elfring <redacted>
Date: Thu, 19 Jan 2017 15:44:03 +0100

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

Signed-off-by: Markus Elfring <redacted>
---
 arch/powerpc/kernel/nvram_64.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/arch/powerpc/kernel/nvram_64.c b/arch/powerpc/kernel/nvram_64.c
index 37d08b95c3f0..cf839adf3aa7 100644
--- a/arch/powerpc/kernel/nvram_64.c
+++ b/arch/powerpc/kernel/nvram_64.c

@@ -802,11 +802,9 @@ static ssize_t dev_nvram_write(struct file *file, const char __user *buf,
 
 	count = min_t(size_t, count, size - *ppos);
 	count = min(count, PAGE_SIZE);
-
-	ret = -ENOMEM;
 	tmp = kmalloc(count, GFP_KERNEL);
 	if (!tmp)
-		goto out;
+		return -ENOMEM;
 
 	ret = -EFAULT;
 	if (copy_from_user(tmp, buf, count))

-- 
2.11.0

From: Markus Elfring <redacted>
Date: Thu, 19 Jan 2017 16:12:48 +0100

* Return directly after an inappropriate input parameter was detected.

* Delete an initialisation for the variable "tmp" at the beginning
  which became unnecessary with this refactoring.

Signed-off-by: Markus Elfring <redacted>
---
 arch/powerpc/kernel/nvram_64.c | 20 +++++++-------------
 1 file changed, 7 insertions(+), 13 deletions(-)

diff --git a/arch/powerpc/kernel/nvram_64.c b/arch/powerpc/kernel/nvram_64.c
index dc90a0e9ad65..463551589b97 100644
--- a/arch/powerpc/kernel/nvram_64.c
+++ b/arch/powerpc/kernel/nvram_64.c

@@ -745,24 +745,18 @@ static ssize_t dev_nvram_read(struct file *file, char __user *buf,
 			  size_t count, loff_t *ppos)
 {
 	ssize_t ret;
-	char *tmp = NULL;
+	char *tmp;
 	ssize_t size;
 
-	if (!ppc_md.nvram_size) {
-		ret = -ENODEV;
-		goto out;
-	}
+	if (!ppc_md.nvram_size)
+		return -ENODEV;
 
 	size = ppc_md.nvram_size();
-	if (size < 0) {
-		ret = size;
-		goto out;
-	}
+	if (size < 0)
+		return size;
 
-	if (*ppos >= size) {
-		ret = 0;
-		goto out;
-	}
+	if (*ppos >= size)
+		return 0;
 
 	count = min_t(size_t, count, size - *ppos);
 	count = min(count, PAGE_SIZE);

-- 
2.11.0

From: Markus Elfring <redacted>
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 <redacted>
---
 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

From: Markus Elfring <redacted>
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 <redacted>
---
 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

From: Markus Elfring <redacted>
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 <redacted>
---
 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

From: Markus Elfring <redacted>
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 <redacted>
---
 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

From: Markus Elfring <redacted>
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 <redacted>
---
 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

On 01/19/2017 08:56 AM, SF Markus Elfring wrote:

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,

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

On 01/19/2017 11:08 PM, SF Markus Elfring wrote:

quoted

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.

quoted

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