BOGUS: megaraid changes

This one came in through my scsi tree. How about we fix the issue in a different way: I can add an option in SCSI to check for the new eh methods and if it doesn't find any at all, panic the system saying that this driver has no error handling but if you reboot with the scsi_no_error_handling option it won't panic again?

James

I dont think panic() is needed bug a loud printk and maybe an error return from scsi_register() would do the trick. We do however have a couple of drivers where "pray, the firmware does all the work" is the right answer, but they really should be setting their own handler that delays rather than aborting/resetting/kicking offline

How about this? It doesn't panic, just refuses to attach the driver (although this will still eventually cause a panic if your root fs is on it).

James

===== hosts.c 1.23 vs edited =====

@@ -51,6 +51,22 @@
 
 static int scsi_host_next_hn;		/* host_no for next new host */
 static int scsi_hosts_registered;	/* cnt of registered scsi hosts */
+static int scsi_ignore_no_error_handling = 0;
+
+#ifdef MODULE
+MODULE_PARM(scsi_ignore_no_error_handling, "i");
+#else
+static int __init
+scsi_ignore_no_error_handling_setup(char *str)
+{
+        scsi_ignore_no_error_handling = 1;
+
+        return 1;
+}
+        
+__setup("scsi_ignore_no_error_handling=", scsi_ignore_no_error_handling_setup)

;
+#endif
+

/**
* scsi_tp_for_each_host - call function for each scsi host off a template
@@ -345,6 +361,25 @@
int gfp_mask;
DECLARE_MUTEX_LOCKED(sem);

+ /*
+ * Determine host number. Check reserved first before allocating
+ * new one
+ */
+ hname = (shost_tp->proc_name) ? shost_tp->proc_name : "";
+ hname_len = strlen(hname);
+
+ /* Check to see if this host has any error handling facilities */
+ if(shost_tp->eh_strategy_handler == NULL &&

+           shost_tp->eh_abort_handler == NULL &&
+           shost_tp->eh_device_reset_handler == NULL &&
+           shost_tp->eh_bus_reset_handler == NULL &&
+           shost_tp->eh_host_reset_handler == NULL) {

+ printk(KERN_ERR "ERROR: SCSI host `%s' has no error
handling\nERROR: This is not a safe way to run your SCSI host\nERROR: The error handling must be added to this driver\n", hname);
+ if(!scsi_ignore_no_error_handling) {
+ printk(KERN_ERR "ERROR: not attaching this
host.\n\nERROR: to force attachment, boot with the scsi_ignore_no_error_handling flag");
+ return NULL;
+ }
+ }
gfp_mask = GFP_KERNEL;
if (shost_tp->unchecked_isa_dma && xtr_bytes)
gfp_mask |= __GFP_DMA;
@@ -356,13 +391,6 @@
}

memset(shost, 0, sizeof(struct Scsi_Host) + xtr_bytes);
-
- /*
- * Determine host number. Check reserved first before allocating
- * new one
- */
- hname = (shost_tp->proc_name) ? shost_tp->proc_name : "";
- hname_len = strlen(hname);

if (hname_len)
list_for_each(lh, &scsi_host_hn_list) {

Looks a good starting point so that people can use the drivers but do get warned

I think this is worse than the current state of art. We don't want to screw with users who can't do anything about it, which is 99.9% of them. TESTING is about as important as anything else, and inconveniencing users is BAD BAD BAD.

Right now drivers with old EH handling will warn at compile time (except when people explicitly disable it, like in megaraid, which is in the process of getting fixed anyway). That's a lot better than irritating users. Especially since this printk() will possibly have scrolled off the screen by the time the "cannot load root" happens.

I've said this before, I'll say it again: anything that breaks _working_ is BAD. Don't do it. Don't make up new ways to screw with people who want to test. Don't add features that have _no_ meaning except to irritate people.

The compile-time warning is _plenty_ good enough.

Linus

OK, what about the runtime warning with no requirement for a special flag to enable attachment.

I don't necessarily agree. It's easy to miss in all the build noise (most average users don't do make -s). And the warning isn't that fierce (it complains about a prototype mismatch), so even if it's noticed, it might get ignored. At least if we have a run time warning, it's in the logs for all to see when a problem gets posted to any given mailing list.

James

That certainly works for me - informational but not irritating. I just suspect it will scroll past without being seen that much, though.

But along with a stack_dump() or something to make it a bit more noticeable it might actually be visible.

I dunno - I personally think more people look at the compile output than at the dmessages when they scroll past. But maybe it's just me (I don't use -s, but I tend to do "make -j4 bzImage > ../makes", so the _only_ thing I see are warnings and errors).

Linus

Hopefully we have a winner...

OK, I'll do it this way and put it into the next SCSI merge.

James

I take this opportunity to advertise for the quiet facility of kbuild:

Sample run:
$> make KBUILD_VERBOSE=0
CC kernel/rcupdate.o
CC kernel/dma.o
kernel/dma.c:102: warning: `proc_dma_show' defined but not used
CC kernel/uid16.o
LD kernel/built-in.o

Warnings really hurts the eye when using this.
And the filename supplied should be good enough for emacs and the like. I have seen only a few persons actually using this and I wonder if people feel they loose some information when presented with this condensed format?

Compared to "make -s" you are able to follow the progress.

Sam

Seconded - not only that people _keep_ removing the old entries without fixing the bug - those bogon megaraid changes are far from unique

Our megaraid problems turned out to be a problem with probing the IBM EXP300 SCSI RAID enclosure. The enclosure responds to the initial inquiry but does not respond to report LUN's adding it to the black list fixes the problem.

Thanks to Matt Domsch and Mark Haverkamp. Don't give up the good work the megaraid driver still needs better error handling.

---------------------------------------------------

diff -urN -X dontdiff linux-2.5/drivers/scsi/scsi_scan.c linux-2.5-megaraid/drivers/scsi/scsi_scan.c
@@ -131,6 +131,7 @@
 	{"MITSUMI", "CD-R CR-2201CS", "6119", BLIST_NOLUN},	/* locks up */
 	{"RELISYS", "Scorpio", NULL, BLIST_NOLUN},	/* responds to all lun */
 	{"MICROTEK", "ScanMaker II", "5.61", BLIST_NOLUN},	/* responds to all lun */
+	{"IBM", "EXP300", "D014", BLIST_NOLUN},		/* locks up */
 
 	/*
 	 * Other types of devices that have special flags.