* [PATCH 0/3] media: staging: atomisp:
@ 2018-05-11 15:06 ` Christophe JAILLET
0 siblings, 0 replies; 16+ messages in thread
From: Christophe JAILLET @ 2018-05-11 15:06 UTC (permalink / raw)
To: alan, sakari.ailus, mchehab, gregkh, andriy.shevchenko,
chen.chenchacha, keescook, arvind.yadav.cs
Cc: devel, Christophe JAILLET, kernel-janitors, linux-kernel,
linux-media
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
^ permalink raw reply [flat|nested] 16+ messages in thread* [PATCH 0/3] media: staging: atomisp:
@ 2018-05-11 15:06 ` Christophe JAILLET
0 siblings, 0 replies; 16+ messages in thread
From: Christophe JAILLET @ 2018-05-11 15:06 UTC (permalink / raw)
To: alan, sakari.ailus, mchehab, gregkh, andriy.shevchenko,
chen.chenchacha, keescook, arvind.yadav.cs
Cc: linux-media, devel, linux-kernel, kernel-janitors,
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
^ permalink raw reply [flat|nested] 16+ messages in thread* [PATCH 1/3] media: staging: atomisp: Return an error code in case of error in 'lm3554_probe()'
2018-05-11 15:06 ` Christophe JAILLET
(?)
@ 2018-05-11 15:06 ` Christophe JAILLET
-1 siblings, 0 replies; 16+ messages in thread
From: Christophe JAILLET @ 2018-05-11 15:06 UTC (permalink / raw)
To: alan, sakari.ailus, mchehab, gregkh, andriy.shevchenko,
chen.chenchacha, keescook, arvind.yadav.cs
Cc: linux-media, devel, linux-kernel, kernel-janitors,
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
^ permalink raw reply related [flat|nested] 16+ messages in thread* [PATCH 1/3] media: staging: atomisp: Return an error code in case of error in 'lm3554_probe()'
@ 2018-05-11 15:06 ` Christophe JAILLET
0 siblings, 0 replies; 16+ messages in thread
From: Christophe JAILLET @ 2018-05-11 15:06 UTC (permalink / raw)
To: alan, sakari.ailus, mchehab, gregkh, andriy.shevchenko,
chen.chenchacha, keescook, arvind.yadav.cs
Cc: devel, Christophe JAILLET, kernel-janitors, linux-kernel,
linux-media
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
^ permalink raw reply related [flat|nested] 16+ messages in thread* [PATCH 1/3] media: staging: atomisp: Return an error code in case of error in 'lm3554_probe()'
@ 2018-05-11 15:06 ` Christophe JAILLET
0 siblings, 0 replies; 16+ messages in thread
From: Christophe JAILLET @ 2018-05-11 15:06 UTC (permalink / raw)
To: alan, sakari.ailus, mchehab, gregkh, andriy.shevchenko,
chen.chenchacha, keescook, arvind.yadav.cs
Cc: linux-media, devel, linux-kernel, kernel-janitors,
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
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH 2/3] media: staging: atomisp: Fix an error handling path in 'lm3554_probe()'
2018-05-11 15:06 ` Christophe JAILLET
(?)
@ 2018-05-11 15:06 ` Christophe JAILLET
-1 siblings, 0 replies; 16+ messages in thread
From: Christophe JAILLET @ 2018-05-11 15:06 UTC (permalink / raw)
To: alan, sakari.ailus, mchehab, gregkh, andriy.shevchenko,
chen.chenchacha, keescook, arvind.yadav.cs
Cc: linux-media, devel, linux-kernel, kernel-janitors,
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
^ permalink raw reply related [flat|nested] 16+ messages in thread* [PATCH 2/3] media: staging: atomisp: Fix an error handling path in 'lm3554_probe()'
@ 2018-05-11 15:06 ` Christophe JAILLET
0 siblings, 0 replies; 16+ messages in thread
From: Christophe JAILLET @ 2018-05-11 15:06 UTC (permalink / raw)
To: alan, sakari.ailus, mchehab, gregkh, andriy.shevchenko,
chen.chenchacha, keescook, arvind.yadav.cs
Cc: devel, Christophe JAILLET, kernel-janitors, linux-kernel,
linux-media
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
^ permalink raw reply related [flat|nested] 16+ messages in thread* [PATCH 2/3] media: staging: atomisp: Fix an error handling path in 'lm3554_probe()'
@ 2018-05-11 15:06 ` Christophe JAILLET
0 siblings, 0 replies; 16+ messages in thread
From: Christophe JAILLET @ 2018-05-11 15:06 UTC (permalink / raw)
To: alan, sakari.ailus, mchehab, gregkh, andriy.shevchenko,
chen.chenchacha, keescook, arvind.yadav.cs
Cc: linux-media, devel, linux-kernel, kernel-janitors,
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
^ permalink raw reply related [flat|nested] 16+ messages in thread* Re: [PATCH 2/3] media: staging: atomisp: Fix an error handling path in 'lm3554_probe()'
2018-05-11 15:06 ` Christophe JAILLET
@ 2018-05-11 15:09 ` Julia Lawall
-1 siblings, 0 replies; 16+ messages in thread
From: Julia Lawall @ 2018-05-11 15:09 UTC (permalink / raw)
To: Christophe JAILLET
Cc: alan, sakari.ailus, mchehab, gregkh, andriy.shevchenko,
chen.chenchacha, keescook, arvind.yadav.cs, linux-media, devel,
linux-kernel, kernel-janitors
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
>
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH 2/3] media: staging: atomisp: Fix an error handling path in 'lm3554_probe()'
@ 2018-05-11 15:09 ` Julia Lawall
0 siblings, 0 replies; 16+ messages in thread
From: Julia Lawall @ 2018-05-11 15:09 UTC (permalink / raw)
To: Christophe JAILLET
Cc: alan, sakari.ailus, mchehab, gregkh, andriy.shevchenko,
chen.chenchacha, keescook, arvind.yadav.cs, linux-media, devel,
linux-kernel, kernel-janitors
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
>
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH 2/3] media: staging: atomisp: Fix an error handling path in 'lm3554_probe()'
2018-05-11 15:09 ` Julia Lawall
@ 2018-05-11 15:31 ` Alan Cox
-1 siblings, 0 replies; 16+ messages in thread
From: Alan Cox @ 2018-05-11 15:31 UTC (permalink / raw)
To: Julia Lawall, Christophe JAILLET
Cc: sakari.ailus, mchehab, gregkh, andriy.shevchenko, chen.chenchacha,
keescook, arvind.yadav.cs, linux-media, devel, linux-kernel,
kernel-janitors
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
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/3] media: staging: atomisp: Fix an error handling path in 'lm3554_probe()'
@ 2018-05-11 15:31 ` Alan Cox
0 siblings, 0 replies; 16+ messages in thread
From: Alan Cox @ 2018-05-11 15:31 UTC (permalink / raw)
To: Julia Lawall, Christophe JAILLET
Cc: sakari.ailus, mchehab, gregkh, andriy.shevchenko, chen.chenchacha,
keescook, arvind.yadav.cs, linux-media, devel, linux-kernel,
kernel-janitors
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
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH 3/3] media: staging: atomisp: Fix usage of 'media_entity_cleanup()'
2018-05-11 15:06 ` Christophe JAILLET
(?)
@ 2018-05-11 15:06 ` Christophe JAILLET
-1 siblings, 0 replies; 16+ messages in thread
From: Christophe JAILLET @ 2018-05-11 15:06 UTC (permalink / raw)
To: alan, sakari.ailus, mchehab, gregkh, andriy.shevchenko,
chen.chenchacha, keescook, arvind.yadav.cs
Cc: linux-media, devel, linux-kernel, kernel-janitors,
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
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH 3/3] media: staging: atomisp: Fix usage of 'media_entity_cleanup()'
@ 2018-05-11 15:06 ` Christophe JAILLET
0 siblings, 0 replies; 16+ messages in thread
From: Christophe JAILLET @ 2018-05-11 15:06 UTC (permalink / raw)
To: alan, sakari.ailus, mchehab, gregkh, andriy.shevchenko,
chen.chenchacha, keescook, arvind.yadav.cs
Cc: devel, Christophe JAILLET, kernel-janitors, linux-kernel,
linux-media
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
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH 3/3] media: staging: atomisp: Fix usage of 'media_entity_cleanup()'
@ 2018-05-11 15:06 ` Christophe JAILLET
0 siblings, 0 replies; 16+ messages in thread
From: Christophe JAILLET @ 2018-05-11 15:06 UTC (permalink / raw)
To: alan, sakari.ailus, mchehab, gregkh, andriy.shevchenko,
chen.chenchacha, keescook, arvind.yadav.cs
Cc: linux-media, devel, linux-kernel, kernel-janitors,
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
^ permalink raw reply related [flat|nested] 16+ messages in thread