Hi,
On 6/8/23 14:48, Laurent Pinchart wrote:
> Hi Sakari,
>
> On Thu, Jun 08, 2023 at 12:44:51PM +0000, Sakari Ailus wrote:
>> On Wed, Jun 07, 2023 at 06:46:49PM +0200, Hans de Goede wrote:
>>> On ov2680_set_fmt() calls with format->which == V4L2_SUBDEV_FORMAT_TRY,
>>> ov2680_set_fmt() does not talk to the sensor.
>>>
>>> So in this case there is no need to lock the sensor->lock mutex or
>>> to check that the sensor is streaming.
>>>
>>> Fixes: 3ee47cad3e69 ("media: ov2680: Add Omnivision OV2680 sensor driver")
>>> Signed-off-by: Hans de Goede <hdegoede@xxxxxxxxxx>
>>> ---
>>> drivers/media/i2c/ov2680.c | 20 +++++++++-----------
>>> 1 file changed, 9 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/drivers/media/i2c/ov2680.c b/drivers/media/i2c/ov2680.c
>>> index d90bbca6e913..a26a6f18f4f1 100644
>>> --- a/drivers/media/i2c/ov2680.c
>>> +++ b/drivers/media/i2c/ov2680.c
>>> @@ -592,24 +592,22 @@ static int ov2680_set_fmt(struct v4l2_subdev *sd,
>>> if (format->pad != 0)
>>> return -EINVAL;
>>>
>>> - mutex_lock(&sensor->lock);
>>> -
>>> - if (sensor->is_streaming) {
>>> - ret = -EBUSY;
>>> - goto unlock;
>>> - }
>>> -
>>> mode = v4l2_find_nearest_size(ov2680_mode_data,
>>> ARRAY_SIZE(ov2680_mode_data), width,
>>> height, fmt->width, fmt->height);
>>> - if (!mode) {
>>> - ret = -EINVAL;
>>> - goto unlock;
>>> - }
>>> + if (!mode)
>>> + return -EINVAL;
>>>
>>> if (format->which == V4L2_SUBDEV_FORMAT_TRY) {
>>> try_fmt = v4l2_subdev_get_try_format(sd, sd_state, 0);
>>> format->format = *try_fmt;
>>
>> Access to sd_state needs to be serialised, so mutex should be held here.
>
> This operation should be called with the state lock held already, so I
> don't think any extra locking is needed.
Right sd_state is allocated per v4l2-subdev fd so I would expect
the caller to take care of any necessary locking here.
Regards,
Hans
>
>>> + return 0;
>>> + }
>>> +
>>> + mutex_lock(&sensor->lock);
>>> +
>>> + if (sensor->is_streaming) {
>>> + ret = -EBUSY;
>>> goto unlock;
>>> }
>>>
>
