[PATCH] uinput: Rename a jump label in uinput_ioctl_handler()

[PATCH] uinput: Rename a jump label in uinput_ioctl_handler()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sat, 12 Dec 2015 10:06:00 +0100

This issue was detected by using the Coccinelle software.

Choose a jump label according to the current Linux coding style convention.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/input/misc/uinput.c | 66 ++++++++++++++++++++++-----------------------
 1 file changed, 33 insertions(+), 33 deletions(-)

diff --git a/drivers/input/misc/uinput.c b/drivers/input/misc/uinput.c
index 5adbced..466f62d 100644
--- a/drivers/input/misc/uinput.c
+++ b/drivers/input/misc/uinput.c
@@ -717,7 +717,7 @@ static long uinput_ioctl_handler(struct file *file, unsigned int cmd,
 	if (!udev->dev) {
 		retval = uinput_allocate_device(udev);
 		if (retval)
-			goto out;
+			goto unlock;
 	}
 
 	switch (cmd) {
@@ -725,82 +725,82 @@ static long uinput_ioctl_handler(struct file *file, unsigned int cmd,
 			if (put_user(UINPUT_VERSION,
 				     (unsigned int __user *)p))
 				retval = -EFAULT;
-			goto out;
+			goto unlock;
 
 		case UI_DEV_CREATE:
 			retval = uinput_create_device(udev);
-			goto out;
+			goto unlock;
 
 		case UI_DEV_DESTROY:
 			uinput_destroy_device(udev);
-			goto out;
+			goto unlock;
 
 		case UI_SET_EVBIT:
 			retval = uinput_set_bit(arg, evbit, EV_MAX);
-			goto out;
+			goto unlock;
 
 		case UI_SET_KEYBIT:
 			retval = uinput_set_bit(arg, keybit, KEY_MAX);
-			goto out;
+			goto unlock;
 
 		case UI_SET_RELBIT:
 			retval = uinput_set_bit(arg, relbit, REL_MAX);
-			goto out;
+			goto unlock;
 
 		case UI_SET_ABSBIT:
 			retval = uinput_set_bit(arg, absbit, ABS_MAX);
-			goto out;
+			goto unlock;
 
 		case UI_SET_MSCBIT:
 			retval = uinput_set_bit(arg, mscbit, MSC_MAX);
-			goto out;
+			goto unlock;
 
 		case UI_SET_LEDBIT:
 			retval = uinput_set_bit(arg, ledbit, LED_MAX);
-			goto out;
+			goto unlock;
 
 		case UI_SET_SNDBIT:
 			retval = uinput_set_bit(arg, sndbit, SND_MAX);
-			goto out;
+			goto unlock;
 
 		case UI_SET_FFBIT:
 			retval = uinput_set_bit(arg, ffbit, FF_MAX);
-			goto out;
+			goto unlock;
 
 		case UI_SET_SWBIT:
 			retval = uinput_set_bit(arg, swbit, SW_MAX);
-			goto out;
+			goto unlock;
 
 		case UI_SET_PROPBIT:
 			retval = uinput_set_bit(arg, propbit, INPUT_PROP_MAX);
-			goto out;
+			goto unlock;
 
 		case UI_SET_PHYS:
 			if (udev->state == UIST_CREATED) {
 				retval = -EINVAL;
-				goto out;
+				goto unlock;
 			}
 
 			phys = strndup_user(p, 1024);
 			if (IS_ERR(phys)) {
 				retval = PTR_ERR(phys);
-				goto out;
+				goto unlock;
 			}
 
 			kfree(udev->dev->phys);
 			udev->dev->phys = phys;
-			goto out;
+			goto unlock;
 
 		case UI_BEGIN_FF_UPLOAD:
 			retval = uinput_ff_upload_from_user(p, &ff_up);
 			if (retval)
-				goto out;
+				goto unlock;
 
 			req = uinput_request_find(udev, ff_up.request_id);
 			if (!req || req->code != UI_FF_UPLOAD ||
 			    !req->u.upload.effect) {
 				retval = -EINVAL;
-				goto out;
+				goto unlock;
 			}
 
 			ff_up.retval = 0;
@@ -811,60 +811,60 @@ static long uinput_ioctl_handler(struct file *file, unsigned int cmd,
 				memset(&ff_up.old, 0, sizeof(struct ff_effect));
 
 			retval = uinput_ff_upload_to_user(p, &ff_up);
-			goto out;
+			goto unlock;
 
 		case UI_BEGIN_FF_ERASE:
 			if (copy_from_user(&ff_erase, p, sizeof(ff_erase))) {
 				retval = -EFAULT;
-				goto out;
+				goto unlock;
 			}
 
 			req = uinput_request_find(udev, ff_erase.request_id);
 			if (!req || req->code != UI_FF_ERASE) {
 				retval = -EINVAL;
-				goto out;
+				goto unlock;
 			}
 
 			ff_erase.retval = 0;
 			ff_erase.effect_id = req->u.effect_id;
 			if (copy_to_user(p, &ff_erase, sizeof(ff_erase))) {
 				retval = -EFAULT;
-				goto out;
+				goto unlock;
 			}
 
-			goto out;
+			goto unlock;
 
 		case UI_END_FF_UPLOAD:
 			retval = uinput_ff_upload_from_user(p, &ff_up);
 			if (retval)
-				goto out;
+				goto unlock;
 
 			req = uinput_request_find(udev, ff_up.request_id);
 			if (!req || req->code != UI_FF_UPLOAD ||
 			    !req->u.upload.effect) {
 				retval = -EINVAL;
-				goto out;
+				goto unlock;
 			}
 
 			req->retval = ff_up.retval;
 			uinput_request_done(udev, req);
-			goto out;
+			goto unlock;
 
 		case UI_END_FF_ERASE:
 			if (copy_from_user(&ff_erase, p, sizeof(ff_erase))) {
 				retval = -EFAULT;
-				goto out;
+				goto unlock;
 			}
 
 			req = uinput_request_find(udev, ff_erase.request_id);
 			if (!req || req->code != UI_FF_ERASE) {
 				retval = -EINVAL;
-				goto out;
+				goto unlock;
 			}
 
 			req->retval = ff_erase.retval;
 			uinput_request_done(udev, req);
-			goto out;
+			goto unlock;
 	}
 
 	size = _IOC_SIZE(cmd);
@@ -874,15 +874,15 @@ static long uinput_ioctl_handler(struct file *file, unsigned int cmd,
 	case UI_GET_SYSNAME(0):
 		if (udev->state != UIST_CREATED) {
 			retval = -ENOENT;
-			goto out;
+			goto unlock;
 		}
 		name = dev_name(&udev->dev->dev);
 		retval = uinput_str_to_user(p, name, size);
-		goto out;
+		goto unlock;
 	}
 
 	retval = -EINVAL;
- out:
+ unlock:
 	mutex_unlock(&udev->mutex);
 	return retval;
 }
-- 
2.6.3

Re: [PATCH] uinput: Rename a jump label in uinput_ioctl_handler()

[Dmitry Torokhov]

Hi Markus,

On Sat, Dec 12, 2015 at 10:16:34AM +0100, SF Markus Elfring wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Sat, 12 Dec 2015 10:06:00 +0100
> 
> This issue was detected by using the Coccinelle software.
> 
> Choose a jump label according to the current Linux coding style convention.

While I am mildly curious where you  found this Coccinelle script
complaining about label names I find the current name is perfectly fine.

Thanks.

> 
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>  drivers/input/misc/uinput.c | 66 ++++++++++++++++++++++-----------------------
>  1 file changed, 33 insertions(+), 33 deletions(-)
> 
> diff --git a/drivers/input/misc/uinput.c b/drivers/input/misc/uinput.c
> index 5adbced..466f62d 100644
> --- a/drivers/input/misc/uinput.c
> +++ b/drivers/input/misc/uinput.c
> @@ -717,7 +717,7 @@ static long uinput_ioctl_handler(struct file *file, unsigned int cmd,
>  	if (!udev->dev) {
>  		retval = uinput_allocate_device(udev);
>  		if (retval)
> -			goto out;
> +			goto unlock;
>  	}
>  
>  	switch (cmd) {
> @@ -725,82 +725,82 @@ static long uinput_ioctl_handler(struct file *file, unsigned int cmd,
>  			if (put_user(UINPUT_VERSION,
>  				     (unsigned int __user *)p))
>  				retval = -EFAULT;
> -			goto out;
> +			goto unlock;
>  
>  		case UI_DEV_CREATE:
>  			retval = uinput_create_device(udev);
> -			goto out;
> +			goto unlock;
>  
>  		case UI_DEV_DESTROY:
>  			uinput_destroy_device(udev);
> -			goto out;
> +			goto unlock;
>  
>  		case UI_SET_EVBIT:
>  			retval = uinput_set_bit(arg, evbit, EV_MAX);
> -			goto out;
> +			goto unlock;
>  
>  		case UI_SET_KEYBIT:
>  			retval = uinput_set_bit(arg, keybit, KEY_MAX);
> -			goto out;
> +			goto unlock;
>  
>  		case UI_SET_RELBIT:
>  			retval = uinput_set_bit(arg, relbit, REL_MAX);
> -			goto out;
> +			goto unlock;
>  
>  		case UI_SET_ABSBIT:
>  			retval = uinput_set_bit(arg, absbit, ABS_MAX);
> -			goto out;
> +			goto unlock;
>  
>  		case UI_SET_MSCBIT:
>  			retval = uinput_set_bit(arg, mscbit, MSC_MAX);
> -			goto out;
> +			goto unlock;
>  
>  		case UI_SET_LEDBIT:
>  			retval = uinput_set_bit(arg, ledbit, LED_MAX);
> -			goto out;
> +			goto unlock;
>  
>  		case UI_SET_SNDBIT:
>  			retval = uinput_set_bit(arg, sndbit, SND_MAX);
> -			goto out;
> +			goto unlock;
>  
>  		case UI_SET_FFBIT:
>  			retval = uinput_set_bit(arg, ffbit, FF_MAX);
> -			goto out;
> +			goto unlock;
>  
>  		case UI_SET_SWBIT:
>  			retval = uinput_set_bit(arg, swbit, SW_MAX);
> -			goto out;
> +			goto unlock;
>  
>  		case UI_SET_PROPBIT:
>  			retval = uinput_set_bit(arg, propbit, INPUT_PROP_MAX);
> -			goto out;
> +			goto unlock;
>  
>  		case UI_SET_PHYS:
>  			if (udev->state == UIST_CREATED) {
>  				retval = -EINVAL;
> -				goto out;
> +				goto unlock;
>  			}
>  
>  			phys = strndup_user(p, 1024);
>  			if (IS_ERR(phys)) {
>  				retval = PTR_ERR(phys);
> -				goto out;
> +				goto unlock;
>  			}
>  
>  			kfree(udev->dev->phys);
>  			udev->dev->phys = phys;
> -			goto out;
> +			goto unlock;
>  
>  		case UI_BEGIN_FF_UPLOAD:
>  			retval = uinput_ff_upload_from_user(p, &ff_up);
>  			if (retval)
> -				goto out;
> +				goto unlock;
>  
>  			req = uinput_request_find(udev, ff_up.request_id);
>  			if (!req || req->code != UI_FF_UPLOAD ||
>  			    !req->u.upload.effect) {
>  				retval = -EINVAL;
> -				goto out;
> +				goto unlock;
>  			}
>  
>  			ff_up.retval = 0;
> @@ -811,60 +811,60 @@ static long uinput_ioctl_handler(struct file *file, unsigned int cmd,
>  				memset(&ff_up.old, 0, sizeof(struct ff_effect));
>  
>  			retval = uinput_ff_upload_to_user(p, &ff_up);
> -			goto out;
> +			goto unlock;
>  
>  		case UI_BEGIN_FF_ERASE:
>  			if (copy_from_user(&ff_erase, p, sizeof(ff_erase))) {
>  				retval = -EFAULT;
> -				goto out;
> +				goto unlock;
>  			}
>  
>  			req = uinput_request_find(udev, ff_erase.request_id);
>  			if (!req || req->code != UI_FF_ERASE) {
>  				retval = -EINVAL;
> -				goto out;
> +				goto unlock;
>  			}
>  
>  			ff_erase.retval = 0;
>  			ff_erase.effect_id = req->u.effect_id;
>  			if (copy_to_user(p, &ff_erase, sizeof(ff_erase))) {
>  				retval = -EFAULT;
> -				goto out;
> +				goto unlock;
>  			}
>  
> -			goto out;
> +			goto unlock;
>  
>  		case UI_END_FF_UPLOAD:
>  			retval = uinput_ff_upload_from_user(p, &ff_up);
>  			if (retval)
> -				goto out;
> +				goto unlock;
>  
>  			req = uinput_request_find(udev, ff_up.request_id);
>  			if (!req || req->code != UI_FF_UPLOAD ||
>  			    !req->u.upload.effect) {
>  				retval = -EINVAL;
> -				goto out;
> +				goto unlock;
>  			}
>  
>  			req->retval = ff_up.retval;
>  			uinput_request_done(udev, req);
> -			goto out;
> +			goto unlock;
>  
>  		case UI_END_FF_ERASE:
>  			if (copy_from_user(&ff_erase, p, sizeof(ff_erase))) {
>  				retval = -EFAULT;
> -				goto out;
> +				goto unlock;
>  			}
>  
>  			req = uinput_request_find(udev, ff_erase.request_id);
>  			if (!req || req->code != UI_FF_ERASE) {
>  				retval = -EINVAL;
> -				goto out;
> +				goto unlock;
>  			}
>  
>  			req->retval = ff_erase.retval;
>  			uinput_request_done(udev, req);
> -			goto out;
> +			goto unlock;
>  	}
>  
>  	size = _IOC_SIZE(cmd);
> @@ -874,15 +874,15 @@ static long uinput_ioctl_handler(struct file *file, unsigned int cmd,
>  	case UI_GET_SYSNAME(0):
>  		if (udev->state != UIST_CREATED) {
>  			retval = -ENOENT;
> -			goto out;
> +			goto unlock;
>  		}
>  		name = dev_name(&udev->dev->dev);
>  		retval = uinput_str_to_user(p, name, size);
> -		goto out;
> +		goto unlock;
>  	}
>  
>  	retval = -EINVAL;
> - out:
> + unlock:
>  	mutex_unlock(&udev->mutex);
>  	return retval;
>  }
> -- 
> 2.6.3
> 

-- 
Dmitry

[PATCH 0/2] Input-at32psif: Fine-tuning for OOM handling in psif_probe()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sat, 2 Jul 2016 20:50:09 +0200

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

Markus Elfring (2):
  Return directly after a failed kzalloc()
  Remove two OOM messages

 drivers/input/serio/at32psif.c | 10 +++-------
 1 file changed, 3 insertions(+), 7 deletions(-)

-- 
2.9.0

[PATCH 1/2] Input-at32psif: Return directly after a failed kzalloc() in psif_probe()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sat, 2 Jul 2016 18:34:43 +0200

Return directly after a memory allocation failed at the beginning.

This issue was detected by using the Coccinelle software.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/input/serio/at32psif.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/input/serio/at32psif.c b/drivers/input/serio/at32psif.c
index 2e4ff5b..fcb769a 100644
--- a/drivers/input/serio/at32psif.c
+++ b/drivers/input/serio/at32psif.c
@@ -212,8 +212,7 @@ static int __init psif_probe(struct platform_device *pdev)
 	psif = kzalloc(sizeof(struct psif), GFP_KERNEL);
 	if (!psif) {
 		dev_dbg(&pdev->dev, "out of memory\n");
-		ret = -ENOMEM;
-		goto out;
+		return -ENOMEM;
 	}
 	psif->pdev = pdev;
 
@@ -297,7 +296,6 @@ out_free_io:
 	kfree(io);
 out_free_psif:
 	kfree(psif);
-out:
 	return ret;
 }
 
-- 
2.9.0

[PATCH 2/2] Input-at32psif: Remove OOM messages in psif_probe()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sat, 2 Jul 2016 20:34:18 +0200

Delete two debug messages because Linux will usually provide
an appropriate information for a memory allocation failure.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/input/serio/at32psif.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/input/serio/at32psif.c b/drivers/input/serio/at32psif.c
index fcb769a..b30503d 100644
--- a/drivers/input/serio/at32psif.c
+++ b/drivers/input/serio/at32psif.c
@@ -210,15 +210,13 @@ static int __init psif_probe(struct platform_device *pdev)
 	int ret;
 
 	psif = kzalloc(sizeof(struct psif), GFP_KERNEL);
-	if (!psif) {
-		dev_dbg(&pdev->dev, "out of memory\n");
+	if (!psif)
 		return -ENOMEM;
-	}
+
 	psif->pdev = pdev;
 
 	io = kzalloc(sizeof(struct serio), GFP_KERNEL);
 	if (!io) {
-		dev_dbg(&pdev->dev, "out of memory\n");
 		ret = -ENOMEM;
 		goto out_free_psif;
 	}
-- 
2.9.0

Re: [PATCH 2/2] Input-at32psif: Remove OOM messages in psif_probe()

[Julia Lawall]

On Sat, 2 Jul 2016, SF Markus Elfring wrote:

> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Sat, 2 Jul 2016 20:34:18 +0200
> 
> Delete two debug messages because Linux will usually provide
> an appropriate information for a memory allocation failure.
> 
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>  drivers/input/serio/at32psif.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/input/serio/at32psif.c b/drivers/input/serio/at32psif.c
> index fcb769a..b30503d 100644
> --- a/drivers/input/serio/at32psif.c
> +++ b/drivers/input/serio/at32psif.c
> @@ -210,15 +210,13 @@ static int __init psif_probe(struct platform_device *pdev)
>  	int ret;
>  
>  	psif = kzalloc(sizeof(struct psif), GFP_KERNEL);
> -	if (!psif) {
> -		dev_dbg(&pdev->dev, "out of memory\n");
> +	if (!psif)
>  		return -ENOMEM;
> -	}
> +

Why add a blank line here?

>  	psif->pdev = pdev;
>  
>  	io = kzalloc(sizeof(struct serio), GFP_KERNEL);
>  	if (!io) {
> -		dev_dbg(&pdev->dev, "out of memory\n");
>  		ret = -ENOMEM;
>  		goto out_free_psif;
>  	}
> -- 
> 2.9.0
> 
> 

Re: [PATCH 0/2] Input-at32psif: Fine-tuning for OOM handling in psif_probe()

[Joe Perches]

On Sat, 2016-07-02 at 21:00 +0200, SF Markus Elfring wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Sat, 2 Jul 2016 20:50:09 +0200
> 
> A few update suggestions were taken into account
> from static source code analysis.
> 
> Markus Elfring (2):
>   Return directly after a failed kzalloc()
>   Remove two OOM messages
> 
>  drivers/input/serio/at32psif.c | 10 +++-------
>  1 file changed, 3 insertions(+), 7 deletions(-)

What possible rationale is there for including this "references" header?
566ABCD9.1060404@users.sourceforge.net

This message id is for your message:
	"Source code review around jump label usage"
sent December 11, 2015!

Please stop adding unnecessary and useless email headers.
--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/2] Input-at32psif: Fine-tuning for OOM handling in psif_probe()

[SF Markus Elfring]

>> A few update suggestions were taken into account
>> from static source code analysis.
>>
>> Markus Elfring (2):
>>   Return directly after a failed kzalloc()
>>   Remove two OOM messages
>>
>>  drivers/input/serio/at32psif.c | 10 +++-------
>>  1 file changed, 3 insertions(+), 7 deletions(-)
> 
> What possible rationale is there for including this "references" header?
> 566ABCD9.1060404@users.sourceforge.net

Do any more software developers dare to reconsider source code
also around a jump label like "out"?


> This message id is for your message:
> 	"Source code review around jump label usage"
> sent December 11, 2015!

Can such an association with a bit of background information
be occasionally useful for clarification of corresponding
implementation details?

Regards,
Markus

Re: [PATCH 0/2] Input-at32psif: Fine-tuning for OOM handling in psif_probe()

[Dmitry Torokhov]

On Sat, Jul 02, 2016 at 09:00:36PM +0200, SF Markus Elfring wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Sat, 2 Jul 2016 20:50:09 +0200
> 
> A few update suggestions were taken into account
> from static source code analysis.
> 
> Markus Elfring (2):
>   Return directly after a failed kzalloc()
>   Remove two OOM messages

I do not see a compelling reason for taking these...

Thanks.

-- 
Dmitry

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

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
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

[PATCH 1/2] Input-evdev: Use kmalloc_array() in evdev_handle_get_val()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
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 <elfring@users.sourceforge.net>
---
 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

Re: [PATCH 1/2] Input-evdev: Use kmalloc_array() in evdev_handle_get_val()

[Dmitry Torokhov]

On Sat, Sep 24, 2016 at 4:08 AM, SF Markus Elfring
<elfring@users.sourceforge.net> wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> 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

Re: Input-evdev: Use kmalloc_array() in evdev_handle_get_val()

[SF Markus Elfring]

> 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

Re: Input-evdev: Use kmalloc_array() in evdev_handle_get_val()

[Dmitry Torokhov]

On Sat, Sep 24, 2016 at 08:16:16PM +0200, SF Markus Elfring wrote:
> > 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?

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

Re: Input-evdev: Use kmalloc_array() in evdev_handle_get_val()

[SF Markus Elfring]

> 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

[PATCH 2/2] Input-evdev: Rename a jump label in two functions

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
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 <elfring@users.sourceforge.net>
---
 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

Re: [PATCH 2/2] Input-evdev: Rename a jump label in two functions

[Dmitry Torokhov]

On Sat, Sep 24, 2016 at 4:10 AM, SF Markus Elfring
<elfring@users.sourceforge.net> wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> 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.

>
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>  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
>



-- 
Dmitry