PM regression in next

Hi all,

I'm seeing a considerable idle power consumption regression in Linux next, with power consumption for my idle test system going to 17.5mW compared to the usual 8mW on my test device.

Git bisect points to merge commit e130bc1d00a4 ("Merge branch 'akpm-current/current'") being the first bad commit.

I have also verified that commit 70286688e5ad ("ipc/mqueue.c:
have RT tasks queue in by priority in wq_add()") is good, and commit e2d7fe89e8ae ("Merge remote-tracking branch 'init_task/init_task'") is good.

Quoted 9 lines · On Thu, 11 Jan 2018 16:01:13 -0800 Tony Lindgren <[email protected]> wrote:

Hi all,

I'm seeing a considerable idle power consumption regression in Linux next, with power consumption for my idle test system going to 17.5mW compared to the usual 8mW on my test device.

Git bisect points to merge commit e130bc1d00a4 ("Merge branch 'akpm-current/current'") being the first bad commit.

Quoted 4 lines

I have also verified that commit 70286688e5ad ("ipc/mqueue.c:
have RT tasks queue in by priority in wq_add()") is good, and commit e2d7fe89e8ae ("Merge remote-tracking branch 'init_task/init_task'") is good.

Do you mean that everything up to and including 70286688e5ad ("ipc/mqueue.c: have RT tasks queue in by priority in wq_add()") is good?

Quoted 16 lines · Andrew Morton

Quoted 13 lines · On Thu, 11 Jan 2018 16:01:13 -0800 Tony Lindgren <[email protected]> wrote:

Hi all,

I'm seeing a considerable idle power consumption regression in Linux next, with power consumption for my idle test system going to 17.5mW compared to the usual 8mW on my test device.

Git bisect points to merge commit e130bc1d00a4 ("Merge branch 'akpm-current/current'") being the first bad commit.

I have also verified that commit 70286688e5ad ("ipc/mqueue.c:
have RT tasks queue in by priority in wq_add()") is good, and commit e2d7fe89e8ae ("Merge remote-tracking branch 'init_task/init_task'") is good.

Quoted 3 lines

Do you mean that everything up to and including 70286688e5ad ("ipc/mqueue.c: have RT tasks queue in by priority in wq_add()") is good?

Yes I'm not seeing the regression in your branch at commit 70286688e5ad. I'm seeing it only with the merge commit e130bc1d00a4.

Quoted 17 lines · On Thu, 11 Jan 2018 16:23:22 -0800 Tony Lindgren <[email protected]> wrote:

Quoted 16 lines · Andrew Morton

Quoted 13 lines · On Thu, 11 Jan 2018 16:01:13 -0800 Tony Lindgren <[email protected]> wrote:

Hi all,

I'm seeing a considerable idle power consumption regression in Linux next, with power consumption for my idle test system going to 17.5mW compared to the usual 8mW on my test device.

Git bisect points to merge commit e130bc1d00a4 ("Merge branch 'akpm-current/current'") being the first bad commit.

I have also verified that commit 70286688e5ad ("ipc/mqueue.c:
have RT tasks queue in by priority in wq_add()") is good, and commit e2d7fe89e8ae ("Merge remote-tracking branch 'init_task/init_task'") is good.

Quoted 8 lines

Quoted 3 lines

Do you mean that everything up to and including 70286688e5ad ("ipc/mqueue.c: have RT tasks queue in by priority in wq_add()") is good?

Yes I'm not seeing the regression in your branch at commit 70286688e5ad. I'm seeing it only with the merge commit e130bc1d00a4.

That's weird. All I'm seeing between 70286688e5ad and end-of-mm is:

tools-objtool-makefile-dont-assume-sync-checksh-is-executable.patch ipc-mqueue-add-missing-error-code-in-init_mqueue_fs.patch

vfs-remove-might_sleep-from-clear_inode.patch

mm-remove-duplicate-includes.patch

mm-remove-unneeded-kallsyms-include.patch
hrtimer-remove-unneeded-kallsyms-include.patch
genirq-remove-unneeded-kallsyms-include.patch

mm-memblock-memblock_is_map-region_memory-can-be-boolean.patch lib-lockref-__lockref_is_dead-can-be-boolean.patch kernel-cpuset-current_cpuset_is_being_rebound-can-be-boolean.patch kernel-resource-iomem_is_exclusive-can-be-boolean.patch kernel-module-module_is_live-can-be-boolean.patch kernel-mutex-mutex_is_locked-can-be-boolean.patch crash_dump-is_kdump_kernel-can-be-boolean.patch

fix-const-confusion-in-certs-blacklist.patch
fix-read-buffer-overflow-in-delta-ipc.patch

kasan-rework-kconfig-settings.patch

sparc64-ng4-memset-32-bits-overflow.patch

lib-crc-ccitt-add-ccitt-false-crc16-variant.patch

And I don't see how any of those can cause this. Did anything else change, like context switch rates, interrupt rates, etc?

Well I tried to measure suspend power consumption and noticed that system suspend fails too hand hangs the network device:

# echo mem > /sys/power/state
[   32.577850] PM: suspend entry (deep)
[   32.582031] PM: Syncing filesystems ... done.
[   32.598083] Freezing user space processes ... (elapsed 0.002 seconds) done.
[   32.608398] OOM killer disabled.
[   32.611846] Freezing remaining freezable tasks ... (elapsed 0.002 seconds) done.
[   32.622192] Suspending console(s) (use no_console_suspend to debug)
[   32.651123] dpm_run_callback(): mdio_bus_suspend+0x0/0x24 returns 4352
[   32.651428] PM: Device 2c000000.ethernet-ffffffff:01 failed to suspend: error 4352
[   32.653289] PM: Some devices failed to suspend, or early wake event detected
[   32.685455] OOM killer enabled.
[   32.688629] Restarting tasks ... done.
[   32.695983] PM: suspend exit
ash: write error: Bad address

That too works just fine at commit 70286688e5ad.

Quoted 3 lines · Tony Lindgren

Well I tried to measure suspend power consumption and noticed that system suspend fails too hand hangs the network device:

Quoted 9 lines

# echo mem > /sys/power/state
[   32.577850] PM: suspend entry (deep)
[   32.582031] PM: Syncing filesystems ... done.
[   32.598083] Freezing user space processes ... (elapsed 0.002 seconds) done.
[   32.608398] OOM killer disabled.
[   32.611846] Freezing remaining freezable tasks ... (elapsed 0.002 seconds) done.
[   32.622192] Suspending console(s) (use no_console_suspend to debug)
[   32.651123] dpm_run_callback(): mdio_bus_suspend+0x0/0x24 returns 4352
[   32.651428] PM: Device 2c000000.ethernet-ffffffff:01 failed to suspend: error 4352

Quoted 7 lines

[   32.653289] PM: Some devices failed to suspend, or early wake event detected
[   32.685455] OOM killer enabled.
[   32.688629] Restarting tasks ... done.
[   32.695983] PM: suspend exit

ash: write error: Bad address

That too works just fine at commit 70286688e5ad.

Suspend fails at commit e2d7fe89e8ae though, so looks like we have two separate issues. I'll try to bisect that separately.

Quoted 4 lines · CET Tony Lindgren

Quoted 3 lines · Tony Lindgren

Well I tried to measure suspend power consumption and noticed that system suspend fails too hand hangs the network device:

Quoted 9 lines

Quoted 9 lines

# echo mem > /sys/power/state
[   32.577850] PM: suspend entry (deep)
[   32.582031] PM: Syncing filesystems ... done.
[   32.598083] Freezing user space processes ... (elapsed 0.002 seconds) done.
[   32.608398] OOM killer disabled.
[   32.611846] Freezing remaining freezable tasks ... (elapsed 0.002 seconds) done.
[   32.622192] Suspending console(s) (use no_console_suspend to debug)
[   32.651123] dpm_run_callback(): mdio_bus_suspend+0x0/0x24 returns 4352
[   32.651428] PM: Device 2c000000.ethernet-ffffffff:01 failed to suspend: error 4352

This looks totally bogus.

First, "error" should be a negative number and we print it as int.

Second, error codes are not in this range anyway.

Quoted 10 lines

Quoted 7 lines

[   32.653289] PM: Some devices failed to suspend, or early wake event detected
[   32.685455] OOM killer enabled.
[   32.688629] Restarting tasks ... done.
[   32.695983] PM: suspend exit

ash: write error: Bad address

That too works just fine at commit 70286688e5ad.

Suspend fails at commit e2d7fe89e8ae though, so looks like we have two separate issues. I'll try to bisect that separately.

Quoted 27 lines · CET Rafael J. Wysocki

Quoted 4 lines · CET Tony Lindgren

Quoted 3 lines · Tony Lindgren

Well I tried to measure suspend power consumption and noticed that system suspend fails too hand hangs the network device:

Quoted 9 lines

Quoted 9 lines

# echo mem > /sys/power/state
[   32.577850] PM: suspend entry (deep)
[   32.582031] PM: Syncing filesystems ... done.
[   32.598083] Freezing user space processes ... (elapsed 0.002 seconds) done.
[   32.608398] OOM killer disabled.
[   32.611846] Freezing remaining freezable tasks ... (elapsed 0.002 seconds) done.
[   32.622192] Suspending console(s) (use no_console_suspend to debug)
[   32.651123] dpm_run_callback(): mdio_bus_suspend+0x0/0x24 returns 4352
[   32.651428] PM: Device 2c000000.ethernet-ffffffff:01 failed to suspend: error 4352

This looks totally bogus.

First, "error" should be a negative number and we print it as int.

Second, error codes are not in this range anyway.

Quoted 6 lines

Quoted 6 lines

[   32.653289] PM: Some devices failed to suspend, or early wake event detected
[   32.685455] OOM killer enabled.
[   32.688629] Restarting tasks ... done.
[   32.695983] PM: suspend exit

ash: write error: Bad address

Quoted 4 lines

Quoted 4 lines

Quoted 1 line

That too works just fine at commit 70286688e5ad.

Suspend fails at commit e2d7fe89e8ae though, so looks like we have two separate issues. I'll try to bisect that separately.

I guess what may happen is that something started to return positive numbers which confuse things all over when passed along by its callers as error codes.

Quoted 30 lines · Rafael J. Wysocki

Quoted 29 lines · CET Rafael J. Wysocki

Quoted 4 lines · CET Tony Lindgren

Quoted 3 lines · Tony Lindgren

Well I tried to measure suspend power consumption and noticed that system suspend fails too hand hangs the network device:

Quoted 9 lines

Quoted 9 lines

# echo mem > /sys/power/state
[   32.577850] PM: suspend entry (deep)
[   32.582031] PM: Syncing filesystems ... done.
[   32.598083] Freezing user space processes ... (elapsed 0.002 seconds) done.
[   32.608398] OOM killer disabled.
[   32.611846] Freezing remaining freezable tasks ... (elapsed 0.002 seconds) done.
[   32.622192] Suspending console(s) (use no_console_suspend to debug)
[   32.651123] dpm_run_callback(): mdio_bus_suspend+0x0/0x24 returns 4352
[   32.651428] PM: Device 2c000000.ethernet-ffffffff:01 failed to suspend: error 4352

This looks totally bogus.

First, "error" should be a negative number and we print it as int.

Second, error codes are not in this range anyway.

Quoted 8 lines

Quoted 7 lines

[   32.653289] PM: Some devices failed to suspend, or early wake event detected
[   32.685455] OOM killer enabled.
[   32.688629] Restarting tasks ... done.
[   32.695983] PM: suspend exit

ash: write error: Bad address

That too works just fine at commit 70286688e5ad.

Quoted 5 lines

Quoted 2 lines

Quoted 2 lines

Suspend fails at commit e2d7fe89e8ae though, so looks like we have two separate issues. I'll try to bisect that separately.

I guess what may happen is that something started to return positive numbers which confuse things all over when passed along by its callers as error codes.

I guess it this: https://patchwork.kernel.org/patch/10151763/

Quoted 31 lines · Lars-Peter Clausen

Quoted 30 lines · Rafael J. Wysocki

Quoted 29 lines · CET Rafael J. Wysocki

Quoted 4 lines · CET Tony Lindgren

Quoted 3 lines · Tony Lindgren

Well I tried to measure suspend power consumption and noticed that system suspend fails too hand hangs the network device:

Quoted 9 lines

Quoted 9 lines

# echo mem > /sys/power/state
[   32.577850] PM: suspend entry (deep)
[   32.582031] PM: Syncing filesystems ... done.
[   32.598083] Freezing user space processes ... (elapsed 0.002 seconds) done.
[   32.608398] OOM killer disabled.
[   32.611846] Freezing remaining freezable tasks ... (elapsed 0.002 seconds) done.
[   32.622192] Suspending console(s) (use no_console_suspend to debug)
[   32.651123] dpm_run_callback(): mdio_bus_suspend+0x0/0x24 returns 4352
[   32.651428] PM: Device 2c000000.ethernet-ffffffff:01 failed to suspend: error 4352

This looks totally bogus.

First, "error" should be a negative number and we print it as int.

Second, error codes are not in this range anyway.

Quoted 8 lines

Quoted 7 lines

[   32.653289] PM: Some devices failed to suspend, or early wake event detected
[   32.685455] OOM killer enabled.
[   32.688629] Restarting tasks ... done.
[   32.695983] PM: suspend exit

ash: write error: Bad address

That too works just fine at commit 70286688e5ad.

Quoted 8 lines

Quoted 5 lines

Quoted 2 lines

Quoted 2 lines

Suspend fails at commit e2d7fe89e8ae though, so looks like we have two separate issues. I'll try to bisect that separately.

I guess what may happen is that something started to return positive numbers which confuse things all over when passed along by its callers as error codes.

I guess it this: https://patchwork.kernel.org/patch/10151763/

Hi Tony,

Please try:

https://patchwork.ozlabs.org/patch/859297/

Hopefully we will get this into net-next soon.

Thanks that fixes the suspend error. And I was able to confirm that the suspend power consumption is OK.

That still leaves the mystery of the runtime idle power consumption being much higher with commit e130bc1d00a4.

Quoted 5 lines

Thanks that fixes the suspend error. And I was able to confirm that the suspend power consumption is OK.

That still leaves the mystery of the runtime idle power consumption being much higher with commit e130bc1d00a4.

Did you re-measure the runtime power? Do you have an unused PHY? It could be it is not getting shut down. 1G PHYs can be quite power hungry.

Quoted 29 lines · Andrew Morton

Quoted 17 lines · On Thu, 11 Jan 2018 16:23:22 -0800 Tony Lindgren <[email protected]> wrote:

Quoted 16 lines · Andrew Morton

Quoted 13 lines · On Thu, 11 Jan 2018 16:01:13 -0800 Tony Lindgren <[email protected]> wrote:

Hi all,

I'm seeing a considerable idle power consumption regression in Linux next, with power consumption for my idle test system going to 17.5mW compared to the usual 8mW on my test device.

Git bisect points to merge commit e130bc1d00a4 ("Merge branch 'akpm-current/current'") being the first bad commit.

I have also verified that commit 70286688e5ad ("ipc/mqueue.c:
have RT tasks queue in by priority in wq_add()") is good, and commit e2d7fe89e8ae ("Merge remote-tracking branch 'init_task/init_task'") is good.

Quoted 8 lines

Quoted 3 lines

Do you mean that everything up to and including 70286688e5ad ("ipc/mqueue.c: have RT tasks queue in by priority in wq_add()") is good?

Yes I'm not seeing the regression in your branch at commit 70286688e5ad. I'm seeing it only with the merge commit e130bc1d00a4.

That's weird. All I'm seeing between 70286688e5ad and end-of-mm is:

Well there are some changes in merge commit e130bc1d00a4..

It's commit 3bb0f7c31b1a ("ASoC: don't use snd_soc_write/read on twl4030"). And that is for the PMIC on my test system, so adding Kuninori and Mark to the thread :)

Kuninori, it seems that commit 3bb0f7c31b1a causes higher power consumption on an idle system on omap3 using twl4030. Reverting 3bb0f7c31b1a makes things behave again. My guess is that twl4030_read does not do the same as snd_soc_read in the driver?

Quoted 9 lines · Tony Lindgren

It's commit 3bb0f7c31b1a ("ASoC: don't use snd_soc_write/read on twl4030"). And that is for the PMIC on my test system, so adding Kuninori and Mark to the thread :)

Kuninori, it seems that commit 3bb0f7c31b1a causes higher power consumption on an idle system on omap3 using twl4030. Reverting 3bb0f7c31b1a makes things behave again. My guess is that twl4030_read does not do the same as snd_soc_read in the driver?

As far as I can tell it should end up boiling down to the same thing but I didn't follow through in detail, they should both bottom out in twl_i2c_read_u8() if they hit hardware - all snd_soc_read() did was call twl4030_read(), the patch just removes the indirection through assigning the pointer.

Could you try deleting the attempt to read from the cache in twl4030_read() and always go to hardware?

(twl4030_dummy_read [snd_soc_twl4030]) from [<bf1cc3b4>]
(snd_soc_codec_drv_read+0x1c/0x28 [snd_soc_core])
(snd_soc_codec_drv_read [snd_soc_core]) from [<bf1da1cc>]
(snd_soc_dapm_new_widgets+0x29c/0x578 [snd_soc_core])

(snd_soc_dapm_new_widgets [snd_soc_core]) from [<bf1d2f30>]
(snd_soc_register_card+0xad0/0xe30 [snd_soc_core])
(snd_soc_register_card [snd_soc_core]) from [<bf1e1850>]
(devm_snd_soc_register_card+0x30/0x70 [snd_soc_core])
(devm_snd_soc_register_card [snd_soc_core]) from [<bf234364>]
(omap_twl4030_probe+0x100/0x1d0 [snd_soc_omap_twl4030])
(omap_twl4030_probe [snd_soc_omap_twl4030]) from [<c0606660>]
(platform_drv_probe+0x50/0xb0)

So probably there are other asoc drivers broken too with these kind of patches until snd_soc_codec_drv_write() and snd_soc_codec_drv_read() are fixed?

Quoted 4 lines · Tony Lindgren

(twl4030_dummy_read [snd_soc_twl4030]) from [<bf1cc3b4>]
(snd_soc_codec_drv_read+0x1c/0x28 [snd_soc_core])
(snd_soc_codec_drv_read [snd_soc_core]) from [<bf1da1cc>]
(snd_soc_dapm_new_widgets+0x29c/0x578 [snd_soc_core])

Quoted 8 lines

(snd_soc_dapm_new_widgets [snd_soc_core]) from [<bf1d2f30>]
(snd_soc_register_card+0xad0/0xe30 [snd_soc_core])
(snd_soc_register_card [snd_soc_core]) from [<bf1e1850>]
(devm_snd_soc_register_card+0x30/0x70 [snd_soc_core])
(devm_snd_soc_register_card [snd_soc_core]) from [<bf234364>]
(omap_twl4030_probe+0x100/0x1d0 [snd_soc_omap_twl4030])
(omap_twl4030_probe [snd_soc_omap_twl4030]) from [<c0606660>]
(platform_drv_probe+0x50/0xb0)

So does the read not happen otherwise? Or is it failing somehow and the error getting ignored?

Quoted 3 lines

So probably there are other asoc drivers broken too with these kind of patches until snd_soc_codec_drv_write() and snd_soc_codec_drv_read() are fixed?

I rather suspect someone would've noticed by now TBH - I suspect this is more likely to be an issue with the rather baroque code that the TWL drivers have possibly coupled with the whole multiple I2C devices issue.

Quoted 3 lines

I rather suspect someone would've noticed by now TBH - I suspect this is more likely to be an issue with the rather baroque code that the TWL drivers have possibly coupled with the whole multiple I2C devices issue.

Hmm these calls are in sound/soc/soc-core.c though? But yeah, sure it could be some legacy code issue somewhere..

sound/soc/codecs/cx20442.c
sound/soc/codecs/tlv320dac33.c
sound/soc/codecs/twl4030.c
sound/soc/codecs/twl6040.c
sound/soc/codecs/uda1380.c
sound/soc/fsl/fsl_ssi.c

Can you confirm that some of these are still working?

Quoted 3 lines · Tony Lindgren

Quoted 3 lines

I rather suspect someone would've noticed by now TBH - I suspect this is more likely to be an issue with the rather baroque code that the TWL drivers have possibly coupled with the whole multiple I2C devices issue.

Quoted 2 lines

Hmm these calls are in sound/soc/soc-core.c though? But yeah, sure it could be some legacy code issue somewhere..

Most devices have one regmap per device which can be retrieved with dev_get_regmap(), it's the attempt to use that which I suspect is broken. Like I said snd_soc_codec_init_regmap() ought to fix things if that's the issue.

Quoted 8 lines

sound/soc/codecs/cx20442.c
sound/soc/codecs/tlv320dac33.c
sound/soc/codecs/twl4030.c
sound/soc/codecs/twl6040.c
sound/soc/codecs/uda1380.c
sound/soc/fsl/fsl_ssi.c

Can you confirm that some of these are still working?

Most of the non-TWL ones have no active users I'm aware of but fsl_ssi.c is under active development at the minute, one series was getting some successful testing earlier today so if it's broken I'd hope someone would have noticed although since it doesn't use DAPM or anything snd_soc_read()/write() will never get used. cx20442 was only in a single archaic consumer product, uda1380 has been legacy since before I was ever involved in ASoC.

Quoted 4 lines · Mark Brown

Most devices have one regmap per device which can be retrieved with dev_get_regmap(), it's the attempt to use that which I suspect is broken. Like I said snd_soc_codec_init_regmap() ought to fix things if that's the issue.

OK. Adding Peter to loop as it's his driver after all. Not sure how well mixing regmap register access to the same module with cached twl4030_read() would work :)

Maybe there should also be some big warning happening if snd_soc_codec_init_regmap() is now needed and no regmap is found?

Quoted 8 lines

Quoted 4 lines

Most devices have one regmap per device which can be retrieved with dev_get_regmap(), it's the attempt to use that which I suspect is broken. Like I said snd_soc_codec_init_regmap() ought to fix things if that's the issue.

OK. Adding Peter to loop as it's his driver after all. Not sure how well mixing regmap register access to the same module with cached twl4030_read() would work :)

Yes, that local cache is not a super good idea any more and hopefully redundant.

Quoted 3 lines

Maybe there should also be some big warning happening if snd_soc_codec_init_regmap() is now needed and no regmap is found?

Some devices just plain don't have registers at all (perhaps GPIOs or just stub drivers providing capability information). However we should be screaming loudly about the fact that the I/O we tried to do fails, that clearly shouldn't be being ignored.

Hi all

Quoted 16 lines

Quoted 8 lines

Quoted 4 lines

Most devices have one regmap per device which can be retrieved with dev_get_regmap(), it's the attempt to use that which I suspect is broken. Like I said snd_soc_codec_init_regmap() ought to fix things if that's the issue.

OK. Adding Peter to loop as it's his driver after all. Not sure how well mixing regmap register access to the same module with cached twl4030_read() would work :)

Yes, that local cache is not a super good idea any more and hopefully redundant.

Quoted 3 lines

Maybe there should also be some big warning happening if snd_soc_codec_init_regmap() is now needed and no regmap is found?

Quoted 4 lines

Some devices just plain don't have registers at all (perhaps GPIOs or just stub drivers providing capability information). However we should be screaming loudly about the fact that the I/O we tried to do fails, that clearly shouldn't be being ignored.

I'm sorry that my patch breaks your drivers.
It seems removing .read/.write callback was too much aggressive.

I hope your driver will be OK by using regmap.
In worst case, we can back .read/.write, but it will be component driver side.

Quoted 2 lines · Kuninori Morimoto

I hope your driver will be OK by using regmap.
In worst case, we can back .read/.write, but it will be component driver side.

Well we have the merge window about to open in a week, so please just revert the known broken patches.

Quoted 7 lines · Tony Lindgren

Quoted 2 lines · Kuninori Morimoto

I hope your driver will be OK by using regmap.
In worst case, we can back .read/.write, but it will be component driver side.

Well we have the merge window about to open in a week, so please just revert the known broken patches.

Did anyone actually try testing the fix I posted yet?

Quoted 1 line · Mark Brown

Did anyone actually try testing the fix I posted yet?

Hmm I don't seem to have anything in my inbox. What might be the subject for it?

Quoted 6 lines · Tony Lindgren

Quoted 1 line · Mark Brown

Did anyone actually try testing the fix I posted yet?

Hmm I don't seem to have anything in my inbox. What might be the subject for it?

Sorry, I didn't actually post it - just suggested adding the snd_soc_codec_set_regmap() call.

Quoted 2 lines · Mark Brown

Sorry, I didn't actually post it - just suggested adding the snd_soc_codec_set_regmap() call.

Oh I see. Yeah that seems like a good long term solution after the regressions are fixed.

Quoted 7 lines · Tony Lindgren

Quoted 2 lines · Mark Brown

Sorry, I didn't actually post it - just suggested adding the snd_soc_codec_set_regmap() call.

Oh I see. Yeah that seems like a good long term solution after the regressions are fixed.

I would like to see us do that sooner rather than later assuming it addresses the issue, it's quicker and simpler.

From tony Mon Sep 17 00:00:00 2001

From tony Mon Sep 17 00:00:00 2001
From: Tony Lindgren <[email protected]>
Date: Fri, 12 Jan 2018 10:24:36 -0800
Subject: [PATCH] ASoC: Fix twl4030 and 6040 regression by adding back read
and write

Commit 3bb0f7c31b1a ("ASoC: don't use snd_soc_write/read on twl4030") caused regressions for both twl4030 and twl6040 as it assumes the ASoC driver is using regmap. As a side effect, this also causes a considerable increase in idle power consumption omap3 boards using twl4030 as the PMIC.

This is because the removal of read and write function pointers causes some of the ASoC IO functions to not do anything. For example, snd_soc_register_card() calls snd_soc_dapm_new_widgets() that calls snd_soc_codec_drv_read() that now does nothing.

A long term solution suggested by Mark Brown <[email protected]> is to make the twl drivers use regmap by adding a call to snd_soc_codec_set_regmap(). This however needs more consideration as currently the driver internal reads do caching and we would have both regmap access and internal read/write access accessing the same hardware registers.

So to fix the regression, let's just do a partial revert adding back the read and write function pointers. Note that other non-regmap ASoC drivers may need similar patches.

Fixes 3bb0f7c31b1a ("ASoC: don't use snd_soc_write/read on twl4030")

Fixes 93a00c467fe9 ("ASoC: don't use snd_soc_write/read on twl6040")

Cc Kuninori Morimoto <[email protected]>

Cc Peter Ujfalusi <[email protected]>

Signed-off-by Tony Lindgren <[email protected]>

---

Quoted 6 lines · Mark Brown

Quoted 4 lines · Tony Lindgren

Quoted 2 lines · Mark Brown

Sorry, I didn't actually post it - just suggested adding the snd_soc_codec_set_regmap() call.

Quoted 5 lines

Quoted 2 lines

Oh I see. Yeah that seems like a good long term solution after the regressions are fixed.

I would like to see us do that sooner rather than later assuming it addresses the issue, it's quicker and simpler.

Peter, can you take a look at that?

Meanwhile, here's a regression fix for Linux next for twl4030 and 6040 drivers.

Regards,

Tony

8< ---------------------
From tony Mon Sep 17 00:00:00 2001
From: Tony Lindgren <[email protected]>
Date: Fri, 12 Jan 2018 10:24:36 -0800
Subject: [PATCH] ASoC: Fix twl4030 and 6040 regression by adding back read
and write

Commit 3bb0f7c31b1a ("ASoC: don't use snd_soc_write/read on twl4030") caused regressions for both twl4030 and twl6040 as it assumes the ASoC driver is using regmap. As a side effect, this also causes a considerable increase in idle power consumption omap3 boards using twl4030 as the PMIC.

This is because the removal of read and write function pointers causes some of the ASoC IO functions to not do anything. For example, snd_soc_register_card() calls snd_soc_dapm_new_widgets() that calls snd_soc_codec_drv_read() that now does nothing.

A long term solution suggested by Mark Brown <[email protected]> is to make the twl drivers use regmap by adding a call to snd_soc_codec_set_regmap(). This however needs more consideration as currently the driver internal reads do caching and we would have both regmap access and internal read/write access accessing the same hardware registers.

So to fix the regression, let's just do a partial revert adding back the read and write function pointers. Note that other non-regmap ASoC drivers may need similar patches.

Fixes 3bb0f7c31b1a ("ASoC: don't use snd_soc_write/read on twl4030")

Fixes 93a00c467fe9 ("ASoC: don't use snd_soc_write/read on twl6040")

Cc Kuninori Morimoto <[email protected]>

Cc Peter Ujfalusi <[email protected]>

Signed-off-by Tony Lindgren <[email protected]>

---
 sound/soc/codecs/twl4030.c | 2 ++
 sound/soc/codecs/twl6040.c | 2 ++
 2 files changed, 4 insertions(+)

sound/soc/codecs/twl4030.c +2 -0

@@ -2195,6 +2195,8 @@ static int twl4030_soc_remove(struct snd_soc_codec *codec)
 static const struct snd_soc_codec_driver soc_codec_dev_twl4030 = {
 	.probe = twl4030_soc_probe,
 	.remove = twl4030_soc_remove,
+	.read = twl4030_read,
+	.write = twl4030_write,
 	.set_bias_level = twl4030_set_bias_level,
 	.idle_bias_off = true,
 

Quoted 2 lines

I'm sorry that my patch breaks your drivers.
It seems removing .read/.write callback was too much aggressive.

No problem, things happen. The thing I'm worried about here though is that these patches were "completely untested clean-up patches" :)

For the next set of changes like this,
please make sure you Cc the driver authors so they can test and ack the changes if you can't test them, OK?

Quoted 3 lines

For the next set of changes like this,
please make sure you Cc the driver authors so they can test and ack the changes if you can't test them, OK?

Actually, I have posted "convert codec/platform to component" patch-set to ALSA ML, without Cc:ing to you.

	Subject: [PATCH 00/38] ASoC: replace platform to component
	Date: Fri, 12 Jan 2018 09:40:29 +0900

	Subject: [PATCH 000/187] ASoC: replace codec to component
	Date: Fri, 12 Jan 2018 10:05:05 +0900

I will send test request mail to each driver's authoer

Tturns out just adding back .read = twl4030_read fixes it.. I added a dummy function for read and am now seeing a bunch of reads that now don't happen: