[PATCH] ASoC: rt5670: Remove unbalanced pm_runtime_put()
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);
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
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
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.
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
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
participants (3)
-
Cezary Rojewski -
Hans de Goede -
Mark Brown