RE: [PATCH] ASoC: dapm: Fix the unpaired runtime_get/put cases
> -Original Message- > From: Mark Brown [mailto:broo...@opensource.wolfsonmicro.com] > Sent: Thursday, December 20, 2012 9:51 PM > To: Liu, Chuansheng > Cc: l...@ti.com; pe...@perex.cz; ti...@suse.de; alsa-de...@alsa-project.org; > linux-kernel@vger.kernel.org > Subject: Re: [PATCH] ASoC: dapm: Fix the unpaired runtime_get/put cases > > On Thu, Dec 20, 2012 at 06:37:26AM +, Liu, Chuansheng wrote: > > > Meanwhile, is it useful to add one warning there for that case? > > After all, in probing, set the bias to _STANDBY even idle_bias_off == 1, and > calling get_runtime_sync(), it > > will let the code more obscure. So giving a warning there to indicate the > driver: > > it is not suggested that in probing, set the bias to _STANDBY even > idle_bias_off == 1. > > Probably, send a patch please. Like I say it is possible to start off > in _STANDBY providing the driver grabs the runtime PM reference too but > I can't think of any reason for doing that so the warning seems sensible. The patch https://lkml.org/lkml/2012/12/20/506 has been sent for giving warning in that case. Thanks your reviewing. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] ASoC: dapm: Fix the unpaired runtime_get/put cases
On Thu, Dec 20, 2012 at 06:37:26AM +, Liu, Chuansheng wrote: > Meanwhile, is it useful to add one warning there for that case? > After all, in probing, set the bias to _STANDBY even idle_bias_off == 1, and > calling get_runtime_sync(), it > will let the code more obscure. So giving a warning there to indicate the > driver: > it is not suggested that in probing, set the bias to _STANDBY even > idle_bias_off == 1. Probably, send a patch please. Like I say it is possible to start off in _STANDBY providing the driver grabs the runtime PM reference too but I can't think of any reason for doing that so the warning seems sensible. signature.asc Description: Digital signature
Re: [PATCH] ASoC: dapm: Fix the unpaired runtime_get/put cases
On Thu, Dec 20, 2012 at 06:37:26AM +, Liu, Chuansheng wrote: Meanwhile, is it useful to add one warning there for that case? After all, in probing, set the bias to _STANDBY even idle_bias_off == 1, and calling get_runtime_sync(), it will let the code more obscure. So giving a warning there to indicate the driver: it is not suggested that in probing, set the bias to _STANDBY even idle_bias_off == 1. Probably, send a patch please. Like I say it is possible to start off in _STANDBY providing the driver grabs the runtime PM reference too but I can't think of any reason for doing that so the warning seems sensible. signature.asc Description: Digital signature
RE: [PATCH] ASoC: dapm: Fix the unpaired runtime_get/put cases
-Original Message- From: Mark Brown [mailto:broo...@opensource.wolfsonmicro.com] Sent: Thursday, December 20, 2012 9:51 PM To: Liu, Chuansheng Cc: l...@ti.com; pe...@perex.cz; ti...@suse.de; alsa-de...@alsa-project.org; linux-kernel@vger.kernel.org Subject: Re: [PATCH] ASoC: dapm: Fix the unpaired runtime_get/put cases On Thu, Dec 20, 2012 at 06:37:26AM +, Liu, Chuansheng wrote: Meanwhile, is it useful to add one warning there for that case? After all, in probing, set the bias to _STANDBY even idle_bias_off == 1, and calling get_runtime_sync(), it will let the code more obscure. So giving a warning there to indicate the driver: it is not suggested that in probing, set the bias to _STANDBY even idle_bias_off == 1. Probably, send a patch please. Like I say it is possible to start off in _STANDBY providing the driver grabs the runtime PM reference too but I can't think of any reason for doing that so the warning seems sensible. The patch https://lkml.org/lkml/2012/12/20/506 has been sent for giving warning in that case. Thanks your reviewing. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH] ASoC: dapm: Fix the unpaired runtime_get/put cases
> -Original Message- > From: Mark Brown [mailto:broo...@opensource.wolfsonmicro.com] > Sent: Wednesday, December 19, 2012 5:11 PM > To: Liu, Chuansheng > Cc: l...@ti.com; pe...@perex.cz; ti...@suse.de; alsa-de...@alsa-project.org; > linux-kernel@vger.kernel.org > Subject: Re: [PATCH] ASoC: dapm: Fix the unpaired runtime_get/put cases > > On Wed, Dec 19, 2012 at 06:36:37PM +0800, Chuansheng Liu wrote: > > > But some devices has been set to STANDY bias directly during device probing, > > such as cs42l73_probe(): > > cs42l73_set_bias_level(codec, SND_SOC_BIAS_STANDBY); > > > Then it will cause runtime_get() not be called but laterly runtime_put() > > will > > be called. Also found some other uppaired cases. > > This is just a bug in the driver, if it's idle_bias_off then it really > should be starting in _OFF or at the very least starting actually in > _STANDBY (including taking the runtime reference) rather than partially > in _STANDBY. Thanks your pointing out. You are totally right. Rechecked the related driver code, there is one place which starting from _STANDBY with idle_bias_off. It should be the main reason of bringing unpaired issue. Meanwhile, is it useful to add one warning there for that case? After all, in probing, set the bias to _STANDBY even idle_bias_off == 1, and calling get_runtime_sync(), it will let the code more obscure. So giving a warning there to indicate the driver: it is not suggested that in probing, set the bias to _STANDBY even idle_bias_off == 1. diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c index 9c768bc..d6adaec 100644 --- a/sound/soc/soc-core.c +++ b/sound/soc/soc-core.c @@ -1102,6 +1102,8 @@ static int soc_probe_codec(struct snd_soc_card *card, if (driver->probe) { ret = driver->probe(codec); + WARN_ON(codec->dapm.idle_bias_off && + codec->dapm.bias_level != SND_SOC_BIAS_OFF); if (ret < 0) { dev_err(codec->dev, "ASoC: failed to probe CODEC %d\n", ret); -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] ASoC: dapm: Fix the unpaired runtime_get/put cases
On Wed, Dec 19, 2012 at 06:36:37PM +0800, Chuansheng Liu wrote: > But some devices has been set to STANDY bias directly during device probing, > such as cs42l73_probe(): > cs42l73_set_bias_level(codec, SND_SOC_BIAS_STANDBY); > Then it will cause runtime_get() not be called but laterly runtime_put() will > be called. Also found some other uppaired cases. This is just a bug in the driver, if it's idle_bias_off then it really should be starting in _OFF or at the very least starting actually in _STANDBY (including taking the runtime reference) rather than partially in _STANDBY. > So here add new flag get_runtime, and the logic will be: > 1/ when device is from off to non-off bias, runtime_get() will be called if > not yet; > 2/ When device is off bias, runtime_put() will be called if runtime_get() has >been called; This is really not a good idea at all, it's just adding new special cases and making the code more obscure. signature.asc Description: Digital signature
Re: [PATCH] ASoC: dapm: Fix the unpaired runtime_get/put cases
On Wed, Dec 19, 2012 at 06:36:37PM +0800, Chuansheng Liu wrote: But some devices has been set to STANDY bias directly during device probing, such as cs42l73_probe(): cs42l73_set_bias_level(codec, SND_SOC_BIAS_STANDBY); Then it will cause runtime_get() not be called but laterly runtime_put() will be called. Also found some other uppaired cases. This is just a bug in the driver, if it's idle_bias_off then it really should be starting in _OFF or at the very least starting actually in _STANDBY (including taking the runtime reference) rather than partially in _STANDBY. So here add new flag get_runtime, and the logic will be: 1/ when device is from off to non-off bias, runtime_get() will be called if not yet; 2/ When device is off bias, runtime_put() will be called if runtime_get() has been called; This is really not a good idea at all, it's just adding new special cases and making the code more obscure. signature.asc Description: Digital signature
RE: [PATCH] ASoC: dapm: Fix the unpaired runtime_get/put cases
-Original Message- From: Mark Brown [mailto:broo...@opensource.wolfsonmicro.com] Sent: Wednesday, December 19, 2012 5:11 PM To: Liu, Chuansheng Cc: l...@ti.com; pe...@perex.cz; ti...@suse.de; alsa-de...@alsa-project.org; linux-kernel@vger.kernel.org Subject: Re: [PATCH] ASoC: dapm: Fix the unpaired runtime_get/put cases On Wed, Dec 19, 2012 at 06:36:37PM +0800, Chuansheng Liu wrote: But some devices has been set to STANDY bias directly during device probing, such as cs42l73_probe(): cs42l73_set_bias_level(codec, SND_SOC_BIAS_STANDBY); Then it will cause runtime_get() not be called but laterly runtime_put() will be called. Also found some other uppaired cases. This is just a bug in the driver, if it's idle_bias_off then it really should be starting in _OFF or at the very least starting actually in _STANDBY (including taking the runtime reference) rather than partially in _STANDBY. Thanks your pointing out. You are totally right. Rechecked the related driver code, there is one place which starting from _STANDBY with idle_bias_off. It should be the main reason of bringing unpaired issue. Meanwhile, is it useful to add one warning there for that case? After all, in probing, set the bias to _STANDBY even idle_bias_off == 1, and calling get_runtime_sync(), it will let the code more obscure. So giving a warning there to indicate the driver: it is not suggested that in probing, set the bias to _STANDBY even idle_bias_off == 1. diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c index 9c768bc..d6adaec 100644 --- a/sound/soc/soc-core.c +++ b/sound/soc/soc-core.c @@ -1102,6 +1102,8 @@ static int soc_probe_codec(struct snd_soc_card *card, if (driver-probe) { ret = driver-probe(codec); + WARN_ON(codec-dapm.idle_bias_off + codec-dapm.bias_level != SND_SOC_BIAS_OFF); if (ret 0) { dev_err(codec-dev, ASoC: failed to probe CODEC %d\n, ret); -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH] ASoC: dapm: Fix the unpaired runtime_get/put cases
Commit f1aac484f7(Take a pm_runtime reference on DAPM devices that are enabled) introduced runtime_get/put calling when devices are in off/non-off bias. It is based on: 1/ device from off to non-off bias is called thru dapm_pre_sequence_async; 2/ device from non-off to off bias is called thru dapm_post_sequence_async; There are the above conditions which then make sure runtime_get/put() are pairs. But some devices has been set to STANDY bias directly during device probing, such as cs42l73_probe(): cs42l73_set_bias_level(codec, SND_SOC_BIAS_STANDBY); Then it will cause runtime_get() not be called but laterly runtime_put() will be called. Also found some other uppaired cases. So here add new flag get_runtime, and the logic will be: 1/ when device is from off to non-off bias, runtime_get() will be called if not yet; 2/ When device is off bias, runtime_put() will be called if runtime_get() has been called; Signed-off-by: liu chuansheng --- include/sound/soc-dapm.h |1 + sound/soc/soc-dapm.c | 18 -- 2 files changed, 13 insertions(+), 6 deletions(-) diff --git a/include/sound/soc-dapm.h b/include/sound/soc-dapm.h index e1ef63d..e52fdcc 100644 --- a/include/sound/soc-dapm.h +++ b/include/sound/soc-dapm.h @@ -586,6 +586,7 @@ struct snd_soc_dapm_context { struct list_head list; int (*stream_event)(struct snd_soc_dapm_context *dapm, int event); + bool get_runtime; #ifdef CONFIG_DEBUG_FS struct dentry *debugfs_dapm; diff --git a/sound/soc/soc-dapm.c b/sound/soc/soc-dapm.c index 1e36bc8..e6d030a 100644 --- a/sound/soc/soc-dapm.c +++ b/sound/soc/soc-dapm.c @@ -1460,12 +1460,15 @@ static void dapm_pre_sequence_async(void *data, async_cookie_t cookie) struct snd_soc_dapm_context *d = data; int ret; + if ((d->target_bias_level != SND_SOC_BIAS_OFF) && + (d->dev) && !(d->get_runtime)) { + pm_runtime_get_sync(d->dev); + d->get_runtime = true; + } + /* If we're off and we're not supposed to be go into STANDBY */ if (d->bias_level == SND_SOC_BIAS_OFF && d->target_bias_level != SND_SOC_BIAS_OFF) { - if (d->dev) - pm_runtime_get_sync(d->dev); - ret = snd_soc_dapm_set_bias_level(d, SND_SOC_BIAS_STANDBY); if (ret != 0) dev_err(d->dev, @@ -1506,9 +1509,6 @@ static void dapm_post_sequence_async(void *data, async_cookie_t cookie) if (ret != 0) dev_err(d->dev, "ASoC: Failed to turn off bias: %d\n", ret); - - if (d->dev) - pm_runtime_put(d->dev); } /* If we just powered up then move to active bias */ @@ -1519,6 +1519,12 @@ static void dapm_post_sequence_async(void *data, async_cookie_t cookie) dev_err(d->dev, "ASoC: Failed to apply active bias: %d\n", ret); } + + if ((d->bias_level == SND_SOC_BIAS_OFF) && + (d->dev) && (d->get_runtime)) { + pm_runtime_put(d->dev); + d->get_runtime = false; + } } static void dapm_widget_set_peer_power(struct snd_soc_dapm_widget *peer, -- 1.7.0.4 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH] ASoC: dapm: Fix the unpaired runtime_get/put cases
Commit f1aac484f7(Take a pm_runtime reference on DAPM devices that are enabled) introduced runtime_get/put calling when devices are in off/non-off bias. It is based on: 1/ device from off to non-off bias is called thru dapm_pre_sequence_async; 2/ device from non-off to off bias is called thru dapm_post_sequence_async; There are the above conditions which then make sure runtime_get/put() are pairs. But some devices has been set to STANDY bias directly during device probing, such as cs42l73_probe(): cs42l73_set_bias_level(codec, SND_SOC_BIAS_STANDBY); Then it will cause runtime_get() not be called but laterly runtime_put() will be called. Also found some other uppaired cases. So here add new flag get_runtime, and the logic will be: 1/ when device is from off to non-off bias, runtime_get() will be called if not yet; 2/ When device is off bias, runtime_put() will be called if runtime_get() has been called; Signed-off-by: liu chuansheng chuansheng@intel.com --- include/sound/soc-dapm.h |1 + sound/soc/soc-dapm.c | 18 -- 2 files changed, 13 insertions(+), 6 deletions(-) diff --git a/include/sound/soc-dapm.h b/include/sound/soc-dapm.h index e1ef63d..e52fdcc 100644 --- a/include/sound/soc-dapm.h +++ b/include/sound/soc-dapm.h @@ -586,6 +586,7 @@ struct snd_soc_dapm_context { struct list_head list; int (*stream_event)(struct snd_soc_dapm_context *dapm, int event); + bool get_runtime; #ifdef CONFIG_DEBUG_FS struct dentry *debugfs_dapm; diff --git a/sound/soc/soc-dapm.c b/sound/soc/soc-dapm.c index 1e36bc8..e6d030a 100644 --- a/sound/soc/soc-dapm.c +++ b/sound/soc/soc-dapm.c @@ -1460,12 +1460,15 @@ static void dapm_pre_sequence_async(void *data, async_cookie_t cookie) struct snd_soc_dapm_context *d = data; int ret; + if ((d-target_bias_level != SND_SOC_BIAS_OFF) + (d-dev) !(d-get_runtime)) { + pm_runtime_get_sync(d-dev); + d-get_runtime = true; + } + /* If we're off and we're not supposed to be go into STANDBY */ if (d-bias_level == SND_SOC_BIAS_OFF d-target_bias_level != SND_SOC_BIAS_OFF) { - if (d-dev) - pm_runtime_get_sync(d-dev); - ret = snd_soc_dapm_set_bias_level(d, SND_SOC_BIAS_STANDBY); if (ret != 0) dev_err(d-dev, @@ -1506,9 +1509,6 @@ static void dapm_post_sequence_async(void *data, async_cookie_t cookie) if (ret != 0) dev_err(d-dev, ASoC: Failed to turn off bias: %d\n, ret); - - if (d-dev) - pm_runtime_put(d-dev); } /* If we just powered up then move to active bias */ @@ -1519,6 +1519,12 @@ static void dapm_post_sequence_async(void *data, async_cookie_t cookie) dev_err(d-dev, ASoC: Failed to apply active bias: %d\n, ret); } + + if ((d-bias_level == SND_SOC_BIAS_OFF) + (d-dev) (d-get_runtime)) { + pm_runtime_put(d-dev); + d-get_runtime = false; + } } static void dapm_widget_set_peer_power(struct snd_soc_dapm_widget *peer, -- 1.7.0.4 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/