* [PATCH 2/3] soundwire: Move handle_nested_irq outside of sdw_dev_lock
2025-06-09 14:30 [PATCH 0/3] Update SoundWire IRQ handling Charles Keepax
2025-06-09 14:30 ` [PATCH 1/3] MAINTAINERS: Remove Sanyog Kale as reviewer on SoundWire Charles Keepax
@ 2025-06-09 14:30 ` Charles Keepax
2025-06-09 14:30 ` [PATCH 3/3] ASoC: cs42l43: Remove unnecessary work functions Charles Keepax
2025-06-09 16:17 ` [PATCH 0/3] Update SoundWire IRQ handling Vinod Koul
3 siblings, 0 replies; 6+ messages in thread
From: Charles Keepax @ 2025-06-09 14:30 UTC (permalink / raw)
To: vkoul
Cc: broonie, lgirdwood, linux-sound, patches, yung-chuan.liao,
pierre-louis.bossart, peter.ujfalusi, linux-kernel
The sdw_dev_lock protects the SoundWire driver callbacks against
the probed flag, which is used to skip the callbacks if the
driver gets removed. For more information see commit bd29c00edd0a
("soundwire: revisit driver bind/unbind and callbacks").
However, this lock is a frequent source of mutex inversions.
Many audio operations eventually hit the hardware resulting in a
SoundWire callback, this means that typically the driver has the
locking order ALSA/ASoC locks -> sdw_dev_lock. Conversely, the IRQ
comes in directly from the SoundWire hardware, but then will often
want to access ALSA/ASoC, such as updating something in DAPM or
an ALSA control. This gives the other lock order sdw_dev_lock ->
ALSA/ASoC locks.
When the IRQ handling was initially added to SoundWire this was
through a callback mechanism. As such it required being covered by
the lock because the callbacks are part of the sdw_driver structure
and are thus present regardless of if the driver is currently
probed.
Since then a newer mechanism using the IRQ framework has been
added, which is currently covered by the same lock but this isn't
actually required. Handlers for the IRQ framework are registered in
probe and should by released during remove, thus the IRQ framework
will have already unbound the IRQ before the slave driver is
removed. Avoid the aforementioned mutex inversion by moving the
handle_nested_irq call outside of the sdw_dev_lock.
Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com>
---
drivers/soundwire/bus.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/soundwire/bus.c b/drivers/soundwire/bus.c
index 68db4b67a86ff..4fd5cac799c54 100644
--- a/drivers/soundwire/bus.c
+++ b/drivers/soundwire/bus.c
@@ -1753,15 +1753,15 @@ static int sdw_handle_slave_alerts(struct sdw_slave *slave)
/* Update the Slave driver */
if (slave_notify) {
+ if (slave->prop.use_domain_irq && slave->irq)
+ handle_nested_irq(slave->irq);
+
mutex_lock(&slave->sdw_dev_lock);
if (slave->probed) {
struct device *dev = &slave->dev;
struct sdw_driver *drv = drv_to_sdw_driver(dev->driver);
- if (slave->prop.use_domain_irq && slave->irq)
- handle_nested_irq(slave->irq);
-
if (drv->ops && drv->ops->interrupt_callback) {
slave_intr.sdca_cascade = sdca_cascade;
slave_intr.control_port = clear;
--
2.39.5
^ permalink raw reply related [flat|nested] 6+ messages in thread* [PATCH 3/3] ASoC: cs42l43: Remove unnecessary work functions
2025-06-09 14:30 [PATCH 0/3] Update SoundWire IRQ handling Charles Keepax
2025-06-09 14:30 ` [PATCH 1/3] MAINTAINERS: Remove Sanyog Kale as reviewer on SoundWire Charles Keepax
2025-06-09 14:30 ` [PATCH 2/3] soundwire: Move handle_nested_irq outside of sdw_dev_lock Charles Keepax
@ 2025-06-09 14:30 ` Charles Keepax
2025-06-09 15:22 ` Mark Brown
2025-06-09 16:17 ` [PATCH 0/3] Update SoundWire IRQ handling Vinod Koul
3 siblings, 1 reply; 6+ messages in thread
From: Charles Keepax @ 2025-06-09 14:30 UTC (permalink / raw)
To: vkoul
Cc: broonie, lgirdwood, linux-sound, patches, yung-chuan.liao,
pierre-louis.bossart, peter.ujfalusi, linux-kernel
Now the SoundWire IRQ lock has been changed in the core, it is no longer
necessary to use a bunch of work functions to dodge mutex inversions.
Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com>
---
sound/soc/codecs/cs42l43-jack.c | 46 +++++++++++++--------------------
sound/soc/codecs/cs42l43.c | 24 ++++-------------
sound/soc/codecs/cs42l43.h | 5 ----
3 files changed, 23 insertions(+), 52 deletions(-)
diff --git a/sound/soc/codecs/cs42l43-jack.c b/sound/soc/codecs/cs42l43-jack.c
index 6165ac16c3a95..72a4150709de8 100644
--- a/sound/soc/codecs/cs42l43-jack.c
+++ b/sound/soc/codecs/cs42l43-jack.c
@@ -362,14 +362,15 @@ static void cs42l43_stop_button_detect(struct cs42l43_codec *priv)
priv->button_detect_running = false;
}
+#define CS42L43_BUTTON_COMB_US 11000
#define CS42L43_BUTTON_COMB_MAX 512
#define CS42L43_BUTTON_ROUT 2210
-void cs42l43_button_press_work(struct work_struct *work)
+irqreturn_t cs42l43_button_press(int irq, void *data)
{
- struct cs42l43_codec *priv = container_of(work, struct cs42l43_codec,
- button_press_work.work);
+ struct cs42l43_codec *priv = data;
struct cs42l43 *cs42l43 = priv->core;
+ irqreturn_t iret = IRQ_NONE;
unsigned int buttons = 0;
unsigned int val = 0;
int i, ret;
@@ -377,7 +378,7 @@ void cs42l43_button_press_work(struct work_struct *work)
ret = pm_runtime_resume_and_get(priv->dev);
if (ret) {
dev_err(priv->dev, "Failed to resume for button press: %d\n", ret);
- return;
+ return iret;
}
mutex_lock(&priv->jack_lock);
@@ -387,6 +388,9 @@ void cs42l43_button_press_work(struct work_struct *work)
goto error;
}
+ // Wait for 2 full cycles of comb filter to ensure good reading
+ usleep_range(2 * CS42L43_BUTTON_COMB_US, 2 * CS42L43_BUTTON_COMB_US + 50);
+
regmap_read(cs42l43->regmap, CS42L43_DETECT_STATUS_1, &val);
/* Bail if jack removed, the button is irrelevant and likely invalid */
@@ -420,34 +424,27 @@ void cs42l43_button_press_work(struct work_struct *work)
snd_soc_jack_report(priv->jack_hp, buttons, CS42L43_JACK_BUTTONS);
+ iret = IRQ_HANDLED;
+
error:
mutex_unlock(&priv->jack_lock);
pm_runtime_mark_last_busy(priv->dev);
pm_runtime_put_autosuspend(priv->dev);
-}
-
-irqreturn_t cs42l43_button_press(int irq, void *data)
-{
- struct cs42l43_codec *priv = data;
-
- // Wait for 2 full cycles of comb filter to ensure good reading
- queue_delayed_work(system_wq, &priv->button_press_work,
- msecs_to_jiffies(20));
- return IRQ_HANDLED;
+ return iret;
}
-void cs42l43_button_release_work(struct work_struct *work)
+irqreturn_t cs42l43_button_release(int irq, void *data)
{
- struct cs42l43_codec *priv = container_of(work, struct cs42l43_codec,
- button_release_work);
+ struct cs42l43_codec *priv = data;
+ irqreturn_t iret = IRQ_NONE;
int ret;
ret = pm_runtime_resume_and_get(priv->dev);
if (ret) {
dev_err(priv->dev, "Failed to resume for button release: %d\n", ret);
- return;
+ return iret;
}
mutex_lock(&priv->jack_lock);
@@ -456,6 +453,8 @@ void cs42l43_button_release_work(struct work_struct *work)
dev_dbg(priv->dev, "Button release IRQ\n");
snd_soc_jack_report(priv->jack_hp, 0, CS42L43_JACK_BUTTONS);
+
+ iret = IRQ_HANDLED;
} else {
dev_dbg(priv->dev, "Spurious button release IRQ\n");
}
@@ -464,15 +463,8 @@ void cs42l43_button_release_work(struct work_struct *work)
pm_runtime_mark_last_busy(priv->dev);
pm_runtime_put_autosuspend(priv->dev);
-}
-irqreturn_t cs42l43_button_release(int irq, void *data)
-{
- struct cs42l43_codec *priv = data;
-
- queue_work(system_wq, &priv->button_release_work);
-
- return IRQ_HANDLED;
+ return iret;
}
void cs42l43_bias_sense_timeout(struct work_struct *work)
@@ -787,8 +779,6 @@ irqreturn_t cs42l43_tip_sense(int irq, void *data)
cancel_delayed_work(&priv->bias_sense_timeout);
cancel_delayed_work(&priv->tip_sense_work);
- cancel_delayed_work(&priv->button_press_work);
- cancel_work(&priv->button_release_work);
// Ensure delay after suspend is long enough to avoid false detection
if (priv->suspend_jack_debounce)
diff --git a/sound/soc/codecs/cs42l43.c b/sound/soc/codecs/cs42l43.c
index ea84ac64c775e..41a0f4529ea1f 100644
--- a/sound/soc/codecs/cs42l43.c
+++ b/sound/soc/codecs/cs42l43.c
@@ -167,13 +167,14 @@ static void cs42l43_hp_ilimit_clear_work(struct work_struct *work)
snd_soc_dapm_mutex_unlock(dapm);
}
-static void cs42l43_hp_ilimit_work(struct work_struct *work)
+static irqreturn_t cs42l43_hp_ilimit(int irq, void *data)
{
- struct cs42l43_codec *priv = container_of(work, struct cs42l43_codec,
- hp_ilimit_work);
+ struct cs42l43_codec *priv = data;
struct snd_soc_dapm_context *dapm = snd_soc_component_get_dapm(priv->component);
struct cs42l43 *cs42l43 = priv->core;
+ dev_dbg(priv->dev, "headphone ilimit IRQ\n");
+
snd_soc_dapm_mutex_lock(dapm);
if (priv->hp_ilimit_count < CS42L43_HP_ILIMIT_MAX_COUNT) {
@@ -183,7 +184,7 @@ static void cs42l43_hp_ilimit_work(struct work_struct *work)
priv->hp_ilimit_count++;
snd_soc_dapm_mutex_unlock(dapm);
- return;
+ return IRQ_HANDLED;
}
dev_err(priv->dev, "Disabling headphone for %dmS, due to frequent current limit\n",
@@ -218,15 +219,6 @@ static void cs42l43_hp_ilimit_work(struct work_struct *work)
priv->hp_ilimited = false;
snd_soc_dapm_mutex_unlock(dapm);
-}
-
-static irqreturn_t cs42l43_hp_ilimit(int irq, void *data)
-{
- struct cs42l43_codec *priv = data;
-
- dev_dbg(priv->dev, "headphone ilimit IRQ\n");
-
- queue_work(system_long_wq, &priv->hp_ilimit_work);
return IRQ_HANDLED;
}
@@ -2159,10 +2151,7 @@ static void cs42l43_component_remove(struct snd_soc_component *component)
cancel_delayed_work_sync(&priv->bias_sense_timeout);
cancel_delayed_work_sync(&priv->tip_sense_work);
- cancel_delayed_work_sync(&priv->button_press_work);
- cancel_work_sync(&priv->button_release_work);
- cancel_work_sync(&priv->hp_ilimit_work);
cancel_delayed_work_sync(&priv->hp_ilimit_clear_work);
priv->component = NULL;
@@ -2314,10 +2303,7 @@ static int cs42l43_codec_probe(struct platform_device *pdev)
INIT_DELAYED_WORK(&priv->tip_sense_work, cs42l43_tip_sense_work);
INIT_DELAYED_WORK(&priv->bias_sense_timeout, cs42l43_bias_sense_timeout);
- INIT_DELAYED_WORK(&priv->button_press_work, cs42l43_button_press_work);
INIT_DELAYED_WORK(&priv->hp_ilimit_clear_work, cs42l43_hp_ilimit_clear_work);
- INIT_WORK(&priv->button_release_work, cs42l43_button_release_work);
- INIT_WORK(&priv->hp_ilimit_work, cs42l43_hp_ilimit_work);
pm_runtime_set_autosuspend_delay(priv->dev, 100);
pm_runtime_use_autosuspend(priv->dev);
diff --git a/sound/soc/codecs/cs42l43.h b/sound/soc/codecs/cs42l43.h
index 1cd9d8a71c439..3ea36362b11a4 100644
--- a/sound/soc/codecs/cs42l43.h
+++ b/sound/soc/codecs/cs42l43.h
@@ -88,8 +88,6 @@ struct cs42l43_codec {
struct delayed_work tip_sense_work;
struct delayed_work bias_sense_timeout;
- struct delayed_work button_press_work;
- struct work_struct button_release_work;
struct completion type_detect;
struct completion load_detect;
@@ -99,7 +97,6 @@ struct cs42l43_codec {
int jack_override;
bool suspend_jack_debounce;
- struct work_struct hp_ilimit_work;
struct delayed_work hp_ilimit_clear_work;
bool hp_ilimited;
int hp_ilimit_count;
@@ -134,8 +131,6 @@ int cs42l43_set_jack(struct snd_soc_component *component,
struct snd_soc_jack *jack, void *d);
void cs42l43_bias_sense_timeout(struct work_struct *work);
void cs42l43_tip_sense_work(struct work_struct *work);
-void cs42l43_button_press_work(struct work_struct *work);
-void cs42l43_button_release_work(struct work_struct *work);
irqreturn_t cs42l43_bias_detect_clamp(int irq, void *data);
irqreturn_t cs42l43_button_press(int irq, void *data);
irqreturn_t cs42l43_button_release(int irq, void *data);
--
2.39.5
^ permalink raw reply related [flat|nested] 6+ messages in thread