Changes all spinlocks that can be held during an irq handler to disable interrupts while the lock is held. Changes spin_[un]lock_irq to use the irqsave/irqrestore variants for robustness and readability. In raw1394.c:handle_iso_listen(), don't grab host_info_lock at all -- we're not accessing host_info_list or host_count, and holding this lock while trying to tasklet_kill the iso tasklet this can cause an ABBA deadlock if ohci:dma_rcv_tasklet is running and tries to grab host_info_lock in raw1394.c:receive_iso. Test program attached reliably deadlocks all SMP machines I have been able to test without this patch. Signed-off-by: Andy Wingo --- (This is my first kernel patch, I hope my MUA gets it right...) ohci1394.c | 6 +-- raw1394.c | 102 +++++++++++++++++++++++++++++++++---------------------------- 2 files changed, 59 insertions(+), 49 deletions(-) Index: linux-source-2.6.12-2.6.12-work/drivers/ieee1394/ohci1394.c =================================================================== --- linux-source-2.6.12-2.6.12-work.orig/drivers/ieee1394/ohci1394.c 2005-06-17 21:48:29.000000000 +0200 +++ linux-source-2.6.12-2.6.12-work/drivers/ieee1394/ohci1394.c 2005-10-05 11:55:13.000000000 +0200 @@ -2281,8 +2281,9 @@ { struct ohci1394_iso_tasklet *t; unsigned long mask; + unsigned long flags; - spin_lock(&ohci->iso_tasklet_list_lock); + spin_lock_irqsave(&ohci->iso_tasklet_list_lock, flags); list_for_each_entry(t, &ohci->iso_tasklet_list, link) { mask = 1 << t->context; @@ -2293,8 +2294,7 @@ tasklet_schedule(&t->tasklet); } - spin_unlock(&ohci->iso_tasklet_list_lock); - + spin_unlock_irqrestore(&ohci->iso_tasklet_list_lock, flags); } static irqreturn_t ohci_irq_handler(int irq, void *dev_id, Index: linux-source-2.6.12-2.6.12-work/drivers/ieee1394/raw1394.c =================================================================== --- linux-source-2.6.12-2.6.12-work.orig/drivers/ieee1394/raw1394.c 2005-06-17 21:48:29.000000000 +0200 +++ linux-source-2.6.12-2.6.12-work/drivers/ieee1394/raw1394.c 2005-10-05 15:23:43.000000000 +0200 @@ -412,6 +412,7 @@ static ssize_t raw1394_read(struct file *file, char __user * buffer, size_t count, loff_t * offset_is_ignored) { + unsigned long flags; struct file_info *fi = (struct file_info *)file->private_data; struct list_head *lh; struct pending_request *req; @@ -435,10 +436,10 @@ } } - spin_lock_irq(&fi->reqlists_lock); + spin_lock_irqsave(&fi->reqlists_lock, flags); lh = fi->req_complete.next; list_del(lh); - spin_unlock_irq(&fi->reqlists_lock); + spin_unlock_irqrestore(&fi->reqlists_lock, flags); req = list_entry(lh, struct pending_request, list); @@ -486,6 +487,7 @@ static int state_initialized(struct file_info *fi, struct pending_request *req) { + unsigned long flags; struct host_info *hi; struct raw1394_khost_list *khl; @@ -499,7 +501,7 @@ switch (req->req.type) { case RAW1394_REQ_LIST_CARDS: - spin_lock_irq(&host_info_lock); + spin_lock_irqsave(&host_info_lock, flags); khl = kmalloc(sizeof(struct raw1394_khost_list) * host_count, SLAB_ATOMIC); @@ -513,7 +515,7 @@ khl++; } } - spin_unlock_irq(&host_info_lock); + spin_unlock_irqrestore(&host_info_lock, flags); if (khl != NULL) { req->req.error = RAW1394_ERROR_NONE; @@ -528,7 +530,7 @@ break; case RAW1394_REQ_SET_CARD: - spin_lock_irq(&host_info_lock); + spin_lock_irqsave(&host_info_lock, flags); if (req->req.misc < host_count) { list_for_each_entry(hi, &host_info_list, list) { if (!req->req.misc--) @@ -550,7 +552,7 @@ } else { req->req.error = RAW1394_ERROR_INVALID_ARG; } - spin_unlock_irq(&host_info_lock); + spin_unlock_irqrestore(&host_info_lock, flags); req->req.length = 0; break; @@ -569,7 +571,6 @@ { int channel = req->req.misc; - spin_lock_irq(&host_info_lock); if ((channel > 63) || (channel < -64)) { req->req.error = RAW1394_ERROR_INVALID_ARG; } else if (channel >= 0) { @@ -601,7 +602,6 @@ req->req.length = 0; queue_complete_req(req); - spin_unlock_irq(&host_info_lock); } static void handle_fcp_listen(struct file_info *fi, struct pending_request *req) @@ -627,6 +627,7 @@ static int handle_async_request(struct file_info *fi, struct pending_request *req, int node) { + unsigned long flags; struct hpsb_packet *packet = NULL; u64 addr = req->req.address & 0xffffffffffffULL; @@ -761,9 +762,9 @@ hpsb_set_packet_complete_task(packet, (void (*)(void *))queue_complete_cb, req); - spin_lock_irq(&fi->reqlists_lock); + spin_lock_irqsave(&fi->reqlists_lock, flags); list_add_tail(&req->list, &fi->req_pending); - spin_unlock_irq(&fi->reqlists_lock); + spin_unlock_irqrestore(&fi->reqlists_lock, flags); packet->generation = req->req.generation; @@ -779,6 +780,7 @@ static int handle_iso_send(struct file_info *fi, struct pending_request *req, int channel) { + unsigned long flags; struct hpsb_packet *packet; packet = hpsb_make_isopacket(fi->host, req->req.length, channel & 0x3f, @@ -804,9 +806,9 @@ (void (*)(void *))queue_complete_req, req); - spin_lock_irq(&fi->reqlists_lock); + spin_lock_irqsave(&fi->reqlists_lock, flags); list_add_tail(&req->list, &fi->req_pending); - spin_unlock_irq(&fi->reqlists_lock); + spin_unlock_irqrestore(&fi->reqlists_lock, flags); /* Update the generation of the packet just before sending. */ packet->generation = req->req.generation; @@ -821,6 +823,7 @@ static int handle_async_send(struct file_info *fi, struct pending_request *req) { + unsigned long flags; struct hpsb_packet *packet; int header_length = req->req.misc & 0xffff; int expect_response = req->req.misc >> 16; @@ -867,9 +870,9 @@ hpsb_set_packet_complete_task(packet, (void (*)(void *))queue_complete_cb, req); - spin_lock_irq(&fi->reqlists_lock); + spin_lock_irqsave(&fi->reqlists_lock, flags); list_add_tail(&req->list, &fi->req_pending); - spin_unlock_irq(&fi->reqlists_lock); + spin_unlock_irqrestore(&fi->reqlists_lock, flags); /* Update the generation of the packet just before sending. */ packet->generation = req->req.generation; @@ -885,6 +888,7 @@ static int arm_read(struct hpsb_host *host, int nodeid, quadlet_t * buffer, u64 addr, size_t length, u16 flags) { + unsigned long irqflags; struct pending_request *req; struct host_info *hi; struct file_info *fi = NULL; @@ -899,7 +903,7 @@ "addr: %4.4x %8.8x length: %Zu", nodeid, (u16) ((addr >> 32) & 0xFFFF), (u32) (addr & 0xFFFFFFFF), length); - spin_lock(&host_info_lock); + spin_lock_irqsave(&host_info_lock, irqflags); hi = find_host_info(host); /* search address-entry */ if (hi != NULL) { list_for_each_entry(fi, &hi->file_info_list, list) { @@ -924,7 +928,7 @@ if (!found) { printk(KERN_ERR "raw1394: arm_read FAILED addr_entry not found" " -> rcode_address_error\n"); - spin_unlock(&host_info_lock); + spin_unlock_irqrestore(&host_info_lock, irqflags); return (RCODE_ADDRESS_ERROR); } else { DBGMSG("arm_read addr_entry FOUND"); @@ -954,7 +958,7 @@ req = __alloc_pending_request(SLAB_ATOMIC); if (!req) { DBGMSG("arm_read -> rcode_conflict_error"); - spin_unlock(&host_info_lock); + spin_unlock_irqrestore(&host_info_lock, irqflags); return (RCODE_CONFLICT_ERROR); /* A resource conflict was detected. The request may be retried */ } @@ -974,7 +978,7 @@ if (!(req->data)) { free_pending_request(req); DBGMSG("arm_read -> rcode_conflict_error"); - spin_unlock(&host_info_lock); + spin_unlock_irqrestore(&host_info_lock, irqflags); return (RCODE_CONFLICT_ERROR); /* A resource conflict was detected. The request may be retried */ } @@ -1031,13 +1035,14 @@ sizeof(struct arm_request)); queue_complete_req(req); } - spin_unlock(&host_info_lock); + spin_unlock_irqrestore(&host_info_lock, irqflags); return (rcode); } static int arm_write(struct hpsb_host *host, int nodeid, int destid, quadlet_t * data, u64 addr, size_t length, u16 flags) { + unsigned long irqflags; struct pending_request *req; struct host_info *hi; struct file_info *fi = NULL; @@ -1052,7 +1057,7 @@ "addr: %4.4x %8.8x length: %Zu", nodeid, (u16) ((addr >> 32) & 0xFFFF), (u32) (addr & 0xFFFFFFFF), length); - spin_lock(&host_info_lock); + spin_lock_irqsave(&host_info_lock, irqflags); hi = find_host_info(host); /* search address-entry */ if (hi != NULL) { list_for_each_entry(fi, &hi->file_info_list, list) { @@ -1077,7 +1082,7 @@ if (!found) { printk(KERN_ERR "raw1394: arm_write FAILED addr_entry not found" " -> rcode_address_error\n"); - spin_unlock(&host_info_lock); + spin_unlock_irqrestore(&host_info_lock, irqflags); return (RCODE_ADDRESS_ERROR); } else { DBGMSG("arm_write addr_entry FOUND"); @@ -1106,7 +1111,7 @@ req = __alloc_pending_request(SLAB_ATOMIC); if (!req) { DBGMSG("arm_write -> rcode_conflict_error"); - spin_unlock(&host_info_lock); + spin_unlock_irqrestore(&host_info_lock, irqflags); return (RCODE_CONFLICT_ERROR); /* A resource conflict was detected. The request my be retried */ } @@ -1118,7 +1123,7 @@ if (!(req->data)) { free_pending_request(req); DBGMSG("arm_write -> rcode_conflict_error"); - spin_unlock(&host_info_lock); + spin_unlock_irqrestore(&host_info_lock, irqflags); return (RCODE_CONFLICT_ERROR); /* A resource conflict was detected. The request may be retried */ } @@ -1165,7 +1170,7 @@ sizeof(struct arm_request)); queue_complete_req(req); } - spin_unlock(&host_info_lock); + spin_unlock_irqrestore(&host_info_lock, irqflags); return (rcode); } @@ -1173,6 +1178,7 @@ u64 addr, quadlet_t data, quadlet_t arg, int ext_tcode, u16 flags) { + unsigned long irqflags; struct pending_request *req; struct host_info *hi; struct file_info *fi = NULL; @@ -1198,7 +1204,7 @@ (u32) (addr & 0xFFFFFFFF), ext_tcode & 0xFF, be32_to_cpu(data), be32_to_cpu(arg)); } - spin_lock(&host_info_lock); + spin_lock_irqsave(&host_info_lock, irqflags); hi = find_host_info(host); /* search address-entry */ if (hi != NULL) { list_for_each_entry(fi, &hi->file_info_list, list) { @@ -1224,7 +1230,7 @@ if (!found) { printk(KERN_ERR "raw1394: arm_lock FAILED addr_entry not found" " -> rcode_address_error\n"); - spin_unlock(&host_info_lock); + spin_unlock_irqrestore(&host_info_lock, irqflags); return (RCODE_ADDRESS_ERROR); } else { DBGMSG("arm_lock addr_entry FOUND"); @@ -1307,7 +1313,7 @@ req = __alloc_pending_request(SLAB_ATOMIC); if (!req) { DBGMSG("arm_lock -> rcode_conflict_error"); - spin_unlock(&host_info_lock); + spin_unlock_irqrestore(&host_info_lock, irqflags); return (RCODE_CONFLICT_ERROR); /* A resource conflict was detected. The request may be retried */ } @@ -1316,7 +1322,7 @@ if (!(req->data)) { free_pending_request(req); DBGMSG("arm_lock -> rcode_conflict_error"); - spin_unlock(&host_info_lock); + spin_unlock_irqrestore(&host_info_lock, irqflags); return (RCODE_CONFLICT_ERROR); /* A resource conflict was detected. The request may be retried */ } @@ -1382,7 +1388,7 @@ sizeof(struct arm_response) + 2 * sizeof(*store)); queue_complete_req(req); } - spin_unlock(&host_info_lock); + spin_unlock_irqrestore(&host_info_lock, irqflags); return (rcode); } @@ -1390,6 +1396,7 @@ u64 addr, octlet_t data, octlet_t arg, int ext_tcode, u16 flags) { + unsigned long irqflags; struct pending_request *req; struct host_info *hi; struct file_info *fi = NULL; @@ -1422,7 +1429,7 @@ (u32) ((be64_to_cpu(arg) >> 32) & 0xFFFFFFFF), (u32) (be64_to_cpu(arg) & 0xFFFFFFFF)); } - spin_lock(&host_info_lock); + spin_lock_irqsave(&host_info_lock, irqflags); hi = find_host_info(host); /* search addressentry in file_info's for host */ if (hi != NULL) { list_for_each_entry(fi, &hi->file_info_list, list) { @@ -1449,7 +1456,7 @@ printk(KERN_ERR "raw1394: arm_lock64 FAILED addr_entry not found" " -> rcode_address_error\n"); - spin_unlock(&host_info_lock); + spin_unlock_irqrestore(&host_info_lock, irqflags); return (RCODE_ADDRESS_ERROR); } else { DBGMSG("arm_lock64 addr_entry FOUND"); @@ -1533,7 +1540,7 @@ DBGMSG("arm_lock64 -> entering notification-section"); req = __alloc_pending_request(SLAB_ATOMIC); if (!req) { - spin_unlock(&host_info_lock); + spin_unlock_irqrestore(&host_info_lock, irqflags); DBGMSG("arm_lock64 -> rcode_conflict_error"); return (RCODE_CONFLICT_ERROR); /* A resource conflict was detected. The request may be retried */ @@ -1542,7 +1549,7 @@ req->data = kmalloc(size, SLAB_ATOMIC); if (!(req->data)) { free_pending_request(req); - spin_unlock(&host_info_lock); + spin_unlock_irqrestore(&host_info_lock, irqflags); DBGMSG("arm_lock64 -> rcode_conflict_error"); return (RCODE_CONFLICT_ERROR); /* A resource conflict was detected. The request may be retried */ @@ -1609,7 +1616,7 @@ sizeof(struct arm_response) + 2 * sizeof(*store)); queue_complete_req(req); } - spin_unlock(&host_info_lock); + spin_unlock_irqrestore(&host_info_lock, irqflags); return (rcode); } @@ -1980,6 +1987,7 @@ struct hpsb_packet *packet = NULL; int retval = 0; quadlet_t data; + unsigned long flags; data = be32_to_cpu((u32) req->req.sendb); DBGMSG("write_phypacket called - quadlet 0x%8.8x ", data); @@ -1990,9 +1998,9 @@ req->packet = packet; hpsb_set_packet_complete_task(packet, (void (*)(void *))queue_complete_cb, req); - spin_lock_irq(&fi->reqlists_lock); + spin_lock_irqsave(&fi->reqlists_lock, flags); list_add_tail(&req->list, &fi->req_pending); - spin_unlock_irq(&fi->reqlists_lock); + spin_unlock_irqrestore(&fi->reqlists_lock, flags); packet->generation = req->req.generation; retval = hpsb_send_packet(packet); DBGMSG("write_phypacket send_packet called => retval: %d ", retval); @@ -2656,14 +2664,15 @@ { struct file_info *fi = file->private_data; unsigned int mask = POLLOUT | POLLWRNORM; + unsigned long flags; poll_wait(file, &fi->poll_wait_complete, pt); - spin_lock_irq(&fi->reqlists_lock); + spin_lock_irqsave(&fi->reqlists_lock, flags); if (!list_empty(&fi->req_complete)) { mask |= POLLIN | POLLRDNORM; } - spin_unlock_irq(&fi->reqlists_lock); + spin_unlock_irqrestore(&fi->reqlists_lock, flags); return mask; } @@ -2707,6 +2716,7 @@ struct arm_addr *arm_addr = NULL; int another_host; int csr_mod = 0; + unsigned long flags; if (fi->iso_state != RAW1394_ISO_INACTIVE) raw1394_iso_shutdown(fi); @@ -2717,13 +2727,13 @@ } } - spin_lock_irq(&host_info_lock); + spin_lock_irqsave(&host_info_lock, flags); fi->listen_channels = 0; - spin_unlock_irq(&host_info_lock); + spin_unlock_irqrestore(&host_info_lock, flags); fail = 0; /* set address-entries invalid */ - spin_lock_irq(&host_info_lock); + spin_lock_irqsave(&host_info_lock, flags); while (!list_empty(&fi->addr_list)) { another_host = 0; @@ -2774,14 +2784,14 @@ vfree(addr->addr_space_buffer); kfree(addr); } /* while */ - spin_unlock_irq(&host_info_lock); + spin_unlock_irqrestore(&host_info_lock, flags); if (fail > 0) { printk(KERN_ERR "raw1394: during addr_list-release " "error(s) occurred \n"); } while (!done) { - spin_lock_irq(&fi->reqlists_lock); + spin_lock_irqsave(&fi->reqlists_lock, flags); while (!list_empty(&fi->req_complete)) { lh = fi->req_complete.next; @@ -2795,7 +2805,7 @@ if (list_empty(&fi->req_pending)) done = 1; - spin_unlock_irq(&fi->reqlists_lock); + spin_unlock_irqrestore(&fi->reqlists_lock, flags); if (!done) down_interruptible(&fi->complete_sem); @@ -2825,9 +2835,9 @@ fi->host->id); if (fi->state == connected) { - spin_lock_irq(&host_info_lock); + spin_lock_irqsave(&host_info_lock, flags); list_del(&fi->list); - spin_unlock_irq(&host_info_lock); + spin_unlock_irqrestore(&host_info_lock, flags); put_device(&fi->host->device); }