Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1547386 > unrolled thread

[PATCH 0/8] [media] v4l2-core: Fine-tuning for some function implementations

Back to article view | Back to linux.kernel

Contents

  [PATCH 0/8] [media] v4l2-core: Fine-tuning for some function  implementations SF Markus Elfring <elfring@users.sourceforge.net> - 2016-12-26 21:50 +0100
    [PATCH 6/8] [media] videobuf-dma-sg: Improve a size determination in  __videobuf_mmap_mapper() SF Markus Elfring <elfring@users.sourceforge.net> - 2016-12-26 22:00 +0100
    [PATCH 8/8] [media] videobuf-dma-sg: Add some spaces for better code  readability in videobuf_dma_init_user_locked() SF Markus Elfring <elfring@users.sourceforge.net> - 2016-12-26 22:00 +0100
    [PATCH 3/8] [media] videobuf-dma-sg: Use kmalloc_array() in  videobuf_dma_init_user_locked() SF Markus Elfring <elfring@users.sourceforge.net> - 2016-12-26 22:00 +0100
    [PATCH 7/8] [media] videobuf-dma-sg: Delete an unnecessary return  statement in videobuf_vm_close() SF Markus Elfring <elfring@users.sourceforge.net> - 2016-12-26 22:00 +0100
    [PATCH 5/8] [media] videobuf-dma-sg: Move two assignments for error  codes in __videobuf_mmap_mapper() SF Markus Elfring <elfring@users.sourceforge.net> - 2016-12-26 22:00 +0100
    Re: [PATCH 0/8] [media] v4l2-core: Fine-tuning for some function  implementations Sakari Ailus <sakari.ailus@iki.fi> - 2016-12-27 13:00 +0100
      Re: [PATCH 0/8] [media] v4l2-core: Fine-tuning for some function  implementations Hans Verkuil <hverkuil@xs4all.nl> - 2017-01-02 16:00 +0100

#1547386 — [PATCH 0/8] [media] v4l2-core: Fine-tuning for some function implementations

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Mon, 26 Dec 2016 21:30:12 +0100

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

Markus Elfring (8):
  v4l2-async: Use kmalloc_array() in v4l2_async_notifier_unregister()
  v4l2-async: Delete an error message for a failed memory allocation in v4l2_async_notifier_unregister()
  videobuf-dma-sg: Use kmalloc_array() in videobuf_dma_init_user_locked()
  videobuf-dma-sg: Adjust 24 checks for null values
  videobuf-dma-sg: Move two assignments for error codes in __videobuf_mmap_mapper()
  videobuf-dma-sg: Improve a size determination in __videobuf_mmap_mapper()
  videobuf-dma-sg: Delete an unnecessary return statement in videobuf_vm_close()
  videobuf-dma-sg: Add some spaces for better code readability in videobuf_dma_init_user_locked()

 drivers/media/v4l2-core/v4l2-async.c      |  7 +---
 drivers/media/v4l2-core/videobuf-dma-sg.c | 65 ++++++++++++++++---------------
 2 files changed, 34 insertions(+), 38 deletions(-)

-- 
2.11.0

[toc] | [next] | [standalone]

#1547388 — [PATCH 6/8] [media] videobuf-dma-sg: Improve a size determination in __videobuf_mmap_mapper()

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Mon, 26 Dec 2016 20:56:41 +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.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/media/v4l2-core/videobuf-dma-sg.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/v4l2-core/videobuf-dma-sg.c b/drivers/media/v4l2-core/videobuf-dma-sg.c
index d09ddf2e56fe..070ba10bbdbc 100644
--- a/drivers/media/v4l2-core/videobuf-dma-sg.c
+++ b/drivers/media/v4l2-core/videobuf-dma-sg.c
@@ -618,7 +618,7 @@ static int __videobuf_mmap_mapper(struct videobuf_queue *q,
 	last = first;
 
 	/* create mapping + update buffer list */
-	map = kmalloc(sizeof(struct videobuf_mapping), GFP_KERNEL);
+	map = kmalloc(sizeof(*map), GFP_KERNEL);
 	if (!map) {
 		retval = -ENOMEM;
 		goto done;
-- 
2.11.0

[toc] | [prev] | [next] | [standalone]

#1547389 — [PATCH 8/8] [media] videobuf-dma-sg: Add some spaces for better code readability in videobuf_dma_init_user_locked()

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Mon, 26 Dec 2016 21:16:51 +0100

Use space characters at some source code places according to
the Linux coding style convention.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/media/v4l2-core/videobuf-dma-sg.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/media/v4l2-core/videobuf-dma-sg.c b/drivers/media/v4l2-core/videobuf-dma-sg.c
index c8658530da57..9f560373d49d 100644
--- a/drivers/media/v4l2-core/videobuf-dma-sg.c
+++ b/drivers/media/v4l2-core/videobuf-dma-sg.c
@@ -171,10 +171,10 @@ static int videobuf_dma_init_user_locked(struct videobuf_dmabuf *dma,
 	}
 
 	first = (data          & PAGE_MASK) >> PAGE_SHIFT;
-	last  = ((data+size-1) & PAGE_MASK) >> PAGE_SHIFT;
+	last  = ((data + size - 1) & PAGE_MASK) >> PAGE_SHIFT;
 	dma->offset = data & ~PAGE_MASK;
 	dma->size = size;
-	dma->nr_pages = last-first+1;
+	dma->nr_pages = last - first + 1;
 	dma->pages = kmalloc_array(dma->nr_pages,
 				   sizeof(*dma->pages),
 				   GFP_KERNEL);
-- 
2.11.0

[toc] | [prev] | [next] | [standalone]

#1547390 — [PATCH 3/8] [media] videobuf-dma-sg: Use kmalloc_array() in videobuf_dma_init_user_locked()

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Mon, 26 Dec 2016 19:46:56 +0100

* 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/media/v4l2-core/videobuf-dma-sg.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/media/v4l2-core/videobuf-dma-sg.c b/drivers/media/v4l2-core/videobuf-dma-sg.c
index ba63ca57ed7e..ab3c1f6a2ca1 100644
--- a/drivers/media/v4l2-core/videobuf-dma-sg.c
+++ b/drivers/media/v4l2-core/videobuf-dma-sg.c
@@ -175,7 +175,9 @@ static int videobuf_dma_init_user_locked(struct videobuf_dmabuf *dma,
 	dma->offset = data & ~PAGE_MASK;
 	dma->size = size;
 	dma->nr_pages = last-first+1;
-	dma->pages = kmalloc(dma->nr_pages * sizeof(struct page *), GFP_KERNEL);
+	dma->pages = kmalloc_array(dma->nr_pages,
+				   sizeof(*dma->pages),
+				   GFP_KERNEL);
 	if (NULL == dma->pages)
 		return -ENOMEM;
 
-- 
2.11.0

[toc] | [prev] | [next] | [standalone]

#1547391 — [PATCH 7/8] [media] videobuf-dma-sg: Delete an unnecessary return statement in videobuf_vm_close()

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Mon, 26 Dec 2016 21:09:01 +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 here.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/media/v4l2-core/videobuf-dma-sg.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/media/v4l2-core/videobuf-dma-sg.c b/drivers/media/v4l2-core/videobuf-dma-sg.c
index 070ba10bbdbc..c8658530da57 100644
--- a/drivers/media/v4l2-core/videobuf-dma-sg.c
+++ b/drivers/media/v4l2-core/videobuf-dma-sg.c
@@ -427,7 +427,6 @@ static void videobuf_vm_close(struct vm_area_struct *vma)
 		videobuf_queue_unlock(q);
 		kfree(map);
 	}
-	return;
 }
 
 /*
-- 
2.11.0

[toc] | [prev] | [next] | [standalone]

#1547392 — [PATCH 5/8] [media] videobuf-dma-sg: Move two assignments for error codes in __videobuf_mmap_mapper()

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Mon, 26 Dec 2016 20:48:50 +0100

Move two assignments for the local variable "retval" so that these statements
will only be executed if a previous action failed in this function.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/media/v4l2-core/videobuf-dma-sg.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/media/v4l2-core/videobuf-dma-sg.c b/drivers/media/v4l2-core/videobuf-dma-sg.c
index 9ccdc11aa016..d09ddf2e56fe 100644
--- a/drivers/media/v4l2-core/videobuf-dma-sg.c
+++ b/drivers/media/v4l2-core/videobuf-dma-sg.c
@@ -596,8 +596,6 @@ static int __videobuf_mmap_mapper(struct videobuf_queue *q,
 	unsigned int first, last, size = 0, i;
 	int retval;
 
-	retval = -EINVAL;
-
 	BUG_ON(!mem);
 	MAGIC_CHECK(mem->magic, MAGIC_SG_MEM);
 
@@ -613,16 +611,18 @@ static int __videobuf_mmap_mapper(struct videobuf_queue *q,
 	if (!size) {
 		dprintk(1, "mmap app bug: offset invalid [offset=0x%lx]\n",
 				(vma->vm_pgoff << PAGE_SHIFT));
+		retval = -EINVAL;
 		goto done;
 	}
 
 	last = first;
 
 	/* create mapping + update buffer list */
-	retval = -ENOMEM;
 	map = kmalloc(sizeof(struct videobuf_mapping), GFP_KERNEL);
-	if (!map)
+	if (!map) {
+		retval = -ENOMEM;
 		goto done;
+	}
 
 	size = 0;
 	for (i = first; i <= last; i++) {
-- 
2.11.0

[toc] | [prev] | [next] | [standalone]

#1547575

Hi Markus,

On Mon, Dec 26, 2016 at 09:41:19PM +0100, SF Markus Elfring wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Mon, 26 Dec 2016 21:30:12 +0100
> 
> Some update suggestions were taken into account
> from static source code analysis.
> 
> Markus Elfring (8):
>   v4l2-async: Use kmalloc_array() in v4l2_async_notifier_unregister()
>   v4l2-async: Delete an error message for a failed memory allocation in v4l2_async_notifier_unregister()
>   videobuf-dma-sg: Use kmalloc_array() in videobuf_dma_init_user_locked()
>   videobuf-dma-sg: Adjust 24 checks for null values
>   videobuf-dma-sg: Move two assignments for error codes in __videobuf_mmap_mapper()
>   videobuf-dma-sg: Improve a size determination in __videobuf_mmap_mapper()
>   videobuf-dma-sg: Delete an unnecessary return statement in videobuf_vm_close()
>   videobuf-dma-sg: Add some spaces for better code readability in videobuf_dma_init_user_locked()

I don't really disagree with the videobuf changes as such --- the original
code sure seems quite odd, but I wonder whether we want to do this kind of
cleanups in videobuf. Videobuf will be removed likely in not too distant
future; when exactly, Hans can guesstimate better than me. Cc him.

-- 
Kind regards,

Sakari Ailus
e-mail: sakari.ailus@iki.fi	XMPP: sailus@retiisi.org.uk

[toc] | [prev] | [next] | [standalone]

#1549233

On 12/27/16 12:51, Sakari Ailus wrote:
> Hi Markus,
>
> On Mon, Dec 26, 2016 at 09:41:19PM +0100, SF Markus Elfring wrote:
>> From: Markus Elfring <elfring@users.sourceforge.net>
>> Date: Mon, 26 Dec 2016 21:30:12 +0100
>>
>> Some update suggestions were taken into account
>> from static source code analysis.
>>
>> Markus Elfring (8):
>>   v4l2-async: Use kmalloc_array() in v4l2_async_notifier_unregister()
>>   v4l2-async: Delete an error message for a failed memory allocation in v4l2_async_notifier_unregister()
>>   videobuf-dma-sg: Use kmalloc_array() in videobuf_dma_init_user_locked()
>>   videobuf-dma-sg: Adjust 24 checks for null values
>>   videobuf-dma-sg: Move two assignments for error codes in __videobuf_mmap_mapper()
>>   videobuf-dma-sg: Improve a size determination in __videobuf_mmap_mapper()
>>   videobuf-dma-sg: Delete an unnecessary return statement in videobuf_vm_close()
>>   videobuf-dma-sg: Add some spaces for better code readability in videobuf_dma_init_user_locked()
>
> I don't really disagree with the videobuf changes as such --- the original
> code sure seems quite odd, but I wonder whether we want to do this kind of
> cleanups in videobuf. Videobuf will be removed likely in not too distant
> future; when exactly, Hans can guesstimate better than me. Cc him.
>

The videobuf code is frozen as far as I am concerned, and I won't pick up
these cleanup patches. While they look perfectly reasonable, I don't want
to risk any breakage there. The last thing I want to do is to have to debug
in the videobuf code.

Sorry Markus, just stay away from the videobuf-* sources.

Regards,

	Hans

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web