media: ov2680: register the v4l2 subdev async at the end of probe
PATCH 1/1 media: ov2680: register the v4l2 subdev async at the end of probe
The driver registers the subdev async in the middle of the probe function but this has to be done at the very end of the probe function to prevent registering a device whose probe function could fail (i.e: the clock and regulators enable can fail, the I2C transfers could return errors, etc).
It could also lead to a media device driver that is waiting to bound the v4l2 subdevice to incorrectly expose its media device to userspace, since the subdev is registered but later its media entity is cleaned up on error.
---
drivers/media/i2c/ov2680.c | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-)
drivers/media/i2c/ov2680.c +4 -5
@@ -983,10 +983,6 @@ static int ov2680_v4l2_init(struct ov2680_dev *sensor) sensor->sd.ctrl_handler = hdl; - ret =v4l2_async_register_subdev (&sensor->sd); -if (ret <0 ) - goto cleanup_entity; -return 0 ; cleanup_entity:
@@ -1096,6 +1092,10 @@ static int ov2680_probe(struct i2c_client *client)if (ret <0 ) goto error_cleanup; + ret =v4l2_async_register_subdev (&sensor->sd); +if (ret <0 ) + goto error_cleanup; +dev_info (dev,"ov2680 init correctly\n" );return 0 ;
@@ -1104,7 +1104,6 @@ static int ov2680_probe(struct i2c_client *client)dev_err (dev,"ov2680 init fail: %d\n" , ret);media_entity_cleanup (&sensor->sd.entity); -v4l2_async_unregister_subdev (&sensor->sd);v4l2_ctrl_handler_free (&sensor->ctrls.handler); lock_destroy:
Signature · 1 line
2.17.1
Hi Javier,
Quoted 33 lines · Javier Martinez Canillas
The driver registers the subdev async in the middle of the probe function but this has to be done at the very end of the probe function to prevent registering a device whose probe function could fail (i.e: the clock and regulators enable can fail, the I2C transfers could return errors, etc).
It could also lead to a media device driver that is waiting to bound the v4l2 subdevice to incorrectly expose its media device to userspace, since the subdev is registered but later its media entity is cleaned up on error.
Fixes 3ee47cad3e69 ("media: ov2680: Add Omnivision OV2680 sensor driver")Signed-off-by Javier Martinez Canillas <[email protected]>---
drivers/media/i2c/ov2680.c | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-)drivers/media/i2c/ov2680.c +0
-4@@ -983,10 +983,6 @@ static int ov2680_v4l2_init(struct ov2680_dev *sensor) sensor->sd.ctrl_handler = hdl; - ret =v4l2_async_register_subdev (&sensor->sd); -if (ret <0 ) - goto cleanup_entity; -return 0 ; cleanup_entity:
drivers/media/i2c/ov2680.c static int ov2680_probe(struct i2c_client *client) · line 1093
Quoted 2 lines
drivers/media/i2c/ov2680.c +0
-0@@ -1096,6 +1092,10 @@ static int ov2680_probe(struct i2c_client *client)if (ret <0 ) goto error_cleanup;
How about instead moving ov2680_check_id() call earlier in probe()? That would seem to be a better fix: the driver should check the device is around before registering anything.
Quoted 16 lines
drivers/media/i2c/ov2680.c +4
-1+ ret =v4l2_async_register_subdev (&sensor->sd); +if (ret <0 ) + goto error_cleanup; +dev_info (dev,"ov2680 init correctly\n" );return 0 ;@@ -1104,7 +1104,6 @@ static int ov2680_probe(struct i2c_client *client)dev_err (dev,"ov2680 init fail: %d\n" , ret);media_entity_cleanup (&sensor->sd.entity); -v4l2_async_unregister_subdev (&sensor->sd);v4l2_ctrl_handler_free (&sensor->ctrls.handler); lock_destroy:
Signature · 4 lines
Regards, Sakari Ailus e-mail: [email protected]
Hi Sakari,
Thanks for the feedback.
Quoted 34 lines · Sakari Ailus
Hi Javier,
Quoted 31 lines · Javier Martinez Canillas
The driver registers the subdev async in the middle of the probe function but this has to be done at the very end of the probe function to prevent registering a device whose probe function could fail (i.e: the clock and regulators enable can fail, the I2C transfers could return errors, etc).
It could also lead to a media device driver that is waiting to bound the v4l2 subdevice to incorrectly expose its media device to userspace, since the subdev is registered but later its media entity is cleaned up on error.
Fixes 3ee47cad3e69 ("media: ov2680: Add Omnivision OV2680 sensor driver")Signed-off-by Javier Martinez Canillas <[email protected]>---
drivers/media/i2c/ov2680.c | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-)drivers/media/i2c/ov2680.c +0
-4@@ -983,10 +983,6 @@ static int ov2680_v4l2_init(struct ov2680_dev *sensor) sensor->sd.ctrl_handler = hdl; - ret =v4l2_async_register_subdev (&sensor->sd); -if (ret <0 ) - goto cleanup_entity; -return 0 ;
Quoted 9 lines
drivers/media/i2c/ov2680.c static int ov2680_probe(struct i2c_client *client) · line 1093
Quoted 4 lines
drivers/media/i2c/ov2680.c +0
-0cleanup_entity:@@ -1096,6 +1092,10 @@ static int ov2680_probe(struct i2c_client *client)if (ret <0 ) goto error_cleanup;How about instead moving
ov2680_check_id()call earlier inprobe()? That would seem to be a better fix: the driver should check the device is around before registering anything.
Yes, that would work too. We can't move it too early though since it has to be after the DT was parsed, the regulator, clocks and gpio looked up, etc.
But moving ov2680_check_id() before ov2680_v4l2_init() would work indeed, in that case ov2680_v4l2_init() should be renamed to ov2680_v4l2_register() I think to make it clear that the function not only does initialization but also register the v4l2 subdevice.
I'll post a v2 doing what you suggest.
Best regards,
Signature · 3 lines
Javier Martinez Canillas Software Engineer - Desktop Hardware Enablement Red Hat