[PATCH 0/4] video-UDLFB: Adjustments for five function implementations

From: Markus Elfring <redacted>
Date: Fri, 24 Nov 2017 21:45:54 +0100

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

Markus Elfring (4):
  Delete an error message for a failed memory allocation in two functions
  Return an error code only as a constant in dlfb_realloc_framebuffer()
  Improve a size determination in dlfb_alloc_urb_list()
  Delete an unnecessary return statement in two functions

 drivers/video/fbdev/udlfb.c | 23 +++++------------------
 1 file changed, 5 insertions(+), 18 deletions(-)

-- 
2.15.0

From: Markus Elfring <redacted>
Date: Fri, 24 Nov 2017 21:12:54 +0100

Omit an extra message for a memory allocation failure in these functions.

This issue was detected by using the Coccinelle software.

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

diff --git a/drivers/video/fbdev/udlfb.c b/drivers/video/fbdev/udlfb.c
index d44f14242016..77633c58bb9b 100644
--- a/drivers/video/fbdev/udlfb.c
+++ b/drivers/video/fbdev/udlfb.c

@@ -1175,10 +1175,8 @@ static int dlfb_realloc_framebuffer(struct dlfb_data *dev, struct fb_info *info)
 		 * Alloc system memory for virtual framebuffer
 		 */
 		new_fb = vmalloc(new_len);
-		if (!new_fb) {
-			pr_err("Virtual framebuffer alloc failed\n");
+		if (!new_fb)
 			goto error;
-		}
 
 		if (info->screen_base) {
 			memcpy(new_fb, old_fb, old_len);

@@ -1599,10 +1597,8 @@ static int dlfb_usb_probe(struct usb_interface *interface,
 	usbdev = interface_to_usbdev(interface);
 
 	dev = kzalloc(sizeof(*dev), GFP_KERNEL);
-	if (dev = NULL) {
-		dev_err(&interface->dev, "dlfb_usb_probe: failed alloc of dev struct\n");
+	if (!dev)
 		goto error;
-	}
 
 	kref_init(&dev->kref); /* matching kref_put in usb .disconnect fn */
 

-- 
2.15.0

From: Markus Elfring <redacted>
Date: Fri, 24 Nov 2017 21:22:25 +0100

* Return an error code without storing it in an intermediate variable.

* Delete the label "error" and local variable "retval"
  which became unnecessary with this refactoring.

Signed-off-by: Markus Elfring <redacted>
---
 drivers/video/fbdev/udlfb.c | 9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)

diff --git a/drivers/video/fbdev/udlfb.c b/drivers/video/fbdev/udlfb.c
index 77633c58bb9b..cb2d59ca67a4 100644
--- a/drivers/video/fbdev/udlfb.c
+++ b/drivers/video/fbdev/udlfb.c

@@ -1159,7 +1159,6 @@ static struct fb_ops dlfb_ops = {
  */
 static int dlfb_realloc_framebuffer(struct dlfb_data *dev, struct fb_info *info)
 {
-	int retval = -ENOMEM;
 	int old_len = info->fix.smem_len;
 	int new_len;
 	unsigned char *old_fb = info->screen_base;

@@ -1176,7 +1175,7 @@ static int dlfb_realloc_framebuffer(struct dlfb_data *dev, struct fb_info *info)
 		 */
 		new_fb = vmalloc(new_len);
 		if (!new_fb)
-			goto error;
+			return -ENOMEM;
 
 		if (info->screen_base) {
 			memcpy(new_fb, old_fb, old_len);

@@ -1203,11 +1202,7 @@ static int dlfb_realloc_framebuffer(struct dlfb_data *dev, struct fb_info *info)
 			dev->backing_buffer = new_back;
 		}
 	}
-
-	retval = 0;
-
-error:
-	return retval;
+	return 0;
 }
 
 /*

-- 
2.15.0

From: Markus Elfring <redacted>
Date: Fri, 24 Nov 2017 21:30:37 +0100

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.

This issue was detected by using the Coccinelle software.

Signed-off-by: Markus Elfring <redacted>
---
 drivers/video/fbdev/udlfb.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/video/fbdev/udlfb.c b/drivers/video/fbdev/udlfb.c
index cb2d59ca67a4..059883962698 100644
--- a/drivers/video/fbdev/udlfb.c
+++ b/drivers/video/fbdev/udlfb.c

@@ -1867,7 +1867,7 @@ static int dlfb_alloc_urb_list(struct dlfb_data *dev, int count, size_t size)
 	INIT_LIST_HEAD(&dev->urbs.list);
 
 	while (i < count) {
-		unode = kzalloc(sizeof(struct urb_node), GFP_KERNEL);
+		unode = kzalloc(sizeof(*unode), GFP_KERNEL);
 		if (!unode)
 			break;
 		unode->dev = dev;

-- 
2.15.0

From: Markus Elfring <redacted>
Date: Fri, 24 Nov 2017 21:36:39 +0100

The script "checkpatch.pl" pointed information out like the following.

WARNING: void function return statements are not generally useful

Thus remove such a statement in the affected functions.

Signed-off-by: Markus Elfring <redacted>
---
 drivers/video/fbdev/udlfb.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/drivers/video/fbdev/udlfb.c b/drivers/video/fbdev/udlfb.c
index 059883962698..c6bf7088f5f4 100644
--- a/drivers/video/fbdev/udlfb.c
+++ b/drivers/video/fbdev/udlfb.c

@@ -505,8 +505,6 @@ static void dlfb_compress_hline(
 	*command_buffer_ptr = cmd;
 	*pixel_start_ptr = pixel;
 	*device_address_ptr = dev_addr;
-
-	return;
 }
 
 /*

@@ -1768,8 +1766,6 @@ static void dlfb_usb_disconnect(struct usb_interface *interface)
 	kref_put(&dev->kref, dlfb_free);
 
 	/* consider dlfb_data freed */
-
-	return;
 }
 
 static struct usb_driver dlfb_driver = {

-- 
2.15.0

On Friday, November 24, 2017 09:48:14 PM SF Markus Elfring wrote:

From: Markus Elfring <redacted>
Date: Fri, 24 Nov 2017 21:45:54 +0100

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

Markus Elfring (4):
  Delete an error message for a failed memory allocation in two functions

This patch removes the information about the device for which the allocation
fails.

  Return an error code only as a constant in dlfb_realloc_framebuffer()

This patch depends on the earlier patch (which is not being merged) so please
re-base it if you want it to be applied.

  Improve a size determination in dlfb_alloc_urb_list()

Patch queued for 4.16, thanks.

  Delete an unnecessary return statement in two functions

Patch queued for 4.16, thanks.
 

 drivers/video/fbdev/udlfb.c | 23 +++++------------------
 1 file changed, 5 insertions(+), 18 deletions(-)

Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics

quoted

  Delete an error message for a failed memory allocation in two functions

This patch removes the information about the device for which the allocation fails.

* Do you find a Linux allocation failure report insufficient in this use case?

* Are you looking for any more clarification?

quoted

  Improve a size determination in dlfb_alloc_urb_list()

Patch queued for 4.16, thanks.

Thanks for your acceptance of other change possibilities.

Regards,
Markus

On Friday, December 29, 2017 07:10:00 PM SF Markus Elfring wrote:

quoted

quoted

  Delete an error message for a failed memory allocation in two functions

This patch removes the information about the device for which the allocation fails.

* Do you find a Linux allocation failure report insufficient in this use case?

Yes, there is more information available currently in the driver and
I see no real improvement in removing it.

* Are you looking for any more clarification?

I will not apply any of such patches for now. The only exception
being drivers that support hardware that can have only one instance
in the system (but it needs to be explicitly stated in the patch
description and the patch needs to be reviewed by a someone else
than the author).

Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics

quoted

* Do you find a Linux allocation failure report insufficient in this use case?

Yes,

Interesting …

there is more information available currently in the driver and
I see no real improvement in removing it.

quoted

* Are you looking for any more clarification?

I will not apply any of such patches for now. The only exception
being drivers that support hardware that can have only one instance
in the system …

Thanks for your feedback.

and the patch needs to be reviewed by a someone else than the author).

I am curious if this will ever happen again for my update suggestions
in such a software area.

Regards,
Markus

From: Markus Elfring <redacted>
Date: Sun, 7 Jan 2018 14:24:34 +0100

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

Markus Elfring (2):
  Return an error code only as a constant
  Delete an error message for a failed memory allocation

---

v2:
Rebased on Linux next-20180105.

 drivers/video/fbdev/udlfb.c | 13 +++----------
 1 file changed, 3 insertions(+), 10 deletions(-)

-- 
2.15.1

From: Markus Elfring <redacted>
Date: Sun, 7 Jan 2018 14:02:36 +0100

* Return an error code without storing it in an intermediate variable.

* Delete the label "error" and local variable "retval"
  which became unnecessary with this refactoring.

Signed-off-by: Markus Elfring <redacted>
---

v2:
This update suggestion was rebased on source files from the software
"Linux next-20180105".

 drivers/video/fbdev/udlfb.c | 9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)

diff --git a/drivers/video/fbdev/udlfb.c b/drivers/video/fbdev/udlfb.c
index 99ce445986b3..560a6b6044a5 100644
--- a/drivers/video/fbdev/udlfb.c
+++ b/drivers/video/fbdev/udlfb.c

@@ -1157,7 +1157,6 @@ static struct fb_ops dlfb_ops = {
  */
 static int dlfb_realloc_framebuffer(struct dlfb_data *dev, struct fb_info *info)
 {
-	int retval = -ENOMEM;
 	int old_len = info->fix.smem_len;
 	int new_len;
 	unsigned char *old_fb = info->screen_base;

@@ -1175,7 +1174,7 @@ static int dlfb_realloc_framebuffer(struct dlfb_data *dev, struct fb_info *info)
 		new_fb = vmalloc(new_len);
 		if (!new_fb) {
 			pr_err("Virtual framebuffer alloc failed\n");
-			goto error;
+			return -ENOMEM;
 		}
 
 		if (info->screen_base) {

@@ -1203,11 +1202,7 @@ static int dlfb_realloc_framebuffer(struct dlfb_data *dev, struct fb_info *info)
 			dev->backing_buffer = new_back;
 		}
 	}
-
-	retval = 0;
-
-error:
-	return retval;
+	return 0;
 }
 
 /*

-- 
2.15.1

From: Markus Elfring <redacted>
Date: Sun, 7 Jan 2018 14:07:36 +0100

Omit an extra message for a memory allocation failure in this function.

This issue was detected by using the Coccinelle software.

Signed-off-by: Markus Elfring <redacted>
---

v2:
This update suggestion was rebased on source files from the software
"Linux next-20180105" together with an other change combination.

 drivers/video/fbdev/udlfb.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/video/fbdev/udlfb.c b/drivers/video/fbdev/udlfb.c
index 560a6b6044a5..0c781b077aab 100644
--- a/drivers/video/fbdev/udlfb.c
+++ b/drivers/video/fbdev/udlfb.c

@@ -1172,10 +1172,8 @@ static int dlfb_realloc_framebuffer(struct dlfb_data *dev, struct fb_info *info)
 		 * Alloc system memory for virtual framebuffer
 		 */
 		new_fb = vmalloc(new_len);
-		if (!new_fb) {
-			pr_err("Virtual framebuffer alloc failed\n");
+		if (!new_fb)
 			return -ENOMEM;
-		}
 
 		if (info->screen_base) {
 			memcpy(new_fb, old_fb, old_len);

-- 
2.15.1

On Sunday, January 07, 2018 02:34:51 PM SF Markus Elfring wrote:

From: Markus Elfring <redacted>
Date: Sun, 7 Jan 2018 14:02:36 +0100

* Return an error code without storing it in an intermediate variable.

* Delete the label "error" and local variable "retval"
  which became unnecessary with this refactoring.

Signed-off-by: Markus Elfring <redacted>

Patch queued for 4.17, thanks.

Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics