[PATCH] ASoC: rt5670: Remove unbalanced pm_runtime_put()

[PATCH] ASoC: rt5670: Remove unbalanced pm_runtime_put()

[Hans de Goede]

For some reason rt5670_i2c_probe() does a pm_runtime_put() at the end
of a successful probe. But it has never done a pm_runtime_get() leading
to the following error being logged into dmesg:

 rt5670 i2c-10EC5640:00: Runtime PM usage count underflow!

Fix this by removing the unnecessary pm_runtime_put().

Fixes: 64e89e5f5548 ("ASoC: rt5670: Add runtime PM support")
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 sound/soc/codecs/rt5670.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/sound/soc/codecs/rt5670.c b/sound/soc/codecs/rt5670.c
index ebac6caeb40a..a230f441559a 100644
--- a/sound/soc/codecs/rt5670.c
+++ b/sound/soc/codecs/rt5670.c
@@ -3311,8 +3311,6 @@ static int rt5670_i2c_probe(struct i2c_client *i2c)
 	if (ret < 0)
 		goto err;
 
-	pm_runtime_put(&i2c->dev);
-
 	return 0;
 err:
 	pm_runtime_disable(&i2c->dev);
-- 
2.38.1

Re: [PATCH] ASoC: rt5670: Remove unbalanced pm_runtime_put()

[Cezary Rojewski]

On 2022-12-13 1:33 PM, Hans de Goede wrote:
> For some reason rt5670_i2c_probe() does a pm_runtime_put() at the end
> of a successful probe. But it has never done a pm_runtime_get() leading
> to the following error being logged into dmesg:
> 
>   rt5670 i2c-10EC5640:00: Runtime PM usage count underflow!
> 
> Fix this by removing the unnecessary pm_runtime_put().
> 
> Fixes: 64e89e5f5548 ("ASoC: rt5670: Add runtime PM support")
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>   sound/soc/codecs/rt5670.c | 2 --
>   1 file changed, 2 deletions(-)
> 
> diff --git a/sound/soc/codecs/rt5670.c b/sound/soc/codecs/rt5670.c
> index ebac6caeb40a..a230f441559a 100644
> --- a/sound/soc/codecs/rt5670.c
> +++ b/sound/soc/codecs/rt5670.c
> @@ -3311,8 +3311,6 @@ static int rt5670_i2c_probe(struct i2c_client *i2c)
>   	if (ret < 0)
>   		goto err;
>   
> -	pm_runtime_put(&i2c->dev);
> -
>   	return 0;
>   err:
>   	pm_runtime_disable(&i2c->dev);


Hello Hans,

Good finding, weird one though. I just analyzed commit pointed by 
64e89e5f5548 and it seems that entire change could be reverted. The 
rt5670 i2c_driver does not assign any PM ops, only ASoC component ones. 
The later is tied to machine board driver though (and it assigning 
snd_soc_pm_ops in its descriptor).

 From i2s (non-sdw) Realtek codec drivers found in sound/soc/codecs it 
seems that only rt9120.c defines PM ops and configures the PM for the 
device. OTOH, there are several which define suspend/resume on ASoC 
component level and do not touch pm_runtime_* at all.

Thus, voting for a complete revert.


Regards,
Czarek

Re: [PATCH] ASoC: rt5670: Remove unbalanced pm_runtime_put()

[Hans de Goede]

Hi Cezary,

On 12/13/22 13:55, Cezary Rojewski wrote:
> On 2022-12-13 1:33 PM, Hans de Goede wrote:
>> For some reason rt5670_i2c_probe() does a pm_runtime_put() at the end
>> of a successful probe. But it has never done a pm_runtime_get() leading
>> to the following error being logged into dmesg:
>>
>>   rt5670 i2c-10EC5640:00: Runtime PM usage count underflow!
>>
>> Fix this by removing the unnecessary pm_runtime_put().
>>
>> Fixes: 64e89e5f5548 ("ASoC: rt5670: Add runtime PM support")
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>>   sound/soc/codecs/rt5670.c | 2 --
>>   1 file changed, 2 deletions(-)
>>
>> diff --git a/sound/soc/codecs/rt5670.c b/sound/soc/codecs/rt5670.c
>> index ebac6caeb40a..a230f441559a 100644
>> --- a/sound/soc/codecs/rt5670.c
>> +++ b/sound/soc/codecs/rt5670.c
>> @@ -3311,8 +3311,6 @@ static int rt5670_i2c_probe(struct i2c_client *i2c)
>>       if (ret < 0)
>>           goto err;
>>   -    pm_runtime_put(&i2c->dev);
>> -
>>       return 0;
>>   err:
>>       pm_runtime_disable(&i2c->dev);
> 
> 
> Hello Hans,
> 
> Good finding, weird one though. I just analyzed commit pointed by 64e89e5f5548 and it seems that entire change could be reverted. The rt5670 i2c_driver does not assign any PM ops, only ASoC component ones. The later is tied to machine board driver though (and it assigning snd_soc_pm_ops in its descriptor).
> 
> From i2s (non-sdw) Realtek codec drivers found in sound/soc/codecs it seems that only rt9120.c defines PM ops and configures the PM for the device. OTOH, there are several which define suspend/resume on ASoC component level and do not touch pm_runtime_* at all.
> 
> Thus, voting for a complete revert.

A complete revert seems fine to me too.

Regards,

Hans

Re: [PATCH] ASoC: rt5670: Remove unbalanced pm_runtime_put()

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 587 bytes --]

On Tue, Dec 13, 2022 at 01:55:50PM +0100, Cezary Rojewski wrote:

> From i2s (non-sdw) Realtek codec drivers found in sound/soc/codecs it seems
> that only rt9120.c defines PM ops and configures the PM for the device.
> OTOH, there are several which define suspend/resume on ASoC component level

> Thus, voting for a complete revert.

Note that with ACPI you can have runtime PM things happening at the ACPI
level so even if a driver does nothing when suspended there can be some
benefit from using runtime PM.  No idea if this applies to any systems
using these devices or not though.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] ASoC: rt5670: Remove unbalanced pm_runtime_put()

[Hans de Goede]

Hi,

On 12/13/22 14:03, Mark Brown wrote:
> On Tue, Dec 13, 2022 at 01:55:50PM +0100, Cezary Rojewski wrote:
> 
>> From i2s (non-sdw) Realtek codec drivers found in sound/soc/codecs it seems
>> that only rt9120.c defines PM ops and configures the PM for the device.
>> OTOH, there are several which define suspend/resume on ASoC component level
> 
>> Thus, voting for a complete revert.
> 
> Note that with ACPI you can have runtime PM things happening at the ACPI
> level so even if a driver does nothing when suspended there can be some
> benefit from using runtime PM.  No idea if this applies to any systems
> using these devices or not though.

Yes I was thinking the same thing, I said in my other email that I was fine
with a revert because in my experience the codec ACPI devices don't have
a _PS0 / _PS3 method.

But to make sure I just checked and I noticed this is not entirely true.

E.g. on the ThinkPad 10 tablet there is:

                Method (_PS3, 0, NotSerialized)  // _PS3: Power State 3
                {
                    CKC3 = Zero
                }

                Method (_PS0, 0, NotSerialized)  // _PS0: Power State 0
                {
                    CKC3 = One
                }


However the "board" driver already directly controls this clock, including
switching to an internal RC oscillator for jack detection when idle, see:

sound/soc/intel/boards/cht_bsw_rt5672.c:

	ret = clk_prepare_enable(ctx->mclk);

	clk_disable_unprepare(ctx->mclk);

	drv->mclk = devm_clk_get(&pdev->dev, "pmc_plt_clk_3");


So also having runtime-pm muck with this is not really useful
and I guess might actually cause problems in some cases (at least
it is one more thing which can go wrong).

Note _PS0 and _PS3 will still run on system suspend/resume
but by then we should have already powered-down the codec,
including calling clk_disable_unprepare(ctx->mclk); .

TL;DR: I'm still fine with reverting the original commit.

Regards,

Hans

Re: [PATCH] ASoC: rt5670: Remove unbalanced pm_runtime_put()

[Mark Brown]

On Tue, 13 Dec 2022 13:33:19 +0100, Hans de Goede wrote:
> For some reason rt5670_i2c_probe() does a pm_runtime_put() at the end
> of a successful probe. But it has never done a pm_runtime_get() leading
> to the following error being logged into dmesg:
> 
>  rt5670 i2c-10EC5640:00: Runtime PM usage count underflow!
> 
> Fix this by removing the unnecessary pm_runtime_put().
> 
> [...]

Applied to

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next

Thanks!

[1/1] ASoC: rt5670: Remove unbalanced pm_runtime_put()
      commit: 6c900dcc3f7331a67ed29739d74524e428d137fb

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark