@@ -256,6 +256,15 @@ static int rt711_sdca_interrupt_callback(struct sdw_slave *slave,
scp_sdca_stat2 = rt711->scp_sdca_stat2;
}
+ /*
+ * The critical section below intentionally protects a rather large piece of code.
+ * We don't want to allow the system suspend to disable an interrupt while we are
+ * processing it, which could be problematic given the quirky SoundWire interrupt
+ * scheme. We do want however to prevent new workqueues from being scheduled if
+ * the disable_irq flag was set during system suspend.
+ */
+ mutex_lock(&rt711->disable_irq_lock);
+
ret = sdw_read_no_pm(rt711->slave, SDW_SCP_SDCA_INT1);
if (ret < 0)
goto io_error;
@@ -314,13 +323,16 @@ static int rt711_sdca_interrupt_callback(struct sdw_slave *slave,
"%s scp_sdca_stat1=0x%x, scp_sdca_stat2=0x%x\n", __func__,
rt711->scp_sdca_stat1, rt711->scp_sdca_stat2);
- if (status->sdca_cascade)
+ if (status->sdca_cascade && !rt711->disable_irq)
mod_delayed_work(system_power_efficient_wq,
&rt711->jack_detect_work, msecs_to_jiffies(30));
+ mutex_unlock(&rt711->disable_irq_lock);
+
return 0;
io_error:
+ mutex_unlock(&rt711->disable_irq_lock);
pr_err_ratelimited("IO error in %s, ret %d\n", __func__, ret);
return ret;
}
@@ -382,6 +394,36 @@ static int __maybe_unused rt711_sdca_dev_suspend(struct device *dev)
return 0;
}
+static int __maybe_unused rt711_sdca_dev_system_suspend(struct device *dev)
+{
+ struct rt711_sdca_priv *rt711_sdca = dev_get_drvdata(dev);
+ struct sdw_slave *slave = dev_to_sdw_dev(dev);
+ int ret1, ret2;
+
+ if (!rt711_sdca->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(&rt711_sdca->disable_irq_lock);
+ rt711_sdca->disable_irq = true;
+ ret1 = sdw_update_no_pm(slave, SDW_SCP_SDCA_INTMASK1,
+ SDW_SCP_SDCA_INTMASK_SDCA_0, 0);
+ ret2 = sdw_update_no_pm(slave, SDW_SCP_SDCA_INTMASK2,
+ SDW_SCP_SDCA_INTMASK_SDCA_8, 0);
+ mutex_unlock(&rt711_sdca->disable_irq_lock);
+
+ if (ret1 < 0 || ret2 < 0) {
+ /* log but don't prevent suspend from happening */
+ dev_dbg(&slave->dev, "%s: could not disable SDCA interrupts\n:", __func__);
+ }
+
+ return rt711_sdca_dev_suspend(dev);
+}
+
#define RT711_PROBE_TIMEOUT 5000
static int __maybe_unused rt711_sdca_dev_resume(struct device *dev)
@@ -413,7 +455,7 @@ static int __maybe_unused rt711_sdca_dev_resume(struct device *dev)
}
static const struct dev_pm_ops rt711_sdca_pm = {
- SET_SYSTEM_SLEEP_PM_OPS(rt711_sdca_dev_suspend, rt711_sdca_dev_resume)
+ SET_SYSTEM_SLEEP_PM_OPS(rt711_sdca_dev_system_suspend, rt711_sdca_dev_resume)
SET_RUNTIME_PM_OPS(rt711_sdca_dev_suspend, rt711_sdca_dev_resume, NULL)
};
@@ -1411,6 +1411,8 @@ int rt711_sdca_init(struct device *dev, struct regmap *regmap,
rt711->regmap = regmap;
rt711->mbq_regmap = mbq_regmap;
+ mutex_init(&rt711->disable_irq_lock);
+
/*
* Mark hw_init to false
* HW init will be performed when device reports present
@@ -1494,6 +1496,8 @@ int rt711_sdca_io_init(struct device *dev, struct sdw_slave *slave)
int ret = 0;
unsigned int val;
+ rt711->disable_irq = false;
+
if (rt711->hw_init)
return 0;
@@ -27,6 +27,8 @@ struct rt711_sdca_priv {
struct delayed_work jack_detect_work;
struct delayed_work jack_btn_check_work;
struct mutex calibrate_mutex; /* for headset calibration */
+ struct mutex disable_irq_lock; /* SDCA irq lock protection */
+ bool disable_irq;
int jack_type, jd_src;
unsigned int scp_sdca_stat1, scp_sdca_stat2;
int hw_ver;
In the initial driver 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. While we did not see this IO error on RT711-sdca-based platforms, the code pattern is similar to the RT700 case where the IO error was noted, so the fix is added for consistency. 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. The code is slightly different from the other codecs since the interrupt callback deals with the SDCA interrupts, leading to a much larger section that's protected by the mutex. The SoundWire interrupt scheme requires a read after clearing a status, it's not clear from the specifications what would happen if SDCA interrupts are disabled in the middle of the sequence, so the entire interrupt status read/write is kept as is, even if in the end we discard the information. BugLink: https://github.com/thesofproject/linux/issues/2943 Fixes: 7ad4d237e7c4 ('ASoC: rt711-sdca: Add RT711 SDCA vendor-specific driver') 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/rt711-sdca-sdw.c | 46 +++++++++++++++++++++++++++++-- sound/soc/codecs/rt711-sdca.c | 4 +++ sound/soc/codecs/rt711-sdca.h | 2 ++ 3 files changed, 50 insertions(+), 2 deletions(-)