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.
--
Kind regards,
Sakari Ailus
