[RFC] Allow transport classes to attach to host, target or device

[RFC] Allow transport classes to attach to host, target or device

[James Bottomley]

This is a nice simple patch that makes the transport class a combination
of up to three sysfs classes, attaching to host, transport or device. 
So that the attributes it's exporting may be attached to the correct
place.

I also got rid of the cleanup methods since the users of the class make
it obvious that these are totally unnecessary (cleanup is done as part
of the class release instead).

This compiles and runs, but is really only a sketch of what's planned.

James

diff -Nru a/drivers/scsi/hosts.c b/drivers/scsi/hosts.c
--- a/drivers/scsi/hosts.c	Mon Sep  6 09:42:22 2004
+++ b/drivers/scsi/hosts.c	Mon Sep  6 09:42:22 2004
@@ -82,6 +82,8 @@
 	set_bit(SHOST_DEL, &shost->shost_state);
 
 	class_device_unregister(&shost->shost_classdev);
+	if (shost->transport_classdev.class)
+		class_device_unregister(&shost->transport_classdev);
 	device_del(&shost->shost_gendev);
 }
 
diff -Nru a/drivers/scsi/scsi_priv.h b/drivers/scsi/scsi_priv.h
--- a/drivers/scsi/scsi_priv.h	Mon Sep  6 09:42:22 2004
+++ b/drivers/scsi/scsi_priv.h	Mon Sep  6 09:42:22 2004
@@ -66,13 +66,17 @@
 struct scsi_target {
 	struct scsi_device	*starget_sdev_user;
 	struct device		dev;
-};
+	struct class_device	transport_classdev;
+	unsigned long		transport_data[0];
+} __attribute__((aligned(sizeof(unsigned long))));
 
 #define to_scsi_target(d)	container_of(d, struct scsi_target, dev)
 static inline struct scsi_target *scsi_target(struct scsi_device *sdev)
 {
 	return to_scsi_target(sdev->sdev_gendev.parent);
 }
+#define transport_classdev_to_starget(class_dev) \
+	container_of(class_dev, struct scsi_target, transport_classdev)
 
 /* hosts.c */
 extern int scsi_init_hosts(void);
diff -Nru a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
--- a/drivers/scsi/scsi_scan.c	Mon Sep  6 09:42:22 2004
+++ b/drivers/scsi/scsi_scan.c	Mon Sep  6 09:42:22 2004
@@ -205,7 +205,8 @@
 	struct scsi_device *sdev, *device;
 	unsigned long flags;
 
-	sdev = kmalloc(sizeof(*sdev) + shost->transportt->size, GFP_ATOMIC);
+	sdev = kmalloc(sizeof(*sdev) + shost->transportt->device_size,
+		       GFP_ATOMIC);
 	if (!sdev)
 		goto out;
 
@@ -256,14 +257,14 @@
 			goto out_free_queue;
 	}
 
-	if (shost->transportt->setup) {
-		if (shost->transportt->setup(sdev))
+	if (shost->transportt->device_setup) {
+		if (shost->transportt->device_setup(sdev))
 			goto out_cleanup_slave;
 	}
 
 	if (get_device(&sdev->host->shost_gendev) == NULL ||
 	    scsi_sysfs_device_initialize(sdev) != 0)
-		goto out_cleanup_transport;
+		goto out_cleanup_slave;
 
 	/*
 	 * If there are any same target siblings, add this to the
@@ -292,9 +293,6 @@
 	spin_unlock_irqrestore(shost->host_lock, flags);
 	return sdev;
 
-out_cleanup_transport:
-	if (shost->transportt->cleanup)
-		shost->transportt->cleanup(sdev);
 out_cleanup_slave:
 	if (shost->hostt->slave_destroy)
 		shost->hostt->slave_destroy(sdev);
@@ -733,8 +731,6 @@
 	} else {
 		if (sdev->host->hostt->slave_destroy)
 			sdev->host->hostt->slave_destroy(sdev);
-		if (sdev->host->transportt->cleanup)
-			sdev->host->transportt->cleanup(sdev);
 		put_device(&sdev->sdev_gendev);
 	}
  out:
@@ -1293,7 +1289,5 @@
 
 	if (sdev->host->hostt->slave_destroy)
 		sdev->host->hostt->slave_destroy(sdev);
-	if (sdev->host->transportt->cleanup)
-		sdev->host->transportt->cleanup(sdev);
 	put_device(&sdev->sdev_gendev);
 }
diff -Nru a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
--- a/drivers/scsi/scsi_sysfs.c	Mon Sep  6 09:42:22 2004
+++ b/drivers/scsi/scsi_sysfs.c	Mon Sep  6 09:42:22 2004
@@ -160,14 +160,17 @@
 
 	spin_lock_irqsave(sdev->host->host_lock, flags);
 	/* If we're the last LUN on the target, destroy the target */
-	delete = (list_empty(&sdev->same_target_siblings) && parent);
+	delete = list_empty(&sdev->same_target_siblings);
 	list_del(&sdev->siblings);
 	list_del(&sdev->same_target_siblings);
 	list_del(&sdev->starved_entry);
 	spin_unlock_irqrestore(sdev->host->host_lock, flags);
 
-	if (delete) {
+	if (delete && parent) {
+		struct scsi_target *starget = to_scsi_target(parent);
 		device_del(parent);
+		if (starget->transport_classdev.class)
+			class_device_unregister(&starget->transport_classdev);
 		put_device(parent);
 	}
 	if (sdev->request_queue)
@@ -509,7 +512,7 @@
 	}
 
  	if (sdev->transport_classdev.class) {
- 		attrs = sdev->host->transportt->attrs;
+ 		attrs = sdev->host->transportt->device_attrs;
  		for (i = 0; attrs[i]; i++) {
  			error = class_device_create_file(&sdev->transport_classdev,
  							 attrs[i]);
@@ -553,8 +556,6 @@
 	scsi_device_set_state(sdev, SDEV_DEL);
 	if (sdev->host->hostt->slave_destroy)
 		sdev->host->hostt->slave_destroy(sdev);
-	if (sdev->host->transportt->cleanup)
-		sdev->host->transportt->cleanup(sdev);
 	put_device(&sdev->sdev_gendev);
 
 out:
@@ -641,6 +642,20 @@
 		}
 	}
 
+	if (shost->transportt->host_setup)
+		shost->transportt->host_setup(shost);
+
+	if (shost->transport_classdev.class) {
+		struct class_device_attribute **attrs =
+			shost->transportt->host_attrs;
+		for (i = 0; attrs[i]; i++) {
+ 			error = class_device_create_file(&shost->transport_classdev,
+ 							 attrs[i]);
+ 			if (error)
+				return error;
+ 		}
+ 	}
+
 	return 0;
 }
 
@@ -662,7 +677,7 @@
 
 	class_device_initialize(&sdev->transport_classdev);
 	sdev->transport_classdev.dev = &sdev->sdev_gendev;
-	sdev->transport_classdev.class = sdev->host->transportt->class;
+	sdev->transport_classdev.class = sdev->host->transportt->device_class;
 	snprintf(sdev->transport_classdev.class_id, BUS_ID_SIZE,
 		 "%d:%d:%d:%d", sdev->host->host_no,
 		 sdev->channel, sdev->id, sdev->lun);
@@ -674,7 +689,7 @@
 	struct scsi_target *starget = NULL;
 	struct scsi_device *sdev_sibling;
 	struct device *dev = NULL;
-	int error;
+	int error, create = 0;
 	unsigned long flags;
 
 	spin_lock_irqsave(sdev->host->host_lock, flags);
@@ -689,7 +704,9 @@
 		}
 	}
 	if (!starget) {
-		starget = kmalloc(sizeof(*starget), GFP_ATOMIC);
+		starget = kmalloc(sizeof(*starget) +
+				  sdev->host->transportt->target_size,
+				  GFP_ATOMIC);
 		if (!starget) {
 			printk(KERN_ERR "%s: allocation failure\n", __FUNCTION__);
 			spin_unlock_irqrestore(sdev->host->host_lock,
@@ -703,6 +720,7 @@
 		dev->release = scsi_target_dev_release;
 		sprintf(dev->bus_id, "target%d:%d:%d",
 			sdev->host->host_no, sdev->channel, sdev->id);
+		create = 1;
 	}
 	get_device(&starget->dev);
 	sdev->sdev_gendev.parent = &starget->dev;
@@ -711,6 +729,22 @@
 		printk(KERN_ERR "Target device_add failed\n");
 		return error;
 	}
+	if (create) {
+		if (sdev->host->transportt->target_setup)
+			sdev->host->transportt->target_setup(starget);
+		if (starget->transport_classdev.class) {
+			int i;
+			struct class_device_attribute **attrs =
+				sdev->host->transportt->target_attrs;
+			for (i = 0; attrs[i]; i++) {
+				error = class_device_create_file(&starget->transport_classdev,
+								 attrs[i]);
+				if (error)
+					return error;
+			}
+		}
+	}
+
 	return 0;
 }
 
diff -Nru a/drivers/scsi/scsi_transport_fc.c b/drivers/scsi/scsi_transport_fc.c
--- a/drivers/scsi/scsi_transport_fc.c	Mon Sep  6 09:42:22 2004
+++ b/drivers/scsi/scsi_transport_fc.c	Mon Sep  6 09:42:22 2004
@@ -156,10 +156,10 @@
 
 	memset(i, 0, sizeof(struct fc_internal));
 
-	i->t.attrs = &i->attrs[0];
-	i->t.class = &fc_transport_class;
-	i->t.setup = &fc_setup_transport_attrs;
-	i->t.size = sizeof(struct fc_transport_attrs);
+	i->t.device_attrs = &i->attrs[0];
+	i->t.device_class = &fc_transport_class;
+	i->t.device_setup = &fc_setup_transport_attrs;
+	i->t.device_size = sizeof(struct fc_transport_attrs);
 	i->f = ft;
 
 	SETUP_ATTRIBUTE_RD(port_id);
diff -Nru a/drivers/scsi/scsi_transport_spi.c b/drivers/scsi/scsi_transport_spi.c
--- a/drivers/scsi/scsi_transport_spi.c	Mon Sep  6 09:42:22 2004
+++ b/drivers/scsi/scsi_transport_spi.c	Mon Sep  6 09:42:22 2004
@@ -666,10 +666,10 @@
 	memset(i, 0, sizeof(struct spi_internal));
 

-	i->t.attrs = &i->attrs[0];
-	i->t.class = &spi_transport_class;
-	i->t.setup = &spi_setup_transport_attrs;
-	i->t.size = sizeof(struct spi_transport_attrs);
+	i->t.device_attrs = &i->attrs[0];
+	i->t.device_class = &spi_transport_class;
+	i->t.device_setup = &spi_setup_transport_attrs;
+	i->t.device_size = sizeof(struct spi_transport_attrs);
 	i->f = ft;
 
 	SETUP_ATTRIBUTE(period);
diff -Nru a/include/scsi/scsi_host.h b/include/scsi/scsi_host.h
--- a/include/scsi/scsi_host.h	Mon Sep  6 09:42:22 2004
+++ b/include/scsi/scsi_host.h	Mon Sep  6 09:42:22 2004
@@ -511,6 +511,13 @@
 	struct list_head sht_legacy_list;
 
 	/*
+	 * Points to the transport data (if any) which is allocated
+	 * separately
+	 */
+	void *transport_data;
+	struct class_device transport_classdev;
+
+	/*
 	 * We should ensure that this is aligned, both for better performance
 	 * and also because some compilers (m68k) don't automatically force
 	 * alignment to a long boundary.
@@ -522,6 +529,9 @@
 	container_of(d, struct Scsi_Host, shost_gendev)
 #define		class_to_shost(d)	\
 	container_of(d, struct Scsi_Host, shost_classdev)
+#define		transport_classdev_to_shost(class_dev) \
+	container_of(class_dev, struct Scsi_Host, transport_classdev)
+
 
 extern struct Scsi_Host *scsi_host_alloc(struct scsi_host_template *, int);
 extern int scsi_add_host(struct Scsi_Host *, struct device *);
diff -Nru a/include/scsi/scsi_transport.h b/include/scsi/scsi_transport.h
--- a/include/scsi/scsi_transport.h	Mon Sep  6 09:42:22 2004
+++ b/include/scsi/scsi_transport.h	Mon Sep  6 09:42:22 2004
@@ -24,18 +24,26 @@
 	/* The NULL terminated list of transport attributes
 	 * that should be exported.
 	 */
-	struct class_device_attribute **attrs;
+	struct class_device_attribute **device_attrs;
+	struct class_device_attribute **target_attrs;
+	struct class_device_attribute **host_attrs;
+
 
 	/* The transport class that the device is in */
-	struct class *class;
+	struct class *device_class;
+	struct class *target_class;
+	struct class *host_class;
+
+	/* Constructor functions */
+	int (*device_setup)(struct scsi_device *);
+	int (*target_setup)(struct scsi_target *);
+	int (*host_setup)(struct Scsi_Host *);
 
-	/* Constructor/Destructor functions */
-	int (* setup)(struct scsi_device *);
-	void (* cleanup)(struct scsi_device *);
 	/* The size of the specific transport attribute structure (a
 	 * space of this size will be left at the end of the
-	 * scsi_device structure */
-	int	size;
+	 * scsi_* structure */
+	int	device_size;
+	int	target_size;
 };
 
 #endif /* SCSI_TRANSPORT_H */

RE: [RFC] Allow transport classes to attach to host, target or device

[James.Smart]

FYI - I've been prototyping host and target attributes in the fc transport using a 9/12 snapshot of the scsi-target-2.6 tree.

I have one issue that I would hope someone with more experience in the underlying class code can help with.

Here's a summary of what's happening...
- If I insmod a test lldd, which attaches a host, passes discovery, and finds 1 device - I can see the classes, and all the attribute files in sysfs. If I never attempt to access the sysfs attributes, I can rmmod the test lldd fine.
- If I repeat, I can access all the attributes in the sdev and starget classes, and I can rmmod the test lldd fine.
- If I repeat, and then access only a host attribute (which works fine), the subsequent rrmod encounters the oops described below.

In trying repeated scenarios, I have the following observations. I have 3 rd only attributes and 1 rdwr attribute on the host. I can access the rdwr attribute all I want and rmmod with no problems. If I access one of the rd only attributes, the rmmod fails. Other than the rd vs rdwr distinction, there is no other real difference. Also - other than where the attributes come from (shost, starget, or sdev), there is no difference in declaration or use of the attributes in the different classes.

Any help in tracking this down further is appreciated. I can forward the patch of the prototype for those that want to look at it.

-- james


The oops is:

>> A NULL pointer dereference in list_del_init() in class_device_del()

Unable to handle kernel NULL pointer dereference at virtual address 00000000
 printing eip:
c01f46fd
*pde = 00000000
Oops: 0002 [#1]
Modules linked in: lpfc scsi_transport_fc autofs4 sunrpc pcnet32 mii ipt_REJECT iptable_filter ip_tables vmhgfs floppy sg microcode uhci_hcd usbcore button battery asus_acpi ac ext3 jbd BusLogic sd_mod scsi_mod
CPU:    0
EIP:    0060:[<c01f46fd>]    Tainted: P   VLI
EFLAGS: 00010246   (2.6.9-rc1) 
EIP is at class_device_del+0x3d/0xc0
eax: f0e1d2c3   ebx: c21d6ddc   ecx: 00000000   edx: 00000000
esi: c21d6ddc   edi: cc91a340   ebp: c9c9e000   esp: c9c9ff3c
ds: 007b   es: 007b   ss: 0068
Process rmmod (pid: 3537, threadinfo=c9c9e000 task=ca094560)
Stack: cc91a38c c21d6ddc c02ddea0 00000000 c9c9e000 c01f4788 c21d6c00 cc9bb14c 
       c21d6c00 cc91c440 cc91c5b0 cc91d200 c012fafd 00000000 6366706c 00000000 
       ca0d2dc0 c0d6d6b4 c014643a b7fea000 b7feb000 c0146769 b7fea000 b7feb000 
Call Trace:
 [<c01f4788>] class_device_unregister+0x8/0x10
 [<cc9bb14c>] scsi_remove_host+0x4c/0x50 [scsi_mod]
 [<cc91c440>] lpfc_exit+0x30/0x55 [lpfc]
 [<c012fafd>] sys_delete_module+0x12d/0x170
 [<c014643a>] unmap_vma_list+0x1a/0x30
 [<c0146769>] do_munmap+0x109/0x150
 [<c01178e0>] do_page_fault+0x0/0x54c
 [<c0105e1d>] sysenter_past_esp+0x52/0x71
Code: 4c ba 42 00 00 00 89 04 24 b8 a0 90 2a c0 e8 2b 5f f2 ff ba 01 00 ff ff 8b 04 24 0f c1 10 85 d2 0f 85 b4 02 00 00 8b 06 8b 56 04 <89> 02 89 36 8b 5f 60 89 50 04 89 76 04 8b 03 0f 18 00 90 8d 6f