Re: [RFC PATCH 17/40] soundwire: bus: use runtime_pm_get_sync/pm when enabled

Re: [RFC PATCH 17/40] soundwire: bus: use runtime_pm_get_sync/pm when enabled

[Pierre-Louis Bossart]

This thread became unreadable with interleaved top-posting, allow me 
restate the options and ask PM folks what they think

On 7/25/19 6:40 PM, Pierre-Louis Bossart wrote:
> Not all platforms support runtime_pm for now, let's use runtime_pm
> only when enabled.
> 
> Suggested-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> ---
>   drivers/soundwire/bus.c | 25 ++++++++++++++++---------
>   1 file changed, 16 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/soundwire/bus.c b/drivers/soundwire/bus.c
> index 5ad4109dc72f..0a45dc5713df 100644
> --- a/drivers/soundwire/bus.c
> +++ b/drivers/soundwire/bus.c
> @@ -332,12 +332,16 @@ int sdw_nread(struct sdw_slave *slave, u32 addr, size_t count, u8 *val)
>   	if (ret < 0)
>   		return ret;
>   
> -	ret = pm_runtime_get_sync(slave->bus->dev);
> -	if (ret < 0)
> -		return ret;
> +	if (pm_runtime_enabled(slave->bus->dev)) {
> +		ret = pm_runtime_get_sync(slave->bus->dev);
> +		if (ret < 0)
> +			return ret;
> +	}
>   
>   	ret = sdw_transfer(slave->bus, &msg);
> -	pm_runtime_put(slave->bus->dev);
> +
> +	if (pm_runtime_enabled(slave->bus->dev))
> +		pm_runtime_put(slave->bus->dev);

This is option1: we explicitly test if pm_runtime is enabled before 
calling _get_sync() and _put()

option2 (suggested by Jan Kotas): catch the -EACCESS error code

  	ret = pm_runtime_get_sync(slave->bus->dev);
-	if (ret < 0)
+	if (ret < 0 && ret != -EACCES)
  		return ret;

option3: ignore the return value as done in quite a few drivers

Are there any other options? I am personally surprised this is not 
handled in the pm_runtime core, not sure why users need to test for this?

Thanks
Pierre

Re: [alsa-devel] [RFC PATCH 17/40] soundwire: bus: use runtime_pm_get_sync/pm when enabled

[Guennadi Liakhovetski]

On Fri, Jul 26, 2019 at 01:08:57PM -0500, Pierre-Louis Bossart wrote:
> This thread became unreadable with interleaved top-posting, allow me restate
> the options and ask PM folks what they think
> 
> On 7/25/19 6:40 PM, Pierre-Louis Bossart wrote:
> > Not all platforms support runtime_pm for now, let's use runtime_pm
> > only when enabled.
> > 
> > Suggested-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
> > Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> > ---
> >   drivers/soundwire/bus.c | 25 ++++++++++++++++---------
> >   1 file changed, 16 insertions(+), 9 deletions(-)
> > 
> > diff --git a/drivers/soundwire/bus.c b/drivers/soundwire/bus.c
> > index 5ad4109dc72f..0a45dc5713df 100644
> > --- a/drivers/soundwire/bus.c
> > +++ b/drivers/soundwire/bus.c
> > @@ -332,12 +332,16 @@ int sdw_nread(struct sdw_slave *slave, u32 addr, size_t count, u8 *val)
> >   	if (ret < 0)
> >   		return ret;
> > -	ret = pm_runtime_get_sync(slave->bus->dev);
> > -	if (ret < 0)
> > -		return ret;
> > +	if (pm_runtime_enabled(slave->bus->dev)) {
> > +		ret = pm_runtime_get_sync(slave->bus->dev);
> > +		if (ret < 0)
> > +			return ret;
> > +	}
> >   	ret = sdw_transfer(slave->bus, &msg);
> > -	pm_runtime_put(slave->bus->dev);
> > +
> > +	if (pm_runtime_enabled(slave->bus->dev))
> > +		pm_runtime_put(slave->bus->dev);
> 
> This is option1: we explicitly test if pm_runtime is enabled before calling
> _get_sync() and _put()
> 
> option2 (suggested by Jan Kotas): catch the -EACCESS error code
> 
>  	ret = pm_runtime_get_sync(slave->bus->dev);
> -	if (ret < 0)
> +	if (ret < 0 && ret != -EACCES)
>  		return ret;
> 
> option3: ignore the return value as done in quite a few drivers
> 
> Are there any other options? I am personally surprised this is not handled
> in the pm_runtime core, not sure why users need to test for this?

option 4: fix this in runtime PM :-) This seems like the best option to me,
but probably not the easiest one. Otherwise I'd go with (2), I think, since
that's also the official purpose of the -EACCESS return code:

https://lists.linuxfoundation.org/pipermail/linux-pm/2011-June/031930.html

Thanks
Guennadi

Re: [alsa-devel] [RFC PATCH 17/40] soundwire: bus: use runtime_pm_get_sync/pm when enabled

[Andy Shevchenko]

On Fri, Jul 26, 2019 at 08:25:35PM +0200, Guennadi Liakhovetski wrote:
> On Fri, Jul 26, 2019 at 01:08:57PM -0500, Pierre-Louis Bossart wrote:
> > On 7/25/19 6:40 PM, Pierre-Louis Bossart wrote:
> > > Not all platforms support runtime_pm for now, let's use runtime_pm
> > > only when enabled.

> > option2 (suggested by Jan Kotas): catch the -EACCESS error code
> > 
> >  	ret = pm_runtime_get_sync(slave->bus->dev);
> > -	if (ret < 0)
> > +	if (ret < 0 && ret != -EACCES)
> >  		return ret;

> Otherwise I'd go with (2), I think, since
> that's also the official purpose of the -EACCESS return code:
> 
> https://lists.linuxfoundation.org/pipermail/linux-pm/2011-June/031930.html

And at least we have examples in the kernel

drivers/gpu/drm/radeon/radeon_fb.c:57:  if (ret < 0 && ret != -EACCES) {

-- 
With Best Regards,
Andy Shevchenko

Re: [RFC PATCH 17/40] soundwire: bus: use runtime_pm_get_sync/pm when enabled

[Andy Shevchenko]

On Fri, Jul 26, 2019 at 01:08:57PM -0500, Pierre-Louis Bossart wrote:
> This thread became unreadable with interleaved top-posting, allow me restate
> the options and ask PM folks what they think
> 
> On 7/25/19 6:40 PM, Pierre-Louis Bossart wrote:
> > Not all platforms support runtime_pm for now, let's use runtime_pm
> > only when enabled.

Just a side note below...

> > -	ret = pm_runtime_get_sync(slave->bus->dev);
> > -	if (ret < 0)

Here...

> > -		return ret;
> > +	if (pm_runtime_enabled(slave->bus->dev)) {
> > +		ret = pm_runtime_get_sync(slave->bus->dev);
> > +		if (ret < 0)

...and thus here...

> > +			return ret;
> > +	}
> >   	ret = sdw_transfer(slave->bus, &msg);
> > -	pm_runtime_put(slave->bus->dev);
> > +
> > +	if (pm_runtime_enabled(slave->bus->dev))
> > +		pm_runtime_put(slave->bus->dev);
> 
> This is option1: we explicitly test if pm_runtime is enabled before calling
> _get_sync() and _put()
> 
> option2 (suggested by Jan Kotas): catch the -EACCESS error code
> 
>  	ret = pm_runtime_get_sync(slave->bus->dev);
> -	if (ret < 0)
> +	if (ret < 0 && ret != -EACCES)

...and here, the pm_runtime_put_noidle() call is missed.

>  		return ret;
> 
> option3: ignore the return value as done in quite a few drivers
> 
> Are there any other options? I am personally surprised this is not handled
> in the pm_runtime core, not sure why users need to test for this?

-- 
With Best Regards,
Andy Shevchenko

Re: [alsa-devel] [RFC PATCH 17/40] soundwire: bus: use runtime_pm_get_sync/pm when enabled

[Pierre-Louis Bossart]

On 7/26/19 2:08 PM, Andy Shevchenko wrote:
> On Fri, Jul 26, 2019 at 01:08:57PM -0500, Pierre-Louis Bossart wrote:
>> This thread became unreadable with interleaved top-posting, allow me restate
>> the options and ask PM folks what they think
>>
>> On 7/25/19 6:40 PM, Pierre-Louis Bossart wrote:
>>> Not all platforms support runtime_pm for now, let's use runtime_pm
>>> only when enabled.
> 
> Just a side note below...
> 
>>> -	ret = pm_runtime_get_sync(slave->bus->dev);
>>> -	if (ret < 0)
> 
> Here...
> 
>>> -		return ret;
>>> +	if (pm_runtime_enabled(slave->bus->dev)) {
>>> +		ret = pm_runtime_get_sync(slave->bus->dev);
>>> +		if (ret < 0)
> 
> ...and thus here...
> 
>>> +			return ret;
>>> +	}
>>>    	ret = sdw_transfer(slave->bus, &msg);
>>> -	pm_runtime_put(slave->bus->dev);
>>> +
>>> +	if (pm_runtime_enabled(slave->bus->dev))
>>> +		pm_runtime_put(slave->bus->dev);
>>
>> This is option1: we explicitly test if pm_runtime is enabled before calling
>> _get_sync() and _put()
>>
>> option2 (suggested by Jan Kotas): catch the -EACCESS error code
>>
>>   	ret = pm_runtime_get_sync(slave->bus->dev);
>> -	if (ret < 0)
>> +	if (ret < 0 && ret != -EACCES)
> 
> ...and here, the pm_runtime_put_noidle() call is missed.

yes but in the example you provided, they actually do more work than 
just decrement the device usage counter:

static int
radeonfb_open(struct fb_info *info, int user)
{
	struct radeon_fbdev *rfbdev = info->par;
	struct radeon_device *rdev = rfbdev->rdev;
	int ret = pm_runtime_get_sync(rdev->ddev->dev);
	if (ret < 0 && ret != -EACCES) {
		pm_runtime_mark_last_busy(rdev->ddev->dev);
		pm_runtime_put_autosuspend(rdev->ddev->dev);
		return ret;
	}
	return 0;
}

unless I am missing something pm_runtime_put_noidle() and 
_put_autosuspend() are not equivalent, are they?

Re: [alsa-devel] [RFC PATCH 17/40] soundwire: bus: use runtime_pm_get_sync/pm when enabled

[Andy Shevchenko]

On Mon, Jul 29, 2019 at 05:07:39PM -0500, Pierre-Louis Bossart wrote:
> On 7/26/19 2:08 PM, Andy Shevchenko wrote:
> > On Fri, Jul 26, 2019 at 01:08:57PM -0500, Pierre-Louis Bossart wrote:

> > > -	if (ret < 0)
> > > +	if (ret < 0 && ret != -EACCES)
> > 
> > ...and here, the pm_runtime_put_noidle() call is missed.
> 
> yes but in the example you provided, they actually do more work than just
> decrement the device usage counter:

In their case they would like to do that. You decide what is appropriate call
in your case.

My point is, that reference counter in case of error handling should be
returned back to its value.

-- 
With Best Regards,
Andy Shevchenko

Re: [alsa-devel] [RFC PATCH 17/40] soundwire: bus: use runtime_pm_get_sync/pm when enabled

[Pierre-Louis Bossart]

On 7/30/19 6:21 AM, Andy Shevchenko wrote:
> On Mon, Jul 29, 2019 at 05:07:39PM -0500, Pierre-Louis Bossart wrote:
>> On 7/26/19 2:08 PM, Andy Shevchenko wrote:
>>> On Fri, Jul 26, 2019 at 01:08:57PM -0500, Pierre-Louis Bossart wrote:
> 
>>>> -	if (ret < 0)
>>>> +	if (ret < 0 && ret != -EACCES)
>>>
>>> ...and here, the pm_runtime_put_noidle() call is missed.
>>
>> yes but in the example you provided, they actually do more work than just
>> decrement the device usage counter:
> 
> In their case they would like to do that. You decide what is appropriate call
> in your case.
> 
> My point is, that reference counter in case of error handling should be
> returned back to its value.

Agree on the reference count.
I am however not clear on the next step and 'what is appropriate'.

If pm_runtime_get_sync() failed, then technically the device was not 
resumed so marking it as last_busy+autosuspend, or using a plain vanilla 
put() will not result in any action. I must be missing something here.

Re: [alsa-devel] [RFC PATCH 17/40] soundwire: bus: use runtime_pm_get_sync/pm when enabled

[Andy Shevchenko]

On Tue, Jul 30, 2019 at 07:57:46AM -0500, Pierre-Louis Bossart wrote:
> On 7/30/19 6:21 AM, Andy Shevchenko wrote:
> > On Mon, Jul 29, 2019 at 05:07:39PM -0500, Pierre-Louis Bossart wrote:
> > > On 7/26/19 2:08 PM, Andy Shevchenko wrote:
> > > > On Fri, Jul 26, 2019 at 01:08:57PM -0500, Pierre-Louis Bossart wrote:
> > 
> > > > > -	if (ret < 0)
> > > > > +	if (ret < 0 && ret != -EACCES)
> > > > 
> > > > ...and here, the pm_runtime_put_noidle() call is missed.
> > > 
> > > yes but in the example you provided, they actually do more work than just
> > > decrement the device usage counter:
> > 
> > In their case they would like to do that. You decide what is appropriate call
> > in your case.
> > 
> > My point is, that reference counter in case of error handling should be
> > returned back to its value.
> 
> Agree on the reference count.
> I am however not clear on the next step and 'what is appropriate'.
> 
> If pm_runtime_get_sync() failed, then technically the device was not resumed

Not so straight. It depends on reference count. It might be true (most cases
I think), or not true, if device had been resumed previously by other call.

> so marking it as last_busy+autosuspend, or using a plain vanilla put() will
> not result in any action. I must be missing something here.

put_noidle(). Because if it failed on the first call and was resumed, put()
will try to shut it down (since reference count goes to no-user base).

-- 
With Best Regards,
Andy Shevchenko

Re: [alsa-devel] [RFC PATCH 17/40] soundwire: bus: use runtime_pm_get_sync/pm when enabled

[Andy Shevchenko]

On Tue, Jul 30, 2019 at 06:58:09PM +0300, Andy Shevchenko wrote:
> On Tue, Jul 30, 2019 at 07:57:46AM -0500, Pierre-Louis Bossart wrote:
> > On 7/30/19 6:21 AM, Andy Shevchenko wrote:
> > > On Mon, Jul 29, 2019 at 05:07:39PM -0500, Pierre-Louis Bossart wrote:
> > > > On 7/26/19 2:08 PM, Andy Shevchenko wrote:

> > so marking it as last_busy+autosuspend, or using a plain vanilla put() will
> > not result in any action. I must be missing something here.
> 
> put_noidle(). Because if it failed on the first call and was resumed, put()
> will try to shut it down (since reference count goes to no-user base).

Sorry, this has to be read as (was -> wasn't)

> put_noidle(). Because if it failed on the first call and wasn't resumed, put()
> will try to shut it down (since reference count goes to no-user base).

-- 
With Best Regards,
Andy Shevchenko