Skip to contentSkip to conversation

raw1394: fix locking in the presence of SMP and interrupts

PATCH In review Andy Wingo 2 participants 2 messages Oct 5, 2005 – Oct 9, 2005

lore
Share & tools
lore

Andy Wingo

author
#0 lore raw

PATCH 1/1 raw1394: fix locking in the presence of SMP and interrupts

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.

---

(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(-)
drivers/ieee1394/ohci1394.c +3 -3
Index: linux-source-2.6.12-2.6.12-work/drivers/ieee1394/ohci1394.c
===================================================================
@@ -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,
drivers/ieee1394/raw1394.c +56 -46

407 lines not on this page · raw message

/* $ gcc pkg-config --cflags --libs glib-2.0 -lraw1394 -o \
raw1394reader_nothreads raw1394reader_nothreads.c
*/
#include <glib.h>
#include <libraw1394/raw1394.h>

static int
iso_receive (raw1394handle_t handle, int channel, size_t len, quadlet_t * data)

{
    switch (len) {
    case 12:
        g_print ("x");
        break;
    case 492:
        g_print (".");
        break;
    default:
        g_print (" %ld ???\n", len);
        break;
    }
    
    return 0;
}

static int
bus_reset (raw1394handle_t handle, unsigned int generation)

{
    g_print ("got bus reset, generation %u\n", generation);
    
    return 0;
}

static gpointer
reader_loop (gpointer data)

{
    raw1394handle_t handle = data;
    
    g_print ("[1] reader loop starting\n");

    /* process */
    while (g_random_double () > 0.001)
        raw1394_loop_iterate (handle);

    g_print ("\n[1] reader thread stopping\n");

    return NULL;
}

int
main (int argc, char *argv[])

{
    raw1394handle_t handle;
    struct raw1394_portinfo pinf[16];
    int num_ports;
    
    handle = raw1394_new_handle ();
    if (!handle) {
        g_warning ("failed to open raw1394 handle");
        return 1;
    }
    
    num_ports = raw1394_get_port_info (handle, pinf, 16);
    if (num_ports <= 0) {
        g_warning ("no ports available");
        return 1;
    }
    
    if (raw1394_set_port (handle, 0) < 0) {
        g_warning ("failed to set port");
        return 1;
    }

    raw1394_set_iso_handler (handle, 63, iso_receive);
    raw1394_set_bus_reset_handler (handle, bus_reset);

    if (raw1394_start_iso_rcv (handle, 63) < 0) {
        g_warning ("failed to start isochronous transfer");
        return 1;
    }

    reader_loop (handle);

    g_print ("[0] stopping iso_rcv\n");

    raw1394_stop_iso_rcv (handle, 63);
    raw1394_destroy_handle (handle);

    g_print ("[0] success\n");

    return 0;
}
#1 lore raw
Quoted 6 lines · Andy Wingo

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.

I have to admit being surprised to see this because I have been using an SMP machine for a few years and not noticed a consistent or reproducible problem here. However, I have to admit the majority of my isochronous usage on kernel 2.6 has been using the new API and implementation (aka rawiso and libiec61883). So, I tested your program on my dual Athlon running a 2.6.12 SMP kernel, and it worked fine a few times when run manually. Then, I put it into a shell loop:
I="0"; while [ $I -lt 1000 ]; do ./raw1394reader_nothreads; \
echo ran iteration $I; I=expr $I + 1; done

Eventually, it did deadlock. So, I applied the patch, and the above looping test ran fine 3 times.Works for me.