[PATCH 0/3] media: staging: atomisp:

[PATCH 0/3] media: staging: atomisp:

[Christophe JAILLET]

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

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH 1/3] media: staging: atomisp: Return an error code in case of error in 'lm3554_probe()'

[Christophe JAILLET]

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 <christophe.jaillet@wanadoo.fr>
---
 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

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH 2/3] media: staging: atomisp: Fix an error handling path in 'lm3554_probe()'

[Christophe JAILLET]

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 <christophe.jaillet@wanadoo.fr>
---
 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

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 2/3] media: staging: atomisp: Fix an error handling path in 'lm3554_probe()'

[Julia Lawall]

On Fri, 11 May 2018, Christophe JAILLET wrote:

> The use of 'fail1' and 'fail2' is not correct. Reorder these calls to
> branch at the right place of the error handling path.

Maybe it would be good to improve the names at the same time?

julia

>
> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> ---
>  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
>
> --
> To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Re: [PATCH 2/3] media: staging: atomisp: Fix an error handling path in 'lm3554_probe()'

[Alan Cox]

On Fri, 2018-05-11 at 17:09 +0200, Julia Lawall wrote:
> 
> On Fri, 11 May 2018, Christophe JAILLET wrote:
> 
> > The use of 'fail1' and 'fail2' is not correct. Reorder these calls
> > to
> > branch at the right place of the error handling path.
> 
> Maybe it would be good to improve the names at the same time?

Its scheduled for deletion - please don't bother.

Alan

[PATCH 3/3] media: staging: atomisp: Fix usage of 'media_entity_cleanup()'

[Christophe JAILLET]

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 <christophe.jaillet@wanadoo.fr>
---
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

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel