On 7/4/2023 10:07 AM, Takashi Iwai wrote:
On Mon, 03 Jul 2023 16:18:27 +0200,
Dan Carpenter wrote:
Hello Amadeusz Sławiński,
The patch 1b6a6fc5280e: "ALSA: jack: Access input_dev under mutex"
from Apr 12, 2022, leads to the following Smatch static checker
warning:
sound/core/jack.c:673 snd_jack_report()
warn: sleeping in atomic context
sound/core/jack.c
663 jack->hw_status_cache = status;
664
665 list_for_each_entry(jack_kctl, &jack->kctl_list, list)
666 if (jack_kctl->sw_inject_enable)
667 mask_bits |= jack_kctl->mask_bits;
668 else
669 snd_kctl_jack_report(jack->card, jack_kctl->kctl,
670 status & jack_kctl->mask_bits);
671
672 #ifdef CONFIG_SND_JACK_INPUT_DEV
--> 673 mutex_lock(&jack->input_dev_lock);
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
That patch adds this mutex but we can't take mutex because we're already
holding a spinlock. The problematic call trees are:
virtsnd_event_notify_cb() <- disables preempt
virtsnd_disable_event_vq() <- disables preempt
-> virtsnd_event_dispatch()
-> virtsnd_jack_event()
-> snd_jack_report()
The virtsnd_event_notify_cb() and virtsnd_disable_event_vq() functions
take the spin_lock_irqsave(&queue->lock, flags);
Indeed it was no good choice to use the mutex there inside the report
function. It's supposed to be callable from an irq-disabled context,
too.
How about simply using the device refcount like below?
Although we may drop the mutex from snd_jack, it can can be left, as
it's still useful for protecting a potential race between creation and
deletion.
thanks,
Takashi
-- 8< --
--- a/sound/core/jack.c
+++ b/sound/core/jack.c
@@ -654,6 +654,7 @@ void snd_jack_report(struct snd_jack *jack, int status)
struct snd_jack_kctl *jack_kctl;
unsigned int mask_bits = 0;
#ifdef CONFIG_SND_JACK_INPUT_DEV
+ struct input_dev *idev;
int i;
#endif
@@ -670,17 +671,15 @@ void snd_jack_report(struct snd_jack *jack, int status)
status & jack_kctl->mask_bits);
#ifdef CONFIG_SND_JACK_INPUT_DEV
- mutex_lock(&jack->input_dev_lock);
- if (!jack->input_dev) {
- mutex_unlock(&jack->input_dev_lock);
+ idev = input_get_device(jack->input_dev);
+ if (!idev)
return;
- }
for (i = 0; i < ARRAY_SIZE(jack->key); i++) {
int testbit = ((SND_JACK_BTN_0 >> i) & ~mask_bits);
if (jack->type & testbit)
- input_report_key(jack->input_dev, jack->key[i],
+ input_report_key(idev, jack->key[i],
status & testbit);
}
@@ -688,13 +687,13 @@ void snd_jack_report(struct snd_jack *jack, int status)
int testbit = ((1 << i) & ~mask_bits);
if (jack->type & testbit)
- input_report_switch(jack->input_dev,
+ input_report_switch(idev,
jack_switch_types[i],
status & testbit);
}
- input_sync(jack->input_dev);
- mutex_unlock(&jack->input_dev_lock);
+ input_sync(idev);
+ input_put_device(idev);
#endif /* CONFIG_SND_JACK_INPUT_DEV */
}
EXPORT_SYMBOL(snd_jack_report);
Looking at code it looks like it should also work. Will schedule test
run tomorrow to see if it causes any problems.
