[PATCH] libsas: fix definition of wideport, include local sas address

[PATCH] libsas: fix definition of wideport, include local sas address

[Dan Williams]

To date libsas has only looked at the attached sas address when
determining the formation of wide ports.  The specification and some
hardware expects that phys with different addresses will not form a wide
port unless the local peer phys also match each other.  Introduce a flag
to select stricter behavior at sas_register_ha() time.  The flag can be
dropped once it is known that all libsas users expect the same behavior.

Current drivers just initialize this field to zero and get the
traditional behavior.

Reported-by: Patrick Thomson <patrick.s.thomson@intel.com>
Cc: Jeff Skirvin <jeffrey.d.skirvin@intel.com>
Cc: Jack Wang <jack_wang@usish.com>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 drivers/scsi/libsas/sas_port.c |   18 +++++++++++++-----
 include/scsi/libsas.h          |    2 ++
 2 files changed, 15 insertions(+), 5 deletions(-)

diff --git a/drivers/scsi/libsas/sas_port.c b/drivers/scsi/libsas/sas_port.c
index fe8b74c..5257fdf 100644
--- a/drivers/scsi/libsas/sas_port.c
+++ b/drivers/scsi/libsas/sas_port.c
@@ -28,6 +28,17 @@
 #include <scsi/scsi_transport_sas.h>
 #include "../scsi_sas_internal.h"
 
+static bool phy_is_wideport_member(struct asd_sas_port *port, struct asd_sas_phy *phy)
+{
+	struct sas_ha_struct *sas_ha = phy->ha;
+
+	if (memcmp(port->attached_sas_addr, phy->attached_sas_addr,
+		   SAS_ADDR_SIZE) != 0 || (sas_ha->strict_wide_ports &&
+	     memcmp(port->sas_addr, phy->sas_addr, SAS_ADDR_SIZE) != 0))
+		return false;
+	return true;
+}
+
 /**
  * sas_form_port -- add this phy to a port
  * @phy: the phy of interest
@@ -45,8 +56,7 @@ static void sas_form_port(struct asd_sas_phy *phy)
 	unsigned long flags;
 
 	if (port) {
-		if (memcmp(port->attached_sas_addr, phy->attached_sas_addr,
-			   SAS_ADDR_SIZE) != 0)
+		if (!phy_is_wideport_member(port, phy))
 			sas_deform_port(phy);
 		else {
 			SAS_DPRINTK("%s: phy%d belongs to port%d already(%d)!\n",
@@ -62,9 +72,7 @@ static void sas_form_port(struct asd_sas_phy *phy)
 		port = sas_ha->sas_port[i];
 		spin_lock(&port->phy_list_lock);
 		if (*(u64 *) port->sas_addr &&
-		    memcmp(port->attached_sas_addr,
-			   phy->attached_sas_addr, SAS_ADDR_SIZE) == 0 &&
-		    port->num_phys > 0) {
+		    phy_is_wideport_member(port, phy) && port->num_phys > 0) {
 			/* wide port */
 			SAS_DPRINTK("phy%d matched wide port%d\n", phy->id,
 				    port->id);
diff --git a/include/scsi/libsas.h b/include/scsi/libsas.h
index 3dec194..5173bba 100644
--- a/include/scsi/libsas.h
+++ b/include/scsi/libsas.h
@@ -361,6 +361,8 @@ struct sas_ha_struct {
 	/* The class calls this to send a task for execution. */
 	int lldd_max_execute_num;
 	int lldd_queue_size;
+	int strict_wide_ports; /* both sas_addr and attached_sas_addr must match
+				* their siblings when forming wide ports */
 
 	/* LLDD calls these to notify the class of an event. */
 	void (*notify_ha_event)(struct sas_ha_struct *, enum ha_event);

RE: [PATCH] libsas: fix definition of wideport, include local sas address

[Jack Wang]

To date libsas has only looked at the attached sas address when
determining the formation of wide ports.  The specification and some
hardware expects that phys with different addresses will not form a wide
port unless the local peer phys also match each other.  Introduce a flag
to select stricter behavior at sas_register_ha() time.  The flag can be
dropped once it is known that all libsas users expect the same behavior.

Current drivers just initialize this field to zero and get the
traditional behavior.
[Jack] I prefer to use the new definition of wideport to the default. 
Reported-by: Patrick Thomson <patrick.s.thomson@intel.com>
Cc: Jeff Skirvin <jeffrey.d.skirvin@intel.com>
Cc: Jack Wang <jack_wang@usish.com>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 drivers/scsi/libsas/sas_port.c |   18 +++++++++++++-----
 include/scsi/libsas.h          |    2 ++
 2 files changed, 15 insertions(+), 5 deletions(-)

diff --git a/drivers/scsi/libsas/sas_port.c b/drivers/scsi/libsas/sas_port.c
index fe8b74c..5257fdf 100644
--- a/drivers/scsi/libsas/sas_port.c
+++ b/drivers/scsi/libsas/sas_port.c
@@ -28,6 +28,17 @@
 #include <scsi/scsi_transport_sas.h>
 #include "../scsi_sas_internal.h"
 
+static bool phy_is_wideport_member(struct asd_sas_port *port, struct
asd_sas_phy *phy)
+{
+	struct sas_ha_struct *sas_ha = phy->ha;
+
+	if (memcmp(port->attached_sas_addr, phy->attached_sas_addr,
+		   SAS_ADDR_SIZE) != 0 || (sas_ha->strict_wide_ports &&
+	     memcmp(port->sas_addr, phy->sas_addr, SAS_ADDR_SIZE) != 0))
+		return false;
+	return true;
+}
+
 /**
  * sas_form_port -- add this phy to a port
  * @phy: the phy of interest
@@ -45,8 +56,7 @@ static void sas_form_port(struct asd_sas_phy *phy)
 	unsigned long flags;
 
 	if (port) {
-		if (memcmp(port->attached_sas_addr, phy->attached_sas_addr,
-			   SAS_ADDR_SIZE) != 0)
+		if (!phy_is_wideport_member(port, phy))
 			sas_deform_port(phy);
 		else {
 			SAS_DPRINTK("%s: phy%d belongs to port%d
already(%d)!\n",
@@ -62,9 +72,7 @@ static void sas_form_port(struct asd_sas_phy *phy)
 		port = sas_ha->sas_port[i];
 		spin_lock(&port->phy_list_lock);
 		if (*(u64 *) port->sas_addr &&
-		    memcmp(port->attached_sas_addr,
-			   phy->attached_sas_addr, SAS_ADDR_SIZE) == 0 &&
-		    port->num_phys > 0) {
+		    phy_is_wideport_member(port, phy) && port->num_phys > 0)
{
 			/* wide port */
 			SAS_DPRINTK("phy%d matched wide port%d\n", phy->id,
 				    port->id);
diff --git a/include/scsi/libsas.h b/include/scsi/libsas.h
index 3dec194..5173bba 100644
--- a/include/scsi/libsas.h
+++ b/include/scsi/libsas.h
@@ -361,6 +361,8 @@ struct sas_ha_struct {
 	/* The class calls this to send a task for execution. */
 	int lldd_max_execute_num;
 	int lldd_queue_size;
+	int strict_wide_ports; /* both sas_addr and attached_sas_addr must
match
+				* their siblings when forming wide ports */
 
 	/* LLDD calls these to notify the class of an event. */
 	void (*notify_ha_event)(struct sas_ha_struct *, enum ha_event);
[Jack] I cann't find where to judge the flag to chose which definition to
use?

Re: [PATCH] libsas: fix definition of wideport, include local sas address

[Dan Williams]

On Sat, Oct 2, 2010 at 6:55 PM, Jack Wang <jack_wang@usish.com> wrote:
>
> To date libsas has only looked at the attached sas address when
> determining the formation of wide ports.  The specification and some
> hardware expects that phys with different addresses will not form a wide
> port unless the local peer phys also match each other.  Introduce a flag
> to select stricter behavior at sas_register_ha() time.  The flag can be
> dropped once it is known that all libsas users expect the same behavior.
>
> Current drivers just initialize this field to zero and get the
> traditional behavior.
> [Jack] I prefer to use the new definition of wideport to the default.

Ok, when mvsas and aic94xx are also ready for this change we can just
make this the default and kill the flag.

> [Jack] I cann't find where to judge the flag to chose which definition to
> use?

I am not clear on the question.  phy_is_wideport_member() should be
the only location that needs to read this flag.  The driver just sets
it to 1 before calling sas_register_ha() and then assumes the strict
definition is in effect for the lifetime of the driver instance.

--
Dan
--
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: [PATCH] libsas: fix definition of wideport, include local sas address

[Jack Wang]

Re: [PATCH] libsas: fix definition of wideport, include local sas address

On Sat, Oct 2, 2010 at 6:55 PM, Jack Wang <jack_wang@usish.com> wrote:
>
> To date libsas has only looked at the attached sas address when
> determining the formation of wide ports.  The specification and some
> hardware expects that phys with different addresses will not form a wide
> port unless the local peer phys also match each other.  Introduce a flag
> to select stricter behavior at sas_register_ha() time.  The flag can be
> dropped once it is known that all libsas users expect the same behavior.
>
> Current drivers just initialize this field to zero and get the
> traditional behavior.
> [Jack] I prefer to use the new definition of wideport to the default.

Ok, when mvsas and aic94xx are also ready for this change we can just
make this the default and kill the flag.

> [Jack] I cann't find where to judge the flag to chose which definition to
> use?

I am not clear on the question.  phy_is_wideport_member() should be
the only location that needs to read this flag.  The driver just sets
it to 1 before calling sas_register_ha() and then assumes the strict
definition is in effect for the lifetime of the driver instance.
[Jack] Ohh, I miss something in the patch. It's ok for me. Thanks for
Explanation.

--
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