[3551] in linux-scsi channel archive
Re: spinlocks in SCSI code
daemon@ATHENA.MIT.EDU (Kurt Garloff)
Wed Mar 18 03:47:53 1998
Date: Wed, 18 Mar 1998 09:45:00 +0100
From: Kurt Garloff <garloff@kg1.ping.de>
To: Doug Ledford <dledford@dialnet.net>
Cc: linux-scsi@vger.rutgers.edu, "Leonard N. Zubkoff" <lnz@dandelion.com>,
Gerard Roudier <groudier@club-internet.fr>
Mail-Followup-To: Doug Ledford <dledford@dialnet.net>,
linux-scsi@vger.rutgers.edu,
"Leonard N. Zubkoff" <lnz@dandelion.com>,
Gerard Roudier <groudier@club-internet.fr>
In-Reply-To: <350F161B.100F3963@dialnet.net>; from Doug Ledford on Tue, Mar 17, 1998 at 06:32:27PM -0600
--ew6BAiZeqk4r7MaW
Content-Type: text/plain; charset=us-ascii
On Tue, Mar 17, 1998 at 06:32:27PM -0600, Doug Ledford wrote:
> Although I'm not the worlds foremost expert on spin locks, I would have to
> guess that this would be, umm, problematic. You need to use the actual
> spin_unlock_irqrestore() function instead so that the cli() doesn't get
> released earlyier than planned. The problem I'm thinking of is this:
>
> spin_lock_irqsave(); [ interrupts turned off ]
> spin_unlock(); [ lock released, interrupts off ]
> function();
> spin_lock_irqsave(); [ lock restored, interrupts turned off again ]
> ...
> spin_unlock_irqrestore(); [ lock released, interrupts *enabled* ]
> return();
> spin_lock(); [ lock restored, interrupts untouched and on ]
> spin_unlock_irqrestore(); [ lock released, interrupts turned on,
> but they were already on anyway ]
>
> In short, I'm not sure, but I don't think the spin_lock_irqsave() and
> associated functions are completely nestable. I would check to find out
> before relying on these locks too much :)
If you consider it as a replacement for the old save_flags(); cli();
mechanism, my code does essentially the same. That's why it perfectly runs
on my UP machine. [I didn't add the spin_lock() of your example though.]
You can mix the IRQ and non-IRQ versions of locking. (The lock is created
and removed using the btsl (bit set) and btrl (bit remove) insns on the ix86.)
You have to make sure, that you restore the IRQ in the end.
It's another question if this makes sense. I temporarily unlocked the lock
to allow the function(); to lock it again, which is an bad workaround for
the problems encountered.
On an UP machine, this is fine (supposed that the behaviour with the old
mechanism was fine). On an SMP machine though, there are chances that the
other CPU starts to change driver data when the lok is temporarily released.
I append the patch (working on my UP machine with SMP kernel) for reference.
I think I will remove the lock from the function() and make sure locking is
done from all callers. I consider this to be much cleaner. There could be a
minimal performance hit on UP machines though, 'cause locking lasts a few
cycles longer (the cycles for the function call).
--
Kurt Garloff, Dortmund
<K.Garloff@ping.de>
PGP key on http://student.physik.uni-dortmund.de/homepages/garloff
--ew6BAiZeqk4r7MaW
Content-Type: text/plain; charset=us-ascii
Content-Disposition: attachment; filename="ncr53c8xx.spinlocks.diff"
--- linux/drivers/scsi/ncr53c8xx.c.2189 Sun Jan 18 14:06:04 1998
+++ linux/drivers/scsi/ncr53c8xx.c Wed Mar 18 01:05:16 1998
@@ -92,7 +92,7 @@
** Shared IRQ (since linux-1.3.72)
*/
-#define SCSI_NCR_DEBUG_FLAGS (0)
+#define SCSI_NCR_DEBUG_FLAGS (0xffff)
#define NCR_GETCC_WITHMSG
@@ -538,6 +538,21 @@
** Other Linux definitions
*/
+/* Locking */
+#if LINUX_VERSION_CODE >= LinuxVersionCode(2,1,30) && !defined(NCRNOSPIN)
+# include <asm/spinlock.h>
+ static spinlock_t ncr53c8xx_lock = SPIN_LOCK_UNLOCKED;
+# define LOCKWARN(s) //printk(s)
+# define NCRLOCK(f) /*printk("NCRLOCK ");*/ spin_lock_irqsave(&ncr53c8xx_lock, f)
+# define NCRUNLOCK(f) /*printk("NCRUNLOCK ");*/ spin_unlock_irqrestore(&ncr53c8xx_lock, f)
+# define NCRUNLOCK2 /*printk("NCRUNLOCK2 ")*/; spin_unlock(&ncr53c8xx_lock)
+#else
+# define LOCKWARN(s)
+# define NCRLOCK(f) save_flags(f); cli()
+# define NCRUNLOCK(f) restore_flags(f)
+# define NCRUNLOCK2
+#endif
+
#define ScsiResult(host_code, scsi_code) (((host_code) << 16) + ((scsi_code) & 0x7f))
#if LINUX_VERSION_CODE >= LinuxVersionCode(2,0,0)
@@ -4617,14 +4632,15 @@
** if interrupts are not enabled yet.
** Then enable disconnects.
*/
- save_flags(flags); cli();
+ LOCKWARN("ncr_attach:4632:");
+ NCRLOCK(flags);
if (ncr_reset_scsi_bus(np, 0, driver_setup.settle_delay) != 0) {
printf("%s: FATAL ERROR: CHECK SCSI BUS - CABLES, TERMINATION, DEVICE POWER etc.!\n", ncr_name(np));
restore_flags(flags);
goto attach_error;
}
ncr_exception (np);
- restore_flags(flags);
+ NCRUNLOCK(flags);
np->disc = 1;
@@ -4777,7 +4793,8 @@
**
**----------------------------------------------------
*/
- save_flags(flags); cli();
+ LOCKWARN("ncr_queue_command:4793:");
+ NCRLOCK(flags);
if (np->settle_time && cmd->timeout_per_command >= HZ &&
np->settle_time > jiffies + cmd->timeout_per_command - HZ) {
@@ -4786,7 +4803,7 @@
if (np->settle_time || !(cp=ncr_get_ccb (np, cmd->target, cmd->lun))) {
insert_into_waiting_list(np, cmd);
- restore_flags(flags);
+ NCRUNLOCK(flags);
return(DID_OK);
}
cp->cmd = cmd;
@@ -5013,7 +5030,7 @@
if (segments < 0) {
ncr_free_ccb(np, cp, cmd->target, cmd->lun);
- restore_flags(flags);
+ NCRUNLOCK(flags);
return(DID_ERROR);
}
@@ -5193,7 +5210,7 @@
/*
** and reenable interrupts
*/
- restore_flags(flags);
+ NCRUNLOCK(flags);
/*
** Command is successfully queued.
@@ -5217,13 +5234,14 @@
static void ncr_start_reset(ncb_p np, int settle_delay)
{
u_long flags;
-
- save_flags(flags); cli();
+
+ LOCKWARN("ncr_start_reset:5235:");
+ NCRLOCK(flags);
if (!np->settle_time) {
(void) ncr_reset_scsi_bus(np, 1, settle_delay);
}
- restore_flags(flags);
+ NCRUNLOCK(flags);
}
static int ncr_reset_scsi_bus(ncb_p np, int enab_int, int settle_delay)
@@ -5303,12 +5321,13 @@
np->stalling = 0;
#endif
- save_flags(flags); cli();
+ LOCKWARN("ncr_reset_bus:5321:");
+ NCRLOCK(flags);
/*
* Return immediately if reset is in progress.
*/
if (np->settle_time) {
- restore_flags(flags);
+ NCRUNLOCK(flags);
return SCSI_RESET_PUNT;
}
/*
@@ -5355,7 +5374,7 @@
cmd->scsi_done(cmd);
}
- restore_flags(flags);
+ NCRUNLOCK(flags);
return SCSI_RESET_SUCCESS;
}
@@ -5385,14 +5404,15 @@
np->stalling = 0;
#endif
- save_flags(flags); cli();
+ LOCKWARN("ncr_abort_command:5404");
+ NCRLOCK(flags);
/*
* First, look for the scsi command in the waiting list
*/
if (remove_from_waiting_list(np, cmd)) {
cmd->result = ScsiResult(DID_ABORT, 0);
cmd->scsi_done(cmd);
- restore_flags(flags);
+ NCRUNLOCK(flags);
return SCSI_ABORT_SUCCESS;
}
@@ -5411,12 +5431,12 @@
}
if (!found) {
- restore_flags(flags);
+ NCRUNLOCK(flags);
return SCSI_ABORT_NOT_RUNNING;
}
if (np->settle_time) {
- restore_flags(flags);
+ NCRUNLOCK(flags);
return SCSI_ABORT_SNOOZE;
}
@@ -5448,7 +5468,7 @@
#endif
OUTB (nc_istat, SIGP);
- restore_flags(flags);
+ NCRUNLOCK(flags);
return retv;
}
@@ -5895,6 +5915,7 @@
/*
** requeue awaiting scsi commands
*/
+ NCRUNLOCK2;
if (np->waiting_list) requeue_waiting_list(np);
/*
@@ -6697,11 +6718,12 @@
if (np->settle_time <= thistime) {
if (bootverbose > 1)
printf("%s: command processing resumed\n", ncr_name(np));
- save_flags(flags); cli();
+ LOCKWARN("ncr_timeout:6717:");
+ NCRLOCK(flags);
np->settle_time = 0;
np->disc = 1;
+ NCRUNLOCK2;
requeue_waiting_list(np);
- restore_flags(flags);
}
return;
}
@@ -6715,7 +6737,8 @@
/*
** block ncr interrupts
*/
- save_flags(flags); cli();
+ LOCKWARN("ncr_timeout:6736:");
+ NCRLOCK(flags);
np->lasttime = thistime;
@@ -6803,7 +6826,7 @@
#endif
OUTB (nc_istat, SIGP);
}
- restore_flags(flags);
+ NCRUNLOCK(flags);
}
#ifdef SCSI_NCR_BROKEN_INTR
@@ -6812,11 +6835,12 @@
/*
** Process pending interrupts.
*/
- save_flags(flags); cli();
+ LOCKWARN("ncr_timeout:6834");
+ NCRLOCK(flags);
if (DEBUG_FLAGS & DEBUG_TINY) printf ("{");
ncr_exception (np);
if (DEBUG_FLAGS & DEBUG_TINY) printf ("}");
- restore_flags(flags);
+ NCRUNLOCK(flags);
}
#endif /* SCSI_NCR_BROKEN_INTR */
}
@@ -9659,12 +9683,16 @@
#if LINUX_VERSION_CODE >= LinuxVersionCode(1,3,70)
static void ncr53c8xx_intr(int irq, void *dev_id, struct pt_regs * regs)
{
+ unsigned long flags;
#ifdef DEBUG_NCR53C8XX
printk("ncr53c8xx : interrupt received\n");
#endif
if (DEBUG_FLAGS & DEBUG_TINY) printf ("[");
+ LOCKWARN("ncr53c8xx_intr:9688:");
+ NCRLOCK(flags);
ncr_exception((ncb_p) dev_id);
+ NCRUNLOCK(flags);
if (DEBUG_FLAGS & DEBUG_TINY) printf ("]\n");
}
@@ -9708,7 +9736,8 @@
printk("ncr53c8xx_reset: pid=%lu reset_flags=%x serial_number=%ld serial_number_at_timeout=%ld\n",
cmd->pid, reset_flags, cmd->serial_number, cmd->serial_number_at_timeout);
- save_flags(flags); cli();
+ LOCKWARN("ncr53c8xx_reset:9735:");
+ NCRLOCK(flags);
/*
* We have to just ignore reset requests in some situations.
@@ -9738,7 +9767,7 @@
#endif
out:
- restore_flags(flags);
+ NCRUNLOCK(flags);
return sts;
}
#else
@@ -9763,7 +9792,8 @@
printk("ncr53c8xx_abort: pid=%lu serial_number=%ld serial_number_at_timeout=%ld\n",
cmd->pid, cmd->serial_number, cmd->serial_number_at_timeout);
- save_flags(flags); cli();
+ LOCKWARN("ncr53c8xx_abort:9791:");
+ NCRLOCK(flags);
/*
* We have to just ignore abort requests in some situations.
@@ -9775,7 +9805,7 @@
sts = ncr_abort_command(cmd);
out:
- restore_flags(flags);
+ NCRUNLOCK(flags);
return sts;
}
#else
@@ -10148,9 +10178,10 @@
else {
long flags;
- save_flags(flags); cli();
+ LOCKWARN("ncr_user_command:10177:");
+ NCRLOCK(flags);
ncr_usercmd (np);
- restore_flags(flags);
+ NCRUNLOCK(flags);
}
return length;
}
--ew6BAiZeqk4r7MaW--
-
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.rutgers.edu
