[PATCH] scsi host/scsi device ref count cleanup 0/4

[PATCH] scsi host/scsi device ref count cleanup 0/4

[Mike Anderson]

This patch set is against scsi-misc-2.5

This set contains the following patches.
	stern_present - refresh of Alan's previous patch.
	shost_present - reduce users of shost_present.
	shost_state - Add some state flags and reorder cleanup.
	sdev_state - Add some state flags and reorder cleanup.

I still have a couple of outstanding issues 
	- sd_shutdown errors on surprise removal.
	- scsi_remove_single_device / scsi_remove_device cleanup around
	  the usage of access_count.

I tested the surprise removal using the scsi_debug add_host attribute.
I created two tgts on each host. One target I created a file system and
the other target I ran dd's. Post "-1" into add_host I did a umount -f
on the mounted file system while monitoring ref counts. I also checked the
reboot notifier list on shutdown to ensure it appeared correct.

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

[PATCH] scsi host/scsi device ref count cleanup 1/4

[Mike Anderson]

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

DESC
This is a refresh of a patch by Alan Stern to correct the removal of scsi
procfs host enteries

http://marc.theaimsgroup.com/?l=linux-scsi&m=105545175600506&w=2
EDESC


 drivers/scsi/hosts.c     |    4 +--
 drivers/scsi/scsi_priv.h |    4 +++
 drivers/scsi/scsi_proc.c |   49 ++++++++++++++++++++++++++++++-----------------
 3 files changed, 38 insertions(+), 19 deletions(-)

diff -puN drivers/scsi/hosts.c~stern_present drivers/scsi/hosts.c
--- remove-scsi-misc-2.5/drivers/scsi/hosts.c~stern_present	Tue Jul  8 14:19:47 2003
+++ remove-scsi-misc-2.5-andmike/drivers/scsi/hosts.c	Tue Jul  8 14:19:47 2003
@@ -112,7 +112,7 @@ void scsi_free_shost(struct Scsi_Host *s
 		shost->eh_notify = NULL;
 	}
 
-	shost->hostt->present--;
+	scsi_proc_hostdir_rm(shost->hostt);
 	scsi_destroy_command_freelist(shost);
 	kfree(shost);
 }
@@ -219,7 +219,7 @@ struct Scsi_Host *scsi_host_alloc(struct
 	kernel_thread((int (*)(void *))scsi_error_handler, shost, 0);
 	wait_for_completion(&complete);
 	shost->eh_notify = NULL;
-	shost->hostt->present++;
+	scsi_proc_hostdir_add(shost->hostt);
 	return shost;
  fail:
 	kfree(shost);
diff -puN drivers/scsi/scsi_priv.h~stern_present drivers/scsi/scsi_priv.h
--- remove-scsi-misc-2.5/drivers/scsi/scsi_priv.h~stern_present	Tue Jul  8 14:19:47 2003
+++ remove-scsi-misc-2.5-andmike/drivers/scsi/scsi_priv.h	Tue Jul  8 14:19:47 2003
@@ -90,11 +90,15 @@ extern void scsi_exit_queue(void);
 
 /* scsi_proc.c */
 #ifdef CONFIG_PROC_FS
+extern void scsi_proc_hostdir_add(struct scsi_host_template *);
+extern void scsi_proc_hostdir_rm(struct scsi_host_template *);
 extern void scsi_proc_host_add(struct Scsi_Host *);
 extern void scsi_proc_host_rm(struct Scsi_Host *);
 extern int scsi_init_procfs(void);
 extern void scsi_exit_procfs(void);
 #else
+# define scsi_proc_hostdir_add(sht)	do { } while (0)
+# define scsi_proc_hostdir_rm(sht)	do { } while (0)
 # define scsi_proc_host_add(shost)	do { } while (0)
 # define scsi_proc_host_rm(shost)	do { } while (0)
 # define scsi_init_procfs()		(0)
diff -puN drivers/scsi/scsi_proc.c~stern_present drivers/scsi/scsi_proc.c
--- remove-scsi-misc-2.5/drivers/scsi/scsi_proc.c~stern_present	Tue Jul  8 14:19:47 2003
+++ remove-scsi-misc-2.5-andmike/drivers/scsi/scsi_proc.c	Tue Jul  8 14:19:47 2003
@@ -41,6 +41,8 @@
 struct proc_dir_entry *proc_scsi;
 EXPORT_SYMBOL(proc_scsi);
 
+/* Protect sht->present and sht->proc_dir */
+static DECLARE_MUTEX(global_host_template_sem);
 
 static int proc_scsi_read(char *buffer, char **start, off_t offset,
 			  int length, int *eof, void *data)
@@ -77,16 +79,10 @@ out:
 	return ret;
 }
 
-void scsi_proc_host_add(struct Scsi_Host *shost)
+void scsi_proc_hostdir_add(struct scsi_host_template *sht)
 {
-	struct scsi_host_template *sht = shost->hostt;
-	struct proc_dir_entry *p;
-	char name[10];
-
-	if (!sht->proc_info)
-		return;
-
-	if (!sht->proc_dir) {
+	down(&global_host_template_sem);
+	if (!sht->present++) {
 		sht->proc_dir = proc_mkdir(sht->proc_name, proc_scsi);
         	if (!sht->proc_dir) {
 			printk(KERN_ERR "%s: proc_mkdir failed for %s\n",
@@ -95,6 +91,27 @@ void scsi_proc_host_add(struct Scsi_Host
 		}
 		sht->proc_dir->owner = sht->module;
 	}
+	up(&global_host_template_sem);
+}
+
+void scsi_proc_hostdir_rm(struct scsi_host_template *sht)
+{
+	down(&global_host_template_sem);
+	if (!--sht->present && sht->proc_dir) {
+		remove_proc_entry(sht->proc_name, proc_scsi);
+		sht->proc_dir = NULL;
+	}
+	up(&global_host_template_sem);
+}
+
+void scsi_proc_host_add(struct Scsi_Host *shost)
+{
+	struct scsi_host_template *sht = shost->hostt;
+	struct proc_dir_entry *p;
+	char name[10];
+
+	if (!sht->proc_dir)
+		return;
 
 	sprintf(name,"%d", shost->host_no);
 	p = create_proc_read_entry(name, S_IFREG | S_IRUGO | S_IWUSR,
@@ -107,20 +124,18 @@ void scsi_proc_host_add(struct Scsi_Host
 	} 
 
 	p->write_proc = proc_scsi_write_proc;
-	p->owner = shost->hostt->module;
+	p->owner = sht->module;
 }
 
 void scsi_proc_host_rm(struct Scsi_Host *shost)
 {
-	struct scsi_host_template *sht = shost->hostt;
 	char name[10];
 
-	if (sht->proc_info) {
-		sprintf(name,"%d", shost->host_no);
-		remove_proc_entry(name, sht->proc_dir);
-		if (!sht->present)
-			remove_proc_entry(sht->proc_name, proc_scsi);
-	}
+	if (!shost->hostt->proc_dir)
+		return;
+
+	sprintf(name,"%d", shost->host_no);
+	remove_proc_entry(name, shost->hostt->proc_dir);
 }
 
 static int proc_print_scsidevice(struct device *dev, void *data)

_

[PATCH] scsi host/scsi device ref count cleanup 2/4

[Mike Anderson]

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

DESC
This patch reduces the user of scsi_host present.

aic79xx_osm and aic7xxx_osm are 2.4 code. ioctl_probe has side effect of
needing present value.
EDESC


 drivers/scsi/NCR53c406a.c    |    5 +++--
 drivers/scsi/aacraid/linit.c |    1 -
 drivers/scsi/scsi_module.c   |    2 +-
 3 files changed, 4 insertions(+), 4 deletions(-)

diff -puN drivers/scsi/aacraid/linit.c~shost_present drivers/scsi/aacraid/linit.c
--- remove-scsi-misc-2.5/drivers/scsi/aacraid/linit.c~shost_present	Tue Jul  8 14:19:50 2003
+++ remove-scsi-misc-2.5-andmike/drivers/scsi/aacraid/linit.c	Tue Jul  8 14:19:50 2003
@@ -295,7 +295,6 @@ static int aac_detect(Scsi_Host_Template
 			printk(KERN_WARNING "aacraid: unable to register \"aac\" device.\n");
 	}
 
-	template->present = aac_count; /* # of cards of this type found */
 	return aac_count;
 }
 
diff -puN drivers/scsi/NCR53c406a.c~shost_present drivers/scsi/NCR53c406a.c
--- remove-scsi-misc-2.5/drivers/scsi/NCR53c406a.c~shost_present	Tue Jul  8 14:19:50 2003
+++ remove-scsi-misc-2.5-andmike/drivers/scsi/NCR53c406a.c	Tue Jul  8 14:19:50 2003
@@ -450,6 +450,7 @@ static __inline__ int NCR53c406a_pio_wri
 
 static int __init NCR53c406a_detect(Scsi_Host_Template * tpnt)
 {
+	int present = 0;
 	struct Scsi_Host *shpnt = NULL;
 #ifndef PORT_BASE
 	int i;
@@ -522,7 +523,7 @@ static int __init NCR53c406a_detect(Scsi
 
 	DEB(printk("NCR53c406a: using port_base 0x%x\n", port_base));
 
-	tpnt->present = 1;
+	present = 1;
 	tpnt->proc_name = "NCR53c406a";
 
 	shpnt = scsi_register(tpnt, 0);
@@ -576,7 +577,7 @@ static int __init NCR53c406a_detect(Scsi
 	sprintf(info_msg, "NCR53c406a at 0x%x, IRQ %d, %s PIO mode.", port_base, irq_level, fast_pio ? "fast" : "slow");
 #endif
 
-	return (tpnt->present);
+	return (present);
 
 #if USE_DMA
       err_free_irq:
diff -puN drivers/scsi/scsi_module.c~shost_present drivers/scsi/scsi_module.c
--- remove-scsi-misc-2.5/drivers/scsi/scsi_module.c~shost_present	Tue Jul  8 14:19:50 2003
+++ remove-scsi-misc-2.5-andmike/drivers/scsi/scsi_module.c	Tue Jul  8 14:19:50 2003
@@ -33,7 +33,7 @@ static int __init init_this_scsi_driver(
 	INIT_LIST_HEAD(&sht->legacy_hosts);
 
 	sht->detect(sht);
-	if (!sht->present)
+	if (list_empty(&sht->legacy_hosts))
 		return -ENODEV;
 
 	list_for_each_entry(shost, &sht->legacy_hosts, sht_legacy_list) {

_

[PATCH] scsi host/scsi device ref count cleanup 3/4

[Mike Anderson]

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

DESC
This is a cleanup patch for scsi_host removal.
	- Addition of a host_flags member to the scsi_host strucuture to
	  contain "state" of the host instance.
	- Added SHOST_ADD, SHOST_DEL, SHOST_CANCEL, SHOST_RECOVERY states
	  for a scsi host
	- Addtion / rename of a new function scsi_host_cancel to cancel
	  IOs in flight.
	- Usage of the host_flags flags member to stop scsi_host_get's and
	  calls to LLDD queucommand under certain states.
	- Reordered some of the scsi host sysfs unregistration.
EDESC


 drivers/scsi/hosts.c       |   62 +++++++++++++++++++++++++++------------------
 drivers/scsi/scsi.c        |   31 +++++++++++++++-------
 drivers/scsi/scsi_debug.c  |    5 ---
 drivers/scsi/scsi_error.c  |    7 ++---
 drivers/scsi/scsi_lib.c    |    5 ++-
 drivers/scsi/scsi_proc.c   |   12 ++++----
 drivers/scsi/scsi_syms.c   |    2 -
 drivers/scsi/scsi_sysfs.c  |   44 ++++++++++++++++++++++---------
 drivers/scsi/sg.c          |    3 +-
 drivers/usb/storage/usb.c  |    7 -----
 include/scsi/scsi_device.h |    2 -
 include/scsi/scsi_host.h   |   16 +++++++++--
 12 files changed, 122 insertions(+), 74 deletions(-)

diff -puN drivers/scsi/hosts.c~shost_state drivers/scsi/hosts.c
--- remove-scsi-misc-2.5/drivers/scsi/hosts.c~shost_state	Tue Jul  8 14:19:52 2003
+++ remove-scsi-misc-2.5-andmike/drivers/scsi/hosts.c	Tue Jul  8 14:19:52 2003
@@ -39,30 +39,35 @@
 
 static int scsi_host_next_hn;		/* host_no for next new host */
 
+
 /**
- * scsi_remove_host - check a scsi host for release and release
- * @shost:	a pointer to a scsi host to release
- *
- * Return value:
- * 	0 on Success / 1 on Failure
+ * scsi_host_cancel - cancel outstanding IO to this host
+ * @shost:	pointer to struct Scsi_Host
+ * recovery:	recovery requested to run.
  **/
-int scsi_remove_host(struct Scsi_Host *shost)
+void scsi_host_cancel(struct Scsi_Host *shost, int recovery)
 {
-	struct scsi_device *sdev;
+	unsigned long flags;
 
-	/*
-	 * FIXME Do ref counting.  We force all of the devices offline to
-	 * help prevent race conditions where other hosts/processors could
-	 * try and get in and queue a command.
-	 */
-	list_for_each_entry(sdev, &shost->my_devices, siblings)
-		sdev->online = FALSE;
+	spin_lock_irqsave(shost->host_lock, flags);
+	__set_bit(SHOST_CANCEL, &shost->host_flags);
+	spin_unlock_irqrestore(shost->host_lock, flags);
+	device_for_each_child(&shost->host_gendev, &recovery,
+			      scsi_device_cancel);
+	wait_event(shost->host_wait, (!test_bit(SHOST_RECOVERY,
+						&shost->host_flags)));
+}
 
+/**
+ * scsi_remove_host - remove a scsi host
+ * @shost:	a pointer to a scsi host to remove
+ **/
+void scsi_remove_host(struct Scsi_Host *shost)
+{
+	scsi_host_cancel(shost, 0);
 	scsi_proc_host_rm(shost);
 	scsi_forget_host(shost);
 	scsi_sysfs_remove_host(shost);
-
-	return 0;
 }
 
 /**
@@ -93,7 +98,7 @@ int scsi_add_host(struct Scsi_Host *shos
 		scsi_proc_host_add(shost);
 		scsi_scan_host(shost);
 	}
-			
+
 	return error;
 }
 
@@ -259,21 +264,21 @@ struct Scsi_Host *scsi_host_lookup(unsig
 {
 	struct class *class = class_get(&shost_class);
 	struct class_device *cdev;
-	struct Scsi_Host *shost = NULL, *p;
+	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) {
-				scsi_host_get(p);
-				shost = p;
+				shost = scsi_host_get(p);
 				break;
 			}
 		}
 		up_read(&class->subsys.rwsem);
 	}
 
+	class_put(&shost_class);
 	return shost;
 }
 
@@ -281,10 +286,20 @@ struct Scsi_Host *scsi_host_lookup(unsig
  * *scsi_host_get - inc a Scsi_Host ref count
  * @shost:	Pointer to Scsi_Host to inc.
  **/
-void scsi_host_get(struct Scsi_Host *shost)
+struct Scsi_Host * scsi_host_get(struct Scsi_Host *shost)
 {
-	get_device(&shost->host_gendev);
-	class_device_get(&shost->class_dev);
+	struct Scsi_Host *res_shost = shost;
+
+	if (res_shost) {
+		if (!test_bit(SHOST_DEL, &shost->host_flags)) {
+			if (!get_device(&res_shost->host_gendev))
+				res_shost = ERR_PTR(-ENXIO);
+		} else {
+			res_shost = ERR_PTR(-ENXIO);
+		}
+	}
+
+	return res_shost;
 }
 
 /**
@@ -293,6 +308,5 @@ void scsi_host_get(struct Scsi_Host *sho
  **/
 void scsi_host_put(struct Scsi_Host *shost)
 {
-	class_device_put(&shost->class_dev);
 	put_device(&shost->host_gendev);
 }
diff -puN drivers/scsi/scsi.c~shost_state drivers/scsi/scsi.c
--- remove-scsi-misc-2.5/drivers/scsi/scsi.c~shost_state	Tue Jul  8 14:19:52 2003
+++ remove-scsi-misc-2.5-andmike/drivers/scsi/scsi.c	Tue Jul  8 14:19:52 2003
@@ -370,7 +370,7 @@ int scsi_dispatch_cmd(struct scsi_cmnd *
 	struct Scsi_Host *host = cmd->device->host;
 	unsigned long flags = 0;
 	unsigned long timeout;
-	int rtn = 1;
+	int rtn = 0;
 
 	/* Assign a unique nonzero serial_number. */
 	/* XXX(hch): this is racy */
@@ -444,7 +444,12 @@ int scsi_dispatch_cmd(struct scsi_cmnd *
 				   host->hostt->queuecommand));
 
 	spin_lock_irqsave(host->host_lock, flags);
-	rtn = host->hostt->queuecommand(cmd, scsi_done);
+	if (unlikely(test_bit(SHOST_CANCEL, &host->host_flags))) {
+		cmd->result = (DID_NO_CONNECT << 16);
+		scsi_done(cmd);
+	} else {
+		rtn = host->hostt->queuecommand(cmd, scsi_done);
+	}
 	spin_unlock_irqrestore(host->host_lock, flags);
 	if (rtn) {
 		scsi_queue_insert(cmd,
@@ -902,17 +907,19 @@ void scsi_device_put(struct scsi_device 
 }
 
 /**
- * scsi_set_device_offline - set scsi_device offline
- * @sdev:	pointer to struct scsi_device to offline. 
+ * scsi_device_cancel - cancel outstanding IO to this device
+ * @sdev:	pointer to struct scsi_device
+ * @data:	pointer to cancel value.
  *
- * Locks:	host_lock held on entry.
  **/
-void scsi_set_device_offline(struct scsi_device *sdev)
+int scsi_device_cancel(struct device *dev, void *data)
 {
 	struct scsi_cmnd *scmd;
 	LIST_HEAD(active_list);
 	struct list_head *lh, *lh_sf;
 	unsigned long flags;
+	struct scsi_device *sdev = to_scsi_device(dev);
+	unsigned int recovery = *(unsigned int *)data;
 
 	sdev->online = 0;
 
@@ -934,11 +941,17 @@ void scsi_set_device_offline(struct scsi
 	if (!list_empty(&active_list)) {
 		list_for_each_safe(lh, lh_sf, &active_list) {
 			scmd = list_entry(lh, struct scsi_cmnd, eh_entry);
-			scsi_eh_scmd_add(scmd, SCSI_EH_CANCEL_CMD);
+			list_del_init(lh);
+			if (recovery) {
+				scsi_eh_scmd_add(scmd, SCSI_EH_CANCEL_CMD);
+			} else {
+				scmd->result = (DID_ABORT << 16);
+				scsi_finish_command(scmd);
+			}
 		}
-	} else {
-		/* FIXME: Send online state change hotplug event */
 	}
+
+	return 0;
 }
 
 MODULE_DESCRIPTION("SCSI core");
diff -puN drivers/scsi/scsi_debug.c~shost_state drivers/scsi/scsi_debug.c
--- remove-scsi-misc-2.5/drivers/scsi/scsi_debug.c~shost_state	Tue Jul  8 14:19:52 2003
+++ remove-scsi-misc-2.5-andmike/drivers/scsi/scsi_debug.c	Tue Jul  8 14:19:52 2003
@@ -1721,10 +1721,7 @@ static int sdebug_driver_remove(struct d
 		return -ENODEV;
 	}
 
-        if (scsi_remove_host(sdbg_host->shost)) {
-                printk(KERN_ERR "%s: scsi_remove_host failed\n", __FUNCTION__);
-                return -EBUSY;
-        }
+        scsi_remove_host(sdbg_host->shost);
 
         list_for_each_safe(lh, lh_sf, &sdbg_host->dev_info_list) {
                 sdbg_devinfo = list_entry(lh, struct sdebug_dev_info,
diff -puN drivers/scsi/scsi_error.c~shost_state drivers/scsi/scsi_error.c
--- remove-scsi-misc-2.5/drivers/scsi/scsi_error.c~shost_state	Tue Jul  8 14:19:52 2003
+++ remove-scsi-misc-2.5-andmike/drivers/scsi/scsi_error.c	Tue Jul  8 14:19:52 2003
@@ -84,7 +84,7 @@ int scsi_eh_scmd_add(struct scsi_cmnd *s
 	 */
 	scmd->serial_number_at_timeout = scmd->serial_number;
 	list_add_tail(&scmd->eh_entry, &shost->eh_cmd_q);
-	shost->in_recovery = 1;
+	set_bit(SHOST_RECOVERY, &shost->host_flags);
 	shost->host_failed++;
 	scsi_eh_wakeup(shost);
 	spin_unlock_irqrestore(shost->host_lock, flags);
@@ -187,7 +187,7 @@ void scsi_times_out(struct scsi_cmnd *sc
  **/
 int scsi_block_when_processing_errors(struct scsi_device *sdev)
 {
-	wait_event(sdev->host->host_wait, (sdev->host->in_recovery == 0));
+	wait_event(sdev->host->host_wait, (!test_bit(SHOST_RECOVERY, &sdev->host->host_flags)));
 
 	SCSI_LOG_ERROR_RECOVERY(5, printk("%s: rtn: %d\n", __FUNCTION__,
 					  sdev->online));
@@ -1389,7 +1389,7 @@ static void scsi_restart_operations(stru
 	SCSI_LOG_ERROR_RECOVERY(3, printk("%s: waking up host to restart\n",
 					  __FUNCTION__));
 
-	shost->in_recovery = 0;
+	clear_bit(SHOST_RECOVERY, &shost->host_flags);
 
 	wake_up(&shost->host_wait);
 
@@ -1599,7 +1599,6 @@ void scsi_error_handler(void *data)
 	 * that's fine.  If the user sent a signal to this thing, we are
 	 * potentially in real danger.
 	 */
-	shost->in_recovery = 0;
 	shost->eh_active = 0;
 	shost->ehandler = NULL;
 
diff -puN drivers/scsi/scsi_lib.c~shost_state drivers/scsi/scsi_lib.c
--- remove-scsi-misc-2.5/drivers/scsi/scsi_lib.c~shost_state	Tue Jul  8 14:19:52 2003
+++ remove-scsi-misc-2.5-andmike/drivers/scsi/scsi_lib.c	Tue Jul  8 14:19:52 2003
@@ -318,7 +318,8 @@ void scsi_device_unbusy(struct scsi_devi
 
 	spin_lock_irqsave(shost->host_lock, flags);
 	shost->host_busy--;
-	if (unlikely(shost->in_recovery && shost->host_failed))
+	if (unlikely(test_bit(SHOST_RECOVERY, &shost->host_flags) &&
+		     shost->host_failed))
 		scsi_eh_wakeup(shost);
 	spin_unlock(shost->host_lock);
 	spin_lock(&sdev->sdev_lock);
@@ -1080,7 +1081,7 @@ static inline int scsi_host_queue_ready(
 				   struct Scsi_Host *shost,
 				   struct scsi_device *sdev)
 {
-	if (shost->in_recovery)
+	if (test_bit(SHOST_RECOVERY, &shost->host_flags))
 		return 0;
 	if (shost->host_busy == 0 && shost->host_blocked) {
 		/*
diff -puN drivers/scsi/scsi_proc.c~shost_state drivers/scsi/scsi_proc.c
--- remove-scsi-misc-2.5/drivers/scsi/scsi_proc.c~shost_state	Tue Jul  8 14:19:52 2003
+++ remove-scsi-misc-2.5-andmike/drivers/scsi/scsi_proc.c	Tue Jul  8 14:19:52 2003
@@ -190,11 +190,11 @@ static int scsi_add_single_device(uint h
 {
 	struct Scsi_Host *shost;
 	struct scsi_device *sdev;
-	int error = -ENODEV;
+	int error = -ENXIO;
 
 	shost = scsi_host_lookup(host);
-	if (!shost)
-		return -ENODEV;
+	if (IS_ERR(shost))
+		return PTR_ERR(shost);
 
 	if (!scsi_find_device(shost, channel, id, lun)) {
 		sdev = scsi_add_device(shost, channel, id, lun);
@@ -212,11 +212,11 @@ static int scsi_remove_single_device(uin
 {
 	struct scsi_device *sdev;
 	struct Scsi_Host *shost;
-	int error = -ENODEV;
+	int error = -ENXIO;
 
 	shost = scsi_host_lookup(host);
-	if (!shost)
-		return -ENODEV;
+	if (IS_ERR(shost))
+		return PTR_ERR(shost);
 	sdev = scsi_find_device(shost, channel, id, lun);
 	if (!sdev)
 		goto out;
diff -puN drivers/scsi/scsi_syms.c~shost_state drivers/scsi/scsi_syms.c
--- remove-scsi-misc-2.5/drivers/scsi/scsi_syms.c~shost_state	Tue Jul  8 14:19:52 2003
+++ remove-scsi-misc-2.5-andmike/drivers/scsi/scsi_syms.c	Tue Jul  8 14:19:52 2003
@@ -85,7 +85,7 @@ EXPORT_SYMBOL(scsi_device_get);
 EXPORT_SYMBOL(scsi_device_put);
 EXPORT_SYMBOL(scsi_add_device);
 EXPORT_SYMBOL(scsi_remove_device);
-EXPORT_SYMBOL(scsi_set_device_offline);
+EXPORT_SYMBOL(scsi_device_cancel);
 
 EXPORT_SYMBOL(__scsi_mode_sense);
 EXPORT_SYMBOL(scsi_mode_sense);
diff -puN drivers/scsi/scsi_sysfs.c~shost_state drivers/scsi/scsi_sysfs.c
--- remove-scsi-misc-2.5/drivers/scsi/scsi_sysfs.c~shost_state	Tue Jul  8 14:19:52 2003
+++ remove-scsi-misc-2.5-andmike/drivers/scsi/scsi_sysfs.c	Tue Jul  8 14:19:52 2003
@@ -44,6 +44,7 @@ shost_rd_attr(host_busy, "%hu\n");
 shost_rd_attr(cmd_per_lun, "%hd\n");
 shost_rd_attr(sg_tablesize, "%hu\n");
 shost_rd_attr(unchecked_isa_dma, "%d\n");
+shost_rd_attr(host_flags, "%lx\n");
 
 struct class_device_attribute *scsi_sysfs_shost_attrs[] = {
 	&class_device_attr_unique_id,
@@ -51,11 +52,30 @@ struct class_device_attribute *scsi_sysf
 	&class_device_attr_cmd_per_lun,
 	&class_device_attr_sg_tablesize,
 	&class_device_attr_unchecked_isa_dma,
+	&class_device_attr_host_flags,
 	NULL
 };
 
+static void scsi_host_cls_release(struct class_device *class_dev)
+{
+	struct Scsi_Host *shost;
+
+	shost = class_to_shost(class_dev);
+	put_device(&shost->host_gendev);
+}
+
+static void scsi_host_dev_release(struct device *dev)
+{
+	struct Scsi_Host *shost;
+
+	device_del(dev);
+	shost = dev_to_shost(dev);
+	scsi_free_shost(shost);
+}
+
 struct class shost_class = {
 	.name		= "scsi_host",
+	.release	= scsi_host_cls_release,
 };
 
 static struct class sdev_class = {
@@ -302,16 +322,6 @@ int scsi_register_interface(struct class
 	return class_interface_register(intf);
 }
 
-static void scsi_host_release(struct device *dev)
-{
-	struct Scsi_Host *shost;
-
-	shost = dev_to_shost(dev);
-	if (!shost)
-		return;
-
-	scsi_free_shost(shost);
-}
 
 void scsi_sysfs_init_host(struct Scsi_Host *shost)
 {
@@ -320,13 +330,14 @@ void scsi_sysfs_init_host(struct Scsi_Ho
 		shost->host_no);
 	snprintf(shost->host_gendev.name, DEVICE_NAME_SIZE, "%s",
 		shost->hostt->proc_name);
-	shost->host_gendev.release = scsi_host_release;
+	shost->host_gendev.release = scsi_host_dev_release;
 
 	class_device_initialize(&shost->class_dev);
 	shost->class_dev.dev = &shost->host_gendev;
 	shost->class_dev.class = &shost_class;
 	snprintf(shost->class_dev.class_id, BUS_ID_SIZE, "host%d",
 		  shost->host_no);
+	get_device(&shost->host_gendev);
 }
 
 /**
@@ -355,6 +366,8 @@ int scsi_sysfs_add_host(struct Scsi_Host
 	if (error)
 		goto clean_class;
 
+	set_bit(SHOST_ADD, &shost->host_flags);
+
 	return error;
 
 clean_class:
@@ -371,8 +384,13 @@ clean_device:
  **/
 void scsi_sysfs_remove_host(struct Scsi_Host *shost)
 {
-	class_device_del(&shost->class_dev);
-	device_del(&shost->host_gendev);
+	unsigned long flags;
+
+	spin_lock_irqsave(shost->host_lock, flags);
+	__set_bit(SHOST_DEL, &shost->host_flags);
+	spin_unlock_irqrestore(shost->host_lock, flags);
+
+	class_device_unregister(&shost->class_dev);
 }
 
 /** scsi_sysfs_modify_shost_attribute - modify or add a host class attribute
diff -puN drivers/scsi/sg.c~shost_state drivers/scsi/sg.c
--- remove-scsi-misc-2.5/drivers/scsi/sg.c~shost_state	Tue Jul  8 14:19:52 2003
+++ remove-scsi-misc-2.5-andmike/drivers/scsi/sg.c	Tue Jul  8 14:19:52 2003
@@ -954,7 +954,8 @@ sg_ioctl(struct inode *inode, struct fil
 		if (sdp->detached)
 			return -ENODEV;
 		if (filp->f_flags & O_NONBLOCK) {
-			if (sdp->device->host->in_recovery)
+			if (test_bit(SHOST_RECOVERY,
+				     &sdp->device->host->host_flags))
 				return -EBUSY;
 		} else if (!scsi_block_when_processing_errors(sdp->device))
 			return -EBUSY;
diff -puN include/scsi/scsi_device.h~shost_state include/scsi/scsi_device.h
--- remove-scsi-misc-2.5/include/scsi/scsi_device.h~shost_state	Tue Jul  8 14:19:52 2003
+++ remove-scsi-misc-2.5-andmike/include/scsi/scsi_device.h	Tue Jul  8 14:19:52 2003
@@ -94,7 +94,7 @@ struct scsi_device {
 extern struct scsi_device *scsi_add_device(struct Scsi_Host *,
 		uint, uint, uint);
 extern int scsi_remove_device(struct scsi_device *);
-extern void scsi_set_device_offline(struct scsi_device *);
+extern int scsi_device_cancel(struct device *, void *);
 
 extern int scsi_device_get(struct scsi_device *);
 extern void scsi_device_put(struct scsi_device *);
diff -puN include/scsi/scsi_host.h~shost_state include/scsi/scsi_host.h
--- remove-scsi-misc-2.5/include/scsi/scsi_host.h~shost_state	Tue Jul  8 14:19:52 2003
+++ remove-scsi-misc-2.5-andmike/include/scsi/scsi_host.h	Tue Jul  8 14:19:52 2003
@@ -348,6 +348,14 @@ struct scsi_host_template {
 	struct list_head legacy_hosts;
 };
 
+/*
+ * shost flags
+ */
+#define SHOST_ADD	1
+#define SHOST_DEL	2
+#define SHOST_CANCEL	3
+#define SHOST_RECOVERY	4
+
 struct Scsi_Host {
 	struct list_head	my_devices;
 	struct scsi_host_cmd_pool *cmd_pool;
@@ -413,7 +421,6 @@ struct Scsi_Host {
 	short unsigned int sg_tablesize;
 	short unsigned int max_sectors;
 
-	unsigned in_recovery:1;
 	unsigned unchecked_isa_dma:1;
 	unsigned use_clustering:1;
 	unsigned highmem_io:1;
@@ -454,6 +461,9 @@ struct Scsi_Host {
 	unsigned char n_io_port;
 	unsigned char dma_channel;
 	unsigned int  irq;
+	
+
+	unsigned long host_flags;
 
 
 	/*
@@ -480,8 +490,8 @@ struct Scsi_Host {
 
 extern struct Scsi_Host *scsi_host_alloc(struct scsi_host_template *, int);
 extern int scsi_add_host(struct Scsi_Host *, struct device *);
-extern int scsi_remove_host(struct Scsi_Host *);
-extern void scsi_host_get(struct Scsi_Host *);
+extern void scsi_remove_host(struct Scsi_Host *);
+extern struct Scsi_Host *scsi_host_get(struct Scsi_Host *);
 extern void scsi_host_put(struct Scsi_Host *t);
 extern struct Scsi_Host *scsi_host_lookup(unsigned short);
 
diff -puN drivers/usb/storage/usb.c~shost_state drivers/usb/storage/usb.c
--- remove-scsi-misc-2.5/drivers/usb/storage/usb.c~shost_state	Tue Jul  8 14:19:52 2003
+++ remove-scsi-misc-2.5-andmike/drivers/usb/storage/usb.c	Tue Jul  8 14:19:52 2003
@@ -993,12 +993,7 @@ static void storage_disconnect(struct us
 	/* Dissociate from the USB device */
 	dissociate_dev(us);
 
-	/* Begin the SCSI host removal sequence */
-	if (scsi_remove_host(us->host)) {
-		US_DEBUGP("-- SCSI refused to remove the host\n");
-		BUG();
-		return;
-	}
+	scsi_remove_host(us->host);
 
 	/* TODO: somehow, wait for the device to
 	 * be 'idle' (tasklet completion) */

_

[PATCH] scsi host/scsi device ref count cleanup 4/4

[Mike Anderson]

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

DESC
This is a cleanup patch for scsi_device removal.
	- Addition of device_flags member to the scsi_device strucuture to
	  contain "state" of the device instance.
	- Added SDEV_ADD, SDEV_DEL, SDEV_CANCEL, SDEV_RECOVERY states for
	  a scsi device
	- Built on previous patch that renamed scsi_set_device_offline to
	  scsi_device_cancel to use SDEV_CANCEL value.
	- Usage of the device_flags flags member to stop scsi_device_get's
	  and final cleanup on access_count going to zero.
	- Added newer release functions and reordered scsi device sysfs
	  unregister.
EDESC


 drivers/scsi/scsi.c        |   36 ++++++++++++++++++++++-----
 drivers/scsi/scsi_priv.h   |    1 
 drivers/scsi/scsi_sysfs.c  |   60 ++++++++++++++++++++++++++++++---------------
 include/scsi/scsi_device.h |   13 +++++++++
 4 files changed, 84 insertions(+), 26 deletions(-)

diff -puN drivers/scsi/scsi.c~sdev_state drivers/scsi/scsi.c
--- remove-scsi-misc-2.5/drivers/scsi/scsi.c~sdev_state	Tue Jul  8 14:19:55 2003
+++ remove-scsi-misc-2.5-andmike/drivers/scsi/scsi.c	Tue Jul  8 14:19:55 2003
@@ -893,17 +893,41 @@ int scsi_track_queue_full(struct scsi_de
 
 int scsi_device_get(struct scsi_device *sdev)
 {
-	if (!try_module_get(sdev->host->hostt->module))
-		return -ENXIO;
+	struct class *class = class_get(&sdev_class);
+	int error = -ENXIO;
 
-	sdev->access_count++;
-	return 0;
+	if (class) {
+		down_write(&class->subsys.rwsem);
+		if (!test_bit(SDEV_DEL, &sdev->device_flags))
+			if (try_module_get(sdev->host->hostt->module)) 
+				if (get_device(&sdev->sdev_driverfs_dev)) {
+					sdev->access_count++;
+					error = 0;
+				}
+		up_write(&class->subsys.rwsem);
+		class_put(&sdev_class);
+	}
+
+	return error;
 }
 
 void scsi_device_put(struct scsi_device *sdev)
 {
-	sdev->access_count--;
+	struct class *class = class_get(&sdev_class);
+
+	if (!class)
+		return;
+
+	down_write(&class->subsys.rwsem);
 	module_put(sdev->host->hostt->module);
+	if (--sdev->access_count == 0) {
+		if (test_bit(SDEV_DEL, &sdev->device_flags))
+			device_del(&sdev->sdev_driverfs_dev);
+	}
+	put_device(&sdev->sdev_driverfs_dev);
+	up_write(&class->subsys.rwsem);
+
+	class_put(&sdev_class);
 }
 
 /**
@@ -921,7 +945,7 @@ int scsi_device_cancel(struct device *de
 	struct scsi_device *sdev = to_scsi_device(dev);
 	unsigned int recovery = *(unsigned int *)data;
 
-	sdev->online = 0;
+	set_bit(SDEV_CANCEL, &sdev->device_flags);
 
 	spin_lock_irqsave(&sdev->list_lock, flags);
 	list_for_each_entry(scmd, &sdev->cmd_list, list) {
diff -puN drivers/scsi/scsi_sysfs.c~sdev_state drivers/scsi/scsi_sysfs.c
--- remove-scsi-misc-2.5/drivers/scsi/scsi_sysfs.c~sdev_state	Tue Jul  8 14:19:55 2003
+++ remove-scsi-misc-2.5-andmike/drivers/scsi/scsi_sysfs.c	Tue Jul  8 14:19:55 2003
@@ -78,8 +78,25 @@ struct class shost_class = {
 	.release	= scsi_host_cls_release,
 };
 
-static struct class sdev_class = {
+static void scsi_device_cls_release(struct class_device *class_dev)
+{
+	struct scsi_device *sdev;
+
+	sdev = class_to_sdev(class_dev);
+	put_device(&sdev->sdev_driverfs_dev);
+}
+
+static void scsi_device_dev_release(struct device *dev)
+{
+	struct scsi_device *sdev;
+
+	sdev = to_scsi_device(dev);
+	scsi_free_sdev(sdev);
+}
+
+struct class sdev_class = {
 	.name		= "scsi_device",
+	.release	= scsi_device_cls_release,
 };
 
 /* all probing is done in the individual ->probe routines */
@@ -129,7 +146,7 @@ void scsi_sysfs_unregister(void)
  */
 #define sdev_show_function(field, format_string)				\
 static ssize_t								\
-show_##field (struct device *dev, char *buf)				\
+sdev_show_##field (struct device *dev, char *buf)				\
 {									\
 	struct scsi_device *sdev;					\
 	sdev = to_scsi_device(dev);					\
@@ -142,7 +159,7 @@ show_##field (struct device *dev, char *
  */
 #define sdev_rd_attr(field, format_string)				\
 	sdev_show_function(field, format_string)				\
-static DEVICE_ATTR(field, S_IRUGO, show_##field, NULL)
+static DEVICE_ATTR(field, S_IRUGO, sdev_show_##field, NULL)
 
 
 /*
@@ -160,7 +177,7 @@ sdev_store_##field (struct device *dev, 
 	snscanf (buf, 20, format_string, &sdev->field);			\
 	return count;							\
 }									\
-static DEVICE_ATTR(field, S_IRUGO | S_IWUSR, show_##field, sdev_store_##field)
+static DEVICE_ATTR(field, S_IRUGO | S_IWUSR, sdev_show_##field, sdev_store_##field)
 
 /*
  * sdev_rd_attr: create a function and attribute variable for a
@@ -182,7 +199,7 @@ sdev_store_##field (struct device *dev, 
 	}								\
 	return ret;							\
 }									\
-static DEVICE_ATTR(field, S_IRUGO | S_IWUSR, show_##field, sdev_store_##field)
+static DEVICE_ATTR(field, S_IRUGO | S_IWUSR, sdev_show_##field, sdev_store_##field)
 
 /*
  * scsi_sdev_check_buf_bit: return 0 if buf is "0", return 1 if buf is "1",
@@ -212,6 +229,7 @@ sdev_rd_attr (access_count, "%d\n");
 sdev_rd_attr (vendor, "%.8s\n");
 sdev_rd_attr (model, "%.16s\n");
 sdev_rd_attr (rev, "%.4s\n");
+sdev_rd_attr (device_flags, "%lx\n");
 sdev_rw_attr_bit (online);
 
 static ssize_t
@@ -233,20 +251,12 @@ struct device_attribute *scsi_sysfs_sdev
 	&dev_attr_vendor,
 	&dev_attr_model,
 	&dev_attr_rev,
+	&dev_attr_device_flags,
 	&dev_attr_online,
 	&dev_attr_rescan,
 	NULL
 };
 
-static void scsi_device_release(struct device *dev)
-{
-	struct scsi_device *sdev;
-
-	sdev = to_scsi_device(dev);
-	if (!sdev)
-		return;
-	scsi_free_sdev(sdev);
-}
 
 /**
  * scsi_device_register - register a scsi device with the scsi bus
@@ -259,12 +269,13 @@ int scsi_device_register(struct scsi_dev
 {
 	int error = 0, i;
 
+	__set_bit(SDEV_ADD, &sdev->device_flags);
 	device_initialize(&sdev->sdev_driverfs_dev);
 	sprintf(sdev->sdev_driverfs_dev.bus_id,"%d:%d:%d:%d",
 		sdev->host->host_no, sdev->channel, sdev->id, sdev->lun);
 	sdev->sdev_driverfs_dev.parent = &sdev->host->host_gendev;
 	sdev->sdev_driverfs_dev.bus = &scsi_bus_type;
-	sdev->sdev_driverfs_dev.release = scsi_device_release;
+	sdev->sdev_driverfs_dev.release = scsi_device_dev_release;
 
 	class_device_initialize(&sdev->sdev_classdev);
 	sdev->sdev_classdev.dev = &sdev->sdev_driverfs_dev;
@@ -284,6 +295,8 @@ int scsi_device_register(struct scsi_dev
 		return error;
 	}
 
+	get_device(&sdev->sdev_driverfs_dev);
+
 	for (i = 0; !error && sdev->host->hostt->sdev_attrs[i] != NULL; i++)
 		error = device_create_file(&sdev->sdev_driverfs_dev,
 					   sdev->host->hostt->sdev_attrs[i]);
@@ -300,12 +313,21 @@ int scsi_device_register(struct scsi_dev
  **/
 void scsi_device_unregister(struct scsi_device *sdev)
 {
-	int i;
+	struct class *class = class_get(&sdev_class);
 
-	for (i = 0; sdev->host->hostt->sdev_attrs[i] != NULL; i++)
-		device_remove_file(&sdev->sdev_driverfs_dev, sdev->host->hostt->sdev_attrs[i]);
+	if (class) {
+		down_write(&class->subsys.rwsem);
+		__set_bit(SDEV_DEL, &sdev->device_flags);
+		if (sdev->access_count == 0)
+			device_del(&sdev->sdev_driverfs_dev);
+		up_write(&class->subsys.rwsem);
+	}
+	
 	class_device_unregister(&sdev->sdev_classdev);
-	device_unregister(&sdev->sdev_driverfs_dev);
+
+	put_device(&sdev->sdev_driverfs_dev);
+
+	class_put(&sdev_class);
 }
 
 int scsi_register_driver(struct device_driver *drv)
diff -puN include/scsi/scsi_device.h~sdev_state include/scsi/scsi_device.h
--- remove-scsi-misc-2.5/include/scsi/scsi_device.h~sdev_state	Tue Jul  8 14:19:55 2003
+++ remove-scsi-misc-2.5-andmike/include/scsi/scsi_device.h	Tue Jul  8 14:19:55 2003
@@ -10,6 +10,14 @@ struct scsi_cmnd;
 struct scsi_mode_data;
 
 
+/*
+ * sdev flags
+ */
+#define SDEV_ADD	1
+#define SDEV_DEL	2
+#define SDEV_CANCEL 	3
+#define SDEV_RECOVERY 	4
+
 struct scsi_device {
 	struct class_device	sdev_classdev;
 
@@ -87,9 +95,13 @@ struct scsi_device {
 #define SCSI_DEFAULT_DEVICE_BLOCKED	3
 
 	struct device sdev_driverfs_dev;
+
+	unsigned long device_flags;
 };
 #define	to_scsi_device(d)	\
 	container_of(d, struct scsi_device, sdev_driverfs_dev)
+#define	class_to_sdev(d)	\
+	container_of(d, struct scsi_device, sdev_classdev)
 
 extern struct scsi_device *scsi_add_device(struct Scsi_Host *,
 		uint, uint, uint);
@@ -107,5 +119,4 @@ extern int scsi_set_medium_removal(struc
 extern int scsi_mode_sense(struct scsi_device *sdev, int dbd, int modepage,
 			   unsigned char *buffer, int len, int timeout,
 			   int retries, struct scsi_mode_data *data);
-
 #endif /* _SCSI_SCSI_DEVICE_H */
diff -puN drivers/scsi/scsi_priv.h~sdev_state drivers/scsi/scsi_priv.h
--- remove-scsi-misc-2.5/drivers/scsi/scsi_priv.h~sdev_state	Tue Jul  8 14:19:55 2003
+++ remove-scsi-misc-2.5-andmike/drivers/scsi/scsi_priv.h	Tue Jul  8 14:19:55 2003
@@ -127,6 +127,7 @@ extern struct class_device_attribute *sc
 extern struct device_attribute *scsi_sysfs_sdev_attrs[];
 
 extern struct class shost_class;
+extern struct class sdev_class;
 extern struct bus_type scsi_bus_type;
 
 #endif /* _SCSI_PRIV_H */

_

Re: [PATCH] scsi host/scsi device ref count cleanup 2/4

[Christoph Hellwig]

On Tue, Jul 08, 2003 at 03:26:22PM -0700, Mike Anderson wrote:
> aic79xx_osm and aic7xxx_osm are 2.4 code. ioctl_probe has side effect of
> needing present value.

ioctl_probe is pretty buggy anyways.  I wonder whether we should just kill it.

Re: [PATCH] scsi host/scsi device ref count cleanup 3/4

[Christoph Hellwig]

On Tue, Jul 08, 2003 at 03:27:07PM -0700, Mike Anderson wrote:
> -andmike
> --
> Michael Anderson
> andmike@us.ibm.com
> 
> DESC
> This is a cleanup patch for scsi_host removal.
> 	- Addition of a host_flags member to the scsi_host strucuture to
> 	  contain "state" of the host instance.
> 	- Added SHOST_ADD, SHOST_DEL, SHOST_CANCEL, SHOST_RECOVERY states
> 	  for a scsi host

Hmm, I'd call this shost_state.


> +	spin_lock_irqsave(shost->host_lock, flags);
> +	__set_bit(SHOST_CANCEL, &shost->host_flags);
> +	spin_unlock_irqrestore(shost->host_lock, flags);
> +	device_for_each_child(&shost->host_gendev, &recovery,
> +			      scsi_device_cancel);
> +	wait_event(shost->host_wait, (!test_bit(SHOST_RECOVERY,
> +						&shost->host_flags)));

Using the non-atomic and the atomic bitops on one variable doesn't
get you atomicity.  Always use the atomic ones if you need them sometimes.

> +/**
> + * scsi_remove_host - remove a scsi host
> + * @shost:	a pointer to a scsi host to remove
> + **/
> +void scsi_remove_host(struct Scsi_Host *shost)

A, cool I already had a patch pending to make this a void retval.

> -void scsi_host_get(struct Scsi_Host *shost)
> +struct Scsi_Host * scsi_host_get(struct Scsi_Host *shost)

struct Scsi_Host *scsi_host_get(struct Scsi_Host *shost)

>  {
> -	get_device(&shost->host_gendev);
> -	class_device_get(&shost->class_dev);
> +	struct Scsi_Host *res_shost = shost;
> +
> +	if (res_shost) {

I don't think we should allow for a NULL argument here.  Also
can't we just return NULL in the error case?  That would
simplify the code to:

struct Scsi_Host *scsi_host_get(struct Scsi_Host *shost)
{
	if (test_bit(SHOST_DEL, &shost->host_flags) ||
	    !get_device(&res_shost->host_gendev))
		return NULL;
	return shost;
}

>  void scsi_host_put(struct Scsi_Host *shost)
>  {
> -	class_device_put(&shost->class_dev);

Can you explain why we don't need that anymore?  /me wants to learn a bit
more about deep LDM magic..

> -void scsi_set_device_offline(struct scsi_device *sdev)
> +int scsi_device_cancel(struct device *dev, void *data)
>  {
>  	struct scsi_cmnd *scmd;
>  	LIST_HEAD(active_list);
>  	struct list_head *lh, *lh_sf;
>  	unsigned long flags;
> +	struct scsi_device *sdev = to_scsi_device(dev);
> +	unsigned int recovery = *(unsigned int *)data;

I think the prototype should be

int scsi_device_cancel(struct scsi_device *sdev, int recovery)

and there should be a small wrapper for the LDM iterator.

> +shost_rd_attr(host_flags, "%lx\n");

Shouldn't these better be printed in ascii instead of the binary
presentation that might aswell change form one kernelrelease to
another?

> +/*
> + * shost flags
> + */
> +#define SHOST_ADD	1
> +#define SHOST_DEL	2
> +#define SHOST_CANCEL	3
> +#define SHOST_RECOVERY	4

Make this an enum?

>  	/* Dissociate from the USB device */
>  	dissociate_dev(us);
>  
> -	/* Begin the SCSI host removal sequence */
> -	if (scsi_remove_host(us->host)) {
> -		US_DEBUGP("-- SCSI refused to remove the host\n");
> -		BUG();
> -		return;
> -	}
> +	scsi_remove_host(us->host);

Also in usb-storage the loop over my_devices to set them offline
just before scsi_remove_host should be nuked now, shouldn't it?

(unknown)

[Christoph Hellwig]

> +	__set_bit(SDEV_ADD, &sdev->device_flags);

Same atomic vs non atomic issue as in the previous patch.

> +/*
> + * sdev flags
> + */
> +#define SDEV_ADD	1
> +#define SDEV_DEL	2
> +#define SDEV_CANCEL 	3
> +#define SDEV_RECOVERY 	4

Enum again?


> +	unsigned long device_flags;

sdev_state?

Re: [PATCH] scsi host/scsi device ref count cleanup 3/4

[Mike Anderson]

Christoph Hellwig [hch@infradead.org] wrote:
> On Tue, Jul 08, 2003 at 03:27:07PM -0700, Mike Anderson wrote:
> > -andmike
> > --
> > Michael Anderson
> > andmike@us.ibm.com
> > 
> > DESC
> > This is a cleanup patch for scsi_host removal.
> > 	- Addition of a host_flags member to the scsi_host strucuture to
> > 	  contain "state" of the host instance.
> > 	- Added SHOST_ADD, SHOST_DEL, SHOST_CANCEL, SHOST_RECOVERY states
> > 	  for a scsi host
> 
> Hmm, I'd call this shost_state.
> 

Done.

> 
> > +	spin_lock_irqsave(shost->host_lock, flags);
> > +	__set_bit(SHOST_CANCEL, &shost->host_flags);
> > +	spin_unlock_irqrestore(shost->host_lock, flags);
> > +	device_for_each_child(&shost->host_gendev, &recovery,
> > +			      scsi_device_cancel);
> > +	wait_event(shost->host_wait, (!test_bit(SHOST_RECOVERY,
> > +						&shost->host_flags)));
> 
> Using the non-atomic and the atomic bitops on one variable doesn't
> get you atomicity.  Always use the atomic ones if you need them sometimes.

Yes, I will clean this up. I went through many different iterations of
state implementations and got sloppy.

> > -void scsi_host_get(struct Scsi_Host *shost)
> > +struct Scsi_Host * scsi_host_get(struct Scsi_Host *shost)
> 
> struct Scsi_Host *scsi_host_get(struct Scsi_Host *shost)
> 
> >  {
> > -	get_device(&shost->host_gendev);
> > -	class_device_get(&shost->class_dev);
> > +	struct Scsi_Host *res_shost = shost;
> > +
> > +	if (res_shost) {
> 
> I don't think we should allow for a NULL argument here.  Also
> can't we just return NULL in the error case?  That would
> simplify the code to:
> 
> struct Scsi_Host *scsi_host_get(struct Scsi_Host *shost)
> {
> 	if (test_bit(SHOST_DEL, &shost->host_flags) ||
> 	    !get_device(&res_shost->host_gendev))
> 		return NULL;
> 	return shost;
> }
> 

Done.

> >  void scsi_host_put(struct Scsi_Host *shost)
> >  {
> > -	class_device_put(&shost->class_dev);
> 
> Can you explain why we don't need that anymore?  /me wants to learn a bit
> more about deep LDM magic..
> 

I do not know if I can reveal the magic as some of this is bread crumbs
created by previous bad paths taken :-).

I think SCSI core has unique issues stuck between the bus layer struct
device LDM registration and the block layer pure kobjects.

I removed the class get/puts to simplify the scsi_host_get code and
reduce ordering problems between the class and device release methods.
We could have chosen either the structure device or the class device to
ref count on. The struct device appears to be a better object to ref
count against as it is the child of the adapter struct device and the
parent of the scsi device struct device.

We also had an issue previously discussed by Alan of no class device
release method which could cause a problem if a user had an open on a
scsi_host class attribute..

So now our scsi host ref counting is like the following:
(Note: scsi_sysfs_remove_host has device_del as the patch I sent out was
wrong in this area and had some old code used to debug a issue with 

scsi_sysfs_init_host:
	(1) device_initialize host_gendev (ref = 1)
	(2) class_device_initialize shost_classdev (ref = 1)
	(3) get_device on host_gendev (ref+1 (2)) (This will keep the
	host_gendev in place until release of the shost_classdev)

scsi_sysfs_add_host
	(1) device_add on host_gendev (ref+1 (3)) and (ref+1) on parent
	(2) get_device on parent to hold them in place post device_del.
	(3) class_device_add on shost_classdev (ref+1 (2))

scsi_sysfs_remove_host
	(1) class_device_unregister on shost_classdev (ref-2) (del
	and put will result in release if no users have opens on host
	class attributes)
	(2) device_del on shost_gendev (ref-1) and (ref-1) on parent

scsi_host_cls_release
	put_device on host_gendev (ref-1) (This will allow the
	host_gendev release to be called)

scsi_host_dev_release
	(1) put_device on parent (ref-1)
	(2) scsi_free_shost

scsi_host_get
	get_device on host_gendev (ref+1)

scsi_host_put
	put_device on shost_gendev (ref-1)

Each scsi_device_register / scsi_device_unregister will incr / dec the
host_gendev ref.
	

> > -void scsi_set_device_offline(struct scsi_device *sdev)
> > +int scsi_device_cancel(struct device *dev, void *data)
> >  {
> >  	struct scsi_cmnd *scmd;
> >  	LIST_HEAD(active_list);
> >  	struct list_head *lh, *lh_sf;
> >  	unsigned long flags;
> > +	struct scsi_device *sdev = to_scsi_device(dev);
> > +	unsigned int recovery = *(unsigned int *)data;
> 
> I think the prototype should be
> 
> int scsi_device_cancel(struct scsi_device *sdev, int recovery)
> 
> and there should be a small wrapper for the LDM iterator.
> 

ok I created a wrapper function called scsi_device_cancel_cb (seems like
others where using callback but that seemed to long).


> > +shost_rd_attr(host_flags, "%lx\n");
> 
> Shouldn't these better be printed in ascii instead of the binary
> presentation that might aswell change form one kernelrelease to
> another?
> 

Yes, I was lazy and did this initially for debug. Do have a suggested
format (i.e., comma separated or colon separated or ??).

> > +/*
> > + * shost flags
> > + */
> > +#define SHOST_ADD	1
> > +#define SHOST_DEL	2
> > +#define SHOST_CANCEL	3
> > +#define SHOST_RECOVERY	4
> 
> Make this an enum?
> 

Done. I guess also since I am using set_bit that I should use the zero
shift (not that we will need all these states).

> >  	/* Dissociate from the USB device */
> >  	dissociate_dev(us);
> >  
> > -	/* Begin the SCSI host removal sequence */
> > -	if (scsi_remove_host(us->host)) {
> > -		US_DEBUGP("-- SCSI refused to remove the host\n");
> > -		BUG();
> > -		return;
> > -	}
> > +	scsi_remove_host(us->host);
> 
> Also in usb-storage the loop over my_devices to set them offline
> just before scsi_remove_host should be nuked now, shouldn't it?

Yes it should. I made this a separate patch in the series
in case Matthew / Alan see other changes needed with usb storage
shutdown.

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

Re: [PATCH] scsi host/scsi device ref count cleanup 3/4

[Christoph Hellwig]

On Thu, Jul 10, 2003 at 07:03:40AM -0700, Mike Anderson wrote:
> > Can you explain why we don't need that anymore?  /me wants to learn a bit
> > more about deep LDM magic..
> > 
> 
> I do not know if I can reveal the magic as some of this is bread crumbs
> created by previous bad paths taken :-).

Hehe.

> I think SCSI core has unique issues stuck between the bus layer struct
> device LDM registration and the block layer pure kobjects.
> 
> I removed the class get/puts to simplify the scsi_host_get code and
> reduce ordering problems between the class and device release methods.
> We could have chosen either the structure device or the class device to
> ref count on. The struct device appears to be a better object to ref
> count against as it is the child of the adapter struct device and the
> parent of the scsi device struct device.

Agreed.  What do we need the classdev for at all?

> ok I created a wrapper function called scsi_device_cancel_cb (seems like
> others where using callback but that seemed to long).

or just name it cancel_callback (I assume it's static)

> > Shouldn't these better be printed in ascii instead of the binary
> > presentation that might aswell change form one kernelrelease to
> > another?
> > 
> 
> Yes, I was lazy and did this initially for debug. Do have a suggested
> format (i.e., comma separated or colon separated or ??).

coma separated looks fine to me.

> 
> Yes it should. I made this a separate patch in the series
> in case Matthew / Alan see other changes needed with usb storage
> shutdown.

Ok.