-
Notifications
You must be signed in to change notification settings - Fork 129
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[BUG] [SDW] IO error in rt700_jack_detect_handler on CML_RVP_SDW #2943
Comments
@XiaoyunWu6666 is this really 100% reproducible? If yes, can you try reverting PR #2941 to see if this makes the problem go away? |
@shumingfan @bardliao I wonder if there is a potential for a race condition. The set_jack callback is invoked from the machine driver level, and I wonder if we should have a pm_runtime_get_sync()/ pm_runtime_put_autosuspend() protection when we try to initialize the jack. The current test on hw_init is a bit flaky IMHO. I also wonder if we should set the jack to NULL when we suspend, as done for a lot of cards. |
@plbossart I also think it is possible that the unexpected interrupt happened after the codec suspended. |
Tested with PR #2949 and the test result still failed. |
@XiaoyunWu6666 Could you enable the debug log of the codec driver and share the kernel log again? |
@bardliao @plbossart I checked this log. The system is so quick going to suspend from the resume state when the issue happened. [ 170.958907] rt700 sdw:1:025d:0700:00: in rt700_dev_resume jd enable There is a Slave impl defined interrupt when the resume state, however, the system went to suspend state before the interrupt handler finishes. |
It is weird. We have pm_runtime_mark_last_busy(&slave->dev); in sdw_handle_slave_alerts. |
@XiaoyunWu6666 Can you try below patch? diff --git a/sound/soc/codecs/rt700-sdw.c b/sound/soc/codecs/rt700-sdw.c
index ff9c081fd52a..5df68de7eac1 100644
--- a/sound/soc/codecs/rt700-sdw.c
+++ b/sound/soc/codecs/rt700-sdw.c
@@ -9,6 +9,7 @@
#include <linux/delay.h>
#include <linux/device.h>
#include <linux/mod_devicetable.h>
+#include <linux/pm_runtime.h>
#include <linux/soundwire/sdw.h>
#include <linux/soundwire/sdw_type.h>
#include <linux/soundwire/sdw_registers.h>
@@ -419,6 +420,7 @@ static int rt700_interrupt_callback(struct sdw_slave *slave,
"%s control_port_stat=%x", __func__, status->control_port);
if (status->control_port & 0x4) {
+ pm_runtime_mark_last_busy(&slave->dev);
mod_delayed_work(system_power_efficient_wq,
&rt700->jack_detect_work, msecs_to_jiffies(250));
} |
@XiaoyunWu6666 Sorry, I meant test with #2949 + my patch |
oh I misunderstood. Let me check it again :) It failed again @bardliao |
@XiaoyunWu6666 Would you please check the node as below after the issue happened? |
|
@bardliao @shumingfan this is a nice bug... static int sdw_handle_slave_alerts(struct sdw_slave *slave)
{
sdw_modify_slave_status(slave, SDW_SLAVE_ALERT);
ret = pm_runtime_get_sync(&slave->dev);
do {
slave_notify = false;
// check interrupts
/* Update the Slave driver */
if (slave_notify && slave->ops &&
slave->ops->interrupt_callback) {
slave_intr.sdca_cascade = sdca_cascade;
slave_intr.control_port = clear;
memcpy(slave_intr.port, &port_status,
sizeof(slave_intr.port));
slave->ops->interrupt_callback(slave, &slave_intr);
}
// recheck interrupt status
/* we can get alerts while processing so keep retrying */
} while (stat != 0 && count < SDW_READ_INTR_CLEAR_RETRY);
if (count == SDW_READ_INTR_CLEAR_RETRY)
dev_warn(&slave->dev, "Reached MAX_RETRY on alert read\n");
io_err:
pm_runtime_mark_last_busy(&slave->dev);
pm_runtime_put_autosuspend(&slave->dev);
return ret; We already state the mark_last_busy at the end of the interrupts handling, so adding one more in the codec driver will do nothing. I think we need more traces to see what happens |
Cannot reproduce this bug on my side, added a test PR #2967 to try to get more traces. I've run 120+ cycles of suspend-resume and counting, no error @XiaoyunWu6666 please re-run this PR if you don't mind. |
Changing test devices, I can reproduce this bug. This is really nasty, we suspend immediately after a resume and have an interrupt after the end of the suspend without resuming.
|
It turns out this is a case where we do a system_suspend immediately after a pm_runtime resume, and we get an interrupt AFTER the system suspend. This is really interesting because we disable interrupts handling at the master level, but because of the parent-child hierarchy the child has to be suspended first, so there's a window where the child is system-suspended, but the parent isn't.
If we have an interrupt while doing a pm_runtime suspend, there is a pm_runtime_get_sync() that will force a resume, but if we do a system suspend this will have no effect. I think the only solution is to disable interrupts when we do a system suspend. Wow, this is a nice one that could happen on all devices that can send an interrupt. @bardliao @shumingfan comments welcome. |
Updated PR #2967 with tentative fix to prevent the interrupt_callback from dealing with jack detection. Not completely happy about a custom solution when this can happen for other devices as well. |
I don't have a better solution, but if we disable the interrupt we will lose the wake up feature, right? For example, a product may want to wake up the system when the headset button is pressed. |
Is it possible to remove the SET_SYSTEM_SLEEP_PM_OPS? |
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: #2943 Fixes: 5f2df2a ('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>
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. While we did not see this IO error on RT711-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. BugLink: #2943 Fixes: 501ef01 ('ASoC: rt711: wait for the delayed work to finish when the system suspends') Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
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: #2943 Fixes: 4a55000 ('ASoC: codecs: rt*.c: remove useless pointer cast') Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
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: #2943 Fixes: 7ad4d23 ('ASoC: rt711-sdca: Add RT711 SDCA vendor-specific driver') Signed-off-by: Pierre-Louis Bossart <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: #2943 Fixes: 5f2df2a ('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>
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. While we did not see this IO error on RT711-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. BugLink: #2943 Fixes: 501ef01 ('ASoC: rt711: 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>
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: #2943 Fixes: 4a55000 ('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>
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: #2943 Fixes: 7ad4d23 ('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>
Fixed by #2978 |
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: thesofproject#2943 Fixes: 5f2df2a ('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>
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. While we did not see this IO error on RT711-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. BugLink: thesofproject#2943 Fixes: 501ef01 ('ASoC: rt711: 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>
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: thesofproject#2943 Fixes: 4a55000 ('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>
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: thesofproject#2943 Fixes: 7ad4d23 ('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>
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: thesofproject#2943 Fixes: 5f2df2a ('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> Link: https://lore.kernel.org/r/20210614180815.153711-3-pierre-louis.bossart@linux.intel.com Signed-off-by: Mark Brown <broonie@kernel.org>
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. While we did not see this IO error on RT711-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. BugLink: thesofproject#2943 Fixes: 501ef01 ('ASoC: rt711: 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> Link: https://lore.kernel.org/r/20210614180815.153711-4-pierre-louis.bossart@linux.intel.com Signed-off-by: Mark Brown <broonie@kernel.org>
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: thesofproject#2943 Fixes: 4a55000 ('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>
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: thesofproject#2943 Fixes: 7ad4d23 ('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> Link: https://lore.kernel.org/r/20210614180815.153711-6-pierre-louis.bossart@linux.intel.com Signed-off-by: Mark Brown <broonie@kernel.org>
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: thesofproject/linux#2943 Fixes: 5f2df2a ('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> Link: https://lore.kernel.org/r/20210614180815.153711-3-pierre-louis.bossart@linux.intel.com Signed-off-by: Mark Brown <broonie@kernel.org> (cherry picked from commit 60888ef) BUG=b:238403794 TEST=Test Audio use cases. Signed-off-by: sboyane <shankarx.boyane@intel.com> Change-Id: I3f31ad47668e0451c29a43e333817364ef5bb0aa
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. While we did not see this IO error on RT711-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. BugLink: thesofproject/linux#2943 Fixes: 501ef01 ('ASoC: rt711: 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> Link: https://lore.kernel.org/r/20210614180815.153711-4-pierre-louis.bossart@linux.intel.com Signed-off-by: Mark Brown <broonie@kernel.org> (cherry picked from commit 1823637) BUG=b:238403794 TEST=Test Audio use cases. Signed-off-by: sboyane <shankarx.boyane@intel.com> Change-Id: I97b4b14b87356353414ff5d05ace277c830164e3
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: thesofproject/linux#2943 Fixes: 7ad4d23 ('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> Link: https://lore.kernel.org/r/20210614180815.153711-6-pierre-louis.bossart@linux.intel.com Signed-off-by: Mark Brown <broonie@kernel.org> (cherry picked from commit d2bf75f) BUG=b:238403794 TEST=Test Audio use cases. Signed-off-by: sboyane <shankarx.boyane@intel.com> Change-Id: I4758f513765c538b0a6339a0f6ad52320f1a2cb0
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: thesofproject/linux#2943 Fixes: 5f2df2a ('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> Link: https://lore.kernel.org/r/20210614180815.153711-3-pierre-louis.bossart@linux.intel.com Signed-off-by: Mark Brown <broonie@kernel.org> (cherry picked from commit 60888ef) BUG=b:238403794 TEST=Test Audio use cases. Signed-off-by: sboyane <shankarx.boyane@intel.com> Change-Id: I3f31ad47668e0451c29a43e333817364ef5bb0aa
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. While we did not see this IO error on RT711-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. BugLink: thesofproject/linux#2943 Fixes: 501ef01 ('ASoC: rt711: 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> Link: https://lore.kernel.org/r/20210614180815.153711-4-pierre-louis.bossart@linux.intel.com Signed-off-by: Mark Brown <broonie@kernel.org> (cherry picked from commit 1823637) BUG=b:238403794 TEST=Test Audio use cases. Signed-off-by: sboyane <shankarx.boyane@intel.com> Change-Id: I97b4b14b87356353414ff5d05ace277c830164e3
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: thesofproject/linux#2943 Fixes: 7ad4d23 ('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> Link: https://lore.kernel.org/r/20210614180815.153711-6-pierre-louis.bossart@linux.intel.com Signed-off-by: Mark Brown <broonie@kernel.org> (cherry picked from commit d2bf75f) BUG=b:238403794 TEST=Test Audio use cases. Signed-off-by: sboyane <shankarx.boyane@intel.com> Change-Id: I4758f513765c538b0a6339a0f6ad52320f1a2cb0
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: thesofproject/linux#2943 Fixes: 5f2df2a ('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> Link: https://lore.kernel.org/r/20210614180815.153711-3-pierre-louis.bossart@linux.intel.com Signed-off-by: Mark Brown <broonie@kernel.org> (cherry picked from commit 60888ef) BUG=b:238403794 TEST=Test Audio use cases. Signed-off-by: SHANKAR BOYANE <shankarx.boyane@intel.com> Change-Id: I3d86aaf6ee4e56abaa2cfac93044d886fedf8437
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. While we did not see this IO error on RT711-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. BugLink: thesofproject/linux#2943 Fixes: 501ef01 ('ASoC: rt711: 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> Link: https://lore.kernel.org/r/20210614180815.153711-4-pierre-louis.bossart@linux.intel.com Signed-off-by: Mark Brown <broonie@kernel.org> (cherry picked from commit 1823637) BUG=b:238403794 TEST=Test Audio use cases. Signed-off-by: SHANKAR BOYANE <shankarx.boyane@intel.com> Change-Id: I25e6d185cfdc6867659200a9059c72059f1268d4
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: thesofproject/linux#2943 Fixes: 7ad4d23 ('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> Link: https://lore.kernel.org/r/20210614180815.153711-6-pierre-louis.bossart@linux.intel.com Signed-off-by: Mark Brown <broonie@kernel.org> (cherry picked from commit d2bf75f) BUG=b:238403794 TEST=Test Audio use cases. Signed-off-by: SHANKAR BOYANE <shankarx.boyane@intel.com> Change-Id: I8d3dba2e69266e00d7dd6692aa4f33f020b4dab0
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: thesofproject/linux#2943 Fixes: 7ad4d23 ('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> Link: https://lore.kernel.org/r/20210614180815.153711-6-pierre-louis.bossart@linux.intel.com Signed-off-by: Mark Brown <broonie@kernel.org> (cherry picked from commit d2bf75f) BUG=b:238403794 TEST=Test Audio use cases. Signed-off-by: SHANKAR BOYANE <shankarx.boyane@intel.com> Change-Id: I99ccdd5e5a0ae43dd012aaf1477f3e1324169194
Describe the bug
Appeared on May 27 internal daily test , 4235 . in case check-suspend-resume-50
IO error in rt700_jack_detect_handler on CML_RVP_SDW
To Reproduce
TPLG=sof-cml-rt700-4ch.tplg ~/sof-test/test-case/check-suspend-resume.sh -l 50
Reproduction Rate
100%
Model
CML_RVP_SDW
Environment
Kernel Branch: topic/sof-dev
Kernel Commit: 6dca03a
SOF Branch: main
SOF Commit: d736888
Screenshots or console output
[console]
The text was updated successfully, but these errors were encountered: