[PATCH] scsi_transport_sas: introduce a sas_port entity

[PATCH] scsi_transport_sas: introduce a sas_port entity

[James Bottomley]

this patch separates out ports and phys, with ports becoming the primary
objects of the tree.  At the moment, this is pretty much an illustration
of how it will be done, with the port having very few actual attributes.
A patch demonstrating this on the aic94xx driver will follow.

James
diff --git a/drivers/scsi/scsi_sas_internal.h b/drivers/scsi/scsi_sas_internal.h
index d76e6e3..e1edab4 100644
--- a/drivers/scsi/scsi_sas_internal.h
+++ b/drivers/scsi/scsi_sas_internal.h
@@ -2,7 +2,8 @@ #ifndef _SCSI_SAS_INTERNAL_H
 #define _SCSI_SAS_INTERNAL_H
 
 #define SAS_HOST_ATTRS		0
-#define SAS_PORT_ATTRS		17
+#define SAS_PHY_ATTRS		17
+#define SAS_PORT_ATTRS		1
 #define SAS_RPORT_ATTRS		7
 #define SAS_END_DEV_ATTRS	3
 #define SAS_EXPANDER_ATTRS	7
@@ -13,12 +14,14 @@ struct sas_internal {
 	struct sas_domain_function_template *dft;
 
 	struct class_device_attribute private_host_attrs[SAS_HOST_ATTRS];
-	struct class_device_attribute private_phy_attrs[SAS_PORT_ATTRS];
+	struct class_device_attribute private_phy_attrs[SAS_PHY_ATTRS];
+	struct class_device_attribute private_port_attrs[SAS_PORT_ATTRS];
 	struct class_device_attribute private_rphy_attrs[SAS_RPORT_ATTRS];
 	struct class_device_attribute private_end_dev_attrs[SAS_END_DEV_ATTRS];
 	struct class_device_attribute private_expander_attrs[SAS_EXPANDER_ATTRS];
 
 	struct transport_container phy_attr_cont;
+	struct transport_container port_attr_cont;
 	struct transport_container rphy_attr_cont;
 	struct transport_container end_dev_attr_cont;
 	struct transport_container expander_attr_cont;
@@ -28,7 +31,8 @@ struct sas_internal {
 	 * needed by scsi_sysfs.c
 	 */
 	struct class_device_attribute *host_attrs[SAS_HOST_ATTRS + 1];
-	struct class_device_attribute *phy_attrs[SAS_PORT_ATTRS + 1];
+	struct class_device_attribute *phy_attrs[SAS_PHY_ATTRS + 1];
+	struct class_device_attribute *port_attrs[SAS_PORT_ATTRS + 1];
 	struct class_device_attribute *rphy_attrs[SAS_RPORT_ATTRS + 1];
 	struct class_device_attribute *end_dev_attrs[SAS_END_DEV_ATTRS + 1];
 	struct class_device_attribute *expander_attrs[SAS_EXPANDER_ATTRS + 1];
diff --git a/drivers/scsi/scsi_transport_sas.c b/drivers/scsi/scsi_transport_sas.c
index f3b1606..19c0e72 100644
--- a/drivers/scsi/scsi_transport_sas.c
+++ b/drivers/scsi/scsi_transport_sas.c
@@ -174,7 +174,11 @@ static int sas_host_match(struct attribu
 
 static int do_sas_phy_delete(struct device *dev, void *data)
 {
-	if (scsi_is_sas_phy(dev))
+	int pass = (int)(unsigned long)data;
+
+	if (pass == 0 && scsi_is_sas_port(dev))
+		sas_port_delete(dev_to_sas_port(dev));
+	else if (pass == 1 && scsi_is_sas_phy(dev))
 		sas_phy_delete(dev_to_phy(dev));
 	return 0;
 }
@@ -188,13 +192,17 @@ static int do_sas_phy_delete(struct devi
  */
 void sas_remove_host(struct Scsi_Host *shost)
 {
-	device_for_each_child(&shost->shost_gendev, NULL, do_sas_phy_delete);
+	device_for_each_child(&shost->shost_gendev, (void *)0,
+			      do_sas_phy_delete);
+	device_for_each_child(&shost->shost_gendev, (void *)1,
+			      do_sas_phy_delete);
+
 }
 EXPORT_SYMBOL(sas_remove_host);
 
 
 /*
- * SAS Port attributes
+ * SAS Phy attributes
  */
 
 #define sas_phy_show_simple(field, name, format_string, cast)		\
@@ -310,7 +318,7 @@ sas_phy_protocol_attr(identify.target_po
 sas_phy_simple_attr(identify.sas_address, sas_address, "0x%016llx\n",
 		unsigned long long);
 sas_phy_simple_attr(identify.phy_identifier, phy_identifier, "%d\n", u8);
-sas_phy_simple_attr(port_identifier, port_identifier, "%d\n", u8);
+//sas_phy_simple_attr(port_identifier, port_identifier, "%d\n", u8);
 sas_phy_linkspeed_attr(negotiated_linkrate);
 sas_phy_linkspeed_attr(minimum_linkrate_hw);
 sas_phy_linkspeed_attr(minimum_linkrate);
@@ -378,6 +386,7 @@ struct sas_phy *sas_phy_alloc(struct dev
 	device_initialize(&phy->dev);
 	phy->dev.parent = get_device(parent);
 	phy->dev.release = sas_phy_release;
+	INIT_LIST_HEAD(&phy->port_siblings);
 	if (scsi_is_sas_expander_device(parent)) {
 		struct sas_rphy *rphy = dev_to_rphy(parent);
 		sprintf(phy->dev.bus_id, "phy-%d-%d:%d", shost->host_no,
@@ -440,8 +449,8 @@ sas_phy_delete(struct sas_phy *phy)
 {
 	struct device *dev = &phy->dev;
 
-	if (phy->rphy)
-		sas_rphy_delete(phy->rphy);
+	/* this happens if the phy is still part of a port when deleted */
+	BUG_ON(!list_empty(&phy->port_siblings));
 
 	transport_remove_device(dev);
 	device_del(dev);
@@ -464,6 +473,222 @@ int scsi_is_sas_phy(const struct device 
 EXPORT_SYMBOL(scsi_is_sas_phy);
 
 /*
+ * SAS Port attributes
+ */
+#define sas_port_show_simple(field, name, format_string, cast)		\
+static ssize_t								\
+show_sas_port_##name(struct class_device *cdev, char *buf)		\
+{									\
+	struct sas_port *port = transport_class_to_sas_port(cdev);	\
+									\
+	return snprintf(buf, 20, format_string, cast port->field);	\
+}
+
+#define sas_port_simple_attr(field, name, format_string, type)		\
+	sas_port_show_simple(field, name, format_string, (type))	\
+static CLASS_DEVICE_ATTR(name, S_IRUGO, show_sas_port_##name, NULL)
+
+sas_port_simple_attr(num_phys, num_phys, "%d\n", int);
+
+static DECLARE_TRANSPORT_CLASS(sas_port_class,
+			       "sas_port", NULL, NULL, NULL);
+
+static int sas_port_match(struct attribute_container *cont, struct device *dev)
+{
+	struct Scsi_Host *shost;
+	struct sas_internal *i;
+
+	if (!scsi_is_sas_port(dev))
+		return 0;
+	shost = dev_to_shost(dev->parent);
+
+	if (!shost->transportt)
+		return 0;
+	if (shost->transportt->host_attrs.ac.class !=
+			&sas_host_class.class)
+		return 0;
+
+	i = to_sas_internal(shost->transportt);
+	return &i->port_attr_cont.ac == cont;
+}
+
+
+static void sas_port_release(struct device *dev)
+{
+	struct sas_port *port = dev_to_sas_port(dev);
+
+	BUG_ON(!list_empty(&port->phy_list));
+
+	put_device(dev->parent);
+	kfree(port);
+}
+
+static void sas_port_create_link(struct sas_port *port,
+				 struct sas_phy *phy)
+{
+	sysfs_create_link(&port->dev.kobj, &phy->dev.kobj, phy->dev.bus_id);
+	sysfs_create_link(&phy->dev.kobj, &port->dev.kobj, "port");
+}
+
+static void sas_port_delete_link(struct sas_port *port,
+				 struct sas_phy *phy)
+{
+	sysfs_remove_link(&port->dev.kobj, phy->dev.bus_id);
+	sysfs_remove_link(&phy->dev.kobj, "port");
+}
+
+/** sas_port_alloc - allocate and initialize a SAS port structure
+ *
+ * @parent:	parent device
+ * @port_id:	port number
+ *
+ * Allocates a SAS port structure.  It will be added to the device tree
+ * below the device specified by @parent which must be either a Scsi_Host
+ * or a sas_expander_device.
+ *
+ * Returns %NULL on error
+ */
+struct sas_port *sas_port_alloc(struct device *parent, int port_id)
+{
+	struct Scsi_Host *shost = dev_to_shost(parent);
+	struct sas_port *port;
+
+	port = kzalloc(sizeof(*port), GFP_KERNEL);
+	if (!port)
+		return NULL;
+
+	port->port_identifier = port_id;
+
+	device_initialize(&port->dev);
+
+	port->dev.parent = get_device(parent);
+	port->dev.release = sas_port_release;
+
+	mutex_init(&port->phy_list_mutex);
+	INIT_LIST_HEAD(&port->phy_list);
+
+	if (scsi_is_sas_expander_device(parent)) {
+		struct sas_rphy *rphy = dev_to_rphy(parent);
+		sprintf(port->dev.bus_id, "port-%d:%d:%d", shost->host_no,
+			rphy->scsi_target_id, port->port_identifier);
+	} else
+		sprintf(port->dev.bus_id, "port-%d:%d", shost->host_no,
+			port->port_identifier);
+
+	transport_setup_device(&port->dev);
+
+	return port;
+}
+EXPORT_SYMBOL(sas_port_alloc);
+
+/**
+ * sas_port_add - add a SAS port to the device hierarchy
+ *
+ * @port:	port to be added
+ *
+ * publishes a port to the rest of the system
+ */
+int sas_port_add(struct sas_port *port)
+{
+	int error;
+
+	/* No phys should be added until this is made visible */
+	BUG_ON(!list_empty(&port->phy_list));
+
+	error = device_add(&port->dev);
+
+	if (error)
+		return error;
+
+	transport_add_device(&port->dev);
+	transport_configure_device(&port->dev);
+
+	return 0;
+}
+EXPORT_SYMBOL(sas_port_add);
+
+/**
+ * sas_port_free  --  free a SAS PORT
+ * @port:	SAS PORT to free
+ *
+ * Frees the specified SAS PORT.
+ *
+ * Note:
+ *   This function must only be called on a PORT that has not
+ *   sucessfully been added using sas_port_add().
+ */
+void sas_port_free(struct sas_port *port)
+{
+	transport_destroy_device(&port->dev);
+	put_device(&port->dev);
+}
+EXPORT_SYMBOL(sas_port_free);
+
+/**
+ * sas_port_delete  --  remove SAS PORT
+ * @port:	SAS PORT to remove
+ *
+ * Removes the specified SAS PORT.  If the SAS PORT has an
+ * associated phys, unlink them from the port as well.
+ */
+void sas_port_delete(struct sas_port *port)
+{
+	struct device *dev = &port->dev;
+	struct sas_phy *phy, *tmp_phy;
+
+	if (port->rphy)
+		sas_rphy_delete(port->rphy);
+
+	mutex_lock(&port->phy_list_mutex);
+	list_for_each_entry_safe(phy, tmp_phy, &port->phy_list,
+				 port_siblings) {
+		sas_port_delete_link(port, phy);
+		list_del_init(&phy->port_siblings);
+	}
+	mutex_unlock(&port->phy_list_mutex);
+
+	transport_remove_device(dev);
+	device_del(dev);
+	transport_destroy_device(dev);
+	put_device(dev);
+}
+EXPORT_SYMBOL(sas_port_delete);
+
+/**
+ * scsi_is_sas_port --  check if a struct device represents a SAS port
+ * @dev:	device to check
+ *
+ * Returns:
+ *	%1 if the device represents a SAS Port, %0 else
+ */
+int scsi_is_sas_port(const struct device *dev)
+{
+	return dev->release == sas_port_release;
+}
+EXPORT_SYMBOL(scsi_is_sas_port);
+	
+void sas_port_add_phy(struct sas_port *port, struct sas_phy *phy)
+{
+	BUG_ON(!list_empty(&phy->port_siblings));
+	mutex_lock(&port->phy_list_mutex);
+	sas_port_create_link(port, phy);
+	list_add_tail(&phy->port_siblings, &port->phy_list);
+	port->num_phys++;
+	mutex_unlock(&port->phy_list_mutex);
+}
+EXPORT_SYMBOL(sas_port_add_phy);
+
+void sas_port_delete_phy(struct sas_port *port, struct sas_phy *phy)
+{
+	mutex_lock(&port->phy_list_mutex);
+	sas_port_delete_link(port, phy);
+	list_del_init(&port->phy_list);
+	port->num_phys--;
+	mutex_unlock(&port->phy_list_mutex);
+}
+EXPORT_SYMBOL(sas_port_delete_phy);
+
+/*
  * SAS remote PHY attributes.
  */
 
@@ -755,7 +980,7 @@ static void sas_end_device_release(struc
  * Returns:
  *	SAS PHY allocated or %NULL if the allocation failed.
  */
-struct sas_rphy *sas_end_device_alloc(struct sas_phy *parent)
+struct sas_rphy *sas_end_device_alloc(struct sas_port *parent)
 {
 	struct Scsi_Host *shost = dev_to_shost(&parent->dev);
 	struct sas_end_device *rdev;
@@ -769,7 +994,7 @@ struct sas_rphy *sas_end_device_alloc(st
 	rdev->rphy.dev.parent = get_device(&parent->dev);
 	rdev->rphy.dev.release = sas_end_device_release;
 	sprintf(rdev->rphy.dev.bus_id, "end_device-%d:%d-%d",
-		shost->host_no, parent->port_identifier, parent->number);
+		shost->host_no, parent->port_identifier, parent->port_identifier);
 	rdev->rphy.identify.device_type = SAS_END_DEVICE;
 	transport_setup_device(&rdev->rphy.dev);
 
@@ -785,7 +1010,7 @@ EXPORT_SYMBOL(sas_end_device_alloc);
  * Returns:
  *	SAS PHY allocated or %NULL if the allocation failed.
  */
-struct sas_rphy *sas_expander_alloc(struct sas_phy *parent,
+struct sas_rphy *sas_expander_alloc(struct sas_port *parent,
 				    enum sas_device_type type)
 {
 	struct Scsi_Host *shost = dev_to_shost(&parent->dev);
@@ -823,7 +1048,7 @@ EXPORT_SYMBOL(sas_expander_alloc);
  */
 int sas_rphy_add(struct sas_rphy *rphy)
 {
-	struct sas_phy *parent = dev_to_phy(rphy->dev.parent);
+	struct sas_port *parent = dev_to_sas_port(rphy->dev.parent);
 	struct Scsi_Host *shost = dev_to_shost(parent->dev.parent);
 	struct sas_host_attrs *sas_host = to_sas_host_attrs(shost);
 	struct sas_identify *identify = &rphy->identify;
@@ -896,7 +1121,7 @@ void
 sas_rphy_delete(struct sas_rphy *rphy)
 {
 	struct device *dev = &rphy->dev;
-	struct sas_phy *parent = dev_to_phy(dev->parent);
+	struct sas_port *parent = dev_to_sas_port(dev->parent);
 	struct Scsi_Host *shost = dev_to_shost(parent->dev.parent);
 	struct sas_host_attrs *sas_host = to_sas_host_attrs(shost);
 
@@ -953,7 +1178,7 @@ static int sas_user_scan(struct Scsi_Hos
 
 	mutex_lock(&sas_host->lock);
 	list_for_each_entry(rphy, &sas_host->rphy_list, list) {
-		struct sas_phy *parent = dev_to_phy(rphy->dev.parent);
+		struct sas_port *parent = dev_to_sas_port(rphy->dev.parent);
 
 		if (rphy->identify.device_type != SAS_END_DEVICE ||
 		    rphy->scsi_target_id == -1)
@@ -989,16 +1214,19 @@ #define SETUP_RPORT_ATTRIBUTE(field) 			
 #define SETUP_OPTIONAL_RPORT_ATTRIBUTE(field, func)			\
 	SETUP_TEMPLATE(rphy_attrs, field, S_IRUGO, i->f->func)
 
-#define SETUP_PORT_ATTRIBUTE(field)					\
+#define SETUP_PHY_ATTRIBUTE(field)					\
 	SETUP_TEMPLATE(phy_attrs, field, S_IRUGO, 1)
 
-#define SETUP_OPTIONAL_PORT_ATTRIBUTE(field, func)			\
+#define SETUP_PORT_ATTRIBUTE(field)					\
+	SETUP_TEMPLATE(port_attrs, field, S_IRUGO, 1)
+
+#define SETUP_OPTIONAL_PHY_ATTRIBUTE(field, func)			\
 	SETUP_TEMPLATE(phy_attrs, field, S_IRUGO, i->f->func)
 
-#define SETUP_PORT_ATTRIBUTE_WRONLY(field)				\
+#define SETUP_PHY_ATTRIBUTE_WRONLY(field)				\
 	SETUP_TEMPLATE(phy_attrs, field, S_IWUGO, 1)
 
-#define SETUP_OPTIONAL_PORT_ATTRIBUTE_WRONLY(field, func)		\
+#define SETUP_OPTIONAL_PHY_ATTRIBUTE_WRONLY(field, func)		\
 	SETUP_TEMPLATE(phy_attrs, field, S_IWUGO, i->f->func)
 
 #define SETUP_END_DEV_ATTRIBUTE(field)					\
@@ -1034,6 +1262,11 @@ sas_attach_transport(struct sas_function
 	i->phy_attr_cont.ac.match = sas_phy_match;
 	transport_container_register(&i->phy_attr_cont);
 
+	i->port_attr_cont.ac.class = &sas_port_class.class;
+	i->port_attr_cont.ac.attrs = &i->port_attrs[0];
+	i->port_attr_cont.ac.match = sas_port_match;
+	transport_container_register(&i->port_attr_cont);
+
 	i->rphy_attr_cont.ac.class = &sas_rphy_class.class;
 	i->rphy_attr_cont.ac.attrs = &i->rphy_attrs[0];
 	i->rphy_attr_cont.ac.match = sas_rphy_match;
@@ -1052,30 +1285,35 @@ sas_attach_transport(struct sas_function
 	i->f = ft;
 
 	count = 0;
+	SETUP_PORT_ATTRIBUTE(num_phys);
 	i->host_attrs[count] = NULL;
 
 	count = 0;
-	SETUP_PORT_ATTRIBUTE(initiator_port_protocols);
-	SETUP_PORT_ATTRIBUTE(target_port_protocols);
-	SETUP_PORT_ATTRIBUTE(device_type);
-	SETUP_PORT_ATTRIBUTE(sas_address);
-	SETUP_PORT_ATTRIBUTE(phy_identifier);
-	SETUP_PORT_ATTRIBUTE(port_identifier);
-	SETUP_PORT_ATTRIBUTE(negotiated_linkrate);
-	SETUP_PORT_ATTRIBUTE(minimum_linkrate_hw);
-	SETUP_PORT_ATTRIBUTE(minimum_linkrate);
-	SETUP_PORT_ATTRIBUTE(maximum_linkrate_hw);
-	SETUP_PORT_ATTRIBUTE(maximum_linkrate);
-
-	SETUP_PORT_ATTRIBUTE(invalid_dword_count);
-	SETUP_PORT_ATTRIBUTE(running_disparity_error_count);
-	SETUP_PORT_ATTRIBUTE(loss_of_dword_sync_count);
-	SETUP_PORT_ATTRIBUTE(phy_reset_problem_count);
-	SETUP_OPTIONAL_PORT_ATTRIBUTE_WRONLY(link_reset, phy_reset);
-	SETUP_OPTIONAL_PORT_ATTRIBUTE_WRONLY(hard_reset, phy_reset);
+	SETUP_PHY_ATTRIBUTE(initiator_port_protocols);
+	SETUP_PHY_ATTRIBUTE(target_port_protocols);
+	SETUP_PHY_ATTRIBUTE(device_type);
+	SETUP_PHY_ATTRIBUTE(sas_address);
+	SETUP_PHY_ATTRIBUTE(phy_identifier);
+	//SETUP_PHY_ATTRIBUTE(port_identifier);
+	SETUP_PHY_ATTRIBUTE(negotiated_linkrate);
+	SETUP_PHY_ATTRIBUTE(minimum_linkrate_hw);
+	SETUP_PHY_ATTRIBUTE(minimum_linkrate);
+	SETUP_PHY_ATTRIBUTE(maximum_linkrate_hw);
+	SETUP_PHY_ATTRIBUTE(maximum_linkrate);
+
+	SETUP_PHY_ATTRIBUTE(invalid_dword_count);
+	SETUP_PHY_ATTRIBUTE(running_disparity_error_count);
+	SETUP_PHY_ATTRIBUTE(loss_of_dword_sync_count);
+	SETUP_PHY_ATTRIBUTE(phy_reset_problem_count);
+	SETUP_OPTIONAL_PHY_ATTRIBUTE_WRONLY(link_reset, phy_reset);
+	SETUP_OPTIONAL_PHY_ATTRIBUTE_WRONLY(hard_reset, phy_reset);
 	i->phy_attrs[count] = NULL;
 
 	count = 0;
+	SETUP_PORT_ATTRIBUTE(num_phys);
+	i->port_attrs[count] = NULL;
+
+	count = 0;
 	SETUP_RPORT_ATTRIBUTE(rphy_initiator_port_protocols);
 	SETUP_RPORT_ATTRIBUTE(rphy_target_port_protocols);
 	SETUP_RPORT_ATTRIBUTE(rphy_device_type);
@@ -1117,6 +1355,7 @@ void sas_release_transport(struct scsi_t
 
 	transport_container_unregister(&i->t.host_attrs);
 	transport_container_unregister(&i->phy_attr_cont);
+	transport_container_unregister(&i->port_attr_cont);
 	transport_container_unregister(&i->rphy_attr_cont);
 	transport_container_unregister(&i->end_dev_attr_cont);
 	transport_container_unregister(&i->expander_attr_cont);
@@ -1135,9 +1374,12 @@ static __init int sas_transport_init(voi
 	error = transport_class_register(&sas_phy_class);
 	if (error)
 		goto out_unregister_transport;
-	error = transport_class_register(&sas_rphy_class);
+	error = transport_class_register(&sas_port_class);
 	if (error)
 		goto out_unregister_phy;
+	error = transport_class_register(&sas_rphy_class);
+	if (error)
+		goto out_unregister_port;
 	error = transport_class_register(&sas_end_dev_class);
 	if (error)
 		goto out_unregister_rphy;
@@ -1151,6 +1393,8 @@ static __init int sas_transport_init(voi
 	transport_class_unregister(&sas_end_dev_class);
  out_unregister_rphy:
 	transport_class_unregister(&sas_rphy_class);
+ out_unregister_port:
+	transport_class_unregister(&sas_port_class);
  out_unregister_phy:
 	transport_class_unregister(&sas_phy_class);
  out_unregister_transport:
diff --git a/include/scsi/scsi_transport_sas.h b/include/scsi/scsi_transport_sas.h
index 93cfb4b..e37b577 100644
--- a/include/scsi/scsi_transport_sas.h
+++ b/include/scsi/scsi_transport_sas.h
@@ -3,6 +3,7 @@ #define SCSI_TRANSPORT_SAS_H
 
 #include <linux/transport_class.h>
 #include <linux/types.h>
+#include <linux/mutex.h>
 
 struct scsi_transport_template;
 struct sas_rphy;
@@ -55,7 +56,6 @@ struct sas_phy {
 	enum sas_linkrate	minimum_linkrate;
 	enum sas_linkrate	maximum_linkrate_hw;
 	enum sas_linkrate	maximum_linkrate;
-	u8			port_identifier;
 
 	/* internal state */
 	unsigned int		local_attached : 1;
@@ -66,8 +66,8 @@ struct sas_phy {
 	u32			loss_of_dword_sync_count;
 	u32			phy_reset_problem_count;
 
-	/* the other end of the link */
-	struct sas_rphy		*rphy;
+	/* for the list of phys belonging to a port */
+	struct list_head	port_siblings;
 };
 
 #define dev_to_phy(d) \
@@ -124,6 +124,24 @@ struct sas_expander_device {
 #define rphy_to_expander_device(r) \
 	container_of((r), struct sas_expander_device, rphy)
 
+struct sas_port {
+	struct device		dev;
+
+	u8			port_identifier;
+	int			num_phys;
+
+	/* the other end of the link */
+	struct sas_rphy		*rphy;
+
+	struct mutex		phy_list_mutex;
+	struct list_head	phy_list;
+};
+
+#define dev_to_sas_port(d) \
+	container_of((d), struct sas_port, dev)
+#define transport_class_to_sas_port(cdev) \
+	dev_to_sas_port((cdev)->dev)
+
 /* The functions by which the transport class and the driver communicate */
 struct sas_function_template {
 	int (*get_linkerrors)(struct sas_phy *);
@@ -141,13 +159,21 @@ extern int sas_phy_add(struct sas_phy *)
 extern void sas_phy_delete(struct sas_phy *);
 extern int scsi_is_sas_phy(const struct device *);
 
-extern struct sas_rphy *sas_end_device_alloc(struct sas_phy *);
-extern struct sas_rphy *sas_expander_alloc(struct sas_phy *, enum sas_device_type);
+extern struct sas_rphy *sas_end_device_alloc(struct sas_port *);
+extern struct sas_rphy *sas_expander_alloc(struct sas_port *, enum sas_device_type);
 void sas_rphy_free(struct sas_rphy *);
 extern int sas_rphy_add(struct sas_rphy *);
 extern void sas_rphy_delete(struct sas_rphy *);
 extern int scsi_is_sas_rphy(const struct device *);
 
+struct sas_port *sas_port_alloc(struct device *, int);
+int sas_port_add(struct sas_port *);
+void sas_port_free(struct sas_port *);
+void sas_port_delete(struct sas_port *);
+void sas_port_add_phy(struct sas_port *, struct sas_phy *);
+void sas_port_delete_phy(struct sas_port *, struct sas_phy *);
+int scsi_is_sas_port(const struct device *);
+
 extern struct scsi_transport_template *
 sas_attach_transport(struct sas_function_template *);
 extern void sas_release_transport(struct scsi_transport_template *);

RE: [PATCH] scsi_transport_sas: introduce a sas_port entity

[Moore, Eric]

On Tuesday, May 30, 2006 10:05 PM, James Bottomley wrote: 

> +void sas_port_add_phy(struct sas_port *port, struct sas_phy *phy)
> +{
> +	BUG_ON(!list_empty(&phy->port_siblings));
> +	mutex_lock(&port->phy_list_mutex);
> +	sas_port_create_link(port, phy);
> +	list_add_tail(&phy->port_siblings, &port->phy_list);
> +	port->num_phys++;
> +	mutex_unlock(&port->phy_list_mutex);
> +}
> +EXPORT_SYMBOL(sas_port_add_phy);
> +
> +void sas_port_delete_phy(struct sas_port *port, struct sas_phy *phy)
> +{
> +	mutex_lock(&port->phy_list_mutex);
> +	sas_port_delete_link(port, phy);
> +	list_del_init(&port->phy_list);
> +	port->num_phys--;
> +	mutex_unlock(&port->phy_list_mutex);
> +}
> +EXPORT_SYMBOL(sas_port_delete_phy);
> +


Just want to confirm whether these two API is for handling
adding/deleting phys to the sas_port object once the wide port is
formed?

During the cable push case, I will be reporting a narrow port
first, then later needing to populate the transport sas_port 
object with the rest of my phys that form the wide port. It doesn't
all happen all at once, probably requiring about 2 seconds for 
each link to come up, thus several calls to my hotplug_work thread.

If that is the case, I will begin adding your new API today into
the fusion driver.   I've already added support in the fusion driver
for forming wide ports in sas_topology tree, that done in my internal
source a couple weeks back.  I will provide you my sotd ( source of the
day) before I go on vacation.

Eric Moore 


    

RE: [PATCH] scsi_transport_sas: introduce a sas_port entity

[James Bottomley]

On Wed, 2006-05-31 at 11:10 -0600, Moore, Eric wrote:
> Just want to confirm whether these two API is for handling
> adding/deleting phys to the sas_port object once the wide port is
> formed?

Yes, that's the idea (and the use case in aic94xx).

There's probably going to be another API iteration, but more around
allowing the adding of phys before the port is made visible.

James

RE: [PATCH] scsi_transport_sas: introduce a sas_port entity

[Moore, Eric]

On Wednesday, May 31, 2006 11:22 AM, James Bottomley wrote: 

> 
> Yes, that's the idea (and the use case in aic94xx).
> 
> There's probably going to be another API iteration, but more around
> allowing the adding of phys before the port is made visible.
> 

I didn't have time today to add your new API into the driver.
Still trying to stabilize things.   I will send you my latest source
in private email.  I'm leaving for vacation now.  Perhaps while I'm
gone,
can you can the new API support into my source?

Thanks !!
Eric

RE: [PATCH] scsi_transport_sas: introduce a sas_port entity

[James Bottomley]

On Wed, 2006-05-31 at 17:25 -0600, Moore, Eric wrote:
> I didn't have time today to add your new API into the driver.
> Still trying to stabilize things.   I will send you my latest source
> in private email.  I'm leaving for vacation now.  Perhaps while I'm
> gone,
> can you can the new API support into my source?

Actually, since the patch was over 100k and I've been running around a
bit, I didn't really have time to look at it.  What I have managed to do
is to wedge the port object into the existing mptsas driver.  This means
that it will still compile and run, and there's now more time for you to
sort out wide ports.

What I did discover is that the actual firmware has no concept of a port
below the adapter, so I actually had to rejig mptsas_probe_one_phy() to
recognise ports.  I also noticed that on an expander device, we allocate
and end device for the port that's connected to the HBA.  I didn't do
anything about this, since that was the behaviour of the old driver
(even if incorrect), so I assume the wide port update will fix this.

James

---

[SCSI] mptsas: introduce the port object

This is a hack just sufficient to get mptsas compiling and running
with the new port objects.  Eric Moore is working on more
comprehensive support.

Index: BUILD-2.6/drivers/message/fusion/mptsas.c
===================================================================
--- BUILD-2.6.orig/drivers/message/fusion/mptsas.c	2006-06-11 22:16:01.000000000 -0500
+++ BUILD-2.6/drivers/message/fusion/mptsas.c	2006-06-11 22:52:53.000000000 -0500
@@ -140,6 +140,7 @@
 struct mptsas_phyinfo {
 	u8	phy_id; 		/* phy index */
 	u8	port_id; 		/* port number this phy is part of */
+	s8	port_num;		/* index of port for port pointer */
 	u8	negotiated_link_rate;	/* nego'd link rate for this phy */
 	u8	hw_link_rate; 		/* hardware max/min phys link rate */
 	u8	programmed_link_rate;	/* programmed max/min phy link rate */
@@ -147,6 +148,8 @@
 	struct mptsas_devinfo attached;	/* point to attached device info */
 	struct sas_phy *phy;
 	struct sas_rphy *rphy;
+	/* FIXME: hack: make the phyinfo also usable as the port info */
+	struct sas_port *port;
 	struct scsi_target *starget;
 };
 
@@ -811,6 +814,7 @@
 		    buffer->PhyData[i].Port;
 		port_info->phy_info[i].negotiated_link_rate =
 		    buffer->PhyData[i].NegotiatedLinkRate;
+		port_info->phy_info[i].port_num = -1;
 	}
 
  out_free_consistent:
@@ -1161,7 +1165,8 @@
 {
 	MPT_ADAPTER *ioc;
 	struct sas_phy *phy;
-	int error;
+	struct sas_port *port;
+	int error, i, port_index;
 
 	if (!dev)
 		return -ENODEV;
@@ -1173,7 +1178,6 @@
 	} else
 		phy = phy_info->phy;
 
-	phy->port_identifier = phy_info->port_id;
 	mptsas_parse_device_info(&phy->identify, &phy_info->identify);
 
 	/*
@@ -1270,8 +1274,43 @@
 		phy_info->phy = phy;
 	}
 
+	if (phy_info->attached.handle) {
+		port_index = -1;
+		for (i = 0; i < index; i++) {
+			struct mptsas_phyinfo *tmp = &phy_info[i - index];
+			if (!tmp->attached.handle)
+				continue;
+			if (phy_info->attached.handle == tmp->attached.handle) {
+				port_index = tmp->port_num;
+				phy_info->port_num = port_index;
+				break;
+			}
+		}
+		if (port_index == -1) {
+			for (i = 0; ; i++) {
+				struct mptsas_phyinfo *tmp = &phy_info[i - index];
+				if (!tmp->port) {
+					port_index = i;
+					phy_info->port_num = i;
+					break;
+				}
+			}
+		}
+
+		port = phy_info[port_index - index].port;
+
+		if (!port) {
+			port = phy_info[port_index - index].port =
+				sas_port_alloc(dev, port_index);
+			if (!port)
+				return -ENOMEM;
+			sas_port_add(port);
+		}
+		sas_port_add_phy(port, phy);
+	}
+
 	if ((phy_info->attached.handle) &&
-	    (!phy_info->rphy)) {
+	    (!phy_info->rphy) && port && !port->rphy) {
 
 		struct sas_rphy *rphy;
 		struct sas_identify identify;
@@ -1290,11 +1329,11 @@
 		mptsas_parse_device_info(&identify, &phy_info->attached);
 		switch (identify.device_type) {
 		case SAS_END_DEVICE:
-			rphy = sas_end_device_alloc(phy);
+			rphy = sas_end_device_alloc(port);
 			break;
 		case SAS_EDGE_EXPANDER_DEVICE:
 		case SAS_FANOUT_EXPANDER_DEVICE:
-			rphy = sas_expander_alloc(phy, identify.device_type);
+			rphy = sas_expander_alloc(port, identify.device_type);
 			break;
 		default:
 			rphy = NULL;
@@ -1371,8 +1410,7 @@
 		}
 
 		mptsas_probe_one_phy(&ioc->sh->shost_gendev,
-		    &port_info->phy_info[i], ioc->sas_index, 1);
-		ioc->sas_index++;
+		    &port_info->phy_info[i], i, 1);
 	}
 
 	return 0;
@@ -1423,6 +1461,8 @@
 			(MPI_SAS_EXPAND_PGAD_FORM_HANDLE_PHY_NUM <<
 			 MPI_SAS_EXPAND_PGAD_FORM_SHIFT), (i << 16) + *handle);
 
+		port_info->phy_info[i].port_num = -1;
+
 		if (port_info->phy_info[i].identify.handle) {
 			mptsas_sas_device_pg0(ioc,
 				&port_info->phy_info[i].identify,
@@ -1453,15 +1493,15 @@
 		list_for_each_entry(p, &ioc->sas_topology, list) {
 			for (j = 0; j < p->num_phys; j++) {
 				if (port_info->phy_info[i].identify.handle ==
-						p->phy_info[j].attached.handle)
+						p->phy_info[j].attached.handle
+				    && p->phy_info[j].rphy)
 					parent = &p->phy_info[j].rphy->dev;
 			}
 		}
 		mutex_unlock(&ioc->sas_topology_mutex);
 
 		mptsas_probe_one_phy(parent, &port_info->phy_info[i],
-		    ioc->sas_index, 0);
-		ioc->sas_index++;
+		    i, 0);
 	}
 
 	return 0;
@@ -1692,6 +1732,7 @@
 	MPT_ADAPTER *ioc = ev->ioc;
 	struct mptsas_phyinfo *phy_info;
 	struct sas_rphy *rphy;
+	struct sas_port *port;
 	struct scsi_device *sdev;
 	struct sas_identify identify;
 	char *ds = NULL;
@@ -1728,6 +1769,8 @@
 			}
 		}
 
+		port = phy_info[phy_info->port_id - phy_info->phy_id].port;
+
 		if (phy_info->attached.device_info & MPI_SAS_DEVICE_INFO_SSP_TARGET)
 			ds = "ssp";
 		if (phy_info->attached.device_info & MPI_SAS_DEVICE_INFO_STP_TARGET)
@@ -1740,6 +1783,7 @@
 		       ioc->name, ds, ev->channel, ev->id, phy_info->phy_id);
 
 		sas_rphy_delete(phy_info->rphy);
+		sas_port_delete(port);
 		memset(&phy_info->attached, 0, sizeof(struct mptsas_devinfo));
 		phy_info->rphy = NULL;
 		phy_info->starget = NULL;
@@ -1822,14 +1866,16 @@
 		       "attaching %s device, channel %d, id %d, phy %d\n",
 		       ioc->name, ds, ev->channel, ev->id, ev->phy_id);
 
+		port = phy_info[phy_info->port_id - phy_info->phy_id].port;
+
 		mptsas_parse_device_info(&identify, &phy_info->attached);
 		switch (identify.device_type) {
 		case SAS_END_DEVICE:
-			rphy = sas_end_device_alloc(phy_info->phy);
+			rphy = sas_end_device_alloc(port);
 			break;
 		case SAS_EDGE_EXPANDER_DEVICE:
 		case SAS_FANOUT_EXPANDER_DEVICE:
-			rphy = sas_expander_alloc(phy_info->phy, identify.device_type);
+			rphy = sas_expander_alloc(port, identify.device_type);
 			break;
 		default:
 			rphy = NULL;

RE: [PATCH] scsi_transport_sas: introduce a sas_port entity

[Moore, Eric]

On Monday, June 12, 2006 8:18 AM, James Bottomley wrote:  

> Actually, since the patch was over 100k and I've been running around a
> bit, I didn't really have time to look at it.  What I have 
> managed to do
> is to wedge the port object into the existing mptsas driver.  
> This means
> that it will still compile and run, and there's now more time 
> for you to
> sort out wide ports.

Sorry for the delay, due to me being out for the past three weeks.
I finally got to start looking at this patch late yesterday.
I notice you've already put this in your scsi-misc tree.
I wished you had looked at the source I sent. I realize it was
over 100K, but most of that was mpi updated headers.

How much testing was done on this patch?  I see sever regression
with the driver.  Hotplug it busted.  Hot adding any device will
panic.  Also, if you load the driver with devices, then unload the
driver, disconnect device, and reload, the driver panics. Also see 
some various panics just load/unload/load the driver stack, 
with no change of the topology.

> 
> What I did discover is that the actual firmware has no 
> concept of a port
> below the adapter

Yes it does.  The port_id field is valid when you've formed
a port by adding attached devices. This is found in phy_info[]->port_id.
The driver retrieves this in sas_io_unit_pg0, device_pg0, and
expander_pg1.
The port_id is formed at the top level of the hba, and all the devices
downstream
will all be on the same port_id, forming a sas domain.


, so I actually had to rejig 
> mptsas_probe_one_phy() to
> recognise ports.  I also noticed that on an expander device, 
> we allocate
> and end device for the port that's connected to the HBA.  I didn't do
> anything about this, since that was the behaviour of the old driver
> (even if incorrect), so I assume the wide port update will fix this.

You must be refering to the rphy parent/child relationship.  I never
understand why this concept was added, actually implemented by
Christoph.

Also, I thought you were removing all the concepts of remote phys when
you were
adding the port API.  I notice the driver still has rphy_add and
rphy_delete.
Can't the rphy concepts die?

> @@ -1270,8 +1274,43 @@
>  		phy_info->phy = phy;
>  	}
>  
> +	if (phy_info->attached.handle) {
> +		port_index = -1;
> +		for (i = 0; i < index; i++) {
> +			struct mptsas_phyinfo *tmp = 
> &phy_info[i - index];
> +			if (!tmp->attached.handle)
> +				continue;
> +			if (phy_info->attached.handle == 
> tmp->attached.handle) {
> +				port_index = tmp->port_num;
> +				phy_info->port_num = port_index;
> +				break;
> +			}
> +		}
> +		if (port_index == -1) {
> +			for (i = 0; ; i++) {
> +				struct mptsas_phyinfo *tmp = 
> &phy_info[i - index];
> +				if (!tmp->port) {
> +					port_index = i;
> +					phy_info->port_num = i;
> +					break;
> +				}
> +			}
> +		}
> +
> +		port = phy_info[port_index - index].port;

I'm not very confortable with this crude method of calulating where
the port object is placed in the the mptsas topology.  Here is
an dump of the data structure, where I have two wide ports, one
starts at phy_id=0, and the other at phy_id=4.  The port object
is phy in phy_info[0], and phy_info[1] respectively.  I'd rather
see the port object placed under the phy_info[] it belongs to, and
have the same object pointer in the place holder for all its phys
that form a wide port.

Current driver is:

phy_id=0 port_id=0 port_num=0 rphy=ffff81000af70430
port=ffff81000aa60400
phy_id=1 port_id=0 port_num=0 rphy=0000000000000000
port=ffff81000d14bc00
phy_id=2 port_id=0 port_num=0 rphy=0000000000000000
port=0000000000000000
phy_id=3 port_id=0 port_num=0 rphy=0000000000000000
port=0000000000000000
phy_id=4 port_id=4 port_num=1 rphy=ffff810005c81800
port=0000000000000000
phy_id=5 port_id=4 port_num=1 rphy=0000000000000000
port=0000000000000000
phy_id=6 port_id=4 port_num=1 rphy=0000000000000000
port=0000000000000000
phy_id=7 port_id=4 port_num=1 rphy=0000000000000000
port=0000000000000000

Id rather see:

phy_id=0 port_id=0 port_num=0 rphy=ffff81000af70430
port=ffff81000aa60400
phy_id=1 port_id=0 port_num=0 rphy=0000000000000000
port=ffff81000aa60400
phy_id=2 port_id=0 port_num=0 rphy=0000000000000000
port=ffff81000aa60400
phy_id=3 port_id=0 port_num=0 rphy=0000000000000000
port=ffff81000aa60400
phy_id=4 port_id=4 port_num=1 rphy=ffff810005c81800
port=ffff81000d14bc00
phy_id=5 port_id=4 port_num=1 rphy=0000000000000000
port=ffff81000d14bc00
phy_id=6 port_id=4 port_num=1 rphy=0000000000000000
port=ffff81000d14bc00
phy_id=7 port_id=4 port_num=1 rphy=0000000000000000
port=ffff81000d14bc00



> +
> +		if (!port) {
> +			port = phy_info[port_index - index].port =
> +				sas_port_alloc(dev, port_index);
> +			if (!port)
> +				return -ENOMEM;
> +			sas_port_add(port);
> +		}
> +		sas_port_add_phy(port, phy);
> +	}

Id rather you be sending the port identifier that our firmware provides
in
phy_info[].port_id, rather then an arbitrary calculated value.


>  		phy_info->starget = NULL;
> @@ -1822,14 +1866,16 @@
>  		       "attaching %s device, channel %d, id %d, 
> phy %d\n",
>  		       ioc->name, ds, ev->channel, ev->id, ev->phy_id);
>  
> +		port = phy_info[phy_info->port_id - 
> phy_info->phy_id].port;
> +

This is sole reason for panic in the ADD_DEVICE case.  The problem
is the port has not be set up by this point.  Usually the fw event
saying a device has been added occurs prior to the discovery event
that refreshes the topology.  Thus we will need to refresh the
topology prior to the time we pull the port, as well as adding a sanity
check to insure the port pointer is non-zero.


>  		mptsas_parse_device_info(&identify, 
> &phy_info->attached);
>  		switch (identify.device_type) {
>  		case SAS_END_DEVICE:
> -			rphy = sas_end_device_alloc(phy_info->phy);
> +			rphy = sas_end_device_alloc(port);
>  			break;
>  		case SAS_EDGE_EXPANDER_DEVICE:
>  		case SAS_FANOUT_EXPANDER_DEVICE:
> -			rphy = 
> sas_expander_alloc(phy_info->phy, identify.device_type);
> +			rphy = sas_expander_alloc(port, 
> identify.device_type);
>  			break;
>  		default:
>  			rphy = NULL;
> 

This switch case for edge and fanout expanders in the hotplug thread
needs
to be removed. Only end devices apply here.  The SAS_DEVICE_ADDED and 
SAS_DEVICE_NOT_RESPONDING events only apply to end devices, not
expanders.
We handle the adding of deleting of expanders when we get a discovery
event.

Also - I would like to point out that when I have a disk device that is
a wide port,
when its hot added containing, I will get the sas_device_added event on
the first 
phy that was added.  The handling of the other phys come later, called
from
a different instance of the hotplug tread.  We will need to call the
transport layer 
API sometime later to notify about the other phys added to the port.
The current code is not handling that case.

BTW: - James, do you have the equipment to create a disk device that is
a wide port?
The way I'm doing it is by connecting two 1068 SAS controller to
another.
On controller is running in initiator mode, and the other running in
target mode.
We have a target mode driver that creates ramdisk images to simulate
disk devices.  
Every disk device on each phy containing the same sas address, thus
forming a wide port.
Connect both controller with a quad crossover cable containing four
phys. 
That is how I'm testing here in our labs. Do you have two 1068s so you
can simulate this? 
If not, allow me to assit you with this setup.

Eric Moore 

RE: [PATCH] scsi_transport_sas: introduce a sas_port entity

[James Bottomley]

On Fri, 2006-06-16 at 18:16 -0600, Moore, Eric wrote:
> Sorry for the delay, due to me being out for the past three weeks.
> I finally got to start looking at this patch late yesterday.
> I notice you've already put this in your scsi-misc tree.
> I wished you had looked at the source I sent. I realize it was
> over 100K, but most of that was mpi updated headers.

With the tiny amount of bandwidth I had left after doing sas_port, I
couldn't find the time to look through a 100k patch that wasn't
production quality to work out what it was doing.

I just need the smallest patch that keeps mptsas compiling while I push
the class changes upstream.  You can patch it out in favour of yours
when you get it working .. as long as you can solve the expander wide
port issue.

> How much testing was done on this patch?  I see sever regression
> with the driver.  Hotplug it busted.  Hot adding any device will
> panic.  Also, if you load the driver with devices, then unload the
> driver, disconnect device, and reload, the driver panics. Also see 
> some various panics just load/unload/load the driver stack, 
> with no change of the topology.

It works for me in my configuration ... which is about the level of
testing of everything I do.

> > 
> > What I did discover is that the actual firmware has no 
> > concept of a port
> > below the adapter
> 
> Yes it does.  The port_id field is valid when you've formed
> a port by adding attached devices. This is found in phy_info[]->port_id.
> The driver retrieves this in sas_io_unit_pg0, device_pg0, and
> expander_pg1.
> The port_id is formed at the top level of the hba, and all the devices
> downstream
> will all be on the same port_id, forming a sas domain.

No, it doesn't ... that's the point and the problem.  Ports are formed
not only on the HBA but also on the expander. See for example the nice
diagram under the SAS v2 standard (r02) section 4.1.10 Figure 29
(Multiple Connections on Wide Ports).  The thing I'm trying to show,
which the fusion BIOS apparently doesn't do is the wide ports on
expanders.  Without this, we can't show the true tree topology.

I used the debugging mode of the driver to verify that for a wide port
to an expander, all the port_id's of the expander phy were the the HBA
port, not the expander port.  If I'd taken that, I'd have rolled up all
the phys with separate ports into a spurious wide port.

> , so I actually had to rejig 
> > mptsas_probe_one_phy() to
> > recognise ports.  I also noticed that on an expander device, 
> > we allocate
> > and end device for the port that's connected to the HBA.  I didn't do
> > anything about this, since that was the behaviour of the old driver
> > (even if incorrect), so I assume the wide port update will fix this.
> 
> You must be refering to the rphy parent/child relationship.  I never
> understand why this concept was added, actually implemented by
> Christoph.
> 
> Also, I thought you were removing all the concepts of remote phys when
> you were
> adding the port API.  I notice the driver still has rphy_add and
> rphy_delete.
> Can't the rphy concepts die?

an rphy is effectively dead ... that's why the umbrella class is called
"sas_device".  It just means think on the other end of the port.

> BTW: - James, do you have the equipment to create a disk device that is
> a wide port?
> The way I'm doing it is by connecting two 1068 SAS controller to
> another.
> On controller is running in initiator mode, and the other running in
> target mode.
> We have a target mode driver that creates ramdisk images to simulate
> disk devices.  
> Every disk device on each phy containing the same sas address, thus
> forming a wide port.
> Connect both controller with a quad crossover cable containing four
> phys. 
> That is how I'm testing here in our labs. Do you have two 1068s so you
> can simulate this? 
> If not, allow me to assit you with this setup.

No ... I'm going to try wiring two expanders together in a wide port to
do this, but I suspect I'm going to have difficulty with the cable
conspiracy.

James

RE: [PATCH] scsi_transport_sas: introduce a sas_port entity

[Moore, Eric]

On Saturday, June 17, 2006 1:06 PM, James Bottomley wrote:
> 
> With the tiny amount of bandwidth I had left after doing sas_port, I
> couldn't find the time to look through a 100k patch that wasn't
> production quality to work out what it was doing.

Ok, I will try to work on it this week, and see what I can accomplish.
Next week I have to refocus on SLES10 drivers having my internal driver 
implementation of wide ports for our customer shipment; e.g.
the 100K patch I sent, since the wide port API is not there.

> 
> I just need the smallest patch that keeps mptsas compiling 
> while I push
> the class changes upstream.  You can patch it out in favour of yours
> when you get it working .. as long as you can solve the expander wide
> port issue.

The scsi-misc-2.6 patch with todays date in your kernel.org
home folder has all the wide port API changes backed out?  
Is that on purpose, and what is your plans for your 2.6.18
push to Linus having this?

http://www.kernel.org/pub/linux/kernel/people/jejb/


> 
> > How much testing was done on this patch?  I see sever regression
> > with the driver.  Hotplug it busted.  Hot adding any device will
> > panic.  Also, if you load the driver with devices, then unload the
> > driver, disconnect device, and reload, the driver panics. Also see 
> > some various panics just load/unload/load the driver stack, 
> > with no change of the topology.
> 
> It works for me in my configuration ... which is about the level of
> testing of everything I do.

Ok, then you've not tested hot adding and removing devices/expanders,
nor
unloading and loading the driver as a module.  After further review,
I notice the mptsas driver is not ever calling sas_port_delete_phy, nor
sas_port_delete at driver unload time, as well as hot removing expanders

(which is handled from mptsas_delete_expander_phys).


> 
> > > 
> > > What I did discover is that the actual firmware has no 
> > > concept of a port
> > > below the adapter
> > 
> > Yes it does.  The port_id field is valid when you've formed
> > a port by adding attached devices. This is found in 
> phy_info[]->port_id.
> > The driver retrieves this in sas_io_unit_pg0, device_pg0, and
> > expander_pg1.
> > The port_id is formed at the top level of the hba, and all 
> the devices
> > downstream
> > will all be on the same port_id, forming a sas domain.
> 
> No, it doesn't ... that's the point and the problem.  Ports are formed
> not only on the HBA but also on the expander. See for example the nice
> diagram under the SAS v2 standard (r02) section 4.1.10 Figure 29
> (Multiple Connections on Wide Ports).  The thing I'm trying to show,
> which the fusion BIOS apparently doesn't do is the wide ports on
> expanders.  Without this, we can't show the true tree topology.
> 
> I used the debugging mode of the driver to verify that for a wide port
> to an expander, all the port_id's of the expander phy were the the HBA
> port, not the expander port.  If I'd taken that, I'd have 
> rolled up all
> the phys with separate ports into a spurious wide port.

Correct.  Our implementation has all the port identifiers the same
as the port identifier downstream from the parent HBA.  However with
your's, all
expanders will be having 0 based port identifiers.  Meaning devices on
one expander will endup with the same port identifier as the devices an
another expanders, since index referencing for all expanders start at
zero.
So what this means if channel matters to anyone, or port identifier,
that
someone migrating from older kernels to newer kernel, will end up with
different values.  Id rather not change current implmentation, honoring 
the firmware port id.  Perhaps someday the firmware folks will fix it,
and
Linux drivers would be in line with what mpt firwmare provides.


> > Can't the rphy concepts die?
> 
> an rphy is effectively dead ... that's why the umbrella class 
> is called
> "sas_device".  It just means think on the other end of the port.

Ok fine.  I still think its not required as the port concept
has been a added.  Also, in the mpt driver the rphy instance 
is created for both sides of the port. There is a parent and child
relationship with objects pointing to each other.  
I wonder if the adaptec driver is doing the same?

> 
> No ... I'm going to try wiring two expanders together in a 
> wide port to
> do this, but I suspect I'm going to have difficulty with the cable
> conspiracy.

Let me know if you want a wide port end device.  Its nice to have that
for testing as well as wide ports between expanders.

RE: [PATCH] scsi_transport_sas: introduce a sas_port entity

[James Bottomley]

On Tue, 2006-06-20 at 11:37 -0600, Moore, Eric wrote:
> The scsi-misc-2.6 patch with todays date in your kernel.org
> home folder has all the wide port API changes backed out?  
> Is that on purpose, and what is your plans for your 2.6.18
> push to Linus having this?

I plan to push the current SCSI misc and hopefully follow on with the
wide port API before the 2.6.18 merge window closes.  I suspect the next
round of kernel releases will be Andrew's requested stability ones, so
if we don't get something in before 1 July, it will likely be 2.6.20 (or
four to six months) before we can get this API in.

James

RE: [PATCH] scsi_transport_sas: introduce a sas_port entity

[Moore, Eric]

On Tuesday, June 20, 2006 11:45 AM, James Bottomley wrote:

> 
> On Tue, 2006-06-20 at 11:37 -0600, Moore, Eric wrote:
> > The scsi-misc-2.6 patch with todays date in your kernel.org
> > home folder has all the wide port API changes backed out?  
> > Is that on purpose, and what is your plans for your 2.6.18
> > push to Linus having this?
> 
> I plan to push the current SCSI misc and hopefully follow on with the
> wide port API before the 2.6.18 merge window closes.  I 
> suspect the next
> round of kernel releases will be Andrew's requested stability ones, so
> if we don't get something in before 1 July, it will likely be 
> 2.6.20 (or
> four to six months) before we can get this API in.

Well, lets not rush this in till all the panics are fix'd.

Here's one of the issues:
Try compile the driver as a module.  Load -> unload -> load -> panic.
I suspect that in the scsi_transport_sas.c, the sas_remove_host is
not cleaning up the wide ports. My setup is one controller
with one expander attached. No end devices.

Here is my trace:

< LOAD DRIVER >

Jun 20 12:41:29 emoore-test6 kernel: Fusion MPT base driver 3.03.10
Jun 20 12:41:29 emoore-test6 kernel: Copyright (c) 1999-2005 LSI Logic
Corporation
Jun 20 12:41:29 emoore-test6 kernel: Fusion MPT SAS Host driver 3.03.10
Jun 20 12:41:29 emoore-test6 kernel: GSI 20 sharing vector 0xC9 and IRQ
20
Jun 20 12:41:29 emoore-test6 kernel: ACPI: PCI Interrupt 0000:09:02.0[A]
-> GSI 72 (level, low) -> IRQ 201
Jun 20 12:41:29 emoore-test6 kernel: mptbase: Initiating ioc0 bringup
Jun 20 12:41:30 emoore-test6 kernel: ioc0: SAS1068:
Capabilities={Initiator}
Jun 20 12:41:36 emoore-test6 kernel: mptbase: ioc0: MPT event:(0Ah) :
Events(ON) Change
Jun 20 12:41:36 emoore-test6 kernel: scsi2 : ioc0: LSISAS1068,
FwRev=00070f00h, Ports=1, MaxQ=511, IRQ=201

< UNLOAD DRIVER >

< LOAD DRIVER >

Jun 20 12:42:28 emoore-test6 kernel: Fusion MPT base driver 3.03.10
Jun 20 12:42:28 emoore-test6 kernel: Copyright (c) 1999-2005 LSI Logic
Corporation
Jun 20 12:42:28 emoore-test6 kernel: Fusion MPT SAS Host driver 3.03.10
Jun 20 12:42:28 emoore-test6 kernel: PCI: Enabling device 0000:09:02.0
(0156 -> 0157)
Jun 20 12:42:28 emoore-test6 kernel: ACPI: PCI Interrupt 0000:09:02.0[A]
-> GSI 72 (level, low) -> IRQ 201
Jun 20 12:42:28 emoore-test6 kernel: mptbase: Initiating ioc0 bringup
Jun 20 12:42:28 emoore-test6 kernel: ioc0: SAS1068:
Capabilities={Initiator}
Jun 20 12:42:35 emoore-test6 kergeneral protection fault: 0000 [1] Snel:
mptbase: ioMP c0: MPT event:(0
Ah) : Events(ON)last sysfs file: /class/scsi_host/host3/proc_name
 C
Entering kdb (current=0xffff81001e111040, pid 7313) on processor 1 Oops:
<NULL>
due to oops @ 0xffffffff802645cc
     r15 = 0x0000000000000000      r14 = 0xffff8100041c1450 
     r13 = 0xffffffff802647b1      r12 = 0xffff8100041c11d8 
     rbp = 0xffff8100041c1000      rbx = 0xffff81001f182700 
     r11 = 0x00000000000081a4      r10 = 0xffff81000e06c488 
      r9 = 0xffff81001fddccd0       r8 = 0xffff81000e06c488 
     rax = 0xffff81000927b000      rcx = 0xffff81000e06c488 
     rdx = 0x0000000000000000      rsi = 0xffff8100041c11d8 
     rdi = 0xffff81001f182700 orig_rax = 0xffffffffffffffff 
     rip = 0xffffffff802645cc       cs = 0x0000000000000010 
  eflags = 0x0000000000010283      rsp = 0xffff810006e61b60 
      ss = 0xffff810006e60000 &regs = 0xffff810006e61ac8
[1]kdb> b[1]kdb> bt[1]kdb> bt

Stack traceback for pid 7313
0xffff81001e111040     7313     7300  1    1   R  0xffff81001e111370
*insmod
RSP           RIP                Function (args)
0xffff810006e61b60 0xffffffff802645cc
attribute_container_device_trigger+0x49 (0xffff81000e06c488,
0xffff81000927b8c0, 0xffff81000e06c4e0)
0xffff810006e61b78 0xffffffff802645cf
attribute_container_device_trigger+0x4c (0xffff8100041c11d8,
0xffff8100041c11d8, 0xffff8100041c1000, 0x0, 0xffff8100041c11d8)
0xffff810006e61b98 0xffffffff802643a0
attribute_container_add_device+0x57 (0xffffffffffffffff,
0xffffffffffffffff, 0xffff81001e4f1090, 0xffff81001e4f0ed0,
0xffffffff88279238)
0xffff810006e61cb8 0xffffffff80203e87 pci_device_probe+0xf7
(0xffff81001e4f1090, 0xffff81001e4f0ed0, 0xffff81001e4fc638)
0xffff810006e61cf8 0xffffffff802614c0 driver_probe_device+0x52 (0x0,
0x0, 0xffffffff88279238, 0xffffffff80261593, 0xffffffff88279238)
0xffff810006e61d18 0xffffffff80261621 __driver_attach+0x8e
(0xffffffff80385ac8, 0xffffffff80385ae0, 0xffff81001e4f0fa8,
0xffffffff803858e0, 0xffffffff88279680)
0xffff810006e61d48 0xffffffff80260ea2 bus_for_each_dev+0x43
(0xffffffff88279238)
0xffff810006e61d98 0xffffffff802613fb driver_attach+0x1c (0xd0,
0xffffffff88279238, 0xffffffff88279680, 0xffffffff80370fa0,
0xffffffff88279680)
0xffff810006e61da8 0xffffffff80260b15 bus_add_driver+0x7e
(0xdead4ead00000001, 0xffffffff, 0xffffffffffffffff, 0xffffffff)
0xffff810006e61de8 0xffffffff80261927 driver_register+0xae
(0xdead4ead00000001, 0xffffffff, 0xffffffffffffffff, 0xffffffff8823b794,
0xffffffff8014a56e)
0xffff810006e61e18 0xffffffff8020409d __pci_register_driver+0x8e
(0xffffffff80370fa0)
0xffff810006e61e58 0xffffffff880970d4 [mptsas]mptsas_init+0xd4
0xffff810006e61e68 0xffffffff8014ce34 sys_init_module+0x162f
[1]kdb>

RE: [PATCH] scsi_transport_sas: introduce a sas_port entity

[James Bottomley]

On Tue, 2006-06-20 at 13:11 -0600, Moore, Eric wrote:
> Stack traceback for pid 7313
> 0xffff81001e111040     7313     7300  1    1   R  0xffff81001e111370
> *insmod
> RSP           RIP                Function (args)
> 0xffff810006e61b60 0xffffffff802645cc
> attribute_container_device_trigger+0x49 (0xffff81000e06c488,
> 0xffff81000927b8c0, 0xffff81000e06c4e0)
> 0xffff810006e61b78 0xffffffff802645cf
> attribute_container_device_trigger+0x4c (0xffff8100041c11d8,
> 0xffff8100041c11d8, 0xffff8100041c1000, 0x0, 0xffff8100041c11d8)
> 0xffff810006e61b98 0xffffffff802643a0
> attribute_container_add_device+0x57 (0xffffffffffffffff,
> 0xffffffffffffffff, 0xffff81001e4f1090, 0xffff81001e4f0ed0,

Oh, this one's actually quite simple: the phys aren't being deleted from
the expander.  Try this fix:

James

diff --git a/drivers/scsi/scsi_transport_sas.c b/drivers/scsi/scsi_transport_sas.c
index 2a5899d..e79d6d3 100644
--- a/drivers/scsi/scsi_transport_sas.c
+++ b/drivers/scsi/scsi_transport_sas.c
@@ -1176,7 +1176,7 @@ sas_rphy_delete(struct sas_rphy *rphy)
 		break;
 	case SAS_EDGE_EXPANDER_DEVICE:
 	case SAS_FANOUT_EXPANDER_DEVICE:
-		device_for_each_child(dev, NULL, do_sas_phy_delete);
+		sas_remove_children(dev);
 		break;
 	default:
 		break;

RE: [PATCH] scsi_transport_sas: introduce a sas_port entity

[Moore, Eric]

On Tuesday, June 20, 2006 3:44 PM, James Bottomley wrote:  

> Oh, this one's actually quite simple: the phys aren't being 
> deleted from
> the expander.  Try this fix:
> 
> James
> 
> diff --git a/drivers/scsi/scsi_transport_sas.c 
> b/drivers/scsi/scsi_transport_sas.c
> index 2a5899d..e79d6d3 100644
> --- a/drivers/scsi/scsi_transport_sas.c
> +++ b/drivers/scsi/scsi_transport_sas.c
> @@ -1176,7 +1176,7 @@ sas_rphy_delete(struct sas_rphy *rphy)
>  		break;
>  	case SAS_EDGE_EXPANDER_DEVICE:
>  	case SAS_FANOUT_EXPANDER_DEVICE:
> -		device_for_each_child(dev, NULL, do_sas_phy_delete);
> +		sas_remove_children(dev);
>  		break;
>  	default:
>  		break;


Yes, that solves that issue.

Thanks,
Eric