* [PATCH RESEND 1/5] staging: media: atomisp: Fix error path in lm3554_probe()
2020-09-21 21:53 [PATCH RESEND 0/5] atomisp: Fixes and cleanups Alex Dewar
@ 2020-09-21 21:53 ` Alex Dewar
2020-09-21 21:53 ` [PATCH RESEND 2/5] staging: media: atomisp: Remove unhelpful info message Alex Dewar
` (4 subsequent siblings)
5 siblings, 0 replies; 16+ messages in thread
From: Alex Dewar @ 2020-09-21 21:53 UTC (permalink / raw)
To: Mauro Carvalho Chehab, Sakari Ailus, Greg Kroah-Hartman,
Alex Dewar, Linus Walleij, Dan Carpenter, Alan Cox, linux-media,
devel, linux-kernel
The error path for lm3554_probe() contains a number of bugs, including:
* resource leaks
* jumping to error labels out of sequence
* not setting the return value appropriately
Fix it up and give the labels more memorable names.
This issue has existed since the code was originally contributed in
commit a49d25364dfb ("staging/atomisp: Add support for the Intel IPU v2"),
although the code was subsequently removed altogether and then
reinstated with commit ad85094b293e ("Revert "media: staging: atomisp: Remove driver"").
Addresses-Coverity: 1496802 ("Resource leaks")
Fixes: a49d25364dfb ("staging/atomisp: Add support for the Intel IPU v2")
Signed-off-by: Alex Dewar <alex.dewar90@gmail.com>
Reviewed-by: Dan Carpenter <dan.carpenter@oracle.com>
---
.../media/atomisp/i2c/atomisp-lm3554.c | 53 +++++++++++--------
1 file changed, 30 insertions(+), 23 deletions(-)
diff --git a/drivers/staging/media/atomisp/i2c/atomisp-lm3554.c b/drivers/staging/media/atomisp/i2c/atomisp-lm3554.c
index 7ca7378b1859..cca10a4c2db0 100644
--- a/drivers/staging/media/atomisp/i2c/atomisp-lm3554.c
+++ b/drivers/staging/media/atomisp/i2c/atomisp-lm3554.c
@@ -833,7 +833,6 @@ static void *lm3554_platform_data_func(struct i2c_client *client)
static int lm3554_probe(struct i2c_client *client)
{
- int err = 0;
struct lm3554 *flash;
unsigned int i;
int ret;
@@ -843,36 +842,38 @@ static int lm3554_probe(struct i2c_client *client)
return -ENOMEM;
flash->pdata = lm3554_platform_data_func(client);
- if (IS_ERR(flash->pdata))
- return PTR_ERR(flash->pdata);
+ if (IS_ERR(flash->pdata)) {
+ ret = PTR_ERR(flash->pdata);
+ goto err_free_flash;
+ }
v4l2_i2c_subdev_init(&flash->sd, client, &lm3554_ops);
flash->sd.internal_ops = &lm3554_internal_ops;
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));
+ ret = v4l2_ctrl_handler_init(&flash->ctrl_handler,
+ ARRAY_SIZE(lm3554_controls));
if (ret) {
- dev_err(&client->dev, "error initialize a ctrl_handler.\n");
- goto fail2;
+ dev_err(&client->dev, "error initializing ctrl_handler");
+ goto err_unregister_sd;
}
for (i = 0; i < ARRAY_SIZE(lm3554_controls); i++)
v4l2_ctrl_new_custom(&flash->ctrl_handler, &lm3554_controls[i],
NULL);
- if (flash->ctrl_handler.error) {
- dev_err(&client->dev, "ctrl_handler error.\n");
- goto fail2;
+ ret = flash->ctrl_handler.error;
+ if (ret) {
+ dev_err(&client->dev, "ctrl_handler error");
+ goto err_free_ctrl_handler;
}
flash->sd.ctrl_handler = &flash->ctrl_handler;
- err = media_entity_pads_init(&flash->sd.entity, 0, NULL);
- if (err) {
- dev_err(&client->dev, "error initialize a media entity.\n");
- goto fail1;
+ ret = media_entity_pads_init(&flash->sd.entity, 0, NULL);
+ if (ret) {
+ dev_err(&client->dev, "error initializing media entity");
+ goto err_free_ctrl_handler;
}
flash->sd.entity.function = MEDIA_ENT_F_FLASH;
@@ -881,20 +882,26 @@ static int lm3554_probe(struct i2c_client *client)
timer_setup(&flash->flash_off_delay, lm3554_flash_off_delay, 0);
- err = lm3554_gpio_init(client);
- if (err) {
+ ret = lm3554_gpio_init(client);
+ if (ret) {
dev_err(&client->dev, "gpio request/direction_output fail");
- goto fail2;
+ goto err_del_timer;
}
- return atomisp_register_i2c_module(&flash->sd, NULL, LED_FLASH);
-fail2:
+
+ ret = atomisp_register_i2c_module(&flash->sd, NULL, LED_FLASH);
+ if (!ret)
+ return 0;
+
+err_del_timer:
+ del_timer_sync(&flash->flash_off_delay);
media_entity_cleanup(&flash->sd.entity);
+err_free_ctrl_handler:
v4l2_ctrl_handler_free(&flash->ctrl_handler);
-fail1:
+err_unregister_sd:
v4l2_device_unregister_subdev(&flash->sd);
+err_free_flash:
kfree(flash);
-
- return err;
+ return ret;
}
static int lm3554_remove(struct i2c_client *client)
--
2.28.0
^ permalink raw reply related [flat|nested] 16+ messages in thread* [PATCH RESEND 2/5] staging: media: atomisp: Remove unhelpful info message
2020-09-21 21:53 [PATCH RESEND 0/5] atomisp: Fixes and cleanups Alex Dewar
2020-09-21 21:53 ` [PATCH RESEND 1/5] staging: media: atomisp: Fix error path in lm3554_probe() Alex Dewar
@ 2020-09-21 21:53 ` Alex Dewar
2020-09-21 21:53 ` [PATCH RESEND 3/5] staging: media: atomisp: Don't do unnecessary zeroing of memory Alex Dewar
` (3 subsequent siblings)
5 siblings, 0 replies; 16+ messages in thread
From: Alex Dewar @ 2020-09-21 21:53 UTC (permalink / raw)
To: Mauro Carvalho Chehab, Sakari Ailus, Greg Kroah-Hartman,
Alex Dewar, Dan Carpenter, Linus Walleij, Alan Cox, linux-media,
devel, linux-kernel
We don't really need to know that the LED pin reset successfully.
Signed-off-by: Alex Dewar <alex.dewar90@gmail.com>
---
drivers/staging/media/atomisp/i2c/atomisp-lm3554.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/drivers/staging/media/atomisp/i2c/atomisp-lm3554.c b/drivers/staging/media/atomisp/i2c/atomisp-lm3554.c
index cca10a4c2db0..d78014847e67 100644
--- a/drivers/staging/media/atomisp/i2c/atomisp-lm3554.c
+++ b/drivers/staging/media/atomisp/i2c/atomisp-lm3554.c
@@ -771,7 +771,6 @@ static int lm3554_gpio_init(struct i2c_client *client)
ret = gpiod_direction_output(pdata->gpio_reset, 0);
if (ret < 0)
return ret;
- dev_info(&client->dev, "flash led reset successfully\n");
if (!pdata->gpio_strobe)
return -EINVAL;
--
2.28.0
^ permalink raw reply related [flat|nested] 16+ messages in thread* [PATCH RESEND 3/5] staging: media: atomisp: Don't do unnecessary zeroing of memory
2020-09-21 21:53 [PATCH RESEND 0/5] atomisp: Fixes and cleanups Alex Dewar
2020-09-21 21:53 ` [PATCH RESEND 1/5] staging: media: atomisp: Fix error path in lm3554_probe() Alex Dewar
2020-09-21 21:53 ` [PATCH RESEND 2/5] staging: media: atomisp: Remove unhelpful info message Alex Dewar
@ 2020-09-21 21:53 ` Alex Dewar
2020-09-21 21:53 ` [PATCH RESEND 4/5] staging: media: atomisp: Don't abort on error in module exit path Alex Dewar
` (2 subsequent siblings)
5 siblings, 0 replies; 16+ messages in thread
From: Alex Dewar @ 2020-09-21 21:53 UTC (permalink / raw)
To: Mauro Carvalho Chehab, Sakari Ailus, Greg Kroah-Hartman,
Alex Dewar, Dan Carpenter, Linus Walleij, Alan Cox, linux-media,
devel, linux-kernel
In a few places in pci/sh_css_params.c, memset is used to zero memory
immediately before it is freed. As none of these structs appear to
contain sensitive information, just remove the calls to memset.
Suggested-by: Dan Carpenter <dan.carpenter@oracle.com>
Signed-off-by: Alex Dewar <alex.dewar90@gmail.com>
---
drivers/staging/media/atomisp/pci/sh_css_params.c | 6 ------
1 file changed, 6 deletions(-)
diff --git a/drivers/staging/media/atomisp/pci/sh_css_params.c b/drivers/staging/media/atomisp/pci/sh_css_params.c
index 2c67c23b3700..24fc497bd491 100644
--- a/drivers/staging/media/atomisp/pci/sh_css_params.c
+++ b/drivers/staging/media/atomisp/pci/sh_css_params.c
@@ -4378,7 +4378,6 @@ ia_css_3a_statistics_free(struct ia_css_3a_statistics *me)
if (me) {
kvfree(me->rgby_data);
kvfree(me->data);
- memset(me, 0, sizeof(struct ia_css_3a_statistics));
kvfree(me);
}
}
@@ -4417,7 +4416,6 @@ ia_css_dvs_statistics_free(struct ia_css_dvs_statistics *me)
if (me) {
kvfree(me->hor_proj);
kvfree(me->ver_proj);
- memset(me, 0, sizeof(struct ia_css_dvs_statistics));
kvfree(me);
}
}
@@ -4459,7 +4457,6 @@ ia_css_dvs_coefficients_free(struct ia_css_dvs_coefficients *me)
if (me) {
kvfree(me->hor_coefs);
kvfree(me->ver_coefs);
- memset(me, 0, sizeof(struct ia_css_dvs_coefficients));
kvfree(me);
}
}
@@ -4551,7 +4548,6 @@ ia_css_dvs2_statistics_free(struct ia_css_dvs2_statistics *me)
kvfree(me->ver_prod.odd_imag);
kvfree(me->ver_prod.even_real);
kvfree(me->ver_prod.even_imag);
- memset(me, 0, sizeof(struct ia_css_dvs2_statistics));
kvfree(me);
}
}
@@ -4635,7 +4631,6 @@ ia_css_dvs2_coefficients_free(struct ia_css_dvs2_coefficients *me)
kvfree(me->ver_coefs.odd_imag);
kvfree(me->ver_coefs.even_real);
kvfree(me->ver_coefs.even_imag);
- memset(me, 0, sizeof(struct ia_css_dvs2_coefficients));
kvfree(me);
}
}
@@ -4710,7 +4705,6 @@ ia_css_dvs2_6axis_config_free(struct ia_css_dvs_6axis_config *dvs_6axis_config)
kvfree(dvs_6axis_config->ycoords_y);
kvfree(dvs_6axis_config->xcoords_uv);
kvfree(dvs_6axis_config->ycoords_uv);
- memset(dvs_6axis_config, 0, sizeof(struct ia_css_dvs_6axis_config));
kvfree(dvs_6axis_config);
}
}
--
2.28.0
^ permalink raw reply related [flat|nested] 16+ messages in thread* [PATCH RESEND 4/5] staging: media: atomisp: Don't abort on error in module exit path
2020-09-21 21:53 [PATCH RESEND 0/5] atomisp: Fixes and cleanups Alex Dewar
` (2 preceding siblings ...)
2020-09-21 21:53 ` [PATCH RESEND 3/5] staging: media: atomisp: Don't do unnecessary zeroing of memory Alex Dewar
@ 2020-09-21 21:53 ` Alex Dewar
2020-09-21 21:53 ` [PATCH RESEND 5/5] staging: media: atomisp: Fix bool-related style issues Alex Dewar
2020-09-22 8:11 ` [PATCH RESEND 0/5] atomisp: Fixes and cleanups Mauro Carvalho Chehab
5 siblings, 0 replies; 16+ messages in thread
From: Alex Dewar @ 2020-09-21 21:53 UTC (permalink / raw)
To: Mauro Carvalho Chehab, Sakari Ailus, Greg Kroah-Hartman,
Alex Dewar, Linus Walleij, Dan Carpenter, Alan Cox, linux-media,
devel, linux-kernel
The function lm3554_remove() checks for the return code for
lm3554_gpio_uninit() even though this is on the exit path and exits the
function, leaving the variable flash unfreed. Instead, print a warning and
free flash unconditionally.
Signed-off-by: Alex Dewar <alex.dewar90@gmail.com>
---
.../staging/media/atomisp/i2c/atomisp-lm3554.c | 18 ++++++------------
1 file changed, 6 insertions(+), 12 deletions(-)
diff --git a/drivers/staging/media/atomisp/i2c/atomisp-lm3554.c b/drivers/staging/media/atomisp/i2c/atomisp-lm3554.c
index d78014847e67..d446ee8e93db 100644
--- a/drivers/staging/media/atomisp/i2c/atomisp-lm3554.c
+++ b/drivers/staging/media/atomisp/i2c/atomisp-lm3554.c
@@ -782,7 +782,7 @@ static int lm3554_gpio_init(struct i2c_client *client)
return 0;
}
-static int lm3554_gpio_uninit(struct i2c_client *client)
+static void lm3554_gpio_uninit(struct i2c_client *client)
{
struct v4l2_subdev *sd = i2c_get_clientdata(client);
struct lm3554 *flash = to_lm3554(sd);
@@ -791,13 +791,13 @@ static int lm3554_gpio_uninit(struct i2c_client *client)
ret = gpiod_direction_output(pdata->gpio_strobe, 0);
if (ret < 0)
- return ret;
+ dev_err(&client->dev,
+ "gpio request/direction_output fail for gpio_strobe");
ret = gpiod_direction_output(pdata->gpio_reset, 0);
if (ret < 0)
- return ret;
-
- return 0;
+ dev_err(&client->dev,
+ "gpio request/direction_output fail for gpio_reset");
}
static void *lm3554_platform_data_func(struct i2c_client *client)
@@ -907,7 +907,6 @@ static int lm3554_remove(struct i2c_client *client)
{
struct v4l2_subdev *sd = i2c_get_clientdata(client);
struct lm3554 *flash = to_lm3554(sd);
- int ret;
media_entity_cleanup(&flash->sd.entity);
v4l2_ctrl_handler_free(&flash->ctrl_handler);
@@ -917,16 +916,11 @@ static int lm3554_remove(struct i2c_client *client)
del_timer_sync(&flash->flash_off_delay);
- ret = lm3554_gpio_uninit(client);
- if (ret < 0)
- goto fail;
+ lm3554_gpio_uninit(client);
kfree(flash);
return 0;
-fail:
- dev_err(&client->dev, "gpio request/direction_output fail");
- return ret;
}
static const struct dev_pm_ops lm3554_pm_ops = {
--
2.28.0
^ permalink raw reply related [flat|nested] 16+ messages in thread* [PATCH RESEND 5/5] staging: media: atomisp: Fix bool-related style issues
2020-09-21 21:53 [PATCH RESEND 0/5] atomisp: Fixes and cleanups Alex Dewar
` (3 preceding siblings ...)
2020-09-21 21:53 ` [PATCH RESEND 4/5] staging: media: atomisp: Don't abort on error in module exit path Alex Dewar
@ 2020-09-21 21:53 ` Alex Dewar
2020-09-22 8:11 ` [PATCH RESEND 0/5] atomisp: Fixes and cleanups Mauro Carvalho Chehab
5 siblings, 0 replies; 16+ messages in thread
From: Alex Dewar @ 2020-09-21 21:53 UTC (permalink / raw)
To: Mauro Carvalho Chehab, Sakari Ailus, Greg Kroah-Hartman,
Alex Dewar, Dan Carpenter, Linus Walleij, Andy Shevchenko,
Alan Cox, linux-media, devel, linux-kernel
Address the following issues:
* unnecessary comparison to true/false
* use of 0/1 instead of bool values
* unnecessary conversion to bool
These were fixed using the following Coccinelle scripts:
* scripts/coccinelle/misc/bool{init,conv,return}.cocci
Build-tested with allmodconfig.
Signed-off-by: Alex Dewar <alex.dewar90@gmail.com>
---
.../staging/media/atomisp/pci/atomisp_cmd.c | 5 ++---
drivers/staging/media/atomisp/pci/sh_css.c | 20 +++++++++----------
.../media/atomisp/pci/sh_css_firmware.c | 2 +-
3 files changed, 13 insertions(+), 14 deletions(-)
diff --git a/drivers/staging/media/atomisp/pci/atomisp_cmd.c b/drivers/staging/media/atomisp/pci/atomisp_cmd.c
index 9f25f13c3255..592ea990d4ca 100644
--- a/drivers/staging/media/atomisp/pci/atomisp_cmd.c
+++ b/drivers/staging/media/atomisp/pci/atomisp_cmd.c
@@ -654,8 +654,7 @@ bool atomisp_buffers_queued(struct atomisp_sub_device *asd)
return asd->video_out_capture.buffers_in_css ||
asd->video_out_vf.buffers_in_css ||
asd->video_out_preview.buffers_in_css ||
- asd->video_out_video_capture.buffers_in_css ?
- true : false;
+ asd->video_out_video_capture.buffers_in_css;
}
/* ISP2401 */
@@ -6549,7 +6548,7 @@ int atomisp_enable_dz_capt_pipe(struct atomisp_sub_device *asd,
if (!enable)
return -EINVAL;
- value = *enable > 0 ? true : false;
+ value = *enable > 0;
atomisp_en_dz_capt_pipe(asd, value);
diff --git a/drivers/staging/media/atomisp/pci/sh_css.c b/drivers/staging/media/atomisp/pci/sh_css.c
index 3e9366b20af0..c50b5fba7b86 100644
--- a/drivers/staging/media/atomisp/pci/sh_css.c
+++ b/drivers/staging/media/atomisp/pci/sh_css.c
@@ -3681,7 +3681,7 @@ static int create_host_video_pipeline(struct ia_css_pipe *pipe)
struct ia_css_frame *tmp_out_frame = NULL;
for (i = 0; i < num_yuv_scaler; i++) {
- if (is_output_stage[i] == true) {
+ if (is_output_stage[i]) {
tmp_out_frame = out_frame;
} else {
tmp_out_frame = NULL;
@@ -4421,7 +4421,7 @@ ia_css_pipe_dequeue_buffer(struct ia_css_pipe *pipe,
case IA_CSS_BUFFER_TYPE_INPUT_FRAME:
case IA_CSS_BUFFER_TYPE_OUTPUT_FRAME:
case IA_CSS_BUFFER_TYPE_SEC_OUTPUT_FRAME:
- if ((pipe) && (pipe->stop_requested == true)) {
+ if (pipe && pipe->stop_requested) {
#if !defined(ISP2401)
/* free mipi frames only for old input system
* for 2401 it is done in ia_css_stream_destroy call
@@ -4782,7 +4782,7 @@ sh_css_pipe_start(struct ia_css_stream *stream) {
pipe_id = pipe->mode;
- if (stream->started == true)
+ if (stream->started)
{
IA_CSS_WARNING("Cannot start stream that is already started");
IA_CSS_LEAVE_ERR(err);
@@ -5919,7 +5919,7 @@ static bool need_capture_pp(
if (IS_ISP2401) {
/* ldc and capture_pp are not supported in the same pipeline */
- if (need_capt_ldc(pipe) == true)
+ if (need_capt_ldc(pipe))
return false;
}
@@ -6135,8 +6135,8 @@ static int load_primary_binaries(
IA_CSS_LEAVE_ERR_PRIVATE(err);
return err;
}
- need_pp = 0;
- need_ldc = 0;
+ need_pp = false;
+ need_ldc = false;
}
/* we build up the pipeline starting at the end */
@@ -9496,10 +9496,10 @@ ia_css_stream_create(const struct ia_css_stream_config *stream_config,
IA_CSS_PIPE_MODE_VIDEO, false);
acc_pipe = find_pipe(pipes, num_pipes,
IA_CSS_PIPE_MODE_ACC, false);
- if (acc_pipe && num_pipes == 2 && curr_stream->cont_capt == true)
+ if (acc_pipe && num_pipes == 2 && curr_stream->cont_capt)
curr_stream->cont_capt =
false; /* preview + QoS case will not need cont_capt switch */
- if (curr_stream->cont_capt == true) {
+ if (curr_stream->cont_capt) {
capture_pipe = find_pipe(pipes, num_pipes,
IA_CSS_PIPE_MODE_CAPTURE, false);
if (!capture_pipe) {
@@ -9521,7 +9521,7 @@ ia_css_stream_create(const struct ia_css_stream_config *stream_config,
preview_pipe->pipe_settings.preview.copy_pipe = copy_pipe;
copy_pipe->stream = curr_stream;
}
- if (preview_pipe && (curr_stream->cont_capt == true)) {
+ if (preview_pipe && curr_stream->cont_capt) {
preview_pipe->pipe_settings.preview.capture_pipe = capture_pipe;
}
if (video_pipe && !video_pipe->pipe_settings.video.copy_pipe) {
@@ -9532,7 +9532,7 @@ ia_css_stream_create(const struct ia_css_stream_config *stream_config,
video_pipe->pipe_settings.video.copy_pipe = copy_pipe;
copy_pipe->stream = curr_stream;
}
- if (video_pipe && (curr_stream->cont_capt == true)) {
+ if (video_pipe && curr_stream->cont_capt) {
video_pipe->pipe_settings.video.capture_pipe = capture_pipe;
}
if (preview_pipe && acc_pipe) {
diff --git a/drivers/staging/media/atomisp/pci/sh_css_firmware.c b/drivers/staging/media/atomisp/pci/sh_css_firmware.c
index 244c7c7780a3..db25e39bea88 100644
--- a/drivers/staging/media/atomisp/pci/sh_css_firmware.c
+++ b/drivers/staging/media/atomisp/pci/sh_css_firmware.c
@@ -207,7 +207,7 @@ sh_css_check_firmware_version(struct device *dev, const char *fw_data)
}
/* For now, let's just accept a wrong version, even if wrong */
- return 0;
+ return false;
}
static const char * const fw_type_name[] = {
--
2.28.0
^ permalink raw reply related [flat|nested] 16+ messages in thread* Re: [PATCH RESEND 0/5] atomisp: Fixes and cleanups
2020-09-21 21:53 [PATCH RESEND 0/5] atomisp: Fixes and cleanups Alex Dewar
` (4 preceding siblings ...)
2020-09-21 21:53 ` [PATCH RESEND 5/5] staging: media: atomisp: Fix bool-related style issues Alex Dewar
@ 2020-09-22 8:11 ` Mauro Carvalho Chehab
2020-09-22 8:16 ` Alex Dewar
5 siblings, 1 reply; 16+ messages in thread
From: Mauro Carvalho Chehab @ 2020-09-22 8:11 UTC (permalink / raw)
To: Alex Dewar
Cc: Sakari Ailus, Greg Kroah-Hartman, Dan Carpenter, Linus Walleij,
Alan Cox, linux-media, devel, linux-kernel
Em Mon, 21 Sep 2020 22:53:49 +0100
Alex Dewar <alex.dewar90@gmail.com> escreveu:
> Hi Mauro,
>
> Over the last month I've sent a few scattered patches to fix various
> warnings from static analysers, but they seem to have fallen through the
> cracks? I'm reposting them here as a series to make them easier to
> review. If you do have any feedback that'd be great :)
Usually, there's no need to re-send the patches, as we pick them
from a patchwork queue.
However, only one of the patches actually applied against the
linux-media tree.
So, please rebased the remaining patches on the top of it.
Thanks,
Mauro
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH RESEND 0/5] atomisp: Fixes and cleanups
2020-09-22 8:11 ` [PATCH RESEND 0/5] atomisp: Fixes and cleanups Mauro Carvalho Chehab
@ 2020-09-22 8:16 ` Alex Dewar
2020-09-22 9:09 ` [PATCH REBASE 0/3] atomisp: Rebased fixes Alex Dewar
0 siblings, 1 reply; 16+ messages in thread
From: Alex Dewar @ 2020-09-22 8:16 UTC (permalink / raw)
To: Mauro Carvalho Chehab
Cc: Sakari Ailus, Greg Kroah-Hartman, Dan Carpenter, Linus Walleij,
Alan Cox, linux-media, devel, linux-kernel
On 22/09/2020 09:11, Mauro Carvalho Chehab wrote:
> Em Mon, 21 Sep 2020 22:53:49 +0100
> Alex Dewar <alex.dewar90@gmail.com> escreveu:
>
>> Hi Mauro,
>>
>> Over the last month I've sent a few scattered patches to fix various
>> warnings from static analysers, but they seem to have fallen through the
>> cracks? I'm reposting them here as a series to make them easier to
>> review. If you do have any feedback that'd be great :)
> Usually, there's no need to re-send the patches, as we pick them
> from a patchwork queue.
>
> However, only one of the patches actually applied against the
> linux-media tree.
>
> So, please rebased the remaining patches on the top of it.
>
> Thanks,
> Mauro
That's weird. They applied cleanly against yesterday's linux-next for
me... I'll rebase on linux-media and resend.
Best,
Alex
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH REBASE 0/3] atomisp: Rebased fixes
2020-09-22 8:16 ` Alex Dewar
@ 2020-09-22 9:09 ` Alex Dewar
2020-09-22 9:09 ` [PATCH REBASE 1/3] staging: media: atomisp: Fix error path in lm3554_probe() Alex Dewar
` (3 more replies)
0 siblings, 4 replies; 16+ messages in thread
From: Alex Dewar @ 2020-09-22 9:09 UTC (permalink / raw)
To: Mauro Carvalho Chehab, Sakari Ailus, Greg Kroah-Hartman,
Alex Dewar, Dan Carpenter, Alan Cox, linux-media, devel,
linux-kernel
Hi Mauro,
I've rebased the patches now, but there is a slight hiccup. For patches 2
and 3 of this series there will now be a conflict with commit 9289cdf39992
("staging: media: atomisp: Convert to GPIO descriptors") in Greg's tree.
I'm not sure what the best way to handle this is? The merge conflicts
will be trivial (due to a conversion between the gpio_* and gpiod_*
APIs), but I could alternatively send these last two patches in via
Greg's tree if that's easier for people. Let me know what works.
Best,
Alex
^ permalink raw reply [flat|nested] 16+ messages in thread* [PATCH REBASE 1/3] staging: media: atomisp: Fix error path in lm3554_probe()
2020-09-22 9:09 ` [PATCH REBASE 0/3] atomisp: Rebased fixes Alex Dewar
@ 2020-09-22 9:09 ` Alex Dewar
2020-09-22 9:09 ` [PATCH REBASE 2/3] staging: media: atomisp: Remove unhelpful info message Alex Dewar
` (2 subsequent siblings)
3 siblings, 0 replies; 16+ messages in thread
From: Alex Dewar @ 2020-09-22 9:09 UTC (permalink / raw)
To: Mauro Carvalho Chehab, Sakari Ailus, Greg Kroah-Hartman,
Alex Dewar, Dan Carpenter, Alan Cox, linux-media, devel,
linux-kernel
The error path for lm3554_probe() contains a number of bugs, including:
* resource leaks
* jumping to error labels out of sequence
* not setting the return value appropriately
Fix it up and give the labels more memorable names.
This issue has existed since the code was originally contributed in
commit a49d25364dfb ("staging/atomisp: Add support for the Intel IPU v2"),
although the code was subsequently removed altogether and then
reinstated with commit ad85094b293e ("Revert "media: staging: atomisp: Remove driver"").
Addresses-Coverity: 1496802 ("Resource leaks")
Fixes: a49d25364dfb ("staging/atomisp: Add support for the Intel IPU v2")
Signed-off-by: Alex Dewar <alex.dewar90@gmail.com>
Reviewed-by: Dan Carpenter <dan.carpenter@oracle.com>
---
.../media/atomisp/i2c/atomisp-lm3554.c | 48 +++++++++++--------
1 file changed, 27 insertions(+), 21 deletions(-)
diff --git a/drivers/staging/media/atomisp/i2c/atomisp-lm3554.c b/drivers/staging/media/atomisp/i2c/atomisp-lm3554.c
index 809010af7855..67e62b96adf9 100644
--- a/drivers/staging/media/atomisp/i2c/atomisp-lm3554.c
+++ b/drivers/staging/media/atomisp/i2c/atomisp-lm3554.c
@@ -847,7 +847,6 @@ static void *lm3554_platform_data_func(struct i2c_client *client)
static int lm3554_probe(struct i2c_client *client)
{
- int err = 0;
struct lm3554 *flash;
unsigned int i;
int ret;
@@ -863,28 +862,28 @@ 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));
+ ret = v4l2_ctrl_handler_init(&flash->ctrl_handler,
+ ARRAY_SIZE(lm3554_controls));
if (ret) {
- dev_err(&client->dev, "error initialize a ctrl_handler.\n");
- goto fail2;
+ dev_err(&client->dev, "error initializing ctrl_handler");
+ goto err_unregister_sd;
}
for (i = 0; i < ARRAY_SIZE(lm3554_controls); i++)
v4l2_ctrl_new_custom(&flash->ctrl_handler, &lm3554_controls[i],
NULL);
- if (flash->ctrl_handler.error) {
- dev_err(&client->dev, "ctrl_handler error.\n");
- goto fail2;
+ ret = flash->ctrl_handler.error;
+ if (ret) {
+ dev_err(&client->dev, "ctrl_handler error");
+ goto err_free_ctrl_handler;
}
flash->sd.ctrl_handler = &flash->ctrl_handler;
- err = media_entity_pads_init(&flash->sd.entity, 0, NULL);
- if (err) {
- dev_err(&client->dev, "error initialize a media entity.\n");
- goto fail1;
+ ret = media_entity_pads_init(&flash->sd.entity, 0, NULL);
+ if (ret) {
+ dev_err(&client->dev, "error initializing media entity");
+ goto err_free_ctrl_handler;
}
flash->sd.entity.function = MEDIA_ENT_F_FLASH;
@@ -893,20 +892,27 @@ static int lm3554_probe(struct i2c_client *client)
timer_setup(&flash->flash_off_delay, lm3554_flash_off_delay, 0);
- err = lm3554_gpio_init(client);
- if (err) {
+ ret = lm3554_gpio_init(client);
+ if (ret) {
dev_err(&client->dev, "gpio request/direction_output fail");
- goto fail2;
+ goto err_del_timer;
}
- return atomisp_register_i2c_module(&flash->sd, NULL, LED_FLASH);
-fail2:
+
+ ret = atomisp_register_i2c_module(&flash->sd, NULL, LED_FLASH);
+ if (ret)
+ return ret;
+
+ return 0;
+
+err_del_timer:
+ del_timer_sync(&flash->flash_off_delay);
media_entity_cleanup(&flash->sd.entity);
+err_free_ctrl_handler:
v4l2_ctrl_handler_free(&flash->ctrl_handler);
-fail1:
+err_unregister_sd:
v4l2_device_unregister_subdev(&flash->sd);
kfree(flash);
-
- return err;
+ return ret;
}
static int lm3554_remove(struct i2c_client *client)
--
2.28.0
^ permalink raw reply related [flat|nested] 16+ messages in thread* [PATCH REBASE 2/3] staging: media: atomisp: Remove unhelpful info message
2020-09-22 9:09 ` [PATCH REBASE 0/3] atomisp: Rebased fixes Alex Dewar
2020-09-22 9:09 ` [PATCH REBASE 1/3] staging: media: atomisp: Fix error path in lm3554_probe() Alex Dewar
@ 2020-09-22 9:09 ` Alex Dewar
2020-09-22 9:09 ` [PATCH REBASE 3/3] staging: media: atomisp: Don't abort on error in module exit path Alex Dewar
2020-09-22 9:27 ` [PATCH REBASE 0/3] atomisp: Rebased fixes Mauro Carvalho Chehab
3 siblings, 0 replies; 16+ messages in thread
From: Alex Dewar @ 2020-09-22 9:09 UTC (permalink / raw)
To: Mauro Carvalho Chehab, Sakari Ailus, Greg Kroah-Hartman,
Alex Dewar, Dan Carpenter, Alan Cox, linux-media, devel,
linux-kernel
We don't really need to know that the LED pin reset successfully.
Signed-off-by: Alex Dewar <alex.dewar90@gmail.com>
---
drivers/staging/media/atomisp/i2c/atomisp-lm3554.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/drivers/staging/media/atomisp/i2c/atomisp-lm3554.c b/drivers/staging/media/atomisp/i2c/atomisp-lm3554.c
index 67e62b96adf9..5e895586e80a 100644
--- a/drivers/staging/media/atomisp/i2c/atomisp-lm3554.c
+++ b/drivers/staging/media/atomisp/i2c/atomisp-lm3554.c
@@ -772,7 +772,6 @@ static int lm3554_gpio_init(struct i2c_client *client)
ret = gpio_direction_output(pdata->gpio_reset, 0);
if (ret < 0)
goto err_gpio_reset;
- dev_info(&client->dev, "flash led reset successfully\n");
if (!gpio_is_valid(pdata->gpio_strobe)) {
ret = -EINVAL;
--
2.28.0
^ permalink raw reply related [flat|nested] 16+ messages in thread* [PATCH REBASE 3/3] staging: media: atomisp: Don't abort on error in module exit path
2020-09-22 9:09 ` [PATCH REBASE 0/3] atomisp: Rebased fixes Alex Dewar
2020-09-22 9:09 ` [PATCH REBASE 1/3] staging: media: atomisp: Fix error path in lm3554_probe() Alex Dewar
2020-09-22 9:09 ` [PATCH REBASE 2/3] staging: media: atomisp: Remove unhelpful info message Alex Dewar
@ 2020-09-22 9:09 ` Alex Dewar
2020-09-22 9:27 ` [PATCH REBASE 0/3] atomisp: Rebased fixes Mauro Carvalho Chehab
3 siblings, 0 replies; 16+ messages in thread
From: Alex Dewar @ 2020-09-22 9:09 UTC (permalink / raw)
To: Mauro Carvalho Chehab, Sakari Ailus, Greg Kroah-Hartman,
Alex Dewar, Dan Carpenter, Alan Cox, linux-media, devel,
linux-kernel
The function lm3554_remove() checks for the return code for
lm3554_gpio_uninit() even though this is on the exit path and exits the
function, leaving the variable flash unfreed. Instead, print a warning and
free flash unconditionally.
Signed-off-by: Alex Dewar <alex.dewar90@gmail.com>
---
.../staging/media/atomisp/i2c/atomisp-lm3554.c | 17 ++++++-----------
1 file changed, 6 insertions(+), 11 deletions(-)
diff --git a/drivers/staging/media/atomisp/i2c/atomisp-lm3554.c b/drivers/staging/media/atomisp/i2c/atomisp-lm3554.c
index 5e895586e80a..84c47c1f9eb4 100644
--- a/drivers/staging/media/atomisp/i2c/atomisp-lm3554.c
+++ b/drivers/staging/media/atomisp/i2c/atomisp-lm3554.c
@@ -794,7 +794,7 @@ static int lm3554_gpio_init(struct i2c_client *client)
return ret;
}
-static int lm3554_gpio_uninit(struct i2c_client *client)
+static void lm3554_gpio_uninit(struct i2c_client *client)
{
struct v4l2_subdev *sd = i2c_get_clientdata(client);
struct lm3554 *flash = to_lm3554(sd);
@@ -803,15 +803,16 @@ static int lm3554_gpio_uninit(struct i2c_client *client)
ret = gpio_direction_output(pdata->gpio_strobe, 0);
if (ret < 0)
- return ret;
+ dev_err(&client->dev,
+ "gpio request/direction_output fail for gpio_strobe");
ret = gpio_direction_output(pdata->gpio_reset, 0);
if (ret < 0)
- return ret;
+ dev_err(&client->dev,
+ "gpio request/direction_output fail for gpio_reset");
gpio_free(pdata->gpio_strobe);
gpio_free(pdata->gpio_reset);
- return 0;
}
static void *lm3554_platform_data_func(struct i2c_client *client)
@@ -918,7 +919,6 @@ static int lm3554_remove(struct i2c_client *client)
{
struct v4l2_subdev *sd = i2c_get_clientdata(client);
struct lm3554 *flash = to_lm3554(sd);
- int ret;
media_entity_cleanup(&flash->sd.entity);
v4l2_ctrl_handler_free(&flash->ctrl_handler);
@@ -928,16 +928,11 @@ static int lm3554_remove(struct i2c_client *client)
del_timer_sync(&flash->flash_off_delay);
- ret = lm3554_gpio_uninit(client);
- if (ret < 0)
- goto fail;
+ lm3554_gpio_uninit(client);
kfree(flash);
return 0;
-fail:
- dev_err(&client->dev, "gpio request/direction_output fail");
- return ret;
}
static const struct dev_pm_ops lm3554_pm_ops = {
--
2.28.0
^ permalink raw reply related [flat|nested] 16+ messages in thread* Re: [PATCH REBASE 0/3] atomisp: Rebased fixes
2020-09-22 9:09 ` [PATCH REBASE 0/3] atomisp: Rebased fixes Alex Dewar
` (2 preceding siblings ...)
2020-09-22 9:09 ` [PATCH REBASE 3/3] staging: media: atomisp: Don't abort on error in module exit path Alex Dewar
@ 2020-09-22 9:27 ` Mauro Carvalho Chehab
2020-09-22 11:02 ` Alex Dewar
3 siblings, 1 reply; 16+ messages in thread
From: Mauro Carvalho Chehab @ 2020-09-22 9:27 UTC (permalink / raw)
To: Alex Dewar
Cc: Sakari Ailus, Greg Kroah-Hartman, Dan Carpenter, Alan Cox,
linux-media, devel, linux-kernel
Em Tue, 22 Sep 2020 10:09:07 +0100
Alex Dewar <alex.dewar90@gmail.com> escreveu:
> Hi Mauro,
>
> I've rebased the patches now, but there is a slight hiccup. For patches 2
> and 3 of this series there will now be a conflict with commit 9289cdf39992
> ("staging: media: atomisp: Convert to GPIO descriptors") in Greg's tree.
>
> I'm not sure what the best way to handle this is? The merge conflicts
> will be trivial (due to a conversion between the gpio_* and gpiod_*
> APIs), but I could alternatively send these last two patches in via
> Greg's tree if that's easier for people. Let me know what works.
Maybe the best would be to re-send those after the merge window, when
both patches will arrive upstream.
Thanks,
Mauro
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH REBASE 0/3] atomisp: Rebased fixes
2020-09-22 9:27 ` [PATCH REBASE 0/3] atomisp: Rebased fixes Mauro Carvalho Chehab
@ 2020-09-22 11:02 ` Alex Dewar
2020-09-22 13:27 ` Dan Carpenter
0 siblings, 1 reply; 16+ messages in thread
From: Alex Dewar @ 2020-09-22 11:02 UTC (permalink / raw)
To: Mauro Carvalho Chehab
Cc: Sakari Ailus, Greg Kroah-Hartman, Dan Carpenter, Alan Cox,
linux-media, devel, linux-kernel
On 22/09/2020 10:27, Mauro Carvalho Chehab wrote:
> Em Tue, 22 Sep 2020 10:09:07 +0100
> Alex Dewar <alex.dewar90@gmail.com> escreveu:
>
>> Hi Mauro,
>>
>> I've rebased the patches now, but there is a slight hiccup. For patches 2
>> and 3 of this series there will now be a conflict with commit 9289cdf39992
>> ("staging: media: atomisp: Convert to GPIO descriptors") in Greg's tree.
>>
>> I'm not sure what the best way to handle this is? The merge conflicts
>> will be trivial (due to a conversion between the gpio_* and gpiod_*
>> APIs), but I could alternatively send these last two patches in via
>> Greg's tree if that's easier for people. Let me know what works.
> Maybe the best would be to re-send those after the merge window, when
> both patches will arrive upstream.
>
> Thanks,
> Mauro
That sounds more sensible. I've also just noticed that I introduced a
bug in the first patch when rebasing it :-/, so let's hold off on the
whole series and I'll do a proper tidy and resend after the next merge
window.
Best,
Alex
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH REBASE 0/3] atomisp: Rebased fixes
2020-09-22 11:02 ` Alex Dewar
@ 2020-09-22 13:27 ` Dan Carpenter
2020-09-22 13:51 ` Alex Dewar
0 siblings, 1 reply; 16+ messages in thread
From: Dan Carpenter @ 2020-09-22 13:27 UTC (permalink / raw)
To: Alex Dewar
Cc: Mauro Carvalho Chehab, devel, Greg Kroah-Hartman, linux-kernel,
Sakari Ailus, Alan Cox, linux-media
On Tue, Sep 22, 2020 at 12:02:33PM +0100, Alex Dewar wrote:
> On 22/09/2020 10:27, Mauro Carvalho Chehab wrote:
> > Em Tue, 22 Sep 2020 10:09:07 +0100
> > Alex Dewar <alex.dewar90@gmail.com> escreveu:
> >
> > > Hi Mauro,
> > >
> > > I've rebased the patches now, but there is a slight hiccup. For patches 2
> > > and 3 of this series there will now be a conflict with commit 9289cdf39992
> > > ("staging: media: atomisp: Convert to GPIO descriptors") in Greg's tree.
> > >
> > > I'm not sure what the best way to handle this is? The merge conflicts
> > > will be trivial (due to a conversion between the gpio_* and gpiod_*
> > > APIs), but I could alternatively send these last two patches in via
> > > Greg's tree if that's easier for people. Let me know what works.
> > Maybe the best would be to re-send those after the merge window, when
> > both patches will arrive upstream.
> >
> > Thanks,
> > Mauro
> That sounds more sensible. I've also just noticed that I introduced a bug in
> the first patch when rebasing it :-/, so let's hold off on the whole series
> and I'll do a proper tidy and resend after the next merge window.
Is the bug the memory leak if lm3554_platform_data_func() fails?
regards,
dan carpenter
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH REBASE 0/3] atomisp: Rebased fixes
2020-09-22 13:27 ` Dan Carpenter
@ 2020-09-22 13:51 ` Alex Dewar
0 siblings, 0 replies; 16+ messages in thread
From: Alex Dewar @ 2020-09-22 13:51 UTC (permalink / raw)
To: Dan Carpenter
Cc: Alex Dewar, Mauro Carvalho Chehab, devel, Greg Kroah-Hartman,
linux-kernel, Sakari Ailus, Alan Cox, linux-media
[snip]
> > That sounds more sensible. I've also just noticed that I introduced a bug in
> > the first patch when rebasing it :-/, so let's hold off on the whole series
> > and I'll do a proper tidy and resend after the next merge window.
>
> Is the bug the memory leak if lm3554_platform_data_func() fails?
>
> regards,
> dan carpenter
>
>
Nope. I put a "return ret" for the last check instead of "goto err_del_timer"...
The version of this code in linux-next does the correct "if (PTR_ERR(...))"
check after calling lm3554_platform_data_func(), but this patch doesn't
seem to have made its way into linux-media yet. All the more reason to
resend my patches after the merge window, I suppose.
Best,
Alex
^ permalink raw reply [flat|nested] 16+ messages in thread