[PATCH] scsi_device refcounting and list lockdown

[PATCH] scsi_device refcounting and list lockdown

[Christoph Hellwig]

 - shost->my_devices is gone and replaced by ->__devices, which is not
   exposed to drivers and locked by the host lock.  Use the exported
   helpers that do proper refcounting to access it.
 - sdev->access_count is gone as a side-effect, the sg interfaces
   to export it now return 1 for a present scsi_device.
 - drivers/scsi/host.h is empty now.


--- 1.44/drivers/ieee1394/sbp2.c	Tue Sep  2 20:08:00 2003
+++ edited/drivers/ieee1394/sbp2.c	Mon Oct 27 12:01:48 2003
@@ -991,7 +991,7 @@
 static void sbp2_remove_device(struct scsi_id_instance_data *scsi_id)
 {
 	struct sbp2scsi_host_info *hi = scsi_id->hi;
-	struct scsi_device *sdev = scsi_find_device(hi->scsi_host, 0, scsi_id->id, 0);
+	struct scsi_device *sdev;
 
 	SBP2_DEBUG("sbp2_remove_device");
 
@@ -999,8 +999,13 @@
 	sbp2scsi_complete_all_commands(scsi_id, DID_NO_CONNECT);
 
 	/* Remove it from the scsi layer now */
-	if (sdev)
+	/* XXX(hch): why can't we simply cache the scsi_device
+	   	     in struct scsi_id_instance_data? */
+	sdev = scsi_device_lookup(hi->scsi_host, 0, scsi_id->id, 0);
+	if (sdev) {
 		scsi_remove_device(sdev);
+		scsi_device_put(sdev);
+	}
 
 	sbp2util_remove_command_orb_pool(scsi_id);
 
--- 1.44/drivers/scsi/53c700.c	Sat Sep 20 11:35:53 2003
+++ edited/drivers/scsi/53c700.c	Mon Oct 27 12:01:48 2003
@@ -1057,15 +1057,18 @@
 		__u8 lun;
 		struct NCR_700_command_slot *slot;
 		__u8 reselection_id = hostdata->reselection_id;
-		Scsi_Device *SDp;
+		struct scsi_device *SDp;
+		unsigned long flags;
 
 		lun = hostdata->msgin[0] & 0x1f;
 
 		hostdata->reselection_id = 0xff;
 		DEBUG(("scsi%d: (%d:%d) RESELECTED!\n",
 		       host->host_no, reselection_id, lun));
+
 		/* clear the reselection indicator */
-		SDp = scsi_find_device(host, 0, reselection_id, lun);
+		spin_lock_irqsave(host->host_lock, flags);
+		SDp = __scsi_device_lookup(host, 0, reselection_id, lun);
 		if(unlikely(SDp == NULL)) {
 			printk(KERN_ERR "scsi%d: (%d:%d) HAS NO device\n",
 			       host->host_no, reselection_id, lun);
@@ -1092,6 +1095,7 @@
 			}
 			slot = (struct NCR_700_command_slot *)SCp->host_scribble;
 		}
+		spin_unlock_irqrestore(host->host_lock, flags);
 
 		if(slot == NULL) {
 			printk(KERN_ERR "scsi%d: (%d:%d) RESELECTED but no saved command (MSG = %02x %02x %02x)!!\n",
@@ -1498,7 +1502,7 @@
 			       host->host_no, SCp, SCp == NULL ? NULL : SCp->host_scribble, dsp, dsp - hostdata->pScript);
 
 			/* clear all the negotiated parameters */
-	    		list_for_each_entry(SDp, &host->my_devices, siblings)
+			__shost_for_each_device(SDp, host)
 				SDp->hostdata = 0;
 			
 			/* clear all the slots and their pending commands */
--- 1.28/drivers/scsi/NCR53C9x.c	Sun Jul 27 21:24:27 2003
+++ edited/drivers/scsi/NCR53C9x.c	Mon Oct 27 12:01:48 2003
@@ -815,6 +815,7 @@
 
 static int esp_host_info(struct NCR_ESP *esp, char *ptr, off_t offset, int len)
 {
+	struct scsi_device *sdev;
 	struct info_str info;
 	int i;
 
@@ -867,23 +868,20 @@
 	
 	/* Now describe the state of each existing target. */
 	copy_info(&info, "Target #\tconfig3\t\tSync Capabilities\tDisconnect\n");
-	for(i = 0; i < 15; i++) {
-		if(esp->targets_present & (1 << i)) {
-			Scsi_Device *SDptr;
-			struct esp_device *esp_dev;
 
-			list_for_each_entry(SDptr, &esp->ehost->my_devices,
-					siblings)
-				if(SDptr->id == i)
-					break;
+	shost_for_each_device(sdev, esp->ehost) {
+		struct esp_device *esp_dev = sdev->hostdata;
+		uint id = sdev->id;
+
+		if (!(esp->targets_present & (1 << id)))
+			continue;
 
-			esp_dev = SDptr->hostdata;
-			copy_info(&info, "%d\t\t", i);
-			copy_info(&info, "%08lx\t", esp->config3[i]);
-			copy_info(&info, "[%02lx,%02lx]\t\t\t", esp_dev->sync_max_offset,
-				  esp_dev->sync_min_period);
-			copy_info(&info, "%s\n", esp_dev->disconnect ? "yes" : "no");
-		}
+		copy_info(&info, "%d\t\t", id);
+		copy_info(&info, "%08lx\t", esp->config3[id]);
+		copy_info(&info, "[%02lx,%02lx]\t\t\t",
+			esp_dev->sync_max_offset,
+			esp_dev->sync_min_period);
+		copy_info(&info, "%s\n", esp_dev->disconnect ? "yes" : "no");
 	}
 
 	return info.pos > info.offset? info.pos - info.offset : 0;
===== drivers/scsi/constants.c 1.13 vs edited =====
--- 1.13/drivers/scsi/constants.c	Thu Oct 16 10:52:45 2003
+++ edited/drivers/scsi/constants.c	Mon Oct 27 12:01:49 2003
@@ -6,13 +6,13 @@
  *   by D. Gilbert and aeb (20020609)
  */
 
-#include <linux/module.h>
-
 #include <linux/config.h>
+#include <linux/module.h>
 #include <linux/blkdev.h>
 #include <linux/kernel.h>
+
+#include <scsi/scsi_host.h>
 #include "scsi.h"
-#include "hosts.h"
 
 #define CONST_COMMAND   0x01
 #define CONST_STATUS    0x02
--- 1.33/drivers/scsi/esp.c	Sun Aug  3 18:07:28 2003
+++ edited/drivers/scsi/esp.c	Mon Oct 27 12:01:49 2003
@@ -1309,6 +1309,7 @@
 
 static int esp_host_info(struct esp *esp, char *ptr, off_t offset, int len)
 {
+	struct scsi_device *sdev;
 	struct info_str info;
 	int i;
 
@@ -1384,25 +1385,23 @@
 	
 	/* Now describe the state of each existing target. */
 	copy_info(&info, "Target #\tconfig3\t\tSync Capabilities\tDisconnect\tWide\n");
-	for (i = 0; i < 15; i++) {
-		if (esp->targets_present & (1 << i)) {
-			Scsi_Device *SDptr;
-			struct esp_device *esp_dev;
 
-			list_for_each_entry(SDptr, &esp->ehost->my_devices,
-					siblings)
-				if(SDptr->id == i)
-					break;
+	shost_for_each_device(sdev, esp->ehost) {
+		struct esp_device *esp_dev = sdev->hostdata;
+		uint id = sdev->id;
+
+		if (!(esp->targets_present & (1 << id)))
+			continue;
 
-			esp_dev = SDptr->hostdata;
-			copy_info(&info, "%d\t\t", i);
-			copy_info(&info, "%08lx\t", esp->config3[i]);
-			copy_info(&info, "[%02lx,%02lx]\t\t\t", esp_dev->sync_max_offset,
-				  esp_dev->sync_min_period);
-			copy_info(&info, "%s\t\t", esp_dev->disconnect ? "yes" : "no");
-			copy_info(&info, "%s\n",
-				  (esp->config3[i] & ESP_CONFIG3_EWIDE) ? "yes" : "no");
-		}
+		copy_info(&info, "%d\t\t", id);
+		copy_info(&info, "%08lx\t", esp->config3[id]);
+		copy_info(&info, "[%02lx,%02lx]\t\t\t",
+			esp_dev->sync_max_offset,
+			esp_dev->sync_min_period);
+		copy_info(&info, "%s\t\t",
+			esp_dev->disconnect ? "yes" : "no");
+		copy_info(&info, "%s\n",
+			(esp->config3[id] & ESP_CONFIG3_EWIDE) ? "yes" : "no");
 	}
 	return info.pos > info.offset? info.pos - info.offset : 0;
 }
===== drivers/scsi/fcal.c 1.13 vs edited =====
--- 1.13/drivers/scsi/fcal.c	Sat Sep 20 16:06:41 2003
+++ edited/drivers/scsi/fcal.c	Mon Oct 27 12:01:49 2003
@@ -228,7 +228,7 @@
 #endif
 	SPRINTF ("Initiator AL-PA: %02x\n", fc->sid);
 
-	SPRINTF ("\nAttached devices: %s\n", !list_empty(&host->my_devices) ? "" : "none");
+	SPRINTF ("\nAttached devices:\n");
 	
 	for (i = 0; i < fc->posmap->len; i++) {
 		unsigned char alpa = fc->posmap->list[i];
===== drivers/scsi/hosts.c 1.94 vs edited =====
--- 1.94/drivers/scsi/hosts.c	Sun Sep 21 19:52:38 2003
+++ edited/drivers/scsi/hosts.c	Mon Oct 27 12:01:49 2003
@@ -1,6 +1,7 @@
 /*
  *  hosts.c Copyright (C) 1992 Drew Eckhardt
  *          Copyright (C) 1993, 1994, 1995 Eric Youngdale
+ *          Copyright (C) 2002-2003 Christoph Hellwig
  *
  *  mid to lowlevel SCSI driver interface
  *      Initial versions: Drew Eckhardt
@@ -30,8 +31,8 @@
 #include <linux/completion.h>
 #include <linux/unistd.h>
 
+#include <scsi/scsi_host.h>
 #include "scsi.h"
-#include "hosts.h"
 
 #include "scsi_priv.h"
 #include "scsi_logging.h"
@@ -50,6 +51,11 @@
 	.release	= scsi_host_cls_release,
 };
 
+static int scsi_device_cancel_cb(struct device *dev, void *data)
+{
+	return scsi_device_cancel(to_scsi_device(dev), *(int *)data);
+}
+
 /**
  * scsi_host_cancel - cancel outstanding IO to this host
  * @shost:	pointer to struct Scsi_Host
@@ -57,11 +63,7 @@
  **/
 void scsi_host_cancel(struct Scsi_Host *shost, int recovery)
 {
-	unsigned long flags;
-
-	spin_lock_irqsave(shost->host_lock, flags);
 	set_bit(SHOST_CANCEL, &shost->shost_state);
-	spin_unlock_irqrestore(shost->host_lock, flags);
 	device_for_each_child(&shost->shost_gendev, &recovery,
 			      scsi_device_cancel_cb);
 	wait_event(shost->host_wait, (!test_bit(SHOST_RECOVERY,
@@ -74,15 +76,11 @@
  **/
 void scsi_remove_host(struct Scsi_Host *shost)
 {
-	unsigned long flags;
-
 	scsi_host_cancel(shost, 0);
 	scsi_proc_host_rm(shost);
 	scsi_forget_host(shost);
 
-	spin_lock_irqsave(shost->host_lock, flags);
 	set_bit(SHOST_DEL, &shost->shost_state);
-	spin_unlock_irqrestore(shost->host_lock, flags);
 
 	class_device_unregister(&shost->shost_classdev);
 	device_del(&shost->shost_gendev);
@@ -209,7 +207,7 @@
 
 	spin_lock_init(&shost->default_lock);
 	scsi_assign_lock(shost, &shost->default_lock);
-	INIT_LIST_HEAD(&shost->my_devices);
+	INIT_LIST_HEAD(&shost->__devices);
 	INIT_LIST_HEAD(&shost->eh_cmd_q);
 	INIT_LIST_HEAD(&shost->starved_list);
 	init_waitqueue_head(&shost->host_wait);
@@ -323,23 +321,20 @@
  **/
 struct Scsi_Host *scsi_host_lookup(unsigned short hostnum)
 {
-	struct class *class = class_get(&shost_class);
+	struct class *class = &shost_class;
 	struct class_device *cdev;
 	struct Scsi_Host *shost = ERR_PTR(-ENXIO), *p;
 
-	if (class) {
-		down_read(&class->subsys.rwsem);
-		list_for_each_entry(cdev, &class->children, node) {
-			p = class_to_shost(cdev);
-			if (p->host_no == hostnum) {
-				shost = scsi_host_get(p);
-				break;
-			}
+	down_read(&class->subsys.rwsem);
+	list_for_each_entry(cdev, &class->children, node) {
+		p = class_to_shost(cdev);
+		if (p->host_no == hostnum) {
+			shost = scsi_host_get(p);
+			break;
 		}
-		up_read(&class->subsys.rwsem);
 	}
+	up_read(&class->subsys.rwsem);
 
-	class_put(&shost_class);
 	return shost;
 }
 
===== drivers/scsi/hosts.h 1.75 vs edited =====
--- 1.75/drivers/scsi/hosts.h	Mon Sep  1 22:56:57 2003
+++ edited/drivers/scsi/hosts.h	Mon Oct 27 12:01:49 2003
@@ -1,49 +1,2 @@
-/*
- *  hosts.h Copyright (C) 1992 Drew Eckhardt
- *          Copyright (C) 1993, 1994, 1995, 1998, 1999 Eric Youngdale
- *
- *  mid to low-level SCSI driver interface header
- *      Initial versions: Drew Eckhardt
- *      Subsequent revisions: Eric Youngdale
- *
- *  <drew@colorado.edu>
- *
- *	 Modified by Eric Youngdale eric@andante.org to
- *	 add scatter-gather, multiple outstanding request, and other
- *	 enhancements.
- *
- *  Further modified by Eric Youngdale to support multiple host adapters
- *  of the same type.
- *
- *  Jiffies wrap fixes (host->resetting), 3 Dec 1998 Andrea Arcangeli
- *
- *  Restructured scsi_host lists and associated functions.
- *  September 04, 2002 Mike Anderson (andmike@us.ibm.com)
- */
-
-#ifndef _HOSTS_H
-#define _HOSTS_H
-
-#include <linux/config.h>
-
+// #warning "This file is obsolete, please use <scsi/scsi_host.h> instead"
 #include <scsi/scsi_host.h>
-
-/**
- * scsi_find_device - find a device given the host
- * @shost:	SCSI host pointer
- * @channel:	SCSI channel (zero if only one channel)
- * @pun:	SCSI target number (physical unit number)
- * @lun:	SCSI Logical Unit Number
- **/
-static inline struct scsi_device *scsi_find_device(struct Scsi_Host *shost,
-                                            int channel, int pun, int lun) {
-        struct scsi_device *sdev;
-
-	list_for_each_entry (sdev, &shost->my_devices, siblings)
-                if (sdev->channel == channel && sdev->id == pun
-                   && sdev->lun ==lun)
-                        return sdev;
-        return NULL;
-}
-
-#endif
--- 1.129/drivers/scsi/scsi.c	Sat Oct 18 01:14:06 2003
+++ edited/drivers/scsi/scsi.c	Mon Oct 27 12:01:49 2003
@@ -1,6 +1,7 @@
 /*
  *  scsi.c Copyright (C) 1992 Drew Eckhardt
  *         Copyright (C) 1993, 1994, 1995, 1999 Eric Youngdale
+ *         Copyright (C) 2002, 2003 Christoph Hellwig
  *
  *  generic mid-level SCSI driver
  *      Initial versions: Drew Eckhardt
@@ -36,7 +37,6 @@
  *  out_of_space hacks, D. Gilbert (dpg) 990608
  */
 
-#include <linux/config.h>
 #include <linux/module.h>
 #include <linux/moduleparam.h>
 #include <linux/kernel.h>
@@ -54,8 +54,8 @@
 #include <linux/kmod.h>
 #include <linux/interrupt.h>
 
+#include <scsi/scsi_host.h>
 #include "scsi.h"
-#include "hosts.h"
 
 #include "scsi_priv.h"
 #include "scsi_logging.h"
@@ -883,49 +883,124 @@
 	return depth;
 }
 
+/**
+ * scsi_device_get  -  get an addition reference to a scsi_device
+ * @sdev:	device to get a reference to
+ *
+ * Gets a reference to the scsi_device and increments the use count
+ * of the underlying LLDD module.  You must hold host_lock of the
+ * parent Scsi_Host or already have a reference when calling this.
+ */
 int scsi_device_get(struct scsi_device *sdev)
 {
-	struct class *class = class_get(&sdev_class);
-
-	if (!class)
-		goto out;
 	if (test_bit(SDEV_DEL, &sdev->sdev_state))
-		goto out;
-	if (!try_module_get(sdev->host->hostt->module))
-		goto out;
+		return -ENXIO;
 	if (!get_device(&sdev->sdev_gendev))
-		goto out_put_module;
-	atomic_inc(&sdev->access_count);
-	class_put(&sdev_class);
+		return -ENXIO;
+	if (!try_module_get(sdev->host->hostt->module)) {
+		put_device(&sdev->sdev_gendev);
+		return -ENXIO;
+	}
 	return 0;
+}
+EXPORT_SYMBOL(scsi_device_get);
 
- out_put_module:
+/**
+ * scsi_device_put  -  release a reference to a scsi_device
+ * @sdev:	device to release a reference on.
+ *
+ * Release a reference to the scsi_device and decrements the use count
+ * of the underlying LLDD module.  The device is freed once the last
+ * user vanishes.
+ */
+void scsi_device_put(struct scsi_device *sdev)
+{
 	module_put(sdev->host->hostt->module);
- out:
-	class_put(&sdev_class);
-	return -ENXIO;
+	put_device(&sdev->sdev_gendev);
 }
+EXPORT_SYMBOL(scsi_device_put);
 
-void scsi_device_put(struct scsi_device *sdev)
+/* helper for shost_for_each_device, thus not documented */
+struct scsi_device *__scsi_iterate_devices(struct Scsi_Host *shost,
+					   struct scsi_device *prev)
 {
-	struct class *class = class_get(&sdev_class);
+	struct list_head *list = (prev ? &prev->siblings : &shost->__devices);
+	struct scsi_device *next = NULL;
+	unsigned long flags;
 
-	if (!class)
-		return;
+	spin_lock_irqsave(shost->host_lock, flags);
+	while (list->next != &shost->__devices) {
+		next = list_entry(list->next, struct scsi_device, siblings);
+		/* skip devices that we can't get a reference to */
+		if (!scsi_device_get(next))
+			break;
+		list = list->next;
+	}
+	spin_unlock_irqrestore(shost->host_lock, flags);
 
-	module_put(sdev->host->hostt->module);
-	atomic_dec(&sdev->access_count);
-	put_device(&sdev->sdev_gendev);
-	class_put(&sdev_class);
+	if (prev)
+		scsi_device_put(prev);
+	return next;
 }
+EXPORT_SYMBOL(__scsi_iterate_devices);
 
-int scsi_device_cancel_cb(struct device *dev, void *data)
+/**
+ * scsi_device_lookup - find a device given the host (UNLOCKED)
+ * @shost:	SCSI host pointer
+ * @channel:	SCSI channel (zero if only one channel)
+ * @pun:	SCSI target number (physical unit number)
+ * @lun:	SCSI Logical Unit Number
+ *
+ * Looks up the scsi_device with the specified @channel, @id, @lun for a
+ * give host. The returned scsi_device does not have an additional reference.
+ * You must hold the host's host_lock over this call and any access to the
+ * returned scsi_device.
+ *
+ * Note:  The only reason why drivers would want to use this is because
+ * they're need to access the device list in irq context.  Otherwise you
+ * really want to use scsi_device_lookup instead.
+ **/
+struct scsi_device *__scsi_device_lookup(struct Scsi_Host *shost,
+		uint channel, uint id, uint lun)
+{
+	struct scsi_device *sdev;
+
+	list_for_each_entry(sdev, &shost->__devices, siblings) {
+		if (sdev->channel == channel && sdev->id == id &&
+				sdev->lun ==lun)
+			return sdev;
+	}
+
+	return NULL;
+}
+EXPORT_SYMBOL(__scsi_device_lookup);
+
+/**
+ * scsi_device_lookup - find a device given the host
+ * @shost:	SCSI host pointer
+ * @channel:	SCSI channel (zero if only one channel)
+ * @id:		SCSI target number (physical unit number)
+ * @lun:	SCSI Logical Unit Number
+ *
+ * Looks up the scsi_device with the specified @channel, @id, @lun for a
+ * give host.  The returned scsi_device has an additional reference that
+ * needs to be release with scsi_host_put once you're done with it.
+ **/
+struct scsi_device *scsi_device_lookup(struct Scsi_Host *shost,
+		uint channel, uint id, uint lun)
 {
-	struct scsi_device *sdev = to_scsi_device(dev);
-	int recovery = *(int *)data;
+	struct scsi_device *sdev;
+	unsigned long flags;
+
+	spin_lock_irqsave(shost->host_lock, flags);
+	sdev = __scsi_device_lookup(shost, channel, id, lun);
+	if (sdev && scsi_device_get(sdev))
+		sdev = NULL;
+	spin_unlock_irqrestore(shost->host_lock, flags);
 
-	return scsi_device_cancel(sdev, recovery);
+	return sdev;
 }
+EXPORT_SYMBOL(scsi_device_lookup);
 
 /**
  * scsi_device_cancel - cancel outstanding IO to this device
===== drivers/scsi/scsi.h 1.91 vs edited =====
--- 1.91/drivers/scsi/scsi.h	Sat Sep 20 13:50:50 2003
+++ edited/drivers/scsi/scsi.h	Mon Oct 27 12:01:41 2003
@@ -116,12 +116,6 @@
 #define scsi_to_pci_dma_dir(scsi_dir)	((int)(scsi_dir))
 #define scsi_to_sbus_dma_dir(scsi_dir)	((int)(scsi_dir))
 
-/*
- * This is the crap from the old error handling code.  We have it in a special
- * place so that we can more easily delete it later on.
- */
-#include "scsi_obsolete.h"
-
 /* obsolete typedef junk. */
 #include "scsi_typedefs.h"
 
--- 1.7/drivers/scsi/scsi_devinfo.c	Thu Oct 16 10:56:58 2003
+++ edited/drivers/scsi/scsi_devinfo.c	Mon Oct 27 12:01:49 2003
@@ -6,10 +6,11 @@
 #include <linux/moduleparam.h>
 #include <linux/proc_fs.h>
 #include <linux/seq_file.h>
-#include <scsi/scsi_devinfo.h>
 
+#include <scsi/scsi_devinfo.h>
+#include <scsi/scsi_host.h>
 #include "scsi.h"
-#include "hosts.h"
+
 #include "scsi_priv.h"
 
 /*
===== drivers/scsi/scsi_error.c 1.65 vs edited =====
--- 1.65/drivers/scsi/scsi_error.c	Sun Sep 21 19:49:36 2003
+++ edited/drivers/scsi/scsi_error.c	Mon Oct 27 12:01:49 2003
@@ -22,11 +22,10 @@
 #include <linux/kernel.h>
 #include <linux/interrupt.h>
 #include <linux/blkdev.h>
-#include <linux/smp_lock.h>
-#include <scsi/scsi_ioctl.h>
 
+#include <scsi/scsi_host.h>
+#include <scsi/scsi_ioctl.h>
 #include "scsi.h"
-#include "hosts.h"
 
 #include "scsi_priv.h"
 #include "scsi_logging.h"
===== drivers/scsi/scsi_ioctl.c 1.24 vs edited =====
--- 1.24/drivers/scsi/scsi_ioctl.c	Mon Aug 25 15:37:40 2003
+++ edited/drivers/scsi/scsi_ioctl.c	Mon Oct 27 12:01:49 2003
@@ -17,11 +17,11 @@
 #include <linux/sched.h>
 #include <linux/mm.h>
 #include <linux/string.h>
-
 #include <linux/blkdev.h>
-#include "scsi.h"
-#include "hosts.h"
+
+#include <scsi/scsi_host.h>
 #include <scsi/scsi_ioctl.h>
+#include "scsi.h"
 
 #include "scsi_logging.h"
 
===== drivers/scsi/scsi_lib.c 1.113 vs edited =====
--- 1.113/drivers/scsi/scsi_lib.c	Sat Sep 20 15:53:02 2003
+++ edited/drivers/scsi/scsi_lib.c	Mon Oct 27 12:01:52 2003
@@ -16,9 +16,9 @@
 #include <linux/init.h>
 #include <linux/pci.h>
 
-#include "scsi.h"
-#include "hosts.h"
 #include <scsi/scsi_driver.h>
+#include <scsi/scsi_host.h>
+#include "scsi.h"
 
 #include "scsi_priv.h"
 #include "scsi_logging.h"
@@ -335,13 +335,14 @@
  */
 static void scsi_single_lun_run(struct scsi_device *current_sdev)
 {
-	struct scsi_device *sdev;
+	struct Scsi_Host *shost = current_sdev->host;
+	struct scsi_device *sdev, *tmp;
 	unsigned long flags;
 
-	spin_lock_irqsave(current_sdev->host->host_lock, flags);
+	spin_lock_irqsave(shost->host_lock, flags);
 	WARN_ON(!current_sdev->sdev_target->starget_sdev_user);
 	current_sdev->sdev_target->starget_sdev_user = NULL;
-	spin_unlock_irqrestore(current_sdev->host->host_lock, flags);
+	spin_unlock_irqrestore(shost->host_lock, flags);
 
 	/*
 	 * Call blk_run_queue for all LUNs on the target, starting with
@@ -351,21 +352,26 @@
 	 */
 	blk_run_queue(current_sdev->request_queue);
 
-	spin_lock_irqsave(current_sdev->host->host_lock, flags);
-	if (current_sdev->sdev_target->starget_sdev_user) {
-		/*
-		 * After unlock, this races with anyone clearing
-		 * starget_sdev_user, but we (should) always enter this
-		 * function again, avoiding any problems.
-		 */
-		spin_unlock_irqrestore(current_sdev->host->host_lock, flags);
-		return;
-	}
-	spin_unlock_irqrestore(current_sdev->host->host_lock, flags);
+	/*
+	 * After unlock, this races with anyone clearing starget_sdev_user,
+	 * but we always enter this function again, avoiding any problems.
+	 */
+	spin_lock_irqsave(shost->host_lock, flags);
+	if (current_sdev->sdev_target->starget_sdev_user)
+		goto out;
+	list_for_each_entry_safe(sdev, tmp, &current_sdev->same_target_siblings,
+			same_target_siblings) {
+		if (scsi_device_get(sdev))
+			continue;
 
-	list_for_each_entry(sdev, &current_sdev->same_target_siblings,
-			    same_target_siblings)
+		spin_unlock_irqrestore(shost->host_lock, flags);
 		blk_run_queue(sdev->request_queue);
+		spin_lock_irqsave(shost->host_lock, flags);
+	
+		scsi_device_put(sdev);
+	}
+ out:
+	spin_unlock_irqrestore(shost->host_lock, flags);
 }
 
 /*
===== drivers/scsi/scsi_module.c 1.6 vs edited =====
--- 1.6/drivers/scsi/scsi_module.c	Thu Jul 31 16:32:16 2003
+++ edited/drivers/scsi/scsi_module.c	Mon Oct 27 12:01:50 2003
@@ -11,8 +11,8 @@
 #include <linux/kernel.h>
 #include <linux/module.h>
 
+#include <scsi/scsi_host.h>
 #include "scsi.h"
-#include "hosts.h"
 
 
 static int __init init_this_scsi_driver(void)
--- 1.4/drivers/scsi/scsi_pc98.c	Fri May  2 21:28:28 2003
+++ edited/drivers/scsi/scsi_pc98.c	Mon Oct 27 12:01:50 2003
@@ -11,8 +11,8 @@
 #include <linux/blkdev.h>
 #include <asm/pc9800.h>
 
+#include <scsi/scsi_host.h>
 #include "scsi.h"
-#include "hosts.h"
 
 
 static int pc98_first_bios_param(struct scsi_device *sdev, int *ip)
--- 1.38/drivers/scsi/scsi_proc.c	Sat Sep 20 11:35:07 2003
+++ edited/drivers/scsi/scsi_proc.c	Mon Oct 27 12:01:50 2003
@@ -27,8 +27,8 @@
 #include <linux/seq_file.h>
 #include <asm/uaccess.h>
 
+#include <scsi/scsi_host.h>
 #include "scsi.h"
-#include "hosts.h"
 
 #include "scsi_priv.h"
 #include "scsi_logging.h"
@@ -212,15 +212,13 @@
 	shost = scsi_host_lookup(host);
 	if (IS_ERR(shost))
 		return PTR_ERR(shost);
-	sdev = scsi_find_device(shost, channel, id, lun);
-	if (!sdev)
-		goto out;
-	if (atomic_read(&sdev->access_count))
-		goto out;
+	sdev = scsi_device_lookup(shost, channel, id, lun);
+	if (sdev) {
+		scsi_remove_device(sdev);
+		scsi_device_put(sdev);
+		error = 0;
+	}
 
-	scsi_remove_device(sdev);
-	error = 0;
-out:
 	scsi_host_put(shost);
 	return error;
 }
===== drivers/scsi/scsi_scan.c 1.109 vs edited =====
--- 1.109/drivers/scsi/scsi_scan.c	Thu Oct 16 10:56:58 2003
+++ edited/drivers/scsi/scsi_scan.c	Mon Oct 27 12:01:50 2003
@@ -32,10 +32,10 @@
 #include <linux/blkdev.h>
 #include <asm/semaphore.h>
 
-#include "scsi.h"
-#include "hosts.h"
 #include <scsi/scsi_driver.h>
 #include <scsi/scsi_devinfo.h>
+#include <scsi/scsi_host.h>
+#include "scsi.h"
 
 #include "scsi_priv.h"
 #include "scsi_logging.h"
@@ -190,13 +190,13 @@
 	       	uint channel, uint id, uint lun)
 {
 	struct scsi_device *sdev, *device;
+	unsigned long flags;
 
 	sdev = kmalloc(sizeof(*sdev), GFP_ATOMIC);
 	if (!sdev)
 		goto out;
 
 	memset(sdev, 0, sizeof(*sdev));
-	atomic_set(&sdev->access_count, 0);
 	sdev->vendor = scsi_null_device_strs;
 	sdev->model = scsi_null_device_strs;
 	sdev->rev = scsi_null_device_strs;
@@ -240,7 +240,8 @@
 	 * If there are any same target siblings, add this to the
 	 * sibling list
 	 */
-	list_for_each_entry(device, &shost->my_devices, siblings) {
+	spin_lock_irqsave(shost->host_lock, flags);
+	list_for_each_entry(device, &shost->__devices, siblings) {
 		if (device->id == sdev->id &&
 		    device->channel == sdev->channel) {
 			list_add_tail(&sdev->same_target_siblings,
@@ -258,10 +259,8 @@
 	if (!sdev->scsi_level)
 		sdev->scsi_level = SCSI_2;
 
-	/*
-	 * Add it to the end of the shost->my_devices list.
-	 */
-	list_add_tail(&sdev->siblings, &shost->my_devices);
+	list_add_tail(&sdev->siblings, &shost->__devices);
+	spin_unlock_irqrestore(shost->host_lock, flags);
 	return sdev;
 
 out_free_queue:
@@ -285,21 +284,21 @@
 {
 	unsigned long flags;
 
+	spin_lock_irqsave(sdev->host->host_lock, flags);
 	list_del(&sdev->siblings);
 	list_del(&sdev->same_target_siblings);
+	spin_unlock_irqrestore(sdev->host->host_lock, flags);
 
 	if (sdev->request_queue)
 		scsi_free_queue(sdev->request_queue);
-	if (sdev->inquiry)
-		kfree(sdev->inquiry);
+
 	spin_lock_irqsave(sdev->host->host_lock, flags);
 	list_del(&sdev->starved_entry);
-	if (sdev->single_lun) {
-		if (--sdev->sdev_target->starget_refcnt == 0)
-			kfree(sdev->sdev_target);
-	}
+	if (sdev->single_lun && --sdev->sdev_target->starget_refcnt == 0)
+		kfree(sdev->sdev_target);
 	spin_unlock_irqrestore(sdev->host->host_lock, flags);
 
+	kfree(sdev->inquiry);
 	kfree(sdev);
 }
 
@@ -678,7 +677,7 @@
 	 * host adapter calls into here with rescan == 0.
 	 */
 	if (rescan) {
-		sdev = scsi_find_device(host, channel, id, lun);
+		sdev = scsi_device_lookup(host, channel, id, lun);
 		if (sdev) {
 			SCSI_LOG_SCAN_BUS(3, printk(KERN_INFO
 				"scsi scan: device exists on <%d:%d:%d:%d>\n",
@@ -689,6 +688,8 @@
 				*bflagsp = scsi_get_device_flags(sdev,
 								 sdev->vendor,
 								 sdev->model);
+			/* XXX: bandaid until callers do refcounting */
+			scsi_device_put(sdev);
 			return SCSI_SCAN_LUN_PRESENT;
 		}
 	}
@@ -1232,14 +1233,25 @@
 
 void scsi_forget_host(struct Scsi_Host *shost)
 {
-	struct list_head *le, *lh;
-	struct scsi_device *sdev;
+	struct scsi_device *sdev, *tmp;
+	unsigned long flags;
 
-	list_for_each_safe(le, lh, &shost->my_devices) {
-		sdev = list_entry(le, struct scsi_device, siblings);
-		
+	/*
+	 * Ok, this look a bit strange.  We always look for the first device
+	 * on the list as scsi_remove_device removes them from it - thus we
+	 * also have to release the lock.
+	 * We don't need to get another reference to the device before
+	 * releasing the lock as we already own the reference from
+	 * scsi_register_device that's release in scsi_remove_device.  And
+	 * after that we don't look at sdev anymore.
+	 */
+	spin_lock_irqsave(shost->host_lock, flags);
+	list_for_each_entry_safe(sdev, tmp, &shost->__devices, siblings) {
+		spin_unlock_irqrestore(shost->host_lock, flags);
 		scsi_remove_device(sdev);
+		spin_lock_irqsave(shost->host_lock, flags);
 	}
+	spin_unlock_irqrestore(shost->host_lock, flags);
 }
 
 /*
===== drivers/scsi/scsi_syms.c 1.45 vs edited =====
--- 1.45/drivers/scsi/scsi_syms.c	Thu Jul 31 16:32:18 2003
+++ edited/drivers/scsi/scsi_syms.c	Mon Oct 27 12:01:50 2003
@@ -18,13 +18,14 @@
 #include <asm/irq.h>
 #include <asm/dma.h>
 
-#include "scsi.h"
 #include <scsi/scsi_driver.h>
+#include <scsi/scsi_host.h>
 #include <scsi/scsi_ioctl.h>
-#include "hosts.h"
+#include <scsi/scsicam.h>
+#include "scsi.h"
+
 #include "scsi_logging.h"
 
-#include <scsi/scsicam.h>
 
 /*
  * This source file contains the symbol table used by scsi loadable
@@ -82,8 +83,6 @@
 
 EXPORT_SYMBOL(scsi_io_completion);
 
-EXPORT_SYMBOL(scsi_device_get);
-EXPORT_SYMBOL(scsi_device_put);
 EXPORT_SYMBOL(scsi_add_device);
 EXPORT_SYMBOL(scsi_remove_device);
 EXPORT_SYMBOL(scsi_device_cancel);
===== drivers/scsi/scsi_sysfs.c 1.35 vs edited =====
--- 1.35/drivers/scsi/scsi_sysfs.c	Sat Oct 18 01:34:55 2003
+++ edited/drivers/scsi/scsi_sysfs.c	Mon Oct 27 12:01:50 2003
@@ -11,8 +11,9 @@
 #include <linux/init.h>
 #include <linux/blkdev.h>
 #include <linux/device.h>
+
+#include <scsi/scsi_host.h>
 #include "scsi.h"
-#include "hosts.h"
 
 #include "scsi_priv.h"
 #include "scsi_logging.h"
@@ -257,20 +258,12 @@
 	scsi_rescan_device(dev);
 	return count;
 }
-
 static DEVICE_ATTR(rescan, S_IWUSR, NULL, store_rescan_field)
 
 static ssize_t sdev_store_delete(struct device *dev, const char *buf,
 				 size_t count)
 {
-	struct scsi_device *sdev = to_scsi_device(dev);
-
-	/*
-	 * FIXME and scsi_proc.c: racey use of access_count,
-	 */
-	if (atomic_read(&sdev->access_count))
-		return -EBUSY;
-	scsi_remove_device(sdev);
+	scsi_remove_device(to_scsi_device(dev));
 	return count;
 };
 static DEVICE_ATTR(delete, S_IWUSR, NULL, sdev_store_delete);
@@ -403,22 +396,11 @@
  **/
 void scsi_remove_device(struct scsi_device *sdev)
 {
-	struct class *class = class_get(&sdev_class);
-
 	class_device_unregister(&sdev->sdev_classdev);
-
-	if (class) {
-		down_write(&class->subsys.rwsem);
-		set_bit(SDEV_DEL, &sdev->sdev_state);
-		if (sdev->host->hostt->slave_destroy)
-			sdev->host->hostt->slave_destroy(sdev);
-		device_del(&sdev->sdev_gendev);
-		up_write(&class->subsys.rwsem);
-	}
-
+	if (sdev->host->hostt->slave_destroy)
+		sdev->host->hostt->slave_destroy(sdev);
+	device_del(&sdev->sdev_gendev);
 	put_device(&sdev->sdev_gendev);
-
-	class_put(&sdev_class);
 }
 
 int scsi_register_driver(struct device_driver *drv)
===== drivers/scsi/sg.c 1.70 vs edited =====
--- 1.70/drivers/scsi/sg.c	Tue Oct 21 00:15:18 2003
+++ edited/drivers/scsi/sg.c	Mon Oct 27 12:01:50 2003
@@ -914,7 +914,8 @@
 	case SG_GET_VERSION_NUM:
 		return put_user(sg_version_num, (int *) arg);
 	case SG_GET_ACCESS_COUNT:
-		val = (sdp->device ? atomic_read(&sdp->device->access_count) : 0);
+		/* faked - we don't have a real access count anymore */
+		val = (sdp->device ? 1 : 0);
 		return put_user(val, (int *) arg);
 	case SG_GET_REQUEST_TABLE:
 		result = verify_area(VERIFY_WRITE, (void *) arg,
@@ -2903,7 +2904,7 @@
 			PRINT_PROC("%d\t%d\t%d\t%d\t%d\t%d\t%d\t%d\t%d\n",
 				   scsidp->host->host_no, scsidp->channel,
 				   scsidp->id, scsidp->lun, (int) scsidp->type,
-				   (int) atomic_read(&scsidp->access_count),
+				   1,
 				   (int) scsidp->queue_depth,
 				   (int) scsidp->device_busy,
 				   (int) scsidp->online);
===== drivers/scsi/arm/acornscsi.c 1.35 vs edited =====
--- 1.35/drivers/scsi/arm/acornscsi.c	Mon Aug 25 15:37:34 2003
+++ edited/drivers/scsi/arm/acornscsi.c	Mon Oct 27 12:01:49 2003
@@ -2930,7 +2930,7 @@
 
     p += sprintf(p, "\nAttached devices:\n");
 
-    list_for_each_entry(scd, &instance->my_devices, siblings) {
+    shost_for_each_device(scd, instance) {
 	p += sprintf(p, "Device/Lun TaggedQ      Sync\n");
 	p += sprintf(p, "     %d/%d   ", scd->id, scd->lun);
 	if (scd->tagged_supported)
@@ -2953,8 +2953,10 @@
 	    p = buffer;
 	}
 	pos = p - buffer;
-	if (pos + begin > offset + length)
+	if (pos + begin > offset + length) {
+	    scsi_device_put(scd);
 	    break;
+	}
     }
 
     pos = p - buffer;
--- 1.9/include/scsi/scsi_device.h	Thu Oct 16 10:56:58 2003
+++ edited/include/scsi/scsi_device.h	Mon Oct 27 12:01:52 2003
@@ -22,10 +22,13 @@
 };
 
 struct scsi_device {
-	struct list_head    siblings;   /* list of all devices on this host */
-	struct list_head    same_target_siblings; /* just the devices sharing same target id */
 	struct Scsi_Host *host;
 	struct request_queue *request_queue;
+
+	/* the next two are protected by the host->host_lock */
+	struct list_head    siblings;   /* list of all devices on this host */
+	struct list_head    same_target_siblings; /* just the devices sharing same target id */
+
 	volatile unsigned short device_busy;	/* commands actually active on low-level */
 	spinlock_t sdev_lock;           /* also the request queue_lock */
 	spinlock_t list_lock;
@@ -45,8 +48,6 @@
 					 * vendor-specific cmd's */
 	unsigned sector_size;	/* size in bytes */
 
-	atomic_t access_count;	/* Count of open channels/mounts */
-
 	void *hostdata;		/* available to low-level driver */
 	char devfs_name[256];	/* devfs junk */
 	char type;
@@ -108,14 +109,48 @@
 extern struct scsi_device *scsi_add_device(struct Scsi_Host *,
 		uint, uint, uint);
 extern void scsi_remove_device(struct scsi_device *);
-extern int scsi_device_cancel_cb(struct device *, void *);
 extern int scsi_device_cancel(struct scsi_device *, int);
 
 extern int scsi_device_get(struct scsi_device *);
 extern void scsi_device_put(struct scsi_device *);
-
+extern struct scsi_device *scsi_device_lookup(struct Scsi_Host *,
+					      uint, uint, uint);
+extern struct scsi_device *__scsi_device_lookup(struct Scsi_Host *,
+						uint, uint, uint);
+
+/* only exposed to implement shost_for_each_device */
+extern struct scsi_device *__scsi_iterate_devices(struct Scsi_Host *,
+						  struct scsi_device *);
+
+/**
+ * shost_for_each_device  -  iterate over all devices of a host
+ * @sdev:	iterator
+ * @host:	host whiches devices we want to iterate over
+ *
+ * This traverses over each devices of @shost.  The devices have
+ * a reference that must be released by scsi_host_put when breaking
+ * out of the loop.
+ */
 #define shost_for_each_device(sdev, shost) \
-	list_for_each_entry((sdev), &((shost)->my_devices), siblings)
+	for ((sdev) = __scsi_iterate_devices((shost), NULL); \
+	     (sdev); \
+	     (sdev) = __scsi_iterate_devices((shost), (sdev)))
+
+/**
+ * __shost_for_each_device  -  iterate over all devices of a host (UNLOCKED)
+ * @sdev:	iterator
+ * @host:	host whiches devices we want to iterate over
+ *
+ * This traverses over each devices of @shost.  It does _not_ take a
+ * reference on the scsi_device, thus it the whole loop must be protected
+ * by shost->host_lock.
+ *
+ * Note:  The only reason why drivers would want to use this is because
+ * they're need to access the device list in irq context.  Otherwise you
+ * really want to use shost_for_each_device instead.
+ */
+#define __shost_for_each_device(sdev, shost) \
+	list_for_each_entry((sdev), &((shost)->__devices), siblings)
 
 extern void scsi_adjust_queue_depth(struct scsi_device *, int, int);
 extern int scsi_track_queue_full(struct scsi_device *, int);
===== include/scsi/scsi_driver.h 1.1 vs edited =====
--- 1.1/include/scsi/scsi_driver.h	Thu Jun 26 19:08:24 2003
+++ edited/include/scsi/scsi_driver.h	Mon Oct 27 12:01:50 2003
@@ -4,6 +4,7 @@
 #include <linux/device.h>
 
 struct module;
+struct scsi_cmnd;
 
 
 struct scsi_driver {
===== include/scsi/scsi_host.h 1.12 vs edited =====
--- 1.12/include/scsi/scsi_host.h	Thu Oct 16 10:56:58 2003
+++ edited/include/scsi/scsi_host.h	Mon Oct 27 12:01:50 2003
@@ -125,15 +125,6 @@
 	int (* eh_host_reset_handler)(struct scsi_cmnd *);
 
 	/*
-	 * Old EH handlers, no longer used. Make them warn the user of old
-	 * drivers by using a wrong type
-	 *
-	 * Status: MORE THAN OBSOLETE
-	 */
-	int (* abort)(int);
-	int (* reset)(int, int);
-
-	/*
 	 * Before the mid layer attempts to scan for a new device where none
 	 * currently exists, it will call this entry in your driver.  Should
 	 * your driver need to allocate any structs or perform any other init
@@ -363,19 +354,30 @@
 };
 
 struct Scsi_Host {
-	struct list_head	my_devices;
+	/*
+	 * __devices is protected by the host_lock, but you should
+	 * usually use scsi_device_lookup / shost_for_each_device
+	 * to access it and don't care about locking yourself.
+	 * In the rare case of beeing in irq context you can use
+	 * their __ prefixed variants with the lock held. NEVER
+	 * access this list directly from a driver.
+	 */
+	struct list_head	__devices;
+	
 	struct scsi_host_cmd_pool *cmd_pool;
 	spinlock_t		free_list_lock;
-	struct list_head	free_list;   /* backup store of cmd structs */
+	struct list_head	free_list; /* backup store of cmd structs */
 	struct list_head	starved_list;
 
 	spinlock_t		default_lock;
 	spinlock_t		*host_lock;
 
+	struct semaphore	scan_mutex;/* serialize scanning activity */
+
 	struct list_head	eh_cmd_q;
 	struct task_struct    * ehandler;  /* Error recovery thread. */
-	struct semaphore      * eh_wait;   /* The error recovery thread waits on
-                                          this. */
+	struct semaphore      * eh_wait;   /* The error recovery thread waits
+					      on this. */
 	struct completion     * eh_notify; /* wait for eh to begin or end */
 	struct semaphore      * eh_action; /* Wait for specific actions on the
                                           host. */
@@ -477,12 +479,6 @@
 	 * module_init/module_exit.
 	 */
 	struct list_head sht_legacy_list;
-
-	/*
-	 * This mutex serializes all scsi scanning activity from kernel- and
-	 * userspace.
-	 */
-	struct semaphore scan_mutex;
 
 	/*
 	 * We should ensure that this is aligned, both for better performance

Re: [PATCH] scsi_device refcounting and list lockdown

[Randy.Dunlap]

On Mon, 27 Oct 2003 16:57:13 +0100 Christoph Hellwig <hch@lst.de> wrote:

| --- 1.129/drivers/scsi/scsi.c	Sat Oct 18 01:14:06 2003
| +++ edited/drivers/scsi/scsi.c	Mon Oct 27 12:01:49 2003
| @@ -883,49 +883,124 @@
|  	return depth;
|  }
|  
| +/**
| + * scsi_device_get  -  get an addition reference to a scsi_device
| + * @sdev:	device to get a reference to
| + *
| + * Gets a reference to the scsi_device and increments the use count
| + * of the underlying LLDD module.  You must hold host_lock of the
| + * parent Scsi_Host or already have a reference when calling this.
| + */
|  int scsi_device_get(struct scsi_device *sdev)
|  {
| -	struct class *class = class_get(&sdev_class);
| -
| -	if (!class)
| -		goto out;
|  	if (test_bit(SDEV_DEL, &sdev->sdev_state))
| -		goto out;
| -	if (!try_module_get(sdev->host->hostt->module))
| -		goto out;
| +		return -ENXIO;
|  	if (!get_device(&sdev->sdev_gendev))
| -		goto out_put_module;
| -	atomic_inc(&sdev->access_count);
| -	class_put(&sdev_class);
| +		return -ENXIO;
| +	if (!try_module_get(sdev->host->hostt->module)) {
| +		put_device(&sdev->sdev_gendev);
| +		return -ENXIO;
| +	}
|  	return 0;
| +}
| +EXPORT_SYMBOL(scsi_device_get);
|  
| - out_put_module:
| +/**
| + * scsi_device_put  -  release a reference to a scsi_device
| + * @sdev:	device to release a reference on.
| + *
| + * Release a reference to the scsi_device and decrements the use count
| + * of the underlying LLDD module.  The device is freed once the last
| + * user vanishes.
| + */
| +void scsi_device_put(struct scsi_device *sdev)
| +{
|  	module_put(sdev->host->hostt->module);
| - out:
| -	class_put(&sdev_class);
| -	return -ENXIO;
| +	put_device(&sdev->sdev_gendev);
|  }
| +EXPORT_SYMBOL(scsi_device_put);


Hi,

Even before this patch (which isn't merged AFAIK), are
scsi_device_get() and scsi_device_put() intended to be used
by SCSI LLDD's for scsi_device reference counts?

I'm trying to determine what needs to be done to fix aha152x.c,
where it creates a struct scsi_cmnd and then inits as follows,
with my example patch:


description:	busfree_run() allocates a cmnd and references
		cmnd->device->... without setting cmd->device
product_versions: Linux 2.6.0-test9
patch_name:	aha152x-device-ptr.patch
author:		Randy.Dunlap <rddunlap@osdl.org>
patch_version:	2003-10-27.10:54:42

diffstat:=
 drivers/scsi/aha152x.c |    1 +
 1 files changed, 1 insertion(+)

diff -Naurp ./drivers/scsi/aha152x.c~aha152xfix ./drivers/scsi/aha152x.c
--- ./drivers/scsi/aha152x.c~aha152xfix	2003-10-25 11:42:50.000000000 -0700
+++ ./drivers/scsi/aha152x.c	2003-10-27 10:54:09.000000000 -0800
@@ -2018,6 +2018,7 @@ static void busfree_run(struct Scsi_Host
 					cmnd->cmnd[4]         = sizeof(ptr->sense_buffer);
 					cmnd->cmnd[5]         = 0;
 					cmnd->cmd_len	      = 6;
+					cmnd->device          = ptr->device;
 					cmnd->device->host    = ptr->device->host;
 					cmnd->device->id      = ptr->device->id;
 					cmnd->device->lun     = ptr->device->lun;




Thanks,
--
~Randy

Re: [PATCH] scsi_device refcounting and list lockdown

[Mike Anderson]

I applied your patch to test9 and ran a few cycles of adds and removes
using scsi_debug.

A few comments below.

Christoph Hellwig [hch@lst.de] wrote:
> @@ -1232,14 +1233,25 @@
>  
>  void scsi_forget_host(struct Scsi_Host *shost)
>  {
> -	struct list_head *le, *lh;
> -	struct scsi_device *sdev;
> +	struct scsi_device *sdev, *tmp;
> +	unsigned long flags;
>  
> -	list_for_each_safe(le, lh, &shost->my_devices) {
> -		sdev = list_entry(le, struct scsi_device, siblings);
> -		
> +	/*
> +	 * Ok, this look a bit strange.  We always look for the first device
> +	 * on the list as scsi_remove_device removes them from it - thus we
> +	 * also have to release the lock.
> +	 * We don't need to get another reference to the device before
> +	 * releasing the lock as we already own the reference from
> +	 * scsi_register_device that's release in scsi_remove_device.  And
> +	 * after that we don't look at sdev anymore.
> +	 */
> +	spin_lock_irqsave(shost->host_lock, flags);
> +	list_for_each_entry_safe(sdev, tmp, &shost->__devices, siblings) {
> +		spin_unlock_irqrestore(shost->host_lock, flags);
>  		scsi_remove_device(sdev);
> +		spin_lock_irqsave(shost->host_lock, flags);
>  	}
> +	spin_unlock_irqrestore(shost->host_lock, flags);
>  }
>  
>  /*

Do we still have issues with other callers of scsi_remove_device?

> @@ -403,22 +396,11 @@
>   **/
>  void scsi_remove_device(struct scsi_device *sdev)
>  {
> -	struct class *class = class_get(&sdev_class);
> -
>  	class_device_unregister(&sdev->sdev_classdev);
> -
> -	if (class) {
> -		down_write(&class->subsys.rwsem);
> -		set_bit(SDEV_DEL, &sdev->sdev_state);
> -		if (sdev->host->hostt->slave_destroy)
> -			sdev->host->hostt->slave_destroy(sdev);
> -		device_del(&sdev->sdev_gendev);
> -		up_write(&class->subsys.rwsem);
> -	}
> -
> +	if (sdev->host->hostt->slave_destroy)
> +		sdev->host->hostt->slave_destroy(sdev);
> +	device_del(&sdev->sdev_gendev);
>  	put_device(&sdev->sdev_gendev);
> -
> -	class_put(&sdev_class);
>  }
>  

Don't we still want to set SDEV_DEL to keep callers to scsi_device_get
from do more gets.

-andmike
--
Michael Anderson
andmike@us.ibm.com

Re: [PATCH] scsi_device refcounting and list lockdown

[Christoph Hellwig]

On Mon, Oct 27, 2003 at 04:01:01PM -0800, Randy.Dunlap wrote:
> Hi,
> 
> Even before this patch (which isn't merged AFAIK), are
> scsi_device_get() and scsi_device_put() intended to be used
> by SCSI LLDD's for scsi_device reference counts?

Yes, if they need to.  But usually they should not have to worry.

> I'm trying to determine what needs to be done to fix aha152x.c,
> where it creates a struct scsi_cmnd and then inits as follows,
> with my example patch:

A driver is not allowed to "create" a scsi_cmnd.  It must use
scsi_get_command to allocate one.

Re: [PATCH] scsi_device refcounting and list lockdown

[Christoph Hellwig]

On Mon, Oct 27, 2003 at 06:32:35PM -0800, Mike Anderson wrote:
> > +	/*
> > +	 * Ok, this look a bit strange.  We always look for the first device
> > +	 * on the list as scsi_remove_device removes them from it - thus we
> > +	 * also have to release the lock.
> > +	 * We don't need to get another reference to the device before
> > +	 * releasing the lock as we already own the reference from
> > +	 * scsi_register_device that's release in scsi_remove_device.  And
> > +	 * after that we don't look at sdev anymore.
> > +	 */
> > +	spin_lock_irqsave(shost->host_lock, flags);
> > +	list_for_each_entry_safe(sdev, tmp, &shost->__devices, siblings) {
> > +		spin_unlock_irqrestore(shost->host_lock, flags);
> >  		scsi_remove_device(sdev);
> > +		spin_lock_irqsave(shost->host_lock, flags);
> >  	}
> > +	spin_unlock_irqrestore(shost->host_lock, flags);
> >  }
> >  
> >  /*
> 
> Do we still have issues with other callers of scsi_remove_device?

Well, the other callers aren't walking the list until it's empty but
just remove a single device which is fine with the existing constructs.

> >  void scsi_remove_device(struct scsi_device *sdev)
> >  {
> > -	struct class *class = class_get(&sdev_class);
> > -
> >  	class_device_unregister(&sdev->sdev_classdev);
> > -
> > -	if (class) {
> > -		down_write(&class->subsys.rwsem);
> > -		set_bit(SDEV_DEL, &sdev->sdev_state);
> > -		if (sdev->host->hostt->slave_destroy)
> > -			sdev->host->hostt->slave_destroy(sdev);
> > -		device_del(&sdev->sdev_gendev);
> > -		up_write(&class->subsys.rwsem);
> > -	}
> > -
> > +	if (sdev->host->hostt->slave_destroy)
> > +		sdev->host->hostt->slave_destroy(sdev);
> > +	device_del(&sdev->sdev_gendev);
> >  	put_device(&sdev->sdev_gendev);
> > -
> > -	class_put(&sdev_class);
> >  }
> >  
> 
> Don't we still want to set SDEV_DEL to keep callers to scsi_device_get
> from do more gets.

Indeed!  James, can you add this back when applying?

Re: [PATCH] scsi_device refcounting and list lockdown

[James Bottomley]

On Tue, 2003-10-28 at 03:07, Christoph Hellwig wrote:
> > Don't we still want to set SDEV_DEL to keep callers to scsi_device_get
> > from do more gets.
> 
> Indeed!  James, can you add this back when applying?

OK, this should be in place on the scsi-bugfixes-2.6 tree if you could
check it out.

James

Re: [PATCH] scsi_device refcounting and list lockdown

[Christoph Hellwig]

On Tue, Oct 28, 2003 at 09:52:42AM -0600, James Bottomley wrote:
> On Tue, 2003-10-28 at 03:07, Christoph Hellwig wrote:
> > > Don't we still want to set SDEV_DEL to keep callers to scsi_device_get
> > > from do more gets.
> > 
> > Indeed!  James, can you add this back when applying?
> 
> OK, this should be in place on the scsi-bugfixes-2.6 tree if you could
> check it out.

Looks fine to me.

[PATCH] [v2] aha152x cmnd->device oops

[Randy.Dunlap]

On Tue, 28 Oct 2003 10:06:00 +0100 Christoph Hellwig <hch@lst.de> wrote:

| On Mon, Oct 27, 2003 at 04:01:01PM -0800, Randy.Dunlap wrote:
| > Hi,
| > 
| > Even before this patch (which isn't merged AFAIK), are
| > scsi_device_get() and scsi_device_put() intended to be used
| > by SCSI LLDD's for scsi_device reference counts?
| 
| Yes, if they need to.  But usually they should not have to worry.
| 
| > I'm trying to determine what needs to be done to fix aha152x.c,
| > where it creates a struct scsi_cmnd and then inits as follows,
| > with my example patch:
| 
| A driver is not allowed to "create" a scsi_cmnd.  It must use
| scsi_get_command to allocate one.

Here's the updated patch.  Comments on it?

Thanks for your help, Christoph.

--
~Randy


description:	(a) aha152x oopses when it references cmnd->device->...
		before cmd->device has been init;
		(b) use scsi_get_command() instead of kmalloc() to
		allocate a scsi_cmnd;
		(c) no need to check/free cmnd on failure;
maintainer:	Juergen Fischer <fischer@norbit.de>
product_versions: Linux 2.6.0-test9
patch_name:	aha152x-cmd-dev.patch
author:		Randy.Dunlap <rddunlap@osdl.org>
patch_version:	2003-10-28.12:35:35

diffstat:=
 drivers/scsi/aha152x.c |    5 ++---
 1 files changed, 2 insertions(+), 3 deletions(-)

diff -Naurp ./drivers/scsi/aha152x.c~aha152xfix ./drivers/scsi/aha152x.c
--- ./drivers/scsi/aha152x.c~aha152xfix	2003-10-25 11:42:50.000000000 -0700
+++ ./drivers/scsi/aha152x.c	2003-10-28 12:32:48.000000000 -0800
@@ -2001,7 +2001,7 @@ static void busfree_run(struct Scsi_Host
 #endif
 
 			if(!(DONE_SC->SCp.Status & not_issued)) {
-				Scsi_Cmnd *cmnd = kmalloc(sizeof(Scsi_Cmnd), GFP_ATOMIC);
+				Scsi_Cmnd *cmnd = scsi_get_command(DONE_SC->device, GFP_ATOMIC);
 
 				if(cmnd) {
 					Scsi_Cmnd *ptr=DONE_SC;
@@ -2018,6 +2018,7 @@ static void busfree_run(struct Scsi_Host
 					cmnd->cmnd[4]         = sizeof(ptr->sense_buffer);
 					cmnd->cmnd[5]         = 0;
 					cmnd->cmd_len	      = 6;
+					cmnd->device          = ptr->device;
 					cmnd->device->host    = ptr->device->host;
 					cmnd->device->id      = ptr->device->id;
 					cmnd->device->lun     = ptr->device->lun;
@@ -2030,8 +2031,6 @@ static void busfree_run(struct Scsi_Host
 					DO_LOCK(flags);
 				} else {
 					printk(ERR_LEAD "allocation failed\n", CMDINFO(CURRENT_SC));
-					if(cmnd)
-						kfree(cmnd);
 				}
 			} else {
 #if 0

Re: [PATCH] [v2] aha152x cmnd->device oops

[Mike Christie]

Hi Randy,

>  			if(!(DONE_SC->SCp.Status & not_issued)) {
> -				Scsi_Cmnd *cmnd = kmalloc(sizeof(Scsi_Cmnd), GFP_ATOMIC);
> +				Scsi_Cmnd *cmnd = scsi_get_command(DONE_SC->device, GFP_ATOMIC);

Do you need to add a scsi_put_command to balance this get when the 
command completes?

> +					cmnd->device          = ptr->device;

Don't even need this now. scsi_get_command will set the device for you.

Mike

Re: [PATCH] [v2] aha152x cmnd->device oops

[Randy.Dunlap]

On Tue, 28 Oct 2003 14:50:28 -0800 Mike Christie <mikenc@us.ibm.com> wrote:

| Hi Randy,
| 
| >  			if(!(DONE_SC->SCp.Status & not_issued)) {
| > -				Scsi_Cmnd *cmnd = kmalloc(sizeof(Scsi_Cmnd), GFP_ATOMIC);
| > +				Scsi_Cmnd *cmnd = scsi_get_command(DONE_SC->device, GFP_ATOMIC);
| 
| Do you need to add a scsi_put_command to balance this get when the 
| command completes?

Yes, if it continues to use scsi_get_command().  However, I'm
working on an alternate patch now.

| > +					cmnd->device          = ptr->device;
| 
| Don't even need this now. scsi_get_command will set the device for you.

Oh, of course.  device was passed to it.

Thanks.

--
~Randy

Re: [PATCH] [v2] aha152x cmnd->device oops

[Randy.Dunlap]

On Tue, 28 Oct 2003 14:50:28 -0800 Mike Christie <mikenc@us.ibm.com> wrote:

| Hi Randy,
| 
| >  			if(!(DONE_SC->SCp.Status & not_issued)) {
| > -				Scsi_Cmnd *cmnd = kmalloc(sizeof(Scsi_Cmnd), GFP_ATOMIC);
| > +				Scsi_Cmnd *cmnd = scsi_get_command(DONE_SC->device, GFP_ATOMIC);
| 
| Do you need to add a scsi_put_command to balance this get when the 
| command completes?
| 
| > +					cmnd->device          = ptr->device;
| 
| Don't even need this now. scsi_get_command will set the device for you.
| 
| Mike

Hers's v3 of the aha152x patch.  However, I'm not satisfied with it,
so I'm not asking James to apply it.  I don't know the state machine
or the hardware well enough to understand and patch it.

Things that I don't like about it:

a.  calling aha152x_internal_queue() with cmnd pointer in 2 places.
    Probably the second one should be NULL.
b.  Easy to confuse the state machine by changing just one variable,
    like DONE_SC (i.e., it's risky and I can't test it, although we
    do have a bug report and can ask that reporter to test it).
c.  int result = SCpnt->SCp.Status; /*FIXME*/
    SCp.Status is not valid here AFAIK, and I don't know how to get
    the current command status at this point.

--
~Randy


description:	aha152x oopses when it references cmnd->device->...
		before cmnd->device has been init, so instead of
		allocating a new scsi_cmnd, just reuse the current
		one for the mode sense;
		then in internal_done(), if the command was a special
		REQUEST_SENSE command, restore the "old" command.
maintainer:	Juergen Fischer <fischer@norbit.de>
product_versions: Linux linux-260-test9-pv
patch_name:	aha152x-cmd-ca.patch
author:		Randy.Dunlap <rddunlap@osdl.org>
patch_version:	2003-10-28.16:17:37

diffstat:=
 drivers/scsi/aha152x.c |   75 +++++++++++++++++++++++++++++--------------------
 1 files changed, 45 insertions(+), 30 deletions(-)

diff -Naurp ./drivers/scsi/aha152x.c~aha152xfix ./drivers/scsi/aha152x.c
--- ./drivers/scsi/aha152x.c~aha152xfix	2003-10-25 11:42:50.000000000 -0700
+++ ./drivers/scsi/aha152x.c	2003-10-28 16:15:24.000000000 -0800
@@ -306,6 +306,8 @@
 
 #define DELAY_DEFAULT 1000
 
+#define AHA15_INTERNAL_SENSE_MAGIC	0x42
+
 #if defined(PCMCIA)
 #define IRQ_MIN 0
 #define IRQ_MAX 16
@@ -1513,6 +1515,21 @@ static void internal_done(Scsi_Cmnd *SCp
 
 	DPRINTK(debug_eh, INFO_LEAD "internal_done called\n", CMDINFO(SCpnt));
 #endif
+	if (SCpnt && SCpnt->cmnd[0] == REQUEST_SENSE && SCpnt->cmnd[6] == AHA15_INTERNAL_SENSE_MAGIC) {
+		int result = SCpnt->SCp.Status; /*FIXME*/
+		/* restore old result if the request sense was successful */
+		if (result == 0)
+			result = SCpnt->cmnd[7];
+		/* now restore the original command */
+		memcpy((void *) SCpnt->cmnd, (void *) SCpnt->data_cmnd,
+		       sizeof(SCpnt->data_cmnd));
+		SCpnt->request_buffer = SCpnt->buffer;
+		SCpnt->request_bufflen = SCpnt->bufflen;
+		SCpnt->use_sg = SCpnt->old_use_sg;
+		SCpnt->cmd_len = SCpnt->old_cmd_len;
+		SCpnt->sc_data_direction = SCpnt->sc_old_data_direction;
+		SCpnt->underflow = SCpnt->old_underflow;
+	}
 	if(SCSEM(SCpnt))
 		up(SCSEM(SCpnt));
 }
@@ -2001,38 +2018,36 @@ static void busfree_run(struct Scsi_Host
 #endif
 
 			if(!(DONE_SC->SCp.Status & not_issued)) {
-				Scsi_Cmnd *cmnd = kmalloc(sizeof(Scsi_Cmnd), GFP_ATOMIC);
-
-				if(cmnd) {
-					Scsi_Cmnd *ptr=DONE_SC;
-					DONE_SC=0;
-
+				Scsi_Cmnd *cmnd = DONE_SC;
+				int status = DONE_SC->SCp.Status;
+				DONE_SC=0;
 #if 0
-					DPRINTK(debug_eh, ERR_LEAD "requesting sense\n", CMDINFO(ptr));
+				DPRINTK(debug_eh, ERR_LEAD "requesting sense\n",
+						CMDINFO(ptr));
 #endif
-
-					cmnd->cmnd[0]         = REQUEST_SENSE;
-					cmnd->cmnd[1]         = 0;
-					cmnd->cmnd[2]         = 0;
-					cmnd->cmnd[3]         = 0;
-					cmnd->cmnd[4]         = sizeof(ptr->sense_buffer);
-					cmnd->cmnd[5]         = 0;
-					cmnd->cmd_len	      = 6;
-					cmnd->device->host    = ptr->device->host;
-					cmnd->device->id      = ptr->device->id;
-					cmnd->device->lun     = ptr->device->lun;
-					cmnd->use_sg          = 0; 
-					cmnd->request_buffer  = ptr->sense_buffer;
-					cmnd->request_bufflen = sizeof(ptr->sense_buffer);
-			
-					DO_UNLOCK(flags);
-					aha152x_internal_queue(cmnd, 0, 0, ptr, internal_done);
-					DO_LOCK(flags);
-				} else {
-					printk(ERR_LEAD "allocation failed\n", CMDINFO(CURRENT_SC));
-					if(cmnd)
-						kfree(cmnd);
-				}
+				cmnd->cmnd[0] = REQUEST_SENSE;
+				cmnd->cmnd[1] = 0;
+				cmnd->cmnd[2] = 0;
+				cmnd->cmnd[3] = 0;
+				cmnd->cmnd[4] = sizeof(cmnd->sense_buffer);
+				cmnd->cmnd[5] = 0;
+				cmnd->cmd_len = 6;
+				/* Here's a quiet hack [from 53c700.c]: the
+				 * REQUEST_SENSE command is six bytes,
+				 * so store a flag indicating that
+				 * this was an internal sense request
+				 * and the original status at the end
+				 * of the command */
+				cmnd->cmnd[6] = AHA15_INTERNAL_SENSE_MAGIC;
+				cmnd->cmnd[7] = status;
+				cmnd->use_sg  = 0; 
+				cmnd->sc_data_direction = SCSI_DATA_READ;
+				cmnd->request_buffer  = cmnd->sense_buffer;
+				cmnd->request_bufflen = sizeof(cmnd->sense_buffer);
+
+				DO_UNLOCK(flags);
+				aha152x_internal_queue(cmnd, 0, 0, cmnd, internal_done);
+				DO_LOCK(flags);
 			} else {
 #if 0
 				DPRINTK(debug_eh, ERR_LEAD "command not issued - CHECK CONDITION ignored\n", CMDINFO(DONE_SC));

Re: [PATCH] [v2] aha152x cmnd->device oops

[Juergen E. Fischer]

Hi Randy,

On Tue, Oct 28, 2003 at 16:26:10 -0800, Randy.Dunlap wrote:
> On Tue, 28 Oct 2003 14:50:28 -0800 Mike Christie <mikenc@us.ibm.com> wrote:
> 
> | Hi Randy,
> | 
> | >  			if(!(DONE_SC->SCp.Status & not_issued)) {
> | > -				Scsi_Cmnd *cmnd = kmalloc(sizeof(Scsi_Cmnd), GFP_ATOMIC);
> | > +				Scsi_Cmnd *cmnd = scsi_get_command(DONE_SC->device, GFP_ATOMIC);
> | 
> | Do you need to add a scsi_put_command to balance this get when the 
> | command completes?
> | 
> | > +					cmnd->device          = ptr->device;
> | 
> | Don't even need this now. scsi_get_command will set the device for you.
> | 
> | Mike
> 
> Hers's v3 of the aha152x patch.  However, I'm not satisfied with it,
> so I'm not asking James to apply it.  I don't know the state machine
> or the hardware well enough to understand and patch it.

> Things that I don't like about it:
> 
> a.  calling aha152x_internal_queue() with cmnd pointer in 2 places.
>     Probably the second one should be NULL.
> b.  Easy to confuse the state machine by changing just one variable,
>     like DONE_SC (i.e., it's risky and I can't test it, although we
>     do have a bug report and can ask that reporter to test it).
> c.  int result = SCpnt->SCp.Status; /*FIXME*/
>     SCp.Status is not valid here AFAIK, and I don't know how to get
>     the current command status at this point.

There are 2 specially "commands" which are used internally in the driver
only (so I don't see a problem in using kmalloc instead of
scsi_get_command; but nevertheless ->device needs to be intialized
correctly, which is what scsi_get_command() does and the current code
fails to do). 

The first is when a command returns with status CHECK CONDITION and the
driver needs to do a REQUEST SENSE to fetch sense data and add that to
the Ssci_Cmnd which resulted in the CHECK CONDITION.  The internally
queued command fills the sense_buffer of the offending command and then
calls its ->scsi_done().

The second it when the controller needs to be resetted.  Both are purely
internal to the driver.

I'll test this in the evening.


Jürgen

-- 
Phase 1: Where do you want to go today?
Phase 2: This is where you want to go today.
Phase 3: You're not going anywhere today.
  -- seen on /.
-
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] [v2] aha152x cmnd->device oops

[Christoph Hellwig]

On Tue, Oct 28, 2003 at 12:45:36PM -0800, Randy.Dunlap wrote:
>  			if(!(DONE_SC->SCp.Status & not_issued)) {

Strange variable name..


> +					cmnd->device          = ptr->device;
>  					cmnd->device->host    = ptr->device->host;
>  					cmnd->device->id      = ptr->device->id;
>  					cmnd->device->lun     = ptr->device->lun;

You shouldn't touch either of these.

> -					if(cmnd)
> -						kfree(cmnd);

You need a scsi_put_command here.

Re: [PATCH] [v2] aha152x cmnd->device oops

[James Bottomley]

On Wed, 2003-10-29 at 06:20, Juergen E. Fischer wrote:
> The first is when a command returns with status CHECK CONDITION and the
> driver needs to do a REQUEST SENSE to fetch sense data and add that to
> the Ssci_Cmnd which resulted in the CHECK CONDITION.  The internally
> queued command fills the sense_buffer of the offending command and then
> calls its ->scsi_done().

You don't actually need a new command for this.  The struct scsi_cmnd
layout is designed to allow you to reuse the current command for
clearing contingent allegiance conditions (the fields you need to alter
all have copies stored somewhere which you can restore when you have the
sense data.  See for example how the 53c700 does this, or aic7xxx_old)

> The second it when the controller needs to be resetted.  Both are purely
> internal to the driver.

As a rule of thumb: as long as the command is never seen outside the
driver, then using kmalloc is fine.  If it is ever seen outside the
driver (like in requeueing conditions) then you should use
scsi_get_command().

James


James

Re: [PATCH] [v2] aha152x cmnd->device oops

[Juergen E. Fischer]

[-- Attachment #1: Type: text/plain, Size: 2482 bytes --]

Hi James,

On Wed, Oct 29, 2003 at 08:58:28 -0600, James Bottomley wrote:
> You don't actually need a new command for this.  The struct scsi_cmnd
> layout is designed to allow you to reuse the current command for
> clearing contingent allegiance conditions (the fields you need to alter
> all have copies stored somewhere which you can restore when you have the
> sense data.  See for example how the 53c700 does this, or aic7xxx_old)

Why not?  It's a new command after all and if the initialization is
done correctly (ie. ->device is setup) it works the way it is now.

> > The second it when the controller needs to be resetted.  Both are purely
> > internal to the driver.
 
> As a rule of thumb: as long as the command is never seen outside the
> driver, then using kmalloc is fine.  If it is ever seen outside the
> driver (like in requeueing conditions) then you should use
> scsi_get_command().

using it now anyway.  Following patch fixes the problem.  Works here.


--- aha152x.c.orig	2003-10-29 18:53:59.000000000 +0100
+++ aha152x.c	2003-10-29 18:38:20.000000000 +0100
@@ -450,7 +450,8 @@
 		/* host lock */
 
 #if defined(AHA152X_DEBUG)
-	char *locker;	/* which function has the lock */
+	const char *locker;
+		/* which function has the lock */
 	int lockerl;	/* where did it get it */
 
 	int debug;	/* current debugging setting */
@@ -1991,7 +1992,7 @@
 				SETPORT(PORTA, 0);	/* turn led off */
 
 			kfree(ptr->host_scribble);
-			kfree(ptr);
+			scsi_put_command(ptr);
 		} else if(DONE_SC->SCp.Status==0x02) {
 #if defined(AHA152X_STAT)
 			HOSTDATA(shpnt)->busfree_with_check_condition++;
@@ -2001,7 +2002,7 @@
 #endif
 
 			if(!(DONE_SC->SCp.Status & not_issued)) {
-				Scsi_Cmnd *cmnd = kmalloc(sizeof(Scsi_Cmnd), GFP_ATOMIC);
+				Scsi_Cmnd *cmnd = scsi_get_command(DONE_SC->device, GFP_ATOMIC);
 
 				if(cmnd) {
 					Scsi_Cmnd *ptr=DONE_SC;
@@ -2018,9 +2019,6 @@
 					cmnd->cmnd[4]         = sizeof(ptr->sense_buffer);
 					cmnd->cmnd[5]         = 0;
 					cmnd->cmd_len	      = 6;
-					cmnd->device->host    = ptr->device->host;
-					cmnd->device->id      = ptr->device->id;
-					cmnd->device->lun     = ptr->device->lun;
 					cmnd->use_sg          = 0; 
 					cmnd->request_buffer  = ptr->sense_buffer;
 					cmnd->request_bufflen = sizeof(ptr->sense_buffer);


Jürgen

-- 
"If Bill Gates had a nickel for every time Windows crashed....
Oh, wait, he does!" 
  -- Gary Oliver

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH] [v2] aha152x cmnd->device oops

[James Bottomley]

On Wed, 2003-10-29 at 11:56, Juergen E. Fischer wrote:
> Why not?  It's a new command after all and if the initialization is
> done correctly (ie. ->device is setup) it works the way it is now.

The usual reason is that ACA emulation is turned around in interrupt
context, so new memory allocations should be avoided if they can be.

for the aha that could eliminate the check for NULL cmnd.

James

Re: [PATCH] [v2] aha152x cmnd->device oops

[Juergen E. Fischer]

[-- Attachment #1.1: Type: text/plain, Size: 745 bytes --]

Hi James,

On Wed, Oct 29, 2003 at 12:10:17 -0600, James Bottomley wrote:
> On Wed, 2003-10-29 at 11:56, Juergen E. Fischer wrote:
> > Why not?  It's a new command after all and if the initialization is
> > done correctly (ie. ->device is setup) it works the way it is now.
> 
> The usual reason is that ACA emulation is turned around in interrupt
> context, so new memory allocations should be avoided if they can be.

ok, attached patch does it that way and also fixes two other problems I
noticed: 

1. unloading the module with two controllers present didn't work,
2. there was a race in is_complete.


Jürgen


-- 
Well, let's just say, 'if your VCR is still blinking 12:00, you don't
want Linux'.
  -- Bruce Perens

[-- Attachment #1.2: aha152x.diff --]
[-- Type: text/plain, Size: 16863 bytes --]

--- aha152x.c.orig	2003-10-29 18:53:59.000000000 +0100
+++ aha152x.c	2003-10-30 21:58:10.000000000 +0100
@@ -13,9 +13,16 @@
  * General Public License for more details.
  *
  *
- * $Id: aha152x.c,v 2.5 2002/04/14 11:24:53 fischer Exp $
+ * $Id: aha152x.c,v 2.6 2003/10/30 20:52:47 fischer Exp $
  *
  * $Log: aha152x.c,v $
+ * Revision 2.6  2003/10/30 20:52:47  fischer
+ * - interfaces changes for kernel 2.6
+ * - aha152x_probe_one introduced for pcmcia stub
+ * - fixed pnpdev handling
+ * - instead of allocation a new one, reuse command for request sense after check condition and reset
+ * - fixes race in is_complete
+ *
  * Revision 2.5  2002/04/14 11:24:53  fischer
  * - isapnp support
  * - abort fixed
@@ -330,6 +337,7 @@
 	syncneg		= 0x0100,	/* synchronous negotiation in progress */
 	aborting	= 0x0200,	/* ABORT is pending */
 	resetting	= 0x0400,	/* BUS DEVICE RESET is pending */
+	check_condition = 0x0800,	/* requesting sense after CHECK CONDITION */
 };
 
 MODULE_AUTHOR("Jürgen Fischer");
@@ -450,7 +458,8 @@
 		/* host lock */
 
 #if defined(AHA152X_DEBUG)
-	char *locker;	/* which function has the lock */
+	const char *locker;
+		/* which function has the lock */
 	int lockerl;	/* where did it get it */
 
 	int debug;	/* current debugging setting */
@@ -516,6 +525,10 @@
 
 	unsigned long io_port0;
 	unsigned long io_port1;
+
+#ifdef __ISAPNP__
+	struct pnp_dev *pnpdev;
+#endif
 };
 
 
@@ -525,7 +538,6 @@
  */
 struct aha152x_scdata {
 	Scsi_Cmnd *next;	/* next sc in queue */
-	Scsi_Cmnd *done;	/* done command */
 	struct semaphore *sem;	/* semaphore to block on */
 };
 
@@ -578,7 +590,6 @@
 
 #define SCDATA(SCpnt)		((struct aha152x_scdata *) (SCpnt)->host_scribble)
 #define SCNEXT(SCpnt)		SCDATA(SCpnt)->next
-#define SCDONE(SCpnt)		SCDATA(SCpnt)->done
 #define SCSEM(SCpnt)		SCDATA(SCpnt)->sem
 
 #define SG_ADDRESS(buffer)	((char *) (page_address((buffer)->page)+(buffer)->offset))
@@ -949,55 +960,47 @@
 	return IRQ_HANDLED;
 }
 
-#ifdef __ISAPNP__
-static struct pnp_dev *pnpdev[2];
-static int num_pnpdevs;
-#endif
 
 struct Scsi_Host *aha152x_probe_one(struct aha152x_setup *setup)
 {
-	struct Scsi_Host *shost, *shpnt;
-	struct aha152x_hostdata *aha;
+	struct Scsi_Host *shpnt;
 
-	/* XXX: shpnt is needed for some broken macros */
-	shost = shpnt = scsi_register(&aha152x_driver_template,
-					sizeof(struct aha152x_hostdata));
-	if (!shost) {
+	shpnt = scsi_register(&aha152x_driver_template, sizeof(struct aha152x_hostdata));
+	if (!shpnt) {
 		printk(KERN_ERR "aha152x: scsi_register failed\n");
 		return NULL;
 	}
 
-	aha = (struct aha152x_hostdata *)&shost->hostdata;
-	memset(aha, 0, sizeof(*aha));
+	memset(HOSTDATA(shpnt), 0, sizeof *HOSTDATA(shpnt));
 
-	shost->io_port   = setup->io_port;
-	shost->n_io_port = IO_RANGE;
-	shost->irq       = setup->irq;
+	shpnt->io_port   = setup->io_port;
+	shpnt->n_io_port = IO_RANGE;
+	shpnt->irq       = setup->irq;
 
 	if (!setup->tc1550) {
-		aha->io_port0 = setup->io_port;
-		aha->io_port1 = setup->io_port;
+		HOSTIOPORT0 = setup->io_port;
+		HOSTIOPORT1 = setup->io_port;
 	} else {
-		aha->io_port0 = setup->io_port+0x10;
-		aha->io_port1 = setup->io_port-0x10;
+		HOSTIOPORT0 = setup->io_port+0x10;
+		HOSTIOPORT1 = setup->io_port-0x10;
 	}
 
-	spin_lock_init(&aha->lock);
-	aha->reconnect = setup->reconnect;
-	aha->synchronous = setup->synchronous;
-	aha->parity = setup->parity;
-	aha->delay = setup->delay;
-	aha->ext_trans = setup->ext_trans;
+	spin_lock_init(&QLOCK);
+	RECONNECT   = setup->reconnect;
+	SYNCHRONOUS = setup->synchronous;
+	PARITY      = setup->parity;
+	DELAY       = setup->delay;
+	EXT_TRANS   = setup->ext_trans;
 
 #if defined(AHA152X_DEBUG)
-	aha->debug = setup->debug;
+	HOSTDATA(shpnt)->debug = setup->debug;
 #endif
 
 	SETPORT(SCSIID, setup->scsiid << 4);
-	shost->this_id = setup->scsiid;
+	shpnt->this_id = setup->scsiid;
 
 	if (setup->reconnect)
-		shost->can_queue = AHA152X_MAXQUEUE;
+		shpnt->can_queue = AHA152X_MAXQUEUE;
 
 	/* RESET OUT */
 	printk("aha152x: resetting bus...\n");
@@ -1006,7 +1009,7 @@
 	SETPORT(SCSISEQ, 0);
 	mdelay(DELAY);
 
-	reset_ports(shost);
+	reset_ports(shpnt);
 
 	printk(KERN_INFO
 	       "aha152x%d%s: "
@@ -1019,43 +1022,41 @@
 	       "synchronous=%s, "
 	       "delay=%d, "
 	       "extended translation=%s\n",
-	       shost->host_no, setup->tc1550 ? " (tc1550 mode)" : "",
+	       shpnt->host_no, setup->tc1550 ? " (tc1550 mode)" : "",
 	       GETPORT(REV) & 0x7,
-	       shost->io_port, aha->io_port0, aha->io_port1,
-	       shost->irq,
-	       shost->this_id,
-	       aha->reconnect ? "enabled" : "disabled",
-	       aha->parity ? "enabled" : "disabled",
-	       aha->synchronous ? "enabled" : "disabled",
-	       aha->delay,
-	       aha->ext_trans ? "enabled" : "disabled");
+	       shpnt->io_port, HOSTIOPORT0, HOSTIOPORT1,
+	       shpnt->irq,
+	       shpnt->this_id,
+	       RECONNECT ? "enabled" : "disabled",
+	       PARITY ? "enabled" : "disabled",
+	       SYNCHRONOUS ? "enabled" : "disabled",
+	       DELAY,
+	       EXT_TRANS ? "enabled" : "disabled");
 
-	if (!request_region(shost->io_port, IO_RANGE, "aha152x"))
+	if (!request_region(shpnt->io_port, IO_RANGE, "aha152x"))
 		goto out_unregister;
 
 	/* not expecting any interrupts */
 	SETPORT(SIMODE0, 0);
 	SETPORT(SIMODE1, 0);
 
-	if (request_irq(shost->irq, swintr, SA_INTERRUPT|SA_SHIRQ,
-			"aha152x", shost) < 0) {
-		printk(KERN_ERR "aha152x%d: driver needs an IRQ.\n", shost->host_no);
+	if (request_irq(shpnt->irq, swintr, SA_INTERRUPT|SA_SHIRQ, "aha152x", shpnt) < 0) {
+		printk(KERN_ERR "aha152x%d: driver needs an IRQ.\n", shpnt->host_no);
 		goto out_release_region;
 	}
 
-	aha->swint = 0;
+	HOSTDATA(shpnt)->swint = 0;
 
-	printk(KERN_INFO "aha152x%d: trying software interrupt, ",
-			 shost->host_no);
+	printk(KERN_INFO "aha152x%d: trying software interrupt, ", shpnt->host_no);
 
 	/* need to have host registered before triggering any interrupt */
-	aha152x_host[registered_count] = shost;
+	aha152x_host[registered_count] = shpnt;
 	mb();
 	SETPORT(DMACNTRL0, SWINT|INTEN);
 	mdelay(1000);
-	free_irq(shost->irq, shost);
+	free_irq(shpnt->irq, shpnt);
 
-	if (!aha->swint) {
+	if (!HOSTDATA(shpnt)->swint) {
 		if (TESTHI(DMASTAT, INTSTAT)) {
 			printk("lost.\n");
 		} else {
@@ -1065,7 +1066,7 @@
 		SETPORT(DMACNTRL0, INTEN);
 
 		printk(KERN_ERR "aha152x%d: IRQ %d possibly wrong.  "
-				"Please verify.\n", shost->host_no, shost->irq);
+				"Please verify.\n", shpnt->host_no, shpnt->irq);
 		goto out_unregister_host;
 	}
 	printk("ok.\n");
@@ -1075,20 +1076,18 @@
 	SETPORT(SSTAT0, 0x7f);
 	SETPORT(SSTAT1, 0xef);
 
-	if (request_irq(shost->irq, intr, SA_INTERRUPT|SA_SHIRQ,
-				"aha152x", shost) < 0) {
-		printk(KERN_ERR "aha152x%d: failed to reassign interrupt.\n",
-				shost->host_no);
+	if (request_irq(shpnt->irq, intr, SA_INTERRUPT|SA_SHIRQ, "aha152x", shpnt) < 0) {
+		printk(KERN_ERR "aha152x%d: failed to reassign interrupt.\n", shpnt->host_no);
 		goto out_unregister_host;
 	}
-	return shost;	/* the pcmcia stub needs the return value; */
+	return shpnt;	/* the pcmcia stub needs the return value; */
 
 out_unregister_host:
 	aha152x_host[registered_count] = NULL;
 out_release_region:
-	release_region(shost->io_port, IO_RANGE);
+	release_region(shpnt->io_port, IO_RANGE);
 out_unregister:
-	scsi_unregister(shost);
+	scsi_unregister(shpnt);
 	return NULL;
 }
 
@@ -1097,9 +1096,9 @@
 	int i, j, ok;
 #if defined(AUTOCONF)
 	aha152x_config conf;
-#ifdef __ISAPNP__
-	struct pnp_dev *dev = NULL;
 #endif
+#ifdef __ISAPNP__
+	struct pnp_dev *dev=0, *pnpdev[2] = {0, 0};
 #endif
 
 	if (setup_count) {
@@ -1269,7 +1268,7 @@
 #if defined(AHA152X_DEBUG)
 		setup[setup_count].debug       = DEBUG_DEFAULT;
 #endif
-		pnpdev[num_pnpdevs++] = dev;
+		pnpdev[setup_count]            = dev;
 		printk (KERN_INFO
 			"aha152x: found ISAPnP AVA-1505A at io=0x%03x, irq=%d\n",
 			setup[setup_count].io_port, setup[setup_count].irq);
@@ -1277,7 +1276,6 @@
 	}
 #endif
 
-
 #if defined(AUTOCONF)
 	if (setup_count<ARRAY_SIZE(setup)) {
 #if !defined(SKIP_BIOSTEST)
@@ -1350,8 +1348,13 @@
 
 	for (i=0; i<setup_count; i++) {
 		aha152x_probe_one(&setup[i]);
-		if (aha152x_host[registered_count])
+		if (aha152x_host[registered_count]) {
+#ifdef __ISAPNP__
+			if(pnpdev[i])
+				HOSTDATA(aha152x_host[registered_count])->pnpdev=pnpdev[i];
+#endif
 			registered_count++;
+		}
 	}
 
 	return registered_count>0;
@@ -1367,9 +1370,10 @@
 		release_region(shpnt->io_port, IO_RANGE);
 
 #ifdef __ISAPNP__
-	while (num_pnpdevs--)
-		pnp_device_detach(pnpdev[num_pnpdevs]);
+	if (HOSTDATA(shpnt)->pnpdev)
+		pnp_device_detach(HOSTDATA(shpnt)->pnpdev);
 #endif
+
 	scsi_unregister(shpnt);
 
 	return 0;
@@ -1418,7 +1422,7 @@
 /* 
  *  Queue a command and setup interrupts for a free bus.
  */
-static int aha152x_internal_queue(Scsi_Cmnd *SCpnt, struct semaphore *sem, int phase, Scsi_Cmnd *done_SC, void (*done)(Scsi_Cmnd *))
+static int aha152x_internal_queue(Scsi_Cmnd *SCpnt, struct semaphore *sem, int phase, void (*done)(Scsi_Cmnd *))
 {
 	struct Scsi_Host *shpnt = SCpnt->device->host;
 	unsigned long flags;
@@ -1438,14 +1442,21 @@
 	SCpnt->SCp.Message	= 0;
 	SCpnt->SCp.have_data_in	= 0;
 	SCpnt->SCp.sent_command	= 0;
-	SCpnt->host_scribble    = kmalloc(sizeof(struct aha152x_scdata), GFP_ATOMIC);
-	if(!SCpnt->host_scribble) {
-		printk(ERR_LEAD "allocation failed\n", CMDINFO(SCpnt));
-		return FAILED;
+
+	if(SCpnt->SCp.phase & (resetting|check_condition)) {
+		if(SCpnt->host_scribble==0 || SCSEM(SCpnt) || SCNEXT(SCpnt)) {
+			printk(ERR_LEAD "cannot reuse command\n", CMDINFO(SCpnt));
+			return FAILED;
+		}
+	} else {
+		SCpnt->host_scribble    = kmalloc(sizeof(struct aha152x_scdata), GFP_ATOMIC);
+		if(SCpnt->host_scribble==0) {
+			printk(ERR_LEAD "allocation failed\n", CMDINFO(SCpnt));
+			return FAILED;
+		}
 	}
 
 	SCNEXT(SCpnt)		= 0;
-	SCDONE(SCpnt)		= done_SC;
 	SCSEM(SCpnt)		= sem;
 
 	/* setup scratch area
@@ -1487,6 +1498,10 @@
 	return 0;
 }
 
+/*
+ *  queue a command
+ *
+ */
 static int aha152x_queue(Scsi_Cmnd *SCpnt, void (*done)(Scsi_Cmnd *))
 {
 #if 0
@@ -1498,23 +1513,25 @@
 	}
 #endif
 
-	return aha152x_internal_queue(SCpnt, 0, 0, 0, done);
+	return aha152x_internal_queue(SCpnt, 0, 0, done);
 }
 
 
 /*
- *  run a command
+ *  
  *
  */
-static void internal_done(Scsi_Cmnd *SCpnt)
+static void reset_done(Scsi_Cmnd *SCpnt)
 {
 #if 0
 	struct Scsi_Host *shpnt = SCpnt->host;
-
-	DPRINTK(debug_eh, INFO_LEAD "internal_done called\n", CMDINFO(SCpnt));
+	DPRINTK(debug_eh, INFO_LEAD "reset_done called\n", CMDINFO(SCpnt));
 #endif
-	if(SCSEM(SCpnt))
+	if(SCSEM(SCpnt)) {
 		up(SCSEM(SCpnt));
+	} else {
+		printk(KERN_ERR "aha152x: reset_done w/o semaphore\n");
+	}
 }
 
 /*
@@ -1600,7 +1617,6 @@
 	struct Scsi_Host *shpnt = SCpnt->device->host;
 	DECLARE_MUTEX_LOCKED(sem);
 	struct timer_list timer;
-	Scsi_Cmnd *cmd;
 	int ret;
 
 #if defined(AHA152X_DEBUG)
@@ -1615,40 +1631,32 @@
 		return FAILED;
 	}
 
-	spin_unlock_irq(shpnt->host_lock);
-	cmd = scsi_get_command(SCpnt->device, GFP_ATOMIC);
-	if (!cmd) {
-		spin_lock_irq(shpnt->host_lock);
-		return FAILED;
-	}
-
-	cmd->cmd_len         = 0;
-	cmd->device->host    = SCpnt->device->host;
-	cmd->device->id      = SCpnt->device->id;
-	cmd->device->lun     = SCpnt->device->lun;
-	cmd->use_sg          = 0;
-	cmd->request_buffer  = 0;
-	cmd->request_bufflen = 0;
+	SCpnt->cmd_len         = 0;
+	SCpnt->use_sg          = 0;
+	SCpnt->request_buffer  = 0;
+	SCpnt->request_bufflen = 0;
 
 	init_timer(&timer);
 	timer.data     = (unsigned long) cmd;
 	timer.expires  = jiffies + 100*HZ;   /* 10s */
 	timer.function = (void (*)(unsigned long)) timer_expired;
 
-	aha152x_internal_queue(cmd, &sem, resetting, 0, internal_done);
-
+	aha152x_internal_queue(SCpnt, &sem, resetting, reset_done);
 	add_timer(&timer);
 	down(&sem);
-
 	del_timer(&timer);
 
-	if(cmd->SCp.phase & resetted) {
+	SCpnt->cmd_len         = SCpnt->old_cmd_len;
+	SCpnt->use_sg          = SCpnt->old_use_sg;
+  	SCpnt->request_buffer  = SCpnt->buffer;
+       	SCpnt->request_bufflen = SCpnt->bufflen;
+
+	if(SCpnt->SCp.phase & resetted) {
 		ret = SUCCESS;
 	} else {
 		ret = FAILED;
 	}
 
-	scsi_put_command(cmd);
 	spin_lock_irq(shpnt->host_lock);
 	return ret;
 }
@@ -1975,23 +1983,26 @@
 #if defined(AHA152X_STAT)
 		action++;
 #endif
-		if(SCDONE(DONE_SC)) {
-			Scsi_Cmnd *ptr=DONE_SC;
-			DONE_SC=SCDONE(DONE_SC);
-
+		if(DONE_SC->SCp.phase & check_condition) {
 #if 0
 			if(HOSTDATA(shpnt)->debug & debug_eh) {
-				printk(ERR_LEAD "received sense: ", CMDINFO(ptr));
+				printk(ERR_LEAD "received sense: ", CMDINFO(DONE_SC));
 				print_sense("bh", DONE_SC);
 			}
 #endif
 
+			/* restore old command */
+			memcpy((void *) DONE_SC->cmnd, (void *) DONE_SC->data_cmnd, sizeof(DONE_SC->data_cmnd));
+			DONE_SC->request_buffer  = DONE_SC->buffer;
+			DONE_SC->request_bufflen = DONE_SC->bufflen;
+			DONE_SC->use_sg          = DONE_SC->old_use_sg;
+			DONE_SC->cmd_len         = DONE_SC->old_cmd_len;
+
+			DONE_SC->SCp.Status = 0x02;
+
 			HOSTDATA(shpnt)->commands--;
 			if (!HOSTDATA(shpnt)->commands)
 				SETPORT(PORTA, 0);	/* turn led off */
-
-			kfree(ptr->host_scribble);
-			kfree(ptr);
 		} else if(DONE_SC->SCp.Status==0x02) {
 #if defined(AHA152X_STAT)
 			HOSTDATA(shpnt)->busfree_with_check_condition++;
@@ -2001,42 +2012,30 @@
 #endif
 
 			if(!(DONE_SC->SCp.Status & not_issued)) {
-				Scsi_Cmnd *cmnd = kmalloc(sizeof(Scsi_Cmnd), GFP_ATOMIC);
-
-				if(cmnd) {
-					Scsi_Cmnd *ptr=DONE_SC;
-					DONE_SC=0;
-
 #if 0
-					DPRINTK(debug_eh, ERR_LEAD "requesting sense\n", CMDINFO(ptr));
+				DPRINTK(debug_eh, ERR_LEAD "requesting sense\n", CMDINFO(DONE_SC));
 #endif
 
-					cmnd->cmnd[0]         = REQUEST_SENSE;
-					cmnd->cmnd[1]         = 0;
-					cmnd->cmnd[2]         = 0;
-					cmnd->cmnd[3]         = 0;
-					cmnd->cmnd[4]         = sizeof(ptr->sense_buffer);
-					cmnd->cmnd[5]         = 0;
-					cmnd->cmd_len	      = 6;
-					cmnd->device->host    = ptr->device->host;
-					cmnd->device->id      = ptr->device->id;
-					cmnd->device->lun     = ptr->device->lun;
-					cmnd->use_sg          = 0; 
-					cmnd->request_buffer  = ptr->sense_buffer;
-					cmnd->request_bufflen = sizeof(ptr->sense_buffer);
+				DONE_SC->cmnd[0]         = REQUEST_SENSE;
+				DONE_SC->cmnd[1]         = 0;
+				DONE_SC->cmnd[2]         = 0;
+				DONE_SC->cmnd[3]         = 0;
+				DONE_SC->cmnd[4]         = sizeof(DONE_SC->sense_buffer);
+				DONE_SC->cmnd[5]         = 0;
+				DONE_SC->cmd_len         = 6;
+				DONE_SC->use_sg          = 0; 
+				DONE_SC->request_buffer  = DONE_SC->sense_buffer;
+				DONE_SC->request_bufflen = sizeof(DONE_SC->sense_buffer);
 			
-					DO_UNLOCK(flags);
-					aha152x_internal_queue(cmnd, 0, 0, ptr, internal_done);
-					DO_LOCK(flags);
-				} else {
-					printk(ERR_LEAD "allocation failed\n", CMDINFO(CURRENT_SC));
-					if(cmnd)
-						kfree(cmnd);
-				}
+				DO_UNLOCK(flags);
+				aha152x_internal_queue(DONE_SC, 0, check_condition, DONE_SC->scsi_done);
+				DO_LOCK(flags);
+
+				DONE_SC=0;
 			} else {
 #if 0
 				DPRINTK(debug_eh, ERR_LEAD "command not issued - CHECK CONDITION ignored\n", CMDINFO(DONE_SC));
-#endif				
+#endif
 			}
 		}
 
@@ -2946,13 +2945,12 @@
 	int pending;
 
 	DO_LOCK(flags);
-	if(HOSTDATA(shpnt)->in_intr!=0) {
+	if(HOSTDATA(shpnt)->in_intr) {
 		DO_UNLOCK(flags);
 		/* aha152x_error never returns.. */
 		aha152x_error(shpnt, "bottom-half already running!?");
 	}
 	HOSTDATA(shpnt)->in_intr++;
-	DO_UNLOCK(flags);
 
 	/*
 	 * loop while there are interrupt conditions pending
@@ -2960,6 +2958,8 @@
 	 */
 	do {
 		unsigned long start = jiffies;
+		DO_UNLOCK(flags);
+
 		dataphase=update_state(shpnt);
 
 		DPRINTK(debug_phases, LEAD "start %s %s(%s)\n", CMDINFO(CURRENT_SC), states[STATE].name, states[PREVSTATE].name, states[LASTSTATE].name);
@@ -3035,7 +3035,6 @@
 			HOSTDATA(shpnt)->count_trans[STATE]++;
 		HOSTDATA(shpnt)->time[STATE] += jiffies-start;
 #endif
-		DO_UNLOCK(flags);
 
 		DPRINTK(debug_phases, LEAD "end %s %s(%s)\n", CMDINFO(CURRENT_SC), states[STATE].name, states[PREVSTATE].name, states[LASTSTATE].name);
 	} while(pending);
@@ -3044,7 +3043,6 @@
 	 * enable interrupts and leave bottom-half
 	 *
 	 */
-	DO_LOCK(flags);
 	HOSTDATA(shpnt)->in_intr--;
 	SETBITS(DMACNTRL0, INTEN);
 	DO_UNLOCK(flags);

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH] scsi_device refcounting and list lockdown

[James Bottomley]

On Mon, 2003-10-27 at 09:57, Christoph Hellwig wrote:
> --- 1.44/drivers/scsi/53c700.c	Sat Sep 20 11:35:53 2003
> +++ edited/drivers/scsi/53c700.c	Mon Oct 27 12:01:48 2003
> @@ -1057,15 +1057,18 @@
>  		__u8 lun;
>  		struct NCR_700_command_slot *slot;
>  		__u8 reselection_id = hostdata->reselection_id;
> -		Scsi_Device *SDp;
> +		struct scsi_device *SDp;
> +		unsigned long flags;
>  
>  		lun = hostdata->msgin[0] & 0x1f;
>  
>  		hostdata->reselection_id = 0xff;
>  		DEBUG(("scsi%d: (%d:%d) RESELECTED!\n",
>  		       host->host_no, reselection_id, lun));
> +
>  		/* clear the reselection indicator */
> -		SDp = scsi_find_device(host, 0, reselection_id, lun);
> +		spin_lock_irqsave(host->host_lock, flags);
> +		SDp = __scsi_device_lookup(host, 0, reselection_id, lun);
>  		if(unlikely(SDp == NULL)) {
>  			printk(KERN_ERR "scsi%d: (%d:%d) HAS NO device\n",
>  			       host->host_no, reselection_id, lun);

This is wrong.  The messages are processed in the middle of a SCSI
transaction, so the host lock is already held.

[...]

> ===== drivers/scsi/scsi.h 1.91 vs edited =====
> --- 1.91/drivers/scsi/scsi.h	Sat Sep 20 13:50:50 2003
> +++ edited/drivers/scsi/scsi.h	Mon Oct 27 12:01:41 2003
> @@ -116,12 +116,6 @@
>  #define scsi_to_pci_dma_dir(scsi_dir)	((int)(scsi_dir))
>  #define scsi_to_sbus_dma_dir(scsi_dir)	((int)(scsi_dir))
>  
> -/*
> - * This is the crap from the old error handling code.  We have it in a special
> - * place so that we can more easily delete it later on.
> - */
> -#include "scsi_obsolete.h"
> -
>  /* obsolete typedef junk. */
>  #include "scsi_typedefs.h"


This isn't a bug fix, and removing the obsolete include breaks all of
the drivers relying on its defines (cpqfc, qlogicisp, qlogicfc,
ncr53c8xx etc.)

> ===== drivers/scsi/scsi_error.c 1.65 vs edited =====
> --- 1.65/drivers/scsi/scsi_error.c	Sun Sep 21 19:49:36 2003
> +++ edited/drivers/scsi/scsi_error.c	Mon Oct 27 12:01:49 2003
> @@ -22,11 +22,10 @@
>  #include <linux/kernel.h>
>  #include <linux/interrupt.h>
>  #include <linux/blkdev.h>
> -#include <linux/smp_lock.h>

This is a required include for lock/unlock_kernel() calls used in the
error handler thread.

> ===== include/scsi/scsi_host.h 1.12 vs edited =====
> --- 1.12/include/scsi/scsi_host.h	Thu Oct 16 10:56:58 2003
> +++ edited/include/scsi/scsi_host.h	Mon Oct 27 12:01:50 2003
> @@ -125,15 +125,6 @@
>  	int (* eh_host_reset_handler)(struct scsi_cmnd *);
>  
>  	/*
> -	 * Old EH handlers, no longer used. Make them warn the user of old
> -	 * drivers by using a wrong type
> -	 *
> -	 * Status: MORE THAN OBSOLETE
> -	 */
> -	int (* abort)(int);
> -	int (* reset)(int, int);
> -
> -	/*

This isn't a bugfix either...it will just break unconverted drivers.

James

Re: [PATCH] scsi_device refcounting and list lockdown

[Christoph Hellwig]

On Thu, Oct 30, 2003 at 04:41:01PM -0600, James Bottomley wrote:
> This is wrong.  The messages are processed in the middle of a SCSI
> transaction, so the host lock is already held.

Ah okay.  Looks like I didn't follow up the callchain enough.

> > ===== drivers/scsi/scsi.h 1.91 vs edited =====
> > --- 1.91/drivers/scsi/scsi.h	Sat Sep 20 13:50:50 2003
> > +++ edited/drivers/scsi/scsi.h	Mon Oct 27 12:01:41 2003
> > @@ -116,12 +116,6 @@
> >  #define scsi_to_pci_dma_dir(scsi_dir)	((int)(scsi_dir))
> >  #define scsi_to_sbus_dma_dir(scsi_dir)	((int)(scsi_dir))
> >  
> > -/*
> > - * This is the crap from the old error handling code.  We have it in a special
> > - * place so that we can more easily delete it later on.
> > - */
> > -#include "scsi_obsolete.h"
> > -
> >  /* obsolete typedef junk. */
> >  #include "scsi_typedefs.h"
> 
> 
> This isn't a bug fix, and removing the obsolete include breaks all of
> the drivers relying on its defines (cpqfc, qlogicisp, qlogicfc,
> ncr53c8xx etc.)

Yes, this slipped through from the other patch and shouldn't be
included.  I disagree though that this shouldn't go in, but it certainly
does not belong into this patch.

> This isn't a bugfix either...it will just break unconverted drivers.

Yes, same as above.

Re: [PATCH] scsi_device refcounting and list lockdown

[Christoph Hellwig]

On Mon, Oct 27, 2003 at 04:57:13PM +0100, Christoph Hellwig wrote:
>  - shost->my_devices is gone and replaced by ->__devices, which is not
>    exposed to drivers and locked by the host lock.  Use the exported
>    helpers that do proper refcounting to access it.
>  - sdev->access_count is gone as a side-effect, the sg interfaces
>    to export it now return 1 for a present scsi_device.
>  - drivers/scsi/host.h is empty now.

Ok, here's a version with all the compaints fixed.  Anton, do you have
a bugzilla PR for the case where you saw the races on ppc64?  Having
one would probably be really helpfull to get this patch into Linus'
tree..

===== drivers/ieee1394/sbp2.c 1.44 vs edited =====
--- 1.44/drivers/ieee1394/sbp2.c	Tue Sep  2 20:08:00 2003
+++ edited/drivers/ieee1394/sbp2.c	Wed Nov 12 14:15:46 2003
@@ -991,7 +991,7 @@
 static void sbp2_remove_device(struct scsi_id_instance_data *scsi_id)
 {
 	struct sbp2scsi_host_info *hi = scsi_id->hi;
-	struct scsi_device *sdev = scsi_find_device(hi->scsi_host, 0, scsi_id->id, 0);
+	struct scsi_device *sdev;
 
 	SBP2_DEBUG("sbp2_remove_device");
 
@@ -999,8 +999,13 @@
 	sbp2scsi_complete_all_commands(scsi_id, DID_NO_CONNECT);
 
 	/* Remove it from the scsi layer now */
-	if (sdev)
+	/* XXX(hch): why can't we simply cache the scsi_device
+	   	     in struct scsi_id_instance_data? */
+	sdev = scsi_device_lookup(hi->scsi_host, 0, scsi_id->id, 0);
+	if (sdev) {
 		scsi_remove_device(sdev);
+		scsi_device_put(sdev);
+	}
 
 	sbp2util_remove_command_orb_pool(scsi_id);
 
===== drivers/scsi/53c700.c 1.44 vs edited =====
--- 1.44/drivers/scsi/53c700.c	Sat Sep 20 11:35:53 2003
+++ edited/drivers/scsi/53c700.c	Wed Nov 12 14:15:46 2003
@@ -1065,7 +1065,7 @@
 		DEBUG(("scsi%d: (%d:%d) RESELECTED!\n",
 		       host->host_no, reselection_id, lun));
 		/* clear the reselection indicator */
-		SDp = scsi_find_device(host, 0, reselection_id, lun);
+		SDp = __scsi_device_lookup(host, 0, reselection_id, lun);
 		if(unlikely(SDp == NULL)) {
 			printk(KERN_ERR "scsi%d: (%d:%d) HAS NO device\n",
 			       host->host_no, reselection_id, lun);
@@ -1498,7 +1498,7 @@
 			       host->host_no, SCp, SCp == NULL ? NULL : SCp->host_scribble, dsp, dsp - hostdata->pScript);
 
 			/* clear all the negotiated parameters */
-	    		list_for_each_entry(SDp, &host->my_devices, siblings)
+			__shost_for_each_device(SDp, host)
 				SDp->hostdata = 0;
 			
 			/* clear all the slots and their pending commands */
===== drivers/scsi/NCR53C9x.c 1.28 vs edited =====
--- 1.28/drivers/scsi/NCR53C9x.c	Sun Jul 27 21:24:27 2003
+++ edited/drivers/scsi/NCR53C9x.c	Wed Nov 12 14:15:46 2003
@@ -815,6 +815,7 @@
 
 static int esp_host_info(struct NCR_ESP *esp, char *ptr, off_t offset, int len)
 {
+	struct scsi_device *sdev;
 	struct info_str info;
 	int i;
 
@@ -867,23 +868,20 @@
 	
 	/* Now describe the state of each existing target. */
 	copy_info(&info, "Target #\tconfig3\t\tSync Capabilities\tDisconnect\n");
-	for(i = 0; i < 15; i++) {
-		if(esp->targets_present & (1 << i)) {
-			Scsi_Device *SDptr;
-			struct esp_device *esp_dev;
 
-			list_for_each_entry(SDptr, &esp->ehost->my_devices,
-					siblings)
-				if(SDptr->id == i)
-					break;
+	shost_for_each_device(sdev, esp->ehost) {
+		struct esp_device *esp_dev = sdev->hostdata;
+		uint id = sdev->id;
+
+		if (!(esp->targets_present & (1 << id)))
+			continue;
 
-			esp_dev = SDptr->hostdata;
-			copy_info(&info, "%d\t\t", i);
-			copy_info(&info, "%08lx\t", esp->config3[i]);
-			copy_info(&info, "[%02lx,%02lx]\t\t\t", esp_dev->sync_max_offset,
-				  esp_dev->sync_min_period);
-			copy_info(&info, "%s\n", esp_dev->disconnect ? "yes" : "no");
-		}
+		copy_info(&info, "%d\t\t", id);
+		copy_info(&info, "%08lx\t", esp->config3[id]);
+		copy_info(&info, "[%02lx,%02lx]\t\t\t",
+			esp_dev->sync_max_offset,
+			esp_dev->sync_min_period);
+		copy_info(&info, "%s\n", esp_dev->disconnect ? "yes" : "no");
 	}
 
 	return info.pos > info.offset? info.pos - info.offset : 0;
--- 1.33/drivers/scsi/esp.c	Sun Aug  3 18:07:28 2003
+++ edited/drivers/scsi/esp.c	Wed Nov 12 14:15:46 2003
@@ -1309,6 +1309,7 @@
 
 static int esp_host_info(struct esp *esp, char *ptr, off_t offset, int len)
 {
+	struct scsi_device *sdev;
 	struct info_str info;
 	int i;
 
@@ -1384,25 +1385,23 @@
 	
 	/* Now describe the state of each existing target. */
 	copy_info(&info, "Target #\tconfig3\t\tSync Capabilities\tDisconnect\tWide\n");
-	for (i = 0; i < 15; i++) {
-		if (esp->targets_present & (1 << i)) {
-			Scsi_Device *SDptr;
-			struct esp_device *esp_dev;
 
-			list_for_each_entry(SDptr, &esp->ehost->my_devices,
-					siblings)
-				if(SDptr->id == i)
-					break;
+	shost_for_each_device(sdev, esp->ehost) {
+		struct esp_device *esp_dev = sdev->hostdata;
+		uint id = sdev->id;
+
+		if (!(esp->targets_present & (1 << id)))
+			continue;
 
-			esp_dev = SDptr->hostdata;
-			copy_info(&info, "%d\t\t", i);
-			copy_info(&info, "%08lx\t", esp->config3[i]);
-			copy_info(&info, "[%02lx,%02lx]\t\t\t", esp_dev->sync_max_offset,
-				  esp_dev->sync_min_period);
-			copy_info(&info, "%s\t\t", esp_dev->disconnect ? "yes" : "no");
-			copy_info(&info, "%s\n",
-				  (esp->config3[i] & ESP_CONFIG3_EWIDE) ? "yes" : "no");
-		}
+		copy_info(&info, "%d\t\t", id);
+		copy_info(&info, "%08lx\t", esp->config3[id]);
+		copy_info(&info, "[%02lx,%02lx]\t\t\t",
+			esp_dev->sync_max_offset,
+			esp_dev->sync_min_period);
+		copy_info(&info, "%s\t\t",
+			esp_dev->disconnect ? "yes" : "no");
+		copy_info(&info, "%s\n",
+			(esp->config3[id] & ESP_CONFIG3_EWIDE) ? "yes" : "no");
 	}
 	return info.pos > info.offset? info.pos - info.offset : 0;
 }
===== drivers/scsi/fcal.c 1.13 vs edited =====
--- 1.13/drivers/scsi/fcal.c	Sat Sep 20 16:06:41 2003
+++ edited/drivers/scsi/fcal.c	Wed Nov 12 14:15:46 2003
@@ -228,7 +228,7 @@
 #endif
 	SPRINTF ("Initiator AL-PA: %02x\n", fc->sid);
 
-	SPRINTF ("\nAttached devices: %s\n", !list_empty(&host->my_devices) ? "" : "none");
+	SPRINTF ("\nAttached devices:\n");
 	
 	for (i = 0; i < fc->posmap->len; i++) {
 		unsigned char alpa = fc->posmap->list[i];
===== drivers/scsi/hosts.c 1.94 vs edited =====
--- 1.94/drivers/scsi/hosts.c	Sun Sep 21 19:52:38 2003
+++ edited/drivers/scsi/hosts.c	Wed Nov 12 14:15:46 2003
@@ -1,6 +1,7 @@
 /*
  *  hosts.c Copyright (C) 1992 Drew Eckhardt
  *          Copyright (C) 1993, 1994, 1995 Eric Youngdale
+ *          Copyright (C) 2002-2003 Christoph Hellwig
  *
  *  mid to lowlevel SCSI driver interface
  *      Initial versions: Drew Eckhardt
@@ -30,8 +31,8 @@
 #include <linux/completion.h>
 #include <linux/unistd.h>
 
+#include <scsi/scsi_host.h>
 #include "scsi.h"
-#include "hosts.h"
 
 #include "scsi_priv.h"
 #include "scsi_logging.h"
@@ -50,6 +51,11 @@
 	.release	= scsi_host_cls_release,
 };
 
+static int scsi_device_cancel_cb(struct device *dev, void *data)
+{
+	return scsi_device_cancel(to_scsi_device(dev), *(int *)data);
+}
+
 /**
  * scsi_host_cancel - cancel outstanding IO to this host
  * @shost:	pointer to struct Scsi_Host
@@ -57,11 +63,7 @@
  **/
 void scsi_host_cancel(struct Scsi_Host *shost, int recovery)
 {
-	unsigned long flags;
-
-	spin_lock_irqsave(shost->host_lock, flags);
 	set_bit(SHOST_CANCEL, &shost->shost_state);
-	spin_unlock_irqrestore(shost->host_lock, flags);
 	device_for_each_child(&shost->shost_gendev, &recovery,
 			      scsi_device_cancel_cb);
 	wait_event(shost->host_wait, (!test_bit(SHOST_RECOVERY,
@@ -74,15 +76,11 @@
  **/
 void scsi_remove_host(struct Scsi_Host *shost)
 {
-	unsigned long flags;
-
 	scsi_host_cancel(shost, 0);
 	scsi_proc_host_rm(shost);
 	scsi_forget_host(shost);
 
-	spin_lock_irqsave(shost->host_lock, flags);
 	set_bit(SHOST_DEL, &shost->shost_state);
-	spin_unlock_irqrestore(shost->host_lock, flags);
 
 	class_device_unregister(&shost->shost_classdev);
 	device_del(&shost->shost_gendev);
@@ -209,7 +207,7 @@
 
 	spin_lock_init(&shost->default_lock);
 	scsi_assign_lock(shost, &shost->default_lock);
-	INIT_LIST_HEAD(&shost->my_devices);
+	INIT_LIST_HEAD(&shost->__devices);
 	INIT_LIST_HEAD(&shost->eh_cmd_q);
 	INIT_LIST_HEAD(&shost->starved_list);
 	init_waitqueue_head(&shost->host_wait);
@@ -323,23 +321,20 @@
  **/
 struct Scsi_Host *scsi_host_lookup(unsigned short hostnum)
 {
-	struct class *class = class_get(&shost_class);
+	struct class *class = &shost_class;
 	struct class_device *cdev;
 	struct Scsi_Host *shost = ERR_PTR(-ENXIO), *p;
 
-	if (class) {
-		down_read(&class->subsys.rwsem);
-		list_for_each_entry(cdev, &class->children, node) {
-			p = class_to_shost(cdev);
-			if (p->host_no == hostnum) {
-				shost = scsi_host_get(p);
-				break;
-			}
+	down_read(&class->subsys.rwsem);
+	list_for_each_entry(cdev, &class->children, node) {
+		p = class_to_shost(cdev);
+		if (p->host_no == hostnum) {
+			shost = scsi_host_get(p);
+			break;
 		}
-		up_read(&class->subsys.rwsem);
 	}
+	up_read(&class->subsys.rwsem);
 
-	class_put(&shost_class);
 	return shost;
 }
 
===== drivers/scsi/hosts.h 1.75 vs edited =====
--- 1.75/drivers/scsi/hosts.h	Mon Sep  1 22:56:57 2003
+++ edited/drivers/scsi/hosts.h	Wed Nov 12 14:15:46 2003
@@ -1,49 +1,2 @@
-/*
- *  hosts.h Copyright (C) 1992 Drew Eckhardt
- *          Copyright (C) 1993, 1994, 1995, 1998, 1999 Eric Youngdale
- *
- *  mid to low-level SCSI driver interface header
- *      Initial versions: Drew Eckhardt
- *      Subsequent revisions: Eric Youngdale
- *
- *  <drew@colorado.edu>
- *
- *	 Modified by Eric Youngdale eric@andante.org to
- *	 add scatter-gather, multiple outstanding request, and other
- *	 enhancements.
- *
- *  Further modified by Eric Youngdale to support multiple host adapters
- *  of the same type.
- *
- *  Jiffies wrap fixes (host->resetting), 3 Dec 1998 Andrea Arcangeli
- *
- *  Restructured scsi_host lists and associated functions.
- *  September 04, 2002 Mike Anderson (andmike@us.ibm.com)
- */
-
-#ifndef _HOSTS_H
-#define _HOSTS_H
-
-#include <linux/config.h>
-
+// #warning "This file is obsolete, please use <scsi/scsi_host.h> instead"
 #include <scsi/scsi_host.h>
-
-/**
- * scsi_find_device - find a device given the host
- * @shost:	SCSI host pointer
- * @channel:	SCSI channel (zero if only one channel)
- * @pun:	SCSI target number (physical unit number)
- * @lun:	SCSI Logical Unit Number
- **/
-static inline struct scsi_device *scsi_find_device(struct Scsi_Host *shost,
-                                            int channel, int pun, int lun) {
-        struct scsi_device *sdev;
-
-	list_for_each_entry (sdev, &shost->my_devices, siblings)
-                if (sdev->channel == channel && sdev->id == pun
-                   && sdev->lun ==lun)
-                        return sdev;
-        return NULL;
-}
-
-#endif
===== drivers/scsi/scsi.c 1.129 vs edited =====
--- 1.129/drivers/scsi/scsi.c	Sat Oct 18 01:14:06 2003
+++ edited/drivers/scsi/scsi.c	Wed Nov 12 14:15:46 2003
@@ -1,6 +1,7 @@
 /*
  *  scsi.c Copyright (C) 1992 Drew Eckhardt
  *         Copyright (C) 1993, 1994, 1995, 1999 Eric Youngdale
+ *         Copyright (C) 2002, 2003 Christoph Hellwig
  *
  *  generic mid-level SCSI driver
  *      Initial versions: Drew Eckhardt
@@ -36,7 +37,6 @@
  *  out_of_space hacks, D. Gilbert (dpg) 990608
  */
 
-#include <linux/config.h>
 #include <linux/module.h>
 #include <linux/moduleparam.h>
 #include <linux/kernel.h>
@@ -54,8 +54,8 @@
 #include <linux/kmod.h>
 #include <linux/interrupt.h>
 
+#include <scsi/scsi_host.h>
 #include "scsi.h"
-#include "hosts.h"
 
 #include "scsi_priv.h"
 #include "scsi_logging.h"
@@ -883,49 +883,124 @@
 	return depth;
 }
 
+/**
+ * scsi_device_get  -  get an addition reference to a scsi_device
+ * @sdev:	device to get a reference to
+ *
+ * Gets a reference to the scsi_device and increments the use count
+ * of the underlying LLDD module.  You must hold host_lock of the
+ * parent Scsi_Host or already have a reference when calling this.
+ */
 int scsi_device_get(struct scsi_device *sdev)
 {
-	struct class *class = class_get(&sdev_class);
-
-	if (!class)
-		goto out;
 	if (test_bit(SDEV_DEL, &sdev->sdev_state))
-		goto out;
-	if (!try_module_get(sdev->host->hostt->module))
-		goto out;
+		return -ENXIO;
 	if (!get_device(&sdev->sdev_gendev))
-		goto out_put_module;
-	atomic_inc(&sdev->access_count);
-	class_put(&sdev_class);
+		return -ENXIO;
+	if (!try_module_get(sdev->host->hostt->module)) {
+		put_device(&sdev->sdev_gendev);
+		return -ENXIO;
+	}
 	return 0;
+}
+EXPORT_SYMBOL(scsi_device_get);
 
- out_put_module:
+/**
+ * scsi_device_put  -  release a reference to a scsi_device
+ * @sdev:	device to release a reference on.
+ *
+ * Release a reference to the scsi_device and decrements the use count
+ * of the underlying LLDD module.  The device is freed once the last
+ * user vanishes.
+ */
+void scsi_device_put(struct scsi_device *sdev)
+{
 	module_put(sdev->host->hostt->module);
- out:
-	class_put(&sdev_class);
-	return -ENXIO;
+	put_device(&sdev->sdev_gendev);
 }
+EXPORT_SYMBOL(scsi_device_put);
 
-void scsi_device_put(struct scsi_device *sdev)
+/* helper for shost_for_each_device, thus not documented */
+struct scsi_device *__scsi_iterate_devices(struct Scsi_Host *shost,
+					   struct scsi_device *prev)
 {
-	struct class *class = class_get(&sdev_class);
+	struct list_head *list = (prev ? &prev->siblings : &shost->__devices);
+	struct scsi_device *next = NULL;
+	unsigned long flags;
 
-	if (!class)
-		return;
+	spin_lock_irqsave(shost->host_lock, flags);
+	while (list->next != &shost->__devices) {
+		next = list_entry(list->next, struct scsi_device, siblings);
+		/* skip devices that we can't get a reference to */
+		if (!scsi_device_get(next))
+			break;
+		list = list->next;
+	}
+	spin_unlock_irqrestore(shost->host_lock, flags);
 
-	module_put(sdev->host->hostt->module);
-	atomic_dec(&sdev->access_count);
-	put_device(&sdev->sdev_gendev);
-	class_put(&sdev_class);
+	if (prev)
+		scsi_device_put(prev);
+	return next;
 }
+EXPORT_SYMBOL(__scsi_iterate_devices);
 
-int scsi_device_cancel_cb(struct device *dev, void *data)
+/**
+ * scsi_device_lookup - find a device given the host (UNLOCKED)
+ * @shost:	SCSI host pointer
+ * @channel:	SCSI channel (zero if only one channel)
+ * @pun:	SCSI target number (physical unit number)
+ * @lun:	SCSI Logical Unit Number
+ *
+ * Looks up the scsi_device with the specified @channel, @id, @lun for a
+ * give host. The returned scsi_device does not have an additional reference.
+ * You must hold the host's host_lock over this call and any access to the
+ * returned scsi_device.
+ *
+ * Note:  The only reason why drivers would want to use this is because
+ * they're need to access the device list in irq context.  Otherwise you
+ * really want to use scsi_device_lookup instead.
+ **/
+struct scsi_device *__scsi_device_lookup(struct Scsi_Host *shost,
+		uint channel, uint id, uint lun)
+{
+	struct scsi_device *sdev;
+
+	list_for_each_entry(sdev, &shost->__devices, siblings) {
+		if (sdev->channel == channel && sdev->id == id &&
+				sdev->lun ==lun)
+			return sdev;
+	}
+
+	return NULL;
+}
+EXPORT_SYMBOL(__scsi_device_lookup);
+
+/**
+ * scsi_device_lookup - find a device given the host
+ * @shost:	SCSI host pointer
+ * @channel:	SCSI channel (zero if only one channel)
+ * @id:		SCSI target number (physical unit number)
+ * @lun:	SCSI Logical Unit Number
+ *
+ * Looks up the scsi_device with the specified @channel, @id, @lun for a
+ * give host.  The returned scsi_device has an additional reference that
+ * needs to be release with scsi_host_put once you're done with it.
+ **/
+struct scsi_device *scsi_device_lookup(struct Scsi_Host *shost,
+		uint channel, uint id, uint lun)
 {
-	struct scsi_device *sdev = to_scsi_device(dev);
-	int recovery = *(int *)data;
+	struct scsi_device *sdev;
+	unsigned long flags;
+
+	spin_lock_irqsave(shost->host_lock, flags);
+	sdev = __scsi_device_lookup(shost, channel, id, lun);
+	if (sdev && scsi_device_get(sdev))
+		sdev = NULL;
+	spin_unlock_irqrestore(shost->host_lock, flags);
 
-	return scsi_device_cancel(sdev, recovery);
+	return sdev;
 }
+EXPORT_SYMBOL(scsi_device_lookup);
 
 /**
  * scsi_device_cancel - cancel outstanding IO to this device
--- 1.113/drivers/scsi/scsi_lib.c	Sat Sep 20 15:53:02 2003
+++ edited/drivers/scsi/scsi_lib.c	Wed Nov 12 14:15:46 2003
@@ -16,9 +16,9 @@
 #include <linux/init.h>
 #include <linux/pci.h>
 
-#include "scsi.h"
-#include "hosts.h"
 #include <scsi/scsi_driver.h>
+#include <scsi/scsi_host.h>
+#include "scsi.h"
 
 #include "scsi_priv.h"
 #include "scsi_logging.h"
@@ -335,13 +335,14 @@
  */
 static void scsi_single_lun_run(struct scsi_device *current_sdev)
 {
-	struct scsi_device *sdev;
+	struct Scsi_Host *shost = current_sdev->host;
+	struct scsi_device *sdev, *tmp;
 	unsigned long flags;
 
-	spin_lock_irqsave(current_sdev->host->host_lock, flags);
+	spin_lock_irqsave(shost->host_lock, flags);
 	WARN_ON(!current_sdev->sdev_target->starget_sdev_user);
 	current_sdev->sdev_target->starget_sdev_user = NULL;
-	spin_unlock_irqrestore(current_sdev->host->host_lock, flags);
+	spin_unlock_irqrestore(shost->host_lock, flags);
 
 	/*
 	 * Call blk_run_queue for all LUNs on the target, starting with
@@ -351,21 +352,26 @@
 	 */
 	blk_run_queue(current_sdev->request_queue);
 
-	spin_lock_irqsave(current_sdev->host->host_lock, flags);
-	if (current_sdev->sdev_target->starget_sdev_user) {
-		/*
-		 * After unlock, this races with anyone clearing
-		 * starget_sdev_user, but we (should) always enter this
-		 * function again, avoiding any problems.
-		 */
-		spin_unlock_irqrestore(current_sdev->host->host_lock, flags);
-		return;
-	}
-	spin_unlock_irqrestore(current_sdev->host->host_lock, flags);
+	/*
+	 * After unlock, this races with anyone clearing starget_sdev_user,
+	 * but we always enter this function again, avoiding any problems.
+	 */
+	spin_lock_irqsave(shost->host_lock, flags);
+	if (current_sdev->sdev_target->starget_sdev_user)
+		goto out;
+	list_for_each_entry_safe(sdev, tmp, &current_sdev->same_target_siblings,
+			same_target_siblings) {
+		if (scsi_device_get(sdev))
+			continue;
 
-	list_for_each_entry(sdev, &current_sdev->same_target_siblings,
-			    same_target_siblings)
+		spin_unlock_irqrestore(shost->host_lock, flags);
 		blk_run_queue(sdev->request_queue);
+		spin_lock_irqsave(shost->host_lock, flags);
+	
+		scsi_device_put(sdev);
+	}
+ out:
+	spin_unlock_irqrestore(shost->host_lock, flags);
 }
 
 /*
--- 1.38/drivers/scsi/scsi_proc.c	Sat Sep 20 11:35:07 2003
+++ edited/drivers/scsi/scsi_proc.c	Wed Nov 12 14:15:46 2003
@@ -27,8 +27,8 @@
 #include <linux/seq_file.h>
 #include <asm/uaccess.h>
 
+#include <scsi/scsi_host.h>
 #include "scsi.h"
-#include "hosts.h"
 
 #include "scsi_priv.h"
 #include "scsi_logging.h"
@@ -212,15 +212,13 @@
 	shost = scsi_host_lookup(host);
 	if (IS_ERR(shost))
 		return PTR_ERR(shost);
-	sdev = scsi_find_device(shost, channel, id, lun);
-	if (!sdev)
-		goto out;
-	if (atomic_read(&sdev->access_count))
-		goto out;
+	sdev = scsi_device_lookup(shost, channel, id, lun);
+	if (sdev) {
+		scsi_remove_device(sdev);
+		scsi_device_put(sdev);
+		error = 0;
+	}
 
-	scsi_remove_device(sdev);
-	error = 0;
-out:
 	scsi_host_put(shost);
 	return error;
 }
===== drivers/scsi/scsi_scan.c 1.109 vs edited =====
--- 1.109/drivers/scsi/scsi_scan.c	Thu Oct 16 10:56:58 2003
+++ edited/drivers/scsi/scsi_scan.c	Wed Nov 12 14:15:46 2003
@@ -32,10 +32,10 @@
 #include <linux/blkdev.h>
 #include <asm/semaphore.h>
 
-#include "scsi.h"
-#include "hosts.h"
 #include <scsi/scsi_driver.h>
 #include <scsi/scsi_devinfo.h>
+#include <scsi/scsi_host.h>
+#include "scsi.h"
 
 #include "scsi_priv.h"
 #include "scsi_logging.h"
@@ -190,13 +190,13 @@
 	       	uint channel, uint id, uint lun)
 {
 	struct scsi_device *sdev, *device;
+	unsigned long flags;
 
 	sdev = kmalloc(sizeof(*sdev), GFP_ATOMIC);
 	if (!sdev)
 		goto out;
 
 	memset(sdev, 0, sizeof(*sdev));
-	atomic_set(&sdev->access_count, 0);
 	sdev->vendor = scsi_null_device_strs;
 	sdev->model = scsi_null_device_strs;
 	sdev->rev = scsi_null_device_strs;
@@ -240,7 +240,8 @@
 	 * If there are any same target siblings, add this to the
 	 * sibling list
 	 */
-	list_for_each_entry(device, &shost->my_devices, siblings) {
+	spin_lock_irqsave(shost->host_lock, flags);
+	list_for_each_entry(device, &shost->__devices, siblings) {
 		if (device->id == sdev->id &&
 		    device->channel == sdev->channel) {
 			list_add_tail(&sdev->same_target_siblings,
@@ -258,10 +259,8 @@
 	if (!sdev->scsi_level)
 		sdev->scsi_level = SCSI_2;
 
-	/*
-	 * Add it to the end of the shost->my_devices list.
-	 */
-	list_add_tail(&sdev->siblings, &shost->my_devices);
+	list_add_tail(&sdev->siblings, &shost->__devices);
+	spin_unlock_irqrestore(shost->host_lock, flags);
 	return sdev;
 
 out_free_queue:
@@ -285,21 +284,21 @@
 {
 	unsigned long flags;
 
+	spin_lock_irqsave(sdev->host->host_lock, flags);
 	list_del(&sdev->siblings);
 	list_del(&sdev->same_target_siblings);
+	spin_unlock_irqrestore(sdev->host->host_lock, flags);
 
 	if (sdev->request_queue)
 		scsi_free_queue(sdev->request_queue);
-	if (sdev->inquiry)
-		kfree(sdev->inquiry);
+
 	spin_lock_irqsave(sdev->host->host_lock, flags);
 	list_del(&sdev->starved_entry);
-	if (sdev->single_lun) {
-		if (--sdev->sdev_target->starget_refcnt == 0)
-			kfree(sdev->sdev_target);
-	}
+	if (sdev->single_lun && --sdev->sdev_target->starget_refcnt == 0)
+		kfree(sdev->sdev_target);
 	spin_unlock_irqrestore(sdev->host->host_lock, flags);
 
+	kfree(sdev->inquiry);
 	kfree(sdev);
 }
 
@@ -678,7 +677,7 @@
 	 * host adapter calls into here with rescan == 0.
 	 */
 	if (rescan) {
-		sdev = scsi_find_device(host, channel, id, lun);
+		sdev = scsi_device_lookup(host, channel, id, lun);
 		if (sdev) {
 			SCSI_LOG_SCAN_BUS(3, printk(KERN_INFO
 				"scsi scan: device exists on <%d:%d:%d:%d>\n",
@@ -689,6 +688,8 @@
 				*bflagsp = scsi_get_device_flags(sdev,
 								 sdev->vendor,
 								 sdev->model);
+			/* XXX: bandaid until callers do refcounting */
+			scsi_device_put(sdev);
 			return SCSI_SCAN_LUN_PRESENT;
 		}
 	}
@@ -1232,14 +1233,25 @@
 
 void scsi_forget_host(struct Scsi_Host *shost)
 {
-	struct list_head *le, *lh;
-	struct scsi_device *sdev;
+	struct scsi_device *sdev, *tmp;
+	unsigned long flags;
 
-	list_for_each_safe(le, lh, &shost->my_devices) {
-		sdev = list_entry(le, struct scsi_device, siblings);
-		
+	/*
+	 * Ok, this look a bit strange.  We always look for the first device
+	 * on the list as scsi_remove_device removes them from it - thus we
+	 * also have to release the lock.
+	 * We don't need to get another reference to the device before
+	 * releasing the lock as we already own the reference from
+	 * scsi_register_device that's release in scsi_remove_device.  And
+	 * after that we don't look at sdev anymore.
+	 */
+	spin_lock_irqsave(shost->host_lock, flags);
+	list_for_each_entry_safe(sdev, tmp, &shost->__devices, siblings) {
+		spin_unlock_irqrestore(shost->host_lock, flags);
 		scsi_remove_device(sdev);
+		spin_lock_irqsave(shost->host_lock, flags);
 	}
+	spin_unlock_irqrestore(shost->host_lock, flags);
 }
 
 /*
===== drivers/scsi/scsi_syms.c 1.45 vs edited =====
--- 1.45/drivers/scsi/scsi_syms.c	Thu Jul 31 16:32:18 2003
+++ edited/drivers/scsi/scsi_syms.c	Wed Nov 12 14:15:46 2003
@@ -18,13 +18,14 @@
 #include <asm/irq.h>
 #include <asm/dma.h>
 
-#include "scsi.h"
 #include <scsi/scsi_driver.h>
+#include <scsi/scsi_host.h>
 #include <scsi/scsi_ioctl.h>
-#include "hosts.h"
+#include <scsi/scsicam.h>
+#include "scsi.h"
+
 #include "scsi_logging.h"
 
-#include <scsi/scsicam.h>
 
 /*
  * This source file contains the symbol table used by scsi loadable
@@ -82,8 +83,6 @@
 
 EXPORT_SYMBOL(scsi_io_completion);
 
-EXPORT_SYMBOL(scsi_device_get);
-EXPORT_SYMBOL(scsi_device_put);
 EXPORT_SYMBOL(scsi_add_device);
 EXPORT_SYMBOL(scsi_remove_device);
 EXPORT_SYMBOL(scsi_device_cancel);
===== drivers/scsi/scsi_sysfs.c 1.35 vs edited =====
--- 1.35/drivers/scsi/scsi_sysfs.c	Sat Oct 18 01:34:55 2003
+++ edited/drivers/scsi/scsi_sysfs.c	Wed Nov 12 14:15:46 2003
@@ -11,8 +11,9 @@
 #include <linux/init.h>
 #include <linux/blkdev.h>
 #include <linux/device.h>
+
+#include <scsi/scsi_host.h>
 #include "scsi.h"
-#include "hosts.h"
 
 #include "scsi_priv.h"
 #include "scsi_logging.h"
@@ -257,20 +258,12 @@
 	scsi_rescan_device(dev);
 	return count;
 }
-
 static DEVICE_ATTR(rescan, S_IWUSR, NULL, store_rescan_field)
 
 static ssize_t sdev_store_delete(struct device *dev, const char *buf,
 				 size_t count)
 {
-	struct scsi_device *sdev = to_scsi_device(dev);
-
-	/*
-	 * FIXME and scsi_proc.c: racey use of access_count,
-	 */
-	if (atomic_read(&sdev->access_count))
-		return -EBUSY;
-	scsi_remove_device(sdev);
+	scsi_remove_device(to_scsi_device(dev));
 	return count;
 };
 static DEVICE_ATTR(delete, S_IWUSR, NULL, sdev_store_delete);
@@ -403,22 +396,12 @@
  **/
 void scsi_remove_device(struct scsi_device *sdev)
 {
-	struct class *class = class_get(&sdev_class);
-
 	class_device_unregister(&sdev->sdev_classdev);
-
-	if (class) {
-		down_write(&class->subsys.rwsem);
-		set_bit(SDEV_DEL, &sdev->sdev_state);
-		if (sdev->host->hostt->slave_destroy)
-			sdev->host->hostt->slave_destroy(sdev);
-		device_del(&sdev->sdev_gendev);
-		up_write(&class->subsys.rwsem);
-	}
-
+	set_bit(SDEV_DEL, &sdev->sdev_state);
+	if (sdev->host->hostt->slave_destroy)
+		sdev->host->hostt->slave_destroy(sdev);
+	device_del(&sdev->sdev_gendev);
 	put_device(&sdev->sdev_gendev);
-
-	class_put(&sdev_class);
 }
 
 int scsi_register_driver(struct device_driver *drv)
===== drivers/scsi/sg.c 1.70 vs edited =====
--- 1.70/drivers/scsi/sg.c	Tue Oct 21 00:15:18 2003
+++ edited/drivers/scsi/sg.c	Wed Nov 12 14:15:46 2003
@@ -914,7 +914,8 @@
 	case SG_GET_VERSION_NUM:
 		return put_user(sg_version_num, (int *) arg);
 	case SG_GET_ACCESS_COUNT:
-		val = (sdp->device ? atomic_read(&sdp->device->access_count) : 0);
+		/* faked - we don't have a real access count anymore */
+		val = (sdp->device ? 1 : 0);
 		return put_user(val, (int *) arg);
 	case SG_GET_REQUEST_TABLE:
 		result = verify_area(VERIFY_WRITE, (void *) arg,
@@ -2903,7 +2904,7 @@
 			PRINT_PROC("%d\t%d\t%d\t%d\t%d\t%d\t%d\t%d\t%d\n",
 				   scsidp->host->host_no, scsidp->channel,
 				   scsidp->id, scsidp->lun, (int) scsidp->type,
-				   (int) atomic_read(&scsidp->access_count),
+				   1,
 				   (int) scsidp->queue_depth,
 				   (int) scsidp->device_busy,
 				   (int) scsidp->online);
===== drivers/scsi/arm/acornscsi.c 1.35 vs edited =====
--- 1.35/drivers/scsi/arm/acornscsi.c	Mon Aug 25 15:37:34 2003
+++ edited/drivers/scsi/arm/acornscsi.c	Wed Nov 12 14:15:46 2003
@@ -2930,7 +2930,7 @@
 
     p += sprintf(p, "\nAttached devices:\n");
 
-    list_for_each_entry(scd, &instance->my_devices, siblings) {
+    shost_for_each_device(scd, instance) {
 	p += sprintf(p, "Device/Lun TaggedQ      Sync\n");
 	p += sprintf(p, "     %d/%d   ", scd->id, scd->lun);
 	if (scd->tagged_supported)
@@ -2953,8 +2953,10 @@
 	    p = buffer;
 	}
 	pos = p - buffer;
-	if (pos + begin > offset + length)
+	if (pos + begin > offset + length) {
+	    scsi_device_put(scd);
 	    break;
+	}
     }
 
     pos = p - buffer;
===== include/scsi/scsi_device.h 1.9 vs edited =====
--- 1.9/include/scsi/scsi_device.h	Thu Oct 16 10:56:58 2003
+++ edited/include/scsi/scsi_device.h	Wed Nov 12 14:15:46 2003
@@ -22,10 +22,13 @@
 };
 
 struct scsi_device {
-	struct list_head    siblings;   /* list of all devices on this host */
-	struct list_head    same_target_siblings; /* just the devices sharing same target id */
 	struct Scsi_Host *host;
 	struct request_queue *request_queue;
+
+	/* the next two are protected by the host->host_lock */
+	struct list_head    siblings;   /* list of all devices on this host */
+	struct list_head    same_target_siblings; /* just the devices sharing same target id */
+
 	volatile unsigned short device_busy;	/* commands actually active on low-level */
 	spinlock_t sdev_lock;           /* also the request queue_lock */
 	spinlock_t list_lock;
@@ -45,8 +48,6 @@
 					 * vendor-specific cmd's */
 	unsigned sector_size;	/* size in bytes */
 
-	atomic_t access_count;	/* Count of open channels/mounts */
-
 	void *hostdata;		/* available to low-level driver */
 	char devfs_name[256];	/* devfs junk */
 	char type;
@@ -108,14 +109,48 @@
 extern struct scsi_device *scsi_add_device(struct Scsi_Host *,
 		uint, uint, uint);
 extern void scsi_remove_device(struct scsi_device *);
-extern int scsi_device_cancel_cb(struct device *, void *);
 extern int scsi_device_cancel(struct scsi_device *, int);
 
 extern int scsi_device_get(struct scsi_device *);
 extern void scsi_device_put(struct scsi_device *);
-
+extern struct scsi_device *scsi_device_lookup(struct Scsi_Host *,
+					      uint, uint, uint);
+extern struct scsi_device *__scsi_device_lookup(struct Scsi_Host *,
+						uint, uint, uint);
+
+/* only exposed to implement shost_for_each_device */
+extern struct scsi_device *__scsi_iterate_devices(struct Scsi_Host *,
+						  struct scsi_device *);
+
+/**
+ * shost_for_each_device  -  iterate over all devices of a host
+ * @sdev:	iterator
+ * @host:	host whiches devices we want to iterate over
+ *
+ * This traverses over each devices of @shost.  The devices have
+ * a reference that must be released by scsi_host_put when breaking
+ * out of the loop.
+ */
 #define shost_for_each_device(sdev, shost) \
-	list_for_each_entry((sdev), &((shost)->my_devices), siblings)
+	for ((sdev) = __scsi_iterate_devices((shost), NULL); \
+	     (sdev); \
+	     (sdev) = __scsi_iterate_devices((shost), (sdev)))
+
+/**
+ * __shost_for_each_device  -  iterate over all devices of a host (UNLOCKED)
+ * @sdev:	iterator
+ * @host:	host whiches devices we want to iterate over
+ *
+ * This traverses over each devices of @shost.  It does _not_ take a
+ * reference on the scsi_device, thus it the whole loop must be protected
+ * by shost->host_lock.
+ *
+ * Note:  The only reason why drivers would want to use this is because
+ * they're need to access the device list in irq context.  Otherwise you
+ * really want to use shost_for_each_device instead.
+ */
+#define __shost_for_each_device(sdev, shost) \
+	list_for_each_entry((sdev), &((shost)->__devices), siblings)
 
 extern void scsi_adjust_queue_depth(struct scsi_device *, int, int);
 extern int scsi_track_queue_full(struct scsi_device *, int);
--- 1.1/include/scsi/scsi_driver.h	Thu Jun 26 19:08:24 2003
+++ edited/include/scsi/scsi_driver.h	Wed Nov 12 14:15:46 2003
@@ -4,6 +4,7 @@
 #include <linux/device.h>
 
 struct module;
+struct scsi_cmnd;
 
 
 struct scsi_driver {
--- 1.12/include/scsi/scsi_host.h	Thu Oct 16 10:56:58 2003
+++ edited/include/scsi/scsi_host.h	Wed Nov 12 14:15:46 2003
@@ -363,19 +354,30 @@
 };
 
 struct Scsi_Host {
-	struct list_head	my_devices;
+	/*
+	 * __devices is protected by the host_lock, but you should
+	 * usually use scsi_device_lookup / shost_for_each_device
+	 * to access it and don't care about locking yourself.
+	 * In the rare case of beeing in irq context you can use
+	 * their __ prefixed variants with the lock held. NEVER
+	 * access this list directly from a driver.
+	 */
+	struct list_head	__devices;
+	
 	struct scsi_host_cmd_pool *cmd_pool;
 	spinlock_t		free_list_lock;
-	struct list_head	free_list;   /* backup store of cmd structs */
+	struct list_head	free_list; /* backup store of cmd structs */
 	struct list_head	starved_list;
 
 	spinlock_t		default_lock;
 	spinlock_t		*host_lock;
 
+	struct semaphore	scan_mutex;/* serialize scanning activity */
+
 	struct list_head	eh_cmd_q;
 	struct task_struct    * ehandler;  /* Error recovery thread. */
-	struct semaphore      * eh_wait;   /* The error recovery thread waits on
-                                          this. */
+	struct semaphore      * eh_wait;   /* The error recovery thread waits
+					      on this. */
 	struct completion     * eh_notify; /* wait for eh to begin or end */
 	struct semaphore      * eh_action; /* Wait for specific actions on the
                                           host. */
@@ -477,12 +479,6 @@
 	 * module_init/module_exit.
 	 */
 	struct list_head sht_legacy_list;
-
-	/*
-	 * This mutex serializes all scsi scanning activity from kernel- and
-	 * userspace.
-	 */
-	struct semaphore scan_mutex;
 
 	/*
 	 * We should ensure that this is aligned, both for better performance