All of lore.kernel.org
 help / color / mirror / Atom feed
From: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
To: alsa-devel@alsa-project.org
Cc: tiwai@suse.de, gregkh@linuxfoundation.org,
	"Péter Ujfalusi" <peter.ujfalusi@linux.intel.com>,
	"Pierre-Louis Bossart" <pierre-louis.bossart@linux.intel.com>,
	"Rander Wang" <rander.wang@intel.com>,
	vkoul@kernel.org, broonie@kernel.org,
	srinivas.kandagatla@linaro.org,
	"Ranjani Sridharan" <ranjani.sridharan@linux.intel.com>,
	"Sanyog Kale" <sanyog.r.kale@intel.com>,
	"Bard liao" <yung-chuan.liao@linux.intel.com>,
	"open list" <linux-kernel@vger.kernel.org>
Subject: [PATCH 3/3] soundwire: intel: use pm_runtime_resume() on component probe
Date: Tue, 21 Jun 2022 17:56:40 -0500	[thread overview]
Message-ID: <20220621225641.221170-4-pierre-louis.bossart@linux.intel.com> (raw)
In-Reply-To: <20220621225641.221170-1-pierre-louis.bossart@linux.intel.com>

During the card registration, transactions on the SoundWire bus can be
initiated. If the ALSA card is registered after the bus suspends,
timeouts can be seen while reading/writing codec registers. This is
extremely easy to reproduce in driver bind/unbind tests.

In an initial experiment, the ASoC soc-component.c code was modified
to initiate a pm_runtime resume on a component probe. The results
showed this was too invasive. Instead this patch suggests resuming the
SoundWire component only.

Because of the parent-child hierarchy enforced by the pm_runtime
framework, it can be argued that the codec component probe should be
enough to resume all necessary devices, and indeed the same resume
will be applied to SoundWire codecs used on Intel platforms.

Calling pm_runtime_resume() on both the Intel and codec sides has the
benefit of resuming the bus without assuming any order during the card
registration. The first component on a dailink to be probed will
resume the bus. In addition, if a codec driver did not implement this
transition, the Intel component would still resume the bus and avoid
timeouts on card registration.

BugLink: https://github.com/thesofproject/linux/issues/3651
Reviewed-by: Rander Wang <rander.wang@intel.com>
Reviewed-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>
Reviewed-by: Bard Liao <yung-chuan.liao@linux.intel.com>
Reviewed-by: Péter Ujfalusi <peter.ujfalusi@linux.intel.com>
Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
---
 drivers/soundwire/intel.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/drivers/soundwire/intel.c b/drivers/soundwire/intel.c
index 2e7c27d303b42..c907bab12ee33 100644
--- a/drivers/soundwire/intel.c
+++ b/drivers/soundwire/intel.c
@@ -1051,6 +1051,23 @@ static int intel_trigger(struct snd_pcm_substream *substream, int cmd, struct sn
 	return ret;
 }
 
+static int intel_component_probe(struct snd_soc_component *component)
+{
+	int ret;
+
+	/*
+	 * make sure the device is pm_runtime_active before initiating
+	 * bus transactions during the card registration.
+	 * We use pm_runtime_resume() here, without taking a reference
+	 * and releasing it immediately.
+	 */
+	ret = pm_runtime_resume(component->dev);
+	if (ret < 0 && ret != -EACCES)
+		return ret;
+
+	return 0;
+}
+
 static int intel_component_dais_suspend(struct snd_soc_component *component)
 {
 	struct snd_soc_dai *dai;
@@ -1106,6 +1123,7 @@ static const struct snd_soc_dai_ops intel_pcm_dai_ops = {
 
 static const struct snd_soc_component_driver dai_component = {
 	.name           = "soundwire",
+	.probe		= intel_component_probe,
 	.suspend	= intel_component_dais_suspend
 };
 
-- 
2.34.1


WARNING: multiple messages have this Message-ID (diff)
From: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
To: alsa-devel@alsa-project.org
Cc: tiwai@suse.de, broonie@kernel.org, vkoul@kernel.org,
	gregkh@linuxfoundation.org, srinivas.kandagatla@linaro.org,
	"Bard liao" <yung-chuan.liao@linux.intel.com>,
	"Pierre-Louis Bossart" <pierre-louis.bossart@linux.intel.com>,
	"Rander Wang" <rander.wang@intel.com>,
	"Ranjani Sridharan" <ranjani.sridharan@linux.intel.com>,
	"Péter Ujfalusi" <peter.ujfalusi@linux.intel.com>,
	"Sanyog Kale" <sanyog.r.kale@intel.com>,
	linux-kernel@vger.kernel.org (open list)
Subject: [PATCH 3/3] soundwire: intel: use pm_runtime_resume() on component probe
Date: Tue, 21 Jun 2022 17:56:40 -0500	[thread overview]
Message-ID: <20220621225641.221170-4-pierre-louis.bossart@linux.intel.com> (raw)
In-Reply-To: <20220621225641.221170-1-pierre-louis.bossart@linux.intel.com>

During the card registration, transactions on the SoundWire bus can be
initiated. If the ALSA card is registered after the bus suspends,
timeouts can be seen while reading/writing codec registers. This is
extremely easy to reproduce in driver bind/unbind tests.

In an initial experiment, the ASoC soc-component.c code was modified
to initiate a pm_runtime resume on a component probe. The results
showed this was too invasive. Instead this patch suggests resuming the
SoundWire component only.

Because of the parent-child hierarchy enforced by the pm_runtime
framework, it can be argued that the codec component probe should be
enough to resume all necessary devices, and indeed the same resume
will be applied to SoundWire codecs used on Intel platforms.

Calling pm_runtime_resume() on both the Intel and codec sides has the
benefit of resuming the bus without assuming any order during the card
registration. The first component on a dailink to be probed will
resume the bus. In addition, if a codec driver did not implement this
transition, the Intel component would still resume the bus and avoid
timeouts on card registration.

BugLink: https://github.com/thesofproject/linux/issues/3651
Reviewed-by: Rander Wang <rander.wang@intel.com>
Reviewed-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>
Reviewed-by: Bard Liao <yung-chuan.liao@linux.intel.com>
Reviewed-by: Péter Ujfalusi <peter.ujfalusi@linux.intel.com>
Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
---
 drivers/soundwire/intel.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/drivers/soundwire/intel.c b/drivers/soundwire/intel.c
index 2e7c27d303b42..c907bab12ee33 100644
--- a/drivers/soundwire/intel.c
+++ b/drivers/soundwire/intel.c
@@ -1051,6 +1051,23 @@ static int intel_trigger(struct snd_pcm_substream *substream, int cmd, struct sn
 	return ret;
 }
 
+static int intel_component_probe(struct snd_soc_component *component)
+{
+	int ret;
+
+	/*
+	 * make sure the device is pm_runtime_active before initiating
+	 * bus transactions during the card registration.
+	 * We use pm_runtime_resume() here, without taking a reference
+	 * and releasing it immediately.
+	 */
+	ret = pm_runtime_resume(component->dev);
+	if (ret < 0 && ret != -EACCES)
+		return ret;
+
+	return 0;
+}
+
 static int intel_component_dais_suspend(struct snd_soc_component *component)
 {
 	struct snd_soc_dai *dai;
@@ -1106,6 +1123,7 @@ static const struct snd_soc_dai_ops intel_pcm_dai_ops = {
 
 static const struct snd_soc_component_driver dai_component = {
 	.name           = "soundwire",
+	.probe		= intel_component_probe,
 	.suspend	= intel_component_dais_suspend
 };
 
-- 
2.34.1


  parent reply	other threads:[~2022-06-21 22:59 UTC|newest]

Thread overview: 8+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-06-21 22:56 [PATCH 0/3] soundwire: revisit peripheral driver bind/unbind Pierre-Louis Bossart
2022-06-21 22:56 ` [PATCH 1/3] soundwire: revisit driver bind/unbind and callbacks Pierre-Louis Bossart
2022-06-21 22:56   ` Pierre-Louis Bossart
2022-06-21 22:56 ` [PATCH 2/3] soundwire: peripheral: remove useless ops pointer Pierre-Louis Bossart
2022-06-21 22:56   ` Pierre-Louis Bossart
2022-06-21 22:56 ` Pierre-Louis Bossart [this message]
2022-06-21 22:56   ` [PATCH 3/3] soundwire: intel: use pm_runtime_resume() on component probe Pierre-Louis Bossart
2022-07-06  5:11 ` [PATCH 0/3] soundwire: revisit peripheral driver bind/unbind Vinod Koul

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20220621225641.221170-4-pierre-louis.bossart@linux.intel.com \
    --to=pierre-louis.bossart@linux.intel.com \
    --cc=alsa-devel@alsa-project.org \
    --cc=broonie@kernel.org \
    --cc=gregkh@linuxfoundation.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=peter.ujfalusi@linux.intel.com \
    --cc=rander.wang@intel.com \
    --cc=ranjani.sridharan@linux.intel.com \
    --cc=sanyog.r.kale@intel.com \
    --cc=srinivas.kandagatla@linaro.org \
    --cc=tiwai@suse.de \
    --cc=vkoul@kernel.org \
    --cc=yung-chuan.liao@linux.intel.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.