Skip to contentSkip to conversation

mwave locking (was: IA32 - 1 New warnings)

PATCH In review Manfred Spraul 2 participants 2 messages Sep 7, 2003 Char and misc drivers

lore
Share & tools
lore
#0 lore raw

PATCH 1/1 mwave locking (was: IA32 - 1 New warnings)

Quoted 3 lines · John

drivers/char/mwave/mwavedd.c:331:2: warning: #warning "Sleeping on spinlock"

Interesting locking strategy:
A spinlock is placed on the stack and then spin_lock_irqsave(&local_lock, flags).

Attached is a patch that removes that. Untested due to lack of hardware. Anyone around such hardware (IBM Thinkpad?) --
Manfred

drivers/char/mwave/mwavedd.c +10 -12
@@ -293,8 +293,6 @@
 	
 		case IOCTL_MW_GET_IPC: {
 			unsigned int ipcnum = (unsigned int) ioarg;
-			spinlock_t ipc_lock = SPIN_LOCK_UNLOCKED;
-			unsigned long flags;
 	
 			PRINTK_3(TRACE_MWAVE,
 				"mwavedd::mwave_ioctl IOCTL_MW_GET_IPC"
@@ -310,32 +308,29 @@
 			}
 	
 			if (pDrvData->IPCs[ipcnum].bIsEnabled == TRUE) {
+				DECLARE_WAITQUEUE(wait, current);
+
 				PRINTK_2(TRACE_MWAVE,
 					"mwavedd::mwave_ioctl, thread for"
 					" ipc %x going to sleep\n",
 					ipcnum);
-	
-				spin_lock_irqsave(&ipc_lock, flags);
+				add_wait_queue(&pDrvData->IPCs[ipcnum].ipc_wait_queue, &wait);
+				pDrvData->IPCs[ipcnum].bIsHere = TRUE;
+				set_current_state(TASK_INTERRUPTIBLE);
 				/* check whether an event was signalled by */
 				/* the interrupt handler while we were gone */
 				if (pDrvData->IPCs[ipcnum].usIntCount == 1) {	/* first int has occurred (race condition) */
 					pDrvData->IPCs[ipcnum].usIntCount = 2;	/* first int has been handled */
-					spin_unlock_irqrestore(&ipc_lock, flags);
 					PRINTK_2(TRACE_MWAVE,
 						"mwavedd::mwave_ioctl"
 						" IOCTL_MW_GET_IPC ipcnum %x"
 						" handling first int\n",
 						ipcnum);
 				} else {	/* either 1st int has not yet occurred, or we have already handled the first int */
-					pDrvData->IPCs[ipcnum].bIsHere = TRUE;
-#warning "Sleeping on spinlock"
-					interruptible_sleep_on(&pDrvData->IPCs[ipcnum].ipc_wait_queue);
-					pDrvData->IPCs[ipcnum].bIsHere = FALSE;
+					schedule();
 					if (pDrvData->IPCs[ipcnum].usIntCount == 1) {
-						pDrvData->IPCs[ipcnum].
-						usIntCount = 2;
+						pDrvData->IPCs[ipcnum].usIntCount = 2;
 					}
-					spin_unlock_irqrestore(&ipc_lock, flags);
 					PRINTK_2(TRACE_MWAVE,
 						"mwavedd::mwave_ioctl"
 						" IOCTL_MW_GET_IPC ipcnum %x"
@@ -343,6 +338,9 @@
 						" application\n",
 						ipcnum);
 				}
+				pDrvData->IPCs[ipcnum].bIsHere = FALSE;
+				remove_wait_queue(&pDrvData->IPCs[ipcnum].ipc_wait_queue, &wait);
+				set_current_state(TASK_RUNNING);
 				PRINTK_2(TRACE_MWAVE,
 					"mwavedd::mwave_ioctl IOCTL_MW_GET_IPC,"
 					" returning thread for ipc %x"
#1 lore raw

Re: [PATCH] mwave locking

Quoted 4 lines · Manfred Spraul
Quoted 2 lines

drivers/char/mwave/mwavedd.c:331:2: warning: #warning "Sleeping on spinlock"

Attached is a patch that removes that. Untested due to lack of hardware. Anyone around such hardware (IBM Thinkpad?)

I'll try it - it's a Thinkpad 600. See also
http://bugzilla.kernel.org/show_bug.cgi?id=185

Jochen

Signature · 1 line
#include <~/.signature>: permission denied