On Thu, 8 Mar 2007 14:52:07 -0800 Luong Ngo wrote:
[...]
> static irqreturn board_isr(int irq, void *dev_id, struct pt_regs* regs)
> {
> spin_lock(&dev->lock);
> if (dev->irqMask & (1 << irqBit)) {
> // Set the interrupt event mask
> dev->irqEvent |= (1 << irqBit);
>
> // Disable this irq, it will be reenabled after processed by board task
> disable_irq(irq);
I assume that your device does not support shared interrupts? If it
does (and a PCI device is required to support them), you cannot use
disable_irq() here (and you need to check a register in the device to
find out if it really did generate an IRQ)...
> // Wake up Board thread that calling IOCTL
> wake_up(&(dev->boardIRQWaitQueue));
> }
> spin_unlock(&dev->lock);
>
> return IRQ_HANDLED;
...and return IRQ_NONE here if the IRQ is not from your device.
>
> }
>
> static int ats89_ioctl(struct inode *inode, struct file *file, u_int
> cmd, u_long arg)
> {
>
> switch(cmd){
> case GET_IRQ_CMD: {
> u32 regMask32;
>
> spin_lock_irq(dev->lock);
> while ((dev->irqMask & dev->irqEvent) == 0) {
> // Sleep until board interrupt happens
> spin_unlock_irq(dev->lock);
> interruptible_sleep_on(&(dev->boardIRQWaitQueue));
> if (uncond_wakeup) {
> /* don't go back to loop */
> break;
> }
> spin_lock_irq(dev->lock);
> }
>
> uncond_wakeup = 0;
>
> // Board interrupt happened
> regMask32 = dev->irqMask & dev->irqEvent;
> if(copy_to_user(&(((ATS89_IOCTL_S *)arg)->mask32),
> ®Mask32, sizeof(u32))) {
> spin_unlock_irq(dev->lock);
> return -EAGAIN;
> }
>
> // Clear the event mask
> dev->irqEvent = 0;
> spin_unlock_irq(dev->lock);
> }
> break;
>
>
> }
> }
And this code is full of bugs:
1) As you have been told already, interruptible_sleep_on() and
sleep_on() functions are broken and should not be used (they are
left in the kernel only to support some obsolete code). Either
use wait_event_interruptible() or work with wait queues directly
(prepare_to_wait(), finish_wait(), ...).
2) The code to handle pending signals is missing - you need to have
this after wait_event_interruptible():
if (signal_pending(current))
return -ERESTARTSYS;
(but be careful - you might need to clean up something before
returning).
This is what causes your problem - interruptible_sleep_on()
returns if a signal is pending, but your code does not check for
signals and therefore invokes interruptible_sleep_on() again; but
if a signal is pending, interruptible_sleep_on() returns
immediately, causing your driver to eat 100% CPU looping in kernel
mode until some device event finally happens.
3) If uncond_wakeup is set, you break out of the loop with dev->lock
unlocked; however, if dev->irqEvent gets set, you exit the loop
with dev->lock locked. The subsequent code always unlocks
dev->lock, so in the uncond_wakeup case you have double unlock.
4) You are doing copy_to_user() while holding a spinlock - this is
prohibited (as any other form of sleep inside a spinlock).
5) The return code for the copy_to_user() failure is wrong - it
should be -EFAULT (this is not a fatal bug, but an annoyance for
users of your driver, who might get such nonstandard error codes
while debugging their programs and wonder what is going on).
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
