raw1394: fix locking in the presence of SMP and interrupts
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
@@ -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 intiso_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 intbus_reset (raw1394handle_t handle, unsigned int generation)
{
g_print ("got bus reset, generation %u\n", generation);
return 0;
}static gpointerreader_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;
}Quoted 6 lines · Andy Wingo
In raw1394.c:handle_iso_listen(), don't grab
host_info_lockat all -- we're not accessinghost_info_listorhost_count, and holding this lock while trying totasklet_killthe iso tasklet this can cause an ABBA deadlock if ohci:dma_rcv_tasklet is running and tries to grabhost_info_lockin 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.