scsi_host sysfs updates scsi-misc-2.5 [0/2]

PATCH In review Mike Anderson 5 participants 22 messages May 9, 2003 – May 13, 2003 SCSI

Share & tools

Mike Anderson

author

2003-05-09 06:18 UTC #0 lore raw

PATCH 1/1 scsi_host sysfs updates scsi-misc-2.5 [0/2]

This series of patches is an update to the scsi_host sysfs / ref counting patches previously merged into the scsi-misc-2.5 tree.

insmod / rmmod Testing:
Legacy registration:
aic7xxx_old, qlogicisp
qla2xxx v8.00.00b1 - Driver oops system with garbled messages on rmmod. Does not seem related to this patch.

New registration:
scsi_debug
aic7xxx has an issue with holding lock while calling scsi_remove_host. I created a test patch to not hold the lock and the scsi mid layer illegal context messages stopped. Justin will need to address a real fix.

I am booted on the ips driver, but have done no insmod / rmmod testing.

Attached is the current Reference Counting Rules and a small future TODO cleanup list.

Post feedback I will work to roll this into the scsi documentation.

-andmike
--
Michael Anderson
[email protected]

1.) Reference counting for scsi_host
scsi_register (scsi_alloc_host):
(A) kmalloc Scsi_Host + xtr_bytes
(B) Call device_initialize, class_device_initialize refcount is 1 on host_gendev. refcount is 1 on class_dev.
scsi_get_host:
(A) Call get_device on host_gendev
refcount +1 on host_gendev
(B) Call class_device_get on class_dev refcount +1 on class_dev
scsi_host_put:
(A) Call put_device on host_gendev
refcount -1 on host_gendev
(B) Call class_device_put on class_dev refcount -1 on class_dev
scsi_add_host:
(A) Call scsi_sysfs_add_host
(1) Call device_add
refcount +1 on parent struct device host_gendev now visible in sysfs tree.
(2) Call class_device_add
refcount +1 on scsi_host class refcount +1 on parent struct device class_dev now visible in sysfs tree
(C) Call scsi_proc_host_add
Scsi_Host now visible in proc.
(D) Call scsi_scan_host
refcount +1 on host_gendev for each scsi_device discovered
(E) Call scsi_attach_device.

scsi_remove_host:
(A) "scsi_offline_host" see note above.
(B) Call scsi_proc_host_rm
(C) Call scsi_forget_host
refcount -1 on host_gendev for each scsi_device unregistered
(D) Call scsi_sysfs_remove_host
(1) Call class_device_del
(2) Call device_del

scsi_unregister:
(A) Call scsi_put_host

scsi_host_release:
(A) Call scsi_free_shost
(1) Remove Scsi_Host from scsi_host_list list.
(2) Kill error recovery thread.
(3) Call scsi_destroy_command_freelist
(4) kfree(shost)

scsi_register_host:
(A) Call template detect
(1) Call scsi_register for each instance
(B) Call scsi_add_host for each instance

scsi_unregister_host:
(A) Call scsi_remove_host, hostt->release for all Scsi_Host instances.

2.) Reference counting for scsi_device

scsi_add_lun:
(A) Call scsi_device_register
refcount 1 on sdev_driverfs_dev
refcount +1 on parent struct device (i.e host_gendev)

scsi_add_device:
(A) Call scsi_probe_and_add_lun

scsi_remove_device:
(A) Call scsi_detach_device
(B) Call scsi_device_unregister
refcount -1 on sdev_driverfs_dev
refcount -1 on parent struct device (i.e host_gendev)

scsi_device_get:
(A) try_module_get
(B) access_count +1

scsi_device_put:
(A) access_count -1
(B) module_put on host module

TODO:
- scsi_set_host_offline needs work on sync between LLDD and mid on surprise remove.

- Move upper level attaches to driver model so scsi_add_host does not need to loop over my_devices. Post the cleanups that have happen on the upper level driver we really close to matching the model already this should be doable. (Christoph has old patch that needs refreshed to handle this)

- Remove scsi_host_get_next.

- Should move scsi_device device_register to (initialize, add) pair. (Low priority)

- Evaluate the need for scsi_host_list as this is duplicated in the scsi_host class structure. (Low priority)

Mike Anderson

author

2003-05-09 06:19 UTC #1 lore raw

PATCH 1/1 scsi_host sysfs updates scsi-misc-2.5 [1/2]

-andmike
--
Michael Anderson
[email protected]

DESC
scsi_debug cleanups for scsi-misc-2.5
- Remove release function.
- Remove scsi_debug wrapper driver register / unregister functions.
- Douglas's target == this_id fix.
- Remove some old cleanups that where incorrect.
- Move code back into sdebug_driver_remove.
EDESC

 drivers/scsi/scsi_debug.c |   78 ++++++++++++++--------------------------------
 drivers/scsi/scsi_debug.h |    1 
 2 files changed, 25 insertions(+), 54 deletions(-)

drivers/scsi/scsi_debug.c renamed from drivers/scsi/scsi_debug.c~scsi_debug-misc-fix +25 ~~-53~~

diff -puN drivers/scsi/scsi_debug.c~scsi_debug-misc-fix drivers/scsi/scsi_debug.c
@@ -55,7 +55,7 @@
 #include "scsi_logging.h"
 #include "scsi_debug.h"
 
-static const char * scsi_debug_version_str = "Version: 1.70 (20030416)";
+static const char * scsi_debug_version_str = "Version: 1.70 (20030507)";
 
 /* Additional Sense Code (ASC) used */
 #define NO_ADDED_SENSE 0x0

@@ -169,7 +169,6 @@ static struct sdebug_queued_cmd queued_a
 static Scsi_Host_Template sdebug_driver_template = {
 	.proc_info =		scsi_debug_proc_info,
 	.name =			"SCSI DEBUG",
-	.release =		scsi_debug_release,
 	.info =			scsi_debug_info,
 	.slave_alloc =		scsi_debug_slave_alloc,
 	.slave_configure =	scsi_debug_slave_configure,

@@ -207,9 +206,11 @@ static char sdebug_proc_name[] = "scsi_d
 
 static int sdebug_driver_probe(struct device *);
 static int sdebug_driver_remove(struct device *);
+static struct bus_type pseudo_lld_bus;
 
 static struct device_driver sdebug_driverfs_driver = {
 	.name 		= sdebug_proc_name,
+	.bus		= &pseudo_lld_bus,
 	.probe          = sdebug_driver_probe,
 	.remove         = sdebug_driver_remove,
 };

@@ -250,8 +251,6 @@ static int sdebug_add_adapter(void);
 static void sdebug_remove_adapter(void);
 static struct device pseudo_primary;
 static struct bus_type pseudo_lld_bus;
-static int scsi_debug_register_driver(struct device_driver *);
-static int scsi_debug_unregister_driver(struct device_driver *);
 
 static unsigned char * scatg2virt(const struct scatterlist * sclp)
 {

@@ -296,10 +295,12 @@ int scsi_debug_queuecommand(struct scsi_
 		bufflen = SDEBUG_SENSE_LEN;
 	}
 
+
         if(target == sdebug_driver_template.this_id) {
-                printk(KERN_WARNING 
-		       "scsi_debug: initiator's id used as target!\n");
-		return schedule_resp(SCpnt, NULL, done, 0, 0);
+		printk(KERN_INFO "scsi_debug: initiator's id used as "
+		       "target!\n");
+		return schedule_resp(SCpnt, NULL, done,
+				     DID_NO_CONNECT << 16, 0);
         }
 
 	if (SCpnt->device->lun >= scsi_debug_max_luns)

@@ -1523,7 +1524,7 @@ static int __init scsi_debug_init(void)
 
 	device_register(&pseudo_primary);
 	bus_register(&pseudo_lld_bus);
-	scsi_debug_register_driver(&sdebug_driverfs_driver);
+	driver_register(&sdebug_driverfs_driver);
 	do_create_driverfs_files();
 
 	sdebug_driver_template.proc_name = (char *)sdebug_proc_name;

@@ -1554,7 +1555,7 @@ static void __exit scsi_debug_exit(void)
 	for (; k; k--)
 		sdebug_remove_adapter();
 	do_remove_driverfs_files();
-	scsi_debug_unregister_driver(&sdebug_driverfs_driver);
+	driver_unregister(&sdebug_driverfs_driver);
 	bus_unregister(&pseudo_lld_bus);
 	device_unregister(&pseudo_primary);

@@ -1580,20 +1581,6 @@ static struct bus_type pseudo_lld_bus = 
         .match = pseudo_lld_bus_match,
 };
 
-static int scsi_debug_register_driver(struct device_driver *dev_driver)
-{
-	dev_driver->bus = &pseudo_lld_bus;
-	driver_register(dev_driver);
-
-	return 0;
-}
-
-static int scsi_debug_unregister_driver(struct device_driver *dev_driver)
-{
-	driver_unregister(dev_driver);
-	return 0;
-}
-
 static void sdebug_release_adapter(struct device * dev)
 {
         struct sdebug_host_info *sdbg_host;

@@ -1698,7 +1685,7 @@ static int sdebug_driver_probe(struct de
         if (NULL == hpnt) {
                 printk(KERN_ERR "%s: scsi_register failed\n", __FUNCTION__);
                 error = -ENODEV;
-                goto clean1;
+		return error;
         }
 
         sdbg_host->shost = hpnt;

@@ -1713,34 +1700,34 @@ static int sdebug_driver_probe(struct de
         if (error) {
                 printk(KERN_ERR "%s: scsi_add_host failed\n", __FUNCTION__);
                 error = -ENODEV;
-                goto clean2;
+		scsi_unregister(hpnt);
         }
 
 
-        return 0;
-
-clean2:
-        scsi_unregister(hpnt);
-clean1:
-        kfree(sdbg_host);
-
         return error;
 }
 
-static int scsi_debug_release(struct Scsi_Host * shost)
+static int sdebug_driver_remove(struct device * dev)
 {
         struct list_head *lh, *lh_sf;
-        struct sdebug_dev_info *sdbg_devinfo;
         struct sdebug_host_info *sdbg_host;
+        struct sdebug_dev_info *sdbg_devinfo;
 
-	sdbg_host = *(struct sdebug_host_info **)shost->hostdata;
-        scsi_unregister(shost);
+	sdbg_host = to_sdebug_host(dev);
 
 	if (!sdbg_host) {
-		printk(KERN_ERR "Unable to locate host info\n");
-		return 0;
+		printk(KERN_ERR "%s: Unable to locate host info\n",
+		       __FUNCTION__);
+		return -ENODEV;
 	}
 
+        if (scsi_remove_host(sdbg_host->shost)) {
+                printk(KERN_ERR "%s: scsi_remove_host failed\n", __FUNCTION__);
+                return -EBUSY;
+        }
+
+        scsi_unregister(sdbg_host->shost);
+
         list_for_each_safe(lh, lh_sf, &sdbg_host->dev_info_list) {
                 sdbg_devinfo = list_entry(lh, struct sdebug_dev_info,
                                           dev_list);

@@ -1749,19 +1736,4 @@ static int scsi_debug_release(struct Scs
         }
 
         return 0;
-
-}
-
-static int sdebug_driver_remove(struct device * dev)
-{
-        struct sdebug_host_info *sdbg_host;
-
-	sdbg_host = to_sdebug_host(dev);
-
-        if (sdbg_host && scsi_remove_host(sdbg_host->shost)) {
-                printk(KERN_ERR "%s: scsi_remove_host failed\n", __FUNCTION__);
-                return -EBUSY;
-        }
-
-        return 0;
 }

drivers/scsi/scsi_debug.h renamed from drivers/scsi/scsi_debug.h~scsi_debug-misc-fix +0 -1

diff -puN drivers/scsi/scsi_debug.h~scsi_debug-misc-fix drivers/scsi/scsi_debug.h
@@ -16,7 +16,6 @@ static int scsi_debug_device_reset(struc
 static int scsi_debug_host_reset(struct scsi_cmnd *);
 static int scsi_debug_proc_info(char *, char **, off_t, int, int, int);
 static const char * scsi_debug_info(struct Scsi_Host *);
-static int scsi_debug_release(struct Scsi_Host *);
 
 /*
  * This driver is written for the lk 2.5 series

Mike Anderson

author

2003-05-09 06:20 UTC #2 lore raw

PATCH 1/1 scsi_host sysfs updates scsi-misc-2.5 [2/2]

-andmike
--
Michael Anderson
[email protected]

DESC
scsi shost sysfs cleanups for scsi-misc-2.5
- Add LLDD short name to scsi_host struct device.
- scsi_host_release now calls scsi_free_shost.
- Switched from device_register / device_unregister and class_register / class_register to initialize, add, del, put pairs.
- Moved some function from scsi_register and scsi_unregister.
- Filled in scsi_host_put and scsi_host_get.
EDESC

 drivers/scsi/hosts.c      |   33 ++++++++++++++++++++++++++++-----
 drivers/scsi/hosts.h      |    2 ++
 drivers/scsi/scsi_sysfs.c |   19 +++++++++++--------
 3 files changed, 41 insertions(+), 13 deletions(-)

drivers/scsi/scsi_sysfs.c renamed from drivers/scsi/scsi_sysfs.c~scsi_shost_sysfs-misc-fix +11 -8

diff -puN drivers/scsi/scsi_sysfs.c~scsi_shost_sysfs-misc-fix drivers/scsi/scsi_sysfs.c
@@ -317,7 +317,8 @@ static void scsi_host_release(struct dev
 	shost = dev_to_shost(dev);
 	if (!shost)
 		return;
-	shost->hostt->release(shost);
+
+	scsi_free_shost(shost);
 }
 
 /**

@@ -329,13 +330,15 @@ int scsi_sysfs_add_host(struct Scsi_Host
 {
 	int i, error;
 
-	sprintf(shost->host_gendev.bus_id,"host%d",
+	snprintf(shost->host_gendev.bus_id, BUS_ID_SIZE, "host%d",
 		shost->host_no);
+	snprintf(shost->host_gendev.name, DEVICE_NAME_SIZE, "%s",
+		shost->hostt->proc_name);
 	if (!shost->host_gendev.parent)
 		shost->host_gendev.parent = (dev) ? dev : &legacy_bus;
 	shost->host_gendev.release = scsi_host_release;
 
-	error = device_register(&shost->host_gendev);
+	error = device_add(&shost->host_gendev);
 	if (error)
 		return error;

@@ -343,7 +346,7 @@ int scsi_sysfs_add_host(struct Scsi_Host
 	shost->class_dev.class = &shost_class;
 	snprintf(shost->class_dev.class_id, BUS_ID_SIZE, "host%d",
 		  shost->host_no);
-	error = class_device_register(&shost->class_dev);
+	error = class_device_add(&shost->class_dev);
 	if (error)
 		goto clean_device;

@@ -356,9 +359,9 @@ int scsi_sysfs_add_host(struct Scsi_Host
 	return error;
 
 clean_class:
-	class_device_unregister(&shost->class_dev);
+	class_device_del(&shost->class_dev);
 clean_device:
-	device_unregister(&shost->host_gendev);
+	device_del(&shost->host_gendev);
 
 	return error;
 }

@@ -369,7 +372,7 @@ clean_device:
  **/
 void scsi_sysfs_remove_host(struct Scsi_Host *shost)
 {
-	class_device_unregister(&shost->class_dev);
-	device_unregister(&shost->host_gendev);
+	class_device_del(&shost->class_dev);
+	device_del(&shost->host_gendev);
 }

drivers/scsi/hosts.c renamed from drivers/scsi/hosts.c~scsi_shost_sysfs-misc-fix +28 -5

diff -puN drivers/scsi/hosts.c~scsi_shost_sysfs-misc-fix drivers/scsi/hosts.c
@@ -212,6 +212,7 @@ int scsi_remove_host(struct Scsi_Host *s
 	list_for_each_entry(sdev, &shost->my_devices, siblings)
 		sdev->online = FALSE;
 
+	scsi_proc_host_rm(shost);
 	scsi_forget_host(shost);
 	scsi_sysfs_remove_host(shost);
 	return 0;

@@ -238,6 +239,8 @@ int scsi_add_host(struct Scsi_Host *shos
 	if (error)
 		return error;
 
+	scsi_proc_host_add(shost);
+
 	scsi_scan_host(shost);
 			
 	list_for_each_entry (sdev, &shost->my_devices, siblings) {

@@ -255,6 +258,15 @@ int scsi_add_host(struct Scsi_Host *shos
  **/
 void scsi_unregister(struct Scsi_Host *shost)
 {
+	scsi_host_put(shost);
+}
+
+/**
+ * scsi_free_sdev - free a scsi hosts resources
+ * @shost:	scsi host to free 
+ **/
+void scsi_free_shost(struct Scsi_Host *shost)
+{
 	/* Remove shost from scsi_host_list */
 	spin_lock(&scsi_host_list_lock);
 	list_del(&shost->sh_list);

@@ -273,7 +285,6 @@ void scsi_unregister(struct Scsi_Host *s
 	}
 
 	shost->hostt->present--;
-	scsi_proc_host_rm(shost);
 	scsi_destroy_command_freelist(shost);
 	kfree(shost);
 }

@@ -394,7 +405,8 @@ found:
 	rval = scsi_setup_command_freelist(shost);
 	if (rval)
 		goto fail;
-	scsi_proc_host_add(shost);
+	device_initialize(&shost->host_gendev);
+	class_device_initialize(&shost->class_dev);
 
 	shost->eh_notify = &sem;
 	kernel_thread((int (*)(void *)) scsi_error_handler, (void *) shost, 0);

@@ -523,15 +535,26 @@ struct Scsi_Host *scsi_host_hn_get(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)
+{
+
+	get_device(&shost->host_gendev);
+	class_device_get(&shost->class_dev);
+	return;
+}
+
+/**
  * *scsi_host_put - dec a Scsi_Host ref count
  * @shost:	Pointer to Scsi_Host to dec.
  **/
 void scsi_host_put(struct Scsi_Host *shost)
 {
 
-	/* XXX Get list lock */
-	/* XXX dec ref count */
-	/* XXX Release list lock */
+	put_device(&shost->host_gendev);
+	class_device_put(&shost->class_dev);
 	return;
 }

drivers/scsi/hosts.h renamed from drivers/scsi/hosts.h~scsi_shost_sysfs-misc-fix +2 -0

diff -puN drivers/scsi/hosts.h~scsi_shost_sysfs-misc-fix drivers/scsi/hosts.h
@@ -556,6 +556,7 @@ extern int scsi_unregister_device(struct
  */
 extern struct Scsi_Host * scsi_register(Scsi_Host_Template *, int);
 extern void scsi_unregister(struct Scsi_Host *);
+extern void scsi_free_shost(struct Scsi_Host *);
 
 /*
  * HBA registration/unregistration.

@@ -570,6 +571,7 @@ extern int scsi_register_host(Scsi_Host_
 extern int scsi_unregister_host(Scsi_Host_Template *);
 
 extern struct Scsi_Host *scsi_host_hn_get(unsigned short);
+extern void scsi_host_get(struct Scsi_Host *);
 extern void scsi_host_put(struct Scsi_Host *);
 
 /**

Christoph Hellwig

2003-05-09 06:47 UTC #3 lore raw

Re: [PATCH] scsi_host sysfs updates scsi-misc-2.5 [2/2]

drivers/scsi/hosts.h extern int scsi_unregister_device(struct · line 559

Quoted 7 lines · Mike Anderson

drivers/scsi/hosts.h renamed from drivers/scsi/hosts.h~scsi_shost_sysfs-misc-fix +1 -0

@@ -556,6 +556,7 @@ extern int scsi_unregister_device(struct
  */
 extern struct Scsi_Host * scsi_register(Scsi_Host_Template *, int);
 extern void scsi_unregister(struct Scsi_Host *);
+extern void scsi_free_shost(struct Scsi_Host *);

This should go into scsi_priv.h

drivers/scsi/hosts.h extern int scsi_register_host(Scsi_Host_ · line 575

Quoted 9 lines

drivers/scsi/hosts.h renamed from drivers/scsi/hosts.h~scsi_shost_sysfs-misc-fix +1 -0

 /*
  * HBA registration/unregistration.

@@ -570,6 +571,7 @@ extern int scsi_register_host(Scsi_Host_
 extern int scsi_unregister_host(Scsi_Host_Template *);
 
 extern struct Scsi_Host *scsi_host_hn_get(unsigned short);
+extern void scsi_host_get(struct Scsi_Host *);
 extern void scsi_host_put(struct Scsi_Host *);

This should go either into scsi_priv.h or exported if we see a use for it in driver (I'd rather avoid drivers messing with reference counts, though)

Patch looks fine otherwise.

Mike Anderson

author

2003-05-09 07:36 UTC #4 lore raw

Re: [PATCH] scsi_host sysfs updates scsi-misc-2.5 [2/2]

Quoted 11 lines · Christoph Hellwig [[email protected]] wrote:

drivers/scsi/hosts.h extern int scsi_unregister_device(struct · line 559
Quoted 7 lines · Mike Anderson
drivers/scsi/hosts.h renamed from drivers/scsi/hosts.h~scsi_shost_sysfs-misc-fix +1 -0
@@ -556,6 +556,7 @@ extern int scsi_unregister_device(struct
  */
 extern struct Scsi_Host * scsi_register(Scsi_Host_Template *, int);
 extern void scsi_unregister(struct Scsi_Host *);
+extern void scsi_free_shost(struct Scsi_Host *);
This should go into scsi_priv.h

Ok I will move this and resend to James.

Quoted 6 lines

Quoted 6 lines
drivers/scsi/hosts.h renamed from drivers/scsi/hosts.h~scsi_shost_sysfs-misc-fix +0 -0
 /*
  * HBA registration/unregistration.
@@ -570,6 +571,7 @@ extern int scsi_register_host(Scsi_Host_
 extern int scsi_unregister_host(Scsi_Host_Template *);
 

Quoted 8 lines

drivers/scsi/hosts.h extern int scsi_register_host(Scsi_Host_ · line 575
Quoted 3 lines
drivers/scsi/hosts.h renamed from drivers/scsi/hosts.h~scsi_shost_sysfs-misc-fix +1 -0
 extern struct Scsi_Host *scsi_host_hn_get(unsigned short);
+extern void scsi_host_get(struct Scsi_Host *);
 extern void scsi_host_put(struct Scsi_Host *);
This should go either into scsi_priv.h or exported if we see a use for it in driver (I'd rather avoid drivers messing with reference counts, though)

I think it should go into scsi_priv.h. I thought it would be good to have a symmetric interface, but would worry about drivers messing with the counts if it was exported.

Note: scsi_host_get_next and scsi_host_hn_get need ref counting, but I wanted to look at this a bit and see if we could utilize some sysfs interfaces.

-andmike
--
Michael Anderson
[email protected]

Mike Anderson

author

2003-05-09 08:06 UTC #5 lore raw

Re: [PATCH] scsi_host sysfs updates scsi-misc-2.5 [2/2]

Here is an update of the patch with the externs in scsi_priv.h

-andmike
--
Michael Anderson
[email protected]

Rev 2 move externs to scsi_priv.h
EDESC

 drivers/scsi/hosts.c      |   33 ++++++++++++++++++++++++++++-----
 drivers/scsi/scsi_priv.h  |    2 ++
 drivers/scsi/scsi_sysfs.c |   19 +++++++++++--------
 3 files changed, 41 insertions(+), 13 deletions(-)

drivers/scsi/scsi_sysfs.c renamed from drivers/scsi/scsi_sysfs.c~scsi_shost_sysfs-misc-fix +11 -8

diff -puN drivers/scsi/scsi_sysfs.c~scsi_shost_sysfs-misc-fix drivers/scsi/scsi_sysfs.c
@@ -317,7 +317,8 @@ static void scsi_host_release(struct dev
 	shost = dev_to_shost(dev);
 	if (!shost)
 		return;
-	shost->hostt->release(shost);
+
+	scsi_free_shost(shost);
 }
 
 /**

@@ -329,13 +330,15 @@ int scsi_sysfs_add_host(struct Scsi_Host
 {
 	int i, error;
 
-	sprintf(shost->host_gendev.bus_id,"host%d",
+	snprintf(shost->host_gendev.bus_id, BUS_ID_SIZE, "host%d",
 		shost->host_no);
+	snprintf(shost->host_gendev.name, DEVICE_NAME_SIZE, "%s",
+		shost->hostt->proc_name);
 	if (!shost->host_gendev.parent)
 		shost->host_gendev.parent = (dev) ? dev : &legacy_bus;
 	shost->host_gendev.release = scsi_host_release;
 
-	error = device_register(&shost->host_gendev);
+	error = device_add(&shost->host_gendev);
 	if (error)
 		return error;

@@ -343,7 +346,7 @@ int scsi_sysfs_add_host(struct Scsi_Host
 	shost->class_dev.class = &shost_class;
 	snprintf(shost->class_dev.class_id, BUS_ID_SIZE, "host%d",
 		  shost->host_no);
-	error = class_device_register(&shost->class_dev);
+	error = class_device_add(&shost->class_dev);
 	if (error)
 		goto clean_device;

@@ -356,9 +359,9 @@ int scsi_sysfs_add_host(struct Scsi_Host
 	return error;
 
 clean_class:
-	class_device_unregister(&shost->class_dev);
+	class_device_del(&shost->class_dev);
 clean_device:
-	device_unregister(&shost->host_gendev);
+	device_del(&shost->host_gendev);
 
 	return error;
 }

@@ -369,7 +372,7 @@ clean_device:
  **/
 void scsi_sysfs_remove_host(struct Scsi_Host *shost)
 {
-	class_device_unregister(&shost->class_dev);
-	device_unregister(&shost->host_gendev);
+	class_device_del(&shost->class_dev);
+	device_del(&shost->host_gendev);
 }

drivers/scsi/hosts.c renamed from drivers/scsi/hosts.c~scsi_shost_sysfs-misc-fix +28 -5

diff -puN drivers/scsi/hosts.c~scsi_shost_sysfs-misc-fix drivers/scsi/hosts.c
@@ -212,6 +212,7 @@ int scsi_remove_host(struct Scsi_Host *s
 	list_for_each_entry(sdev, &shost->my_devices, siblings)
 		sdev->online = FALSE;
 
+	scsi_proc_host_rm(shost);
 	scsi_forget_host(shost);
 	scsi_sysfs_remove_host(shost);
 	return 0;

@@ -238,6 +239,8 @@ int scsi_add_host(struct Scsi_Host *shos
 	if (error)
 		return error;
 
+	scsi_proc_host_add(shost);
+
 	scsi_scan_host(shost);
 			
 	list_for_each_entry (sdev, &shost->my_devices, siblings) {

@@ -255,6 +258,15 @@ int scsi_add_host(struct Scsi_Host *shos
  **/
 void scsi_unregister(struct Scsi_Host *shost)
 {
+	scsi_host_put(shost);
+}
+
+/**
+ * scsi_free_sdev - free a scsi hosts resources
+ * @shost:	scsi host to free 
+ **/
+void scsi_free_shost(struct Scsi_Host *shost)
+{
 	/* Remove shost from scsi_host_list */
 	spin_lock(&scsi_host_list_lock);
 	list_del(&shost->sh_list);

@@ -273,7 +285,6 @@ void scsi_unregister(struct Scsi_Host *s
 	}
 
 	shost->hostt->present--;
-	scsi_proc_host_rm(shost);
 	scsi_destroy_command_freelist(shost);
 	kfree(shost);
 }

@@ -394,7 +405,8 @@ found:
 	rval = scsi_setup_command_freelist(shost);
 	if (rval)
 		goto fail;
-	scsi_proc_host_add(shost);
+	device_initialize(&shost->host_gendev);
+	class_device_initialize(&shost->class_dev);
 
 	shost->eh_notify = &sem;
 	kernel_thread((int (*)(void *)) scsi_error_handler, (void *) shost, 0);

@@ -523,15 +535,26 @@ struct Scsi_Host *scsi_host_hn_get(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)
+{
+
+	get_device(&shost->host_gendev);
+	class_device_get(&shost->class_dev);
+	return;
+}
+
+/**
  * *scsi_host_put - dec a Scsi_Host ref count
  * @shost:	Pointer to Scsi_Host to dec.
  **/
 void scsi_host_put(struct Scsi_Host *shost)
 {
 
-	/* XXX Get list lock */
-	/* XXX dec ref count */
-	/* XXX Release list lock */
+	put_device(&shost->host_gendev);
+	class_device_put(&shost->class_dev);
 	return;
 }

drivers/scsi/scsi_priv.h renamed from drivers/scsi/scsi_priv.h~scsi_shost_sysfs-misc-fix +2 -0

diff -puN drivers/scsi/scsi_priv.h~scsi_shost_sysfs-misc-fix drivers/scsi/scsi_priv.h
@@ -109,6 +109,8 @@ extern void scsi_exit_procfs(void);
 extern void scsi_scan_host(struct Scsi_Host *shost);
 extern void scsi_forget_host(struct Scsi_Host *shost);
 extern void scsi_free_sdev(struct scsi_device *);
+extern void scsi_free_shost(struct Scsi_Host *);
+extern void scsi_host_get(struct Scsi_Host *);
 
 /* scsi_sysfs.c */
 extern int scsi_device_register(struct scsi_device *);

James Bottomley

2003-05-12 03:45 UTC #6 lore raw

In reply to Mike Anderson

Quoted 2 lines · Mike Anderson

This series of patches is an update to the scsi_host sysfs / ref counting patches previously merged into the scsi-misc-2.5 tree.

I'm still getting an oops in a scsi_register followed by a scsi_unregister (because of an error in the driver setup). The problem occurs because the mid-layer is now dependent on the host_gendev.release method. Unfortunately, this isn't set until scsi_host_add, which may be quite a while after scsi_register.

The quick "fix" is attached below, but I think we need all of this to be symmetric (i.e. scsi_unregister can be called any time after scsi_register) so probably the host_gendev.release method should be set elsewhere.

James

===== hosts.c 1.62 vs edited =====

hosts.c renamed from drivers/scsi/hosts.c +4 -1

@@ -258,7 +258,10 @@
  **/
 void scsi_unregister(struct Scsi_Host *shost)
 {
-	scsi_host_put(shost);
+	if(shost->host_gendev.release == NULL)
+		scsi_free_shost(shost);
+	else
+		scsi_host_put(shost);
 }
 
 /**

Mike Anderson

author

2003-05-12 06:23 UTC #7 lore raw

Quoted 10 lines · James Bottomley [[email protected]] wrote:

Quoted 2 lines · Mike Anderson
This series of patches is an update to the scsi_host sysfs / ref counting patches previously merged into the scsi-misc-2.5 tree.
I'm still getting an oops in a scsi_register followed by a scsi_unregister (because of an error in the driver setup). The problem occurs because the mid-layer is now dependent on the host_gendev.release method. Unfortunately, this isn't set until scsi_host_add, which may be quite a while after scsi_register.

Quoted 4 lines

The quick "fix" is attached below, but I think we need all of this to be symmetric (i.e. scsi_unregister can be called any time after scsi_register) so probably the host_gendev.release method should be set elsewhere.

I agree on the symmetric interface. I attached a patch that adds a scsi_sysfs_init_host function call which moves more initialization during the scsi_register time frame.

I tested the attached patch on my current config of ips, qlogicisp, aic7xxx, and scsi_debug.

Can you run this on your system and see if it addresses your issue? If not I can hack scsi_debug to make back to back scsi_register / scsi_unregister calls.

-andmike
--
Michael Anderson
[email protected]

DESC
Patch against scsi-misc-2.5

Fix scsi sysfs init so that a scsi_unregister can be called anytime after a scsi_register.
- Create scsi_sysfs_init_host function and call from scsi_register.
EDESC

 drivers/scsi/hosts.c      |    4 ++--
 drivers/scsi/scsi_priv.h  |    1 +
 drivers/scsi/scsi_sysfs.c |   25 ++++++++++++++++---------
 3 files changed, 19 insertions(+), 11 deletions(-)

drivers/scsi/scsi_priv.h renamed from drivers/scsi/scsi_priv.h~scsi_unregister-syfs-fix +1 -0

diff -puN drivers/scsi/scsi_priv.h~scsi_unregister-syfs-fix drivers/scsi/scsi_priv.h
@@ -123,6 +123,7 @@ extern int scsi_device_register(struct s
 extern void scsi_device_unregister(struct scsi_device *);
 extern int scsi_upper_driver_register(struct Scsi_Device_Template *);
 extern void scsi_upper_driver_unregister(struct Scsi_Device_Template *);
+extern void scsi_sysfs_init_host(struct Scsi_Host *);
 extern int scsi_sysfs_add_host(struct Scsi_Host *, struct device *);
 extern void scsi_sysfs_remove_host(struct Scsi_Host *);
 extern int scsi_sysfs_register(void);

drivers/scsi/scsi_sysfs.c renamed from drivers/scsi/scsi_sysfs.c~scsi_unregister-syfs-fix +16 -9

diff -puN drivers/scsi/scsi_sysfs.c~scsi_unregister-syfs-fix drivers/scsi/scsi_sysfs.c
@@ -321,6 +321,22 @@ static void scsi_host_release(struct dev
 	scsi_free_shost(shost);
 }
 
+void scsi_sysfs_init_host(struct Scsi_Host *shost)
+{
+	device_initialize(&shost->host_gendev);
+	snprintf(shost->host_gendev.bus_id, BUS_ID_SIZE, "host%d",
+		shost->host_no);
+	snprintf(shost->host_gendev.name, DEVICE_NAME_SIZE, "%s",
+		shost->hostt->proc_name);
+	shost->host_gendev.release = scsi_host_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);
+}
+
 /**
  * scsi_sysfs_add_host - add scsi host to subsystem
  * @shost:     scsi host struct to add to subsystem

@@ -330,22 +346,13 @@ int scsi_sysfs_add_host(struct Scsi_Host
 {
 	int i, error;
 
-	snprintf(shost->host_gendev.bus_id, BUS_ID_SIZE, "host%d",
-		shost->host_no);
-	snprintf(shost->host_gendev.name, DEVICE_NAME_SIZE, "%s",
-		shost->hostt->proc_name);
 	if (!shost->host_gendev.parent)
 		shost->host_gendev.parent = (dev) ? dev : &legacy_bus;
-	shost->host_gendev.release = scsi_host_release;
 
 	error = device_add(&shost->host_gendev);
 	if (error)
 		return error;
 
-	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);
 	error = class_device_add(&shost->class_dev);
 	if (error)
 		goto clean_device;

drivers/scsi/hosts.c renamed from drivers/scsi/hosts.c~scsi_unregister-syfs-fix +2 -2

diff -puN drivers/scsi/hosts.c~scsi_unregister-syfs-fix drivers/scsi/hosts.c
@@ -405,8 +405,8 @@ found:
 	rval = scsi_setup_command_freelist(shost);
 	if (rval)
 		goto fail;
-	device_initialize(&shost->host_gendev);
-	class_device_initialize(&shost->class_dev);
+
+	scsi_sysfs_init_host(shost);
 
 	shost->eh_notify = &sem;
 	kernel_thread((int (*)(void *)) scsi_error_handler, (void *) shost, 0);

James Bottomley

2003-05-12 17:38 UTC #8 lore raw

Quoted 3 lines · Mike Anderson

Can you run this on your system and see if it addresses your issue? If not I can hack scsi_debug to make back to back scsi_register / scsi_unregister calls.

Surprisingly, it still fails. I've debugged the refcounting and I see it fall to zero in scsi_host_put but the dev release method doesn't trigger for no reason I can see.

It's probably a kobject initialisation problem somewhere.

Also, eventually, don't we need to do class_device_remove_files() on the created class files, so that will have to be done in release and we've got symmetry problems again?

James

James Bottomley

2003-05-12 17:46 UTC #9 lore raw

Quoted 5 lines · James Bottomley

Surprisingly, it still fails. I've debugged the refcounting and I see it fall to zero in scsi_host_put but the dev release method doesn't trigger for no reason I can see.
It's probably a kobject initialisation problem somewhere.

OK, found it. the ->release is triggered by the kobj_kset, which isn't set until device_add time, not device init time.

Therefore, we shouldn't really put and get objects until they've been added (or the kobj_set_kset_s needs to be moved to device_initialize()).

The more I think about it, the more it does look like we need to separate the partially initialized case from the fully initialized one, so perhaps the initial patch I made and thought was awful may actually turn out to be the more correct one.

James

Mike Anderson

author

2003-05-12 18:26 UTC #10 lore raw

Quoted 5 lines · James Bottomley [[email protected]] wrote:

Quoted 4 lines · James Bottomley
Surprisingly, it still fails. I've debugged the refcounting and I see it fall to zero in scsi_host_put but the dev release method doesn't trigger for no reason I can see.

Quoted 8 lines

Quoted 1 line
It's probably a kobject initialisation problem somewhere.
OK, found it. the ->release is triggered by the kobj_kset, which isn't set until device_add time, not device init time.
Therefore, we shouldn't really put and get objects until they've been added (or the kobj_set_kset_s needs to be moved to device_initialize()).

I would like to have feedback from Mochel on the device_initialize call.

The documentation I have read indicates the device_initialize call was to allow the struct device to be usable for reference counting. I think we should determine if that is the intent and can we call kobj_set_kset_s in device_initialize.

Another option may be to set the ktype value on our struct device kobject until device_add overrides this with the call to kobj_set_kset_s.

-andmike
--
Michael Anderson
[email protected]

James Bottomley

2003-05-12 19:57 UTC #11 lore raw

Quoted 10 lines · Mike Anderson

I would like to have feedback from Mochel on the device_initialize call.
The documentation I have read indicates the device_initialize call was to allow the struct device to be usable for reference counting. I think we should determine if that is the intent and can we call kobj_set_kset_s in device_initialize.
Another option may be to set the ktype value on our struct device kobject until device_add overrides this with the call to kobj_set_kset_s.

I can confirm that moving kobj_set_kset_s fixes my boot up problem (with your other patch). However, now I'm running into an obscure slab corruption issue, so I suspect the move has to be more carefully orchestrated.

James

Mike Anderson

author

2003-05-12 20:20 UTC #12 lore raw

Quoted 9 lines · James Bottomley [[email protected]] wrote:

Quoted 8 lines · Mike Anderson
I would like to have feedback from Mochel on the device_initialize call.
The documentation I have read indicates the device_initialize call was to allow the struct device to be usable for reference counting. I think we should determine if that is the intent and can we call kobj_set_kset_s in device_initialize.

Quoted 7 lines

Quoted 2 lines
Another option may be to set the ktype value on our struct device kobject until device_add overrides this with the call to kobj_set_kset_s.
I can confirm that moving kobj_set_kset_s fixes my boot up problem (with your other patch). However, now I'm running into an obscure slab corruption issue, so I suspect the move has to be more carefully orchestrated.

I checked out that setting kobj.ktype also seems to solve the problem (using a hacked version of scsi_debug and calling scsi_register followed by a call to scsi_unregister). This would give a more localized work around, but device_initialize kobj_set_kset_s may be a better choice.

What slab size? (size-512)

-andmike
--
Michael Anderson
[email protected]

James Bottomley

2003-05-12 20:29 UTC #13 lore raw

Quoted 4 lines · Mike Anderson

I checked out that setting kobj.ktype also seems to solve the problem (using a hacked version of scsi_debug and calling scsi_register followed by a call to scsi_unregister). This would give a more localized work around, but device_initialize kobj_set_kset_s may be a better choice.

Yes, that might fix it, setting the kset would cause a spurious kset_put. I'll try it out.

Quoted 1 line

What slab size? (size-512)

size-4096

James

James Bottomley

2003-05-12 20:40 UTC #14 lore raw

Quoted 6 lines · James Bottomley

Quoted 4 lines · Mike Anderson
I checked out that setting kobj.ktype also seems to solve the problem (using a hacked version of scsi_debug and calling scsi_register followed by a call to scsi_unregister). This would give a more localized work around, but device_initialize kobj_set_kset_s may be a better choice.

Quoted 6 lines

Yes, that might fix it, setting the kset would cause a spurious kset_put. I'll try it out.
Quoted 1 line
What slab size? (size-512)
size-4096

well, just setting the kset still gives me my check_poison_obj use after free. I'll see if I can find it.

James

Mike Anderson

author

2003-05-12 21:33 UTC #15 lore raw

In reply to James Bottomley

Quoted 4 lines · James Bottomley [[email protected]] wrote:

Quoted 1 line
What slab size? (size-512)
size-4096

I am seeing size-512 with my modified version of scsi_debug. I modified slab.c to store last user for this size and it indicates it was scsi_free_shost. I am looking at this now.

-andmike
--
Michael Anderson
[email protected]

James Bottomley

2003-05-12 21:37 UTC #16 lore raw

Quoted 3 lines · Mike Anderson

I am seeing size-512 with my modified version of scsi_debug. I modified slab.c to store last user for this size and it indicates it was scsi_free_shost. I am looking at this now.

I found it: you have a use after free in the sysfs code:

scsi_host_put does put_device followed by class_device_put, but the put_device will free the shost containing the class_device in it's release, so the class_device_put touches a freed object.

The solution is just to reverse the order of the puts.

James

Mike Anderson

author

2003-05-12 22:00 UTC #17 lore raw

Quoted 5 lines · James Bottomley [[email protected]] wrote:

Quoted 3 lines · Mike Anderson
I am seeing size-512 with my modified version of scsi_debug. I modified slab.c to store last user for this size and it indicates it was scsi_free_shost. I am looking at this now.

Quoted 7 lines

I found it: you have a use after free in the sysfs code:
scsi_host_put does put_device followed by class_device_put, but the put_device will free the shost containing the class_device in it's release, so the class_device_put touches a freed object.
The solution is just to reverse the order of the puts.

Thanks for finding this. This fixes my slab issue also.

-andmike
--
Michael Anderson
[email protected]

Patrick Mochel

2003-05-13 23:47 UTC #18 lore raw

In reply to Mike Anderson

Quoted 11 lines · Mike Anderson

Quoted 10 lines · James Bottomley [[email protected]] wrote:
Quoted 5 lines · James Bottomley
Surprisingly, it still fails. I've debugged the refcounting and I see it fall to zero in scsi_host_put but the dev release method doesn't trigger for no reason I can see.
It's probably a kobject initialisation problem somewhere.
OK, found it. the ->release is triggered by the kobj_kset, which isn't set until device_add time, not device init time.

Quoted 6 lines

Quoted 3 lines
Therefore, we shouldn't really put and get objects until they've been added (or the kobj_set_kset_s needs to be moved to device_initialize()).
I would like to have feedback from Mochel on the device_initialize call.

Just to set the record straight - James suggested this on irc yesterday, and I've added it to my tree. I haven't seen any issues with it, and based on later emails, you've worked out the slab issue. So, it'll be pushed to Linus.

-pat

Mike Anderson

author

2003-05-12 18:03 UTC #19 lore raw

In reply to James Bottomley

Quoted 4 lines · James Bottomley [[email protected]] wrote:

Also, eventually, don't we need to do class_device_remove_files() on the created class files, so that will have to be done in release and we've got symmetry problems again?

I have been told that we you do not need to call class_device_remove_files as the cleanup functionality of *_remove_files happens implicitly through the call to sysfs_remove_dir. I will verify this.

If we needed to add this back in we would add it to scsi_sysfs_remove_host as they are created in scsi_sysfs_add_host.

-andmike
--
Michael Anderson
[email protected]

Andrew Vasquez

2003-05-12 15:06 UTC #20 lore raw

In reply to Mike Anderson

Quoted 9 lines · Mike Anderson

This series of patches is an update to the scsi_host sysfs / ref counting patches previously merged into the scsi-misc-2.5 tree.
insmod / rmmod Testing:
Legacy registration:
aic7xxx_old, qlogicisp
qla2xxx v8.00.00b1 - Driver oops system with garbled messages on rmmod. Does not seem related to this patch.

Is the driver still oops'ng during your testing? Could you provide some specifics on attached storage as well as environment (fabric, loop, hba type)? I'll resync my working tree with scsi-misc and perform some additional investigation.

Thanks,
Andrew Vasquez

Mike Anderson

author

2003-05-13 18:36 UTC #21 lore raw

Quoted 12 lines · Andrew Vasquez [[email protected]] wrote:

Quoted 9 lines · Mike Anderson
This series of patches is an update to the scsi_host sysfs / ref counting patches previously merged into the scsi-misc-2.5 tree.
insmod / rmmod Testing:
Legacy registration:
aic7xxx_old, qlogicisp
qla2xxx v8.00.00b1 - Driver oops system with garbled messages on rmmod. Does not seem related to this patch.

Quoted 4 lines

Is the driver still oops'ng during your testing? Could you provide some specifics on attached storage as well as environment (fabric, loop, hba type)? I'll resync my working tree with scsi-misc and perform some additional investigation.

The driver is still oops'ng on current scsi-misc-2.5, but I tried linux-2.5 bk 2.5.69+ (change set 1.1095) and it did not oops after multiple insmod/rmmod cycles.

I do not get consistent oops, but the one below occurs frequently on rmmod. I will track it down more on my system and see if there are changes between linux-2.5 and scsi-misc that would address what I am seeing.

My configuration is 2 qla2300, IBM 2109 switch, IBM FASTt 200, 10 FC JBOD disks.

-andmike
--
Michael Anderson
[email protected]

Unable to handle kernel paging request at virtual address d08a9110
 printing eip:
d08a9110
*pde = 00000000
Oops: 0000 [#1]
CPU:    1
EIP:    0060:[<d08a9110>]    Not tainted
EFLAGS: 00010246

EIP is at 0xd08a9110
eax: 00000000 ebx: c12ba000 ecx: cb967948 edx: c12bbf24
esi: cb9641c0 edi: c1287f80 ebp: c1287f80 esp: c12bbf10
ds: 007b es: 007b ss: 0068
Process swapper (pid: 0, threadinfo=c12ba000 task=c12bc6a<10>)Un abSlet atcko: h acn0dl1e2 d1kce0r ncebl 9p6a4g1cin0g rce12bqbuefs24t a<t4> vci12rbtbuaf2l4 add0dr8eas9s11 d00 8ca9c128510f
8 p<4r>icnct2i8n5gf e48 ip:0
000d0080a19 11<40> 0

* pd e = bf<4ff>fcf03699
0U8n abflffe ftfof fdha nd0l0e00 k00er46n elc 0pa12gi8cn6gb recq03u9es2ta0 8 at <v4i>0r0tu00a0l0 a0d4 dr0e0ssed 70aff8f0 f2ca412
b f7prci n<ti4>ng b
e i p :
c0<14>1cd0012078
a0* p<d4e>c 0=1 1a5953ba50a 5a0
00fff cdc22084 c0391f00 ce560080 c01089a0 c01089a0

Call Trace:
 [<c012d1c0>] run_timer_softirq+0x1c0/0x270
 [<c0128c6b>] do_softirq+0x6b/0xd0
 [<c01089a0>] default_idle+0x0/0x40
 [<c01193b0>] smp_apic_timer_interrupt+0x140/0x160
 [<c01089a0>] default_idle+0x0/0x40
 [<c01089a0>] default_idle+0x0/0x40
 [<c010b8ce>] apic_timer_interrupt+0x1a/0x20
 [<c01089a0>] default_idle+0x0/0x40
 [<c01089a0>] default_idle+0x0/0x40
 [<c01089ca>] default_idle+0x2a/0x40
 [<c0108a72>] cpu_idle+0x52/0x70
 [<c0125533>] release_console_sem+0xd3/0x190