[PATCH] media: i2c: ov5648: Fix lockdep error

[PATCH] media: i2c: ov5648: Fix lockdep error

[Hans de Goede]

ov5648_state_init() calls ov5648_state_mipi_configure() which uses
__v4l2_ctrl_s_ctrl[_int64](). This means that sensor->mutex (which
is also sensor->ctrls.handler.lock) must be locked before calling
ov5648_state_init().

ov5648_state_mipi_configure() is also used in other places where
the lock is already held so it cannot be changed itself.

Note this is based on an identical (tested) fix for the ov8865 driver,
this has only been compile-tested.

Cc: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/media/i2c/ov5648.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/media/i2c/ov5648.c b/drivers/media/i2c/ov5648.c
index 947d437ed0ef..9f24bdccf50f 100644
--- a/drivers/media/i2c/ov5648.c
+++ b/drivers/media/i2c/ov5648.c
@@ -2546,7 +2546,9 @@ static int ov5648_probe(struct i2c_client *client)
 	if (ret)
 		goto error_mutex;
 
+	mutex_lock(&sensor->mutex);
 	ret = ov5648_state_init(sensor);
+	mutex_unlock(&sensor->mutex);
 	if (ret)
 		goto error_ctrls;
 
-- 
2.33.1

Re: [PATCH] media: i2c: ov5648: Fix lockdep error

[Paul Kocialkowski]

[-- Attachment #1: Type: text/plain, Size: 1551 bytes --]

Hi Hans,

On Thu 25 Nov 21, 21:31, Hans de Goede wrote:
> ov5648_state_init() calls ov5648_state_mipi_configure() which uses
> __v4l2_ctrl_s_ctrl[_int64](). This means that sensor->mutex (which
> is also sensor->ctrls.handler.lock) must be locked before calling
> ov5648_state_init().
> 
> ov5648_state_mipi_configure() is also used in other places where
> the lock is already held so it cannot be changed itself.
> 
> Note this is based on an identical (tested) fix for the ov8865 driver,
> this has only been compile-tested.

Good catch, thanks!

Though I think it would look slightly better to have the lock in the
ov5648_state_init function itself rather than in the probe.
I guess the same applies to the ov8865 fix, if it's not too late.

Cheers,

Paul

> Cc: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/media/i2c/ov5648.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/media/i2c/ov5648.c b/drivers/media/i2c/ov5648.c
> index 947d437ed0ef..9f24bdccf50f 100644
> --- a/drivers/media/i2c/ov5648.c
> +++ b/drivers/media/i2c/ov5648.c
> @@ -2546,7 +2546,9 @@ static int ov5648_probe(struct i2c_client *client)
>  	if (ret)
>  		goto error_mutex;
>  
> +	mutex_lock(&sensor->mutex);
>  	ret = ov5648_state_init(sensor);
> +	mutex_unlock(&sensor->mutex);
>  	if (ret)
>  		goto error_ctrls;
>  
> -- 
> 2.33.1
> 

-- 
Paul Kocialkowski, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] media: i2c: ov5648: Fix lockdep error

[Hans de Goede]

Hi,

On 11/26/21 11:08, Paul Kocialkowski wrote:
> Hi Hans,
> 
> On Thu 25 Nov 21, 21:31, Hans de Goede wrote:
>> ov5648_state_init() calls ov5648_state_mipi_configure() which uses
>> __v4l2_ctrl_s_ctrl[_int64](). This means that sensor->mutex (which
>> is also sensor->ctrls.handler.lock) must be locked before calling
>> ov5648_state_init().
>>
>> ov5648_state_mipi_configure() is also used in other places where
>> the lock is already held so it cannot be changed itself.
>>
>> Note this is based on an identical (tested) fix for the ov8865 driver,
>> this has only been compile-tested.
> 
> Good catch, thanks!
> 
> Though I think it would look slightly better to have the lock in the
> ov5648_state_init function itself rather than in the probe.

Ack, that makes sense. I will send a v2 with this change.

Regards,

Hans



>> Cc: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>>  drivers/media/i2c/ov5648.c | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/drivers/media/i2c/ov5648.c b/drivers/media/i2c/ov5648.c
>> index 947d437ed0ef..9f24bdccf50f 100644
>> --- a/drivers/media/i2c/ov5648.c
>> +++ b/drivers/media/i2c/ov5648.c
>> @@ -2546,7 +2546,9 @@ static int ov5648_probe(struct i2c_client *client)
>>  	if (ret)
>>  		goto error_mutex;
>>  
>> +	mutex_lock(&sensor->mutex);
>>  	ret = ov5648_state_init(sensor);
>> +	mutex_unlock(&sensor->mutex);
>>  	if (ret)
>>  		goto error_ctrls;
>>  
>> -- 
>> 2.33.1
>>
>