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, srinivas.kandagatla@linaro.org
Cc: tiwai@suse.de, broonie@kernel.org, vkoul@kernel.org,
	gregkh@linuxfoundation.org,
	"Bard liao" <yung-chuan.liao@linux.intel.com>,
	"Rander Wang" <rander.wang@linux.intel.com>,
	"Shuming Fan" <shumingf@realtek.com>,
	"Jack Yu" <jack.yu@realtek.com>,
	"Oder Chiou" <oder_chiou@realtek.com>,
	"Jaroslav Kysela" <perex@perex.cz>,
	"Hui Wang" <hui.wang@canonical.com>,
	"Pierre-Louis Bossart" <pierre-louis.bossart@linux.intel.com>,
	"Bard Liao" <bard.liao@intel.com>,
	"Péter Ujfalusi" <peter.ujfalusi@linux.intel.com>,
	"Liam Girdwood" <lgirdwood@gmail.com>,
	"Takashi Iwai" <tiwai@suse.com>,
	linux-kernel@vger.kernel.org (open list)
Subject: [PATCH 2/5] ASoC: rt700-sdw: fix race condition on system suspend
Date: Mon, 14 Jun 2021 13:08:12 -0500	[thread overview]
Message-ID: <20210614180815.153711-3-pierre-louis.bossart@linux.intel.com> (raw)
In-Reply-To: <20210614180815.153711-1-pierre-louis.bossart@linux.intel.com>

In previous commits we cancelled deferred work, but there is still a
window of time where a new interrupt could result in new deferred work
executed after the link is disabled, leading to an IO error.

This patch uses an 'disable_irq_lock' mutex to prevent new interrupts
from happening after the start of the system suspend. The choice of a
mutex v. a spinlock is mainly due to the time required to clear
interrupts, which requires a command to be transmitted by the
SoundWire host IP and acknowledged with an interrupt. The
'interrupt_callback' routine is also not meant to be called from an
interrupt context.

An additional 'disable_irq' flag prevents race conditions where the
status changes before the interrupts are disabled, but the workqueue
handling status changes is scheduled after the completion of the
system suspend. On resume the interrupts are re-enabled already by the
io_init routine so we only clear the flag.

BugLink: https://github.com/thesofproject/linux/issues/2943
Fixes: 5f2df2a4583b ('ASoC: rt700: wait for the delayed work to finish when the system suspends')
Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Reviewed-by: Bard Liao <bard.liao@intel.com>
Reviewed-by: Péter Ujfalusi <peter.ujfalusi@linux.intel.com>
---
 sound/soc/codecs/rt700-sdw.c | 34 ++++++++++++++++++++++++++++++++--
 sound/soc/codecs/rt700.c     |  4 ++++
 sound/soc/codecs/rt700.h     |  2 ++
 3 files changed, 38 insertions(+), 2 deletions(-)

diff --git a/sound/soc/codecs/rt700-sdw.c b/sound/soc/codecs/rt700-sdw.c
index d1d9c0f455b4..bda594899664 100644
--- a/sound/soc/codecs/rt700-sdw.c
+++ b/sound/soc/codecs/rt700-sdw.c
@@ -418,10 +418,12 @@ static int rt700_interrupt_callback(struct sdw_slave *slave,
 	dev_dbg(&slave->dev,
 		"%s control_port_stat=%x", __func__, status->control_port);
 
-	if (status->control_port & 0x4) {
+	mutex_lock(&rt700->disable_irq_lock);
+	if (status->control_port & 0x4 && !rt700->disable_irq) {
 		mod_delayed_work(system_power_efficient_wq,
 			&rt700->jack_detect_work, msecs_to_jiffies(250));
 	}
+	mutex_unlock(&rt700->disable_irq_lock);
 
 	return 0;
 }
@@ -490,6 +492,34 @@ static int __maybe_unused rt700_dev_suspend(struct device *dev)
 	return 0;
 }
 
+static int __maybe_unused rt700_dev_system_suspend(struct device *dev)
+{
+	struct sdw_slave *slave = dev_to_sdw_dev(dev);
+	struct rt700_priv *rt700 = dev_get_drvdata(dev);
+	int ret;
+
+	if (!rt700->hw_init)
+		return 0;
+
+	/*
+	 * prevent new interrupts from being handled after the
+	 * deferred work completes and before the parent disables
+	 * interrupts on the link
+	 */
+	mutex_lock(&rt700->disable_irq_lock);
+	rt700->disable_irq = true;
+	ret = sdw_update_no_pm(slave, SDW_SCP_INTMASK1,
+			       SDW_SCP_INT1_IMPL_DEF, 0);
+	mutex_unlock(&rt700->disable_irq_lock);
+
+	if (ret < 0) {
+		/* log but don't prevent suspend from happening */
+		dev_dbg(&slave->dev, "%s: could not disable imp-def interrupts\n:", __func__);
+	}
+
+	return rt700_dev_suspend(dev);
+}
+
 #define RT700_PROBE_TIMEOUT 5000
 
 static int __maybe_unused rt700_dev_resume(struct device *dev)
@@ -521,7 +551,7 @@ static int __maybe_unused rt700_dev_resume(struct device *dev)
 }
 
 static const struct dev_pm_ops rt700_pm = {
-	SET_SYSTEM_SLEEP_PM_OPS(rt700_dev_suspend, rt700_dev_resume)
+	SET_SYSTEM_SLEEP_PM_OPS(rt700_dev_system_suspend, rt700_dev_resume)
 	SET_RUNTIME_PM_OPS(rt700_dev_suspend, rt700_dev_resume, NULL)
 };
 
diff --git a/sound/soc/codecs/rt700.c b/sound/soc/codecs/rt700.c
index 01af9d9dd3ca..921382724f9c 100644
--- a/sound/soc/codecs/rt700.c
+++ b/sound/soc/codecs/rt700.c
@@ -1112,6 +1112,8 @@ int rt700_init(struct device *dev, struct regmap *sdw_regmap,
 	rt700->sdw_regmap = sdw_regmap;
 	rt700->regmap = regmap;
 
+	mutex_init(&rt700->disable_irq_lock);
+
 	/*
 	 * Mark hw_init to false
 	 * HW init will be performed when device reports present
@@ -1133,6 +1135,8 @@ int rt700_io_init(struct device *dev, struct sdw_slave *slave)
 {
 	struct rt700_priv *rt700 = dev_get_drvdata(dev);
 
+	rt700->disable_irq = false;
+
 	if (rt700->hw_init)
 		return 0;
 
diff --git a/sound/soc/codecs/rt700.h b/sound/soc/codecs/rt700.h
index 794ee2e29051..bed9d1de6d5b 100644
--- a/sound/soc/codecs/rt700.h
+++ b/sound/soc/codecs/rt700.h
@@ -23,6 +23,8 @@ struct  rt700_priv {
 	struct delayed_work jack_detect_work;
 	struct delayed_work jack_btn_check_work;
 	int jack_type;
+	struct mutex disable_irq_lock; /* imp-def irq lock protection */
+	bool disable_irq;
 };
 
 struct sdw_stream_data {
-- 
2.25.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, srinivas.kandagatla@linaro.org
Cc: "Oder Chiou" <oder_chiou@realtek.com>,
	"Jack Yu" <jack.yu@realtek.com>,
	"open list" <linux-kernel@vger.kernel.org>,
	"Liam Girdwood" <lgirdwood@gmail.com>,
	tiwai@suse.de, gregkh@linuxfoundation.org,
	"Takashi Iwai" <tiwai@suse.com>,
	"Péter Ujfalusi" <peter.ujfalusi@linux.intel.com>,
	"Pierre-Louis Bossart" <pierre-louis.bossart@linux.intel.com>,
	"Hui Wang" <hui.wang@canonical.com>,
	vkoul@kernel.org, broonie@kernel.org,
	"Shuming Fan" <shumingf@realtek.com>,
	"Bard liao" <yung-chuan.liao@linux.intel.com>,
	"Rander Wang" <rander.wang@linux.intel.com>,
	"Bard Liao" <bard.liao@intel.com>
Subject: [PATCH 2/5] ASoC: rt700-sdw: fix race condition on system suspend
Date: Mon, 14 Jun 2021 13:08:12 -0500	[thread overview]
Message-ID: <20210614180815.153711-3-pierre-louis.bossart@linux.intel.com> (raw)
In-Reply-To: <20210614180815.153711-1-pierre-louis.bossart@linux.intel.com>

In previous commits we cancelled deferred work, but there is still a
window of time where a new interrupt could result in new deferred work
executed after the link is disabled, leading to an IO error.

This patch uses an 'disable_irq_lock' mutex to prevent new interrupts
from happening after the start of the system suspend. The choice of a
mutex v. a spinlock is mainly due to the time required to clear
interrupts, which requires a command to be transmitted by the
SoundWire host IP and acknowledged with an interrupt. The
'interrupt_callback' routine is also not meant to be called from an
interrupt context.

An additional 'disable_irq' flag prevents race conditions where the
status changes before the interrupts are disabled, but the workqueue
handling status changes is scheduled after the completion of the
system suspend. On resume the interrupts are re-enabled already by the
io_init routine so we only clear the flag.

BugLink: https://github.com/thesofproject/linux/issues/2943
Fixes: 5f2df2a4583b ('ASoC: rt700: wait for the delayed work to finish when the system suspends')
Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Reviewed-by: Bard Liao <bard.liao@intel.com>
Reviewed-by: Péter Ujfalusi <peter.ujfalusi@linux.intel.com>
---
 sound/soc/codecs/rt700-sdw.c | 34 ++++++++++++++++++++++++++++++++--
 sound/soc/codecs/rt700.c     |  4 ++++
 sound/soc/codecs/rt700.h     |  2 ++
 3 files changed, 38 insertions(+), 2 deletions(-)

diff --git a/sound/soc/codecs/rt700-sdw.c b/sound/soc/codecs/rt700-sdw.c
index d1d9c0f455b4..bda594899664 100644
--- a/sound/soc/codecs/rt700-sdw.c
+++ b/sound/soc/codecs/rt700-sdw.c
@@ -418,10 +418,12 @@ static int rt700_interrupt_callback(struct sdw_slave *slave,
 	dev_dbg(&slave->dev,
 		"%s control_port_stat=%x", __func__, status->control_port);
 
-	if (status->control_port & 0x4) {
+	mutex_lock(&rt700->disable_irq_lock);
+	if (status->control_port & 0x4 && !rt700->disable_irq) {
 		mod_delayed_work(system_power_efficient_wq,
 			&rt700->jack_detect_work, msecs_to_jiffies(250));
 	}
+	mutex_unlock(&rt700->disable_irq_lock);
 
 	return 0;
 }
@@ -490,6 +492,34 @@ static int __maybe_unused rt700_dev_suspend(struct device *dev)
 	return 0;
 }
 
+static int __maybe_unused rt700_dev_system_suspend(struct device *dev)
+{
+	struct sdw_slave *slave = dev_to_sdw_dev(dev);
+	struct rt700_priv *rt700 = dev_get_drvdata(dev);
+	int ret;
+
+	if (!rt700->hw_init)
+		return 0;
+
+	/*
+	 * prevent new interrupts from being handled after the
+	 * deferred work completes and before the parent disables
+	 * interrupts on the link
+	 */
+	mutex_lock(&rt700->disable_irq_lock);
+	rt700->disable_irq = true;
+	ret = sdw_update_no_pm(slave, SDW_SCP_INTMASK1,
+			       SDW_SCP_INT1_IMPL_DEF, 0);
+	mutex_unlock(&rt700->disable_irq_lock);
+
+	if (ret < 0) {
+		/* log but don't prevent suspend from happening */
+		dev_dbg(&slave->dev, "%s: could not disable imp-def interrupts\n:", __func__);
+	}
+
+	return rt700_dev_suspend(dev);
+}
+
 #define RT700_PROBE_TIMEOUT 5000
 
 static int __maybe_unused rt700_dev_resume(struct device *dev)
@@ -521,7 +551,7 @@ static int __maybe_unused rt700_dev_resume(struct device *dev)
 }
 
 static const struct dev_pm_ops rt700_pm = {
-	SET_SYSTEM_SLEEP_PM_OPS(rt700_dev_suspend, rt700_dev_resume)
+	SET_SYSTEM_SLEEP_PM_OPS(rt700_dev_system_suspend, rt700_dev_resume)
 	SET_RUNTIME_PM_OPS(rt700_dev_suspend, rt700_dev_resume, NULL)
 };
 
diff --git a/sound/soc/codecs/rt700.c b/sound/soc/codecs/rt700.c
index 01af9d9dd3ca..921382724f9c 100644
--- a/sound/soc/codecs/rt700.c
+++ b/sound/soc/codecs/rt700.c
@@ -1112,6 +1112,8 @@ int rt700_init(struct device *dev, struct regmap *sdw_regmap,
 	rt700->sdw_regmap = sdw_regmap;
 	rt700->regmap = regmap;
 
+	mutex_init(&rt700->disable_irq_lock);
+
 	/*
 	 * Mark hw_init to false
 	 * HW init will be performed when device reports present
@@ -1133,6 +1135,8 @@ int rt700_io_init(struct device *dev, struct sdw_slave *slave)
 {
 	struct rt700_priv *rt700 = dev_get_drvdata(dev);
 
+	rt700->disable_irq = false;
+
 	if (rt700->hw_init)
 		return 0;
 
diff --git a/sound/soc/codecs/rt700.h b/sound/soc/codecs/rt700.h
index 794ee2e29051..bed9d1de6d5b 100644
--- a/sound/soc/codecs/rt700.h
+++ b/sound/soc/codecs/rt700.h
@@ -23,6 +23,8 @@ struct  rt700_priv {
 	struct delayed_work jack_detect_work;
 	struct delayed_work jack_btn_check_work;
 	int jack_type;
+	struct mutex disable_irq_lock; /* imp-def irq lock protection */
+	bool disable_irq;
 };
 
 struct sdw_stream_data {
-- 
2.25.1


  parent reply	other threads:[~2021-06-14 18:08 UTC|newest]

Thread overview: 15+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-06-14 18:08 [PATCH 0/5] ASoC/SoundWire: fix race condition on system suspend Pierre-Louis Bossart
2021-06-14 18:08 ` [PATCH 1/5] soundwire: export sdw_update() and sdw_update_no_pm() Pierre-Louis Bossart
2021-06-14 18:08   ` Pierre-Louis Bossart
2021-06-20 11:08   ` Vinod Koul
2021-06-20 11:08     ` Vinod Koul
2021-06-21 18:46   ` Mark Brown
2021-06-21 18:46     ` Mark Brown
2021-06-14 18:08 ` Pierre-Louis Bossart [this message]
2021-06-14 18:08   ` [PATCH 2/5] ASoC: rt700-sdw: fix race condition on system suspend Pierre-Louis Bossart
2021-06-14 18:08 ` [PATCH 3/5] ASoC: rt711-sdw: " Pierre-Louis Bossart
2021-06-14 18:08   ` Pierre-Louis Bossart
2021-06-14 18:08 ` [PATCH 4/5] ASoC: rt5682-sdw: " Pierre-Louis Bossart
2021-06-14 18:08   ` Pierre-Louis Bossart
2021-06-14 18:08 ` [PATCH 5/5] ASoC: rt711-sdca-sdw: " Pierre-Louis Bossart
2021-06-14 18:08   ` Pierre-Louis Bossart

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=20210614180815.153711-3-pierre-louis.bossart@linux.intel.com \
    --to=pierre-louis.bossart@linux.intel.com \
    --cc=alsa-devel@alsa-project.org \
    --cc=bard.liao@intel.com \
    --cc=broonie@kernel.org \
    --cc=gregkh@linuxfoundation.org \
    --cc=hui.wang@canonical.com \
    --cc=jack.yu@realtek.com \
    --cc=lgirdwood@gmail.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=oder_chiou@realtek.com \
    --cc=perex@perex.cz \
    --cc=peter.ujfalusi@linux.intel.com \
    --cc=rander.wang@linux.intel.com \
    --cc=shumingf@realtek.com \
    --cc=srinivas.kandagatla@linaro.org \
    --cc=tiwai@suse.com \
    --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.