[PATCH 1/4] libsas: Don't BUG when connecting two expanders via wide port

[PATCH 1/4] libsas: Don't BUG when connecting two expanders via wide port

[Darrick J. Wong]

When a device is connected to an expander, the discovery process goes through
sas_ex_discover_dev to figure out what's attached to the phy.  If it is the
case that the phy being discovered happens to be the second phy of a wide link
to an expander, that discover_dev function will incorrectly call
sas_ex_discover_expander, which creates another sas_port and tries to attach the
other sas_phys to the new port, thus triggering a BUG.  The correct thing to do is
to check the other ex_phys of the expander to see if there's a sas_port for this
sas_phy, and attach the sas_phy to the existing sas_port.

This is easily triggered if one enables the phys of a wide port between
expanders one by one.

Signed-off-by: Darrick J. Wong <djwong@us.ibm.com>
---

 drivers/scsi/libsas/sas_expander.c |   30 ++++++++++++++++++++++++++++++
 1 files changed, 30 insertions(+), 0 deletions(-)

diff --git a/drivers/scsi/libsas/sas_expander.c b/drivers/scsi/libsas/sas_expander.c
index d9b9a00..3eeb3fb 100644
--- a/drivers/scsi/libsas/sas_expander.c
+++ b/drivers/scsi/libsas/sas_expander.c
@@ -678,6 +678,29 @@ static struct domain_device *sas_ex_disc
 	return NULL;
 }
 
+/* See if this phy is part of a wide port */
+static int sas_ex_join_wide_port(struct domain_device *parent, int phy_id)
+{
+	struct ex_phy *phy = &parent->ex_dev.ex_phy[phy_id];
+	int i;
+
+	for (i = 0; i < parent->ex_dev.num_phys; i++) {
+		struct ex_phy *ephy = &parent->ex_dev.ex_phy[i];
+
+		if (ephy == phy)
+			continue;
+
+		if (!memcmp(phy->attached_sas_addr, ephy->attached_sas_addr,
+			    SAS_ADDR_SIZE)) {
+			sas_port_add_phy(ephy->port, phy->phy);
+			phy->phy_state = PHY_DEVICE_DISCOVERED;
+			return 0;
+		}
+	}
+
+	return -ENODEV;
+}
+
 static struct domain_device *sas_ex_discover_expander(
 	struct domain_device *parent, int phy_id)
 {
@@ -810,6 +833,13 @@ static int sas_ex_discover_dev(struct do
 		return res;
 	}
 
+	res = sas_ex_join_wide_port(dev, phy_id);
+	if (!res) {
+		SAS_DPRINTK("Attaching ex phy%d to wide port %016llx\n",
+			    phy_id, SAS_ADDR(ex_phy->attached_sas_addr));
+		return res;
+	}
+
 	switch (ex_phy->attached_dev_type) {
 	case SAS_END_DEV:
 		child = sas_ex_discover_end_dev(dev, phy_id);

[PATCH 1/4 v2] libsas: Don't BUG when connecting two expanders via wide port

[Darrick J. Wong]

libsas: Don't BUG when connecting two expanders via wide port

When a device is connected to an expander, the discovery process goes through
sas_ex_discover_dev to figure out what's attached to the phy.  If it is the
case that the phy being discovered happens to be the second phy of a wide link
to an expander, that discover_dev function will incorrectly call
sas_ex_discover_expander, which creates another sas_port and tries to attach the
other sas_phys to the new port, thus triggering a BUG.  The correct thing to do is
to check the other ex_phys of the expander to see if there's a sas_port for this
sas_phy, and attach the sas_phy to the existing sas_port.

This is easily triggered if one enables the phys of a wide port between
expanders one by one.

This second version of the patch fixes a small regression in the case where
all the phys show up at once and we accidentally try to attach to a port
that hasn't been created yet.

Signed-off-by: Darrick J. Wong <djwong@us.ibm.com>
---

 drivers/scsi/libsas/sas_expander.c |   30 ++++++++++++++++++++++++++++++
 1 files changed, 30 insertions(+), 0 deletions(-)

diff --git a/drivers/scsi/libsas/sas_expander.c b/drivers/scsi/libsas/sas_expander.c
index 114e26c..2f3b8e1 100644
--- a/drivers/scsi/libsas/sas_expander.c
+++ b/drivers/scsi/libsas/sas_expander.c
@@ -736,6 +736,29 @@ static struct domain_device *sas_ex_disc
 	return NULL;
 }
 
+/* See if this phy is part of a wide port */
+static int sas_ex_join_wide_port(struct domain_device *parent, int phy_id)
+{
+	struct ex_phy *phy = &parent->ex_dev.ex_phy[phy_id];
+	int i;
+
+	for (i = 0; i < parent->ex_dev.num_phys; i++) {
+		struct ex_phy *ephy = &parent->ex_dev.ex_phy[i];
+
+		if (ephy == phy)
+			continue;
+
+		if (!memcmp(phy->attached_sas_addr, ephy->attached_sas_addr,
+			    SAS_ADDR_SIZE) && ephy->port) {
+			sas_port_add_phy(ephy->port, phy->phy);
+			phy->phy_state = PHY_DEVICE_DISCOVERED;
+			return 0;
+		}
+	}
+
+	return -ENODEV;
+}
+
 static struct domain_device *sas_ex_discover_expander(
 	struct domain_device *parent, int phy_id)
 {
@@ -868,6 +891,13 @@ static int sas_ex_discover_dev(struct do
 		return res;
 	}
 
+	res = sas_ex_join_wide_port(dev, phy_id);
+	if (!res) {
+		SAS_DPRINTK("Attaching ex phy%d to wide port %016llx\n",
+			    phy_id, SAS_ADDR(ex_phy->attached_sas_addr));
+		return res;
+	}
+
 	switch (ex_phy->attached_dev_type) {
 	case SAS_END_DEV:
 		child = sas_ex_discover_end_dev(dev, phy_id);

SAS illegal toplogies [was Re: [PATCH 1/4 v2] libsas: Don't BUG when connecting two expanders via wide port]

[Douglas Gilbert]

Darrick J. Wong wrote:
> libsas: Don't BUG when connecting two expanders via wide port
> 
> When a device is connected to an expander, the discovery process goes through
> sas_ex_discover_dev to figure out what's attached to the phy.  If it is the
> case that the phy being discovered happens to be the second phy of a wide link
> to an expander, that discover_dev function will incorrectly call
> sas_ex_discover_expander, which creates another sas_port and tries to attach the
> other sas_phys to the new port, thus triggering a BUG.  The correct thing to do is
> to check the other ex_phys of the expander to see if there's a sas_port for this
> sas_phy, and attach the sas_phy to the existing sas_port.
> 
> This is easily triggered if one enables the phys of a wide port between
> expanders one by one.
> 
> This second version of the patch fixes a small regression in the case where
> all the phys show up at once and we accidentally try to attach to a port
> that hasn't been created yet.

Darrick,
Okay.

Now I'm wondering what the discovery algorithm in libsas
does if it finds truly illegal connections between expanders.
The spec defines what is illegal but says it is vendor specific
what will be done.

One approach is to use the SMP PHY CONTROL function to disable
the phy (or the phys at both ends of the illegal link). The
next trick is how to tell the user who just connected a cable
between expanders that "you can't do that!". Tools like my
smp_discover could alert a user to a disabled phy but
without turning it back on (and causing the libsas discovery
algorithm another headache) my SMP utilities don't know what
it is connected to.

Another question is which link to disable. Imagine three
expanders interconnected with 3 links which is illegal.
Breaking any one link makes it legal, but which one
to break? Last seen, or perhaps the link which has
the largest SAS address sum ...

Doug Gilbert

[PATCH 2/4] libsas: Add an LU reset mechanism to the error handler

[Darrick J. Wong]

After discussion with andmike and dougg, it seems that the purpose of
eh_device_reset_handler is to issue LU resets, and that
eh_bus_reset_handler would be a more appropriate place for a phy reset.

Signed-off-by: Darrick J. Wong <djwong@us.ibm.com>
---

 drivers/scsi/libsas/sas_scsi_host.c |   40 ++++++++++++++++++++++++++++++++---
 include/scsi/libsas.h               |    1 +
 2 files changed, 37 insertions(+), 4 deletions(-)

diff --git a/drivers/scsi/libsas/sas_scsi_host.c b/drivers/scsi/libsas/sas_scsi_host.c
index 04eace9..261b85a 100644
--- a/drivers/scsi/libsas/sas_scsi_host.c
+++ b/drivers/scsi/libsas/sas_scsi_host.c
@@ -421,16 +421,37 @@ struct sas_phy *find_local_sas_phy(struc
 	return exphy->phy;
 }
 
-/* Attempt to send a target reset message to a device */
+/* Attempt to send a LUN reset message to a device */
 int sas_eh_device_reset_handler(struct scsi_cmnd *cmd)
 {
 	struct domain_device *dev = cmd_to_domain_dev(cmd);
+	struct sas_internal *i =
+		to_sas_internal(dev->port->ha->core.shost->transportt);
+	struct scsi_lun lun;
+	int res;
+
+	int_to_scsilun(cmd->device->lun, &lun);
+
+	if (!i->dft->lldd_lu_reset)
+		return FAILED;
+
+	res = i->dft->lldd_lu_reset(dev, lun.scsi_lun);
+	if (res == TMF_RESP_FUNC_SUCC || res == TMF_RESP_FUNC_COMPLETE)
+		return SUCCESS;
+
+	return FAILED;
+}
+
+/* Attempt to send a phy (bus) reset */
+int sas_eh_bus_reset_handler(struct scsi_cmnd *cmd)
+{
+	struct domain_device *dev = cmd_to_domain_dev(cmd);
 	struct sas_phy *phy = find_local_sas_phy(dev);
 	int res;
 
 	res = sas_phy_reset(phy, 1);
 	if (res)
-		SAS_DPRINTK("Device reset of %s failed 0x%x\n",
+		SAS_DPRINTK("Bus reset of %s failed 0x%x\n",
 			    phy->dev.kobj.k_name,
 			    res);
 	if (res == TMF_RESP_FUNC_SUCC || res == TMF_RESP_FUNC_COMPLETE)
@@ -443,10 +464,20 @@ int sas_eh_device_reset_handler(struct s
 static int try_to_reset_cmd_device(struct Scsi_Host *shost,
 				   struct scsi_cmnd *cmd)
 {
+	int res;
+
 	if (!shost->hostt->eh_device_reset_handler)
-		return FAILED;
+		goto try_bus_reset;
+
+	res = shost->hostt->eh_device_reset_handler(cmd);
+	if (res == SUCCESS)
+		return res;
 
-	return shost->hostt->eh_device_reset_handler(cmd);
+try_bus_reset:
+	if (shost->hostt->eh_bus_reset_handler)
+		return shost->hostt->eh_bus_reset_handler(cmd);
+
+	return FAILED;
 }
 
 static int sas_eh_handle_sas_errors(struct Scsi_Host *shost,
@@ -976,3 +1007,4 @@ EXPORT_SYMBOL_GPL(sas_task_abort);
 EXPORT_SYMBOL_GPL(sas_phy_reset);
 EXPORT_SYMBOL_GPL(sas_phy_enable);
 EXPORT_SYMBOL_GPL(sas_eh_device_reset_handler);
+EXPORT_SYMBOL_GPL(sas_eh_bus_reset_handler);
diff --git a/include/scsi/libsas.h b/include/scsi/libsas.h
index b200233..8516ba6 100644
--- a/include/scsi/libsas.h
+++ b/include/scsi/libsas.h
@@ -661,5 +661,6 @@ void sas_init_dev(struct domain_device *
 void sas_task_abort(struct sas_task *);
 int __sas_task_abort(struct sas_task *);
 int sas_eh_device_reset_handler(struct scsi_cmnd *cmd);
+int sas_eh_bus_reset_handler(struct scsi_cmnd *cmd);
 
 #endif /* _SASLIB_H_ */

[PATCH 3/4] aic94xx: Remove TMF result code munging

[Darrick J. Wong]

In asd_initiate_ssp_tmf, the TMF result code is replaced with
TMF_RESP_FUNC_FAILED except when the TMF returns a result code immediately.
However, TMFs can return result codes via an ESCB... yet these codes are
also replaced with "FAILED".  The only values that can fall into that case
are TMF_* codes anyway, so get rid of this code where COMPLETE and SUCCESS
are turned into FAILED.  This also lets us propagate those TMF_* codes up
to the caller.

Signed-off-by: Darrick J. Wong <djwong@us.ibm.com>
---

 drivers/scsi/aic94xx/aic94xx_tmf.c |    9 +--------
 1 files changed, 1 insertions(+), 8 deletions(-)

diff --git a/drivers/scsi/aic94xx/aic94xx_tmf.c b/drivers/scsi/aic94xx/aic94xx_tmf.c
index fd5269e..9a14a6d 100644
--- a/drivers/scsi/aic94xx/aic94xx_tmf.c
+++ b/drivers/scsi/aic94xx/aic94xx_tmf.c
@@ -566,14 +566,7 @@ static int asd_initiate_ssp_tmf(struct d
 		res = TMF_RESP_FUNC_ESUPP;
 		break;
 	default:
-		if (tmf == TMF_QUERY_TASK) {
-			ASD_DPRINTK("%s: QUERY_SSP_TASK response: 0x%x\n",
-				    __FUNCTION__, res);
-			break;
-		}
-		ASD_DPRINTK("%s: converting result 0x%x to TMF_RESP_FUNC_FAILED\n",
-			    __FUNCTION__, res);
-		res = TMF_RESP_FUNC_FAILED;
+		/* Allow TMF response codes to propagate upwards */
 		break;
 	}
 out_err:

[PATCH 4/4] aic94xx: Add default bus reset handler

[Darrick J. Wong]

Signed-off-by: Darrick J. Wong <djwong@us.ibm.com>
---

 drivers/scsi/aic94xx/aic94xx_init.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/drivers/scsi/aic94xx/aic94xx_init.c b/drivers/scsi/aic94xx/aic94xx_init.c
index 3aa568f..bc7744e 100644
--- a/drivers/scsi/aic94xx/aic94xx_init.c
+++ b/drivers/scsi/aic94xx/aic94xx_init.c
@@ -80,6 +80,7 @@ static struct scsi_host_template aic94xx
 	.max_sectors		= SCSI_DEFAULT_MAX_SECTORS,
 	.use_clustering		= ENABLE_CLUSTERING,
 	.eh_device_reset_handler	= sas_eh_device_reset_handler,
+	.eh_bus_reset_handler	= sas_eh_bus_reset_handler,
 };
 
 static int __devinit asd_map_memio(struct asd_ha_struct *asd_ha)