[PATCH 0/5] s390/pci: Fine-tuning for three function implementations

[PATCH 0/5] s390/pci: Fine-tuning for three function implementations

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sat, 21 Jan 2017 18:58:18 +0100

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

Markus Elfring (5):
  Return directly after a failed parameter validation in clp_normal_command()
  Return directly after a failed clp_alloc_block() in clp_normal_command()
  Move three assignments for the variable "rc" in clp_normal_command()
  Move an assignment for the variable "ret" in s390_pci_mmio_write()
  Move two assignments for the variable "ret" in get_pfn()

 arch/s390/pci/pci_clp.c  | 20 +++++++++-----------
 arch/s390/pci/pci_mmio.c | 19 ++++++++++---------
 2 files changed, 19 insertions(+), 20 deletions(-)

-- 
2.11.0

[PATCH 1/5] s390/pci: Return directly after a failed parameter validation in clp_normal_command()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sat, 21 Jan 2017 17:49:00 +0100

Return directly after an inappropriate input parameter was detected.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 arch/s390/pci/pci_clp.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/arch/s390/pci/pci_clp.c b/arch/s390/pci/pci_clp.c
index 1c3332ac1957..4f5a1bf3b14b 100644
--- a/arch/s390/pci/pci_clp.c
+++ b/arch/s390/pci/pci_clp.c
@@ -514,9 +514,8 @@ static int clp_normal_command(struct clp_req *req)
 	void __user *uptr;
 	int rc;
 
-	rc = -EINVAL;
 	if (req->lps != 0 && req->lps != 2)
-		goto out;
+		return -EINVAL;
 
 	rc = -ENOMEM;
 	lpcb = clp_alloc_block(GFP_KERNEL);
-- 
2.11.0

[PATCH 2/5] s390/pci: Return directly after a failed clp_alloc_block() in clp_normal_command()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sat, 21 Jan 2017 17:56:40 +0100

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

* Delete the jump label "out" which became unnecessary with
  this refactoring.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 arch/s390/pci/pci_clp.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/arch/s390/pci/pci_clp.c b/arch/s390/pci/pci_clp.c
index 4f5a1bf3b14b..28e13fa7a79c 100644
--- a/arch/s390/pci/pci_clp.c
+++ b/arch/s390/pci/pci_clp.c
@@ -517,10 +517,9 @@ static int clp_normal_command(struct clp_req *req)
 	if (req->lps != 0 && req->lps != 2)
 		return -EINVAL;
 
-	rc = -ENOMEM;
 	lpcb = clp_alloc_block(GFP_KERNEL);
 	if (!lpcb)
-		goto out;
+		return -ENOMEM;
 
 	rc = -EFAULT;
 	uptr = (void __force __user *)(unsigned long) req->data_p;
@@ -550,7 +549,6 @@ static int clp_normal_command(struct clp_req *req)
 
 out_free:
 	clp_free_block(lpcb);
-out:
 	return rc;
 }
 
-- 
2.11.0

[PATCH 3/5] s390/pci: Move three assignments for the variable "rc" in clp_normal_command()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sat, 21 Jan 2017 18:10:57 +0100

A local variable was set to an error code before a concrete error situation
was detected. Thus move the corresponding assignments into if branches
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/s390/pci/pci_clp.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/arch/s390/pci/pci_clp.c b/arch/s390/pci/pci_clp.c
index 28e13fa7a79c..bde0291cc724 100644
--- a/arch/s390/pci/pci_clp.c
+++ b/arch/s390/pci/pci_clp.c
@@ -521,14 +521,16 @@ static int clp_normal_command(struct clp_req *req)
 	if (!lpcb)
 		return -ENOMEM;
 
-	rc = -EFAULT;
 	uptr = (void __force __user *)(unsigned long) req->data_p;
-	if (copy_from_user(lpcb, uptr, PAGE_SIZE) != 0)
+	if (copy_from_user(lpcb, uptr, PAGE_SIZE) != 0) {
+		rc = -EFAULT;
 		goto out_free;
+	}
 
-	rc = -EINVAL;
-	if (lpcb->fmt != 0 || lpcb->reserved1 != 0 || lpcb->reserved2 != 0)
+	if (lpcb->fmt != 0 || lpcb->reserved1 != 0 || lpcb->reserved2 != 0) {
+		rc = -EINVAL;
 		goto out_free;
+	}
 
 	switch (req->lps) {
 	case 0:
@@ -541,9 +543,8 @@ static int clp_normal_command(struct clp_req *req)
 	if (rc)
 		goto out_free;
 
-	rc = -EFAULT;
 	if (copy_to_user(uptr, lpcb, PAGE_SIZE) != 0)
-		goto out_free;
+		rc = -EFAULT;
 
 	rc = 0;
 
-- 
2.11.0

Re: [PATCH 3/5] s390/pci: Move three assignments for the variable "rc" in clp_normal_command()

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 3223 bytes --]

Hi Markus,

[auto build test WARNING on s390/features]
[also build test WARNING on v4.10-rc4 next-20170120]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/SF-Markus-Elfring/s390-pci-Fine-tuning-for-three-function-implementations/20170122-021955
base:   https://git.kernel.org/pub/scm/linux/kernel/git/s390/linux.git features
config: s390-allmodconfig (attached as .config)
compiler: s390x-linux-gnu-gcc (Debian 6.1.1-9) 6.1.1 20160705
reproduce:
        wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=s390 

Note: it may well be a FALSE warning. FWIW you are at least aware of it now.
http://gcc.gnu.org/wiki/Better_Uninitialized_Warnings

All warnings (new ones prefixed by >>):

   arch/s390/pci/pci_clp.c: In function 'clp_misc_ioctl':
>> arch/s390/pci/pci_clp.c:543:5: warning: 'rc' may be used uninitialized in this function [-Wmaybe-uninitialized]
     if (rc)
        ^
   arch/s390/pci/pci_clp.c:515:6: note: 'rc' was declared here
     int rc;
         ^~

vim +/rc +543 arch/s390/pci/pci_clp.c

988b86e69 Martin Schwidefsky 2016-01-13  527  		goto out_free;
e201d01da Markus Elfring     2017-01-21  528  	}
988b86e69 Martin Schwidefsky 2016-01-13  529  
e201d01da Markus Elfring     2017-01-21  530  	if (lpcb->fmt != 0 || lpcb->reserved1 != 0 || lpcb->reserved2 != 0) {
988b86e69 Martin Schwidefsky 2016-01-13  531  		rc = -EINVAL;
988b86e69 Martin Schwidefsky 2016-01-13  532  		goto out_free;
e201d01da Markus Elfring     2017-01-21  533  	}
988b86e69 Martin Schwidefsky 2016-01-13  534  
988b86e69 Martin Schwidefsky 2016-01-13  535  	switch (req->lps) {
988b86e69 Martin Schwidefsky 2016-01-13  536  	case 0:
988b86e69 Martin Schwidefsky 2016-01-13  537  		rc = clp_base_command(req, lpcb);
988b86e69 Martin Schwidefsky 2016-01-13  538  		break;
988b86e69 Martin Schwidefsky 2016-01-13  539  	case 2:
988b86e69 Martin Schwidefsky 2016-01-13  540  		rc = clp_pci_command(req, lpcb);
988b86e69 Martin Schwidefsky 2016-01-13  541  		break;
988b86e69 Martin Schwidefsky 2016-01-13  542  	}
988b86e69 Martin Schwidefsky 2016-01-13 @543  	if (rc)
988b86e69 Martin Schwidefsky 2016-01-13  544  		goto out_free;
988b86e69 Martin Schwidefsky 2016-01-13  545  
988b86e69 Martin Schwidefsky 2016-01-13  546  	if (copy_to_user(uptr, lpcb, PAGE_SIZE) != 0)
e201d01da Markus Elfring     2017-01-21  547  		rc = -EFAULT;
988b86e69 Martin Schwidefsky 2016-01-13  548  
988b86e69 Martin Schwidefsky 2016-01-13  549  	rc = 0;
988b86e69 Martin Schwidefsky 2016-01-13  550  
988b86e69 Martin Schwidefsky 2016-01-13  551  out_free:

:::::: The code at line 543 was first introduced by commit
:::::: 988b86e69ded17f0f1209fd3ef1c4c7f1567dcc1 s390/pci: add ioctl interface for CLP

:::::: TO: Martin Schwidefsky <schwidefsky@de.ibm.com>
:::::: CC: Martin Schwidefsky <schwidefsky@de.ibm.com>

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 44078 bytes --]

[PATCH 4/5] s390/pci: Move an assignment for the variable "ret" in s390_pci_mmio_write()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sat, 21 Jan 2017 18:35:00 +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/s390/pci/pci_mmio.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/arch/s390/pci/pci_mmio.c b/arch/s390/pci/pci_mmio.c
index b1bb2b72302c..f125858a77e9 100644
--- a/arch/s390/pci/pci_mmio.c
+++ b/arch/s390/pci/pci_mmio.c
@@ -57,12 +57,11 @@ SYSCALL_DEFINE3(s390_pci_mmio_write, unsigned long, mmio_addr,
 		goto out;
 	io_addr = (void __iomem *)((pfn << PAGE_SHIFT) | (mmio_addr & ~PAGE_MASK));
 
-	ret = -EFAULT;
-	if ((unsigned long) io_addr < ZPCI_IOMAP_ADDR_BASE)
-		goto out;
-
-	if (copy_from_user(buf, user_buffer, length))
+	if ((unsigned long)io_addr < ZPCI_IOMAP_ADDR_BASE ||
+	    copy_from_user(buf, user_buffer, length)) {
+		ret = -EFAULT;
 		goto out;
+	}
 
 	ret = zpci_memcpy_toio(io_addr, buf, length);
 out:
-- 
2.11.0

Re: [PATCH 4/5] s390/pci: Move an assignment for the variable "ret" in s390_pci_mmio_write()

[Dan Carpenter]

The original was more readable.

regards,
dan carpenter

[PATCH 5/5] s390/pci: Move two assignments for the variable "ret" in get_pfn()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sat, 21 Jan 2017 18:48:45 +0100

A local variable was set to an error code in two cases before a concrete
error situation was detected. Thus move the corresponding assignments into
if branches 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/s390/pci/pci_mmio.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/arch/s390/pci/pci_mmio.c b/arch/s390/pci/pci_mmio.c
index f125858a77e9..400af13ef275 100644
--- a/arch/s390/pci/pci_mmio.c
+++ b/arch/s390/pci/pci_mmio.c
@@ -18,13 +18,15 @@ static long get_pfn(unsigned long user_addr, unsigned long access,
 	long ret;
 
 	down_read(&current->mm->mmap_sem);
-	ret = -EINVAL;
 	vma = find_vma(current->mm, user_addr);
-	if (!vma)
+	if (!vma) {
+		ret = -EINVAL;
 		goto out;
-	ret = -EACCES;
-	if (!(vma->vm_flags & access))
+	}
+	if (!(vma->vm_flags & access)) {
+		ret = -EACCES;
 		goto out;
+	}
 	ret = follow_pfn(vma, user_addr, pfn);
 out:
 	up_read(&current->mm->mmap_sem);
-- 
2.11.0

Re: [PATCH 0/5] s390/pci: Fine-tuning for three function implementations

[Dan Carpenter]

You should compile test your patches...  No one should have to explain
that.

regards,
dan carpenter