Re: [PATCH] scsi_device refcounting and list lockdown

[Christoph Hellwig <hch@lst.de>]

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