[PATCH 0/2] Input-evdev: Fine-tuning for three function implementations

From: Markus Elfring <redacted>
Date: Sat, 24 Sep 2016 13:03:13 +0200

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

Markus Elfring (2):
  Use kmalloc_array()
  Rename a jump label in two functions

 drivers/input/evdev.c | 20 +++++++-------------
 1 file changed, 7 insertions(+), 13 deletions(-)

-- 
2.10.0

From: Markus Elfring <redacted>
Date: Sat, 24 Sep 2016 12:42:45 +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.

* Delete the local variable "len" which became unnecessary with
  this refactoring.

Signed-off-by: Markus Elfring <redacted>
---
 drivers/input/evdev.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/drivers/input/evdev.c b/drivers/input/evdev.c
index e9ae3d5..83fcfd6 100644
--- a/drivers/input/evdev.c
+++ b/drivers/input/evdev.c

@@ -919,18 +919,14 @@ static int evdev_handle_get_val(struct evdev_client *client,
 {
 	int ret;
 	unsigned long *mem;
-	size_t len;
 
-	len = BITS_TO_LONGS(maxbit) * sizeof(unsigned long);
-	mem = kmalloc(len, GFP_KERNEL);
+	mem = kmalloc_array(BITS_TO_LONGS(maxbit), sizeof(*mem), GFP_KERNEL);
 	if (!mem)
 		return -ENOMEM;
 
 	spin_lock_irq(&dev->event_lock);
 	spin_lock(&client->buffer_lock);
-
-	memcpy(mem, bits, len);
-
+	memcpy(mem, bits, sizeof(*mem) * BITS_TO_LONGS(maxbit));
 	spin_unlock(&dev->event_lock);
 
 	__evdev_flush_queue(client, type);

-- 
2.10.0

From: Markus Elfring <redacted>
Date: Sat, 24 Sep 2016 12:50:31 +0200

Adjust a jump label according to the current Linux coding style convention.
Thus replace the identifier "out" by "unlock".

Signed-off-by: Markus Elfring <redacted>
---
 drivers/input/evdev.c | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

diff --git a/drivers/input/evdev.c b/drivers/input/evdev.c
index 83fcfd6..548874d 100644
--- a/drivers/input/evdev.c
+++ b/drivers/input/evdev.c

@@ -545,22 +545,21 @@ static ssize_t evdev_write(struct file *file, const char __user *buffer,
 
 	if (!evdev->exist || client->revoked) {
 		retval = -ENODEV;
-		goto out;
+		goto unlock;
 	}
 
 	while (retval + input_event_size() <= count) {
 
 		if (input_event_from_user(buffer + retval, &event)) {
 			retval = -EFAULT;
-			goto out;
+			goto unlock;
 		}
 		retval += input_event_size();
 
 		input_inject_event(&evdev->handle,
 				   event.type, event.code, event.value);
 	}
-
- out:
+unlock:
 	mutex_unlock(&evdev->mutex);
 	return retval;
 }

@@ -1292,12 +1291,11 @@ static long evdev_ioctl_handler(struct file *file, unsigned int cmd,
 
 	if (!evdev->exist || client->revoked) {
 		retval = -ENODEV;
-		goto out;
+		goto unlock;
 	}
 
 	retval = evdev_do_ioctl(file, cmd, p, compat_mode);
-
- out:
+unlock:
 	mutex_unlock(&evdev->mutex);
 	return retval;
 }

-- 
2.10.0

On Sat, Sep 24, 2016 at 4:10 AM, SF Markus Elfring
[off-list ref] wrote:

From: Markus Elfring <redacted>
Date: Sat, 24 Sep 2016 12:50:31 +0200

Adjust a jump label according to the current Linux coding style convention.
Thus replace the identifier "out" by "unlock".

No, just no.

-- 
Dmitry

On Sat, Sep 24, 2016 at 4:08 AM, SF Markus Elfring
[off-list ref] wrote:

From: Markus Elfring <redacted>
Date: Sat, 24 Sep 2016 12:42:45 +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.

* Delete the local variable "len" which became unnecessary with
  this refactoring.

So we have to multiply twice now, once in kmalloc_array, the second
time in memcpy(). No, thank you.

Also, please note that we do not really treat the allocated "mem" as
an array, but rather area of memory that holds all bits that we need
to transfer, and so I consider using kmalloc_array() actually wrong
here.

Please do not blindly follow checkpatch and coccinelle suggestions.
They are just that: suggestions and not hared rules.

Thanks.

-- 
Dmitry

So we have to multiply twice now, once in kmalloc_array, the second
time in memcpy().

It looks so in the source code after the suggested refactoring.

No, thank you.

Would you like to check any further if a specific compiler implementation
will still optimise common subexpressions as you desired it?

Also, please note that we do not really treat the allocated "mem" as an array,
but rather area of memory that holds all bits that we need to transfer,
and so I consider using kmalloc_array() actually wrong here.

Thanks for your explanation.

Please do not blindly follow checkpatch and coccinelle suggestions.
They are just that: suggestions and not hared rules.

I am curious on how to clarify corresponding deviations further.


Would you like to suggest any other details so that the evolving scripts
can become better and safer for static source code analysis?

Do you know any special properties which should be additionally checked
at call sites which are similar to the discussed place?

Regards,
Markus

On Sat, Sep 24, 2016 at 08:16:16PM +0200, SF Markus Elfring wrote:

quoted

So we have to multiply twice now, once in kmalloc_array, the second
time in memcpy().

It looks so in the source code after the suggested refactoring.

quoted

No, thank you.

Would you like to check any further if a specific compiler implementation
will still optimise common subexpressions as you desired it?

quoted

Also, please note that we do not really treat the allocated "mem" as an array,
but rather area of memory that holds all bits that we need to transfer,
and so I consider using kmalloc_array() actually wrong here.

Thanks for your explanation.

quoted

Please do not blindly follow checkpatch and coccinelle suggestions.
They are just that: suggestions and not hared rules.

I am curious on how to clarify corresponding deviations further.


Would you like to suggest any other details so that the evolving scripts
can become better and safer for static source code analysis?

Do you know any special properties which should be additionally checked
at call sites which are similar to the discussed place?

If you are asking for some formal rules then no.

Again, what is the purpose of the changes? Are you working on the code
and the fact that the driver is older-style hinders your progress? Or
there are runtime improvements from your changes? Correctness issues?

I do not very much appreciate changes just to satisfy checkpatch rule
du jour.

Thanks.

-- 
Dmitry

Again, what is the purpose of the changes?

I came also along a few source code places where their maintainers
requested the usage of a function like "kmalloc_array".

How do you think about to clarify the consistent use of programming
interfaces for Linux a bit more?

Are you working on the code

I am trying to improve various free software (including Linux).

and the fact that the driver is older-style hinders your progress?

Not for me directly.

Or there are runtime improvements from your changes?

It depends on some factors.

Can an array memory allocator organise the desired data in a safer
and more efficient way than a "default approach"?

Correctness issues?

This can be.

Do you care for source code annotations?

I do not very much appreciate changes just to satisfy checkpatch rule
du jour.

Do these rules contain knowledge which you give a significant value?

Regards,
Markus