lkml.org 
[lkml]   [2001]   [Mar]   [17]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [CHECKER] 28 potential interrupt errors
    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.
    -
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

    \
     
     \ /
      Last update: 2005-03-22 13:29    [W:0.059 / U:31.300 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site