Re: [PATCH v2 5/7] media: ov8858: Use pm_runtime_get_if_active(), put usage_count correctly

[Sakari Ailus <sakari.ailus@linux.intel.com>]

Hi Laurent,

On Sat, Nov 18, 2023 at 07:33:15PM +0200, Laurent Pinchart wrote:
> Hi Sakari
> 
> On Sat, Nov 18, 2023 at 11:12:41AM +0000, Sakari Ailus wrote:
> > On Fri, Nov 17, 2023 at 04:30:15PM +0100, Jacopo Mondi wrote:
> > > On Fri, Nov 17, 2023 at 01:14:31PM +0200, Sakari Ailus wrote:
> > > > Use pm_runtime_get_if_active() to get the device's runtime PM usage_count
> > > > and set controls, then use runtime PM autosuspend once the controls have
> > > > been set (instead of likely transitioning to suspended state immediately).
> > > >
> > > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > > > ---
> > > >  drivers/media/i2c/ov8858.c | 8 +++++---
> > > >  1 file changed, 5 insertions(+), 3 deletions(-)
> > > >
> > > > diff --git a/drivers/media/i2c/ov8858.c b/drivers/media/i2c/ov8858.c
> > > > index 3af6125a2eee..a99b91700a8d 100644
> > > > --- a/drivers/media/i2c/ov8858.c
> > > > +++ b/drivers/media/i2c/ov8858.c
> > > > @@ -1538,7 +1538,7 @@ static int ov8858_set_ctrl(struct v4l2_ctrl *ctrl)
> > > >  	struct v4l2_subdev_state *state;
> > > >  	u16 digi_gain;
> > > >  	s64 max_exp;
> > > > -	int ret;
> > > > +	int ret, pm_status;
> > > >
> > > >  	/*
> > > >  	 * The control handler and the subdev state use the same mutex and the
> > > > @@ -1561,7 +1561,8 @@ static int ov8858_set_ctrl(struct v4l2_ctrl *ctrl)
> > > >  		break;
> > > >  	}
> > > >
> > > > -	if (!pm_runtime_get_if_in_use(&client->dev))
> > > > +	pm_status = pm_runtime_get_if_active(&client->dev);
> > > > +	if (!pm_status)
> > > >  		return 0;
> > > >
> > > >  	switch (ctrl->id) {
> > > > @@ -1601,7 +1602,8 @@ static int ov8858_set_ctrl(struct v4l2_ctrl *ctrl)
> > > >  		break;
> > > >  	}
> > > >
> > > > -	pm_runtime_put(&client->dev);
> > > > +	if (pm_status > 0)
> > > 
> > > I'm not 100% sure I get this bit.
> > > 
> > > If we get here it means pm_status is either -EINVAL or > 0, otherwise
> > > we would have exited earlier.
> > > 
> > > What's the point of checking for > 0 here ?
> > > 
> > > There are two cases where pm_status is -EINVAL, either !CONFIG_PM and
> > > the the below call is a nop, or if pm_runtime has not been enabled by
> > > the driver, which means the driver doesn't use pm_runtime at all.
> > > 
> > > Are there other cases I have missed that require checking here for
> > > pm_status > 0 ?
> > 
> > Other than Runtime PM being disabled, I don't think that should happen.
> > 
> > So as such this patch does not fix a bug. I just prefer to be extra
> > cautious when it comes to use counts.
> 
> What happened to the old motto of "if it's not broken, don't fix it" ?

People tend to take patterns used in drivers and this indeed works here,
but only when taken with the rest of the Runtime PM API usage.

> :-) I like how this series (slightly) simplifies the runtime PM API by
> giving pm_runtime_get_if_active() the right behaviour for the most
> common use cases. Let's continue in that direction, and evolve the API
> to simplify driver, not render them more complex.
> 
> I would prefer refactoring this series to first switch drivers to
> pm_runtime_get_if_active(), and then use autosuspend at the end of the
> .s_ctrl() handler. That can be two patches, each modifying all relevant
> sensor driver.

I think it should be fine to do both in the same patch indeed.

-- 
Regards,

Sakari Ailus