Skip to contentSkip to conversation

sym53c8xx timer and smp fixes

PATCH In review Tim Hockin 3 participants 4 messages Jun 1, 2001 – Jun 3, 2001 SCSI

lore
Share & tools
lore

Tim Hockin

author
#0 lore raw

PATCH 1/1 sym53c8xx timer and smp fixes

All,

Attached is a patch for sym53c8xx.c to handle the error timer better, and be more proper for SMP. The changes are very simple, and have been beaten on by us. Please let me know if there are any problems accepting this patch for general inclusion.

Tim

Signature · 4 lines
Tim Hockin
Systems Software Engineer
Sun Microsystems, Cobalt Server Appliances
[email protected]
drivers/scsi/sym53c8xx.c +28 -37
diff -ruN dist-2.4.5/drivers/scsi/sym53c8xx.c cobalt-2.4.5/drivers/scsi/sym53c8xx.c
@@ -634,8 +636,11 @@
 #if LINUX_VERSION_CODE >= LinuxVersionCode(2,1,93)
 
 spinlock_t sym53c8xx_lock = SPIN_LOCK_UNLOCKED;
+spinlock_t sym53c8xx_host_lock = SPIN_LOCK_UNLOCKED;
 #define	NCR_LOCK_DRIVER(flags)     spin_lock_irqsave(&sym53c8xx_lock, flags)
 #define	NCR_UNLOCK_DRIVER(flags)   spin_unlock_irqrestore(&sym53c8xx_lock,flags)
+#define	NCR_LOCK_HOSTS(flags)     spin_lock_irqsave(&sym53c8xx_host_lock, flags)
+#define	NCR_UNLOCK_HOSTS(flags)   spin_unlock_irqrestore(&sym53c8xx_host_lock,flags)
 
 #define NCR_INIT_LOCK_NCB(np)      spin_lock_init(&np->smp_lock);
 #define	NCR_LOCK_NCB(np, flags)    spin_lock_irqsave(&np->smp_lock, flags)
@@ -650,6 +655,8 @@
 
 #define	NCR_LOCK_DRIVER(flags)     do { save_flags(flags); cli(); } while (0)
 #define	NCR_UNLOCK_DRIVER(flags)   do { restore_flags(flags); } while (0)
+#define	NCR_LOCK_HOSTS(flags)     do { save_flags(flags); cli(); } while (0)
+#define	NCR_UNLOCK_HOSTS(flags)   do { restore_flags(flags); } while (0)
 
 #define	NCR_INIT_LOCK_NCB(np)      do { } while (0)
 #define	NCR_LOCK_NCB(np, flags)    do { save_flags(flags); cli(); } while (0)
@@ -695,7 +702,7 @@
 	return page_remapped? (page_remapped + page_offs) : 0UL;
 }
 
-static void __init unmap_pci_mem(u_long vaddr, u_long size)
+static void unmap_pci_mem(u_long vaddr, u_long size)
 {
 	if (vaddr)
 		iounmap((void *) (vaddr & PAGE_MASK));
@@ -2249,7 +2265,6 @@
 	**----------------------------------------------------------------
 	*/
 	struct usrcmd	user;		/* Command from user		*/
-	volatile u_char	release_stage;	/* Synchronisation stage on release  */
 
 	/*----------------------------------------------------------------
 	**	Fields that are used (primarily) for integrity check
@@ -5868,7 +5883,12 @@
 	**	start the timeout daemon
 	*/
 	np->lasttime=0;
-	ncr_timeout (np);
+#ifdef SCSI_NCR_PCIQ_BROKEN_INTR
+	np->timer.expires = ktime_get((HZ+9)/10);
+#else
+	np->timer.expires = ktime_get(SCSI_NCR_TIMER_INTERVAL);
+#endif
+	add_timer(&np->timer);
 
 	/*
 	**  use SIMPLE TAG messages by default
@@ -7227,23 +7247,19 @@
 **==========================================================
 */
 
-#ifdef MODULE
 static int ncr_detach(ncb_p np)
 {
-	int i;
+	unsigned long flags;
 
 	printk("%s: detaching ...\n", ncr_name(np));
 
 /*
-**	Stop the ncr_timeout process
-**	Set release_stage to 1 and wait that ncr_timeout() set it to 2.
+**	Stop the ncr_timeout process - lock it to ensure no timer is running
+**	on a different CPU, or anything
 */
-	np->release_stage = 1;
-	for (i = 50 ; i && np->release_stage != 2 ; i--) MDELAY (100);
-	if (np->release_stage != 2)
-		printk("%s: the timer seems to be already stopped\n",
-			ncr_name(np));
-	else np->release_stage = 2;
+	NCR_LOCK_NCB(np, flags);
+	del_timer(&np->timer);
+	NCR_UNLOCK_NCB(np, flags);
 
 /*
 **	Reset NCR chip.
@@ -7273,7 +7289,6 @@
 
 	return 1;
 }
-#endif
 
 /*==========================================================
 **
@@ -8600,23 +8615,11 @@
 {
 	u_long	thistime = ktime_get(0);
 
-	/*
-	**	If release process in progress, let's go
-	**	Set the release stage from 1 to 2 to synchronize
-	**	with the release process.
-	*/
-
-	if (np->release_stage) {
-		if (np->release_stage == 1) np->release_stage = 2;
-		return;
-	}
-
 #ifdef SCSI_NCR_PCIQ_BROKEN_INTR
-	np->timer.expires = ktime_get((HZ+9)/10);
+	mod_timer(&np->timer, ktime_get((HZ+9)/10));
 #else
-	np->timer.expires = ktime_get(SCSI_NCR_TIMER_INTERVAL);
+	mod_timer(&np->timer, ktime_get(SCSI_NCR_TIMER_INTERVAL));
 #endif
-	add_timer(&np->timer);
 
 	/*
 	**	If we are resetting the ncr, wait for settle_time before 
@@ -13071,7 +13075,7 @@
 		(int) (PciDeviceFn(pdev) & 7));
 
 #ifdef SCSI_NCR_DYNAMIC_DMA_MAPPING
-	if (pci_set_dma_mask(pdev, (dma_addr_t) (0xffffffffUL))) {
+	if (!pci_dma_supported(pdev, (dma_addr_t) (0xffffffffUL))) {
 		printk(KERN_WARNING NAME53C8XX
 		       "32 BIT PCI BUS DMA ADDRESSING NOT SUPPORTED\n");
 		return -1;
@@ -14181,13 +14215,15 @@
 {
 	struct Scsi_Host *host;
 	struct host_data *host_data;
+	unsigned long flags;
 	ncb_p ncb = 0;
-	int retv;
+	int retv = -EINVAL;
 
 #ifdef DEBUG_PROC_INFO
 printk("sym53c8xx_proc_info: hostno=%d, func=%d\n", hostno, func);
 #endif
 
+	NCR_LOCK_HOSTS(flags);
 	for (host = first_host; host; host = host->next) {
 		if (host->hostt != first_host->hostt)
 			continue;
@@ -14199,25 +14235,22 @@
 	}
 
 	if (!ncb)
-		return -EINVAL;
+		goto proc_done;
 
 	if (func) {
 #ifdef	SCSI_NCR_USER_COMMAND_SUPPORT
 		retv = ncr_user_command(ncb, buffer, length);
-#else
-		retv = -EINVAL;
 #endif
-	}
-	else {
+	} else {
 		if (start)
 			*start = buffer;
 #ifdef SCSI_NCR_USER_INFO_SUPPORT
 		retv = ncr_host_info(ncb, buffer, offset, length);
-#else
-		retv = -EINVAL;
 #endif
 	}
+	NCR_UNLOCK_HOSTS(flags);
 
+proc_done:
 	return retv;
 }
 
#1 lore raw

drivers/scsi/sym53c8xx.c

Quoted 19 lines · Tim Hockin

spinlock_t sym53c8xx_lock = SPIN_LOCK_UNLOCKED;

drivers/scsi/sym53c8xx.c +5 -0
+spinlock_t sym53c8xx_host_lock = SPIN_LOCK_UNLOCKED;
 #define        NCR_LOCK_DRIVER(flags)     spin_lock_irqsave(&sym53c8xx_lock, flags)
 #define        NCR_UNLOCK_DRIVER(flags)   spin_unlock_irqrestore(&sym53c8xx_lock,flags)
+#define        NCR_LOCK_HOSTS(flags)     spin_lock_irqsave(&sym53c8xx_host_lock, flags)
+#define        NCR_UNLOCK_HOSTS(flags)   spin_unlock_irqrestore(&sym53c8xx_host_lock,flags)
 
 #define NCR_INIT_LOCK_NCB(np)      spin_lock_init(&np->smp_lock);
 #define        NCR_LOCK_NCB(np, flags)    spin_lock_irqsave(&np->smp_lock, flags)
@@ -650,6 +655,8 @@
 
 #define        NCR_LOCK_DRIVER(flags)     do { save_flags(flags); cli(); } while (0)
 #define        NCR_UNLOCK_DRIVER(flags)   do { restore_flags(flags); } while (0)
+#define        NCR_LOCK_HOSTS(flags)     do { save_flags(flags); cli(); } while (0)
+#define        NCR_UNLOCK_HOSTS(flags)   do { restore_flags(flags); } while (0)
 
 #define        NCR_INIT_LOCK_NCB(np)      do { } while (0)
 #define        NCR_LOCK_NCB(np, flags)    do { save_flags(flags); cli(); } while (0)
@@ -695,7 +702,7 @@

so, this driver is mixed spinlocks and save/restore_flags? Any chance this can be converted to all spinlocks?

drivers/scsi/sym53c8xx.c line 13078

Quoted 3 lines
drivers/scsi/sym53c8xx.c +1 -1
 #ifdef SCSI_NCR_DYNAMIC_DMA_MAPPING
-       if (pci_set_dma_mask(pdev, (dma_addr_t) (0xffffffffUL))) {
+       if (!pci_dma_supported(pdev, (dma_addr_t) (0xffffffffUL))) {

totally wrong. you are going backwards.

Signature · 3 lines
Jeff Garzik      | Disbelief, that's why you fail.
Building 1024    |
MandrakeSoft     |
#2 lore raw
Quoted 17 lines · Jeff Garzik
Quoted 16 lines · Tim Hockin

spinlock_t sym53c8xx_lock = SPIN_LOCK_UNLOCKED;

drivers/scsi/sym53c8xx.c +5 -0
+spinlock_t sym53c8xx_host_lock = SPIN_LOCK_UNLOCKED;
 #define        NCR_LOCK_DRIVER(flags)     spin_lock_irqsave(&sym53c8xx_lock, flags)
 #define        NCR_UNLOCK_DRIVER(flags)   spin_unlock_irqrestore(&sym53c8xx_lock,flags)
+#define        NCR_LOCK_HOSTS(flags)     spin_lock_irqsave(&sym53c8xx_host_lock, flags)
+#define        NCR_UNLOCK_HOSTS(flags)   spin_unlock_irqrestore(&sym53c8xx_host_lock,flags)
 
 #define NCR_INIT_LOCK_NCB(np)      spin_lock_init(&np->smp_lock);
 #define        NCR_LOCK_NCB(np, flags)    spin_lock_irqsave(&np->smp_lock, flags)
@@ -650,6 +655,8 @@
 
 #define        NCR_LOCK_DRIVER(flags)     do { save_flags(flags); cli(); } while (0)
 #define        NCR_UNLOCK_DRIVER(flags)   do { restore_flags(flags); } while (0)
+#define        NCR_LOCK_HOSTS(flags)     do { save_flags(flags); cli(); } while (0)
+#define        NCR_UNLOCK_HOSTS(flags)   do { restore_flags(flags); } while (0)
 
Quoted 6 lines

drivers/scsi/sym53c8xx.c

Quoted 3 lines
drivers/scsi/sym53c8xx.c +0 -0
 #define        NCR_INIT_LOCK_NCB(np)      do { } while (0)
 #define        NCR_LOCK_NCB(np, flags)    do { save_flags(flags); cli(); } while (0)
@@ -695,7 +702,7 @@

so, this driver is mixed spinlocks and save/restore_flags? Any chance this can be converted to all spinlocks?

This has been done years ago for linux 2.1.93.
The save/restore flags locking methods are conditionnaly compiled for earlier kernels. This makes the corresponding code very probably quite useless nowadays and I should remove it from the source.

Gérard.

#3 lore raw
In reply to Tim Hockin
Quoted 6 lines · Tim Hockin

All,

Attached is a patch for sym53c8xx.c to handle the error timer better, and be more proper for SMP. The changes are very simple, and have been beaten on by us. Please let me know if there are any problems accepting this patch for general inclusion.

I have no problems accepting your patch. Thanks for it.

I just want to have to deal with a human manageable finite number of actual driver versions :). I also want the same driver source to also be useable on recent 2.2 kernels.

About timers in modules and more generally either timers in drivers or modules unloading, you must keep in mind that this stuff has been racy for years in Linux. Allow time for me to check if it is really fixed in latest kernel and so to make sure it is worthwhile to apply your patch.

Gérard.