[PATCH] i2c-core: One function call less in acpi_i2c_space_handler() after error detection

[PATCH] i2c-core: One function call less in acpi_i2c_space_handler() after error detection

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sat, 26 Dec 2015 07:30:35 +0100

The kfree() function was called in one case by the
acpi_i2c_space_handler() function during error handling
even if the passed variable "client" contained a null pointer.

Implementation details could be improved by the adjustment of jump targets.

This issue was detected by using the Coccinelle software.

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

diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
index 7349b00..a24e06c 100644
--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -313,18 +313,18 @@ acpi_i2c_space_handler(u32 function, acpi_physical_address command,
 	client = kzalloc(sizeof(*client), GFP_KERNEL);
 	if (!client) {
 		ret = AE_NO_MEMORY;
-		goto err;
+		goto free_ares;
 	}
 
 	if (!value64 || ares->type != ACPI_RESOURCE_TYPE_SERIAL_BUS) {
 		ret = AE_BAD_PARAMETER;
-		goto err;
+		goto free_client;
 	}
 
 	sb = &ares->data.i2c_serial_bus;
 	if (sb->type != ACPI_RESOURCE_SERIAL_TYPE_I2C) {
 		ret = AE_BAD_PARAMETER;
-		goto err;
+		goto free_client;
 	}
 
 	client->adapter = adapter;
@@ -405,9 +405,9 @@ acpi_i2c_space_handler(u32 function, acpi_physical_address command,
 	}
 
 	gsb->status = status;
-
- err:
+free_client:
 	kfree(client);
+free_ares:
 	ACPI_FREE(ares);
 	return ret;
 }
-- 
2.6.3

Re: [PATCH] i2c-core: One function call less in acpi_i2c_space_handler() after error detection

[kbuild test robot]

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

[if your patch is applied to the wrong git tree, please drop us a note to help improving the system]
Hi Markus,

[auto build test ERROR on wsa/i2c/for-next]
[also build test ERROR on v4.4-rc6 next-20151223]

url:    https://github.com/0day-ci/linux/commits/SF-Markus-Elfring/i2c-core-One-function-call-less-in-acpi_i2c_space_handler-after-error-detection/20151226-143820
base:   https://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux i2c/for-next
config: x86_64-randconfig-x010-12251849 (attached as .config)
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

All errors (new ones prefixed by >>):

   drivers/i2c/i2c-core.c: In function 'acpi_i2c_space_handler':
>> drivers/i2c/i2c-core.c:404:3: error: label 'err' used but not defined
      goto err;
      ^

vim +/err +404 drivers/i2c/i2c-core.c

17f4a5c4 Wolfram Sang 2014-09-22  398  		}
17f4a5c4 Wolfram Sang 2014-09-22  399  		break;
17f4a5c4 Wolfram Sang 2014-09-22  400  
17f4a5c4 Wolfram Sang 2014-09-22  401  	default:
17f4a5c4 Wolfram Sang 2014-09-22  402  		pr_info("protocol(0x%02x) is not supported.\n", accessor_type);
17f4a5c4 Wolfram Sang 2014-09-22  403  		ret = AE_BAD_PARAMETER;
17f4a5c4 Wolfram Sang 2014-09-22 @404  		goto err;
17f4a5c4 Wolfram Sang 2014-09-22  405  	}
17f4a5c4 Wolfram Sang 2014-09-22  406  
17f4a5c4 Wolfram Sang 2014-09-22  407  	gsb->status = status;

:::::: The code at line 404 was first introduced by commit
:::::: 17f4a5c47f28de9ea59182f48d07f8c44ee5dcc9 i2c: move acpi code back into the core

:::::: TO: Wolfram Sang <wsa@the-dreams.de>
:::::: CC: Wolfram Sang <wsa@the-dreams.de>

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

[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 25102 bytes --]

[PATCH v2] i2c-core: One function call less in acpi_i2c_space_handler() after error detection

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sat, 26 Dec 2015 08:00:52 +0100

The kfree() function was called in one case by the
acpi_i2c_space_handler() function during error handling
even if the passed variable "client" contained a null pointer.

Implementation details could be improved by the adjustment of jump targets
according to the Linux coding style convention.

This issue was detected by using the Coccinelle software.

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

diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
index 7349b00..9996531 100644
--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -313,18 +313,18 @@ acpi_i2c_space_handler(u32 function, acpi_physical_address command,
 	client = kzalloc(sizeof(*client), GFP_KERNEL);
 	if (!client) {
 		ret = AE_NO_MEMORY;
-		goto err;
+		goto free_ares;
 	}
 
 	if (!value64 || ares->type != ACPI_RESOURCE_TYPE_SERIAL_BUS) {
 		ret = AE_BAD_PARAMETER;
-		goto err;
+		goto free_client;
 	}
 
 	sb = &ares->data.i2c_serial_bus;
 	if (sb->type != ACPI_RESOURCE_SERIAL_TYPE_I2C) {
 		ret = AE_BAD_PARAMETER;
-		goto err;
+		goto free_client;
 	}
 
 	client->adapter = adapter;
@@ -401,13 +401,13 @@ acpi_i2c_space_handler(u32 function, acpi_physical_address command,
 	default:
 		pr_info("protocol(0x%02x) is not supported.\n", accessor_type);
 		ret = AE_BAD_PARAMETER;
-		goto err;
+		goto free_client;
 	}
 
 	gsb->status = status;
-
- err:
+free_client:
 	kfree(client);
+free_ares:
 	ACPI_FREE(ares);
 	return ret;
 }
-- 
2.6.3

Re: [PATCH v2] i2c-core: One function call less in acpi_i2c_space_handler() after error detection

[Wolfram Sang]

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

> The kfree() function was called in one case by the
> acpi_i2c_space_handler() function during error handling
> even if the passed variable "client" contained a null pointer.

This is OK. kfree() is known to be NULL-tolerant and we rely on it in
various places to keep the code simpler.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: i2c-core: One function call less in acpi_i2c_space_handler() after error detection

[SF Markus Elfring]

>> The kfree() function was called in one case by the
>> acpi_i2c_space_handler() function during error handling
>> even if the passed variable "client" contained a null pointer.
> 
> This is OK. kfree() is known to be NULL-tolerant and we rely on it in
> various places to keep the code simpler.

I would appreciate if an unnecessary function call can be avoided here
so that the affected exception handling can become also a bit more efficient.

Regards,
Markus

Re: i2c-core: One function call less in acpi_i2c_space_handler() after error detection

[Wolfram Sang]

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

On Sat, Dec 26, 2015 at 09:52:11AM +0100, SF Markus Elfring wrote:
> >> The kfree() function was called in one case by the
> >> acpi_i2c_space_handler() function during error handling
> >> even if the passed variable "client" contained a null pointer.
> > 
> > This is OK. kfree() is known to be NULL-tolerant and we rely on it in
> > various places to keep the code simpler.
> 
> I would appreciate if an unnecessary function call can be avoided here
> so that the affected exception handling can become also a bit more efficient.

Simpler code is easier to maintain. See your patch, you didn't get it
correctly at your first try. Also, this is not a hot path, so I see
it as a micro-optimization also adding complexity. I don't favor that.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: i2c-core: One function call less in acpi_i2c_space_handler() after error detection

[SF Markus Elfring]

>> I would appreciate if an unnecessary function call can be avoided here
>> so that the affected exception handling can become also a bit more efficient.
> 
> Simpler code is easier to maintain.

There are different opinions available around the desired simplicity.


> See your patch, you didn't get it correctly at your first try.

I wonder myself about the circumstances on how my incomplete update suggestion
did happen.


> Also, this is not a hot path,

I'm curious if approaches around better exception handling can eventually become
a "hot topic".


> so I see it as a micro-optimization

I can agree to this view for this function implementation.

 
> also adding complexity.

There are the usual software development trade-offs.


> I don't favor that.

Thanks for your constructive feedback.

Is an identifier like "free_client" a bit nicer (according to the Linux coding
style recommendations) than the short jump label "err" in the discussed use case?

Regards,
Markus

[PATCH 0/4] i2c-dev: Fine-tuning for four function implementations

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Fri, 23 Sep 2016 21:38:21 +0200

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

Markus Elfring (4):
  Use kmalloc_array() in i2cdev_ioctl_rdwr()
  Improve another size determination in i2cdev_ioctl_rdwr()
  Rename jump labels in i2cdev_attach_adapter()
  Adjust checks for null pointers in three functions

 drivers/i2c/i2c-dev.c | 20 +++++++++++---------
 1 file changed, 11 insertions(+), 9 deletions(-)

-- 
2.10.0

[PATCH 1/4] i2c-dev: Use kmalloc_array() in i2cdev_ioctl_rdwr()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Fri, 23 Sep 2016 20:30:07 +0200

* A multiplication for the size determination of a memory allocation
  indicated that an array data structure should be processed.
  Thus use the corresponding function "kmalloc_array".

  This issue was detected by using the Coccinelle software.

* Replace the specification of a data type by a pointer dereference
  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/i2c/i2c-dev.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/i2c/i2c-dev.c b/drivers/i2c/i2c-dev.c
index 66f323f..6d8226d 100644
--- a/drivers/i2c/i2c-dev.c
+++ b/drivers/i2c/i2c-dev.c
@@ -260,7 +260,9 @@ static noinline int i2cdev_ioctl_rdwr(struct i2c_client *client,
 	if (IS_ERR(rdwr_pa))
 		return PTR_ERR(rdwr_pa);
 
-	data_ptrs = kmalloc(rdwr_arg.nmsgs * sizeof(u8 __user *), GFP_KERNEL);
+	data_ptrs = kmalloc_array(rdwr_arg.nmsgs,
+				  sizeof(*data_ptrs),
+				  GFP_KERNEL);
 	if (data_ptrs == NULL) {
 		kfree(rdwr_pa);
 		return -ENOMEM;
-- 
2.10.0

[PATCH 2/4] i2c-dev: Improve another size determination in i2cdev_ioctl_rdwr()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Fri, 23 Sep 2016 20:45:40 +0200

Replace the specification of a data structure by a pointer dereference
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/i2c/i2c-dev.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/i2c/i2c-dev.c b/drivers/i2c/i2c-dev.c
index 6d8226d..a6e35ce 100644
--- a/drivers/i2c/i2c-dev.c
+++ b/drivers/i2c/i2c-dev.c
@@ -256,7 +256,7 @@ static noinline int i2cdev_ioctl_rdwr(struct i2c_client *client,
 		return -EINVAL;
 
 	rdwr_pa = memdup_user(rdwr_arg.msgs,
-			      rdwr_arg.nmsgs * sizeof(struct i2c_msg));
+			      rdwr_arg.nmsgs * sizeof(*rdwr_pa));
 	if (IS_ERR(rdwr_pa))
 		return PTR_ERR(rdwr_pa);
 
-- 
2.10.0

[PATCH 3/4] i2c-dev: Rename jump labels in i2cdev_attach_adapter()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Fri, 23 Sep 2016 21:21:39 +0200

Adjust jump labels according to the current Linux coding style convention.

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

diff --git a/drivers/i2c/i2c-dev.c b/drivers/i2c/i2c-dev.c
index a6e35ce..7d3e3ca 100644
--- a/drivers/i2c/i2c-dev.c
+++ b/drivers/i2c/i2c-dev.c
@@ -555,7 +555,7 @@ static int i2cdev_attach_adapter(struct device *dev, void *dummy)
 	i2c_dev->cdev.owner = THIS_MODULE;
 	res = cdev_add(&i2c_dev->cdev, MKDEV(I2C_MAJOR, adap->nr), 1);
 	if (res)
-		goto error_cdev;
+		goto put_i2c;
 
 	/* register this i2c device with the driver core */
 	i2c_dev->dev = device_create(i2c_dev_class, &adap->dev,
@@ -563,15 +563,15 @@ static int i2cdev_attach_adapter(struct device *dev, void *dummy)
 				     "i2c-%d", adap->nr);
 	if (IS_ERR(i2c_dev->dev)) {
 		res = PTR_ERR(i2c_dev->dev);
-		goto error;
+		goto delete_cdev;
 	}
 
 	pr_debug("i2c-dev: adapter [%s] registered as minor %d\n",
 		 adap->name, adap->nr);
 	return 0;
-error:
+delete_cdev:
 	cdev_del(&i2c_dev->cdev);
-error_cdev:
+put_i2c:
 	put_i2c_dev(i2c_dev);
 	return res;
 }
-- 
2.10.0

[PATCH 4/4] i2c-dev: Adjust checks for null pointers in three functions

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Fri, 23 Sep 2016 21:30:20 +0200
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

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

Comparison to NULL could be written !…

Thus fix the affected source code places.

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

diff --git a/drivers/i2c/i2c-dev.c b/drivers/i2c/i2c-dev.c
index 7d3e3ca..8f7eddd 100644
--- a/drivers/i2c/i2c-dev.c
+++ b/drivers/i2c/i2c-dev.c
@@ -147,7 +147,7 @@ static ssize_t i2cdev_read(struct file *file, char __user *buf, size_t count,
 		count = 8192;
 
 	tmp = kmalloc(count, GFP_KERNEL);
-	if (tmp == NULL)
+	if (!tmp)
 		return -ENOMEM;
 
 	pr_debug("i2c-dev: i2c-%d reading %zu bytes.\n",
@@ -263,7 +263,7 @@ static noinline int i2cdev_ioctl_rdwr(struct i2c_client *client,
 	data_ptrs = kmalloc_array(rdwr_arg.nmsgs,
 				  sizeof(*data_ptrs),
 				  GFP_KERNEL);
-	if (data_ptrs == NULL) {
+	if (!data_ptrs) {
 		kfree(rdwr_pa);
 		return -ENOMEM;
 	}
@@ -374,7 +374,7 @@ static noinline int i2cdev_ioctl_smbus(struct i2c_client *client,
 				      client->flags, data_arg.read_write,
 				      data_arg.command, data_arg.size, NULL);
 
-	if (data_arg.data == NULL) {
+	if (!data_arg.data) {
 		dev_dbg(&client->adapter->dev,
 			"data is NULL pointer in ioctl I2C_SMBUS.\n");
 		return -EINVAL;
-- 
2.10.0