[isci PATCH v3 0/4] isci: suspend/resume support+

[isci PATCH v3 0/4] isci: suspend/resume support+

[Dan Williams]

Changes since v2: http://marc.info/?l=linux-scsi&m=133144999728737&w=2

This topic has been rebased on top of the libsas topic / scsi-misc:
http://marc.info/?l=linux-scsi&m=133170574921314&w=2

1/ Fixed handling of the 'device unplugged during suspend' case.
   sd_remove() sets up an ABBA deadlock with the driver-core device_lock()
   and async_synchronize_full(), so this version arranges for resume
   actions to be flushed before attempting removals.

2/ Add a new fix for a case where the isci driver inadvertently filters
   BCNs

3/ Fix up compile breakage in the CONFIG_PM=n case

4/ Minor fixups for the rebase (sas_init_dev reindent collision)

[isci PATCH v3 1/4] libata: export ata_port suspend/resume infrastructure for sas
[isci PATCH v3 2/4] libsas: drop sata port multiplier infrastructure
[isci PATCH v3 3/4] libsas: suspend / resume support
[isci PATCH v3 4/4] isci: enable BCN in sci_port_add_phy()

Full diffstat versus scsi-misc (cd8df93..isci-for-3.4-v3):

Andrzej Jakowski (1):
      isci: Changes in COMSAS timings enabling ISCI to detect buggy disc drives.

Artur Wojcik (1):
      isci: implement suspend/resume support

Dan Williams (19):
      libsas: introduce sas_work to fix sas_drain_work vs sas_queue_work
      libsas: cleanup spurious calls to scsi_schedule_eh
      libata, libsas: introduce sched_eh and end_eh port ops
      libsas: enforce eh strategy handlers only in eh context
      isci: improve 'invalid state' warnings
      isci: kill ->is_direct_attached
      isci: kill sci_phy_protocol and sci_request_protocol
      isci: kill ->status, and ->state_lock in isci_host
      isci: kill isci_port.domain_dev_list
      isci: refactor initialization for S3/S4
      isci: fix controller stop
      isci: fix 'link-up' events occur after 'start-complete'
      isci: fix interrupt disable
      isci: kill isci_host.shost
      libata: make ata_print_id atomic
      libsas: continue revalidation
      libata: export ata_port suspend/resume infrastructure for sas
      libsas: drop sata port multiplier infrastructure
      libsas: suspend / resume support

Jesper Juhl (1):
      isci: Just #include "host.h" once in host.c

Maciej Trela (1):
      isci: enable BCN in sci_port_add_phy()

Tom Jackson (1):
      isci: Don't filter BROADCAST CHANGE primitives

 drivers/ata/libata-core.c                     |   66 +++-
 drivers/ata/libata-eh.c                       |   57 ++-
 drivers/ata/libata-scsi.c                     |    4 +-
 drivers/ata/libata.h                          |    2 +-
 drivers/scsi/isci/host.c                      |  565 +++++++++----------------
 drivers/scsi/isci/host.h                      |  118 ++----
 drivers/scsi/isci/init.c                      |  271 +++++++++++--
 drivers/scsi/isci/phy.c                       |   74 +++-
 drivers/scsi/isci/phy.h                       |    9 +-
 drivers/scsi/isci/port.c                      |   23 +-
 drivers/scsi/isci/port.h                      |    6 -
 drivers/scsi/isci/port_config.c               |   18 +-
 drivers/scsi/isci/probe_roms.c                |   12 -
 drivers/scsi/isci/probe_roms.h                |    2 -
 drivers/scsi/isci/registers.h                 |    8 +
 drivers/scsi/isci/remote_device.c             |   29 +-
 drivers/scsi/isci/remote_device.h             |    1 -
 drivers/scsi/isci/remote_node_context.c       |   60 ++--
 drivers/scsi/isci/request.c                   |   19 +-
 drivers/scsi/isci/request.h                   |    9 +-
 drivers/scsi/isci/unsolicited_frame_control.c |   30 +-
 drivers/scsi/isci/unsolicited_frame_control.h |    6 +-
 drivers/scsi/libsas/sas_ata.c                 |  149 ++++++--
 drivers/scsi/libsas/sas_discover.c            |  132 +++++--
 drivers/scsi/libsas/sas_dump.c                |    1 +
 drivers/scsi/libsas/sas_event.c               |   40 +-
 drivers/scsi/libsas/sas_expander.c            |    8 +-
 drivers/scsi/libsas/sas_init.c                |  129 +++++-
 drivers/scsi/libsas/sas_internal.h            |    7 +-
 drivers/scsi/libsas/sas_phy.c                 |   42 ++-
 drivers/scsi/libsas/sas_port.c                |   67 +++-
 drivers/scsi/libsas/sas_scsi_host.c           |  153 ++++++-
 include/linux/libata.h                        |   15 +
 include/scsi/libsas.h                         |   76 +++-
 include/scsi/sas.h                            |    1 +
 include/scsi/sas_ata.h                        |   19 +-
 36 files changed, 1409 insertions(+), 819 deletions(-)

[isci PATCH v3 1/4] libata: export ata_port suspend/resume infrastructure for sas

[Dan Williams]

Reuse ata_port_{suspend|resume}_common for sas.  This path is chosen
over adding coordination between ata-tranport and sas-transport because
libsas wants to revalidate the domain at resume-time at the host level.
It can not validate links have resumed properly until libata has had a
chance to perform its revalidation, and any sane placing of an ata_port
in the sas-transport model would delay it's resumption until after the
host.

Export the common portion of port suspend/resume (bypass pm_runtime),
and allow sas to perform these operations asynchronously (similar to the
libsas async-ata probe implmentation).  Async operation is determined by
having an external, rather than stack based, location for storing the
result of the operation.

Reviewed-by: Jacek Danecki <jacek.danecki@intel.com>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 drivers/ata/libata-core.c |   58 ++++++++++++++++++++++++++++++++++++---------
 include/linux/libata.h    |   11 +++++++++
 2 files changed, 57 insertions(+), 12 deletions(-)

diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index bd79c8b..a13c36d 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -5241,16 +5241,20 @@ bool ata_link_offline(struct ata_link *link)
 #ifdef CONFIG_PM
 static int ata_port_request_pm(struct ata_port *ap, pm_message_t mesg,
 			       unsigned int action, unsigned int ehi_flags,
-			       int wait)
+			       int *async)
 {
 	struct ata_link *link;
 	unsigned long flags;
-	int rc;
+	int rc = 0;
 
 	/* Previous resume operation might still be in
 	 * progress.  Wait for PM_PENDING to clear.
 	 */
 	if (ap->pflags & ATA_PFLAG_PM_PENDING) {
+		if (async) {
+			*async = -EAGAIN;
+			return 0;
+		}
 		ata_port_wait_eh(ap);
 		WARN_ON(ap->pflags & ATA_PFLAG_PM_PENDING);
 	}
@@ -5259,10 +5263,10 @@ static int ata_port_request_pm(struct ata_port *ap, pm_message_t mesg,
 	spin_lock_irqsave(ap->lock, flags);
 
 	ap->pm_mesg = mesg;
-	if (wait) {
-		rc = 0;
+	if (async)
+		ap->pm_result = async;
+	else
 		ap->pm_result = &rc;
-	}
 
 	ap->pflags |= ATA_PFLAG_PM_PENDING;
 	ata_for_each_link(link, ap, HOST_FIRST) {
@@ -5275,7 +5279,7 @@ static int ata_port_request_pm(struct ata_port *ap, pm_message_t mesg,
 	spin_unlock_irqrestore(ap->lock, flags);
 
 	/* wait and check result */
-	if (wait) {
+	if (!async) {
 		ata_port_wait_eh(ap);
 		WARN_ON(ap->pflags & ATA_PFLAG_PM_PENDING);
 	}
@@ -5285,9 +5289,8 @@ static int ata_port_request_pm(struct ata_port *ap, pm_message_t mesg,
 
 #define to_ata_port(d) container_of(d, struct ata_port, tdev)
 
-static int ata_port_suspend_common(struct device *dev, pm_message_t mesg)
+static int __ata_port_suspend_common(struct ata_port *ap, pm_message_t mesg, int *async)
 {
-	struct ata_port *ap = to_ata_port(dev);
 	unsigned int ehi_flags = ATA_EHI_QUIET;
 	int rc;
 
@@ -5302,10 +5305,17 @@ static int ata_port_suspend_common(struct device *dev, pm_message_t mesg)
 	if (mesg.event == PM_EVENT_SUSPEND)
 		ehi_flags |= ATA_EHI_NO_AUTOPSY | ATA_EHI_NO_RECOVERY;
 
-	rc = ata_port_request_pm(ap, mesg, 0, ehi_flags, 1);
+	rc = ata_port_request_pm(ap, mesg, 0, ehi_flags, async);
 	return rc;
 }
 
+static int ata_port_suspend_common(struct device *dev, pm_message_t mesg)
+{
+	struct ata_port *ap = to_ata_port(dev);
+
+	return __ata_port_suspend_common(ap, mesg, NULL);
+}
+
 static int ata_port_suspend(struct device *dev)
 {
 	if (pm_runtime_suspended(dev))
@@ -5330,16 +5340,22 @@ static int ata_port_poweroff(struct device *dev)
 	return ata_port_suspend_common(dev, PMSG_HIBERNATE);
 }
 
-static int ata_port_resume_common(struct device *dev)
+static int __ata_port_resume_common(struct ata_port *ap, int *async)
 {
-	struct ata_port *ap = to_ata_port(dev);
 	int rc;
 
 	rc = ata_port_request_pm(ap, PMSG_ON, ATA_EH_RESET,
-		ATA_EHI_NO_AUTOPSY | ATA_EHI_QUIET, 1);
+		ATA_EHI_NO_AUTOPSY | ATA_EHI_QUIET, async);
 	return rc;
 }
 
+static int ata_port_resume_common(struct device *dev)
+{
+	struct ata_port *ap = to_ata_port(dev);
+
+	return __ata_port_resume_common(ap, NULL);
+}
+
 static int ata_port_resume(struct device *dev)
 {
 	int rc;
@@ -5372,6 +5388,24 @@ static const struct dev_pm_ops ata_port_pm_ops = {
 	.runtime_idle = ata_port_runtime_idle,
 };
 
+/* sas ports don't participate in pm runtime management of ata_ports,
+ * and need to resume ata devices at the domain level, not the per-port
+ * level. sas suspend/resume is async to allow parallel port recovery
+ * since sas has multiple ata_port instances per Scsi_Host.
+ */
+int ata_sas_port_async_suspend(struct ata_port *ap, int *async)
+{
+	return __ata_port_suspend_common(ap, PMSG_SUSPEND, async);
+}
+EXPORT_SYMBOL_GPL(ata_sas_port_async_suspend);
+
+int ata_sas_port_async_resume(struct ata_port *ap, int *async)
+{
+	return __ata_port_resume_common(ap, async);
+}
+EXPORT_SYMBOL_GPL(ata_sas_port_async_resume);
+
+
 /**
  *	ata_host_suspend - suspend host
  *	@host: host to suspend
diff --git a/include/linux/libata.h b/include/linux/libata.h
index 83ff256..e2da9c0 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -1013,6 +1013,17 @@ extern bool ata_link_offline(struct ata_link *link);
 #ifdef CONFIG_PM
 extern int ata_host_suspend(struct ata_host *host, pm_message_t mesg);
 extern void ata_host_resume(struct ata_host *host);
+extern int ata_sas_port_async_suspend(struct ata_port *ap, int *async);
+extern int ata_sas_port_async_resume(struct ata_port *ap, int *async);
+#else
+static inline int ata_sas_port_async_suspend(struct ata_port *ap, int *async)
+{
+	return 0;
+}
+static inline int ata_sas_port_async_resume(struct ata_port *ap, int *async)
+{
+	return 0;
+}
 #endif
 extern int ata_ratelimit(void);
 extern void ata_msleep(struct ata_port *ap, unsigned int msecs);

[isci PATCH v3 2/4] libsas: drop sata port multiplier infrastructure

[Dan Williams]

On the way to add a new sata_device field, noticed that libsas is
carrying port multiplier infrastructure that is explicitly disabled by
sas_discover_sata().  The aic94xx touches the unused port_no, so leave
that field in case there was some use for it.

Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 drivers/scsi/libsas/sas_discover.c |    6 ------
 include/scsi/libsas.h              |    1 -
 2 files changed, 0 insertions(+), 7 deletions(-)

diff --git a/drivers/scsi/libsas/sas_discover.c b/drivers/scsi/libsas/sas_discover.c
index c140355..2864b17 100644
--- a/drivers/scsi/libsas/sas_discover.c
+++ b/drivers/scsi/libsas/sas_discover.c
@@ -46,12 +46,6 @@ void sas_init_dev(struct domain_device *dev)
 		INIT_LIST_HEAD(&dev->ex_dev.children);
 		mutex_init(&dev->ex_dev.cmd_mutex);
 		break;
-	case SATA_DEV:
-	case SATA_PM:
-	case SATA_PM_PORT:
-	case SATA_PENDING:
-		INIT_LIST_HEAD(&dev->sata_dev.children);
-		break;
 	default:
 		break;
 	}
diff --git a/include/scsi/libsas.h b/include/scsi/libsas.h
index 1b14d80..ad5229a 100644
--- a/include/scsi/libsas.h
+++ b/include/scsi/libsas.h
@@ -167,7 +167,6 @@ struct sata_device {
         enum   ata_command_set command_set;
         struct smp_resp        rps_resp; /* report_phy_sata_resp */
         u8     port_no;        /* port number, if this is a PM (Port) */
-        struct list_head children; /* PM Ports if this is a PM */
 
 	struct ata_port *ap;
 	struct ata_host ata_host;

[isci PATCH v3 3/4] libsas: suspend / resume support

[Dan Williams]

libsas power management routines to suspend and recover the sas domain
based on a model where the lldd is allowed and expected to be
"forgetful".

sas_suspend_ha - disable event processing allowing the lldd to take down
                 links without concern for causing hotplug events.
                 Regardless of whether the lldd actually posts link down
                 messages libsas notifies the lldd that all
                 domain_devices are gone.

sas_prep_resume_ha - on the way back up before the lldd starts link
                     training clean out any spurious events that were
                     generated on the way down, and re-enable event
                     processing

sas_resume_ha - after the lldd has started and decided that all phys
		have posted link-up events this routine is called to let
		libsas start it's own timeout of any phys that did not
		resume.  After the timeout an lldd can cancel the
                phy teardown by posting a link-up event.

Storage for ex_change_count (u16) and phy_change_count (u8) are changed
to int so they can be set to -1 to indicate 'invalidated'.

There's a hack added to sas_destruct_devices to workaround a deadlock
when a device is removed across the suspend cycle.  sd_remove is called
under device_lock() and wants to async_synchronize_full(), while the
resume path is running in an async callback and is trying to grab
device_lock()... fun ensues.  So we guarantee that async resume actions
have been flushed before allowing new invocations of sd_remove.

Cc: Alan Stern <stern@rowland.harvard.edu>
Reviewed-by: Jacek Danecki <jacek.danecki@intel.com>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 drivers/scsi/libsas/sas_ata.c      |   86 ++++++++++++++++++++++++++++++
 drivers/scsi/libsas/sas_discover.c |   81 +++++++++++++++++++++++++----
 drivers/scsi/libsas/sas_dump.c     |    1 
 drivers/scsi/libsas/sas_event.c    |    4 +
 drivers/scsi/libsas/sas_init.c     |  102 ++++++++++++++++++++++++++++++++++++
 drivers/scsi/libsas/sas_internal.h |    1 
 drivers/scsi/libsas/sas_phy.c      |   21 +++++++
 drivers/scsi/libsas/sas_port.c     |   52 ++++++++++++++++++
 include/scsi/libsas.h              |   21 ++++++-
 include/scsi/sas_ata.h             |   10 ++++
 10 files changed, 360 insertions(+), 19 deletions(-)

diff --git a/drivers/scsi/libsas/sas_ata.c b/drivers/scsi/libsas/sas_ata.c
index d30a093..b161bdd 100644
--- a/drivers/scsi/libsas/sas_ata.c
+++ b/drivers/scsi/libsas/sas_ata.c
@@ -677,6 +677,92 @@ void sas_probe_sata(struct asd_sas_port *port)
 		if (ata_dev_disabled(sas_to_ata_dev(dev)))
 			sas_fail_probe(dev, __func__, -ENODEV);
 	}
+
+}
+
+static bool sas_ata_flush_pm_eh(struct asd_sas_port *port, const char *func)
+{
+	struct domain_device *dev, *n;
+	bool retry = false;
+
+	list_for_each_entry_safe(dev, n, &port->dev_list, dev_list_node) {
+		int rc;
+
+		if (!dev_is_sata(dev))
+			continue;
+
+		sas_ata_wait_eh(dev);
+		rc = dev->sata_dev.pm_result;
+		if (rc == -EAGAIN)
+			retry = true;
+		else if (rc) {
+			/* since we don't have a
+			 * ->port_{suspend|resume} routine in our
+			 *  ata_port ops, and no entanglements with
+			 *  acpi, suspend should just be mechanical trip
+			 *  through eh, catch cases where these
+			 *  assumptions are invalidated
+			 */
+			WARN_ONCE(1, "failed %s %s error: %d\n", func,
+				 dev_name(&dev->rphy->dev), rc);
+		}
+
+		/* if libata failed to power manage the device, tear it down */
+		if (ata_dev_disabled(sas_to_ata_dev(dev)))
+			sas_fail_probe(dev, func, -ENODEV);
+	}
+
+	return retry;
+}
+
+void sas_suspend_sata(struct asd_sas_port *port)
+{
+	struct domain_device *dev;
+
+ retry:
+	mutex_lock(&port->ha->disco_mutex);
+	list_for_each_entry(dev, &port->dev_list, dev_list_node) {
+		struct sata_device *sata;
+
+		if (!dev_is_sata(dev))
+			continue;
+
+		sata = &dev->sata_dev;
+		if (sata->ap->pm_mesg.event == PM_EVENT_SUSPEND)
+			continue;
+
+		sata->pm_result = -EIO;
+		ata_sas_port_async_suspend(sata->ap, &sata->pm_result);
+	}
+	mutex_unlock(&port->ha->disco_mutex);
+
+	if (sas_ata_flush_pm_eh(port, __func__))
+		goto retry;
+}
+
+void sas_resume_sata(struct asd_sas_port *port)
+{
+	struct domain_device *dev;
+
+ retry:
+	mutex_lock(&port->ha->disco_mutex);
+	list_for_each_entry(dev, &port->dev_list, dev_list_node) {
+		struct sata_device *sata;
+
+		if (!dev_is_sata(dev))
+			continue;
+
+		sata = &dev->sata_dev;
+		if (sata->ap->pm_mesg.event == PM_EVENT_ON)
+			continue;
+
+		sata->pm_result = -EIO;
+		ata_sas_port_async_resume(sata->ap, &sata->pm_result);
+	}
+	mutex_unlock(&port->ha->disco_mutex);
+
+	if (sas_ata_flush_pm_eh(port, __func__))
+		goto retry;
 }
 
 /**
diff --git a/drivers/scsi/libsas/sas_discover.c b/drivers/scsi/libsas/sas_discover.c
index 2864b17..007917e 100644
--- a/drivers/scsi/libsas/sas_discover.c
+++ b/drivers/scsi/libsas/sas_discover.c
@@ -24,6 +24,7 @@
 
 #include <linux/scatterlist.h>
 #include <linux/slab.h>
+#include <linux/async.h>
 #include <scsi/scsi_host.h>
 #include <scsi/scsi_eh.h>
 #include "sas_internal.h"
@@ -171,16 +172,18 @@ int sas_notify_lldd_dev_found(struct domain_device *dev)
 	struct Scsi_Host *shost = sas_ha->core.shost;
 	struct sas_internal *i = to_sas_internal(shost->transportt);
 
-	if (i->dft->lldd_dev_found) {
-		res = i->dft->lldd_dev_found(dev);
-		if (res) {
-			printk("sas: driver on pcidev %s cannot handle "
-			       "device %llx, error:%d\n",
-			       dev_name(sas_ha->dev),
-			       SAS_ADDR(dev->sas_addr), res);
-		}
-		kref_get(&dev->kref);
+	if (!i->dft->lldd_dev_found)
+		return 0;
+
+	res = i->dft->lldd_dev_found(dev);
+	if (res) {
+		printk("sas: driver on pcidev %s cannot handle "
+		       "device %llx, error:%d\n",
+		       dev_name(sas_ha->dev),
+		       SAS_ADDR(dev->sas_addr), res);
 	}
+	set_bit(SAS_DEV_FOUND, &dev->state);
+	kref_get(&dev->kref);
 	return res;
 }
 
@@ -191,7 +194,10 @@ void sas_notify_lldd_dev_gone(struct domain_device *dev)
 	struct Scsi_Host *shost = sas_ha->core.shost;
 	struct sas_internal *i = to_sas_internal(shost->transportt);
 
-	if (i->dft->lldd_dev_gone) {
+	if (!i->dft->lldd_dev_gone)
+		return;
+
+	if (test_and_clear_bit(SAS_DEV_FOUND, &dev->state)) {
 		i->dft->lldd_dev_gone(dev);
 		sas_put_device(dev);
 	}
@@ -225,6 +231,47 @@ static void sas_probe_devices(struct work_struct *work)
 	}
 }
 
+static void sas_suspend_devices(struct work_struct *work)
+{
+	struct asd_sas_phy *phy;
+	struct domain_device *dev;
+	struct sas_discovery_event *ev = to_sas_discovery_event(work);
+	struct asd_sas_port *port = ev->port;
+	struct Scsi_Host *shost = port->ha->core.shost;
+	struct sas_internal *si = to_sas_internal(shost->transportt);
+
+	clear_bit(DISCE_SUSPEND, &port->disc.pending);
+
+	sas_suspend_sata(port);
+
+	/* lldd is free to forget the domain_device across the
+	 * suspension, we force the issue here to keep the reference
+	 * counts aligned
+	 */
+	list_for_each_entry(dev, &port->dev_list, dev_list_node)
+		sas_notify_lldd_dev_gone(dev);
+
+	/* we are suspending, so we know events are disabled and
+	 * phy_list is not being mutated
+	 */
+	list_for_each_entry(phy, &port->phy_list, port_phy_el) {
+		if (si->dft->lldd_port_formed)
+			si->dft->lldd_port_deformed(phy);
+		phy->suspended = 1;
+		port->suspended = 1;
+	}
+}
+
+static void sas_resume_devices(struct work_struct *work)
+{
+	struct sas_discovery_event *ev = to_sas_discovery_event(work);
+	struct asd_sas_port *port = ev->port;
+
+	clear_bit(DISCE_RESUME, &port->disc.pending);
+
+	sas_resume_sata(port);
+}
+
 /**
  * sas_discover_end_dev -- discover an end device (SSP, etc)
  * @end: pointer to domain device of interest
@@ -302,6 +349,18 @@ static void sas_destruct_devices(struct work_struct *work)
 
 	clear_bit(DISCE_DESTRUCT, &port->disc.pending);
 
+	/* removal wants to synchronize async actions, so we need to
+	 * prevent removals during suspend
+	 */
+	if (test_bit(SAS_HA_SUSPENDED, &port->ha->state))
+		return;
+	/* FIXME: force resume to complete before we start removing
+	 * devices, critically before we take device_lock() and call
+	 * async_synchronize_full in sd_remove (in all its
+	 * lockdep_novalidate glory, sigh)
+	 */
+	async_synchronize_full();
+
 	list_for_each_entry_safe(dev, n, &port->destroy_list, disco_list_node) {
 		list_del_init(&dev->disco_list_node);
 
@@ -521,6 +580,8 @@ void sas_init_disc(struct sas_discovery *disc, struct asd_sas_port *port)
 		[DISCE_DISCOVER_DOMAIN] = sas_discover_domain,
 		[DISCE_REVALIDATE_DOMAIN] = sas_revalidate_domain,
 		[DISCE_PROBE] = sas_probe_devices,
+		[DISCE_SUSPEND] = sas_suspend_devices,
+		[DISCE_RESUME] = sas_resume_devices,
 		[DISCE_DESTRUCT] = sas_destruct_devices,
 	};
 
diff --git a/drivers/scsi/libsas/sas_dump.c b/drivers/scsi/libsas/sas_dump.c
index fc46093..cd6f99c 100644
--- a/drivers/scsi/libsas/sas_dump.c
+++ b/drivers/scsi/libsas/sas_dump.c
@@ -41,6 +41,7 @@ static const char *sas_phye_str[] = {
 	[1] = "PHYE_OOB_DONE",
 	[2] = "PHYE_OOB_ERROR",
 	[3] = "PHYE_SPINUP_HOLD",
+	[4] = "PHYE_RESUME_TIMEOUT",
 };
 
 void sas_dprint_porte(int phyid, enum port_event pe)
diff --git a/drivers/scsi/libsas/sas_event.c b/drivers/scsi/libsas/sas_event.c
index 789c4d8..aadbd53 100644
--- a/drivers/scsi/libsas/sas_event.c
+++ b/drivers/scsi/libsas/sas_event.c
@@ -134,7 +134,7 @@ static void notify_port_event(struct asd_sas_phy *phy, enum port_event event)
 			&phy->port_events[event].work, ha);
 }
 
-static void notify_phy_event(struct asd_sas_phy *phy, enum phy_event event)
+void sas_notify_phy_event(struct asd_sas_phy *phy, enum phy_event event)
 {
 	struct sas_ha_struct *ha = phy->ha;
 
@@ -159,7 +159,7 @@ int sas_init_events(struct sas_ha_struct *sas_ha)
 
 	sas_ha->notify_ha_event = notify_ha_event;
 	sas_ha->notify_port_event = notify_port_event;
-	sas_ha->notify_phy_event = notify_phy_event;
+	sas_ha->notify_phy_event = sas_notify_phy_event;
 
 	return 0;
 }
diff --git a/drivers/scsi/libsas/sas_init.c b/drivers/scsi/libsas/sas_init.c
index 1bbab3d..30dbcbb 100644
--- a/drivers/scsi/libsas/sas_init.c
+++ b/drivers/scsi/libsas/sas_init.c
@@ -159,7 +159,7 @@ Undo_phys:
 	return error;
 }
 
-int sas_unregister_ha(struct sas_ha_struct *sas_ha)
+static void sas_disable_events(struct sas_ha_struct *sas_ha)
 {
 	/* Set the state to unregistered to avoid further unchained
 	 * events to be queued, and flush any in-progress drainers
@@ -170,7 +170,11 @@ int sas_unregister_ha(struct sas_ha_struct *sas_ha)
 	spin_unlock_irq(&sas_ha->lock);
 	__sas_drain_work(sas_ha);
 	mutex_unlock(&sas_ha->drain_mutex);
+}
 
+int sas_unregister_ha(struct sas_ha_struct *sas_ha)
+{
+	sas_disable_events(sas_ha);
 	sas_unregister_ports(sas_ha);
 
 	/* flush unregistration work */
@@ -362,6 +366,102 @@ int sas_set_phy_speed(struct sas_phy *phy,
 	return ret;
 }
 
+void sas_prep_resume_ha(struct sas_ha_struct *ha)
+{
+	int i;
+
+	set_bit(SAS_HA_REGISTERED, &ha->state);
+
+	/* clear out any stale link events/data from the suspension path */
+	for (i = 0; i < ha->num_phys; i++) {
+		struct asd_sas_phy *phy = ha->sas_phy[i];
+
+		memset(phy->attached_sas_addr, 0, SAS_ADDR_SIZE);
+		phy->port_events_pending = 0;
+		phy->phy_events_pending = 0;
+		phy->frame_rcvd_size = 0;
+	}
+}
+EXPORT_SYMBOL(sas_prep_resume_ha);
+
+static int phys_suspended(struct sas_ha_struct *ha)
+{
+	int i, rc = 0;
+
+	for (i = 0; i < ha->num_phys; i++) {
+		struct asd_sas_phy *phy = ha->sas_phy[i];
+
+		if (phy->suspended)
+			rc++;
+	}
+
+	return rc;
+}
+
+void sas_resume_ha(struct sas_ha_struct *ha)
+{
+	const unsigned long tmo = msecs_to_jiffies(25000);
+	int i;
+
+	/* deform ports on phys that did not resume
+	 * at this point we may be racing the phy coming back (as posted
+	 * by the lldd).  So we post the event and once we are in the
+	 * libsas context check that the phy remains suspended before
+	 * tearing it down.
+	 */
+	i = phys_suspended(ha);
+	if (i)
+		dev_info(ha->dev, "waiting up to 25 seconds for %d phy%s to resume\n",
+			 i, i > 1 ? "s" : "");
+	wait_event_timeout(ha->eh_wait_q, phys_suspended(ha) == 0, tmo);
+	for (i = 0; i < ha->num_phys; i++) {
+		struct asd_sas_phy *phy = ha->sas_phy[i];
+
+		if (phy->suspended) {
+			dev_warn(&phy->phy->dev, "resume timeout\n");
+			sas_notify_phy_event(phy, PHYE_RESUME_TIMEOUT);
+		}
+	}
+
+	/* all phys are back up or timed out, turn on i/o so we can
+	 * flush out disks that did not return
+	 */
+	scsi_unblock_requests(ha->core.shost);
+	sas_drain_work(ha);
+
+	/* post the disk removals asynchronously (no drain) since sd_remove
+	 * wants to call async_synchronize_full, and we may be in async
+	 * context for suspend (i.e. deadlock)
+	 */
+	clear_bit(SAS_HA_SUSPENDED, &ha->state);
+	for (i = 0; i < ha->num_phys; i++) {
+		struct asd_sas_port *port = ha->sas_port[i];
+
+		sas_discover_event(port, DISCE_DESTRUCT);
+	}
+}
+EXPORT_SYMBOL(sas_resume_ha);
+
+void sas_suspend_ha(struct sas_ha_struct *ha)
+{
+	int i;
+
+	sas_disable_events(ha);
+	set_bit(SAS_HA_SUSPENDED, &ha->state);
+	scsi_block_requests(ha->core.shost);
+	for (i = 0; i < ha->num_phys; i++) {
+		struct asd_sas_port *port = ha->sas_port[i];
+
+		sas_discover_event(port, DISCE_SUSPEND);
+	}
+
+	/* flush suspend events while unregistered */
+	mutex_lock(&ha->drain_mutex);
+	__sas_drain_work(ha);
+	mutex_unlock(&ha->drain_mutex);
+}
+EXPORT_SYMBOL(sas_suspend_ha);
+
 static void sas_phy_release(struct sas_phy *phy)
 {
 	kfree(phy->hostdata);
diff --git a/drivers/scsi/libsas/sas_internal.h b/drivers/scsi/libsas/sas_internal.h
index 507e4cf..1de6796 100644
--- a/drivers/scsi/libsas/sas_internal.h
+++ b/drivers/scsi/libsas/sas_internal.h
@@ -89,6 +89,7 @@ int sas_smp_phy_control(struct domain_device *dev, int phy_id,
 			enum phy_func phy_func, struct sas_phy_linkrates *);
 int sas_smp_get_phy_events(struct sas_phy *phy);
 
+void sas_notify_phy_event(struct asd_sas_phy *phy, enum phy_event event);
 void sas_device_set_phy(struct domain_device *dev, struct sas_port *port);
 struct domain_device *sas_find_dev_by_rphy(struct sas_rphy *rphy);
 struct domain_device *sas_ex_to_ata(struct domain_device *ex_dev, int phy_id);
diff --git a/drivers/scsi/libsas/sas_phy.c b/drivers/scsi/libsas/sas_phy.c
index 521422e..cdee446 100644
--- a/drivers/scsi/libsas/sas_phy.c
+++ b/drivers/scsi/libsas/sas_phy.c
@@ -94,6 +94,25 @@ static void sas_phye_spinup_hold(struct work_struct *work)
 	i->dft->lldd_control_phy(phy, PHY_FUNC_RELEASE_SPINUP_HOLD, NULL);
 }
 
+static void sas_phye_resume_timeout(struct work_struct *work)
+{
+	struct asd_sas_event *ev = to_asd_sas_event(work);
+	struct asd_sas_phy *phy = ev->phy;
+
+	clear_bit(PHYE_RESUME_TIMEOUT, &phy->phy_events_pending);
+
+	/* phew, lldd got the phy back in the nick of time */
+	if (!phy->suspended) {
+		dev_info(&phy->phy->dev, "resume timeout cancelled\n");
+		return;
+	}
+
+	phy->error = 0;
+	phy->suspended = 0;
+	sas_deform_port(phy, 1);
+}
+
+
 /* ---------- Phy class registration ---------- */
 
 int sas_register_phys(struct sas_ha_struct *sas_ha)
@@ -105,6 +124,8 @@ int sas_register_phys(struct sas_ha_struct *sas_ha)
 		[PHYE_OOB_DONE] = sas_phye_oob_done,
 		[PHYE_OOB_ERROR] = sas_phye_oob_error,
 		[PHYE_SPINUP_HOLD] = sas_phye_spinup_hold,
+		[PHYE_RESUME_TIMEOUT] = sas_phye_resume_timeout,
+
 	};
 
 	static const work_func_t sas_port_event_fns[PORT_NUM_EVENTS] = {
diff --git a/drivers/scsi/libsas/sas_port.c b/drivers/scsi/libsas/sas_port.c
index 1cf7d75..1ac0c8b 100644
--- a/drivers/scsi/libsas/sas_port.c
+++ b/drivers/scsi/libsas/sas_port.c
@@ -39,6 +39,49 @@ static bool phy_is_wideport_member(struct asd_sas_port *port, struct asd_sas_phy
 	return true;
 }
 
+static void sas_resume_port(struct asd_sas_phy *phy)
+{
+	struct domain_device *dev;
+	struct asd_sas_port *port = phy->port;
+	struct sas_ha_struct *sas_ha = phy->ha;
+	struct sas_internal *si = to_sas_internal(sas_ha->core.shost->transportt);
+
+	if (si->dft->lldd_port_formed)
+		si->dft->lldd_port_formed(phy);
+
+	if (port->suspended)
+		port->suspended = 0;
+	else {
+		/* we only need to handle "link returned" actions once */
+		return;
+	}
+
+	/* if the port came back:
+	 * 1/ presume every device came back
+	 * 2/ force the next revalidation to check all expander phys
+	 */
+	list_for_each_entry(dev, &port->dev_list, dev_list_node) {
+		int i, rc;
+
+		rc = sas_notify_lldd_dev_found(dev);
+		if (rc) {
+			sas_unregister_dev(port, dev);
+			continue;
+		}
+
+		if (dev->dev_type == EDGE_DEV || dev->dev_type == FANOUT_DEV) {
+			dev->ex_dev.ex_change_count = -1;
+			for (i = 0; i < dev->ex_dev.num_phys; i++) {
+				struct ex_phy *phy = &dev->ex_dev.ex_phy[i];
+
+				phy->phy_change_count = -1;
+			}
+		}
+	}
+
+	sas_discover_event(port, DISCE_RESUME);
+}
+
 /**
  * sas_form_port -- add this phy to a port
  * @phy: the phy of interest
@@ -58,7 +101,14 @@ static void sas_form_port(struct asd_sas_phy *phy)
 	if (port) {
 		if (!phy_is_wideport_member(port, phy))
 			sas_deform_port(phy, 0);
-		else {
+		else if (phy->suspended) {
+			phy->suspended = 0;
+			sas_resume_port(phy);
+
+			/* phy came back, try to cancel the timeout */
+			wake_up(&sas_ha->eh_wait_q);
+			return;
+		} else {
 			SAS_DPRINTK("%s: phy%d belongs to port%d already(%d)!\n",
 				    __func__, phy->id, phy->port->id,
 				    phy->port->num_phys);
diff --git a/include/scsi/libsas.h b/include/scsi/libsas.h
index ad5229a..8ebd2747 100644
--- a/include/scsi/libsas.h
+++ b/include/scsi/libsas.h
@@ -79,7 +79,8 @@ enum phy_event {
 	PHYE_OOB_DONE         = 1,
 	PHYE_OOB_ERROR        = 2,
 	PHYE_SPINUP_HOLD      = 3, /* hot plug SATA, no COMWAKE sent */
-	PHY_NUM_EVENTS        = 4,
+	PHYE_RESUME_TIMEOUT   = 4,
+	PHY_NUM_EVENTS        = 5,
 };
 
 enum discover_event {
@@ -87,8 +88,10 @@ enum discover_event {
 	DISCE_REVALIDATE_DOMAIN = 1,
 	DISCE_PORT_GONE         = 2,
 	DISCE_PROBE		= 3,
-	DISCE_DESTRUCT		= 4,
-	DISC_NUM_EVENTS		= 5,
+	DISCE_SUSPEND		= 4,
+	DISCE_RESUME		= 5,
+	DISCE_DESTRUCT		= 6,
+	DISC_NUM_EVENTS		= 7,
 };
 
 /* ---------- Expander Devices ---------- */
@@ -128,7 +131,7 @@ struct ex_phy {
 	u8   attached_sas_addr[SAS_ADDR_SIZE];
 	u8   attached_phy_id;
 
-	u8   phy_change_count;
+	int phy_change_count;
 	enum routing_attribute routing_attr;
 	u8   virtual:1;
 
@@ -141,7 +144,7 @@ struct ex_phy {
 struct expander_device {
 	struct list_head children;
 
-	u16    ex_change_count;
+	int    ex_change_count;
 	u16    max_route_indexes;
 	u8     num_phys;
 
@@ -167,6 +170,7 @@ struct sata_device {
         enum   ata_command_set command_set;
         struct smp_resp        rps_resp; /* report_phy_sata_resp */
         u8     port_no;        /* port number, if this is a PM (Port) */
+	int    pm_result;
 
 	struct ata_port *ap;
 	struct ata_host ata_host;
@@ -180,6 +184,7 @@ struct ssp_device {
 
 enum {
 	SAS_DEV_GONE,
+	SAS_DEV_FOUND, /* device notified to lldd */
 	SAS_DEV_DESTROY,
 	SAS_DEV_EH_PENDING,
 	SAS_DEV_LU_RESET,
@@ -271,6 +276,7 @@ struct asd_sas_port {
 	enum   sas_linkrate linkrate;
 
 	struct sas_work work;
+	int suspended;
 
 /* public: */
 	int id;
@@ -319,6 +325,7 @@ struct asd_sas_phy {
 	unsigned long phy_events_pending;
 
 	int error;
+	int suspended;
 
 	struct sas_phy *phy;
 
@@ -382,6 +389,7 @@ enum sas_ha_state {
 	SAS_HA_DRAINING,
 	SAS_HA_ATA_EH_ACTIVE,
 	SAS_HA_FROZEN,
+	SAS_HA_SUSPENDED,
 };
 
 struct sas_ha_struct {
@@ -681,6 +689,9 @@ struct sas_domain_function_template {
 
 extern int sas_register_ha(struct sas_ha_struct *);
 extern int sas_unregister_ha(struct sas_ha_struct *);
+extern void sas_prep_resume_ha(struct sas_ha_struct *sas_ha);
+extern void sas_resume_ha(struct sas_ha_struct *sas_ha);
+extern void sas_suspend_ha(struct sas_ha_struct *sas_ha);
 
 int sas_set_phy_speed(struct sas_phy *phy,
 		      struct sas_phy_linkrates *rates);
diff --git a/include/scsi/sas_ata.h b/include/scsi/sas_ata.h
index 3e13c28..5bb5b1a 100644
--- a/include/scsi/sas_ata.h
+++ b/include/scsi/sas_ata.h
@@ -44,6 +44,8 @@ void sas_ata_eh(struct Scsi_Host *shost, struct list_head *work_q,
 void sas_ata_schedule_reset(struct domain_device *dev);
 void sas_ata_wait_eh(struct domain_device *dev);
 void sas_probe_sata(struct asd_sas_port *port);
+void sas_suspend_sata(struct asd_sas_port *port);
+void sas_resume_sata(struct asd_sas_port *port);
 void sas_ata_end_eh(struct ata_port *ap);
 #else
 
@@ -78,6 +80,14 @@ static inline void sas_probe_sata(struct asd_sas_port *port)
 {
 }
 
+static inline void sas_suspend_sata(struct asd_sas_port *port)
+{
+}
+
+static inline void sas_resume_sata(struct asd_sas_port *port)
+{
+}
+
 static inline int sas_get_ata_info(struct domain_device *dev, struct ex_phy *phy)
 {
 	return 0;

[isci PATCH v3 4/4] isci: enable BCN in sci_port_add_phy()

[Dan Williams]

From: Maciej Trela <maciej.trela@intel.com>

Ensure we enable receiving BCN's from the
hardware when adding phy to isci_port.
Otherwise if we get BCN before the port is
created we won't see any BCN

Signed-off-by: Maciej Trela <maciej.trela@intel.com>
Reported-by: Richard Boyd <richard.g.boyd@intel.com>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 drivers/scsi/isci/port.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/drivers/scsi/isci/port.c b/drivers/scsi/isci/port.c
index 6ef4bd9..0a3aec1 100644
--- a/drivers/scsi/isci/port.c
+++ b/drivers/scsi/isci/port.c
@@ -1199,6 +1199,8 @@ enum sci_status sci_port_add_phy(struct isci_port *iport,
 	enum sci_status status;
 	enum sci_port_states state;
 
+	sci_port_bcn_enable(iport);
+
 	state = iport->sm.current_state_id;
 	switch (state) {
 	case SCI_PORT_STOPPED: {

Re: [isci PATCH v3 3/4] libsas: suspend / resume support

[Alan Stern]

On Wed, 14 Mar 2012, Dan Williams wrote:

> libsas power management routines to suspend and recover the sas domain
> based on a model where the lldd is allowed and expected to be
> "forgetful".

What exactly does that mean?  What is there for the lldd to remember?  
Does it maintain some sort of state information about the devices 
attached to the links?

> sas_suspend_ha - disable event processing allowing the lldd to take down
>                  links without concern for causing hotplug events.
>                  Regardless of whether the lldd actually posts link down
>                  messages libsas notifies the lldd that all
>                  domain_devices are gone.
> 
> sas_prep_resume_ha - on the way back up before the lldd starts link
>                      training clean out any spurious events that were
>                      generated on the way down, and re-enable event
>                      processing
> 
> sas_resume_ha - after the lldd has started and decided that all phys
> 		have posted link-up events this routine is called to let
> 		libsas start it's own timeout of any phys that did not
> 		resume.  After the timeout an lldd can cancel the
>                 phy teardown by posting a link-up event.
> 
> Storage for ex_change_count (u16) and phy_change_count (u8) are changed
> to int so they can be set to -1 to indicate 'invalidated'.
> 
> There's a hack added to sas_destruct_devices to workaround a deadlock
> when a device is removed across the suspend cycle.  sd_remove is called
> under device_lock() and wants to async_synchronize_full(), while the
> resume path is running in an async callback and is trying to grab
> device_lock()... fun ensues.  So we guarantee that async resume actions
> have been flushed before allowing new invocations of sd_remove.

Ooh, that doesn't sound like a good way to handle it.  For one thing, 
it's possible for the resume routine to be called while the caller 
holds the device-lock for the device being resumed.  It would be best 
if the resume path defers removal of devices that have disappeared to a 
different thread.

Alan Stern

Re: [isci PATCH v3 3/4] libsas: suspend / resume support

[Dan Williams]

On Wed, 2012-03-14 at 10:21 -0400, Alan Stern wrote:
> On Wed, 14 Mar 2012, Dan Williams wrote:
> 
> > libsas power management routines to suspend and recover the sas domain
> > based on a model where the lldd is allowed and expected to be
> > "forgetful".
> 
> What exactly does that mean?  What is there for the lldd to remember?  
> Does it maintain some sort of state information about the devices 
> attached to the links?

The lldd is responsible for reporting link-up events at power on.
Rather than require lldds to remember the state of the links across
suspend (and filter link-up messages for "suspended" links), this
implementation puts all the responsibility on libsas.  The goal being to
try to minimize the need for special case code in the lldd.  The lldd
simply redoes init and posts link-up events as normal.  Then at resume
time libsas reminds the lldd about the topology of devices that was
previously reachable via a given link.

> > sas_suspend_ha - disable event processing allowing the lldd to take down
> >                  links without concern for causing hotplug events.
> >                  Regardless of whether the lldd actually posts link down
> >                  messages libsas notifies the lldd that all
> >                  domain_devices are gone.
> > 
> > sas_prep_resume_ha - on the way back up before the lldd starts link
> >                      training clean out any spurious events that were
> >                      generated on the way down, and re-enable event
> >                      processing
> > 
> > sas_resume_ha - after the lldd has started and decided that all phys
> > 		have posted link-up events this routine is called to let
> > 		libsas start it's own timeout of any phys that did not
> > 		resume.  After the timeout an lldd can cancel the
> >                 phy teardown by posting a link-up event.
> > 
> > Storage for ex_change_count (u16) and phy_change_count (u8) are changed
> > to int so they can be set to -1 to indicate 'invalidated'.
> > 
> > There's a hack added to sas_destruct_devices to workaround a deadlock
> > when a device is removed across the suspend cycle.  sd_remove is called
> > under device_lock() and wants to async_synchronize_full(), while the
> > resume path is running in an async callback and is trying to grab
> > device_lock()... fun ensues.  So we guarantee that async resume actions
> > have been flushed before allowing new invocations of sd_remove.
> 
> Ooh, that doesn't sound like a good way to handle it.  For one thing, 
> it's possible for the resume routine to be called while the caller 
> holds the device-lock for the device being resumed.  It would be best 
> if the resume path defers removal of devices that have disappeared to a 
> different thread.

In fact, that's what the hack does (in Scsi_Host workqueue context), but
that's only part of the solution.  We also need to flush resume
operations otherwise we can trigger the following deadlock (that lockdep
can't catch thanks to lockdep_set_novalidate_class(&dev->mutex); and the
lack of lockdep annotation for async_sychronize_full()):

[  494.237079] INFO: task kworker/u:3:554 blocked for more than 120 seconds.
[  494.294396] "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this message.
[  494.360809] kworker/u:3     D 0000000000000000     0   554      2 0x00000000
[  494.420739]  ffff88012e4d3af0 0000000000000046 ffff88013200c160 ffff88012e4d3fd8
[  494.484392]  ffff88012e4d3fd8 0000000000012500 ffff8801394ea0b0 ffff88013200c160
[  494.548038]  ffff88012e4d3ae0 00000000000001e3 ffffffff81a249e0 ffff8801321c5398
[  494.611685] Call Trace:
[  494.632649]  [<ffffffff8149dd25>] schedule+0x5a/0x5c
[  494.674687]  [<ffffffff8104b968>] async_synchronize_cookie_domain+0xb6/0x112
[  494.734177]  [<ffffffff810461ff>] ? __init_waitqueue_head+0x50/0x50
[  494.787134]  [<ffffffff8131a224>] ? scsi_remove_target+0x48/0x48
[  494.837900]  [<ffffffff8104b9d9>] async_synchronize_cookie+0x15/0x17
[  494.891567]  [<ffffffff8104ba49>] async_synchronize_full+0x54/0x70  <-- here we wait for async contexts to complete
[  494.943783]  [<ffffffff8104b9f5>] ? async_synchronize_full_domain+0x1a/0x1a
[  495.002547]  [<ffffffffa00114b1>] sd_remove+0x2c/0xa2 [sd_mod]
[  495.051861]  [<ffffffff812fe94f>] __device_release_driver+0x86/0xcf
[  495.104807]  [<ffffffff812fe9bd>] device_release_driver+0x25/0x32  <-- here we take device_lock()

[  853.511341] INFO: task kworker/u:4:549 blocked for more than 120 seconds.
[  853.568693] "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this message.
[  853.635119] kworker/u:4     D ffff88013097b5d0     0   549      2 0x00000000
[  853.695129]  ffff880132773c40 0000000000000046 ffff880130790000 ffff880132773fd8
[  853.758990]  ffff880132773fd8 0000000000012500 ffff88013288a0b0 ffff880130790000
[  853.822796]  0000000000000246 0000000000000040 ffff88013097b5c8 ffff880130790000
[  853.886633] Call Trace:
[  853.907631]  [<ffffffff8149dd25>] schedule+0x5a/0x5c
[  853.949670]  [<ffffffff8149cc44>] __mutex_lock_common+0x220/0x351
[  854.001225]  [<ffffffff81304bd7>] ? device_resume+0x58/0x1c4
[  854.049082]  [<ffffffff81304bd7>] ? device_resume+0x58/0x1c4
[  854.097011]  [<ffffffff8149ce48>] mutex_lock_nested+0x2f/0x36   <-- here we wait for device_lock()
[  854.145591]  [<ffffffff81304bd7>] device_resume+0x58/0x1c4
[  854.192066]  [<ffffffff81304d61>] async_resume+0x1e/0x45
[  854.237019]  [<ffffffff8104bc93>] async_run_entry_fn+0xc6/0x173  <-- ...while running in async context

--
Dan

Re: [isci PATCH v3 3/4] libsas: suspend / resume support

[Alan Stern]

On Wed, 14 Mar 2012, Dan Williams wrote:

> > Ooh, that doesn't sound like a good way to handle it.  For one thing, 
> > it's possible for the resume routine to be called while the caller 
> > holds the device-lock for the device being resumed.  It would be best 
> > if the resume path defers removal of devices that have disappeared to a 
> > different thread.
> 
> In fact, that's what the hack does (in Scsi_Host workqueue context), but
> that's only part of the solution.  We also need to flush resume
> operations otherwise we can trigger the following deadlock (that lockdep
> can't catch thanks to lockdep_set_novalidate_class(&dev->mutex); and the
> lack of lockdep annotation for async_sychronize_full()):

Unfortunately, lockdep isn't able to track dev->mutex accesses 
usefully.  It would be nice if it could.

> [  494.611685] Call Trace:
> [  494.632649]  [<ffffffff8149dd25>] schedule+0x5a/0x5c
> [  494.674687]  [<ffffffff8104b968>] async_synchronize_cookie_domain+0xb6/0x112
> [  494.734177]  [<ffffffff810461ff>] ? __init_waitqueue_head+0x50/0x50
> [  494.787134]  [<ffffffff8131a224>] ? scsi_remove_target+0x48/0x48
> [  494.837900]  [<ffffffff8104b9d9>] async_synchronize_cookie+0x15/0x17
> [  494.891567]  [<ffffffff8104ba49>] async_synchronize_full+0x54/0x70  <-- here we wait for async contexts to complete
> [  494.943783]  [<ffffffff8104b9f5>] ? async_synchronize_full_domain+0x1a/0x1a
> [  495.002547]  [<ffffffffa00114b1>] sd_remove+0x2c/0xa2 [sd_mod]
> [  495.051861]  [<ffffffff812fe94f>] __device_release_driver+0x86/0xcf
> [  495.104807]  [<ffffffff812fe9bd>] device_release_driver+0x25/0x32  <-- here we take device_lock()

> [  853.886633] Call Trace:
> [  853.907631]  [<ffffffff8149dd25>] schedule+0x5a/0x5c
> [  853.949670]  [<ffffffff8149cc44>] __mutex_lock_common+0x220/0x351
> [  854.001225]  [<ffffffff81304bd7>] ? device_resume+0x58/0x1c4
> [  854.049082]  [<ffffffff81304bd7>] ? device_resume+0x58/0x1c4
> [  854.097011]  [<ffffffff8149ce48>] mutex_lock_nested+0x2f/0x36   <-- here we wait for device_lock()
> [  854.145591]  [<ffffffff81304bd7>] device_resume+0x58/0x1c4
> [  854.192066]  [<ffffffff81304d61>] async_resume+0x1e/0x45
> [  854.237019]  [<ffffffff8104bc93>] async_run_entry_fn+0xc6/0x173  <-- ...while running in async context

I see.  It's a nasty situation; I guess the best way to describe it is 
a conflict between the requirements of the PM and SCSI subsystems:

	The PM core runs suspend and resume operations in async 
	threads, and these threads need to acquire the device lock;

	The sd driver needs to insure that async probing is finished
	when starting its remove routine, and it is called with the
	device lock held.

The best solution might be to use a workqueue for sd's async probing 
instead of the "async" threads.  Then the work routine could be 
cancelled without doing async_synchronize_full().

Alan Stern

Re: [isci PATCH v3 3/4] libsas: suspend / resume support

[Dan Williams]

On Wed, 2012-03-14 at 17:11 -0400, Alan Stern wrote:
> I see.  It's a nasty situation; I guess the best way to describe it is 
> a conflict between the requirements of the PM and SCSI subsystems:
> 
> 	The PM core runs suspend and resume operations in async 
> 	threads, and these threads need to acquire the device lock;
> 
> 	The sd driver needs to insure that async probing is finished
> 	when starting its remove routine, and it is called with the
> 	device lock held.
> 
> The best solution might be to use a workqueue for sd's async probing 
> instead of the "async" threads.  Then the work routine could be 
> cancelled without doing async_synchronize_full().

Hmm... I was wondering why we needed a kernel global sync.  If this is
just for probe work what about just doing something like the following?
Keep the sync operation local to probe-work and not entangle with the
resume-work?  I'll give this a shot when I get back to my test system.

diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index bd17cf8..ec69e35 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -2629,6 +2629,8 @@ static void sd_probe_async(void *data, async_cookie_t cookie)
 	put_device(&sdkp->dev);
 }
 
+static LIST_HEAD(sd_probe_domain);
+
 /**
  *	sd_probe - called during driver initialization and whenever a
  *	new scsi device is attached to the system. It is called once
@@ -2717,7 +2719,7 @@ static int sd_probe(struct device *dev)
 	dev_set_drvdata(dev, sdkp);
 
 	get_device(&sdkp->dev);	/* prevent release before async_schedule */
-	async_schedule(sd_probe_async, sdkp);
+	async_schedule_domain(sd_probe_async, sdkp, &sd_probe_domain);
 
 	return 0;
 
@@ -2751,7 +2753,7 @@ static int sd_remove(struct device *dev)
 	sdkp = dev_get_drvdata(dev);
 	scsi_autopm_get_device(sdkp->device);
 
-	async_synchronize_full();
+	async_synchronize_full_domain(&sd_probe_domain);
 	blk_queue_prep_rq(sdkp->device->request_queue, scsi_prep_fn);
 	blk_queue_unprep_rq(sdkp->device->request_queue, NULL);
 	device_del(&sdkp->dev);

Re: [isci PATCH v3 3/4] libsas: suspend / resume support

[Williams, Dan J]

On Wed, Mar 14, 2012 at 2:33 PM, Dan Williams <dan.j.williams@intel.com> wrote:
> On Wed, 2012-03-14 at 17:11 -0400, Alan Stern wrote:
>> I see.  It's a nasty situation; I guess the best way to describe it is
>> a conflict between the requirements of the PM and SCSI subsystems:
>>
>>       The PM core runs suspend and resume operations in async
>>       threads, and these threads need to acquire the device lock;
>>
>>       The sd driver needs to insure that async probing is finished
>>       when starting its remove routine, and it is called with the
>>       device lock held.
>>
>> The best solution might be to use a workqueue for sd's async probing
>> instead of the "async" threads.  Then the work routine could be
>> cancelled without doing async_synchronize_full().
>
> Hmm... I was wondering why we needed a kernel global sync.  If this is
> just for probe work what about just doing something like the following?
> Keep the sync operation local to probe-work and not entangle with the
> resume-work?  I'll give this a shot when I get back to my test system.
>
> diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
> index bd17cf8..ec69e35 100644
> --- a/drivers/scsi/sd.c
> +++ b/drivers/scsi/sd.c
> @@ -2629,6 +2629,8 @@ static void sd_probe_async(void *data, async_cookie_t cookie)
>        put_device(&sdkp->dev);
>  }
>
> +static LIST_HEAD(sd_probe_domain);
> +
>  /**
>  *     sd_probe - called during driver initialization and whenever a
>  *     new scsi device is attached to the system. It is called once
> @@ -2717,7 +2719,7 @@ static int sd_probe(struct device *dev)
>        dev_set_drvdata(dev, sdkp);
>
>        get_device(&sdkp->dev); /* prevent release before async_schedule */
> -       async_schedule(sd_probe_async, sdkp);
> +       async_schedule_domain(sd_probe_async, sdkp, &sd_probe_domain);
>
>        return 0;
>
> @@ -2751,7 +2753,7 @@ static int sd_remove(struct device *dev)
>        sdkp = dev_get_drvdata(dev);
>        scsi_autopm_get_device(sdkp->device);
>
> -       async_synchronize_full();
> +       async_synchronize_full_domain(&sd_probe_domain);
>        blk_queue_prep_rq(sdkp->device->request_queue, scsi_prep_fn);
>        blk_queue_unprep_rq(sdkp->device->request_queue, NULL);
>        device_del(&sdkp->dev);

This works fine for me for resolving the deadlock, but I found I also
needed the following to fix a spurious:

   scsi 6:0:1:0: Illegal state transition deleted->running

...in the resume path.

@@ -2348,10 +2349,11 @@ EXPORT_SYMBOL(scsi_device_quiesce);
  *
  *     Must be called with user context, may sleep.
  */
-void
-scsi_device_resume(struct scsi_device *sdev)
+void scsi_device_resume(struct scsi_device *sdev)
 {
-       if(scsi_device_set_state(sdev, SDEV_RUNNING))
+       /* check if the device was deleted during suspend */
+       if (sdev->sdev_state == SDEV_DEL ||
+           scsi_device_set_state(sdev, SDEV_RUNNING))
                return;
        scsi_run_queue(sdev->request_queue);
 }

Unless someone can point out something I'm missing I'll go ahead and
roll this into it's own patch and rebase/drop the hack out of the
libsas resume code.

--
Dan

Re: [isci PATCH v3 3/4] libsas: suspend / resume support

[Alan Stern]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: TEXT/PLAIN; charset=UTF-8, Size: 2209 bytes --]

On Thu, 15 Mar 2012, Williams, Dan J wrote:

> > Hmm... I was wondering why we needed a kernel global sync.  If this is
> > just for probe work what about just doing something like the following?
> > Keep the sync operation local to probe-work and not entangle with the
> > resume-work?  I'll give this a shot when I get back to my test system.
> >
> > diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
> > index bd17cf8..ec69e35 100644
> > --- a/drivers/scsi/sd.c
> > +++ b/drivers/scsi/sd.c
> > @@ -2629,6 +2629,8 @@ static void sd_probe_async(void *data, async_cookie_t cookie)
> >        put_device(&sdkp->dev);
> >  }
> >
> > +static LIST_HEAD(sd_probe_domain);

Take a look at fea6d607e154cf96ab22254ccb48addfd43d4cb5; it might be a
good idea to make this domain available to scsi_bus_prepare().  For
example, it could made into a SCSI-wide domain, defined in the SCSI
core and exported for use by sd.

> This works fine for me for resolving the deadlock, but I found I also
> needed the following to fix a spurious:
> 
>    scsi 6:0:1:0: Illegal state transition deleted->running
> 
> ...in the resume path.
> 
> @@ -2348,10 +2349,11 @@ EXPORT_SYMBOL(scsi_device_quiesce);
>   *
>   *     Must be called with user context, may sleep.
>   */
> -void
> -scsi_device_resume(struct scsi_device *sdev)
> +void scsi_device_resume(struct scsi_device *sdev)
>  {
> -       if(scsi_device_set_state(sdev, SDEV_RUNNING))
> +       /* check if the device was deleted during suspend */
> +       if (sdev->sdev_state == SDEV_DEL ||
> +           scsi_device_set_state(sdev, SDEV_RUNNING))
>                 return;
>         scsi_run_queue(sdev->request_queue);
>  }
> 
> Unless someone can point out something I'm missing I'll go ahead and
> roll this into it's own patch and rebase/drop the hack out of the
> libsas resume code.

The device might be in some other state.  Perhaps it would be better to 
do

	if (sdev->sdev_state != SDEV_QUIESCE ||
			scsi_device_set_state(sdev, SDEV_RUNNING))

I'm not sure what guarantees this function is supposed to provide, but 
the comment indicates that it's meant just to handle quiesced devices.

Alan Stern

Re: [isci PATCH v3 3/4] libsas: suspend / resume support

[Williams, Dan J]

On Thu, Mar 15, 2012 at 12:54 PM, Alan Stern <stern@rowland.harvard.edu> wrote:
> On Thu, 15 Mar 2012, Williams, Dan J wrote:
>
>> > Hmm... I was wondering why we needed a kernel global sync.  If this is
>> > just for probe work what about just doing something like the following?
>> > Keep the sync operation local to probe-work and not entangle with the
>> > resume-work?  I'll give this a shot when I get back to my test system.
>> >
>> > diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
>> > index bd17cf8..ec69e35 100644
>> > --- a/drivers/scsi/sd.c
>> > +++ b/drivers/scsi/sd.c
>> > @@ -2629,6 +2629,8 @@ static void sd_probe_async(void *data, async_cookie_t cookie)
>> >        put_device(&sdkp->dev);
>> >  }
>> >
>> > +static LIST_HEAD(sd_probe_domain);
>
> Take a look at fea6d607e154cf96ab22254ccb48addfd43d4cb5; it might be a
> good idea to make this domain available to scsi_bus_prepare().  For
> example, it could made into a SCSI-wide domain, defined in the SCSI
> core and exported for use by sd.

Nice, thanks for the pointer.  Yes, I'll up-level this.

>
>> This works fine for me for resolving the deadlock, but I found I also
>> needed the following to fix a spurious:
>>
>>    scsi 6:0:1:0: Illegal state transition deleted->running
>>
>> ...in the resume path.
>>
>> @@ -2348,10 +2349,11 @@ EXPORT_SYMBOL(scsi_device_quiesce);
>>   *
>>   *     Must be called with user context, may sleep.
>>   */
>> -void
>> -scsi_device_resume(struct scsi_device *sdev)
>> +void scsi_device_resume(struct scsi_device *sdev)
>>  {
>> -       if(scsi_device_set_state(sdev, SDEV_RUNNING))
>> +       /* check if the device was deleted during suspend */
>> +       if (sdev->sdev_state == SDEV_DEL ||
>> +           scsi_device_set_state(sdev, SDEV_RUNNING))
>>                 return;
>>         scsi_run_queue(sdev->request_queue);
>>  }
>>
>> Unless someone can point out something I'm missing I'll go ahead and
>> roll this into it's own patch and rebase/drop the hack out of the
>> libsas resume code.
>
> The device might be in some other state.  Perhaps it would be better to
> do
>
>        if (sdev->sdev_state != SDEV_QUIESCE ||
>                        scsi_device_set_state(sdev, SDEV_RUNNING))
>
> I'm not sure what guarantees this function is supposed to provide, but
> the comment indicates that it's meant just to handle quiesced devices.
>

I'm not sure either, but I can get on board with this change to say
"the world changed when you weren't looking, assume whomever changed
the state is taking care of the device".

--
Dan

RE: [isci PATCH v3 3/4] libsas: suspend / resume support

[Jack Wang]

Dan,

I found problem about SATA error handle and this also led to panic when run suspend/resume test. Thanks again for your share debug method and patch of testing suspend/resume problem.

The problem is:

When probe libata probe sata will try to reset device and send IDENTIFY DEVICE.. commands when 3 times fail will give up but never tell libsas and lldd to clean up the resource. And if some time later the device came up report BROADCAST CHANGE and libsas found device try to  tell lldd, but lldd is not forget the original one will reject this. 
Should we export sas_unregister_devs_sas_addr let ata call this when give up.


For suspend/resume here I've seen Hard LOCKUP when this kind of things happen.
Will look into the problem deeper to see the root cause.

************************************************************************

Jack Wang 王金浦 Software Engineer

RD2 (Storage) 

Department of Storage Products R&D, SCS

Universal Scientific Industrial (Shanghai) Co., Ltd

421 Lishizhen Rd, Pudong New Area, Shanghai, 

P.R. China, 201203

上海市浦东新区李时珍路421号    邮编: 201203

Tel :  +86 -21-5896 6996  ext. 81526

Fax : +86 -21-5080 4268

http://www.usi.com.tw  MAIL: jack_wang@usish.com

************************************************************************

 
> -----邮件原件-----
> 发件人: linux-scsi-owner@vger.kernel.org
> [mailto:linux-scsi-owner@vger.kernel.org] 代表 Williams, Dan J
> 发送时间: 2012年3月16日 6:16
> 收件人: Alan Stern
> 抄送: linux-scsi@vger.kernel.org; linux-ide@vger.kernel.org; Jacek Danecki
> 主题: Re: [isci PATCH v3 3/4] libsas: suspend / resume support
> 
> On Thu, Mar 15, 2012 at 12:54 PM, Alan Stern <stern@rowland.harvard.edu> wrote:
> > On Thu, 15 Mar 2012, Williams, Dan J wrote:
> >
> >> > Hmm... I was wondering why we needed a kernel global sync.  If this is
> >> > just for probe work what about just doing something like the following?
> >> > Keep the sync operation local to probe-work and not entangle with the
> >> > resume-work?  I'll give this a shot when I get back to my test system.
> >> >
> >> > diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
> >> > index bd17cf8..ec69e35 100644
> >> > --- a/drivers/scsi/sd.c
> >> > +++ b/drivers/scsi/sd.c
> >> > @@ -2629,6 +2629,8 @@ static void sd_probe_async(void *data,
> async_cookie_t cookie)
> >> >        put_device(&sdkp->dev);
> >> >  }
> >> >
> >> > +static LIST_HEAD(sd_probe_domain);
> >
> > Take a look at fea6d607e154cf96ab22254ccb48addfd43d4cb5; it might be a
> > good idea to make this domain available to scsi_bus_prepare().  For
> > example, it could made into a SCSI-wide domain, defined in the SCSI
> > core and exported for use by sd.
> 
> Nice, thanks for the pointer.  Yes, I'll up-level this.
> 
> >
> >> This works fine for me for resolving the deadlock, but I found I also
> >> needed the following to fix a spurious:
> >>
> >>    scsi 6:0:1:0: Illegal state transition deleted->running
> >>
> >> ...in the resume path.
> >>
> >> @@ -2348,10 +2349,11 @@ EXPORT_SYMBOL(scsi_device_quiesce);
> >>   *
> >>   *     Must be called with user context, may sleep.
> >>   */
> >> -void
> >> -scsi_device_resume(struct scsi_device *sdev)
> >> +void scsi_device_resume(struct scsi_device *sdev)
> >>  {
> >> -       if(scsi_device_set_state(sdev, SDEV_RUNNING))
> >> +       /* check if the device was deleted during suspend */
> >> +       if (sdev->sdev_state == SDEV_DEL ||
> >> +           scsi_device_set_state(sdev, SDEV_RUNNING))
> >>                 return;
> >>         scsi_run_queue(sdev->request_queue);
> >>  }
> >>
> >> Unless someone can point out something I'm missing I'll go ahead and
> >> roll this into it's own patch and rebase/drop the hack out of the
> >> libsas resume code.
> >
> > The device might be in some other state.  Perhaps it would be better to
> > do
> >
> >        if (sdev->sdev_state != SDEV_QUIESCE ||
> >                        scsi_device_set_state(sdev, SDEV_RUNNING))
> >
> > I'm not sure what guarantees this function is supposed to provide, but
> > the comment indicates that it's meant just to handle quiesced devices.
> >
> 
> I'm not sure either, but I can get on board with this change to say
> "the world changed when you weren't looking, assume whomever changed
> the state is taking care of the device".
> 
> --
> 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: [isci PATCH v3 3/4] libsas: suspend / resume support

[Williams, Dan J]

2012/3/16 Jack Wang <jack_wang@usish.com>:
> Dan,
>
> I found problem about SATA error handle and this also led to panic when run suspend/resume test. Thanks again for your share debug method and patch of testing suspend/resume problem.
>
> The problem is:
>
> When probe libata probe sata will try to reset device and send IDENTIFY DEVICE.. commands when 3 times fail will give up but never tell libsas and lldd to clean up the resource. And if some time later the device came up report BROADCAST CHANGE and libsas found device try to  tell lldd, but lldd is not forget the original one will reject this.
> Should we export sas_unregister_devs_sas_addr let ata call this when give up.

Have a look at "libsas: fix ata_eh clobbering ex_phys via
smp_ata_check_ready" [1], this fixed up cases where libsas failed to
handle ata device unplug in our environment.

--
Dan

[1]: http://marc.info/?l=linux-scsi&m=133239709603452&w=2

Re: [isci PATCH v3 0/4] isci: suspend/resume support+

[Dan Williams]

On Wed, Mar 14, 2012 at 12:13 AM, Dan Williams <dan.j.williams@intel.com> wrote:
> Changes since v2: http://marc.info/?l=linux-scsi&m=133144999728737&w=2
>
> This topic has been rebased on top of the libsas topic / scsi-misc:
> http://marc.info/?l=linux-scsi&m=133170574921314&w=2
>
> 1/ Fixed handling of the 'device unplugged during suspend' case.
>   sd_remove() sets up an ABBA deadlock with the driver-core device_lock()
>   and async_synchronize_full(), so this version arranges for resume
>   actions to be flushed before attempting removals.
>
> 2/ Add a new fix for a case where the isci driver inadvertently filters
>   BCNs
>
> 3/ Fix up compile breakage in the CONFIG_PM=n case
>
> 4/ Minor fixups for the rebase (sas_init_dev reindent collision)
>
> [isci PATCH v3 1/4] libata: export ata_port suspend/resume infrastructure for sas
> [isci PATCH v3 2/4] libsas: drop sata port multiplier infrastructure
> [isci PATCH v3 3/4] libsas: suspend / resume support
> [isci PATCH v3 4/4] isci: enable BCN in sci_port_add_phy()
>
> Full diffstat versus scsi-misc (cd8df93..isci-for-3.4-v3):

v4 of this topic is available here:

 git://git.kernel.org/pub/scm/linux/kernel/git/djbw/isci.git isci-for-3.4-v4

the only difference is that it has been rebased to move the libsas
patches (suspend/resume support) to libsas-eh-reworks-v12.  Patch
contents are identical

The following changes since commit f0238d3adfebb8a1f556fe0df5dfe2b3a93fe660:

  libsas: suspend / resume support (2012-03-21 21:59:45 -0700)

are available in the git repository at:
  git://git.kernel.org/pub/scm/linux/kernel/git/djbw/isci.git isci-for-3.4-v4

Andrzej Jakowski (1):
      isci: Changes in COMSAS timings enabling ISCI to detect buggy disc drives.

Artur Wojcik (1):
      isci: implement suspend/resume support

Dan Williams (10):
      isci: improve 'invalid state' warnings
      isci: kill ->is_direct_attached
      isci: kill sci_phy_protocol and sci_request_protocol
      isci: kill ->status, and ->state_lock in isci_host
      isci: kill isci_port.domain_dev_list
      isci: refactor initialization for S3/S4
      isci: fix controller stop
      isci: fix 'link-up' events occur after 'start-complete'
      isci: fix interrupt disable
      isci: kill isci_host.shost

Jesper Juhl (1):
      isci: Just #include "host.h" once in host.c

Maciej Trela (1):
      isci: enable BCN in sci_port_add_phy()

Tom Jackson (1):
      isci: Don't filter BROADCAST CHANGE primitives

 drivers/scsi/isci/host.c                      |  565 +++++++++----------------
 drivers/scsi/isci/host.h                      |  118 ++----
 drivers/scsi/isci/init.c                      |  271 +++++++++++--
 drivers/scsi/isci/phy.c                       |   74 +++-
 drivers/scsi/isci/phy.h                       |    9 +-
 drivers/scsi/isci/port.c                      |   23 +-
 drivers/scsi/isci/port.h                      |    6 -
 drivers/scsi/isci/port_config.c               |   18 +-
 drivers/scsi/isci/probe_roms.c                |   12 -
 drivers/scsi/isci/probe_roms.h                |    2 -
 drivers/scsi/isci/registers.h                 |    8 +
 drivers/scsi/isci/remote_device.c             |   29 +-
 drivers/scsi/isci/remote_device.h             |    1 -
 drivers/scsi/isci/remote_node_context.c       |   60 ++--
 drivers/scsi/isci/request.c                   |   19 +-
 drivers/scsi/isci/request.h                   |    9 +-
 drivers/scsi/isci/unsolicited_frame_control.c |   30 +-
 drivers/scsi/isci/unsolicited_frame_control.h |    6 +-
 include/scsi/sas.h                            |    1 +
 19 files changed, 626 insertions(+), 635 deletions(-)
--
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: [isci PATCH v3 3/4] libsas: suspend / resume support

[Jack Wang]

> 
> 2012/3/16 Jack Wang <jack_wang@usish.com>:
> > Dan,
> >
> > I found problem about SATA error handle and this also led to panic when run
> suspend/resume test. Thanks again for your share debug method and patch of
> testing suspend/resume problem.
> >
> > The problem is:
> >
> > When probe libata probe sata will try to reset device and send IDENTIFY DEVICE..
> commands when 3 times fail will give up but never tell libsas and lldd to clean
> up the resource. And if some time later the device came up report BROADCAST
> CHANGE and libsas found device try to  tell lldd, but lldd is not forget the
> original one will reject this.
> > Should we export sas_unregister_devs_sas_addr let ata call this when give
> up.
> 
> Have a look at "libsas: fix ata_eh clobbering ex_phys via
> smp_ata_check_ready" [1], this fixed up cases where libsas failed to
> handle ata device unplug in our environment.
> 
> --
> Dan
> 
> [1]: http://marc.info/?l=linux-scsi&m=133239709603452&w=2
[Jack Wang] 
Thanks Dan, we are testing your new patchset in our test environment, will report back result.
> --
> 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: [isci PATCH v3 1/4] libata: export ata_port suspend/resume infrastructure for sas

[Jeff Garzik]

On 03/14/2012 03:13 AM, Dan Williams wrote:
> Reuse ata_port_{suspend|resume}_common for sas.  This path is chosen
> over adding coordination between ata-tranport and sas-transport because
> libsas wants to revalidate the domain at resume-time at the host level.
> It can not validate links have resumed properly until libata has had a
> chance to perform its revalidation, and any sane placing of an ata_port
> in the sas-transport model would delay it's resumption until after the
> host.
>
> Export the common portion of port suspend/resume (bypass pm_runtime),
> and allow sas to perform these operations asynchronously (similar to the
> libsas async-ata probe implmentation).  Async operation is determined by
> having an external, rather than stack based, location for storing the
> result of the operation.
>
> Reviewed-by: Jacek Danecki<jacek.danecki@intel.com>
> Signed-off-by: Dan Williams<dan.j.williams@intel.com>
> ---
>   drivers/ata/libata-core.c |   58 ++++++++++++++++++++++++++++++++++++---------
>   include/linux/libata.h    |   11 +++++++++
>   2 files changed, 57 insertions(+), 12 deletions(-)

Now that libata's runtime PM problems seem to be fixed for the moment, 
we can revisit port PM here.  Just checking...  Is this patch still needed?

Re: [isci PATCH v3 1/4] libata: export ata_port suspend/resume infrastructure for sas

[Dan Williams]

On Fri, Apr 20, 2012 at 11:26 PM, Jeff Garzik <jeff@garzik.org> wrote:
> On 03/14/2012 03:13 AM, Dan Williams wrote:
>>
>> Reuse ata_port_{suspend|resume}_common for sas.  This path is chosen
>> over adding coordination between ata-tranport and sas-transport because
>> libsas wants to revalidate the domain at resume-time at the host level.
>> It can not validate links have resumed properly until libata has had a
>> chance to perform its revalidation, and any sane placing of an ata_port
>> in the sas-transport model would delay it's resumption until after the
>> host.
>>
>> Export the common portion of port suspend/resume (bypass pm_runtime),
>> and allow sas to perform these operations asynchronously (similar to the
>> libsas async-ata probe implmentation).  Async operation is determined by
>> having an external, rather than stack based, location for storing the
>> result of the operation.
>>
>> Reviewed-by: Jacek Danecki<jacek.danecki@intel.com>
>> Signed-off-by: Dan Williams<dan.j.williams@intel.com>
>> ---
>>  drivers/ata/libata-core.c |   58
>> ++++++++++++++++++++++++++++++++++++---------
>>  include/linux/libata.h    |   11 +++++++++
>>  2 files changed, 57 insertions(+), 12 deletions(-)
>
>
> Now that libata's runtime PM problems seem to be fixed for the moment, we
> can revisit port PM here.  Just checking...  Is this patch still needed?

Yeah, this one is still needed.  libsas wants to validate links at the
host level.  I tried letting these routines be called "naturally" by
registering libsas-ata_ports with the ata-transport class, but it
hangs / got messy in cases where the link needed recovery
(SATA_PENDING), but the ata_port was still suspended.  Also, I think
the ata_port pm_runtime bits need a review for suitability for the
libsas case.

So, with this approach libsas does not register libsas-ata_ports with
the ata transport class and instead calls the core routines directly
(similar to how libata did before the dev_pm_ops rework).

--
Dan