[PATCH 1/3] media: staging: atomisp: Return an error code in case of error in 'lm3554_probe()'
If 'v4l2_ctrl_handler_init()' fails, we go to the error handling path, do
some clean-up and return err, which is known to be 0 (i.e. success).
Axe the 'ret' variable and use 'err' directly in order to return the error
code instead.
Also remove the initialization of 'err' which was hiding this issue.
Signed-off-by: Christophe JAILLET
---
drivers/staging/media/atomisp/i2c/atomisp-lm3554.c | 10 --
1 file changed, 4 insertions(+), 6 deletions(-)
diff --git a/drivers/staging/media/atomisp/i2c/atomisp-lm3554.c
b/drivers/staging/media/atomisp/i2c/atomisp-lm3554.c
index 7098bf317f16..723fa74ff815 100644
--- a/drivers/staging/media/atomisp/i2c/atomisp-lm3554.c
+++ b/drivers/staging/media/atomisp/i2c/atomisp-lm3554.c
@@ -852,10 +852,9 @@ static void *lm3554_platform_data_func(struct i2c_client
*client)
static int lm3554_probe(struct i2c_client *client)
{
- int err = 0;
+ int err;
struct lm3554 *flash;
unsigned int i;
- int ret;
flash = kzalloc(sizeof(*flash), GFP_KERNEL);
if (!flash)
@@ -868,10 +867,9 @@ static int lm3554_probe(struct i2c_client *client)
flash->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
flash->mode = ATOMISP_FLASH_MODE_OFF;
flash->timeout = LM3554_MAX_TIMEOUT / LM3554_TIMEOUT_STEPSIZE - 1;
- ret =
- v4l2_ctrl_handler_init(&flash->ctrl_handler,
- ARRAY_SIZE(lm3554_controls));
- if (ret) {
+ err = v4l2_ctrl_handler_init(&flash->ctrl_handler,
+ARRAY_SIZE(lm3554_controls));
+ if (err) {
dev_err(&client->dev, "error initialize a ctrl_handler.\n");
goto fail2;
}
--
2.17.0
[PATCH 2/3] media: staging: atomisp: Fix an error handling path in 'lm3554_probe()'
The use of 'fail1' and 'fail2' is not correct. Reorder these calls to
branch at the right place of the error handling path.
Signed-off-by: Christophe JAILLET
---
drivers/staging/media/atomisp/i2c/atomisp-lm3554.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/drivers/staging/media/atomisp/i2c/atomisp-lm3554.c
b/drivers/staging/media/atomisp/i2c/atomisp-lm3554.c
index 723fa74ff815..1e5f516f6e50 100644
--- a/drivers/staging/media/atomisp/i2c/atomisp-lm3554.c
+++ b/drivers/staging/media/atomisp/i2c/atomisp-lm3554.c
@@ -871,7 +871,7 @@ static int lm3554_probe(struct i2c_client *client)
ARRAY_SIZE(lm3554_controls));
if (err) {
dev_err(&client->dev, "error initialize a ctrl_handler.\n");
- goto fail2;
+ goto fail1;
}
for (i = 0; i < ARRAY_SIZE(lm3554_controls); i++)
@@ -879,7 +879,6 @@ static int lm3554_probe(struct i2c_client *client)
NULL);
if (flash->ctrl_handler.error) {
-
dev_err(&client->dev, "ctrl_handler error.\n");
goto fail2;
}
@@ -888,7 +887,7 @@ static int lm3554_probe(struct i2c_client *client)
err = media_entity_pads_init(&flash->sd.entity, 0, NULL);
if (err) {
dev_err(&client->dev, "error initialize a media entity.\n");
- goto fail1;
+ goto fail2;
}
flash->sd.entity.function = MEDIA_ENT_F_FLASH;
--
2.17.0
[PATCH 3/3] media: staging: atomisp: Fix usage of 'media_entity_cleanup()'
According to the doc, 'media_entity_cleanup()' must be called after unregistering the entity. All places I've check do it that way. So, move the call after 'v4l2_device_unregister_subdev()' as done elsewhere. Actually, this is not an issue, because 'media_entity_cleanup()' does nothing, but it is more future proof. Signed-off-by: Christophe JAILLET --- The change from '&flash->sd.entity' to '&sd->entity' in the last hunk is done because most of the drivers I've checked do it that way. Not sure if it is correct. It looks logical to me and it can be compiled. That's all I know. If this patch is reviewed and confirmed, I'll propose similar fixes for some other drivers. --- drivers/staging/media/atomisp/i2c/atomisp-lm3554.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/drivers/staging/media/atomisp/i2c/atomisp-lm3554.c b/drivers/staging/media/atomisp/i2c/atomisp-lm3554.c index 1e5f516f6e50..b369b101abe7 100644 --- a/drivers/staging/media/atomisp/i2c/atomisp-lm3554.c +++ b/drivers/staging/media/atomisp/i2c/atomisp-lm3554.c @@ -902,11 +902,12 @@ static int lm3554_probe(struct i2c_client *client) goto fail2; } return atomisp_register_i2c_module(&flash->sd, NULL, LED_FLASH); + fail2: - media_entity_cleanup(&flash->sd.entity); v4l2_ctrl_handler_free(&flash->ctrl_handler); fail1: v4l2_device_unregister_subdev(&flash->sd); + media_entity_cleanup(&flash->sd.entity); kfree(flash); return err; @@ -918,9 +919,9 @@ static int lm3554_remove(struct i2c_client *client) struct lm3554 *flash = to_lm3554(sd); int ret; - media_entity_cleanup(&flash->sd.entity); v4l2_ctrl_handler_free(&flash->ctrl_handler); v4l2_device_unregister_subdev(sd); + media_entity_cleanup(&sd->entity); atomisp_gmin_remove_subdev(sd); -- 2.17.0
[PATCH 0/3] media: staging: atomisp:
These 3 patches fixes (at least I hope) some issues found in or around 'lm3554_probe()'. Please review them carefully. I've only compile tested the changes and I propose them because they sound logical to me. The first one, return an error code instead of 0 if the call to an initialisation function fails. The 2nd one reorders own some label are reached in order to have a logical flow (first error goes to last label, last error goes to first label) The 3rd one fix the use 'media_entity_cleanup()'. If this one is correct, some other drivers will need to be fixed the same way. Christophe JAILLET (3): media: staging: atomisp: Return an error code in case of error in 'lm3554_probe()' media: staging: atomisp: Fix an error handling path in 'lm3554_probe()' media: staging: atomisp: Fix usage of 'media_entity_cleanup()' .../media/atomisp/i2c/atomisp-lm3554.c| 20 +-- 1 file changed, 9 insertions(+), 11 deletions(-) -- 2.17.0
[PATCH] media: i2c: tda1997: Fix an error handling path 'tda1997x_probe()'
If 'media_entity_pads_init()' fails, we must free the resources allocated
by 'v4l2_ctrl_handler_init()', as already done in the previous error
handling path.
'goto' the right label to fix it.
Fixes: 9ac0038db9a7 ("media: i2c: Add TDA1997x HDMI receiver driver")
Signed-off-by: Christophe JAILLET
---
drivers/media/i2c/tda1997x.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/media/i2c/tda1997x.c b/drivers/media/i2c/tda1997x.c
index 31bdab96f380..039a92c3294a 100644
--- a/drivers/media/i2c/tda1997x.c
+++ b/drivers/media/i2c/tda1997x.c
@@ -2723,7 +2723,7 @@ static int tda1997x_probe(struct i2c_client *client,
state->pads);
if (ret) {
v4l_err(client, "failed entity_init: %d", ret);
- goto err_free_mutex;
+ goto err_free_handler;
}
ret = v4l2_async_register_subdev(sd);
--
2.17.0
[PATCH] media: bt8xx: Fix err 'bt878_probe()'
This is odd to call 'pci_disable_device()' in an error path before a
coresponding successful 'pci_enable_device()'.
Return directly instead.
Fixes: 77e0be12100a ("V4L/DVB (4176): Bug-fix: Fix memory overflow")
Signed-off-by: Christophe JAILLET
---
drivers/media/pci/bt8xx/bt878.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/media/pci/bt8xx/bt878.c b/drivers/media/pci/bt8xx/bt878.c
index a5f52137d306..d4bc78b4fcb5 100644
--- a/drivers/media/pci/bt8xx/bt878.c
+++ b/drivers/media/pci/bt8xx/bt878.c
@@ -422,8 +422,7 @@ static int bt878_probe(struct pci_dev *dev, const struct
pci_device_id *pci_id)
bt878_num);
if (bt878_num >= BT878_MAX) {
printk(KERN_ERR "bt878: Too many devices inserted\n");
- result = -ENOMEM;
- goto fail0;
+ return -ENOMEM;
}
if (pci_enable_device(dev))
return -EIO;
--
2.11.0
[PATCH V2] media: v4l2-pci-skeleton: Fix error handling path in 'skeleton_probe()'
If this memory allocation fails, we must release some resources, as
already done in the code below and above.
Signed-off-by: Christophe JAILLET
---
v2: linux-media@vger.kernel.org added in cc
---
samples/v4l/v4l2-pci-skeleton.c | 6 --
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/samples/v4l/v4l2-pci-skeleton.c b/samples/v4l/v4l2-pci-skeleton.c
index 483e9bca9444..f520e3aef9c6 100644
--- a/samples/v4l/v4l2-pci-skeleton.c
+++ b/samples/v4l/v4l2-pci-skeleton.c
@@ -772,8 +772,10 @@ static int skeleton_probe(struct pci_dev *pdev, const
struct pci_device_id *ent)
/* Allocate a new instance */
skel = devm_kzalloc(&pdev->dev, sizeof(struct skeleton), GFP_KERNEL);
- if (!skel)
- return -ENOMEM;
+ if (!skel) {
+ ret = -ENOMEM;
+ goto disable_pci;
+ }
/* Allocate the interrupt */
ret = devm_request_irq(&pdev->dev, pdev->irq,
--
2.11.0
[PATCH] [media] smiapp: check memory allocation failure
Check memory allocation failure and return -ENOMEM in such a case.
Signed-off-by: Christophe JAILLET
---
drivers/media/i2c/smiapp/smiapp-core.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/media/i2c/smiapp/smiapp-core.c
b/drivers/media/i2c/smiapp/smiapp-core.c
index aff55e1dffe7..700f433261d0 100644
--- a/drivers/media/i2c/smiapp/smiapp-core.c
+++ b/drivers/media/i2c/smiapp/smiapp-core.c
@@ -841,6 +841,8 @@ static int smiapp_get_mbus_formats(struct smiapp_sensor
*sensor)
&client->dev,
compressed_max_bpp - sensor->compressed_min_bpp + 1,
sizeof(*sensor->valid_link_freqs), GFP_KERNEL);
+ if (!sensor->valid_link_freqs)
+ return -ENOMEM;
for (i = 0; i < ARRAY_SIZE(smiapp_csi_data_formats); i++) {
const struct smiapp_csi_data_format *f =
--
2.11.0
[PATCH] Staging: media: Release the correct resource in an error handling path
'res' is reassigned several times in the function and if we 'goto
error_unmap', its value is not the returned value of 'request_mem_region()'
anymore.
Introduce a new 'struct resource *' variable (i.e. res2) to keep a pointer
to the right resource, if needed in the error handling path.
Fixes: 4b4eda001704 ("Staging: media: Unmap and release region obtained by
ioremap_nocache")
Signed-off-by: Christophe JAILLET
---
drivers/staging/media/davinci_vpfe/dm365_ipipe.c | 8
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/staging/media/davinci_vpfe/dm365_ipipe.c
b/drivers/staging/media/davinci_vpfe/dm365_ipipe.c
index 6a3434cebd79..8d2d3f8edc07 100644
--- a/drivers/staging/media/davinci_vpfe/dm365_ipipe.c
+++ b/drivers/staging/media/davinci_vpfe/dm365_ipipe.c
@@ -1791,7 +1791,7 @@ vpfe_ipipe_init(struct vpfe_ipipe_device *ipipe, struct
platform_device *pdev)
struct v4l2_subdev *sd = &ipipe->subdev;
struct media_entity *me = &sd->entity;
static resource_size_t res_len;
- struct resource *res;
+ struct resource *res, *res2;
res = platform_get_resource(pdev, IORESOURCE_MEM, 4);
if (!res)
@@ -1805,10 +1805,10 @@ vpfe_ipipe_init(struct vpfe_ipipe_device *ipipe, struct
platform_device *pdev)
if (!ipipe->base_addr)
goto error_release;
- res = platform_get_resource(pdev, IORESOURCE_MEM, 6);
- if (!res)
+ res2 = platform_get_resource(pdev, IORESOURCE_MEM, 6);
+ if (!res2)
goto error_unmap;
- ipipe->isp5_base_addr = ioremap_nocache(res->start, res_len);
+ ipipe->isp5_base_addr = ioremap_nocache(res2->start, res_len);
if (!ipipe->isp5_base_addr)
goto error_unmap;
--
2.11.0
[PATCH v2] [media] vb2: Fix an off by one error in 'vb2_plane_vaddr'
We should ensure that 'plane_no' is '< vb->num_planes' as done in
'vb2_plane_cookie' just a few lines below.
Cc: sta...@vger.kernel.org
Fixes: e23ccc0ad925 ("[media] v4l: add videobuf2 Video for Linux 2 driver
framework")
Signed-off-by: Christophe JAILLET
---
v2: add CC and Fixes tags
---
drivers/media/v4l2-core/videobuf2-core.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/media/v4l2-core/videobuf2-core.c
b/drivers/media/v4l2-core/videobuf2-core.c
index 94afbbf92807..c0175ea7e7ad 100644
--- a/drivers/media/v4l2-core/videobuf2-core.c
+++ b/drivers/media/v4l2-core/videobuf2-core.c
@@ -868,7 +868,7 @@ EXPORT_SYMBOL_GPL(vb2_core_create_bufs);
void *vb2_plane_vaddr(struct vb2_buffer *vb, unsigned int plane_no)
{
- if (plane_no > vb->num_planes || !vb->planes[plane_no].mem_priv)
+ if (plane_no >= vb->num_planes || !vb->planes[plane_no].mem_priv)
return NULL;
return call_ptr_memop(vb, vaddr, vb->planes[plane_no].mem_priv);
--
2.11.0
Re: [PATCH 1/2] [media] vb2: Fix an off by one error in 'vb2_plane_vaddr'
Le 24/04/2017 à 22:29, Sakari Ailus a écrit :
Hi Christophe,
On Mon, Apr 24, 2017 at 10:00:24PM +0200, Christophe JAILLET wrote:
Le 24/04/2017 à 16:16, Sakari Ailus a écrit :
On Sun, Apr 23, 2017 at 11:32:57PM +0200, Christophe JAILLET wrote:
We should ensure that 'plane_no' is '< vb->num_planes' as done in
'vb2_plane_cookie' just a few lines below.
Signed-off-by: Christophe JAILLET
---
drivers/media/v4l2-core/videobuf2-core.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/media/v4l2-core/videobuf2-core.c
b/drivers/media/v4l2-core/videobuf2-core.c
index 94afbbf92807..c0175ea7e7ad 100644
--- a/drivers/media/v4l2-core/videobuf2-core.c
+++ b/drivers/media/v4l2-core/videobuf2-core.c
@@ -868,7 +868,7 @@ EXPORT_SYMBOL_GPL(vb2_core_create_bufs);
void *vb2_plane_vaddr(struct vb2_buffer *vb, unsigned int plane_no)
{
- if (plane_no > vb->num_planes || !vb->planes[plane_no].mem_priv)
+ if (plane_no >= vb->num_planes || !vb->planes[plane_no].mem_priv)
return NULL;
return call_ptr_memop(vb, vaddr, vb->planes[plane_no].mem_priv);
Oh my. How could this happen?
This should go to stable as well.
Should I resubmit with "Cc: sta...@vger.kernel.org" or will you add it
yourself?
Please resend. And preferrably figure out which version is the first one
requiring the fix.
Mauro can then pick it up, and it ends up to stable through his tree. I.e.
Cc: stable ... tag is enough, no need to send an actual e-mail there.
Thanks!
Hmm, funny to see:
https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/drivers/media/v4l2-core/videobuf2-core.c?id=a9ae4692eda4b99f85757b15d60971ff78a0a0e2
Anyway,
3.2.88:
still have the issue for both 'vb2_plane_vaddr' and
'vb2_plane_cookie', but the file is in a slightly different
directory*and the code is also slightly different*
3.4.113:
still have the issue for both 'vb2_plane_vaddr' and
'vb2_plane_cookie', but the file is in a slightly different directory
3.10.105, *3.12.73*:
still have the issue for both 'vb2_plane_vaddr' and 'vb2_plane_cookie'
3.16.43 and up:
'vb2_plane_cookie' is fixed there.
So, I guess, that the same +3.16 should be proposed here, to be
consistent. Ok for you?
Should a:
Fixes: e23ccc0ad9258 ("[media] v4l: add videobuf2 Video for Linux 2
driver framework")
be also added? I've read somewhere that Fixes tags were needed for
backport to stable.
CJ
Re: [PATCH 2/2] [media] vb2: Fix error handling in '__vb2_buf_mem_alloc'
Le 24/04/2017 à 16:23, Sakari Ailus a écrit :
Hi Christophe,
On Sun, Apr 23, 2017 at 11:40:30PM +0200, Christophe JAILLET wrote:
'call_ptr_memop' can return NULL, so we must test its return value with
'IS_ERR_OR_NULL'. Otherwise, the test 'if (mem_priv)' is meaningless.
Signed-off-by: Christophe JAILLET
---
Note that error checking after 'call_ptr_memop' calls is not consistent
in this file. I guess that 'IS_ERR_OR_NULL' should be used everywhere
and that the corresponding error handling code should be tweaked just as
the code in this function.
---
drivers/media/v4l2-core/videobuf2-core.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/media/v4l2-core/videobuf2-core.c
b/drivers/media/v4l2-core/videobuf2-core.c
index c0175ea7e7ad..d1d3f5dd57b9 100644
--- a/drivers/media/v4l2-core/videobuf2-core.c
+++ b/drivers/media/v4l2-core/videobuf2-core.c
@@ -210,7 +210,7 @@ static int __vb2_buf_mem_alloc(struct vb2_buffer *vb)
mem_priv = call_ptr_memop(vb, alloc,
q->alloc_devs[plane] ? : q->dev,
q->dma_attrs, size, dma_dir, q->gfp_flags);
- if (IS_ERR(mem_priv)) {
+ if (IS_ERR_OR_NULL(mem_priv)) {
if (mem_priv)
ret = PTR_ERR(mem_priv);
goto free;
If NULL will always equate -ENOMEM, shouldn't call_ptr_memop() be changed
instead to convert NULL to ERR_PTR(-ENOMEM)?
I agree with you, but in fact, I don't know if "NULL will always equate
-ENOMEM"
The return value of 'call_ptr_memop' is likely the result of a function
called via a function pointer. I don't know if this function can return
NULL or not.
I don't know the code enough to see if it would be safe and if this
assertion is correct.
So the easiest for me is to just propose a fix to accept NULL.
*** Digging deeper ***
In 'videobuf2-core.h', there is:
/**
* struct vb2_mem_ops - memory handling/memory allocator operations
* @alloc: allocate video memory and, optionally, allocator
private data,
* return ERR_PTR() on failure or a pointer to allocator
private,
* per-buffer data on success; the returned private structure
* will then be passed as @buf_priv argument to other ops
in this
* structure. Additional gfp_flags to use when allocating the
* are also passed to this operation. These flags are from the
* gfp_flags field of vb2_queue.
So the 'alloc' function should return an ERR_PTR in case of error.
'vb2_vmalloc_alloc', 'vb2_dc_alloc' and 'vb2_dma_sg_alloc' does.
(confirmed by code inspection, based on the 'standard' list of alloc
functions found in see https://lwn.net/Articles/447435/)
But what if a module implements its own set of functions and returns
NULL in such a case?
Anyway, I will propose a patch that returns ERR_PTR(-ENOMEM) instead of
NULL, but I won't be able to test it on my own.
Thanks,
CJ
Re: [PATCH 1/2] [media] vb2: Fix an off by one error in 'vb2_plane_vaddr'
Le 24/04/2017 à 16:16, Sakari Ailus a écrit :
On Sun, Apr 23, 2017 at 11:32:57PM +0200, Christophe JAILLET wrote:
We should ensure that 'plane_no' is '< vb->num_planes' as done in
'vb2_plane_cookie' just a few lines below.
Signed-off-by: Christophe JAILLET
---
drivers/media/v4l2-core/videobuf2-core.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/media/v4l2-core/videobuf2-core.c
b/drivers/media/v4l2-core/videobuf2-core.c
index 94afbbf92807..c0175ea7e7ad 100644
--- a/drivers/media/v4l2-core/videobuf2-core.c
+++ b/drivers/media/v4l2-core/videobuf2-core.c
@@ -868,7 +868,7 @@ EXPORT_SYMBOL_GPL(vb2_core_create_bufs);
void *vb2_plane_vaddr(struct vb2_buffer *vb, unsigned int plane_no)
{
- if (plane_no > vb->num_planes || !vb->planes[plane_no].mem_priv)
+ if (plane_no >= vb->num_planes || !vb->planes[plane_no].mem_priv)
return NULL;
return call_ptr_memop(vb, vaddr, vb->planes[plane_no].mem_priv);
Oh my. How could this happen?
This should go to stable as well.
Should I resubmit with "Cc: sta...@vger.kernel.org" or will you add it
yourself?
CJ
Reviewed-by: Sakari Ailus
[PATCH 1/2] [media] vb2: Fix an off by one error in 'vb2_plane_vaddr'
We should ensure that 'plane_no' is '< vb->num_planes' as done in
'vb2_plane_cookie' just a few lines below.
Signed-off-by: Christophe JAILLET
---
drivers/media/v4l2-core/videobuf2-core.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/media/v4l2-core/videobuf2-core.c
b/drivers/media/v4l2-core/videobuf2-core.c
index 94afbbf92807..c0175ea7e7ad 100644
--- a/drivers/media/v4l2-core/videobuf2-core.c
+++ b/drivers/media/v4l2-core/videobuf2-core.c
@@ -868,7 +868,7 @@ EXPORT_SYMBOL_GPL(vb2_core_create_bufs);
void *vb2_plane_vaddr(struct vb2_buffer *vb, unsigned int plane_no)
{
- if (plane_no > vb->num_planes || !vb->planes[plane_no].mem_priv)
+ if (plane_no >= vb->num_planes || !vb->planes[plane_no].mem_priv)
return NULL;
return call_ptr_memop(vb, vaddr, vb->planes[plane_no].mem_priv);
--
2.11.0
[PATCH 2/2] [media] vb2: Fix error handling in '__vb2_buf_mem_alloc'
'call_ptr_memop' can return NULL, so we must test its return value with
'IS_ERR_OR_NULL'. Otherwise, the test 'if (mem_priv)' is meaningless.
Signed-off-by: Christophe JAILLET
---
Note that error checking after 'call_ptr_memop' calls is not consistent
in this file. I guess that 'IS_ERR_OR_NULL' should be used everywhere
and that the corresponding error handling code should be tweaked just as
the code in this function.
---
drivers/media/v4l2-core/videobuf2-core.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/media/v4l2-core/videobuf2-core.c
b/drivers/media/v4l2-core/videobuf2-core.c
index c0175ea7e7ad..d1d3f5dd57b9 100644
--- a/drivers/media/v4l2-core/videobuf2-core.c
+++ b/drivers/media/v4l2-core/videobuf2-core.c
@@ -210,7 +210,7 @@ static int __vb2_buf_mem_alloc(struct vb2_buffer *vb)
mem_priv = call_ptr_memop(vb, alloc,
q->alloc_devs[plane] ? : q->dev,
q->dma_attrs, size, dma_dir, q->gfp_flags);
- if (IS_ERR(mem_priv)) {
+ if (IS_ERR_OR_NULL(mem_priv)) {
if (mem_priv)
ret = PTR_ERR(mem_priv);
goto free;
--
2.11.0
Re: [PATCH] [media] exynos4-is: Add missing 'of_node_put'
Le 24/02/2017 à 22:19, Javier Martinez Canillas a écrit :
Thanks for the patch, but Krzysztof sent the exact same patch before
[0]. There
was feedback from Sylwester at the time that you can also look at [0]. Could you
please take that into account and post a patch according to what he suggested?
[0]:
http://lists.infradead.org/pipermail/linux-arm-kernel/2016-March/415207.html
Best regards,
Hi,
apparently, the patch has already been taken into -next by Mauro
Carvalho Chehab.
Moreover, I personally don't think that what is proposed in [0] is more
readable.
There is not that much code between 'of_get_next_child' and
'of_node_put' in the normal path and not so many error handling paths
between the 2 function calls.
Adding some indentation level is not an improvement, IMHO.
If you really want, I could propose something like the code below (not
carefully checked, just to give an idea), but honestly, I don't think it
is needed.
It avoids some new indent and avoids duplicating 'of_node_put'.
Tell me if you think it is an improvement and I'll send a patch.
CJ
diff --git a/drivers/media/platform/exynos4-is/media-dev.c
b/drivers/media/platform/exynos4-is/media-dev.c
index e3a8709138fa..da5b76c1df98 100644
--- a/drivers/media/platform/exynos4-is/media-dev.c
+++ b/drivers/media/platform/exynos4-is/media-dev.c
@@ -385,38 +385,35 @@ static void fimc_md_pipelines_free(struct fimc_md
*fmd)
static int fimc_md_parse_port_node(struct fimc_md *fmd,
struct device_node *port,
unsigned int index)
{
struct fimc_source_info *pd = &fmd->sensor[index].pdata;
-struct device_node *rem, *ep, *np;
+struct device_node *rem = NULL, *ep = NULL, *np;
struct v4l2_of_endpoint endpoint;
int ret;
/* Assume here a port node can have only one endpoint node. */
ep = of_get_next_child(port, NULL);
if (!ep)
return 0;
ret = v4l2_of_parse_endpoint(ep, &endpoint);
-if (ret) {
-of_node_put(ep);
-return ret;
-}
+if (ret)
+goto put_ep;
if (WARN_ON(endpoint.base.port == 0) || index >= FIMC_MAX_SENSORS) {
-of_node_put(ep);
-return -EINVAL;
+ret = -EINVAL;
+goto put_ep;
}
pd->mux_id = (endpoint.base.port - 1) & 0x1;
rem = of_graph_get_remote_port_parent(ep);
-of_node_put(ep);
if (rem == NULL) {
v4l2_info(&fmd->v4l2_dev, "Remote device at %s not found\n",
ep->full_name);
-return 0;
+goto out;
}
if (fimc_input_is_parallel(endpoint.base.port)) {
if (endpoint.bus_type == V4L2_MBUS_PARALLEL)
pd->sensor_bus_type = FIMC_BUS_TYPE_ITU_601;
@@ -447,22 +444,28 @@ static int fimc_md_parse_port_node(struct fimc_md
*fmd,
pd->fimc_bus_type = FIMC_BUS_TYPE_ISP_WRITEBACK;
else
pd->fimc_bus_type = pd->sensor_bus_type;
if (WARN_ON(index >= ARRAY_SIZE(fmd->sensor))) {
-of_node_put(rem);
-return -EINVAL;
+ret = -EINVAL;
+goto put_rem;
}
fmd->sensor[index].asd.match_type = V4L2_ASYNC_MATCH_OF;
fmd->sensor[index].asd.match.of.node = rem;
fmd->async_subdevs[index] = &fmd->sensor[index].asd;
fmd->num_sensors++;
+out:
+ret = 0;
+put_rem:
of_node_put(rem);
-return 0;
+
+put_ep:
+of_node_put(ep);
+return ret;
}
/* Register all SoC external sub-devices */
static int fimc_md_register_sensor_entities(struct fimc_md *fmd)
{
[PATCH v2] staging: bcm2835-camera: Fix a memory leak in error handling path in 'bm2835_mmal_init()'
If 'kzalloc()' fails, we should release resources allocated so far, just as
done in all other cases in this function.
Signed-off-by: Christophe JAILLET
---
Not sure that the error handling path is correct.
Is 'gdev[0]' freed? Should it be?
v2: Rename patch to include '-camera' in the subject
---
drivers/staging/media/platform/bcm2835/bcm2835-camera.c | 6 --
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/staging/media/platform/bcm2835/bcm2835-camera.c
b/drivers/staging/media/platform/bcm2835/bcm2835-camera.c
index ca15a698e018..9651b9bc3439 100644
--- a/drivers/staging/media/platform/bcm2835/bcm2835-camera.c
+++ b/drivers/staging/media/platform/bcm2835/bcm2835-camera.c
@@ -1914,8 +1914,10 @@ static int __init bm2835_mmal_init(void)
for (camera = 0; camera < num_cameras; camera++) {
dev = kzalloc(sizeof(struct bm2835_mmal_dev), GFP_KERNEL);
- if (!dev)
- return -ENOMEM;
+ if (!dev) {
+ ret = -ENOMEM;
+ goto free_dev;
+ }
dev->camera_num = camera;
dev->max_width = resolutions[camera][0];
--
2.9.3
[PATCH] [media] tm6000: Fix resource freeing in 'tm6000_prepare_isoc()'
'usb_free_urb(urb)' is a no-op, because urb is known to be NULL.
It is likelly that releasing resources allocated by
'tm6000_alloc_urb_buffers()' just a few lines above is expected here.
This has been spotted by the following coccinelle script:
@@
expression ret, x, e;
identifier f;
@@
* if (x == NULL)
{
... when != x = e;
(
*f(<+...x...+>);
|
*ret = f(<+...x...+>);
)
...
}
Signed-off-by: Christophe JAILLET
---
drivers/media/usb/tm6000/tm6000-video.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/media/usb/tm6000/tm6000-video.c
b/drivers/media/usb/tm6000/tm6000-video.c
index c4fdc1fa32ef..7e960d0a5b92 100644
--- a/drivers/media/usb/tm6000/tm6000-video.c
+++ b/drivers/media/usb/tm6000/tm6000-video.c
@@ -631,7 +631,7 @@ static int tm6000_prepare_isoc(struct tm6000_core *dev)
urb = usb_alloc_urb(max_packets, GFP_KERNEL);
if (!urb) {
tm6000_uninit_isoc(dev);
- usb_free_urb(urb);
+ tm6000_free_urb_buffers(dev);
return -ENOMEM;
}
dev->isoc_ctl.urb[i] = urb;
--
2.9.3
[PATCH] [media] s5p-g2d: Fix error handling
According to the surrounding goto, it is likely that 'unprep_clk_gate' was
expected here.
Signed-off-by: Christophe JAILLET
---
drivers/media/platform/s5p-g2d/g2d.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/media/platform/s5p-g2d/g2d.c
b/drivers/media/platform/s5p-g2d/g2d.c
index 62c0dec30b59..81ed5cd5cd5d 100644
--- a/drivers/media/platform/s5p-g2d/g2d.c
+++ b/drivers/media/platform/s5p-g2d/g2d.c
@@ -679,7 +679,7 @@ static int g2d_probe(struct platform_device *pdev)
0, pdev->name, dev);
if (ret) {
dev_err(&pdev->dev, "failed to install IRQ\n");
- goto put_clk_gate;
+ goto unprep_clk_gate;
}
vb2_dma_contig_set_max_seg_size(&pdev->dev, DMA_BIT_MASK(32));
--
2.9.3
[PATCH] staging: bcm2835: Fix a memory leak in error handling path
If 'kzalloc()' fails, we should release resources allocated so far, just as
done in all other cases in this function.
Signed-off-by: Christophe JAILLET
---
Not sure that the error handling path is correct.
Is 'gdev[0]' freed? Should it be?
---
drivers/staging/media/platform/bcm2835/bcm2835-camera.c | 6 --
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/staging/media/platform/bcm2835/bcm2835-camera.c
b/drivers/staging/media/platform/bcm2835/bcm2835-camera.c
index ca15a698e018..9651b9bc3439 100644
--- a/drivers/staging/media/platform/bcm2835/bcm2835-camera.c
+++ b/drivers/staging/media/platform/bcm2835/bcm2835-camera.c
@@ -1914,8 +1914,10 @@ static int __init bm2835_mmal_init(void)
for (camera = 0; camera < num_cameras; camera++) {
dev = kzalloc(sizeof(struct bm2835_mmal_dev), GFP_KERNEL);
- if (!dev)
- return -ENOMEM;
+ if (!dev) {
+ ret = -ENOMEM;
+ goto free_dev;
+ }
dev->camera_num = camera;
dev->max_width = resolutions[camera][0];
--
2.9.3
[PATCH] [media] exynos4-is: Add missing 'of_node_put'
It is likely that a "of_node_put(ep)" is missing here.
There is one in the previous error handling code, and one a few lines
below in the normal case as well.
Signed-off-by: Christophe JAILLET
---
drivers/media/platform/exynos4-is/media-dev.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/media/platform/exynos4-is/media-dev.c
b/drivers/media/platform/exynos4-is/media-dev.c
index e3a8709138fa..da5b76c1df98 100644
--- a/drivers/media/platform/exynos4-is/media-dev.c
+++ b/drivers/media/platform/exynos4-is/media-dev.c
@@ -402,8 +402,10 @@ static int fimc_md_parse_port_node(struct fimc_md *fmd,
return ret;
}
- if (WARN_ON(endpoint.base.port == 0) || index >= FIMC_MAX_SENSORS)
+ if (WARN_ON(endpoint.base.port == 0) || index >= FIMC_MAX_SENSORS) {
+ of_node_put(ep);
return -EINVAL;
+ }
pd->mux_id = (endpoint.base.port - 1) & 0x1;
--
2.9.3
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] [media] soc-camera: Fix a return value in case of error
If 'ov9640_reg_read()' does not return 0, then 'val' is left unmodified.
As it is not initialized either, the return value can be anything.
It is likely that returning the error code was expected here.
Signed-off-by: Christophe JAILLET
---
drivers/media/i2c/soc_camera/ov9640.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/media/i2c/soc_camera/ov9640.c
b/drivers/media/i2c/soc_camera/ov9640.c
index 8c93c57af71c..65085a235128 100644
--- a/drivers/media/i2c/soc_camera/ov9640.c
+++ b/drivers/media/i2c/soc_camera/ov9640.c
@@ -233,7 +233,7 @@ static int ov9640_reg_rmw(struct i2c_client *client, u8
reg, u8 set, u8 unset)
if (ret) {
dev_err(&client->dev,
"[Read]-Modify-Write of register %02x failed!\n", reg);
- return val;
+ return ret;
}
val |= set;
--
2.9.3
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] [media] VPU: mediatek: Fix return value in case of error
If 'dma_alloc_coherent()' returns NULL, 'vpu_alloc_ext_mem()' will
return 0 which means success.
Return -ENOMEM instead.
Signed-off-by: Christophe JAILLET
---
drivers/media/platform/mtk-vpu/mtk_vpu.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/media/platform/mtk-vpu/mtk_vpu.c
b/drivers/media/platform/mtk-vpu/mtk_vpu.c
index c9bf58c97878..3edb5ed852e6 100644
--- a/drivers/media/platform/mtk-vpu/mtk_vpu.c
+++ b/drivers/media/platform/mtk-vpu/mtk_vpu.c
@@ -674,7 +674,7 @@ static int vpu_alloc_ext_mem(struct mtk_vpu *vpu, u32
fw_type)
GFP_KERNEL);
if (!vpu->extmem[fw_type].va) {
dev_err(dev, "Failed to allocate the extended program
memory\n");
- return PTR_ERR(vpu->extmem[fw_type].va);
+ return -ENOMEM;
}
/* Disable extend0. Enable extend1 */
--
2.7.4
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] [media] s5p_cec: Fix memory allocation failure check
It is likely that checking the result of the memory allocation just above is expected here. Signed-off-by: Christophe JAILLET --- drivers/staging/media/s5p-cec/s5p_cec.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/staging/media/s5p-cec/s5p_cec.c b/drivers/staging/media/s5p-cec/s5p_cec.c index 78333273c4e5..636bac182e8e 100644 --- a/drivers/staging/media/s5p-cec/s5p_cec.c +++ b/drivers/staging/media/s5p-cec/s5p_cec.c @@ -173,7 +173,7 @@ static int s5p_cec_probe(struct platform_device *pdev) int ret; cec = devm_kzalloc(&pdev->dev, sizeof(*cec), GFP_KERNEL); - if (!dev) + if (!cec) return -ENOMEM; cec->dev = dev; -- 2.7.4 -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] [media] drxd_hard: Add missing error code assignment before test
It is likely that checking the result of the 2nd 'read16' is expected here. Signed-off-by: Christophe JAILLET --- drivers/media/dvb-frontends/drxk_hard.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/media/dvb-frontends/drxk_hard.c b/drivers/media/dvb-frontends/drxk_hard.c index b975da099929..c595adc61c6f 100644 --- a/drivers/media/dvb-frontends/drxk_hard.c +++ b/drivers/media/dvb-frontends/drxk_hard.c @@ -6448,7 +6448,7 @@ static int get_strength(struct drxk_state *state, u64 *strength) return status; /* SCU c.o.c. */ - read16(state, SCU_RAM_AGC_RF_IACCU_HI_CO__A, &scu_coc); + status = read16(state, SCU_RAM_AGC_RF_IACCU_HI_CO__A, &scu_coc); if (status < 0) return status; -- 2.7.4 --- L'absence de virus dans ce courrier électronique a été vérifiée par le logiciel antivirus Avast. https://www.avast.com/antivirus -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
