ASoC: rt5682-sdw: fix race condition on system suspend

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 RT5682-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 Fixes tag points to a 5.10 commit, there's no need to propagate
this change to earlier upstream versions.

BugLink: https://github.com/thesofproject/linux/issues/2943
Fixes: 4a55000722d7 ('ASoC: codecs: rt*.c: remove useless pointer cast')
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>
Link: https://lore.kernel.org/r/20210614180815.153711-5-pierre-louis.bossart@linux.intel.com
Signed-off-by: Mark Brown <broonie@kernel.org>

diff --git a/sound/soc/codecs/rt5682-sdw.c b/sound/soc/codecs/rt5682-sdw.c
index 54873730bec550e94ac7d84c68fec73aca0e221d..31a4f286043e46644e2f8f8dd5190020d9c46fb0 100644 (file)
--- a/sound/soc/codecs/rt5682-sdw.c
+++ b/sound/soc/codecs/rt5682-sdw.c
@@ -344,6 +344,8 @@ static int rt5682_sdw_init(struct device *dev, struct regmap *regmap,
        rt5682->sdw_regmap = regmap;
        rt5682->is_sdw = true;
 
+       mutex_init(&rt5682->disable_irq_lock);
+
        rt5682->regmap = devm_regmap_init(dev, NULL, dev,
                                          &rt5682_sdw_indirect_regmap);
        if (IS_ERR(rt5682->regmap)) {
@@ -378,6 +380,8 @@ static int rt5682_io_init(struct device *dev, struct sdw_slave *slave)
        int ret = 0, loop = 10;
        unsigned int val;
 
+       rt5682->disable_irq = false;
+
        if (rt5682->hw_init)
                return 0;
 
@@ -679,10 +683,12 @@ static int rt5682_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(&rt5682->disable_irq_lock);
+       if (status->control_port & 0x4 && !rt5682->disable_irq) {
                mod_delayed_work(system_power_efficient_wq,
                        &rt5682->jack_detect_work, msecs_to_jiffies(rt5682->irq_work_delay_time));
        }
+       mutex_unlock(&rt5682->disable_irq_lock);
 
        return 0;
 }
@@ -740,6 +746,34 @@ static int __maybe_unused rt5682_dev_suspend(struct device *dev)
        return 0;
 }
 
+static int __maybe_unused rt5682_dev_system_suspend(struct device *dev)
+{
+       struct rt5682_priv *rt5682 = dev_get_drvdata(dev);
+       struct sdw_slave *slave = dev_to_sdw_dev(dev);
+       int ret;
+
+       if (!rt5682->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(&rt5682->disable_irq_lock);
+       rt5682->disable_irq = true;
+       ret = sdw_update_no_pm(slave, SDW_SCP_INTMASK1,
+                              SDW_SCP_INT1_IMPL_DEF, 0);
+       mutex_unlock(&rt5682->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 rt5682_dev_suspend(dev);
+}
+
 static int __maybe_unused rt5682_dev_resume(struct device *dev)
 {
        struct sdw_slave *slave = dev_to_sdw_dev(dev);
@@ -768,7 +802,7 @@ regmap_sync:
 }
 
 static const struct dev_pm_ops rt5682_pm = {
-       SET_SYSTEM_SLEEP_PM_OPS(rt5682_dev_suspend, rt5682_dev_resume)
+       SET_SYSTEM_SLEEP_PM_OPS(rt5682_dev_system_suspend, rt5682_dev_resume)
        SET_RUNTIME_PM_OPS(rt5682_dev_suspend, rt5682_dev_resume, NULL)
 };
 

diff --git a/sound/soc/codecs/rt5682.h b/sound/soc/codecs/rt5682.h
index 74ff667670169e1025cc16a01bff6fb05b7abc92..b59221048ebf9d584b09e5fa7c202d2d386e3d21 100644 (file)
--- a/sound/soc/codecs/rt5682.h
+++ b/sound/soc/codecs/rt5682.h
@@ -1415,6 +1415,8 @@ struct rt5682_priv {
        struct regulator_bulk_data supplies[RT5682_NUM_SUPPLIES];
        struct delayed_work jack_detect_work;
        struct delayed_work jd_check_work;
+       struct mutex disable_irq_lock; /* imp-def irq lock protection */
+       bool disable_irq;
        struct mutex calibrate_mutex;
        struct sdw_slave *slave;
        enum sdw_slave_status status;