Transport Attributes -- attempt#2

Transport Attributes -- attempt#2

[Martin Hicks]

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


Hi,

Here's my next whack at export Transport-specific attributes to sysfs.

This version has the following changes.  The person who suggested the
change is quoted in brackets.

-Stuck the transport attributes and trancport class pointer into the
 host template. (hch)

-Separated the two classes into their own .c files. (jejb)

-Removed attribute overloading. (jejb)

The number of changes that must be done in the host driver are much
smaller now, and I think the whole idea is a little cleaner.  Removing
the attribute overloading really cleaned things up a lot.

There are three patches attached.  The core patch, and two driver
patches (qla2xxx and qla1280).

Opinions or other comments are welcome.
mh

-- 
Martin Hicks                Wild Open Source Inc.
mort@wildopensource.com     613-266-2296

[-- Attachment #2: scsi-transport-class-attr-core-v3-3.diff --]
[-- Type: text/plain, Size: 20570 bytes --]

# This is a BitKeeper generated patch for the following project:
# Project Name: Linux kernel tree
# This patch format is intended for GNU patch command version 2.5 or higher.
# This patch includes the following deltas:
#	           ChangeSet	1.1520  -> 1.1521 
#	include/scsi/scsi_device.h	1.11    -> 1.12   
#	include/scsi/scsi_host.h	1.13    -> 1.14   
#	drivers/scsi/scsi_sysfs.c	1.38    -> 1.39   
#	drivers/scsi/scsi_scan.c	1.114   -> 1.115  
#	drivers/scsi/Makefile	1.50    -> 1.51   
#	drivers/scsi/Kconfig	1.47    -> 1.48   
#	               (new)	        -> 1.1     drivers/scsi/scsi_transport.c
#	               (new)	        -> 1.1     include/scsi/scsi_transport.h
#	               (new)	        -> 1.1     include/scsi/scsi_transport_pscsi.h
#	               (new)	        -> 1.1     include/scsi/scsi_transport_fc.h
#	               (new)	        -> 1.1     drivers/scsi/scsi_transport_pscsi.c
#	               (new)	        -> 1.1     drivers/scsi/scsi_transport_fc.c
#
# The following is the BitKeeper ChangeSet Log
# --------------------------------------------
# 04/01/07	mort@tomahawk.engr.sgi.com	1.1521
# Add the transport attributes core.
# --------------------------------------------
#
diff -Nru a/drivers/scsi/Kconfig b/drivers/scsi/Kconfig
--- a/drivers/scsi/Kconfig	Wed Jan  7 10:39:14 2004
+++ b/drivers/scsi/Kconfig	Wed Jan  7 10:39:14 2004
@@ -196,6 +196,25 @@
 	  there should be no noticeable performance impact as long as you have
 	  logging turned off.
 
+menu "SCSI Transport Attributes (EXPERIMENTAL)"
+	depends on EXPERIMENTAL && SCSI!=n
+
+config SCSI_PSCSI_ATTRS
+	bool "Parallel SCSI Transport Attributes"
+	depends on SCSI
+	help
+	  If you wish to export transport-specific information about
+	  each attached SCSI device to sysfs, say Y.  Otherwise, say N.
+
+config SCSI_FC_ATTRS
+	bool "FiberChannel Transport Attributes"
+	depends on SCSI
+	help
+	  If you wish to export transport-specific information about
+	  each attached FiberChannel device to sysfs, say Y.
+	  Otherwise, say N.
+
+endmenu
 
 menu "SCSI low-level drivers"
 	depends on SCSI!=n
diff -Nru a/drivers/scsi/Makefile b/drivers/scsi/Makefile
--- a/drivers/scsi/Makefile	Wed Jan  7 10:39:14 2004
+++ b/drivers/scsi/Makefile	Wed Jan  7 10:39:14 2004
@@ -127,14 +127,17 @@
 obj-$(CONFIG_BLK_DEV_SR)	+= sr_mod.o
 obj-$(CONFIG_CHR_DEV_SG)	+= sg.o
 
+obj-$(CONFIG_SCSI_PSCSI_ATTRS)	+= scsi_transport_pscsi.o
+obj-$(CONFIG_SCSI_FC_ATTRS)	+= scsi_transport_fc.o
+
 scsi_mod-y			+= scsi.o hosts.o scsi_ioctl.o constants.o \
 				   scsicam.o scsi_error.o scsi_lib.o \
 				   scsi_scan.o scsi_syms.o scsi_sysfs.o \
-				   scsi_devinfo.o
+				   scsi_devinfo.o scsi_transport.o
 scsi_mod-$(CONFIG_SYSCTL)	+= scsi_sysctl.o
 scsi_mod-$(CONFIG_SCSI_PROC_FS)	+= scsi_proc.o
 scsi_mod-$(CONFIG_X86_PC9800)	+= scsi_pc98.o
-			
+
 sd_mod-objs	:= sd.o
 sr_mod-objs	:= sr.o sr_ioctl.o sr_vendor.o
 initio-objs	:= ini9100u.o i91uscsi.o
diff -Nru a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
--- a/drivers/scsi/scsi_scan.c	Wed Jan  7 10:39:14 2004
+++ b/drivers/scsi/scsi_scan.c	Wed Jan  7 10:39:14 2004
@@ -238,7 +238,6 @@
 	}
 
 	if (get_device(&sdev->host->shost_gendev)) {
-
 		device_initialize(&sdev->sdev_gendev);
 		sdev->sdev_gendev.parent = &sdev->host->shost_gendev;
 		sdev->sdev_gendev.bus = &scsi_bus_type;
@@ -251,6 +250,13 @@
 		sdev->sdev_classdev.dev = &sdev->sdev_gendev;
 		sdev->sdev_classdev.class = &sdev_class;
 		snprintf(sdev->sdev_classdev.class_id, BUS_ID_SIZE,
+			 "%d:%d:%d:%d", sdev->host->host_no,
+			 sdev->channel, sdev->id, sdev->lun);
+
+		class_device_initialize(&sdev->transport_classdev);
+		sdev->transport_classdev.dev = &sdev->sdev_gendev;
+		sdev->transport_classdev.class = sdev->host->hostt->transport_class;
+		snprintf(sdev->transport_classdev.class_id, BUS_ID_SIZE,
 			 "%d:%d:%d:%d", sdev->host->host_no,
 			 sdev->channel, sdev->id, sdev->lun);
 	} else
diff -Nru a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
--- a/drivers/scsi/scsi_sysfs.c	Wed Jan  7 10:39:14 2004
+++ b/drivers/scsi/scsi_sysfs.c	Wed Jan  7 10:39:14 2004
@@ -13,6 +13,9 @@
 #include <linux/device.h>
 
 #include <scsi/scsi_host.h>
+#include <scsi/scsi_transport.h>
+#include <scsi/scsi_transport_pscsi.h>
+#include <scsi/scsi_transport_fc.h>
 #include "scsi.h"
 
 #include "scsi_priv.h"
@@ -162,13 +165,41 @@
 	int error;
 
 	error = bus_register(&scsi_bus_type);
-	if (!error) {
-		error = class_register(&sdev_class);
-		if (error)
-			bus_unregister(&scsi_bus_type);
-	}
+	if (error)
+		return error;
+
+	error = class_register(&sdev_class);
+	if (error)
+		goto undo_bus;
+
+#ifdef CONFIG_SCSI_PSCSI_ATTRS
+	error = scsi_pscsi_transport_init();
+	if (error)
+		goto undo_sdev;
+#endif
+#ifdef CONFIG_SCSI_FC_ATTRS
+	error = scsi_fc_transport_init();
+	if (error)
+		goto undo_all;
+#endif
 
+ out:
 	return error;
+
+	/* Prevent "unused label" warnings when either of the
+	 * transport attributes config options are turned off. */
+	goto undo_all;
+	goto undo_sdev;
+
+ undo_all:
+#ifdef CONFIG_SCSI_PSCSI_ATTRS
+	scsi_pscsi_transport_exit();
+#endif
+ undo_sdev:
+	class_unregister(&sdev_class);
+ undo_bus:
+	bus_unregister(&scsi_bus_type);
+	goto out;
 }
 
 void scsi_sysfs_unregister(void)
@@ -364,6 +395,12 @@
 		return error;
 	}
 
+	if (sdev->transport_classdev.class) {
+		error = class_device_add(&sdev->transport_classdev);
+		if (error)
+			goto clean_device2;
+	}
+
 	get_device(&sdev->sdev_gendev);
 
 	if (sdev->host->hostt->sdev_attrs) {
@@ -385,9 +422,17 @@
 		}
 	}
 
+	if (sdev->transport_classdev.class) {
+		error = scsi_add_transport_attributes(sdev);
+		if (error)
+			scsi_remove_device(sdev);
+	}
+
 	return error;
 
-clean_device:
+ clean_device2:
+	class_device_del(&sdev->sdev_classdev);
+ clean_device:
 	sdev->sdev_state = SDEV_CANCEL;
 
 	device_del(&sdev->sdev_gendev);
diff -Nru a/drivers/scsi/scsi_transport.c b/drivers/scsi/scsi_transport.c
--- /dev/null	Wed Dec 31 16:00:00 1969
+++ b/drivers/scsi/scsi_transport.c	Wed Jan  7 10:39:14 2004
@@ -0,0 +1,39 @@
+/* 
+ *  Transport specific attributes exported to sysfs.
+ *
+ *  Copyright (c) 2003 Silicon Graphics, Inc.  All rights reserved.
+ *
+ *  This program is free software; you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License as published by
+ *  the Free Software Foundation; either version 2 of the License, or
+ *  (at your option) any later version.
+ *
+ *  This program is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License
+ *  along with this program; if not, write to the Free Software
+ *  Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
+ */
+
+#include <linux/config.h>
+#include <scsi/scsi_device.h>
+#include <scsi/scsi_host.h>
+#include <scsi/scsi_transport_pscsi.h>
+#include <scsi/scsi_transport_fc.h>
+
+int scsi_add_transport_attributes(struct scsi_device *sdev)
+{
+	int error, i;
+	struct class_device_attribute **attrs = sdev->host->hostt->transport_attrs;
+
+	for (i = 0; attrs[i]; i++) {
+                error = class_device_create_file(&sdev->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
--- /dev/null	Wed Dec 31 16:00:00 1969
+++ b/drivers/scsi/scsi_transport_fc.c	Wed Jan  7 10:39:14 2004
@@ -0,0 +1,108 @@
+/* 
+ *  FiberChannel transport specific attributes exported to sysfs.
+ *
+ *  Copyright (c) 2003 Silicon Graphics, Inc.  All rights reserved.
+ *
+ *  This program is free software; you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License as published by
+ *  the Free Software Foundation; either version 2 of the License, or
+ *  (at your option) any later version.
+ *
+ *  This program is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License
+ *  along with this program; if not, write to the Free Software
+ *  Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
+ */
+
+#include <scsi/scsi_device.h>
+#include <scsi/scsi_host.h>
+#include <scsi/scsi_transport.h>
+#include <scsi/scsi_transport_fc.h>
+
+struct class fc_transport_class = {
+	.name = "fc_transport",
+	.release = transport_class_release,
+};
+
+
+#define fc_transport_show_function(field, format_string, cast)			\
+static ssize_t									\
+show_fc_transport_##field (struct class_device *cdev, char *buf)		\
+{										\
+	struct scsi_device *sdev = transport_class_to_sdev(cdev);		\
+	struct fc_transport_attrs *tp;						\
+	tp = (struct fc_transport_attrs *)sdev->transport_attr_values;		\
+	return snprintf(buf, 20, format_string, cast tp->field);		\
+}
+
+#define fc_transport_rd_attr(field, format_string)				\
+	fc_transport_show_function(field, format_string, )			\
+static CLASS_DEVICE_ATTR( field, S_IRUGO, show_fc_transport_##field, NULL)
+
+#define fc_transport_rd_attr_cast(field, format_string, cast)			\
+	fc_transport_show_function(field, format_string, (cast))		\
+static CLASS_DEVICE_ATTR( field, S_IRUGO, show_fc_transport_##field, NULL)
+
+
+/* the FiberChannel Tranport Attributes: */
+fc_transport_rd_attr_cast(node_name, "0x%llx\n", unsigned long long);
+fc_transport_rd_attr_cast(port_name, "0x%llx\n", unsigned long long);
+fc_transport_rd_attr(port_id, "0x%x\n");
+
+struct class_device_attribute *fc_transport_attrs[] = {
+	&class_device_attr_node_name,
+	&class_device_attr_port_name,
+	&class_device_attr_port_id,
+	NULL
+};
+
+
+int scsi_fc_transport_init(void)
+{
+	return class_register(&fc_transport_class);
+}
+
+void scsi_fc_tranport_exit(void)
+{
+	class_unregister(&fc_transport_class);
+}
+
+void scsi_sysfs_cleanup_fc_transport(struct scsi_device *sdev)
+{
+	kfree(sdev->transport_attr_values);
+	put_device(&sdev->host->shost_gendev);
+}
+
+/* Allocates the struct to place the attribute values in.
+ * Usually called from the scsi host driver's slave_alloc
+ * function.
+ */
+int fc_alloc_transport_attrs(struct scsi_device *sdev)
+{
+	struct fc_transport_attrs *ptr;
+
+	ptr = (struct fc_transport_attrs *)
+		kmalloc(sizeof(struct fc_transport_attrs),
+			GFP_KERNEL);
+	if (!ptr)
+		return -ENOMEM;
+
+	INIT_FC_TRANSPORT(ptr);
+	sdev->transport_attr_values = ptr;
+
+	return 0;
+}
+
+/* Cleans up after fc_alloc_transport_attrs().
+ * Usually called from the scsi host driver's slave_destroy
+ * function
+ */
+void fc_destroy_transport_attrs(struct scsi_device *sdev)
+{
+	kfree(sdev->transport_attr_values);
+}
+
diff -Nru a/drivers/scsi/scsi_transport_pscsi.c b/drivers/scsi/scsi_transport_pscsi.c
--- /dev/null	Wed Dec 31 16:00:00 1969
+++ b/drivers/scsi/scsi_transport_pscsi.c	Wed Jan  7 10:39:14 2004
@@ -0,0 +1,104 @@
+/* 
+ *  Parallel SCSI transport specific attributes exported to sysfs.
+ *
+ *  Copyright (c) 2003 Silicon Graphics, Inc.  All rights reserved.
+ *
+ *  This program is free software; you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License as published by
+ *  the Free Software Foundation; either version 2 of the License, or
+ *  (at your option) any later version.
+ *
+ *  This program is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License
+ *  along with this program; if not, write to the Free Software
+ *  Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
+ */
+
+#include <scsi/scsi_device.h>
+#include <scsi/scsi_host.h>
+#include <scsi/scsi_transport.h>
+#include <scsi/scsi_transport_pscsi.h>
+
+struct class pscsi_transport_class = {
+	.name = "pscsi_transport",
+	.release = transport_class_release,
+};
+
+
+#define pscsi_transport_show_function(field, format_string)			\
+static ssize_t									\
+show_pscsi_transport_##field (struct class_device *cdev, char *buf)		\
+{										\
+	struct scsi_device *sdev = transport_class_to_sdev(cdev);		\
+	struct pscsi_transport_attrs *tp;					\
+	tp = (struct pscsi_transport_attrs *)sdev->transport_attr_values;	\
+	return snprintf(buf, 20, format_string, tp->field);			\
+}
+
+#define pscsi_transport_rd_attr(field, format_string)				\
+	pscsi_transport_show_function(field, format_string)			\
+static CLASS_DEVICE_ATTR( field, S_IRUGO, show_pscsi_transport_##field, NULL)
+
+
+/* The Parallel SCSI Tranport Attributes: */
+pscsi_transport_rd_attr(period, "%d\n");
+pscsi_transport_rd_attr(offset, "%d\n");
+
+struct class_device_attribute *pscsi_transport_attrs[] = {
+	&class_device_attr_period,
+	&class_device_attr_offset,
+	NULL
+};
+
+
+int scsi_pscsi_transport_init(void)
+{
+	return class_register(&pscsi_transport_class);
+}
+
+void scsi_pscsi_transport_exit(void)
+{
+	class_unregister(&pscsi_transport_class);
+}
+
+
+void scsi_sysfs_cleanup_scsi_transport(struct scsi_device *sdev)
+{
+	kfree(sdev->transport_attr_values);
+	put_device(&sdev->host->shost_gendev);
+}
+
+
+/* Allocates the struct to place the attribute values in.
+ * Usually called from the scsi host driver's slave_alloc
+ * function.
+ */
+int pscsi_alloc_transport_attrs(struct scsi_device *sdev)
+{
+	struct pscsi_transport_attrs *ptr;
+
+	ptr = (struct pscsi_transport_attrs *)
+		kmalloc(sizeof(struct pscsi_transport_attrs),
+			GFP_KERNEL);
+	if (!ptr)
+		return -ENOMEM;
+
+	INIT_PSCSI_TRANSPORT(ptr);
+	sdev->transport_attr_values = ptr;
+
+	return 0;
+}
+
+
+/* Cleans up after fc_alloc_transport_attrs().
+ * Usually called from the scsi host driver's slave_destroy
+ * function
+ */
+void pscsi_destroy_transport_attrs(struct scsi_device *sdev)
+{
+	kfree(sdev->transport_attr_values);
+}
diff -Nru a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
--- a/include/scsi/scsi_device.h	Wed Jan  7 10:39:14 2004
+++ b/include/scsi/scsi_device.h	Wed Jan  7 10:39:14 2004
@@ -103,6 +103,9 @@
 	struct device		sdev_gendev;
 	struct class_device	sdev_classdev;
 
+	void *			transport_attr_values;
+	struct class_device	transport_classdev;
+
 	enum scsi_device_state sdev_state;
 };
 #define	to_scsi_device(d)	\
diff -Nru a/include/scsi/scsi_host.h b/include/scsi/scsi_host.h
--- a/include/scsi/scsi_host.h	Wed Jan  7 10:39:14 2004
+++ b/include/scsi/scsi_host.h	Wed Jan  7 10:39:14 2004
@@ -337,6 +337,12 @@
 	struct device_attribute **sdev_attrs;
 
 	/*
+	 * Pointers to the Transport attributes and the Transport class.
+	 */
+	struct class_device_attribute **transport_attrs;
+	struct class *transport_class;
+
+	/*
 	 * List of hosts per template.
 	 *
 	 * This is only for use by scsi_module.c for legacy templates.
diff -Nru a/include/scsi/scsi_transport.h b/include/scsi/scsi_transport.h
--- /dev/null	Wed Dec 31 16:00:00 1969
+++ b/include/scsi/scsi_transport.h	Wed Jan  7 10:39:14 2004
@@ -0,0 +1,39 @@
+/* 
+ *  Transport specific attributes exported to sysfs.
+ *
+ *  Copyright (c) 2003 Silicon Graphics, Inc.  All rights reserved.
+ *
+ *  This program is free software; you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License as published by
+ *  the Free Software Foundation; either version 2 of the License, or
+ *  (at your option) any later version.
+ *
+ *  This program is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License
+ *  along with this program; if not, write to the Free Software
+ *  Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
+ */
+#ifndef SCSI_TRANSPORT_H
+#define SCSI_TRANSPORT_H
+
+#include <scsi/scsi_device.h>
+#include <scsi/scsi_transport_pscsi.h>
+#include <scsi/scsi_transport_fc.h>
+
+#define transport_class_to_sdev(class_dev) \
+	container_of(class_dev, struct scsi_device, transport_classdev)
+
+static inline void transport_class_release(struct class_device *class_dev)
+{
+	struct scsi_device *sdev = transport_class_to_sdev(class_dev);
+	put_device(&sdev->sdev_gendev);
+}
+
+extern int scsi_add_transport_attributes(struct scsi_device *sdev);
+extern int scsi_export_transport_attributes(struct scsi_device *sdev);
+
+#endif /* SCSI_TRANSPORT_H */
diff -Nru a/include/scsi/scsi_transport_fc.h b/include/scsi/scsi_transport_fc.h
--- /dev/null	Wed Dec 31 16:00:00 1969
+++ b/include/scsi/scsi_transport_fc.h	Wed Jan  7 10:39:14 2004
@@ -0,0 +1,44 @@
+/* 
+ *  FiberChannel transport specific attributes exported to sysfs.
+ *
+ *  Copyright (c) 2003 Silicon Graphics, Inc.  All rights reserved.
+ *
+ *  This program is free software; you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License as published by
+ *  the Free Software Foundation; either version 2 of the License, or
+ *  (at your option) any later version.
+ *
+ *  This program is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License
+ *  along with this program; if not, write to the Free Software
+ *  Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
+ */
+#ifndef SCSI_TRANSPORT_FC_H
+#define SCSI_TRANSPORT_FC_H
+
+extern struct class fc_transport_class;
+extern struct class_device_attribute *fc_transport_attrs[];
+
+struct fc_transport_attrs {
+	int port_id;
+	uint64_t node_name;
+	uint64_t port_name;
+};
+
+#define INIT_FC_TRANSPORT(ptr) do {	\
+	(ptr)->port_id		= -1;	\
+	(ptr)->node_name	= -1;	\
+	(ptr)->port_name	= -1;	\
+} while (0)
+
+extern int scsi_fc_transport_init(void);
+extern void scsi_fc_transport_exit(void);
+extern int fc_alloc_transport_attrs(struct scsi_device *sdev);
+extern void fc_destroy_transport_attrs(struct scsi_device *sdev);
+extern int scsi_add_fc_transport_attributes(struct scsi_device *sdev);
+
+#endif /* SCSI_TRANSPORT_FC_H */
diff -Nru a/include/scsi/scsi_transport_pscsi.h b/include/scsi/scsi_transport_pscsi.h
--- /dev/null	Wed Dec 31 16:00:00 1969
+++ b/include/scsi/scsi_transport_pscsi.h	Wed Jan  7 10:39:14 2004
@@ -0,0 +1,42 @@
+/* 
+ *  Parallel SCSI transport specific attributes exported to sysfs.
+ *
+ *  Copyright (c) 2003 Silicon Graphics, Inc.  All rights reserved.
+ *
+ *  This program is free software; you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License as published by
+ *  the Free Software Foundation; either version 2 of the License, or
+ *  (at your option) any later version.
+ *
+ *  This program is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License
+ *  along with this program; if not, write to the Free Software
+ *  Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
+ */
+#ifndef SCSI_TRANSPORT_PSCSI_H
+#define SCSI_TRANSPORT_PSCSI_H
+
+extern struct class pscsi_transport_class;
+extern struct class_device_attribute *pscsi_transport_attrs[];
+
+struct pscsi_transport_attrs {
+	int period;
+	int offset;
+};
+
+#define INIT_PSCSI_TRANSPORT(ptr) do {	\
+	(ptr)->period		= -1;	\
+	(ptr)->offset		= -1;	\
+} while (0)
+
+extern int scsi_pscsi_transport_init(void);
+extern void scsi_pscsi_transport_exit(void);
+extern int pscsi_alloc_transport_attrs(struct scsi_device *sdev);
+extern void pscsi_destroy_transport_attrs(struct scsi_device *sdev);
+extern int scsi_add_pscsi_transport_attributes(struct scsi_device *sdev);
+
+#endif /* SCSI_TRANSPORT_PSCSI_H */

[-- Attachment #3: scsi-transport-class-attr-qla1280-v3-3.diff --]
[-- Type: text/plain, Size: 4398 bytes --]

# This is a BitKeeper generated patch for the following project:
# Project Name: Linux kernel tree
# This patch format is intended for GNU patch command version 2.5 or higher.
# This patch includes the following deltas:
#	           ChangeSet	1.1521  -> 1.1522 
#	drivers/scsi/Kconfig	1.48    -> 1.49   
#	drivers/scsi/qla1280.c	1.51    -> 1.52   
#
# The following is the BitKeeper ChangeSet Log
# --------------------------------------------
# 04/01/07	mort@tomahawk.engr.sgi.com	1.1522
# Export transport attributes from qla1280
# --------------------------------------------
#
diff -Nru a/drivers/scsi/Kconfig b/drivers/scsi/Kconfig
--- a/drivers/scsi/Kconfig	Wed Jan  7 08:33:58 2004
+++ b/drivers/scsi/Kconfig	Wed Jan  7 08:33:58 2004
@@ -1210,7 +1210,7 @@
 
 config SCSI_QLOGIC_1280
 	tristate "Qlogic QLA 1280 SCSI support"
-	depends on PCI && SCSI
+	depends on PCI && SCSI && SCSI_PSCSI_ATTRS
 	help
 	  Say Y if you have a QLogic ISP1x80/1x160 SCSI host adapter.
 
diff -Nru a/drivers/scsi/qla1280.c b/drivers/scsi/qla1280.c
--- a/drivers/scsi/qla1280.c	Wed Jan  7 08:33:58 2004
+++ b/drivers/scsi/qla1280.c	Wed Jan  7 08:33:58 2004
@@ -324,6 +324,8 @@
 
 #if LINUX_VERSION_CODE >= 0x020545
 #include <scsi/scsi_host.h>
+#include <scsi/scsi_transport.h>
+#include <scsi/scsi_transport_pscsi.h>
 #include "scsi.h"
 #else
 #include <linux/blk.h>
@@ -440,7 +442,9 @@
  */
 static void qla1280_done(struct scsi_qla_host *, struct srb **, struct srb **);
 static void qla1280_done_q_put(struct srb *, struct srb **, struct srb **);
+static int qla1280_slave_alloc(Scsi_Device *);
 static int qla1280_slave_configure(Scsi_Device *);
+static void qla1280_slave_destroy(Scsi_Device *);
 #if LINUX_VERSION_CODE < 0x020545
 static void qla1280_select_queue_depth(struct Scsi_Host *, Scsi_Device *);
 static void qla1280_get_target_options(struct scsi_cmnd *, struct scsi_qla_host *);
@@ -949,6 +953,9 @@
 	ha->instance = num_hosts;
 	host->unique_id = ha->instance;
 
+	host->hostt->transport_attrs = pscsi_transport_attrs;
+	host->hostt->transport_class = &pscsi_transport_class;
+
 	if (qla1280_pci_config(ha)) {
 		printk(KERN_INFO "qla1x160: Unable to configure PCI\n");
 		goto error_mem_alloced;
@@ -1670,6 +1677,17 @@
 	return status;
 }
 
+static int
+qla1280_slave_alloc(Scsi_Device *device)
+{
+	int error = 0;
+
+	error = pscsi_alloc_transport_attrs(device);
+	if (error)
+		return error;
+
+	return error;
+}
 
 /**************************************************************************
  *   qla1280_slave_configure
@@ -1686,12 +1704,14 @@
 qla1280_slave_configure(Scsi_Device *device)
 {
 	struct scsi_qla_host *ha;
+	struct pscsi_transport_attrs *attrs;
 	int default_depth = 3;
 	int bus = device->channel;
 	int target = device->id;
 	int status = 0;
 	struct nvram *nv;
 	unsigned long flags;
+	int is1x160;
 
 	ha = (struct scsi_qla_host *)device->host->hostdata;
 	nv = &ha->nvram;
@@ -1699,6 +1719,12 @@
 	if (qla1280_check_for_dead_scsi_bus(ha, bus))
 		return 1;
 
+	if (ha->device_id == PCI_DEVICE_ID_QLOGIC_ISP12160 ||
+	    ha->device_id == PCI_DEVICE_ID_QLOGIC_ISP10160)
+		is1x160 = 1;
+	else
+		is1x160 = 0;
+
 	if (device->tagged_supported &&
 	    (ha->bus_settings[bus].qtag_enables & (BIT_0 << target))) {
 		scsi_adjust_queue_depth(device, MSG_ORDERED_TAG,
@@ -1736,9 +1762,25 @@
 
 	qla12160_get_target_parameters(ha, device);
 	spin_unlock_irqrestore(HOST_LOCK, flags);
+
+	/* Set the parallel scsi transport attributes */
+	attrs = (struct pscsi_transport_attrs *)device->transport_attr_values;
+	attrs->period = nv->bus[bus].target[target].sync_period;
+	if (is1x160)
+		attrs->offset = nv->bus[bus].target[target].flags.
+			flags1x160.sync_offset;
+	else
+		attrs->offset = nv->bus[bus].target[target].flags.
+			flags1x80.sync_offset;
+
 	return status;
 }
 
+void qla1280_slave_destroy(Scsi_Device *device)
+{
+	pscsi_destroy_transport_attrs(device);
+}
+
 #if LINUX_VERSION_CODE < 0x020545
 /**************************************************************************
  *   qla1280_select_queue_depth
@@ -5145,7 +5187,9 @@
 	.info			= qla1280_info,
 	.queuecommand		= qla1280_queuecommand,
 #if LINUX_VERSION_CODE >= 0x020545
+	.slave_alloc		= qla1280_slave_alloc,
 	.slave_configure	= qla1280_slave_configure,
+	.slave_destroy		= qla1280_slave_destroy,
 #endif
 	.eh_abort_handler	= qla1280_eh_abort,
 	.eh_device_reset_handler= qla1280_eh_device_reset,

[-- Attachment #4: scsi-transport-class-attr-qla2xxx-v3-3.diff --]
[-- Type: text/plain, Size: 4851 bytes --]

# This is a BitKeeper generated patch for the following project:
# Project Name: Linux kernel tree
# This patch format is intended for GNU patch command version 2.5 or higher.
# This patch includes the following deltas:
#	           ChangeSet	1.1522  -> 1.1523 
#	drivers/scsi/qla2xxx/Kconfig	1.1     -> 1.2    
#	drivers/scsi/qla2xxx/qla_os.c	1.1     -> 1.2    
#	drivers/scsi/qla2xxx/qla_os.h	1.1     -> 1.2    
#
# The following is the BitKeeper ChangeSet Log
# --------------------------------------------
# 04/01/07	mort@tomahawk.engr.sgi.com	1.1523
# Export transport attributes from qla2xxx
# --------------------------------------------
#
diff -Nru a/drivers/scsi/qla2xxx/Kconfig b/drivers/scsi/qla2xxx/Kconfig
--- a/drivers/scsi/qla2xxx/Kconfig	Wed Jan  7 08:33:22 2004
+++ b/drivers/scsi/qla2xxx/Kconfig	Wed Jan  7 08:33:22 2004
@@ -1,6 +1,6 @@
 config SCSI_QLA2XXX
 	bool "QLogic ISP2xxx PCI/PCI-X Fibre Channel HBA Support"
-	depends on PCI && SCSI
+	depends on PCI && SCSI && SCSI_FC_ATTRS
 	---help---
 	These drivers support the QLogic 2xxx host adapter family of SCSI FCP
 	HBAs.
diff -Nru a/drivers/scsi/qla2xxx/qla_os.c b/drivers/scsi/qla2xxx/qla_os.c
--- a/drivers/scsi/qla2xxx/qla_os.c	Wed Jan  7 08:33:22 2004
+++ b/drivers/scsi/qla2xxx/qla_os.c	Wed Jan  7 08:33:22 2004
@@ -167,7 +167,9 @@
 /*
  * SCSI host template entry points 
  */
+static int qla2xxx_slave_alloc(struct scsi_device *device);
 static int qla2xxx_slave_configure(struct scsi_device * device);
+static void qla2xxx_slave_destroy(struct scsi_device *device);
 extern int qla2x00_ioctl(struct scsi_device *, int , void *);
 static int qla2xxx_eh_abort(struct scsi_cmnd *);
 static int qla2xxx_eh_device_reset(struct scsi_cmnd *);
@@ -191,7 +193,9 @@
 	.eh_bus_reset_handler	= qla2xxx_eh_bus_reset,
 	.eh_host_reset_handler	= qla2xxx_eh_host_reset,
 
+	.slave_alloc		= qla2xxx_slave_alloc,
 	.slave_configure	= qla2xxx_slave_configure,
+	.slave_destroy		= qla2xxx_slave_destroy,
 
 	.this_id		= -1,
 	.can_queue		= REQUEST_ENTRY_CNT+128,
@@ -1849,7 +1853,24 @@
 }
 
 /**************************************************************************
-* qla2x00_slave_configure
+ * qla2xxx_slave_alloc
+ *
+ * Description:
+ *************************************************************************/
+static int
+qla2xxx_slave_alloc(struct scsi_device *sdev)
+{
+	int error=0;
+
+	error = fc_alloc_transport_attrs(sdev);
+	if (error)
+		return error;
+
+	return error;
+}
+
+/**************************************************************************
+* qla2xxx_slave_configure
 *
 * Description:
 **************************************************************************/
@@ -1857,6 +1878,8 @@
 qla2xxx_slave_configure(struct scsi_device *sdev)
 {
 	scsi_qla_host_t *ha = to_qla_host(sdev->host);
+	struct fc_transport_attrs *attrs;
+	struct fc_port *fc;
 	int queue_depth;
 
 	if (IS_QLA2100(ha) || IS_QLA2200(ha))
@@ -1883,9 +1906,32 @@
 		     sdev->host->hostt->cmd_per_lun /* 3 */);
 	}
 
-	return (0);
+	attrs = (struct fc_transport_attrs *)sdev->transport_attr_values;
+	list_for_each_entry(fc, &ha->fcports, list) {
+		if (fc->os_target_id == sdev->id) {
+			attrs->port_name = __be64_to_cpu(*(uint64_t *)fc->port_name);
+			attrs->node_name = __be64_to_cpu(*(uint64_t *)fc->node_name);
+			attrs->port_id = fc->d_id.b24;
+			break;
+		}
+	}
+
+	return 0;
+}
+ 
+ 
+/**************************************************************************
+ * qla2xxx_slave_destroy
+ *
+ * Description:
+ *************************************************************************/
+static void
+qla2xxx_slave_destroy(struct scsi_device *sdev)
+{
+	fc_destroy_transport_attrs(sdev);
 }
 
+
 /**
  * qla2x00_config_dma_addressing() - Configure OS DMA addressing method.
  * @ha: HA context
@@ -1998,10 +2044,10 @@
 	ha->mmio_length = mmio_len;
 #endif
 
-	return (0);
+	return 0;
 
 iospace_error_exit:
-	return (-ENOMEM);
+	return -ENOMEM;
 }
 
 /*
@@ -2238,6 +2284,9 @@
 
 	sysfs_create_bin_file(&host->shost_gendev.kobj, &sysfs_fw_dump_attr);
 	sysfs_create_bin_file(&host->shost_gendev.kobj, &sysfs_nvram_attr);
+
+	host->hostt->transport_attrs = fc_transport_attrs;
+	host->hostt->transport_class = &fc_transport_class;
 
 	qla_printk(KERN_INFO, ha, "\n"
 	    " QLogic ISP2xxx PCI/PCI-X Fibre Channel HBA Driver: %s\n"
diff -Nru a/drivers/scsi/qla2xxx/qla_os.h b/drivers/scsi/qla2xxx/qla_os.h
--- a/drivers/scsi/qla2xxx/qla_os.h	Wed Jan  7 08:33:22 2004
+++ b/drivers/scsi/qla2xxx/qla_os.h	Wed Jan  7 08:33:22 2004
@@ -56,6 +56,7 @@
 #include <linux/module.h>
 #include <linux/version.h>
 #include <linux/init.h>
+#include <linux/list.h>
 #include <linux/string.h>
 #include <linux/errno.h>
 #include <linux/kernel.h>
@@ -91,7 +92,7 @@
 
 #include "scsi.h"
 #include "hosts.h"
-
+#include <scsi/scsi_transport.h>
 #include <scsi/scsicam.h>
 #include <scsi/scsi_ioctl.h>
 

Re: Transport Attributes -- attempt#2

[Christoph Hellwig]

+config SCSI_PSCSI_ATTRS
+	bool "Parallel SCSI Transport Attributes"

>> Scan we replacte the PSCSI/pscsi in identifiers with SPI/spi as used in
>> the t10 standards documents please?  This is a bit shorter and the correct
>> name.  In user-visible string Parallel SCSI is fine with me, but
>> maybe it should rather be

	bool "Parallel SCSI (SPI) Transport Attributes"

 
+obj-$(CONFIG_SCSI_PSCSI_ATTRS)	+= scsi_transport_pscsi.o
+obj-$(CONFIG_SCSI_FC_ATTRS)	+= scsi_transport_fc.o

>> This is wrong.  Currently the option are bool and you'd build them into
>> the kernel even if the scsi code is modular.  Either use scsi_mod-*
>> or make them their own modules using tristate (I still think that's
>> overkill just for the attributes, but jejb seems to like it)

 		snprintf(sdev->sdev_classdev.class_id, BUS_ID_SIZE,
+			 "%d:%d:%d:%d", sdev->host->host_no,
+			 sdev->channel, sdev->id, sdev->lun);
+
+		class_device_initialize(&sdev->transport_classdev);
+		sdev->transport_classdev.dev = &sdev->sdev_gendev;
+		sdev->transport_classdev.class = sdev->host->hostt->transport_class;
+		snprintf(sdev->transport_classdev.class_id, BUS_ID_SIZE,

>> Out of curiosity:  Why do we need _another_ classdev.  Can't you
>> reuse the current one?

+	error = class_register(&sdev_class);
+	if (error)
+		goto undo_bus;
+
+#ifdef CONFIG_SCSI_PSCSI_ATTRS
+	error = scsi_pscsi_transport_init();
+	if (error)
+		goto undo_sdev;
+#endif
+#ifdef CONFIG_SCSI_FC_ATTRS
+	error = scsi_fc_transport_init();
+	if (error)
+		goto undo_all;
+#endif

> Please put stubs in the header instead of ifdefs in the code.
> (And 1:0 for jejb, with the moular approach this would just go away :))
 
+ out:
 	return error;
+
+	/* Prevent "unused label" warnings when either of the
+	 * transport attributes config options are turned off. */
+	goto undo_all;
+	goto undo_sdev;

>> Yikes!

+int scsi_add_transport_attributes(struct scsi_device *sdev)
+{
+	int error, i;
+	struct class_device_attribute **attrs = sdev->host->hostt->transport_attrs;
+
+	for (i = 0; attrs[i]; i++) {
+                error = class_device_create_file(&sdev->transport_classdev,
+                                                 attrs[i]);
+                if (error)
+                        return error;
+        }
+        return 0;
+}

Please merge this into scsi_sysfs.c, no need for another file.

+	struct fc_transport_attrs *ptr;
+
+	ptr = (struct fc_transport_attrs *)
+		kmalloc(sizeof(struct fc_transport_attrs),
+			GFP_KERNEL);

>> no need to cast here, this is not C++ (fortunately..)

+	INIT_FC_TRANSPORT(ptr);

>> this is just obsfucation, please kill the macro.

+	ptr = (struct pscsi_transport_attrs *)
+		kmalloc(sizeof(struct pscsi_transport_attrs),
+			GFP_KERNEL);
+	if (!ptr)
+		return -ENOMEM;
+
+	INIT_PSCSI_TRANSPORT(ptr);

>> same comments as above

+	void *			transport_attr_values;

>>	void			*transport_attr_values;

+#define transport_class_to_sdev(class_dev) \
+	container_of(class_dev, struct scsi_device, transport_classdev)

>> should just go to scsi_device.h

+
+static inline void transport_class_release(struct class_device *class_dev)
+{
+	struct scsi_device *sdev = transport_class_to_sdev(class_dev);
+	put_device(&sdev->sdev_gendev);
+}

>> this is callback, thus the inline doesn't make sense at all.  just
>> give spi and fc their own copy - that scales better for possible future
>> additions anyway

+extern int scsi_add_transport_attributes(struct scsi_device *sdev);

>> this is internal to the scsi midlayer, thus should go to scsi_priv.h
>> (or be static to scsi_sysfs.c as suggested above)

+extern int scsi_export_transport_attributes(struct scsi_device *sdev);

>> doesn't seem to exist at all..


+	host->hostt->transport_attrs = pscsi_transport_attrs;
+	host->hostt->transport_class = &pscsi_transport_class;

>> Please set them _directrly_ in the host template. 
>> Currently you're writing to the host template for every found adapter..

+static int
+qla1280_slave_alloc(Scsi_Device *device)

>> should be struct scsi_device *sdev

+{
+	int error = 0;
+
+	error = pscsi_alloc_transport_attrs(device);
+	if (error)
+		return error;
+
+	return error;
+}

>> Okay, this is ugly as hell, as every driver needs to duplicate this
>> verbosely.  Better have a scsi_set_{spi,fc}_transport function that
>> can be called in module_init and then does not only set the calls
>> and attributes (so they don't need to exposed anymore but also a
>> default constructor for the attributes.

+void qla1280_slave_destroy(Scsi_Device *device)
+{
+	pscsi_destroy_transport_attrs(device);
+}

>> I don't think the _destroy_transport_attrs call makes sense,
>> just kfree the transport_attrs field in the core code uncondititionally.
>> If you plan more complex destructors for some reason put a destructor
>> in the host teplate, similar to the constructore mentioned above.

In fact after all the above suggestions it might be useful to have
a struct scsi_transport_attribute instead of stuffing all that into
the host template (sorry for guiding you into that direction).  The most
logical interface to set it would be to have a third argument for
scsi_host_alloc, but we can't break that interface now, so we should
probably add scsi_set_transport(struct Scsi_host *,
struct scsi_transport_template *), then and the driver will do in it's probe
rountine, somewhere between scsi_host_alloc and scsi_add_host a
scsi_set_transport(shost, &scsi_spi_template);

Re: Transport Attributes -- attempt#2

[Martin Hicks]

On Thu, Jan 08, 2004 at 01:17:17PM +0000, Christoph Hellwig wrote:
> +config SCSI_PSCSI_ATTRS
> +	bool "Parallel SCSI Transport Attributes"
> 
> >> Scan we replacte the PSCSI/pscsi in identifiers with SPI/spi as used in
> >> the t10 standards documents please?  This is a bit shorter and the correct
> >> name.  In user-visible string Parallel SCSI is fine with me, but
> >> maybe it should rather be
> 
> 	bool "Parallel SCSI (SPI) Transport Attributes"

okay.

> 
>  
> +obj-$(CONFIG_SCSI_PSCSI_ATTRS)	+= scsi_transport_pscsi.o
> +obj-$(CONFIG_SCSI_FC_ATTRS)	+= scsi_transport_fc.o
> 
> >> This is wrong.  Currently the option are bool and you'd build them into
> >> the kernel even if the scsi code is modular.  Either use scsi_mod-*
> >> or make them their own modules using tristate (I still think that's
> >> overkill just for the attributes, but jejb seems to like it)
> 

Agreed.  I had the options as tristate inititally, then changed them
to bool.  I missed changing this stuff.

>  		snprintf(sdev->sdev_classdev.class_id, BUS_ID_SIZE,
> +			 "%d:%d:%d:%d", sdev->host->host_no,
> +			 sdev->channel, sdev->id, sdev->lun);
> +
> +		class_device_initialize(&sdev->transport_classdev);
> +		sdev->transport_classdev.dev = &sdev->sdev_gendev;
> +		sdev->transport_classdev.class = sdev->host->hostt->transport_class;
> +		snprintf(sdev->transport_classdev.class_id, BUS_ID_SIZE,
> 
> >> Out of curiosity:  Why do we need _another_ classdev.  Can't you
> >> reuse the current one?

It guess we could re-use the scsi_device classdev.  This would mean that
all the attribute files would show up in /sys/class/scsi_device/w:x:y:z.
We would have to export an attribute called "transport" to tell which
transport the device is using.  I'm agreeable to this solution.  I don't
much like having an fc_transport and pscsi_transport directory in
/sys/class.

> 
> +	error = class_register(&sdev_class);
> +	if (error)
> +		goto undo_bus;
> +
> +#ifdef CONFIG_SCSI_PSCSI_ATTRS
> +	error = scsi_pscsi_transport_init();
> +	if (error)
> +		goto undo_sdev;
> +#endif
> +#ifdef CONFIG_SCSI_FC_ATTRS
> +	error = scsi_fc_transport_init();
> +	if (error)
> +		goto undo_all;
> +#endif
> 
> > Please put stubs in the header instead of ifdefs in the code.
> > (And 1:0 for jejb, with the moular approach this would just go away :))
>  

okay.

> + out:
>  	return error;
> +
> +	/* Prevent "unused label" warnings when either of the
> +	 * transport attributes config options are turned off. */
> +	goto undo_all;
> +	goto undo_sdev;
> 
> >> Yikes!

Agreed.

> 
> +extern int scsi_add_transport_attributes(struct scsi_device *sdev);
> 
> >> this is internal to the scsi midlayer, thus should go to scsi_priv.h
> >> (or be static to scsi_sysfs.c as suggested above)

Okay.  I wasn't sure where to put such functions.

> 
> +extern int scsi_export_transport_attributes(struct scsi_device *sdev);
> 
> >> doesn't seem to exist at all..

missed that during a cleanup. Thanks.

> +	host->hostt->transport_attrs = pscsi_transport_attrs;
> +	host->hostt->transport_class = &pscsi_transport_class;
> 
> >> Please set them _directrly_ in the host template. 
> >> Currently you're writing to the host template for every found adapter..
> 
> +static int
> +qla1280_slave_alloc(Scsi_Device *device)
> 
> >> should be struct scsi_device *sdev

I agree.  I'm just following what the rest of the driver does.

> 
> +{
> +	int error = 0;
> +
> +	error = pscsi_alloc_transport_attrs(device);
> +	if (error)
> +		return error;
> +
> +	return error;
> +}
> 
> >> Okay, this is ugly as hell, as every driver needs to duplicate this
> >> verbosely.  Better have a scsi_set_{spi,fc}_transport function that
> >> can be called in module_init and then does not only set the calls
> >> and attributes (so they don't need to exposed anymore but also a
> >> default constructor for the attributes.
> 
> +void qla1280_slave_destroy(Scsi_Device *device)
> +{
> +	pscsi_destroy_transport_attrs(device);
> +}
> 
> >> I don't think the _destroy_transport_attrs call makes sense,
> >> just kfree the transport_attrs field in the core code uncondititionally.
> >> If you plan more complex destructors for some reason put a destructor
> >> in the host teplate, similar to the constructore mentioned above.

okay.  It should be done wherever the sdev is de-allocated.

> 
> In fact after all the above suggestions it might be useful to have
> a struct scsi_transport_attribute instead of stuffing all that into
> the host template (sorry for guiding you into that direction).  The most
> logical interface to set it would be to have a third argument for
> scsi_host_alloc, but we can't break that interface now, so we should
> probably add scsi_set_transport(struct Scsi_host *,
> struct scsi_transport_template *), then and the driver will do in it's probe
> rountine, somewhere between scsi_host_alloc and scsi_add_host a
> scsi_set_transport(shost, &scsi_spi_template);

what do others think about this?  This seems nice to me.  I'm going to
go ahead with working on this.

More feedback is always welcome!
mh

-- 
Martin Hicks                Wild Open Source Inc.
mort@wildopensource.com     613-266-2296

Re: Transport Attributes -- attempt#2

[James Bottomley]

On Thu, 2004-01-08 at 09:01, Martin Hicks wrote:
> > >> Out of curiosity:  Why do we need _another_ classdev.  Can't you
> > >> reuse the current one?
> 
> It guess we could re-use the scsi_device classdev.  This would mean that
> all the attribute files would show up in /sys/class/scsi_device/w:x:y:z.
> We would have to export an attribute called "transport" to tell which
> transport the device is using.  I'm agreeable to this solution.  I don't
> much like having an fc_transport and pscsi_transport directory in
> /sys/class.

I disagree with this.  We put attributes that apply to all devices in
scsi_device classdev.  We're implementing a separate transport interface
here, so it really does need its own class_device, otherwise separating
it from scsi_sysfs and allowing arbitrary transport class creation will
become a real pain.

A class is supposed to represent a device interface, and the transport
class does just that for transport attributes.

James

Transport Attributes -- attempt#3

[Martin Hicks]

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


Hi,

Here is round#3 of the patch.  I think I've addressed all of the points
that were brought up by Christoph last time.

Once again, three patches:  core, qla1280 and qla2xxx.  The core patch
expects to be applied on top of the patch that I sent recently to
linux-scsi:

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

thanks
mh

-- 
Martin Hicks                Wild Open Source Inc.
mort@wildopensource.com     613-266-2296

[-- Attachment #2: scsi-transport-class-attr-core-v4-3.diff --]
[-- Type: text/plain, Size: 22151 bytes --]

# This is a BitKeeper generated patch for the following project:
# Project Name: Linux kernel tree
# This patch format is intended for GNU patch command version 2.5 or higher.
# This patch includes the following deltas:
#	           ChangeSet	1.1492  -> 1.1493 
#	include/scsi/scsi_device.h	1.11    -> 1.12   
#	include/scsi/scsi_host.h	1.13    -> 1.14   
#	drivers/scsi/scsi_sysfs.c	1.38    -> 1.39   
#	drivers/scsi/scsi_syms.c	1.46    -> 1.47   
#	drivers/scsi/scsi_scan.c	1.115   -> 1.116  
#	drivers/scsi/Makefile	1.50    -> 1.51   
#	drivers/scsi/Kconfig	1.47    -> 1.48   
#	               (new)	        -> 1.1     include/scsi/scsi_transport.h
#	               (new)	        -> 1.1     drivers/scsi/scsi_transport_spi.c
#	               (new)	        -> 1.1     include/scsi/scsi_transport_spi.h
#	               (new)	        -> 1.1     include/scsi/scsi_transport_fc.h
#	               (new)	        -> 1.1     drivers/scsi/scsi_transport_fc.c
#
# The following is the BitKeeper ChangeSet Log
# --------------------------------------------
# 04/01/14	mort@green.i.bork.org	1.1493
# The core functionality of the Transport Attributes patch.
# --------------------------------------------
#
diff -Nru a/drivers/scsi/Kconfig b/drivers/scsi/Kconfig
--- a/drivers/scsi/Kconfig	Wed Jan 14 12:59:11 2004
+++ b/drivers/scsi/Kconfig	Wed Jan 14 12:59:11 2004
@@ -196,6 +196,25 @@
 	  there should be no noticeable performance impact as long as you have
 	  logging turned off.
 
+menu "SCSI Transport Attributes (EXPERIMENTAL)"
+	depends on EXPERIMENTAL && SCSI!=n
+
+config SCSI_SPI_ATTRS
+	bool "Parallel SCSI (SPI) Transport Attributes"
+	depends on SCSI
+	help
+	  If you wish to export transport-specific information about
+	  each attached SCSI device to sysfs, say Y.  Otherwise, say N.
+
+config SCSI_FC_ATTRS
+	bool "FiberChannel Transport Attributes"
+	depends on SCSI
+	help
+	  If you wish to export transport-specific information about
+	  each attached FiberChannel device to sysfs, say Y.
+	  Otherwise, say N.
+
+endmenu
 
 menu "SCSI low-level drivers"
 	depends on SCSI!=n
diff -Nru a/drivers/scsi/Makefile b/drivers/scsi/Makefile
--- a/drivers/scsi/Makefile	Wed Jan 14 12:59:11 2004
+++ b/drivers/scsi/Makefile	Wed Jan 14 12:59:11 2004
@@ -127,14 +127,21 @@
 obj-$(CONFIG_BLK_DEV_SR)	+= sr_mod.o
 obj-$(CONFIG_CHR_DEV_SG)	+= sg.o
 
+ifdef CONFIG_SCSI_SPI_ATTRS
+transport-objs			+= scsi_transport_spi.o
+endif
+ifdef CONFIG_SCSI_FC_ATTRS
+transport-objs			+= scsi_transport_fc.o
+endif
+
 scsi_mod-y			+= scsi.o hosts.o scsi_ioctl.o constants.o \
 				   scsicam.o scsi_error.o scsi_lib.o \
 				   scsi_scan.o scsi_syms.o scsi_sysfs.o \
-				   scsi_devinfo.o
+				   scsi_devinfo.o $(transport-objs)
 scsi_mod-$(CONFIG_SYSCTL)	+= scsi_sysctl.o
 scsi_mod-$(CONFIG_SCSI_PROC_FS)	+= scsi_proc.o
 scsi_mod-$(CONFIG_X86_PC9800)	+= scsi_pc98.o
-			
+
 sd_mod-objs	:= sd.o
 sr_mod-objs	:= sr.o sr_ioctl.o sr_vendor.o
 initio-objs	:= ini9100u.o i91uscsi.o
diff -Nru a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
--- a/drivers/scsi/scsi_scan.c	Wed Jan 14 12:59:11 2004
+++ b/drivers/scsi/scsi_scan.c	Wed Jan 14 12:59:11 2004
@@ -35,6 +35,7 @@
 #include <scsi/scsi_driver.h>
 #include <scsi/scsi_devinfo.h>
 #include <scsi/scsi_host.h>
+#include <scsi/scsi_transport.h>
 #include "scsi.h"
 
 #include "scsi_priv.h"
@@ -237,6 +238,16 @@
 			goto out_free_queue;
 	}
 
+	/* Give each shost a default transportt if the driver
+	 * doesn't yet support Transport Attributes */
+	if (!shost->transportt) 
+		shost->transportt = &blank_transport_template;
+
+	if (shost->transportt->setup) {
+		if (shost->transportt->setup(sdev))
+			goto out_cleanup_slave;
+	}
+
 	if (get_device(&sdev->host->shost_gendev)) {
 
 		device_initialize(&sdev->sdev_gendev);
@@ -253,8 +264,15 @@
 		snprintf(sdev->sdev_classdev.class_id, BUS_ID_SIZE,
 			 "%d:%d:%d:%d", sdev->host->host_no,
 			 sdev->channel, sdev->id, sdev->lun);
+
+		class_device_initialize(&sdev->transport_classdev);
+		sdev->transport_classdev.dev = &sdev->sdev_gendev;
+		sdev->transport_classdev.class = sdev->host->transportt->class;
+		snprintf(sdev->transport_classdev.class_id, BUS_ID_SIZE,
+			 "%d:%d:%d:%d", sdev->host->host_no,
+			 sdev->channel, sdev->id, sdev->lun);
 	} else
-		goto out_cleanup_slave;
+		goto out_cleanup_transport;
 
 	/*
 	 * If there are any same target siblings, add this to the
@@ -283,6 +301,9 @@
 	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);
@@ -744,6 +765,8 @@
 	} 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:
@@ -1300,5 +1323,7 @@
 
 	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_syms.c b/drivers/scsi/scsi_syms.c
--- a/drivers/scsi/scsi_syms.c	Wed Jan 14 12:59:11 2004
+++ b/drivers/scsi/scsi_syms.c	Wed Jan 14 12:59:11 2004
@@ -21,6 +21,8 @@
 #include <scsi/scsi_driver.h>
 #include <scsi/scsi_host.h>
 #include <scsi/scsi_ioctl.h>
+#include <scsi/scsi_transport_spi.h>
+#include <scsi/scsi_transport_fc.h>
 #include <scsi/scsicam.h>
 #include "scsi.h"
 
@@ -107,3 +109,10 @@
  */
 EXPORT_SYMBOL(scsi_add_timer);
 EXPORT_SYMBOL(scsi_delete_timer);
+
+#ifdef CONFIG_SCSI_SPI_ATTRS
+EXPORT_SYMBOL(spi_transport_template);
+#endif
+#ifdef CONFIG_SCSI_FC_ATTRS
+EXPORT_SYMBOL(fc_transport_template);
+#endif
diff -Nru a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
--- a/drivers/scsi/scsi_sysfs.c	Wed Jan 14 12:59:11 2004
+++ b/drivers/scsi/scsi_sysfs.c	Wed Jan 14 12:59:11 2004
@@ -13,6 +13,9 @@
 #include <linux/device.h>
 
 #include <scsi/scsi_host.h>
+#include <scsi/scsi_transport.h>
+#include <scsi/scsi_transport_spi.h>
+#include <scsi/scsi_transport_fc.h>
 #include "scsi.h"
 
 #include "scsi_priv.h"
@@ -162,13 +165,31 @@
 	int error;
 
 	error = bus_register(&scsi_bus_type);
-	if (!error) {
-		error = class_register(&sdev_class);
-		if (error)
-			bus_unregister(&scsi_bus_type);
-	}
+	if (error)
+		return error;
 
+	error = class_register(&sdev_class);
+	if (error)
+		goto undo_bus;
+
+	error = scsi_spi_transport_init();
+	if (error)
+		goto undo_sdev;
+
+	error = scsi_fc_transport_init();
+	if (error)
+		goto undo_all;
+
+ out:
 	return error;
+
+ undo_all:
+	scsi_spi_transport_exit();
+ undo_sdev:
+	class_unregister(&sdev_class);
+ undo_bus:
+	bus_unregister(&scsi_bus_type);
+	goto out;
 }
 
 void scsi_sysfs_unregister(void)
@@ -344,6 +365,7 @@
  **/
 int scsi_sysfs_add_sdev(struct scsi_device *sdev)
 {
+	struct class_device_attribute **attrs;
 	int error = -EINVAL, i;
 
 	if (sdev->sdev_state != SDEV_CREATED)
@@ -364,6 +386,12 @@
 		return error;
 	}
 
+	if (sdev->transport_classdev.class) {
+		error = class_device_add(&sdev->transport_classdev);
+		if (error)
+			goto clean_device2;
+	}
+
 	get_device(&sdev->sdev_gendev);
 
 	if (sdev->host->hostt->sdev_attrs) {
@@ -385,16 +413,27 @@
 		}
 	}
 
+	if (sdev->transport_classdev.class) {
+		attrs = sdev->host->transportt->attrs;
+		for (i = 0; attrs[i]; i++) {
+			error = class_device_create_file(&sdev->transport_classdev,
+							 attrs[i]);
+			if (error)
+				scsi_remove_device(sdev);
+		}
+	}
+
 	return error;
 
-clean_device:
+ clean_device2:
+	class_device_del(&sdev->sdev_classdev);
+ clean_device:
 	sdev->sdev_state = SDEV_CANCEL;
 
 	device_del(&sdev->sdev_gendev);
 	put_device(&sdev->sdev_gendev);
 
 	return error;
-
 }
 
 /**
@@ -409,6 +448,8 @@
 		device_del(&sdev->sdev_gendev);
 		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);
 	}
 }
@@ -495,3 +536,7 @@
 
 	return 0;
 }
+
+/* A blank transport template that is used in drivers that don't
+ * yet implement Transport Attributes */
+struct scsi_transport_template blank_transport_template = { NULL };
diff -Nru a/drivers/scsi/scsi_transport_fc.c b/drivers/scsi/scsi_transport_fc.c
--- /dev/null	Wed Dec 31 16:00:00 1969
+++ b/drivers/scsi/scsi_transport_fc.c	Wed Jan 14 12:59:11 2004
@@ -0,0 +1,121 @@
+/* 
+ *  FiberChannel transport specific attributes exported to sysfs.
+ *
+ *  Copyright (c) 2003 Silicon Graphics, Inc.  All rights reserved.
+ *
+ *  This program is free software; you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License as published by
+ *  the Free Software Foundation; either version 2 of the License, or
+ *  (at your option) any later version.
+ *
+ *  This program is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License
+ *  along with this program; if not, write to the Free Software
+ *  Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
+ */
+
+#include <scsi/scsi_device.h>
+#include <scsi/scsi_host.h>
+#include <scsi/scsi_transport.h>
+#include <scsi/scsi_transport_fc.h>
+
+static void transport_class_release(struct class_device *class_dev);
+
+struct class fc_transport_class = {
+	.name = "fc_transport",
+	.release = transport_class_release,
+};
+
+
+int scsi_fc_transport_init(void)
+{
+	return class_register(&fc_transport_class);
+}
+
+void scsi_fc_tranport_exit(void)
+{
+	class_unregister(&fc_transport_class);
+}
+
+void scsi_sysfs_cleanup_fc_transport(struct scsi_device *sdev)
+{
+	kfree(sdev->transport_attr_values);
+	put_device(&sdev->host->shost_gendev);
+}
+
+/* Allocates the struct to place the attribute values in.
+ * Usually called from the scsi host driver's slave_alloc
+ * function.
+ */
+static int fc_alloc_transport_attrs(struct scsi_device *sdev)
+{
+	sdev->transport_attr_values = kmalloc(sizeof(struct fc_transport_attrs),
+					      GFP_ATOMIC);
+	if (!sdev->transport_attr_values)
+		return -ENOMEM;
+
+	memcpy(sdev->transport_attr_values, sdev->host->transportt->default_attr_values,
+	       sizeof(struct fc_transport_attrs));
+
+	return 0;
+}
+
+static void fc_destroy_transport_attrs(struct scsi_device *sdev)
+{
+	kfree(sdev->transport_attr_values);
+}
+
+static void transport_class_release(struct class_device *class_dev)
+{
+	struct scsi_device *sdev = transport_class_to_sdev(class_dev);
+	put_device(&sdev->sdev_gendev);
+}
+
+#define fc_transport_show_function(field, format_string, cast)			\
+static ssize_t									\
+show_fc_transport_##field (struct class_device *cdev, char *buf)		\
+{										\
+	struct scsi_device *sdev = transport_class_to_sdev(cdev);		\
+	struct fc_transport_attrs *tp;						\
+	tp = (struct fc_transport_attrs *)sdev->transport_attr_values;		\
+	return snprintf(buf, 20, format_string, cast tp->field);		\
+}
+
+#define fc_transport_rd_attr(field, format_string)				\
+	fc_transport_show_function(field, format_string, )			\
+static CLASS_DEVICE_ATTR( field, S_IRUGO, show_fc_transport_##field, NULL)
+
+#define fc_transport_rd_attr_cast(field, format_string, cast)			\
+	fc_transport_show_function(field, format_string, (cast))		\
+static CLASS_DEVICE_ATTR( field, S_IRUGO, show_fc_transport_##field, NULL)
+
+
+/* the FiberChannel Tranport Attributes: */
+fc_transport_rd_attr_cast(node_name, "0x%llx\n", unsigned long long);
+fc_transport_rd_attr_cast(port_name, "0x%llx\n", unsigned long long);
+fc_transport_rd_attr(port_id, "0x%x\n");
+
+struct class_device_attribute *fc_transport_attrs[] = {
+	&class_device_attr_node_name,
+	&class_device_attr_port_name,
+	&class_device_attr_port_id,
+	NULL
+};
+
+struct fc_transport_attrs fc_transport_attr_defaults = {
+	.node_name = -1,
+	.port_name = -1,
+	.port_id = -1,
+};
+
+struct scsi_transport_template fc_transport_template = {
+	.attrs = fc_transport_attrs,
+	.class = &fc_transport_class,
+	.setup = &fc_alloc_transport_attrs,
+	.cleanup = &fc_destroy_transport_attrs,
+	.default_attr_values = &fc_transport_attr_defaults,
+};
diff -Nru a/drivers/scsi/scsi_transport_spi.c b/drivers/scsi/scsi_transport_spi.c
--- /dev/null	Wed Dec 31 16:00:00 1969
+++ b/drivers/scsi/scsi_transport_spi.c	Wed Jan 14 12:59:11 2004
@@ -0,0 +1,103 @@
+/* 
+ *  Parallel SCSI (SPI) transport specific attributes exported to sysfs.
+ *
+ *  Copyright (c) 2003 Silicon Graphics, Inc.  All rights reserved.
+ *
+ *  This program is free software; you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License as published by
+ *  the Free Software Foundation; either version 2 of the License, or
+ *  (at your option) any later version.
+ *
+ *  This program is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License
+ *  along with this program; if not, write to the Free Software
+ *  Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
+ */
+
+#include <scsi/scsi_device.h>
+#include <scsi/scsi_host.h>
+#include <scsi/scsi_transport.h>
+#include <scsi/scsi_transport_spi.h>
+
+static void transport_class_release(struct class_device *class_dev);
+
+struct class spi_transport_class = {
+	.name = "spi_transport",
+	.release = transport_class_release,
+};
+
+int scsi_spi_transport_init(void)
+{
+	return class_register(&spi_transport_class);
+}
+
+void scsi_spi_transport_exit(void)
+{
+	class_unregister(&spi_transport_class);
+}
+
+static int spi_alloc_transport_attrs(struct scsi_device *sdev)
+{
+	sdev->transport_attr_values = kmalloc(sizeof(struct spi_transport_attrs),
+					      GFP_ATOMIC);
+	if (!sdev->transport_attr_values)
+		return -ENOMEM;
+
+	memcpy(sdev->transport_attr_values, sdev->host->transportt->default_attr_values,
+	       sizeof(struct spi_transport_attrs));
+
+	return 0;
+}
+
+static void spi_destroy_transport_attrs(struct scsi_device *sdev)
+{
+	kfree(sdev->transport_attr_values);
+}
+
+static void transport_class_release(struct class_device *class_dev)
+{
+	struct scsi_device *sdev = transport_class_to_sdev(class_dev);
+	put_device(&sdev->sdev_gendev);
+}
+
+#define spi_transport_show_function(field, format_string)			\
+static ssize_t									\
+show_spi_transport_##field (struct class_device *cdev, char *buf)		\
+{										\
+	struct scsi_device *sdev = transport_class_to_sdev(cdev);		\
+	struct spi_transport_attrs *tp;						\
+	tp = (struct spi_transport_attrs *)sdev->transport_attr_values;		\
+	return snprintf(buf, 20, format_string, tp->field);			\
+}
+
+#define spi_transport_rd_attr(field, format_string)				\
+	spi_transport_show_function(field, format_string)			\
+static CLASS_DEVICE_ATTR( field, S_IRUGO, show_spi_transport_##field, NULL)
+
+
+/* The Parallel SCSI Tranport Attributes: */
+spi_transport_rd_attr(period, "%d\n");
+spi_transport_rd_attr(offset, "%d\n");
+
+struct class_device_attribute *spi_transport_attrs[] = {
+	&class_device_attr_period,
+	&class_device_attr_offset,
+	NULL
+};
+
+struct spi_transport_attrs spi_transport_attr_defaults = {
+	.period = -1,
+	.offset = -1,
+};
+
+struct scsi_transport_template spi_transport_template = {
+	.attrs = spi_transport_attrs,
+	.class = &spi_transport_class,
+	.setup = &spi_alloc_transport_attrs,
+	.cleanup = &spi_destroy_transport_attrs,
+	.default_attr_values = &spi_transport_attr_defaults,
+};
diff -Nru a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
--- a/include/scsi/scsi_device.h	Wed Jan 14 12:59:11 2004
+++ b/include/scsi/scsi_device.h	Wed Jan 14 12:59:11 2004
@@ -103,12 +103,17 @@
 	struct device		sdev_gendev;
 	struct class_device	sdev_classdev;
 
+	void			* transport_attr_values;
+	struct class_device	transport_classdev;
+
 	enum scsi_device_state sdev_state;
 };
 #define	to_scsi_device(d)	\
 	container_of(d, struct scsi_device, sdev_gendev)
 #define	class_to_sdev(d)	\
 	container_of(d, struct scsi_device, sdev_classdev)
+#define transport_class_to_sdev(class_dev) \
+	container_of(class_dev, struct scsi_device, transport_classdev)
 
 extern struct scsi_device *scsi_add_device(struct Scsi_Host *,
 		uint, uint, uint);
diff -Nru a/include/scsi/scsi_host.h b/include/scsi/scsi_host.h
--- a/include/scsi/scsi_host.h	Wed Jan 14 12:59:11 2004
+++ b/include/scsi/scsi_host.h	Wed Jan 14 12:59:11 2004
@@ -11,6 +11,7 @@
 struct scsi_device;
 struct Scsi_Host;
 struct scsi_host_cmd_pool;
+struct scsi_transport_template;
 
 
 /*
@@ -395,6 +396,7 @@
 	unsigned int            eh_kill:1; /* set when killing the eh thread */
 	wait_queue_head_t       host_wait;
 	struct scsi_host_template *hostt;
+	struct scsi_transport_template *transportt;
 	volatile unsigned short host_busy;   /* commands actually active on low-level */
 	volatile unsigned short host_failed; /* commands that failed. */
     
diff -Nru a/include/scsi/scsi_transport.h b/include/scsi/scsi_transport.h
--- /dev/null	Wed Dec 31 16:00:00 1969
+++ b/include/scsi/scsi_transport.h	Wed Jan 14 12:59:11 2004
@@ -0,0 +1,42 @@
+/* 
+ *  Transport specific attributes.
+ *
+ *  Copyright (c) 2003 Silicon Graphics, Inc.  All rights reserved.
+ *
+ *  This program is free software; you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License as published by
+ *  the Free Software Foundation; either version 2 of the License, or
+ *  (at your option) any later version.
+ *
+ *  This program is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License
+ *  along with this program; if not, write to the Free Software
+ *  Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
+ */
+#ifndef SCSI_TRANSPORT_H
+#define SCSI_TRANSPORT_H
+
+struct scsi_transport_template {
+	/* The NULL terminated list of transport attributes
+	 * that should be exported.
+	 */
+	struct class_device_attribute **attrs;
+
+	/* The transport class that the device is in */
+	struct class *class;
+
+	/* Constructor/Destructor functions */
+	int (* setup)(struct scsi_device *);
+	void (* cleanup)(struct scsi_device *);
+
+	/* Default values for the transport attributes */
+	void *default_attr_values;
+};
+
+extern struct scsi_transport_template blank_transport_template;
+
+#endif /* SCSI_TRANSPORT_H */
diff -Nru a/include/scsi/scsi_transport_fc.h b/include/scsi/scsi_transport_fc.h
--- /dev/null	Wed Dec 31 16:00:00 1969
+++ b/include/scsi/scsi_transport_fc.h	Wed Jan 14 12:59:11 2004
@@ -0,0 +1,41 @@
+/* 
+ *  FiberChannel transport specific attributes exported to sysfs.
+ *
+ *  Copyright (c) 2003 Silicon Graphics, Inc.  All rights reserved.
+ *
+ *  This program is free software; you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License as published by
+ *  the Free Software Foundation; either version 2 of the License, or
+ *  (at your option) any later version.
+ *
+ *  This program is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License
+ *  along with this program; if not, write to the Free Software
+ *  Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
+ */
+#ifndef SCSI_TRANSPORT_FC_H
+#define SCSI_TRANSPORT_FC_H
+
+struct scsi_transport_template;
+
+struct fc_transport_attrs {
+	int port_id;
+	uint64_t node_name;
+	uint64_t port_name;
+};
+
+extern struct scsi_transport_template fc_transport_template;
+
+#ifdef CONFIG_SCSI_FC_ATTRS
+extern int scsi_fc_transport_init(void);
+extern void scsi_fc_transport_exit(void);
+#else
+# define scsi_fc_transport_init() 0
+# define scsi_fc_transport_exit()
+#endif
+
+#endif /* SCSI_TRANSPORT_FC_H */
diff -Nru a/include/scsi/scsi_transport_spi.h b/include/scsi/scsi_transport_spi.h
--- /dev/null	Wed Dec 31 16:00:00 1969
+++ b/include/scsi/scsi_transport_spi.h	Wed Jan 14 12:59:11 2004
@@ -0,0 +1,42 @@
+/* 
+ *  Parallel SCSI (SPI) transport specific attributes exported to sysfs.
+ *
+ *  Copyright (c) 2003 Silicon Graphics, Inc.  All rights reserved.
+ *
+ *  This program is free software; you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License as published by
+ *  the Free Software Foundation; either version 2 of the License, or
+ *  (at your option) any later version.
+ *
+ *  This program is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License
+ *  along with this program; if not, write to the Free Software
+ *  Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
+ */
+#ifndef SCSI_TRANSPORT_SPI_H
+#define SCSI_TRANSPORT_SPI_H
+
+#include <linux/config.h>
+
+struct scsi_transport_template;
+
+struct spi_transport_attrs {
+	int period;
+	int offset;
+};
+
+extern struct scsi_transport_template spi_transport_template;
+
+#ifdef CONFIG_SCSI_SPI_ATTRS
+extern int scsi_spi_transport_init(void);
+extern void scsi_spi_transport_exit(void);
+#else
+# define scsi_spi_transport_init() 0
+# define scsi_spi_transport_exit()
+#endif
+
+#endif /* SCSI_TRANSPORT_SPI_H */

[-- Attachment #3: scsi-transport-class-attr-qla1280-v4-3.diff --]
[-- Type: text/plain, Size: 2870 bytes --]

# This is a BitKeeper generated patch for the following project:
# Project Name: Linux kernel tree
# This patch format is intended for GNU patch command version 2.5 or higher.
# This patch includes the following deltas:
#	           ChangeSet	1.1522  -> 1.1523 
#	drivers/scsi/Kconfig	1.48    -> 1.49   
#	drivers/scsi/qla1280.c	1.51    -> 1.52   
#
# The following is the BitKeeper ChangeSet Log
# --------------------------------------------
# 04/01/14	mort@tomahawk.engr.sgi.com	1.1523
# Transport Attributes additions for qla1280
# --------------------------------------------
#
diff -Nru a/drivers/scsi/Kconfig b/drivers/scsi/Kconfig
--- a/drivers/scsi/Kconfig	Wed Jan 14 08:24:20 2004
+++ b/drivers/scsi/Kconfig	Wed Jan 14 08:24:20 2004
@@ -1210,7 +1210,7 @@
 
 config SCSI_QLOGIC_1280
 	tristate "Qlogic QLA 1280 SCSI support"
-	depends on PCI && SCSI
+	depends on PCI && SCSI && SCSI_SPI_ATTRS
 	help
 	  Say Y if you have a QLogic ISP1x80/1x160 SCSI host adapter.
 
diff -Nru a/drivers/scsi/qla1280.c b/drivers/scsi/qla1280.c
--- a/drivers/scsi/qla1280.c	Wed Jan 14 08:24:20 2004
+++ b/drivers/scsi/qla1280.c	Wed Jan 14 08:24:20 2004
@@ -324,6 +324,8 @@
 
 #if LINUX_VERSION_CODE >= 0x020545
 #include <scsi/scsi_host.h>
+#include <scsi/scsi_transport.h>
+#include <scsi/scsi_transport_spi.h>
 #include "scsi.h"
 #else
 #include <linux/blk.h>
@@ -949,6 +951,8 @@
 	ha->instance = num_hosts;
 	host->unique_id = ha->instance;
 
+	host->transportt = &spi_transport_template;
+
 	if (qla1280_pci_config(ha)) {
 		printk(KERN_INFO "qla1x160: Unable to configure PCI\n");
 		goto error_mem_alloced;
@@ -1686,12 +1690,14 @@
 qla1280_slave_configure(Scsi_Device *device)
 {
 	struct scsi_qla_host *ha;
+	struct spi_transport_attrs *attrs;
 	int default_depth = 3;
 	int bus = device->channel;
 	int target = device->id;
 	int status = 0;
 	struct nvram *nv;
 	unsigned long flags;
+	int is1x160;
 
 	ha = (struct scsi_qla_host *)device->host->hostdata;
 	nv = &ha->nvram;
@@ -1699,6 +1705,12 @@
 	if (qla1280_check_for_dead_scsi_bus(ha, bus))
 		return 1;
 
+	if (ha->device_id == PCI_DEVICE_ID_QLOGIC_ISP12160 ||
+	    ha->device_id == PCI_DEVICE_ID_QLOGIC_ISP10160)
+		is1x160 = 1;
+	else
+		is1x160 = 0;
+
 	if (device->tagged_supported &&
 	    (ha->bus_settings[bus].qtag_enables & (BIT_0 << target))) {
 		scsi_adjust_queue_depth(device, MSG_ORDERED_TAG,
@@ -1736,6 +1748,17 @@
 
 	qla12160_get_target_parameters(ha, device);
 	spin_unlock_irqrestore(HOST_LOCK, flags);
+
+	/* Set the parallel scsi transport attributes */
+	attrs = (struct spi_transport_attrs *)device->transport_attr_values;
+	attrs->period = nv->bus[bus].target[target].sync_period;
+	if (is1x160)
+		attrs->offset = nv->bus[bus].target[target].flags.
+			flags1x160.sync_offset;
+	else
+		attrs->offset = nv->bus[bus].target[target].flags.
+			flags1x80.sync_offset;
+
 	return status;
 }
 

[-- Attachment #4: scsi-transport-class-attr-qla2xxx-v4-3.diff --]
[-- Type: text/plain, Size: 3411 bytes --]

# This is a BitKeeper generated patch for the following project:
# Project Name: Linux kernel tree
# This patch format is intended for GNU patch command version 2.5 or higher.
# This patch includes the following deltas:
#	           ChangeSet	1.1523  -> 1.1524 
#	drivers/scsi/qla2xxx/Kconfig	1.1     -> 1.2    
#	drivers/scsi/qla2xxx/qla_os.c	1.1     -> 1.2    
#	drivers/scsi/qla2xxx/qla_os.h	1.1     -> 1.2    
#
# The following is the BitKeeper ChangeSet Log
# --------------------------------------------
# 04/01/14	mort@tomahawk.engr.sgi.com	1.1524
# Transport Attributes additions for qla2xxx
# --------------------------------------------
#
diff -Nru a/drivers/scsi/qla2xxx/Kconfig b/drivers/scsi/qla2xxx/Kconfig
--- a/drivers/scsi/qla2xxx/Kconfig	Wed Jan 14 08:24:33 2004
+++ b/drivers/scsi/qla2xxx/Kconfig	Wed Jan 14 08:24:33 2004
@@ -1,6 +1,6 @@
 config SCSI_QLA2XXX
 	bool "QLogic ISP2xxx PCI/PCI-X Fibre Channel HBA Support"
-	depends on PCI && SCSI
+	depends on PCI && SCSI && SCSI_FC_ATTRS
 	---help---
 	These drivers support the QLogic 2xxx host adapter family of SCSI FCP
 	HBAs.
diff -Nru a/drivers/scsi/qla2xxx/qla_os.c b/drivers/scsi/qla2xxx/qla_os.c
--- a/drivers/scsi/qla2xxx/qla_os.c	Wed Jan 14 08:24:33 2004
+++ b/drivers/scsi/qla2xxx/qla_os.c	Wed Jan 14 08:24:33 2004
@@ -1849,7 +1849,7 @@
 }
 
 /**************************************************************************
-* qla2x00_slave_configure
+* qla2xxx_slave_configure
 *
 * Description:
 **************************************************************************/
@@ -1857,6 +1857,8 @@
 qla2xxx_slave_configure(struct scsi_device *sdev)
 {
 	scsi_qla_host_t *ha = to_qla_host(sdev->host);
+	struct fc_transport_attrs *attrs;
+	struct fc_port *fc;
 	int queue_depth;
 
 	if (IS_QLA2100(ha) || IS_QLA2200(ha))
@@ -1883,9 +1885,19 @@
 		     sdev->host->hostt->cmd_per_lun /* 3 */);
 	}
 
-	return (0);
-}
+	attrs = (struct fc_transport_attrs *)sdev->transport_attr_values;
+	list_for_each_entry(fc, &ha->fcports, list) {
+		if (fc->os_target_id == sdev->id) {
+			attrs->port_name = __be64_to_cpu(*(uint64_t *)fc->port_name);
+			attrs->node_name = __be64_to_cpu(*(uint64_t *)fc->node_name);
+			attrs->port_id = fc->d_id.b24;
+			break;
+		}
+	}
 
+	return 0;
+}
+ 
 /**
  * qla2x00_config_dma_addressing() - Configure OS DMA addressing method.
  * @ha: HA context
@@ -1998,10 +2010,10 @@
 	ha->mmio_length = mmio_len;
 #endif
 
-	return (0);
+	return 0;
 
 iospace_error_exit:
-	return (-ENOMEM);
+	return -ENOMEM;
 }
 
 /*
@@ -2238,6 +2250,8 @@
 
 	sysfs_create_bin_file(&host->shost_gendev.kobj, &sysfs_fw_dump_attr);
 	sysfs_create_bin_file(&host->shost_gendev.kobj, &sysfs_nvram_attr);
+
+	host->transportt = &fc_transport_template;
 
 	qla_printk(KERN_INFO, ha, "\n"
 	    " QLogic ISP2xxx PCI/PCI-X Fibre Channel HBA Driver: %s\n"
diff -Nru a/drivers/scsi/qla2xxx/qla_os.h b/drivers/scsi/qla2xxx/qla_os.h
--- a/drivers/scsi/qla2xxx/qla_os.h	Wed Jan 14 08:24:33 2004
+++ b/drivers/scsi/qla2xxx/qla_os.h	Wed Jan 14 08:24:33 2004
@@ -56,6 +56,7 @@
 #include <linux/module.h>
 #include <linux/version.h>
 #include <linux/init.h>
+#include <linux/list.h>
 #include <linux/string.h>
 #include <linux/errno.h>
 #include <linux/kernel.h>
@@ -91,7 +92,8 @@
 
 #include "scsi.h"
 #include "hosts.h"
-
+#include <scsi/scsi_transport.h>
+#include <scsi/scsi_transport_fc.h>
 #include <scsi/scsicam.h>
 #include <scsi/scsi_ioctl.h>
 

Re: Transport Attributes -- attempt#3

[Andrew Vasquez]

On Wed, 14 Jan 2004, Martin Hicks wrote:

> Here is round#3 of the patch.  I think I've addressed all of the points
> that were brought up by Christoph last time.
> 
> Once again, three patches:  core, qla1280 and qla2xxx.  The core patch
> expects to be applied on top of the patch that I sent recently to
> linux-scsi:
> 

Just a few comments below:

[snip]

> +
> +/* the FiberChannel Tranport Attributes: */
> +fc_transport_rd_attr_cast(node_name, "0x%llx\n", unsigned long long);
> +fc_transport_rd_attr_cast(port_name, "0x%llx\n", unsigned long long);
> +fc_transport_rd_attr(port_id, "0x%x\n");
> +

Purely aesthetics : PortIDs are typically viewed as three hex bytes
padded with zeros, i.e. 010203.  Perhaps this may be more appropriate:

	fc_transport_rd_attr(port_id, "0x%06x\n");

[snip]
> +	attrs = (struct fc_transport_attrs *)sdev->transport_attr_values;
> +	list_for_each_entry(fc, &ha->fcports, list) {
> +		if (fc->os_target_id == sdev->id) {
> +			attrs->port_name = __be64_to_cpu(*(uint64_t *)fc->port_name);
> +			attrs->node_name = __be64_to_cpu(*(uint64_t *)fc->node_name);
> +			attrs->port_id = fc->d_id.b24;

This port_id assignment will not give you what you expect on
big-endian machines.  Given the previous port_id example value
(010203), on BE machines, the attribute will read as 030201 when
displayed.  A more endian safe way if you want to continue with the
pure-type usages (uint64_t/int) rather than byte arrays would be:

	attrs->port_id = fc->d_id.b.domain << 16 |
		fc->d_id.b.area << 8 |
		fc->d_id.b.al_pa;


Regards, 
Andrew Vasquez

Re: Transport Attributes -- attempt#3

[Patrick Mansfield]

On Wed, Jan 14, 2004 at 01:12:41PM -0500, Martin Hicks wrote:
> 
> Hi,
> 
> Here is round#3 of the patch.  I think I've addressed all of the points
> that were brought up by Christoph last time.

Nice, I can now easily see LUN to port mappings, though I don't easily get
port to LUN mapping.

> diff -Nru a/drivers/scsi/scsi_syms.c b/drivers/scsi/scsi_syms.c
> --- a/drivers/scsi/scsi_syms.c	Wed Jan 14 12:59:11 2004
> +++ b/drivers/scsi/scsi_syms.c	Wed Jan 14 12:59:11 2004
> @@ -21,6 +21,8 @@
>  #include <scsi/scsi_driver.h>
>  #include <scsi/scsi_host.h>
>  #include <scsi/scsi_ioctl.h>
> +#include <scsi/scsi_transport_spi.h>
> +#include <scsi/scsi_transport_fc.h>
>  #include <scsi/scsicam.h>
>  #include "scsi.h"
>  
> @@ -107,3 +109,10 @@
>   */
>  EXPORT_SYMBOL(scsi_add_timer);
>  EXPORT_SYMBOL(scsi_delete_timer);
> +
> +#ifdef CONFIG_SCSI_SPI_ATTRS
> +EXPORT_SYMBOL(spi_transport_template);
> +#endif
> +#ifdef CONFIG_SCSI_FC_ATTRS
> +EXPORT_SYMBOL(fc_transport_template);
> +#endif

Can you move these exports to their respective source files? scsi_syms.c
should go away someday. And then we touch one less scsi core file.

> diff -Nru a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
> --- a/drivers/scsi/scsi_sysfs.c	Wed Jan 14 12:59:11 2004
> +++ b/drivers/scsi/scsi_sysfs.c	Wed Jan 14 12:59:11 2004
> @@ -13,6 +13,9 @@
>  #include <linux/device.h>
>  
>  #include <scsi/scsi_host.h>
> +#include <scsi/scsi_transport.h>
> +#include <scsi/scsi_transport_spi.h>
> +#include <scsi/scsi_transport_fc.h>
>  #include "scsi.h"
>  
>  #include "scsi_priv.h"
> @@ -162,13 +165,31 @@
>  	int error;
>  
>  	error = bus_register(&scsi_bus_type);
> -	if (!error) {
> -		error = class_register(&sdev_class);
> -		if (error)
> -			bus_unregister(&scsi_bus_type);
> -	}
> +	if (error)
> +		return error;
>  
> +	error = class_register(&sdev_class);
> +	if (error)
> +		goto undo_bus;
> +
> +	error = scsi_spi_transport_init();
> +	if (error)
> +		goto undo_sdev;
> +
> +	error = scsi_fc_transport_init();
> +	if (error)
> +		goto undo_all;
> +
> + out:
>  	return error;
> +
> + undo_all:
> +	scsi_spi_transport_exit();
> + undo_sdev:
> +	class_unregister(&sdev_class);
> + undo_bus:
> +	bus_unregister(&scsi_bus_type);
> +	goto out;

What happened to making the transports their own module? Then their init
code can register their class, and we don't need to modify any scsi core
files if a new transport file is added.

Or, do a lazy init in the setup functions based on a static (and use a
semaphore to prevent races). That would also keep empty class
fc_transport and spi_transport directories from showing up for the most
scsi users (those with only usb mass storage devices).

> +/* the FiberChannel Tranport Attributes: */
> +fc_transport_rd_attr_cast(node_name, "0x%llx\n", unsigned long long);
> +fc_transport_rd_attr_cast(port_name, "0x%llx\n", unsigned long long);
> +fc_transport_rd_attr(port_id, "0x%x\n");

These are all per target attributes (are all fc level attributes?).

Does that matter? I think it's ok , though someday we should have a target
representation. 

Example:

[elm3b79 scsi]$ cat /sys/class/fc_transport/11\:0\:0\:*/port_name | head -4
0x200400a0b80b17a4
0x200400a0b80b17a4
0x200400a0b80b17a4
0x200400a0b80b17a4
[elm3b79 scsi]$ cat /sys/class/fc_transport/11\:0\:0\:*/port_id | head -4
0x210a13
0x210a13
0x210a13
0x210a13
[elm3b79 scsi]$ cat /sys/class/fc_transport/11\:0\:0\:*/node_name | head -4
0x200400a0b80b17a3
0x200400a0b80b17a3
0x200400a0b80b17a3
0x200400a0b80b17a3

-- Patrick Mansfield

Re: Transport Attributes -- attempt#3

[Martin Peschke3]

Hi,

+     if (sdev->transport_classdev.class) {
+           attrs = sdev->host->transportt->attrs;
+           for (i = 0; attrs[i]; i++) {
+                 error = class_device_create_file
(&sdev->transport_classdev,
+                                         attrs[i]);
+                 if (error)
+                       scsi_remove_device(sdev);
+           }
+     }

Isn't there a break needed in the error case to quit
the loop after scsi_remove_device(sdev) in order to
avoid trouble with sdev in subsequent iterations?
I am not very familiar with the 2.6 SCSI code. But,
is scsi_remove_device(sdev) an appropriate answer
to the failed registration of sysfs attributes?


Martin Peschke

IBM Deutschland Entwicklung GmbH
Linux for eServer Development
Phone: +49-(0)7031-16-2349


Martin Hicks <mort@wildopensource.com>@vger.kernel.org on 14/01/2004
19:12:41

Sent by:    linux-scsi-owner@vger.kernel.org


To:    Christoph Hellwig <hch@infradead.org>
cc:    linux-scsi@vger.kernel.org
Subject:    Transport Attributes -- attempt#3




Hi,

Here is round#3 of the patch.  I think I've addressed all of the points
that were brought up by Christoph last time.

Once again, three patches:  core, qla1280 and qla2xxx.  The core patch
expects to be applied on top of the patch that I sent recently to
linux-scsi:

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

thanks
mh

--
Martin Hicks                Wild Open Source Inc.
mort@wildopensource.com     613-266-2296

Re: Transport Attributes -- attempt#3

[Christoph Hellwig]

> +menu "SCSI Transport Attributes (EXPERIMENTAL)"
> +	depends on EXPERIMENTAL && SCSI!=n

Do you really need the experimental flags? Actually this option is
rather bogus anyway - with your patches posted the qlogic drivers won't
compile anymore without those set.  So either make it unconditional or
let the drives select it.

> +ifdef CONFIG_SCSI_SPI_ATTRS
> +transport-objs			+= scsi_transport_spi.o
> +endif
> +ifdef CONFIG_SCSI_FC_ATTRS
> +transport-objs			+= scsi_transport_fc.o
> +endif
> +
>  scsi_mod-y			+= scsi.o hosts.o scsi_ioctl.o constants.o \
>  				   scsicam.o scsi_error.o scsi_lib.o \
>  				   scsi_scan.o scsi_syms.o scsi_sysfs.o \
> -				   scsi_devinfo.o
> +				   scsi_devinfo.o $(transport-objs)

This still looks quite overcomplicated, what about a simple:

scsi_mod-$(CONFIG_SCSI_SPI_ATTRS) += scsi_transport_spi.o
scsi_mod-$(CONFIG_SCSI_FC_ATTRS)  += scsi_transport_fc.o

> +#ifdef CONFIG_SCSI_SPI_ATTRS
> +EXPORT_SYMBOL(spi_transport_template);
> +#endif
> +#ifdef CONFIG_SCSI_FC_ATTRS
> +EXPORT_SYMBOL(fc_transport_template);
> +#endif

Should just move to the files declaring it.

> +	if (sdev->transport_classdev.class) {
> +		attrs = sdev->host->transportt->attrs;
> +		for (i = 0; attrs[i]; i++) {
> +			error = class_device_create_file(&sdev->transport_classdev,
> +							 attrs[i]);
> +			if (error)
> +				scsi_remove_device(sdev);

Missing break here, in fact the cleanup mioght better be after a lable
you goto to.

> +/* A blank transport template that is used in drivers that don't
> + * yet implement Transport Attributes */
> +struct scsi_transport_template blank_transport_template = { NULL };

The zero-initialization is not needed.

> +void scsi_sysfs_cleanup_fc_transport(struct scsi_device *sdev)
> +{
> +	kfree(sdev->transport_attr_values);

I think you'll have to do the kfree in the device's ->release, but maybe
I got all that sysfs-foo wrong :)

> +	void			* transport_attr_values;

	void			*transport_attr_values;

> +	/* Default values for the transport attributes */
> +	void *default_attr_values;

Should this really be a void *?

> +extern struct scsi_transport_template blank_transport_template;

Should be in scsi_priv.h as it's not exported, no?

Re: Transport Attributes -- attempt#3

[Martin Hicks]

On Wed, Jan 14, 2004 at 03:34:57PM -0800, Andrew Vasquez wrote:
> On Wed, 14 Jan 2004, Martin Hicks wrote:
> 
> > +
> > +/* the FiberChannel Tranport Attributes: */
> > +fc_transport_rd_attr_cast(node_name, "0x%llx\n", unsigned long long);
> > +fc_transport_rd_attr_cast(port_name, "0x%llx\n", unsigned long long);
> > +fc_transport_rd_attr(port_id, "0x%x\n");
> > +
> 
> Purely aesthetics : PortIDs are typically viewed as three hex bytes
> padded with zeros, i.e. 010203.  Perhaps this may be more appropriate:
> 
> 	fc_transport_rd_attr(port_id, "0x%06x\n");

Okay.  Will do.

> 
> [snip]
> > +	attrs = (struct fc_transport_attrs *)sdev->transport_attr_values;
> > +	list_for_each_entry(fc, &ha->fcports, list) {
> > +		if (fc->os_target_id == sdev->id) {
> > +			attrs->port_name = __be64_to_cpu(*(uint64_t *)fc->port_name);
> > +			attrs->node_name = __be64_to_cpu(*(uint64_t *)fc->node_name);
> > +			attrs->port_id = fc->d_id.b24;
> 
> This port_id assignment will not give you what you expect on
> big-endian machines.  Given the previous port_id example value
> (010203), on BE machines, the attribute will read as 030201 when
> displayed.  A more endian safe way if you want to continue with the
> pure-type usages (uint64_t/int) rather than byte arrays would be:
> 
> 	attrs->port_id = fc->d_id.b.domain << 16 |
> 		fc->d_id.b.area << 8 |
> 		fc->d_id.b.al_pa;

I'll fix that up.

Thanks,
mh

-- 
Martin Hicks || mort@bork.org || PGP/GnuPG: 0x4C7F2BEE

Re: Transport Attributes -- attempt#3

[Martin Hicks]

On Thu, Jan 15, 2004 at 01:52:16PM +0100, Martin Peschke3 wrote:
> Hi,
> 
> +     if (sdev->transport_classdev.class) {
> +           attrs = sdev->host->transportt->attrs;
> +           for (i = 0; attrs[i]; i++) {
> +                 error = class_device_create_file
> (&sdev->transport_classdev,
> +                                         attrs[i]);
> +                 if (error)
> +                       scsi_remove_device(sdev);
> +           }
> +     }
> 
> Isn't there a break needed in the error case to quit
> the loop after scsi_remove_device(sdev) in order to
> avoid trouble with sdev in subsequent iterations?
> I am not very familiar with the 2.6 SCSI code. But,
> is scsi_remove_device(sdev) an appropriate answer
> to the failed registration of sysfs attributes?

It looks that way.  There are a couple other places in the same function
that need similar fixes.

Thanks
mh

-- 
Martin Hicks || mort@bork.org || PGP/GnuPG: 0x4C7F2BEE

Re: Transport Attributes -- attempt#3

[Martin Hicks]

On Fri, Jan 16, 2004 at 02:54:57PM +0000, Christoph Hellwig wrote:
> > +menu "SCSI Transport Attributes (EXPERIMENTAL)"
> > +	depends on EXPERIMENTAL && SCSI!=n
> 
> Do you really need the experimental flags? Actually this option is
> rather bogus anyway - with your patches posted the qlogic drivers won't
> compile anymore without those set.  So either make it unconditional or
> let the drives select it.

Nope.  I just turned it on while I was testing stuff.  I will remove it
for the next patch, since a few people have tried it out now :-)

> 
> > +ifdef CONFIG_SCSI_SPI_ATTRS
> > +transport-objs			+= scsi_transport_spi.o
> > +endif
> > +ifdef CONFIG_SCSI_FC_ATTRS
> > +transport-objs			+= scsi_transport_fc.o
> > +endif
> > +
> >  scsi_mod-y			+= scsi.o hosts.o scsi_ioctl.o constants.o \
> >  				   scsicam.o scsi_error.o scsi_lib.o \
> >  				   scsi_scan.o scsi_syms.o scsi_sysfs.o \
> > -				   scsi_devinfo.o
> > +				   scsi_devinfo.o $(transport-objs)
> 
> This still looks quite overcomplicated, what about a simple:
> 
> scsi_mod-$(CONFIG_SCSI_SPI_ATTRS) += scsi_transport_spi.o
> scsi_mod-$(CONFIG_SCSI_FC_ATTRS)  += scsi_transport_fc.o

Okay.  I wasn't sure how to do that.  My Makefile foo is lacking.

Thanks.


> > +/* A blank transport template that is used in drivers that don't
> > + * yet implement Transport Attributes */
> > +struct scsi_transport_template blank_transport_template = { NULL };
> 
> The zero-initialization is not needed.

right.

> 
> > +void scsi_sysfs_cleanup_fc_transport(struct scsi_device *sdev)
> > +{
> > +	kfree(sdev->transport_attr_values);
> 
> I think you'll have to do the kfree in the device's ->release, but maybe
> I got all that sysfs-foo wrong :)

I'll take a closer look.

> > +	/* Default values for the transport attributes */
> > +	void *default_attr_values;
> 
> Should this really be a void *?

I think so.  It has to be a struct spi_transport_attrs or a
fc_transport_attrs, depending on which transport template we're talking
about.

> 
> > +extern struct scsi_transport_template blank_transport_template;
> 
> Should be in scsi_priv.h as it's not exported, no?

Yup.

Thanks again,
mh

-- 
Martin Hicks || mort@bork.org || PGP/GnuPG: 0x4C7F2BEE

Re: Transport Attributes -- attempt#3

[Lincoln Dale]

Hi Andrew,

At 10:34 AM 15/01/2004, Andrew Vasquez wrote:
> > +/* the FiberChannel Tranport Attributes: */
> > +fc_transport_rd_attr_cast(node_name, "0x%llx\n", unsigned long long);
> > +fc_transport_rd_attr_cast(port_name, "0x%llx\n", unsigned long long);
> > +fc_transport_rd_attr(port_id, "0x%x\n");
>
>Purely aesthetics : PortIDs are typically viewed as three hex bytes
>padded with zeros, i.e. 010203.  Perhaps this may be more appropriate:
>
>         fc_transport_rd_attr(port_id, "0x%06x\n");

yes, i'd like to see that too.

but note that most FC switches out there don't support the full range of 
239 Domain_IDs (first octet of FC_ID), so for 'most' people, it'd work out 
correctly anyway (domain_id between 97 and 127).


cheers,

lincoln.

Re: Transport Attributes -- attempt#3

[Brian King]

I just submitted a new LLD to linux-scsi and Christoph suggested I use 
this patch for setting SCSI bus attributes. I looked through the patches 
and noticed that these are per device attributes. I need to be able to 
set per SCSI bus attributes. The attributes I would like to be able to 
set include: initiator ID, maximum bus speed, wide enabled/disabled, and 
device spinup delay. Would it make sense to create a scsi_bus class or 
something similar for these types of attributes?



-- 
Brian King
eServer Storage I/O
IBM Linux Technology Center

Re: Transport Attributes -- attempt#3

[Martin Peschke3]

> I just submitted a new LLD to linux-scsi and Christoph suggested I use
> this patch for setting SCSI bus attributes. I looked through the patches
> and noticed that these are per device attributes. I need to be able to
> set per SCSI bus attributes. The attributes I would like to be able to
> set include: initiator ID, maximum bus speed, wide enabled/disabled, and
> device spinup delay. Would it make sense to create a scsi_bus class or
> something similar for these types of attributes?

I think extending the scheme would be fine, though some of the
attributes that you have mentioned only apply for SPI. There are
other attributes for other transports, e.g. FC link speed, which
could be of interest too.
Taking this a step further, we will probably find useful attributes
for all levels of the <host>:<bus>:<target>:<lun> hierarchy, which
vary for different transport types.
My FCP focussed brain can't help but bring up an example like this:

<host>
      firmware version
      local WWNN
      ...
<bus>
      local WWPN
      S_ID
      port speed
      link speed
      service class
      topology
      ...
<target>
      peer WWPN
      peer WWNN
      D_ID
      ...
<lun>
      FCP_LUN     (though, I still think <lun> should equal FCP_LUN)
      tagged command queueing
      ...

Mit freundlichen Grüßen / with kind regards

Martin Peschke

IBM Deutschland Entwicklung GmbH
Linux for eServer Development
Phone: +49-(0)7031-16-2349

-
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Transport Attributes -- attempt#3

[Patrick Mansfield]

On Mon, Jan 19, 2004 at 06:07:31PM -0600, Brian King wrote:
> I just submitted a new LLD to linux-scsi and Christoph suggested I use 
> this patch for setting SCSI bus attributes. I looked through the patches 
> and noticed that these are per device attributes. I need to be able to 
> set per SCSI bus attributes. The attributes I would like to be able to 
> set include: initiator ID, maximum bus speed, wide enabled/disabled, and 
> device spinup delay. Would it make sense to create a scsi_bus class or 
> something similar for these types of attributes?

For the IPR driver -

Is the spinup delay for the entire card, not per bus? If so it should be
a scsi_host attribute set via shost_attrs. (No one uses shost_attrs, but
its use is the same as sdev_attrs usage in 53c700.c)

And are the other values for the actual physical disks? If they are not
visible to the host, then there is no bus visible to the host (i.e. scsi
core can't see them). 

Having the physical disk show up in scsi core as drives is nice for
managing them, but if they are ready for writing it could be confusing,
and potentially disasterous. We could do hacks to get different behaviour
for the disks (so only sg shows up, or so sd shows up read only), like: add
a flag in scsi_host; modify the INQUIRY type in the ipr driver; or modify
the MODE SENSE page 0x3f (and/or page 0) result in the ipr driver so they
show up read only (though on failure of a MODE SENSE they would become
writable).

If they were visible to scsi core, the intiator ID (this_id) would be
found in the scsi_host, but would need to be added as a sysfs attribute.

We could add a scsi_bus class, but we first need a bus/fabric struct that
can include the class, and that in turn requires quite a bit more work
to get it in the right place in the driver model tree (we would want a
bus/fabric per host:channel pair).

As noted before, we also don't have a target, and the transport attributes
(per the patch) show up as per LUN, even though they might be per target,
or even per bus/fabric. IMO that is fine for now.

i.e. we have a sysfs tree like this:

`-- pcifoo
    `-- host14
        |-- host_attrs
        |-- 14:0:0:0
        |   `-- sdev_attrs
        `-- 14:0:0:1
            `-- sdev_attrs

But we need something like this:

`-- pcifoo
    `-- host14
        |-- host_attrs
        `-- 14:0
            |-- bus_fabric_attrs
            `-- 14:0:0
                |-- target_attrs
                |-- 14:0:0:0
                |   `-- sdev_attrs
                `-- 14:0:0:1
                    `-- sdev_attrs

And then driver model class or bus code can be used with the above as
needed.

-- Patrick Mansfield

Re: Transport Attributes -- attempt#3

[Brian King]

Patrick Mansfield wrote:
> For the IPR driver -
> 
> Is the spinup delay for the entire card, not per bus? If so it should be
> a scsi_host attribute set via shost_attrs. (No one uses shost_attrs, but
> its use is the same as sdev_attrs usage in 53c700.c)

No, the spinup delay is per bus.

> And are the other values for the actual physical disks? If they are not
> visible to the host, then there is no bus visible to the host (i.e. scsi
> core can't see them). 

The other values are also for the entire scsi bus, not for individual 
targets.

> Having the physical disk show up in scsi core as drives is nice for
> managing them, but if they are ready for writing it could be confusing,
> and potentially disasterous. We could do hacks to get different behaviour
> for the disks (so only sg shows up, or so sd shows up read only), like: add
> a flag in scsi_host; modify the INQUIRY type in the ipr driver; or modify
> the MODE SENSE page 0x3f (and/or page 0) result in the ipr driver so they
> show up read only (though on failure of a MODE SENSE they would become
> writable).

Christoph has suggested I create psuedo devices for them, but not report 
them to the upper layer drivers, similar to how scsi_get_host_dev works. 
  This means they won't show up in sysfs, AFAIK. Which would then pose 
the problem you mention above of the midlayer not seeing the bus if 
there are only RAID disks on the bus. For this reason, it might be nice 
if the devices did get reported in, maybe as sg devices only? There 
should really be no reason to create an sd device, as it would only 
cause problems.

> If they were visible to scsi core, the intiator ID (this_id) would be
> found in the scsi_host, but would need to be added as a sysfs attribute.

Once again, for the adapter, the initiator ID is per bus, rather than 
per adapter. I could force them to be the same for all busses, however.

> We could add a scsi_bus class, but we first need a bus/fabric struct that
> can include the class, and that in turn requires quite a bit more work
> to get it in the right place in the driver model tree (we would want a
> bus/fabric per host:channel pair).
> 
> As noted before, we also don't have a target, and the transport attributes
> (per the patch) show up as per LUN, even though they might be per target,
> or even per bus/fabric. IMO that is fine for now.
> 
> i.e. we have a sysfs tree like this:
> 
> `-- pcifoo
>     `-- host14
>         |-- host_attrs
>         |-- 14:0:0:0
>         |   `-- sdev_attrs
>         `-- 14:0:0:1
>             `-- sdev_attrs
> 
> But we need something like this:
> 
> `-- pcifoo
>     `-- host14
>         |-- host_attrs
>         `-- 14:0
>             |-- bus_fabric_attrs
>             `-- 14:0:0
>                 |-- target_attrs
>                 |-- 14:0:0:0
>                 |   `-- sdev_attrs
>                 `-- 14:0:0:1
>                     `-- sdev_attrs
> 
> And then driver model class or bus code can be used with the above as
> needed.

That seems reasonable to me.


-- 
Brian King
eServer Storage I/O
IBM Linux Technology Center

Re: Transport Attributes -- attempt#3

[Martin Peschke3]

Patrick Mansfield wrote:
> But we need something like this:
>
> `-- pcifoo
>     `-- host14
>         |-- host_attrs
>         `-- 14:0
>             |-- bus_fabric_attrs
>             `-- 14:0:0
>                 |-- target_attrs
>                 |-- 14:0:0:0
>                 |   `-- sdev_attrs
>                 `-- 14:0:0:1
>                     `-- sdev_attrs
>
> And then driver model class or bus code can be used with the above as
> needed.

And a rectified hierarchy of SCSI data structures should leave its mark
on other SCSI stack functions. I am particularly thinking of error
recovery, which should always be limited to the actual scope of failure,
i.e. lun, target, bus, adapter, and be enlarged on demand, i.e. from
lun to target, and so on, if some sort of escalation is needed.
I have seen (2.4) SCSI stacks recover for hours while there were
probably pretty operational devices blocked due to some recovery
going on the same adapter.
What about a more sophisticated blocking interface?
Or, various types of hotplug events, e.g. a FC port (= target)
appearing on an LLDD's radar.
Dreaming about 2.7 ...



Mit freundlichen Grüßen / with kind regards

Martin Peschke

IBM Deutschland Entwicklung GmbH
Linux for eServer Development
Phone: +49-(0)7031-16-2349

-
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Transport Attributes -- attempt#3

[Mike Anderson]

Martin Peschke3 [MPESCHKE@de.ibm.com] wrote:
> And a rectified hierarchy of SCSI data structures should leave its mark
> on other SCSI stack functions. I am particularly thinking of error
> recovery, which should always be limited to the actual scope of failure,
> i.e. lun, target, bus, adapter, and be enlarged on demand, i.e. from
> lun to target, and so on, if some sort of escalation is needed.
> I have seen (2.4) SCSI stacks recover for hours while there were
> probably pretty operational devices blocked due to some recovery
> going on the same adapter.

I believe the goal is finer grain control of recovery and / or transport
specific recovery. While 2.6 is better in that we do not retry the IO in
the context of the error handler we still suspend all IO and wait on
inflight IO before recovery starts.

> What about a more sophisticated blocking interface?
> Or, various types of hotplug events, e.g. a FC port (= target)
> appearing on an LLDD's radar.
> Dreaming about 2.7 ...
> 

On hoptplug events it looks like Greg set out the exported interface
patch the other day. So we can all use this common function now.

http://marc.theaimsgroup.com/?l=linux-kernel&m=107456203121162&w=2

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