| Message ID | 20220616220427.136036-1-pierre-louis.bossart@linux.intel.com |
|---|---|
| Headers | show |
| Series | ASoC: use pm_runtime_resume_and_get() when possible | expand |
On 2022-06-17 12:04 AM, Pierre-Louis Bossart wrote: > The current code does not check for errors and does not release the > reference on errors. Thanks for the fixes. Acked-by: Cezary Rojewski <cezary.rojewski@intel.com>
On 2022-06-17 12:04 AM, Pierre-Louis Bossart wrote: > After a set of SOF-specific changes, this patchset correct problematic > uses of pm_runtime_get_sync() in ASoC, or simplifies the flow with no > functional changes. Two patches for Intel platforms also add a test on > resume success. > > Additional changes were initially suggested to completely remove the > use of pm_runtime_get_sync(). These changes were dropped since they > are way too invasive, specifically in cases where the return values > were not tested, which would lead to duplicate pm_runtime_put(). The > remaining uses of pm_runtime_get_sync() cannot really be blindly > modified without context and knowledge of each driver. > > Pierre-Louis Bossart (11): > ASoC: Intel: catpt: use pm_runtime_resume_and_get() > ASoC: Intel: skylake: skl-pcm: use pm_runtime_resume_and_get() > ASoC: soc-component: use pm_runtime_resume_and_get() > ASoC: wcd-mbhc-v2: use pm_runtime_resume_and_get() > ASoC: wsa881x: use pm_runtime_resume_and_get() > ASoC: rockchip: i2s_tdm: use pm_runtime_resume_and_get() > ASoC: fsl: fsl_sai: use pm_runtime_resume_and_get() > ASoC: img: img-i2s-out: use pm_runtime_resume_and_get() > ASoC: rockchip: pdm: use pm_runtime_resume_and_get() > ASoC: tas2552: use pm_runtime_resume_and_get() > ASoC: ti: davinci-mcasp: use pm_runtime_resume_and_get() For the non-acked patches: Reviewed-by: Cezary Rojewski <cezary.rojewski@intel.com>
Hi Pierre, On 17/06/2022 01:04, Pierre-Louis Bossart wrote: > The use of pm_runtime_get_sync() is buggy with no use of put_noidle on > error. > > Use pm_runtime_resume_and_get() instead. Thanks for the fix, Acked-by: Peter Ujfalusi <peter.ujfalusi@gmail.com> > > Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> > Reviewed-by: Bard Liao <yung-chuan.liao@linux.intel.com> > Reviewed-by: Kai Vehmanen <kai.vehmanen@linux.intel.com> > Reviewed-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com> > --- > sound/soc/ti/davinci-mcasp.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > > diff --git a/sound/soc/ti/davinci-mcasp.c b/sound/soc/ti/davinci-mcasp.c > index e2aab4729f3ab..0201000b619f6 100644 > --- a/sound/soc/ti/davinci-mcasp.c > +++ b/sound/soc/ti/davinci-mcasp.c > @@ -2111,8 +2111,7 @@ static int davinci_mcasp_gpio_request(struct gpio_chip *chip, unsigned offset) > } > > /* Do not change the PIN yet */ > - > - return pm_runtime_get_sync(mcasp->dev); > + return pm_runtime_resume_and_get(mcasp->dev); > } > > static void davinci_mcasp_gpio_free(struct gpio_chip *chip, unsigned offset)
On 6/16/22 17:04, Pierre-Louis Bossart wrote: > simplify the flow. No functionality change, except that on -EACCESS > the reference count will be decreased. This patch turns out to be incorrect and should not be merged. I missed the fact that the component pm_runtime_put() will decrease the reference count that is already decreased with pm_runtime_resume_and_get() when pm_runtime is not enabled. This leads to warnings: snd-soc-dummy snd-soc-dummy: Runtime PM usage count underflow! Unfortunately we missed those warnings during validation, that's not so good. pm_runtime_resume_and_get() really needs to be used ONLY when the get/put are part of the same function and the reference count can be checked. When the get/put are in different functions, it's asking for trouble. Also the check on -EACCES is problematic when the component is handled by a framework, it's not clear if that can happen or not. The rest of the patches follow the pattern get_sync/put and don't have a problem. Sorry for the noise. > > Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> > Reviewed-by: Bard Liao <yung-chuan.liao@linux.intel.com> > Reviewed-by: Kai Vehmanen <kai.vehmanen@linux.intel.com> > Reviewed-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com> > --- > sound/soc/soc-component.c | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/sound/soc/soc-component.c b/sound/soc/soc-component.c > index e12f8244242b9..cb92e002c38bc 100644 > --- a/sound/soc/soc-component.c > +++ b/sound/soc/soc-component.c > @@ -1213,11 +1213,11 @@ int snd_soc_pcm_component_pm_runtime_get(struct snd_soc_pcm_runtime *rtd, > int i; > > for_each_rtd_components(rtd, i, component) { > - int ret = pm_runtime_get_sync(component->dev); > - if (ret < 0 && ret != -EACCES) { > - pm_runtime_put_noidle(component->dev); > + int ret = pm_runtime_resume_and_get(component->dev); > + > + if (ret < 0 && ret != -EACCES) > return soc_component_ret(component, ret); > - } > + > /* mark stream if succeeded */ > soc_component_mark_push(component, stream, pm); > }
On 16/06/2022 15:04, Pierre-Louis Bossart wrote: > simplify the flow. No functionality change, except that on -EACCESS > the reference count will be decreased. > > Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> > Reviewed-by: Bard Liao <yung-chuan.liao@linux.intel.com> > Reviewed-by: Kai Vehmanen <kai.vehmanen@linux.intel.com> > Reviewed-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com> > --- Thanks Pierre, LGTM, Reviewed-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org> --srini > sound/soc/codecs/wsa881x.c | 6 ++---- > 1 file changed, 2 insertions(+), 4 deletions(-) > > diff --git a/sound/soc/codecs/wsa881x.c b/sound/soc/codecs/wsa881x.c > index f3a56f3ce4871..dc954b85a9881 100644 > --- a/sound/soc/codecs/wsa881x.c > +++ b/sound/soc/codecs/wsa881x.c > @@ -749,11 +749,9 @@ static int wsa881x_put_pa_gain(struct snd_kcontrol *kc, > unsigned int mask = (1 << fls(max)) - 1; > int val, ret, min_gain, max_gain; > > - ret = pm_runtime_get_sync(comp->dev); > - if (ret < 0 && ret != -EACCES) { > - pm_runtime_put_noidle(comp->dev); > + ret = pm_runtime_resume_and_get(comp->dev); > + if (ret < 0 && ret != -EACCES) > return ret; > - } > > max_gain = (max - ucontrol->value.integer.value[0]) & mask; > /*
On 16/06/2022 15:04, Pierre-Louis Bossart wrote: > simplify the flow. No functionality change, except that on -EACCESS > the reference count will be decreased. > > Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> > Reviewed-by: Bard Liao <yung-chuan.liao@linux.intel.com> > Reviewed-by: Kai Vehmanen <kai.vehmanen@linux.intel.com> > Reviewed-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com> > --- Thanks Pierre, LGTM, Reviewed-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org> --srini > sound/soc/codecs/wcd-mbhc-v2.c | 10 ++++------ > 1 file changed, 4 insertions(+), 6 deletions(-) > > diff --git a/sound/soc/codecs/wcd-mbhc-v2.c b/sound/soc/codecs/wcd-mbhc-v2.c > index 31009283e7d4a..98baef594bf31 100644 > --- a/sound/soc/codecs/wcd-mbhc-v2.c > +++ b/sound/soc/codecs/wcd-mbhc-v2.c > @@ -714,12 +714,11 @@ static int wcd_mbhc_initialise(struct wcd_mbhc *mbhc) > struct snd_soc_component *component = mbhc->component; > int ret; > > - ret = pm_runtime_get_sync(component->dev); > + ret = pm_runtime_resume_and_get(component->dev); > if (ret < 0 && ret != -EACCES) { > dev_err_ratelimited(component->dev, > - "pm_runtime_get_sync failed in %s, ret %d\n", > + "pm_runtime_resume_and_get failed in %s, ret %d\n", > __func__, ret); > - pm_runtime_put_noidle(component->dev); > return ret; > } > > @@ -1097,12 +1096,11 @@ static void wcd_correct_swch_plug(struct work_struct *work) > mbhc = container_of(work, struct wcd_mbhc, correct_plug_swch); > component = mbhc->component; > > - ret = pm_runtime_get_sync(component->dev); > + ret = pm_runtime_resume_and_get(component->dev); > if (ret < 0 && ret != -EACCES) { > dev_err_ratelimited(component->dev, > - "pm_runtime_get_sync failed in %s, ret %d\n", > + "pm_runtime_resume_and_get failed in %s, ret %d\n", > __func__, ret); > - pm_runtime_put_noidle(component->dev); > return; > } > micbias_mv = wcd_mbhc_get_micbias(mbhc);
On Thu, 16 Jun 2022 17:04:16 -0500, Pierre-Louis Bossart wrote: > After a set of SOF-specific changes, this patchset correct problematic > uses of pm_runtime_get_sync() in ASoC, or simplifies the flow with no > functional changes. Two patches for Intel platforms also add a test on > resume success. > > Additional changes were initially suggested to completely remove the > use of pm_runtime_get_sync(). These changes were dropped since they > are way too invasive, specifically in cases where the return values > were not tested, which would lead to duplicate pm_runtime_put(). The > remaining uses of pm_runtime_get_sync() cannot really be blindly > modified without context and knowledge of each driver. > > [...] Applied to https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next Thanks! [01/11] ASoC: Intel: catpt: use pm_runtime_resume_and_get() commit: 82102a24c930986aedc572f89b437cd9e4d44d7e [02/11] ASoC: Intel: skylake: skl-pcm: use pm_runtime_resume_and_get() commit: 7213170a9515109322f75c08b5268d8e9cdad8e4 [04/11] ASoC: wcd-mbhc-v2: use pm_runtime_resume_and_get() commit: ddea4bbf287b6028eaa15a185d0693856956ecf2 [05/11] ASoC: wsa881x: use pm_runtime_resume_and_get() commit: 9a1a28610a1c49bf93777d017aa3fe121eef944e [06/11] ASoC: rockchip: i2s_tdm: use pm_runtime_resume_and_get() commit: 8c8a13e83c29472044d733dfb1fced2ccd025d35 [07/11] ASoC: fsl: fsl_sai: use pm_runtime_resume_and_get() commit: 37cb8a58013fc6ca2febaed355f6559012699542 [08/11] ASoC: img: img-i2s-out: use pm_runtime_resume_and_get() commit: 57d714535051b1baca9ffd92e79fbda1fae3177a [09/11] ASoC: rockchip: pdm: use pm_runtime_resume_and_get() commit: 76a6f4537650e6d2211f34661de35630487c7c64 [10/11] ASoC: tas2552: use pm_runtime_resume_and_get() commit: 05b71fb2a5014d2430ff6c5678db021c67afa9ec [11/11] ASoC: ti: davinci-mcasp: use pm_runtime_resume_and_get() commit: cecc81d6a5deb094bdbc6a1d7f2c014ba9b71cf8 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