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?
