[PATCH 0/3] libata bug fixes and updates as a precursor for converting libsas to the new eh

[PATCH 0/3] libata bug fixes and updates as a precursor for converting libsas to the new eh

[James Bottomley]

While I was playing around with converting libsas to the new error
handler, I ran into the two locking bugs and the fact that sas uses non
standard submission paths.  These patches fix all of that.  In theory,
the error handler locking bug is serious, since the eh_cmd_q list is
being processed on the wrong lock, but in practise, since libata has one
host per port, I don't think it will ever have any visible consequence
(until, of course, libsas comes along with multiple ports per host plus
non-libata ports).

James

---

James Bottomley (3):
  libata: plumb sas port scan into standard libata paths
  libata: fix locking for sas paths
  libata: fix eh locking

 drivers/ata/libata-core.c |   46 +++++++++++++++++++++-----------------------
 drivers/ata/libata-eh.c   |    9 ++++++-
 drivers/ata/libata-scsi.c |    4 +-
 drivers/ata/libata.h      |    1 +
 4 files changed, 32 insertions(+), 28 deletions(-)

[PATCH 1/3] libata: plumb sas port scan into standard libata paths

[James Bottomley]

The function ata_sas_port_init() has always really done its own thing.
However, as a precursor to moving to the libata new eh, it has to be
properly using the standard libata scan paths.  This means separating
the current libata scan paths into pieces which can be shared with
libsas and pieces which cant (really just the async call and the host
scan).

Signed-off-by: James Bottomley <James.Bottomley@suse.de>
---
 drivers/ata/libata-core.c |   46 +++++++++++++++++++++-----------------------
 drivers/ata/libata-scsi.c |    2 +-
 drivers/ata/libata.h      |    1 +
 3 files changed, 24 insertions(+), 25 deletions(-)

diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index a31fe96..99df83c 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -5886,21 +5886,9 @@ void ata_host_init(struct ata_host *host, struct device *dev,
 	host->ops = ops;
 }
 
-
-static void async_port_probe(void *data, async_cookie_t cookie)
+int ata_port_probe(struct ata_port *ap)
 {
-	int rc;
-	struct ata_port *ap = data;
-
-	/*
-	 * If we're not allowed to scan this host in parallel,
-	 * we need to wait until all previous scans have completed
-	 * before going further.
-	 * Jeff Garzik says this is only within a controller, so we
-	 * don't need to wait for port 0, only for later ports.
-	 */
-	if (!(ap->host->flags & ATA_HOST_PARALLEL_SCAN) && ap->port_no != 0)
-		async_synchronize_cookie(cookie);
+	int rc = 0;
 
 	/* probe */
 	if (ap->ops->error_handler) {
@@ -5926,23 +5914,33 @@ static void async_port_probe(void *data, async_cookie_t cookie)
 		DPRINTK("ata%u: bus probe begin\n", ap->print_id);
 		rc = ata_bus_probe(ap);
 		DPRINTK("ata%u: bus probe end\n", ap->print_id);
-
-		if (rc) {
-			/* FIXME: do something useful here?
-			 * Current libata behavior will
-			 * tear down everything when
-			 * the module is removed
-			 * or the h/w is unplugged.
-			 */
-		}
 	}
+	return rc;
+}
+
+
+static void async_port_probe(void *data, async_cookie_t cookie)
+{
+	struct ata_port *ap = data;
+	
+	/*
+	 * If we're not allowed to scan this host in parallel,
+	 * we need to wait until all previous scans have completed
+	 * before going further.
+	 * Jeff Garzik says this is only within a controller, so we
+	 * don't need to wait for port 0, only for later ports.
+	 */
+	if (!(ap->host->flags & ATA_HOST_PARALLEL_SCAN) && ap->port_no != 0)
+		async_synchronize_cookie(cookie);
+
+	(void)ata_port_probe(ap);
 
 	/* in order to keep device order, we need to synchronize at this point */
 	async_synchronize_cookie(cookie);
 
 	ata_scsi_scan_host(ap, 1);
-
 }
+
 /**
  *	ata_host_register - register initialized ATA host
  *	@host: ATA host to register
diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index 5defc74..8970667 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -3809,7 +3809,7 @@ int ata_sas_port_init(struct ata_port *ap)
 
 	if (!rc) {
 		ap->print_id = ata_print_id++;
-		rc = ata_bus_probe(ap);
+		rc = ata_port_probe(ap);
 	}
 
 	return rc;
diff --git a/drivers/ata/libata.h b/drivers/ata/libata.h
index a9be110..773de97 100644
--- a/drivers/ata/libata.h
+++ b/drivers/ata/libata.h
@@ -103,6 +103,7 @@ extern int ata_task_ioctl(struct scsi_device *scsidev, void __user *arg);
 extern int ata_cmd_ioctl(struct scsi_device *scsidev, void __user *arg);
 extern struct ata_port *ata_port_alloc(struct ata_host *host);
 extern const char *sata_spd_string(unsigned int spd);
+extern int ata_port_probe(struct ata_port *ap);
 
 /* libata-acpi.c */
 #ifdef CONFIG_ATA_ACPI
-- 
1.7.2.3

[PATCH 2/3] libata: fix locking for sas paths

[James Bottomley]

For historical reasons, libsas uses the scsi host lock as the ata port
lock, and libata always uses the ata host.  For the old eh, this was
largely irrelevant since the two locks were never mixed inside the
code.  However, the new eh has a case where it nests acquisition of
the host lock inside the port lock (this does look rather deadlock
prone).  Obviously this would be an instant deadlock if the port lock
were the host lock, so switch the libsas paths to use the ata host
lock as well.

Signed-off-by: James Bottomley <James.Bottomley@suse.de>
---
 drivers/ata/libata-scsi.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index 8970667..b935f3a 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -3747,7 +3747,7 @@ struct ata_port *ata_sas_port_alloc(struct ata_host *host,
 		return NULL;
 
 	ap->port_no = 0;
-	ap->lock = shost->host_lock;
+	ap->lock = &host->lock;
 	ap->pio_mask = port_info->pio_mask;
 	ap->mwdma_mask = port_info->mwdma_mask;
 	ap->udma_mask = port_info->udma_mask;
-- 
1.7.2.3

[PATCH 3/3] libata: fix eh locking

[James Bottomley]

The SCSI host eh_cmd_q should be protected by the host lock (not the
port lock).  This probably doesn't matter that much at the moment,
since we try to serialise the add and eh pieces, but it might matter
in future for more convenient error handling.  Plus this switches
libata to the standard eh pattern where you lock, remove from the cmd
queue to a local list and unlock and then operate on the local list.

Signed-off-by: James Bottomley <James.Bottomley@suse.de>
---
 drivers/ata/libata-eh.c |    9 +++++++--
 1 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c
index 17a6378..fc3f339 100644
--- a/drivers/ata/libata-eh.c
+++ b/drivers/ata/libata-eh.c
@@ -589,9 +589,14 @@ void ata_scsi_error(struct Scsi_Host *host)
 	struct ata_port *ap = ata_shost_to_port(host);
 	int i;
 	unsigned long flags;
+	LIST_HEAD(eh_work_q);
 
 	DPRINTK("ENTER\n");
 
+	spin_lock_irqsave(host->host_lock, flags);
+	list_splice_init(&host->eh_cmd_q, &eh_work_q);
+	spin_unlock_irqrestore(host->host_lock, flags);
+
 	/* make sure sff pio task is not running */
 	ata_sff_flush_pio_task(ap);
 
@@ -627,7 +632,7 @@ void ata_scsi_error(struct Scsi_Host *host)
 		if (ap->ops->lost_interrupt)
 			ap->ops->lost_interrupt(ap);
 
-		list_for_each_entry_safe(scmd, tmp, &host->eh_cmd_q, eh_entry) {
+		list_for_each_entry_safe(scmd, tmp, &eh_work_q, eh_entry) {
 			struct ata_queued_cmd *qc;
 
 			for (i = 0; i < ATA_MAX_QUEUE; i++) {
@@ -762,7 +767,7 @@ void ata_scsi_error(struct Scsi_Host *host)
 	}
 
 	/* finish or retry handled scmd's and clean up */
-	WARN_ON(host->host_failed || !list_empty(&host->eh_cmd_q));
+	WARN_ON(host->host_failed || !list_empty(&eh_work_q));
 
 	scsi_eh_flush_done_q(&ap->eh_done_q);
 
-- 
1.7.2.3