[alsa-devel] ALSA Control Questions
I've got some questions concerning the ALSA control interface...
1. What is a "simple" control? I would like to use the control interface to set more than just volume (e.g. I would like to set input gain, output attenuation, input channel selection, output channel selection, output channel activation...), so I am wondering if I should use SNDRV_CTL_ELEM_IFACE_CARD or SNDRV_CTL_ELEM_IFACE_MIXER, as they are obviously in context with "simple" control...
2. The more interesting question: as written in "Writing an alsa driver" callbacks are basically not atomic, but I am getting BUG: scheduling while atomic: amixer/0x00000001/430 Call Trace: .[..] in "put" callback?? So what is meant with "basically not atomic"?? What is allowed and what is not allowed, am I able to get hardware interrupts in this context?
On Thu, Dec 03, 2009 at 07:30:38PM +0100, Tobias Schneider wrote:
- The more interesting question: as written in "Writing an alsa driver"
callbacks are basically not atomic, but I am getting BUG: scheduling while atomic: amixer/0x00000001/430 Call Trace: .[..] in "put" callback?? So what is meant with "basically not atomic"?? What is allowed and what is not allowed, am I able to get hardware interrupts in this context?
That message basically means that you called a scheduling function while you have preemption switched off. One common pitfall is to let the CPU reschedule from an interrupt handler or while holding a spinlock.
The basic problem is that if you hold a spinlock and let the CPU do something else, some other execution path (on the same CPU) could also try to access the same lock which then deadlocks your system.
It all boils down to: from interrupt handlers or while you hold a spinlock, you must never call any function that will sleep (and thereby reschedule). Some functions such like kmalloc have a flag that you can pass depending on whether you want it to allow to sleep (GFP_KERNEL) or not (GFP_ATOMIC). If you necessarily _have_ to sleep from atomic context, you must postpone your work using a work_struct or something similar.
You should post your code so people can have a look.
HTH, Daniel
Thanks for the reply.
Actually I know where the problem in my code is: inside the put callback (see below) I am calling a function (dsp_setoutput) that is sending the signal to the DSP, there I wanted to wait for the reply of the DSP to get a feedback whether the request was successful or not. This was done by a semaphore (going down and waiting for IRQ from DSP that is setting sema up), and thats the point where the posted bug occured.
So I was just wondering that it seems that there is no possibility to wait for a reply of the DSP, to be sure that a given value has been set?
(for the interested ones, here is the code..)
static int snd_mychip_output_set_put(struct snd_kcontrol *kcontrol, struct snd_ctl_elem_value *ucontrol) { struct snd_mychip *mychip = snd_kcontrol_chip(kcontrol); int addr = kcontrol->private_value; short change = 0;
spin_lock_irq(&mychip->mixer_lock); if (ucontrol->value.integer.value[0] != mychip->output_set[addr]) { if (dsp_setoutput(addr+1, ucontrol->value.integer.value[0]) >= 0) { mychip->output_set[addr] = ucontrol->value.integer.value[0]; change = 1; } else // error sending signal { change = -EBUSY; // device or resource busy } } spin_unlock_irq(&mychip->mixer_lock); return change; }
later on in the called function... // wait for reply via semaphore while(!end) { if (down_interruptible(&dsp_reply_sema)==-EINTR) // HERE WE GET BUG: scheduling while atomic: amixer/0x00000001/430 ... // waked by signal (terminate), leave thread printk(INFO "reply: catched signal\n"); end=1; break; } printk(INFO "dsp_reply signaled (saved status = %i)\n",stDspReply.status); end=1; ... }
Another point: I figured out that the "ALSA middlelayer" seems to filter the given values of a control. So if the user sets value=1000 where maximum is 10, I will get 10 instead of 1000 in the put callback...so it's not necessary to check values in those callbacks, but unfortunately that's not always good - because sometimes it doesn't make sense to use the max or min values if the user just entered a wrong number...
thanks so far
On Fri, Dec 04, 2009 at 10:31:06AM +0100, Tobias Schneider wrote:
Actually I know where the problem in my code is: inside the put callback (see below) I am calling a function (dsp_setoutput) that is sending the signal to the DSP, there I wanted to wait for the reply of the DSP to get a feedback whether the request was successful or not. This was done by a semaphore (going down and waiting for IRQ from DSP that is setting sema up), and thats the point where the posted bug occured.
So I was just wondering that it seems that there is no possibility to wait for a reply of the DSP, to be sure that a given value has been set?
(for the interested ones, here is the code..)
static int snd_mychip_output_set_put(struct snd_kcontrol *kcontrol, struct snd_ctl_elem_value *ucontrol) { struct snd_mychip *mychip = snd_kcontrol_chip(kcontrol); int addr = kcontrol->private_value; short change = 0;
spin_lock_irq(&mychip->mixer_lock);
As I said - here you're holding a spinlock ...
if (ucontrol->value.integer.value[0] != mychip->output_set[addr]) { if (dsp_setoutput(addr+1, ucontrol->value.integer.value[0]) >= 0) { mychip->output_set[addr] = ucontrol->value.integer.value[0]; change = 1; } else // error sending signal { change = -EBUSY; // device or resource busy } } spin_unlock_irq(&mychip->mixer_lock); return change; }
later on in the called function... // wait for reply via semaphore while(!end) { if (down_interruptible(&dsp_reply_sema)==-EINTR) // HERE WE GET
And here you allow the CPU to reschedule while waiting for a mutex. That doesn't fly. You should remove the spinlock from the function above and move it to the actual lowlevel I/O function (whatever dsp_setoutput() does eventually).
Think about where locking is actually needed, at which point your function flow must not be interrupted by anything to avoid data corruption. Use locking there, and remove it from other places.
This might also be a good read to get an overview:
http://www.kernel.org/pub/linux/kernel/people/rusty/kernel-locking/index.htm...
Another point: I figured out that the "ALSA middlelayer" seems to filter the given values of a control. So if the user sets value=1000 where maximum is 10, I will get 10 instead of 1000 in the put callback...so it's not necessary to check values in those callbacks, but unfortunately that's not always good - because sometimes it doesn't make sense to use the max or min values if the user just entered a wrong number...
For integer values, it does make sense to clamp values to their limits. For enums, that might be different. Not sure what the ALSA core does there.
Daniel
On Fri, Dec 04, 2009 at 10:31:06AM +0100, Tobias Schneider wrote:
(for the interested ones, here is the code..)
One more thing: in case you intend to bring your code mainline, please run scripts/checkpatch.pl on the patches before you submit. The sniplet you posted has a number of style issues that should be fixed. In particular, pay attention to indentation, comment style and parenthesis positions - checkpatch.pl will help pointing out most if them.
Daniel
static int snd_mychip_output_set_put(struct snd_kcontrol *kcontrol, struct snd_ctl_elem_value *ucontrol) { struct snd_mychip *mychip = snd_kcontrol_chip(kcontrol); int addr = kcontrol->private_value; short change = 0;
spin_lock_irq(&mychip->mixer_lock); if (ucontrol->value.integer.value[0] != mychip->output_set[addr]) { if (dsp_setoutput(addr+1, ucontrol->value.integer.value[0]) >= 0) { mychip->output_set[addr] = ucontrol->value.integer.value[0]; change = 1; } else // error sending signal { change = -EBUSY; // device or resource busy } } spin_unlock_irq(&mychip->mixer_lock); return change; }
later on in the called function... // wait for reply via semaphore while(!end) { if (down_interruptible(&dsp_reply_sema)==-EINTR) // HERE WE GET BUG: scheduling while atomic: amixer/0x00000001/430 ... // waked by signal (terminate), leave thread printk(INFO "reply: catched signal\n"); end=1; break; } printk(INFO "dsp_reply signaled (saved status = %i)\n",stDspReply.status); end=1; ... }
okay, so it was my fault, thanks!
One more thing: in case you intend to bring your code mainline, please run scripts/checkpatch.pl on the patches before you submit. The sniplet you posted has a number of style issues that should be fixed. In particular, pay attention to indentation, comment style and parenthesis positions - checkpatch.pl will help pointing out most if them.
Daniel
is there also any document (something like ALSA coding style guide)?
so far, thanks a lot Tobias
On Fri, Dec 04, 2009 at 01:02:35PM +0100, Tobias Schneider wrote:
One more thing: in case you intend to bring your code mainline, please run scripts/checkpatch.pl on the patches before you submit. The sniplet you posted has a number of style issues that should be fixed. In particular, pay attention to indentation, comment style and parenthesis positions - checkpatch.pl will help pointing out most if them.
Daniel
is there also any document (something like ALSA coding style guide)?
It's not ALSA specific but applies (or should apply) to all kernel code. Read Documentation/CodingStyle :)
Daniel
Tobias Schneider wrote:
Another point: I figured out that the "ALSA middlelayer" seems to filter the given values of a control. So if the user sets value=1000 where maximum is 10, I will get 10 instead of 1000 in the put callback...so it's not necessary to check values in those callbacks,
It is, because it is possible to use low-level ALSA functions or to call the IOCTLs directly, and in this way arbitrary values can reach your driver's put callback.
Best regards, Clemens
participants (3)
-
Clemens Ladisch -
Daniel Mack -
Tobias Schneider