Tomasz and Sakari, thanks.
On 11/18/20 9:46 PM, Sakari Ailus wrote:
> Hi Tomasz,
>
> On Wed, Nov 18, 2020 at 09:41:11PM +0900, Tomasz Figa wrote:
>> On Tue, Nov 17, 2020 at 5:37 PM Sergey Senozhatsky
>> <sergey.senozhatsky@xxxxxxxxx> wrote:
>>>
>>> On (20/11/17 17:07), Sergey Senozhatsky wrote:
>>>>> +static int ov9734_set_stream(struct v4l2_subdev *sd, int enable)
>>>>> +{
>>>>> + struct ov9734 *ov9734 = to_ov9734(sd);
>>>>> + struct i2c_client *client = v4l2_get_subdevdata(sd);
>>>>> + int ret = 0;
>>>>> +
>>>>> + if (ov9734->streaming == enable)
>>>>> + return 0;
>>>>> +
>>>>> + mutex_lock(&ov9734->mutex);
>>>>> + if (enable) {
>>>>> + ret = pm_runtime_get_sync(&client->dev);
>>>>> + if (ret < 0) {
>>>>> + pm_runtime_put_noidle(&client->dev);
>>>>> + mutex_unlock(&ov9734->mutex);
>>>>> + return ret;
>>>>> + }
>>>>> +
>>>>> + ret = ov9734_start_streaming(ov9734);
>>>>> + if (ret) {
>>>>> + enable = 0;
>>>>> + ov9734_stop_streaming(ov9734);
>>>>> + pm_runtime_put(&client->dev);
>>>>> + }
>>>>> + } else {
>>>>> + ov9734_stop_streaming(ov9734);
>>>>> + pm_runtime_put(&client->dev);
>>>>> + }
>>>>
>>>> I assume that we will never see erroneous ->s_stream(0) calls or
>>>> ->s_stream(0) after unsuccessful ->s_stream(1). Otherwise we will
>>>> do extra pm_runtime_put(), not matched by pm_runtime_get().
>>>
>>> Oh, no. There is (unprotected) if (ov9734->streaming) check
>>> at the very top, so we are probably fine.
>>
>> Hmm, should it be protected?
>
> In principle, yes.
>
> The patch is already in a pull request I've sent to Mauro, so any changes
> should be on top.
Will submit a patch based on the top.
>
--
Best regards,
Bingbu Cao
