From fa6a0c0c1dd53b3949ca56bf7213648dfd6a62ee Mon Sep 17 00:00:00 2001 From: Hans de Goede Date: Tue, 12 Sep 2023 13:32:40 +0200 Subject: ASoC: rt5640: Revert "Fix sleep in atomic context" Commit 70a6404ff610 ("ASoC: rt5640: Fix sleep in atomic context") not only switched from request_irq() to request_threaded_irq(), to fix the sleep in atomic context issue, but it also added devm management of the IRQ by actually switching to devm_request_threaded_irq() (without any explanation in the commit message for this change). This is wrong since the IRQ was already explicitly managed by the driver. On unbind the ASoC core will call rt5640_set_jack(NULL) which in turn will call rt5640_disable_jack_detect() which frees the IRQ already. So now we have a double free. Besides the unexplained switch to devm being wrong, the actual fix for the sleep in atomic context issue also is not the best solution. The only thing which rt5640_irq() does is cancel + (re-)queue the jack_work delayed_work. This can be done in a single non sleeping call by replacing queue_delayed_work() with mod_delayed_work(), which does not sleep. Using mod_delayed_work() is a much better fix then adding a thread which does nothing other then queuing a work-item. This patch is a straight revert of the troublesome changes, the switch to mod_delayed_work() is done in a separate follow-up patch. Fixes: 70a6404ff610 ("ASoC: rt5640: Fix sleep in atomic context") Cc: Sameer Pujar Cc: Oder Chiou Signed-off-by: Hans de Goede Link: https://lore.kernel.org/r/20230912113245.320159-2-hdegoede@redhat.com Signed-off-by: Mark Brown --- sound/soc/codecs/rt5640.c | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) (limited to 'sound') diff --git a/sound/soc/codecs/rt5640.c b/sound/soc/codecs/rt5640.c index 15e1a62b9e57..05ff8066171b 100644 --- a/sound/soc/codecs/rt5640.c +++ b/sound/soc/codecs/rt5640.c @@ -2565,10 +2565,9 @@ static void rt5640_enable_jack_detect(struct snd_soc_component *component, if (jack_data && jack_data->use_platform_clock) rt5640->use_platform_clock = jack_data->use_platform_clock; - ret = devm_request_threaded_irq(component->dev, rt5640->irq, - NULL, rt5640_irq, - IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING | IRQF_ONESHOT, - "rt5640", rt5640); + ret = request_irq(rt5640->irq, rt5640_irq, + IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING | IRQF_ONESHOT, + "rt5640", rt5640); if (ret) { dev_warn(component->dev, "Failed to request IRQ %d: %d\n", rt5640->irq, ret); rt5640_disable_jack_detect(component); @@ -2621,9 +2620,8 @@ static void rt5640_enable_hda_jack_detect( rt5640->jack = jack; - ret = devm_request_threaded_irq(component->dev, rt5640->irq, - NULL, rt5640_irq, IRQF_TRIGGER_RISING | IRQF_ONESHOT, - "rt5640", rt5640); + ret = request_irq(rt5640->irq, rt5640_irq, + IRQF_TRIGGER_RISING | IRQF_ONESHOT, "rt5640", rt5640); if (ret) { dev_warn(component->dev, "Failed to request IRQ %d: %d\n", rt5640->irq, ret); rt5640->irq = -ENXIO; -- cgit v1.2.3-70-g09d2 From df7d595f6bd9dc96cc275cc4b0f313fcfa423c58 Mon Sep 17 00:00:00 2001 From: Hans de Goede Date: Tue, 12 Sep 2023 13:32:41 +0200 Subject: ASoC: rt5640: Fix sleep in atomic context Following prints are observed while testing audio on Jetson AGX Orin which has onboard RT5640 audio codec: BUG: sleeping function called from invalid context at kernel/workqueue.c:3027 in_atomic(): 1, irqs_disabled(): 128, non_block: 0, pid: 0, name: swapper/0 preempt_count: 10001, expected: 0 RCU nest depth: 0, expected: 0 ------------[ cut here ]------------ WARNING: CPU: 0 PID: 0 at kernel/irq/handle.c:159 __handle_irq_event_percpu+0x1e0/0x270 ---[ end trace ad1c64905aac14a6 ]- The IRQ handler rt5640_irq() runs in interrupt context and can sleep during cancel_delayed_work_sync(). The only thing which rt5640_irq() does is cancel + (re-)queue the jack_work delayed_work. This can be done in a single non sleeping call by replacing queue_delayed_work() with mod_delayed_work(), avoiding the sleep in atomic context. Fixes: 051dade34695 ("ASoC: rt5640: Fix the wrong state of JD1 and JD2") Reported-by: Sameer Pujar Closes: https://lore.kernel.org/r/1688015537-31682-4-git-send-email-spujar@nvidia.com Cc: Oder Chiou Signed-off-by: Hans de Goede Link: https://lore.kernel.org/r/20230912113245.320159-3-hdegoede@redhat.com Signed-off-by: Mark Brown --- sound/soc/codecs/rt5640.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) (limited to 'sound') diff --git a/sound/soc/codecs/rt5640.c b/sound/soc/codecs/rt5640.c index 05ff8066171b..5c34c045d396 100644 --- a/sound/soc/codecs/rt5640.c +++ b/sound/soc/codecs/rt5640.c @@ -2403,13 +2403,11 @@ static irqreturn_t rt5640_irq(int irq, void *data) struct rt5640_priv *rt5640 = data; int delay = 0; - if (rt5640->jd_src == RT5640_JD_SRC_HDA_HEADER) { - cancel_delayed_work_sync(&rt5640->jack_work); + if (rt5640->jd_src == RT5640_JD_SRC_HDA_HEADER) delay = 100; - } if (rt5640->jack) - queue_delayed_work(system_long_wq, &rt5640->jack_work, delay); + mod_delayed_work(system_long_wq, &rt5640->jack_work, delay); return IRQ_HANDLED; } -- cgit v1.2.3-70-g09d2 From 786120ebb649b166021f0212250e8627e53d068a Mon Sep 17 00:00:00 2001 From: Hans de Goede Date: Tue, 12 Sep 2023 13:32:42 +0200 Subject: ASoC: rt5640: Do not disable/enable IRQ twice on suspend/resume When jack-detect was originally added disabling the IRQ during suspend was done by the sound/soc/intel/boards/bytcr_rt5640.c driver calling snd_soc_component_set_jack(NULL) on suspend, which calls rt5640_disable_jack_detect(), which calls free_irq() which also disables it. Commit 5fabcc90e79b ("ASoC: rt5640: Fix Jack work after system suspend") added disable_irq() / enable_irq() calls on suspend/resume for machine drivers which do not call snd_soc_component_set_jack(NULL) on suspend. The new disable_irq() / enable_irq() are made conditional by "if (rt5640->irq)" statements, but this is true for the machine drivers which do call snd_soc_component_set_jack(NULL) on suspend too, causing a disable_irq() call there on the already free-ed IRQ. Change the "if (rt5640->irq)" condition to "if (rt5640->jack)" to fix this, rt5640->jack is only set if the jack-detect IRQ handler is still active when rt5640_suspend() runs. And adjust rt5640_enable_hda_jack_detect()'s request_irq() error handling to set rt5640->jack to NULL to match (note that the old setting of irq to -ENOXIO still resulted in disable_irq(-ENOXIO) calls on suspend). Fixes: 5fabcc90e79b ("ASoC: rt5640: Fix Jack work after system suspend") Cc: Oder Chiou Signed-off-by: Hans de Goede Link: https://lore.kernel.org/r/20230912113245.320159-4-hdegoede@redhat.com Signed-off-by: Mark Brown --- sound/soc/codecs/rt5640.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) (limited to 'sound') diff --git a/sound/soc/codecs/rt5640.c b/sound/soc/codecs/rt5640.c index 5c34c045d396..1bc281d42ca8 100644 --- a/sound/soc/codecs/rt5640.c +++ b/sound/soc/codecs/rt5640.c @@ -2622,7 +2622,7 @@ static void rt5640_enable_hda_jack_detect( IRQF_TRIGGER_RISING | IRQF_ONESHOT, "rt5640", rt5640); if (ret) { dev_warn(component->dev, "Failed to request IRQ %d: %d\n", rt5640->irq, ret); - rt5640->irq = -ENXIO; + rt5640->jack = NULL; return; } @@ -2797,7 +2797,7 @@ static int rt5640_suspend(struct snd_soc_component *component) { struct rt5640_priv *rt5640 = snd_soc_component_get_drvdata(component); - if (rt5640->irq) { + if (rt5640->jack) { /* disable jack interrupts during system suspend */ disable_irq(rt5640->irq); } @@ -2825,10 +2825,9 @@ static int rt5640_resume(struct snd_soc_component *component) regcache_cache_only(rt5640->regmap, false); regcache_sync(rt5640->regmap); - if (rt5640->irq) + if (rt5640->jack) { enable_irq(rt5640->irq); - if (rt5640->jack) { if (rt5640->jd_src == RT5640_JD_SRC_HDA_HEADER) { snd_soc_component_update_bits(component, RT5640_DUMMY2, 0x1100, 0x1100); -- cgit v1.2.3-70-g09d2 From b5e85e535551bf82242aa5896e14a136ed3c156d Mon Sep 17 00:00:00 2001 From: Hans de Goede Date: Tue, 12 Sep 2023 13:32:43 +0200 Subject: ASoC: rt5640: Enable the IRQ on resume after configuring jack-detect The jack-detect IRQ should be enabled *after* the jack-detect related configuration registers have been programmed. Move the enable_irq() call for this to after the register setup. Fixes: 5fabcc90e79b ("ASoC: rt5640: Fix Jack work after system suspend") Cc: Oder Chiou Signed-off-by: Hans de Goede Link: https://lore.kernel.org/r/20230912113245.320159-5-hdegoede@redhat.com Signed-off-by: Mark Brown --- sound/soc/codecs/rt5640.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) (limited to 'sound') diff --git a/sound/soc/codecs/rt5640.c b/sound/soc/codecs/rt5640.c index 1bc281d42ca8..03c866c04c7a 100644 --- a/sound/soc/codecs/rt5640.c +++ b/sound/soc/codecs/rt5640.c @@ -2826,8 +2826,6 @@ static int rt5640_resume(struct snd_soc_component *component) regcache_sync(rt5640->regmap); if (rt5640->jack) { - enable_irq(rt5640->irq); - if (rt5640->jd_src == RT5640_JD_SRC_HDA_HEADER) { snd_soc_component_update_bits(component, RT5640_DUMMY2, 0x1100, 0x1100); @@ -2854,6 +2852,7 @@ static int rt5640_resume(struct snd_soc_component *component) } } + enable_irq(rt5640->irq); queue_delayed_work(system_long_wq, &rt5640->jack_work, 0); } -- cgit v1.2.3-70-g09d2 From 8c8bf3df6b7c0ed1c4dd373b23eb0ce13a63f452 Mon Sep 17 00:00:00 2001 From: Hans de Goede Date: Tue, 12 Sep 2023 13:32:44 +0200 Subject: ASoC: rt5640: Fix IRQ not being free-ed for HDA jack detect mode Set "rt5640->irq_requested = true" after a successful request_irq() in rt5640_enable_hda_jack_detect(), so that rt5640_disable_jack_detect() properly frees the IRQ. This fixes the IRQ not being freed on rmmod / driver unbind. Fixes: 2b9c8d2b3c89 ("ASoC: rt5640: Add the HDA header support") Cc: Oder Chiou Signed-off-by: Hans de Goede Link: https://lore.kernel.org/r/20230912113245.320159-6-hdegoede@redhat.com Signed-off-by: Mark Brown --- sound/soc/codecs/rt5640.c | 1 + 1 file changed, 1 insertion(+) (limited to 'sound') diff --git a/sound/soc/codecs/rt5640.c b/sound/soc/codecs/rt5640.c index 03c866c04c7a..a4a11407ab10 100644 --- a/sound/soc/codecs/rt5640.c +++ b/sound/soc/codecs/rt5640.c @@ -2625,6 +2625,7 @@ static void rt5640_enable_hda_jack_detect( rt5640->jack = NULL; return; } + rt5640->irq_requested = true; /* sync initial jack state */ queue_delayed_work(system_long_wq, &rt5640->jack_work, 0); -- cgit v1.2.3-70-g09d2 From 8fc7cc507d61fc655172836c74fb7fcc8b7a978b Mon Sep 17 00:00:00 2001 From: Hans de Goede Date: Tue, 12 Sep 2023 13:32:45 +0200 Subject: ASoC: rt5640: Only cancel jack-detect work on suspend if active If jack-detection is not used; or has already been disabled then there is no need to call rt5640_cancel_work(). Move the rt5640_cancel_work() inside the "if (rt5640->jack) {}" block, grouping it together with the disabling of the IRQ which queues the work in the first place. This also makes suspend() symetrical with resume() which re-queues the work in an "if (rt5640->jack) {}" block. Cc: Oder Chiou Signed-off-by: Hans de Goede Link: https://lore.kernel.org/r/20230912113245.320159-7-hdegoede@redhat.com Signed-off-by: Mark Brown --- sound/soc/codecs/rt5640.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'sound') diff --git a/sound/soc/codecs/rt5640.c b/sound/soc/codecs/rt5640.c index a4a11407ab10..e8cdc166bdaa 100644 --- a/sound/soc/codecs/rt5640.c +++ b/sound/soc/codecs/rt5640.c @@ -2801,9 +2801,9 @@ static int rt5640_suspend(struct snd_soc_component *component) if (rt5640->jack) { /* disable jack interrupts during system suspend */ disable_irq(rt5640->irq); + rt5640_cancel_work(rt5640); } - rt5640_cancel_work(rt5640); snd_soc_component_force_bias_level(component, SND_SOC_BIAS_OFF); rt5640_reset(component); regcache_cache_only(rt5640->regmap, true); -- cgit v1.2.3-70-g09d2