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

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

[SF Markus Elfring]

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

[PATCH 1/8] [media] v4l2-async: Use kmalloc_array() in v4l2_async_notifier_unregister()

[SF Markus Elfring]

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

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

diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c
index 5bada202b2d3..277183f2d514 100644
--- a/drivers/media/v4l2-core/v4l2-async.c
+++ b/drivers/media/v4l2-core/v4l2-async.c
@@ -202,7 +202,7 @@ void v4l2_async_notifier_unregister(struct v4l2_async_notifier *notifier)
 	if (!notifier->v4l2_dev)
 		return;
 
-	dev = kmalloc(n_subdev * sizeof(*dev), GFP_KERNEL);
+	dev = kmalloc_array(n_subdev, sizeof(*dev), GFP_KERNEL);
 	if (!dev) {
 		dev_err(notifier->v4l2_dev->dev,
 			"Failed to allocate device cache!\n");
-- 
2.11.0

[PATCH 2/8] [media] v4l2-async: Delete an error message for a failed memory allocation in v4l2_async_notifier_unregister()

[SF Markus Elfring]

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

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

WARNING: Possible unnecessary 'out of memory' message

Thus fix the affected source code place.

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

diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c
index 277183f2d514..812d0b2a2f73 100644
--- a/drivers/media/v4l2-core/v4l2-async.c
+++ b/drivers/media/v4l2-core/v4l2-async.c
@@ -203,11 +203,6 @@ void v4l2_async_notifier_unregister(struct v4l2_async_notifier *notifier)
 		return;
 
 	dev = kmalloc_array(n_subdev, sizeof(*dev), GFP_KERNEL);
-	if (!dev) {
-		dev_err(notifier->v4l2_dev->dev,
-			"Failed to allocate device cache!\n");
-	}
-
 	mutex_lock(&list_lock);
 
 	list_del(&notifier->list);
-- 
2.11.0

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

[SF Markus Elfring]

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

[PATCH 4/8] [media] videobuf-dma-sg: Adjust 24 checks for null values

[SF Markus Elfring]

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

Convert comparisons with the preprocessor symbol "NULL" or the value "0"
to condition checks without it.

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

diff --git a/drivers/media/v4l2-core/videobuf-dma-sg.c b/drivers/media/v4l2-core/videobuf-dma-sg.c
index ab3c1f6a2ca1..9ccdc11aa016 100644
--- a/drivers/media/v4l2-core/videobuf-dma-sg.c
+++ b/drivers/media/v4l2-core/videobuf-dma-sg.c
@@ -70,12 +70,12 @@ static struct scatterlist *videobuf_vmalloc_to_sg(unsigned char *virt,
 	int i;
 
 	sglist = vzalloc(nr_pages * sizeof(*sglist));
-	if (NULL == sglist)
+	if (!sglist)
 		return NULL;
 	sg_init_table(sglist, nr_pages);
 	for (i = 0; i < nr_pages; i++, virt += PAGE_SIZE) {
 		pg = vmalloc_to_page(virt);
-		if (NULL == pg)
+		if (!pg)
 			goto err;
 		BUG_ON(PageHighMem(pg));
 		sg_set_page(&sglist[i], pg, PAGE_SIZE, 0);
@@ -98,10 +98,10 @@ static struct scatterlist *videobuf_pages_to_sg(struct page **pages,
 	struct scatterlist *sglist;
 	int i;
 
-	if (NULL == pages[0])
+	if (!pages[0])
 		return NULL;
 	sglist = vmalloc(nr_pages * sizeof(*sglist));
-	if (NULL == sglist)
+	if (!sglist)
 		return NULL;
 	sg_init_table(sglist, nr_pages);
 
@@ -112,7 +112,7 @@ static struct scatterlist *videobuf_pages_to_sg(struct page **pages,
 			min_t(size_t, PAGE_SIZE - offset, size), offset);
 	size -= min_t(size_t, PAGE_SIZE - offset, size);
 	for (i = 1; i < nr_pages; i++) {
-		if (NULL == pages[i])
+		if (!pages[i])
 			goto nopage;
 		if (PageHighMem(pages[i]))
 			goto highmem;
@@ -178,7 +178,7 @@ static int videobuf_dma_init_user_locked(struct videobuf_dmabuf *dma,
 	dma->pages = kmalloc_array(dma->nr_pages,
 				   sizeof(*dma->pages),
 				   GFP_KERNEL);
-	if (NULL == dma->pages)
+	if (!dma->pages)
 		return -ENOMEM;
 
 	if (rw == READ)
@@ -233,14 +233,14 @@ static int videobuf_dma_init_kernel(struct videobuf_dmabuf *dma, int direction,
 
 		addr = dma_alloc_coherent(dma->dev, PAGE_SIZE,
 					  &(dma->dma_addr[i]), GFP_KERNEL);
-		if (addr == NULL)
+		if (!addr)
 			goto out_free_pages;
 
 		dma->vaddr_pages[i] = virt_to_page(addr);
 	}
 	dma->vaddr = vmap(dma->vaddr_pages, nr_pages, VM_MAP | VM_IOREMAP,
 			  PAGE_KERNEL);
-	if (NULL == dma->vaddr) {
+	if (!dma->vaddr) {
 		dprintk(1, "vmalloc_32(%d pages) failed\n", nr_pages);
 		goto out_free_pages;
 	}
@@ -277,7 +277,7 @@ static int videobuf_dma_init_overlay(struct videobuf_dmabuf *dma, int direction,
 		nr_pages, (unsigned long)addr);
 	dma->direction = direction;
 
-	if (0 == addr)
+	if (!addr)
 		return -EINVAL;
 
 	dma->bus_addr = addr;
@@ -289,7 +289,7 @@ static int videobuf_dma_init_overlay(struct videobuf_dmabuf *dma, int direction,
 static int videobuf_dma_map(struct device *dev, struct videobuf_dmabuf *dma)
 {
 	MAGIC_CHECK(dma->magic, MAGIC_DMABUF);
-	BUG_ON(0 == dma->nr_pages);
+	BUG_ON(!dma->nr_pages);
 
 	if (dma->pages) {
 		dma->sglist = videobuf_pages_to_sg(dma->pages, dma->nr_pages,
@@ -301,7 +301,7 @@ static int videobuf_dma_map(struct device *dev, struct videobuf_dmabuf *dma)
 	}
 	if (dma->bus_addr) {
 		dma->sglist = vmalloc(sizeof(*dma->sglist));
-		if (NULL != dma->sglist) {
+		if (dma->sglist) {
 			dma->sglen = 1;
 			sg_dma_address(&dma->sglist[0])	= dma->bus_addr
 							& PAGE_MASK;
@@ -309,14 +309,14 @@ static int videobuf_dma_map(struct device *dev, struct videobuf_dmabuf *dma)
 			sg_dma_len(&dma->sglist[0]) = dma->nr_pages * PAGE_SIZE;
 		}
 	}
-	if (NULL == dma->sglist) {
+	if (!dma->sglist) {
 		dprintk(1, "scatterlist is NULL\n");
 		return -ENOMEM;
 	}
 	if (!dma->bus_addr) {
 		dma->sglen = dma_map_sg(dev, dma->sglist,
 					dma->nr_pages, dma->direction);
-		if (0 == dma->sglen) {
+		if (!dma->sglen) {
 			printk(KERN_WARNING
 			       "%s: videobuf_map_sg failed\n", __func__);
 			vfree(dma->sglist);
@@ -406,11 +406,11 @@ static void videobuf_vm_close(struct vm_area_struct *vma)
 		map->count, vma->vm_start, vma->vm_end);
 
 	map->count--;
-	if (0 == map->count) {
+	if (!map->count) {
 		dprintk(1, "munmap %p q=%p\n", map, q);
 		videobuf_queue_lock(q);
 		for (i = 0; i < VIDEO_MAX_FRAME; i++) {
-			if (NULL == q->bufs[i])
+			if (!q->bufs[i])
 				continue;
 			mem = q->bufs[i]->priv;
 			if (!mem)
@@ -518,20 +518,20 @@ static int __videobuf_iolock(struct videobuf_queue *q,
 	switch (vb->memory) {
 	case V4L2_MEMORY_MMAP:
 	case V4L2_MEMORY_USERPTR:
-		if (0 == vb->baddr) {
+		if (!vb->baddr) {
 			/* no userspace addr -- kernel bounce buffer */
 			pages = PAGE_ALIGN(vb->size) >> PAGE_SHIFT;
 			err = videobuf_dma_init_kernel(&mem->dma,
 						       DMA_FROM_DEVICE,
 						       pages);
-			if (0 != err)
+			if (err)
 				return err;
 		} else if (vb->memory == V4L2_MEMORY_USERPTR) {
 			/* dma directly to userspace */
 			err = videobuf_dma_init_user(&mem->dma,
 						     DMA_FROM_DEVICE,
 						     vb->baddr, vb->bsize);
-			if (0 != err)
+			if (err)
 				return err;
 		} else {
 			/* NOTE: HACK: videobuf_iolock on V4L2_MEMORY_MMAP
@@ -542,12 +542,12 @@ static int __videobuf_iolock(struct videobuf_queue *q,
 			err = videobuf_dma_init_user_locked(&mem->dma,
 						      DMA_FROM_DEVICE,
 						      vb->baddr, vb->bsize);
-			if (0 != err)
+			if (err)
 				return err;
 		}
 		break;
 	case V4L2_MEMORY_OVERLAY:
-		if (NULL == fbuf)
+		if (!fbuf)
 			return -EINVAL;
 		/* FIXME: need sanity checks for vb->boff */
 		/*
@@ -559,14 +559,14 @@ static int __videobuf_iolock(struct videobuf_queue *q,
 		pages = PAGE_ALIGN(vb->size) >> PAGE_SHIFT;
 		err = videobuf_dma_init_overlay(&mem->dma, DMA_FROM_DEVICE,
 						bus, pages);
-		if (0 != err)
+		if (err)
 			return err;
 		break;
 	default:
 		BUG();
 	}
 	err = videobuf_dma_map(q->dev, &mem->dma);
-	if (0 != err)
+	if (err)
 		return err;
 
 	return 0;
@@ -621,12 +621,12 @@ static int __videobuf_mmap_mapper(struct videobuf_queue *q,
 	/* create mapping + update buffer list */
 	retval = -ENOMEM;
 	map = kmalloc(sizeof(struct videobuf_mapping), GFP_KERNEL);
-	if (NULL == map)
+	if (!map)
 		goto done;
 
 	size = 0;
 	for (i = first; i <= last; i++) {
-		if (NULL == q->bufs[i])
+		if (!q->bufs[i])
 			continue;
 		q->bufs[i]->map   = map;
 		q->bufs[i]->baddr = vma->vm_start + size;
-- 
2.11.0

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

[SF Markus Elfring]

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

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

[SF Markus Elfring]

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

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

[SF Markus Elfring]

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

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

[SF Markus Elfring]

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

Re: [PATCH 1/8] [media] v4l2-async: Use kmalloc_array() in v4l2_async_notifier_unregister()

[Sakari Ailus]

Thanks!

On Mon, Dec 26, 2016 at 09:43:23PM +0100, SF Markus Elfring wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Mon, 26 Dec 2016 18:14:33 +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.
> 
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>

Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com>

> ---
>  drivers/media/v4l2-core/v4l2-async.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c
> index 5bada202b2d3..277183f2d514 100644
> --- a/drivers/media/v4l2-core/v4l2-async.c
> +++ b/drivers/media/v4l2-core/v4l2-async.c
> @@ -202,7 +202,7 @@ void v4l2_async_notifier_unregister(struct v4l2_async_notifier *notifier)
>  	if (!notifier->v4l2_dev)
>  		return;
>  
> -	dev = kmalloc(n_subdev * sizeof(*dev), GFP_KERNEL);
> +	dev = kmalloc_array(n_subdev, sizeof(*dev), GFP_KERNEL);
>  	if (!dev) {
>  		dev_err(notifier->v4l2_dev->dev,
>  			"Failed to allocate device cache!\n");

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

Re: [PATCH 2/8] [media] v4l2-async: Delete an error message for a failed memory allocation in v4l2_async_notifier_unregister()

[Sakari Ailus]

On Mon, Dec 26, 2016 at 09:45:50PM +0100, SF Markus Elfring wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Mon, 26 Dec 2016 19:19:49 +0100
> 
> The script "checkpatch.pl" pointed information out like the following.
> 
> WARNING: Possible unnecessary 'out of memory' message
> 
> Thus fix the affected source code place.
> 
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>  drivers/media/v4l2-core/v4l2-async.c | 5 -----
>  1 file changed, 5 deletions(-)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c
> index 277183f2d514..812d0b2a2f73 100644
> --- a/drivers/media/v4l2-core/v4l2-async.c
> +++ b/drivers/media/v4l2-core/v4l2-async.c
> @@ -203,11 +203,6 @@ void v4l2_async_notifier_unregister(struct v4l2_async_notifier *notifier)
>  		return;
>  
>  	dev = kmalloc_array(n_subdev, sizeof(*dev), GFP_KERNEL);
> -	if (!dev) {
> -		dev_err(notifier->v4l2_dev->dev,
> -			"Failed to allocate device cache!\n");
> -	}
> -

I'd leave the empty line where it is.

Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com>

>  	mutex_lock(&list_lock);
>  
>  	list_del(&notifier->list);
> -- 
> 2.11.0
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-media" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

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

[Sakari Ailus]

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

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

[Hans Verkuil]

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

Re: [PATCH 4/8] [media] videobuf-dma-sg: Adjust 24 checks for null values

[Dan Carpenter]

On Mon, Dec 26, 2016 at 09:48:19PM +0100, SF Markus Elfring wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Mon, 26 Dec 2016 20:30:19 +0100
> 
> Convert comparisons with the preprocessor symbol "NULL" or the value "0"
> to condition checks without it.

Generally lengths are numbers and not booleans so "len == 0" is ok.
Checkpatch doesn't complain about that.

regards,
dan carpenter

Re: [PATCH 2/8] [media] v4l2-async: Delete an error message for a failed memory allocation in v4l2_async_notifier_unregister()

[Mauro Carvalho Chehab]

Em Mon, 26 Dec 2016 21:45:50 +0100
SF Markus Elfring <elfring@users.sourceforge.net> escreveu:

> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Mon, 26 Dec 2016 19:19:49 +0100
> 
> The script "checkpatch.pl" pointed information out like the following.
> 
> WARNING: Possible unnecessary 'out of memory' message
> 
> Thus fix the affected source code place.
> 
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>  drivers/media/v4l2-core/v4l2-async.c | 5 -----
>  1 file changed, 5 deletions(-)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c
> index 277183f2d514..812d0b2a2f73 100644
> --- a/drivers/media/v4l2-core/v4l2-async.c
> +++ b/drivers/media/v4l2-core/v4l2-async.c
> @@ -203,11 +203,6 @@ void v4l2_async_notifier_unregister(struct v4l2_async_notifier *notifier)
>  		return;
>  
>  	dev = kmalloc_array(n_subdev, sizeof(*dev), GFP_KERNEL);
> -	if (!dev) {
> -		dev_err(notifier->v4l2_dev->dev,
> -			"Failed to allocate device cache!\n");
> -	}
> -

In this specific case, we should keep it, as the message means that
the unregister logic won't work properly, as this loop won't run:

        /*
         * Call device_attach() to reprobe devices
         *
         * NOTE: If dev allocation fails, i is 0, and the whole loop won't be
         * executed.
         */
	while (i--) {
                struct device *d = dev[i];

                if (d && device_attach(d) < 0) {
                        const char *name = "(none)";
                        int lock = device_trylock(d);

                        if (lock && d->driver)
                                name = d->driver->name;
                        dev_err(d, "Failed to re-probe to %s\n", name);
                        if (lock)
                                device_unlock(d);
                }
                put_device(d);
        }

So, IMHO, the proper patch would be to change the message to
be more comprehensive, describing the consequences of not being
able to allocate the dev cache.


>  	mutex_lock(&list_lock);
>  
>  	list_del(&notifier->list);



Thanks,
Mauro

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

[SF Markus Elfring]

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

Will the software evolution be continued for related source files?
Are there any update candidates left over in the directory “v4l2-core”?

Regards,
Markus

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

[Hans Verkuil]

On 22/09/17 22:08, SF Markus Elfring wrote:
>> Sorry Markus, just stay away from the videobuf-* sources.
> 
> Will the software evolution be continued for related source files?
> Are there any update candidates left over in the directory “v4l2-core”?

Sorry, I don't understand the question. We don't want to touch the
videobuf-* files unless there is a very good reason. That old videobuf
framework is deprecated and the code is quite fragile (i.e. easy to break
things).

Everything else in that directory is under continuous development.

It's core code though, so it gets a much more in-depth code review than
patches for e.g. a driver.

Regards,

	Hans

Re: [media] v4l2-core: Fine-tuning for some function implementations

[SF Markus Elfring]

>> Will the software evolution be continued for related source files?
>> Are there any update candidates left over in the directory “v4l2-core”?
> 
> Sorry, I don't understand the question.

I try to explain my view again.


> We don't want to touch the videobuf-* files unless there is a very good reason.

I hoped that my update suggestions could be good enough once more for this area.


> That old videobuf framework is deprecated and the code is quite fragile
> (i.e. easy to break things).

How do you think about to move this stuff into a separate subdirectory
so that it might become a bit easier to distinguish these software components?


> Everything else in that directory is under continuous development.

I am curious if there are still update candidates left over
(also from my selection of change possibilities).

Regards,
Markus