Re: [CHECKER] 28 potential interrupt errors

[Andrew Morton <andrewm@uow.edu.au>]

Junfeng Yang wrote:
> 
> ...
> 
> [BUG] sleep_or_timeout will call interruptible_sleep_on, which will save disabled flags and then restore them.
> 
> /u2/acc/oses/linux/2.4.1/drivers/cdrom/cm206.c:474:send_command: ERROR:INTR:462:474: Interrupts inconsistent, severity `20':474
> 
>   if (!(inw(r_line_status) & ls_transmitter_buffer_empty)) {
>     cd->command = command;
> Start --->
>     cli();                      /* don't interrupt before sleep */
>     outw(dc_mask_sync_error | dc_no_stop_on_error |
>          (inw(r_data_status) & 0x7f), r_data_control);
>     /* interrupt routine sends command */
> 
> Save & Restore
> flags here --->
>     if (sleep_or_timeout(&cd->uart, UART_TIMEOUT)) {
>       debug(("Time out on write-buffer\n"));
>       stats(write_timeout);
> 
>         ... DELETED 2 lines ...
> 
>     }
>     debug(("Write commmand delayed\n"));
>   }
>   else outw(command, r_uart_transmit);
> Error --->
> }

Yes, this is a bug.

> ...

> /u2/acc/oses/linux/2.4.1/drivers/net/irda/irport.c:943:irport_net_ioctl: ERROR:INTR:947:997: Interrupts inconsistent, severity `20':997
> 
>         /* Disable interrupts & save flags */
>         save_flags(flags);
> Start --->
>         cli();
> 
>         switch (cmd) {
>         case SIOCSBANDWIDTH: /* Set bandwidth */
>                 if (!capable(CAP_NET_ADMIN))
>                         return -EPERM;
>                 irda_task_execute(self, __irport_change_speed, NULL, NULL,
> 
>         ... DELETED 40 lines ...
> 
>         }
> 
>         restore_flags(flags);
> 
> Error --->
>         return ret;
> }

Your reporting is a little misleading here.

Yes, there's a bug in this function - the `return -EPERM'
doesn't do a `restore_flags()'.  But there is no bug
in the place you've reported.

(Personally, I think *any* C function which has more than
 one `return' statement is a bug, and we see a classic
 instance here of one of the problems which this practice
 can cause.  Religious issue. )


> ...

> [BUG] error path
> 
> /u2/acc/oses/linux/2.4.1/drivers/net/wan/comx-hw-mixcom.c:505:MIXCOM_open: ERROR:INTR:514:562: Interrupts inconsistent, severity `20':562
> 
>         }
> 
> Start --->
>         save_flags(flags); cli();
> 
>         if(hw->channel==1) {
>                 request_region(dev->base_addr, MIXCOM_IO_EXTENT, dev->name);
>         }
> 
>         if(hw->channel==0 && !(ch->init_status & IRQ_ALLOCATED)) {
> 
>         ... DELETED 38 lines ...
> 
>                         procfile->mode = S_IFREG |  0444;
>                 }
>         }
> 
> Error --->
>         return 0;
> }

There's another problem here.  We're calling request_region()
inside cli().  request_region() can sleep.

On SMP, cli() does all sorts of bizarre things which are
quite different between different architectures.  I don't
know if this practice is actually unsafe on any architectures,
but it is really bad practice.  It's certainly the case that
schedue() will enable interrupts for you, so whatever you're
protecting won't be protected!

So I'd add `sleep inside cli()' to your list of things to
look out for.

Does your tool have the ability to track which functions
can and can't sleep?  This is a very common source of bug.
Grab a spinlock, then call a function which calls a function
which calls a function which calls kmalloc(GFP_KERNEL).  Unless
the spinlock is always protected by a semaphore, this can deadlock.

> 
> /u2/acc/oses/linux/2.4.1/drivers/scsi/eata_dma.c:490:eata_queue: ERROR:INTR:464:506: Interrupts inconsistent, severity `20':506
> 
>     save_flags(flags);
> Start --->
>     cli();
> 
> #if 0
>     for (x = 1, sh = first_HBA; x <= registered_HBAs; x++, sh = SD(sh)->next) {
>       if(inb((uint)sh->base + HA_RAUXSTAT) & HA_AIRQ) {
>             printk("eata_dma: scsi%d interrupt pending in eata_queue.\n"
>                    "          Calling interrupt handler.\n", sh->host_no);
> 
>         ... DELETED 32 lines ...
> 
>             printk(KERN_CRIT "eata_queue pid %ld, HBA QUEUE FULL..., "
>                    "returning DID_BUS_BUSY\n", cmd->pid));
>         done(cmd);
>         restore_flags(flags);
> Error --->
>         return(0);
>     }
>     ccb = &hd->ccb[y];

Something's gone a little wrong here.  The bug is in fact about
20 lines higher up.


Generally: yes, everything you've found needs fixing.