Skip to contentSkip to conversation

resubmitting sym53c8xx patches

PATCH In review Tim Hockin 2 participants 4 messages Oct 17, 2001 – Oct 22, 2001 SCSI

lore
Share & tools
lore

Tim Hockin

author
#0 lore raw

PATCH 1/1 resubmitting sym53c8xx patches

All,

I've submitted this patch a few times, and had it OKed each time, but it hasn't made it in.

it does:
* cleanup timer handling
* spin lock host list
* adds a reboot handler
* remove __init from a function called from a non __init (needed for reboot handler)

Please apply this for the next 2.4.x.

Appreciated!

Tim

Signature · 4 lines
Tim Hockin
Systems Software Engineer
Sun Microsystems, Cobalt Server Appliances
[email protected]
drivers/scsi/sym53c8xx.c +66 -38
diff -ruN dist-2.4.12+patches/drivers/scsi/sym53c8xx.c cvs-2.4.12+patches/drivers/scsi/sym53c8xx.c
@@ -111,6 +111,8 @@
 #elif LINUX_VERSION_CODE >= LinuxVersionCode(2,1,93)
 #include <asm/spinlock.h>
 #endif
+#include <linux/notifier.h>
+#include <linux/reboot.h>
 #include <linux/delay.h>
 #include <linux/signal.h>
 #include <linux/sched.h>

@@ -635,8 +637,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)

@@ -651,6 +656,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)

@@ -696,7 +703,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));

@@ -1302,6 +1309,15 @@
 			int length, int hostno, int func);
 #endif
 
+/* 
+** reset the scsi controller on a "reboot." it should be way at the
+** end.
+*/
+static int sym53c8xx_halt(struct notifier_block *, ulong, void *);
+static struct notifier_block sym53c8xx_notifier = {
+  sym53c8xx_halt, NULL, 0
+};
+
 /*
 **	Driver setup.
 **

@@ -2250,7 +2266,6 @@
 	**----------------------------------------------------------------
 	*/
 	struct usrcmd	user;		/* Command from user		*/
-	volatile u_char	release_stage;	/* Synchronisation stage on release  */
 
 	/*----------------------------------------------------------------
 	**	Fields that are used (primarily) for integrity check

@@ -5869,7 +5884,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

@@ -7232,23 +7252,14 @@
 **==========================================================
 */
 
-#ifdef MODULE
 static int ncr_detach(ncb_p np)
 {
-	int i;
-
 	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.
 */
-	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;
+	del_timer_sync(&np->timer);
 
 /*
 **	Reset NCR chip.

@@ -7278,7 +7289,6 @@
 
 	return 1;
 }
-#endif
 
 /*==========================================================
 **

@@ -8605,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 

@@ -13070,7 +13075,8 @@
 	}
 
 	m_free(devtbl, PAGE_SIZE, "devtbl");
-
+	if (attach_count)
+ 	          register_reboot_notifier(&sym53c8xx_notifier);
 	return attach_count;
 }
 

@@ -13802,19 +13808,49 @@
 }
 
 
+static int sym53c8xx_halt(struct notifier_block *block, ulong event, void *buf)
+{
+	unsigned long flags;
+	struct Scsi_Host *host, *next;
+
+	switch (event) {
+	case SYS_RESTART:
+	case SYS_HALT:
+	case SYS_POWER_OFF:
+		break;
+	default:
+		return NOTIFY_DONE;
+	}
+
+	printk(KERN_INFO "resetting sym53c8xx scsi bus(es)\n");
+	NCR_LOCK_HOSTS(flags);
+	next = first_host;
+	while ((host = next)) {
+		struct host_data *data = (struct host_data *)host->hostdata;
+
+		next = host->next;	
+		if (data && data->ncb)
+			ncr_detach(data->ncb);
+	}
+	NCR_UNLOCK_HOSTS(flags);
+
+	return NOTIFY_OK;
+}
+
 #ifdef MODULE
 int sym53c8xx_release(struct Scsi_Host *host)
 {
 #ifdef DEBUG_SYM53C8XX
 printk("sym53c8xx : release\n");
 #endif
+     if (first_host)
+	unregister_reboot_notifier(&sym53c8xx_notifier);
      ncr_detach(((struct host_data *) host->hostdata)->ncb);
 
      return 1;
 }
 #endif
 
-
 /*
 **	Scsi command waiting list management.
 **

@@ -14209,13 +14245,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;

@@ -14227,25 +14265,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
 	}
+proc_done:
+	NCR_UNLOCK_HOSTS(flags);
 
 	return retv;
 }
#1 lore raw
Quoted 13 lines · Tim Hockin

All,

I've submitted this patch a few times, and had it OKed each time, but it hasn't made it in.

it does:
* cleanup timer handling
* spin lock host list
* adds a reboot handler
* remove __init from a function called from a non __init (needed for reboot handler)

Quoted 1 line

Please apply this for the next 2.4.x.

As long as the kernel does not look trustable to me, I avoid to send driver changes that are not absolutely needed. Doing so just makes user suspect the driver for any breakage that occurs after driver changes and this wastes my time for no valuable reasons.

About your proposal, it has not been NOKed, but it is not the way I would have implemented it. By the way, I already have cleaned up the module timer killing ins sym-2.1.15 driver (easily back-portable to sym53c8xx). You may look at it (ftp.tux.org) if you are interested in knowing how I would like it to have been done. For example, there are a couple of some _smart_ #if that allows to preserve kernel 2.2 compatibility with the same source. People who donnot like cpp should switch to Java in my opinion. Compiling lot of stuff like inlines that will not in fact be used is false cosmetic in my opinion.

The reboot handler stuff is useless in my opinion and OTOH last time I looked in the kernel code related to reboot handler stuff it looks to me very incomplete. Useless stuff implies additionnal bugs that could have been avoided.

I am not opposed to your patch and will not complain if it is applied to kernel 2.4. I just haven't time for submitting another patch quickly nor have time for following any breakage due to its new interactions with the kernel.

Regards,
Gérard.

author
#2 lore raw
Quoted 4 lines · Gérard Roudier

About your proposal, it has not been NOKed, but it is not the way I would have implemented it. By the way, I already have cleaned up the module timer killing ins sym-2.1.15 driver (easily back-portable to sym53c8xx). You may look at it (ftp.tux.org) if you are interested in knowing how I

I will look at it.

Quoted 4 lines

The reboot handler stuff is useless in my opinion and OTOH last time I looked in the kernel code related to reboot handler stuff it looks to me very incomplete. Useless stuff implies additionnal bugs that could have been avoided.

We have a bootloader that loads a kernel, then loads another kernel. Without the reboot handler, we get weird behavior, and SCSI that does not initialize properly for the second kernel. It may not be useful to everyone, but it is essential to some.

Quoted 4 lines

I am not opposed to your patch and will not complain if it is applied to kernel 2.4. I just haven't time for submitting another patch quickly nor have time for following any breakage due to its new interactions with the kernel.

I appreciate your concern on this. I'll re-examine the timer stuff, and
if possible, I'd like to get to agreement ASAP, so we can X it off our list of changes.

Thanks

Tim

Signature · 4 lines
Tim Hockin
Systems Software Engineer
Sun Microsystems, Cobalt Server Appliances
[email protected]
author
#3 lore raw
In reply to Gérard Roudier
Quoted 4 lines · Gérard Roudier

I am not opposed to your patch and will not complain if it is applied to kernel 2.4. I just haven't time for submitting another patch quickly nor have time for following any breakage due to its new interactions with the kernel.

We'll deal with any breakage this patch incurs. Alan,Linus Is there any problem applying this patch for 2.4.x ?

it does:
* cleanup timer handling
* spin lock host list
* adds a reboot handler (for multi-stage boot setups like ours)
* remove __init from a function called from a non __init (needed for reboot handler)

Signature · 4 lines
Tim Hockin
Systems Software Engineer
Sun Microsystems, Cobalt Server Appliances
[email protected]
drivers/scsi/sym53c8xx.c +66 -38
diff -ruN dist-2.4.12+patches/drivers/scsi/sym53c8xx.c cvs-2.4.12+patches/drivers/scsi/sym53c8xx.c
@@ -111,6 +111,8 @@
 #elif LINUX_VERSION_CODE >= LinuxVersionCode(2,1,93)
 #include <asm/spinlock.h>
 #endif
+#include <linux/notifier.h>
+#include <linux/reboot.h>
 #include <linux/delay.h>
 #include <linux/signal.h>
 #include <linux/sched.h>

@@ -635,8 +637,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)

@@ -651,6 +656,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)

@@ -696,7 +703,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));

@@ -1302,6 +1309,15 @@
 			int length, int hostno, int func);
 #endif
 
+/* 
+** reset the scsi controller on a "reboot." it should be way at the
+** end.
+*/
+static int sym53c8xx_halt(struct notifier_block *, ulong, void *);
+static struct notifier_block sym53c8xx_notifier = {
+  sym53c8xx_halt, NULL, 0
+};
+
 /*
 **	Driver setup.
 **

@@ -2250,7 +2266,6 @@
 	**----------------------------------------------------------------
 	*/
 	struct usrcmd	user;		/* Command from user		*/
-	volatile u_char	release_stage;	/* Synchronisation stage on release  */
 
 	/*----------------------------------------------------------------
 	**	Fields that are used (primarily) for integrity check

@@ -5869,7 +5884,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

@@ -7232,23 +7252,14 @@
 **==========================================================
 */
 
-#ifdef MODULE
 static int ncr_detach(ncb_p np)
 {
-	int i;
-
 	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.
 */
-	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;
+	del_timer_sync(&np->timer);
 
 /*
 **	Reset NCR chip.

@@ -7278,7 +7289,6 @@
 
 	return 1;
 }
-#endif
 
 /*==========================================================
 **

@@ -8605,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 

@@ -13070,7 +13075,8 @@
 	}
 
 	m_free(devtbl, PAGE_SIZE, "devtbl");
-
+	if (attach_count)
+ 	          register_reboot_notifier(&sym53c8xx_notifier);
 	return attach_count;
 }
 

@@ -13802,19 +13808,49 @@
 }
 
 
+static int sym53c8xx_halt(struct notifier_block *block, ulong event, void *buf)
+{
+	unsigned long flags;
+	struct Scsi_Host *host, *next;
+
+	switch (event) {
+	case SYS_RESTART:
+	case SYS_HALT:
+	case SYS_POWER_OFF:
+		break;
+	default:
+		return NOTIFY_DONE;
+	}
+
+	printk(KERN_INFO "resetting sym53c8xx scsi bus(es)\n");
+	NCR_LOCK_HOSTS(flags);
+	next = first_host;
+	while ((host = next)) {
+		struct host_data *data = (struct host_data *)host->hostdata;
+
+		next = host->next;	
+		if (data && data->ncb)
+			ncr_detach(data->ncb);
+	}
+	NCR_UNLOCK_HOSTS(flags);
+
+	return NOTIFY_OK;
+}
+
 #ifdef MODULE
 int sym53c8xx_release(struct Scsi_Host *host)
 {
 #ifdef DEBUG_SYM53C8XX
 printk("sym53c8xx : release\n");
 #endif
+     if (first_host)
+	unregister_reboot_notifier(&sym53c8xx_notifier);
      ncr_detach(((struct host_data *) host->hostdata)->ncb);
 
      return 1;
 }
 #endif
 
-
 /*
 **	Scsi command waiting list management.
 **

@@ -14209,13 +14245,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;

@@ -14227,25 +14265,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
 	}
+proc_done:
+	NCR_UNLOCK_HOSTS(flags);
 
 	return retv;
 }