[PATCH 3/4] libata: use ata_scsi_timed_out()

[PATCH 3/4] libata: use ata_scsi_timed_out()

[Tejun Heo]

Make all libata low level drivers use ata_scsi_timed_out().

Signed-off-by: Tejun Heo <htejun@gmail.com>

---

 drivers/scsi/ahci.c         |    1 +
 drivers/scsi/ata_piix.c     |    1 +
 drivers/scsi/pdc_adma.c     |    1 +
 drivers/scsi/sata_mv.c      |    1 +
 drivers/scsi/sata_nv.c      |    1 +
 drivers/scsi/sata_promise.c |    1 +
 drivers/scsi/sata_qstor.c   |    1 +
 drivers/scsi/sata_sil.c     |    1 +
 drivers/scsi/sata_sil24.c   |    1 +
 drivers/scsi/sata_sis.c     |    1 +
 drivers/scsi/sata_svw.c     |    1 +
 drivers/scsi/sata_sx4.c     |    1 +
 drivers/scsi/sata_uli.c     |    1 +
 drivers/scsi/sata_via.c     |    1 +
 drivers/scsi/sata_vsc.c     |    1 +
 15 files changed, 15 insertions(+), 0 deletions(-)

f8ba8f199f983312d2d2191e8af1e18954f410a3
diff --git a/drivers/scsi/ahci.c b/drivers/scsi/ahci.c
index c840d5e..c67189a 100644
--- a/drivers/scsi/ahci.c
+++ b/drivers/scsi/ahci.c
@@ -206,6 +206,7 @@ static struct scsi_host_template ahci_sh
 	.name			= DRV_NAME,
 	.ioctl			= ata_scsi_ioctl,
 	.queuecommand		= ata_scsi_queuecmd,
+	.eh_timed_out		= ata_scsi_timed_out,
 	.eh_strategy_handler	= ata_scsi_error,
 	.can_queue		= ATA_DEF_QUEUE,
 	.this_id		= ATA_SHT_THIS_ID,
diff --git a/drivers/scsi/ata_piix.c b/drivers/scsi/ata_piix.c
index 49cc420..4933ba2 100644
--- a/drivers/scsi/ata_piix.c
+++ b/drivers/scsi/ata_piix.c
@@ -180,6 +180,7 @@ static struct scsi_host_template piix_sh
 	.name			= DRV_NAME,
 	.ioctl			= ata_scsi_ioctl,
 	.queuecommand		= ata_scsi_queuecmd,
+	.eh_timed_out		= ata_scsi_timed_out,
 	.eh_strategy_handler	= ata_scsi_error,
 	.can_queue		= ATA_DEF_QUEUE,
 	.this_id		= ATA_SHT_THIS_ID,
diff --git a/drivers/scsi/pdc_adma.c b/drivers/scsi/pdc_adma.c
index 3a6bf58..d0ad3eb 100644
--- a/drivers/scsi/pdc_adma.c
+++ b/drivers/scsi/pdc_adma.c
@@ -143,6 +143,7 @@ static struct scsi_host_template adma_at
 	.name			= DRV_NAME,
 	.ioctl			= ata_scsi_ioctl,
 	.queuecommand		= ata_scsi_queuecmd,
+	.eh_timed_out		= ata_scsi_timed_out,
 	.eh_strategy_handler	= ata_scsi_error,
 	.can_queue		= ATA_DEF_QUEUE,
 	.this_id		= ATA_SHT_THIS_ID,
diff --git a/drivers/scsi/sata_mv.c b/drivers/scsi/sata_mv.c
index 3e91632..1db05f2 100644
--- a/drivers/scsi/sata_mv.c
+++ b/drivers/scsi/sata_mv.c
@@ -378,6 +378,7 @@ static struct scsi_host_template mv_sht 
 	.name			= DRV_NAME,
 	.ioctl			= ata_scsi_ioctl,
 	.queuecommand		= ata_scsi_queuecmd,
+	.eh_timed_out		= ata_scsi_timed_out,
 	.eh_strategy_handler	= ata_scsi_error,
 	.can_queue		= MV_USE_Q_DEPTH,
 	.this_id		= ATA_SHT_THIS_ID,
diff --git a/drivers/scsi/sata_nv.c b/drivers/scsi/sata_nv.c
index bbbb55e..cdfeb9a 100644
--- a/drivers/scsi/sata_nv.c
+++ b/drivers/scsi/sata_nv.c
@@ -229,6 +229,7 @@ static struct scsi_host_template nv_sht 
 	.name			= DRV_NAME,
 	.ioctl			= ata_scsi_ioctl,
 	.queuecommand		= ata_scsi_queuecmd,
+	.eh_timed_out		= ata_scsi_timed_out,
 	.eh_strategy_handler	= ata_scsi_error,
 	.can_queue		= ATA_DEF_QUEUE,
 	.this_id		= ATA_SHT_THIS_ID,
diff --git a/drivers/scsi/sata_promise.c b/drivers/scsi/sata_promise.c
index 0950a8e..5d4ed54 100644
--- a/drivers/scsi/sata_promise.c
+++ b/drivers/scsi/sata_promise.c
@@ -111,6 +111,7 @@ static struct scsi_host_template pdc_ata
 	.name			= DRV_NAME,
 	.ioctl			= ata_scsi_ioctl,
 	.queuecommand		= ata_scsi_queuecmd,
+	.eh_timed_out		= ata_scsi_timed_out,
 	.eh_strategy_handler	= ata_scsi_error,
 	.can_queue		= ATA_DEF_QUEUE,
 	.this_id		= ATA_SHT_THIS_ID,
diff --git a/drivers/scsi/sata_qstor.c b/drivers/scsi/sata_qstor.c
index 2afbeb7..82c3df7 100644
--- a/drivers/scsi/sata_qstor.c
+++ b/drivers/scsi/sata_qstor.c
@@ -132,6 +132,7 @@ static struct scsi_host_template qs_ata_
 	.name			= DRV_NAME,
 	.ioctl			= ata_scsi_ioctl,
 	.queuecommand		= ata_scsi_queuecmd,
+	.eh_timed_out		= ata_scsi_timed_out,
 	.eh_strategy_handler	= ata_scsi_error,
 	.can_queue		= ATA_DEF_QUEUE,
 	.this_id		= ATA_SHT_THIS_ID,
diff --git a/drivers/scsi/sata_sil.c b/drivers/scsi/sata_sil.c
index 17f74d3..f40f25e 100644
--- a/drivers/scsi/sata_sil.c
+++ b/drivers/scsi/sata_sil.c
@@ -135,6 +135,7 @@ static struct scsi_host_template sil_sht
 	.name			= DRV_NAME,
 	.ioctl			= ata_scsi_ioctl,
 	.queuecommand		= ata_scsi_queuecmd,
+	.eh_timed_out		= ata_scsi_timed_out,
 	.eh_strategy_handler	= ata_scsi_error,
 	.can_queue		= ATA_DEF_QUEUE,
 	.this_id		= ATA_SHT_THIS_ID,
diff --git a/drivers/scsi/sata_sil24.c b/drivers/scsi/sata_sil24.c
index 7222fc7..f4742ad 100644
--- a/drivers/scsi/sata_sil24.c
+++ b/drivers/scsi/sata_sil24.c
@@ -280,6 +280,7 @@ static struct scsi_host_template sil24_s
 	.name			= DRV_NAME,
 	.ioctl			= ata_scsi_ioctl,
 	.queuecommand		= ata_scsi_queuecmd,
+	.eh_timed_out		= ata_scsi_timed_out,
 	.eh_strategy_handler	= ata_scsi_error,
 	.can_queue		= ATA_DEF_QUEUE,
 	.this_id		= ATA_SHT_THIS_ID,
diff --git a/drivers/scsi/sata_sis.c b/drivers/scsi/sata_sis.c
index 2df8c56..2f18157 100644
--- a/drivers/scsi/sata_sis.c
+++ b/drivers/scsi/sata_sis.c
@@ -87,6 +87,7 @@ static struct scsi_host_template sis_sht
 	.name			= DRV_NAME,
 	.ioctl			= ata_scsi_ioctl,
 	.queuecommand		= ata_scsi_queuecmd,
+	.eh_timed_out		= ata_scsi_timed_out,
 	.eh_strategy_handler	= ata_scsi_error,
 	.can_queue		= ATA_DEF_QUEUE,
 	.this_id		= ATA_SHT_THIS_ID,
diff --git a/drivers/scsi/sata_svw.c b/drivers/scsi/sata_svw.c
index d847256..f369c30 100644
--- a/drivers/scsi/sata_svw.c
+++ b/drivers/scsi/sata_svw.c
@@ -288,6 +288,7 @@ static struct scsi_host_template k2_sata
 	.name			= DRV_NAME,
 	.ioctl			= ata_scsi_ioctl,
 	.queuecommand		= ata_scsi_queuecmd,
+	.eh_timed_out		= ata_scsi_timed_out,
 	.eh_strategy_handler	= ata_scsi_error,
 	.can_queue		= ATA_DEF_QUEUE,
 	.this_id		= ATA_SHT_THIS_ID,
diff --git a/drivers/scsi/sata_sx4.c b/drivers/scsi/sata_sx4.c
index 9f992fb..3319f03 100644
--- a/drivers/scsi/sata_sx4.c
+++ b/drivers/scsi/sata_sx4.c
@@ -182,6 +182,7 @@ static struct scsi_host_template pdc_sat
 	.name			= DRV_NAME,
 	.ioctl			= ata_scsi_ioctl,
 	.queuecommand		= ata_scsi_queuecmd,
+	.eh_timed_out		= ata_scsi_timed_out,
 	.eh_strategy_handler	= ata_scsi_error,
 	.can_queue		= ATA_DEF_QUEUE,
 	.this_id		= ATA_SHT_THIS_ID,
diff --git a/drivers/scsi/sata_uli.c b/drivers/scsi/sata_uli.c
index 9635ca7..c500f24 100644
--- a/drivers/scsi/sata_uli.c
+++ b/drivers/scsi/sata_uli.c
@@ -75,6 +75,7 @@ static struct scsi_host_template uli_sht
 	.name			= DRV_NAME,
 	.ioctl			= ata_scsi_ioctl,
 	.queuecommand		= ata_scsi_queuecmd,
+	.eh_timed_out		= ata_scsi_timed_out,
 	.eh_strategy_handler	= ata_scsi_error,
 	.can_queue		= ATA_DEF_QUEUE,
 	.this_id		= ATA_SHT_THIS_ID,
diff --git a/drivers/scsi/sata_via.c b/drivers/scsi/sata_via.c
index 6d5b0a7..2e20887 100644
--- a/drivers/scsi/sata_via.c
+++ b/drivers/scsi/sata_via.c
@@ -94,6 +94,7 @@ static struct scsi_host_template svia_sh
 	.name			= DRV_NAME,
 	.ioctl			= ata_scsi_ioctl,
 	.queuecommand		= ata_scsi_queuecmd,
+	.eh_timed_out		= ata_scsi_timed_out,
 	.eh_strategy_handler	= ata_scsi_error,
 	.can_queue		= ATA_DEF_QUEUE,
 	.this_id		= ATA_SHT_THIS_ID,
diff --git a/drivers/scsi/sata_vsc.c b/drivers/scsi/sata_vsc.c
index 2e2c3b7..cf1f8a6 100644
--- a/drivers/scsi/sata_vsc.c
+++ b/drivers/scsi/sata_vsc.c
@@ -223,6 +223,7 @@ static struct scsi_host_template vsc_sat
 	.name			= DRV_NAME,
 	.ioctl			= ata_scsi_ioctl,
 	.queuecommand		= ata_scsi_queuecmd,
+	.eh_timed_out		= ata_scsi_timed_out,
 	.eh_strategy_handler	= ata_scsi_error,
 	.can_queue		= ATA_DEF_QUEUE,
 	.this_id		= ATA_SHT_THIS_ID,
-- 
1.1.5

[PATCH 2/4] libata: implement ata_scsi_timed_out()

[Tejun Heo]

Implement ata_scsi_timed_out(), to be used as
scsi_host_template->eh_timed_out callback for all libata drivers.
Without this function, the following race exists.

If a qc completes after SCSI timer expires but before libata EH kicks
in, the qc gets completed but the scsicmd still gets passed to libata
EH resulting in ->eng_timeout invocation with NULL qc, which none is
handling properly.

This patch makes sure that scmd and qc share the same lifetime.
Original idea from Jeff Garzik <jgarzik@pobox.com>.

Signed-off-by: Tejun Heo <htejun@gmail.com>

---

 drivers/scsi/libata-core.c |    1 +
 drivers/scsi/libata-scsi.c |   41 +++++++++++++++++++++++++++++++++++++++++
 include/linux/libata.h     |    1 +
 3 files changed, 43 insertions(+), 0 deletions(-)

0421953f2a2dee014a3e288c523129a3c602e3ae
diff --git a/drivers/scsi/libata-core.c b/drivers/scsi/libata-core.c
index 977a53d..d53e0bc 100644
--- a/drivers/scsi/libata-core.c
+++ b/drivers/scsi/libata-core.c
@@ -4913,6 +4913,7 @@ EXPORT_SYMBOL_GPL(ata_ratelimit);
 EXPORT_SYMBOL_GPL(ata_busy_sleep);
 EXPORT_SYMBOL_GPL(ata_scsi_ioctl);
 EXPORT_SYMBOL_GPL(ata_scsi_queuecmd);
+EXPORT_SYMBOL_GPL(ata_scsi_timed_out);
 EXPORT_SYMBOL_GPL(ata_scsi_error);
 EXPORT_SYMBOL_GPL(ata_scsi_slave_config);
 EXPORT_SYMBOL_GPL(ata_scsi_release);
diff --git a/drivers/scsi/libata-scsi.c b/drivers/scsi/libata-scsi.c
index 1df468e..d67cc2f 100644
--- a/drivers/scsi/libata-scsi.c
+++ b/drivers/scsi/libata-scsi.c
@@ -717,6 +717,47 @@ int ata_scsi_slave_config(struct scsi_de
 }
 
 /**
+ *	ata_scsi_timed_out - SCSI layer time out callback
+ *	@cmd: timed out SCSI command
+ *
+ *	Handles SCSI layer timeout.  We race with normal completion of
+ *	the qc for @cmd.  If the qc is already gone, we lose and let
+ *	the scsi command finish (EH_HANDLED).  Otherwise, the qc has
+ *	timed out and EH should be invoked.  Prevent ata_qc_complete()
+ *	from finishing it by setting EH_SCHEDULED and return
+ *	EH_NOT_HANDLED.
+ *
+ *	LOCKING:
+ *	Called from timer context
+ *
+ *	RETURNS:
+ *	EH_HANDLED or EH_NOT_HANDLED
+ */
+enum scsi_eh_timer_return ata_scsi_timed_out(struct scsi_cmnd *cmd)
+{
+	struct Scsi_Host *host = cmd->device->host;
+	struct ata_port *ap = (struct ata_port *) &host->hostdata[0];
+	unsigned long flags;
+	struct ata_queued_cmd *qc;
+	enum scsi_eh_timer_return ret = EH_HANDLED;
+
+	DPRINTK("ENTER\n");
+
+	spin_lock_irqsave(&ap->host_set->lock, flags);
+	qc = ata_qc_from_tag(ap, ap->active_tag);
+	if (qc) {
+		assert(qc->scsicmd == cmd);
+		qc->flags |= ATA_QCFLAG_EH_SCHEDULED;
+		qc->err_mask |= AC_ERR_TIMEOUT;
+		ret = EH_NOT_HANDLED;
+	}
+	spin_unlock_irqrestore(&ap->host_set->lock, flags);
+
+	DPRINTK("EXIT, ret=%d\n", ret);
+	return ret;
+}
+
+/**
  *	ata_scsi_error - SCSI layer error handler callback
  *	@host: SCSI host on which error occurred
  *
diff --git a/include/linux/libata.h b/include/linux/libata.h
index 5c70a57..c1e1986 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -509,6 +509,7 @@ extern void ata_host_set_remove(struct a
 extern int ata_scsi_detect(struct scsi_host_template *sht);
 extern int ata_scsi_ioctl(struct scsi_device *dev, int cmd, void __user *arg);
 extern int ata_scsi_queuecmd(struct scsi_cmnd *cmd, void (*done)(struct scsi_cmnd *));
+extern enum scsi_eh_timer_return ata_scsi_timed_out(struct scsi_cmnd *cmd);
 extern int ata_scsi_error(struct Scsi_Host *host);
 extern void ata_eh_qc_complete(struct ata_queued_cmd *qc);
 extern void ata_eh_qc_retry(struct ata_queued_cmd *qc);
-- 
1.1.5

[PATCHSET] libata: various fixes related to EH, take #4

[Tejun Heo]

Hello, Jeff.

This patchset is the fourth take of 'various fixes related to EH'
patchset.  From the previous take[1], %01 and %02 made into the tree
and %03 got nacked and #04 acked but didn't make into the tree due to
dependency to %03.  (% denotes patch numbers in the previous take, #
in this take)

Changes from the last take.

* %03 reimplemented using sht->eh_timed_out() callback as Jeff
  suggested[2].

* assert(ata_qc_from_tag(ap, ap->active_tag)) is added to
  ata_scsi_error() in patch %04.

This patchset contains the following patches.

#01-03	: implement and use ata_scsi_timed_out()
#04	: kill NULL qc handling and add above described assertion

I used ATA_QCFLAG_EH_SCHEDULED instead of ATA_QCFLAG_TIMEOUT because
the same flag will be used later by EH patchset in the same manner.

Simulated the following three cases and all work fine.

* normal time out
* timeout wins, but qc completion kicks in before EH
* timeout loses (ata_scsi_timed_out() returns EH_HANDLED)

Thanks.

--
tejun

[1] http://article.gmane.org/gmane.linux.ide/7788
[2] http://article.gmane.org/gmane.linux.ide/7890

[PATCH 1/4] libata: add ATA_QCFLAG_EH_SCHEDULED

[Tejun Heo]

Add ATA_QCFLAG_EH_SCHEDULED.  If this flag is set, the qc is owned by
EH and normal completion path is not allowed to finish it.  This patch
doesn't actually use this flag.

Signed-off-by: Tejun Heo <htejun@gmail.com>

---

 drivers/scsi/libata-core.c |   33 ++++++++++++++++++++-------------
 drivers/scsi/libata-scsi.c |    2 +-
 drivers/scsi/libata.h      |    1 +
 include/linux/libata.h     |    1 +
 4 files changed, 23 insertions(+), 14 deletions(-)

8720412361bfa8a3eb54474c2ceb215ace0a3301
diff --git a/drivers/scsi/libata-core.c b/drivers/scsi/libata-core.c
index 22db739..977a53d 100644
--- a/drivers/scsi/libata-core.c
+++ b/drivers/scsi/libata-core.c
@@ -3620,19 +3620,7 @@ void ata_qc_free(struct ata_queued_cmd *
 	}
 }
 
-/**
- *	ata_qc_complete - Complete an active ATA command
- *	@qc: Command to complete
- *	@err_mask: ATA Status register contents
- *
- *	Indicate to the mid and upper layers that an ATA
- *	command has completed, with either an ok or not-ok status.
- *
- *	LOCKING:
- *	spin_lock_irqsave(host_set lock)
- */
-
-void ata_qc_complete(struct ata_queued_cmd *qc)
+inline void __ata_qc_complete(struct ata_queued_cmd *qc)
 {
 	assert(qc != NULL);	/* ata_qc_from_tag _might_ return NULL */
 	assert(qc->flags & ATA_QCFLAG_ACTIVE);
@@ -3650,6 +3638,25 @@ void ata_qc_complete(struct ata_queued_c
 	qc->complete_fn(qc);
 }
 
+/**
+ *	ata_qc_complete - Complete an active ATA command
+ *	@qc: Command to complete
+ *	@err_mask: ATA Status register contents
+ *
+ *	Indicate to the mid and upper layers that an ATA
+ *	command has completed, with either an ok or not-ok status.
+ *
+ *	LOCKING:
+ *	spin_lock_irqsave(host_set lock)
+ */
+void ata_qc_complete(struct ata_queued_cmd *qc)
+{
+	if (unlikely(qc->flags & ATA_QCFLAG_EH_SCHEDULED))
+		return;
+
+	__ata_qc_complete(qc);
+}
+
 static inline int ata_should_dma_map(struct ata_queued_cmd *qc)
 {
 	struct ata_port *ap = qc->ap;
diff --git a/drivers/scsi/libata-scsi.c b/drivers/scsi/libata-scsi.c
index b007bb4..1df468e 100644
--- a/drivers/scsi/libata-scsi.c
+++ b/drivers/scsi/libata-scsi.c
@@ -770,7 +770,7 @@ static void __ata_eh_qc_complete(struct 
 
 	spin_lock_irqsave(&ap->host_set->lock, flags);
 	qc->scsidone = ata_eh_scsidone;
-	ata_qc_complete(qc);
+	__ata_qc_complete(qc);
 	assert(!ata_tag_valid(qc->tag));
 	spin_unlock_irqrestore(&ap->host_set->lock, flags);
 
diff --git a/drivers/scsi/libata.h b/drivers/scsi/libata.h
index 9d76923..1cd071a 100644
--- a/drivers/scsi/libata.h
+++ b/drivers/scsi/libata.h
@@ -46,6 +46,7 @@ extern struct ata_queued_cmd *ata_qc_new
 extern int ata_rwcmd_protocol(struct ata_queued_cmd *qc);
 extern void ata_qc_free(struct ata_queued_cmd *qc);
 extern unsigned int ata_qc_issue(struct ata_queued_cmd *qc);
+extern void __ata_qc_complete(struct ata_queued_cmd *qc);
 extern int ata_check_atapi_dma(struct ata_queued_cmd *qc);
 extern void ata_dev_select(struct ata_port *ap, unsigned int device,
                            unsigned int wait, unsigned int can_sleep);
diff --git a/include/linux/libata.h b/include/linux/libata.h
index 68b3fe6..5c70a57 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -169,6 +169,7 @@ enum {
 	ATA_QCFLAG_SG		= (1 << 3), /* have s/g table? */
 	ATA_QCFLAG_SINGLE	= (1 << 4), /* no s/g, just a single buffer */
 	ATA_QCFLAG_DMAMAP	= ATA_QCFLAG_SG | ATA_QCFLAG_SINGLE,
+	ATA_QCFLAG_EH_SCHEDULED = (1 << 5), /* EH scheduled */
 
 	/* various lengths of time */
 	ATA_TMOUT_EDD		= 5 * HZ,	/* heuristic */
-- 
1.1.5

[PATCH 4/4] libata: kill NULL qc handling from ->eng_timeout callbacks

[Tejun Heo]

->eng_timeout cannot be invoked with NULL qc anymore.  Add an
assertion in ata_scsi_error() and kill NULL qc handling from all
->eng_timeout callbacks.

Signed-off-by: Tejun Heo <htejun@gmail.com>

---

 drivers/scsi/ahci.c         |   12 +++---------
 drivers/scsi/libata-core.c  |   12 +-----------
 drivers/scsi/libata-scsi.c  |    1 +
 drivers/scsi/sata_mv.c      |    9 ++-------
 drivers/scsi/sata_promise.c |    9 +--------
 drivers/scsi/sata_sil24.c   |    5 -----
 drivers/scsi/sata_sx4.c     |    9 +--------
 7 files changed, 9 insertions(+), 48 deletions(-)

51abbd79791f47e86ba59ab163498203f6eb0ca0
diff --git a/drivers/scsi/ahci.c b/drivers/scsi/ahci.c
index c67189a..fa01894 100644
--- a/drivers/scsi/ahci.c
+++ b/drivers/scsi/ahci.c
@@ -677,19 +677,13 @@ static void ahci_eng_timeout(struct ata_
 
 	spin_lock_irqsave(&host_set->lock, flags);
 
+	ahci_restart_port(ap, readl(port_mmio + PORT_IRQ_STAT));
 	qc = ata_qc_from_tag(ap, ap->active_tag);
-	if (!qc) {
-		printk(KERN_ERR "ata%u: BUG: timeout without command\n",
-		       ap->id);
-	} else {
-		ahci_restart_port(ap, readl(port_mmio + PORT_IRQ_STAT));
-		qc->err_mask |= AC_ERR_TIMEOUT;
-	}
+	qc->err_mask |= AC_ERR_TIMEOUT;
 
 	spin_unlock_irqrestore(&host_set->lock, flags);
 
-	if (qc)
-		ata_eh_qc_complete(qc);
+	ata_eh_qc_complete(qc);
 }
 
 static inline int ahci_host_intr(struct ata_port *ap, struct ata_queued_cmd *qc)
diff --git a/drivers/scsi/libata-core.c b/drivers/scsi/libata-core.c
index d53e0bc..b938c7a 100644
--- a/drivers/scsi/libata-core.c
+++ b/drivers/scsi/libata-core.c
@@ -3524,20 +3524,10 @@ static void ata_qc_timeout(struct ata_qu
 
 void ata_eng_timeout(struct ata_port *ap)
 {
-	struct ata_queued_cmd *qc;
-
 	DPRINTK("ENTER\n");
 
-	qc = ata_qc_from_tag(ap, ap->active_tag);
-	if (qc)
-		ata_qc_timeout(qc);
-	else {
-		printk(KERN_ERR "ata%u: BUG: timeout without command\n",
-		       ap->id);
-		goto out;
-	}
+	ata_qc_timeout(ata_qc_from_tag(ap, ap->active_tag));
 
-out:
 	DPRINTK("EXIT\n");
 }
 
diff --git a/drivers/scsi/libata-scsi.c b/drivers/scsi/libata-scsi.c
index d67cc2f..9d67c67 100644
--- a/drivers/scsi/libata-scsi.c
+++ b/drivers/scsi/libata-scsi.c
@@ -782,6 +782,7 @@ int ata_scsi_error(struct Scsi_Host *hos
 	spin_lock_irqsave(&ap->host_set->lock, flags);
 	assert(!(ap->flags & ATA_FLAG_IN_EH));
 	ap->flags |= ATA_FLAG_IN_EH;
+	assert(ata_qc_from_tag(ap, ap->active_tag) != NULL);
 	spin_unlock_irqrestore(&ap->host_set->lock, flags);
 
 	ap->ops->eng_timeout(ap);
diff --git a/drivers/scsi/sata_mv.c b/drivers/scsi/sata_mv.c
index 1db05f2..6c80527 100644
--- a/drivers/scsi/sata_mv.c
+++ b/drivers/scsi/sata_mv.c
@@ -2027,13 +2027,8 @@ static void mv_eng_timeout(struct ata_po
 	mv_err_intr(ap);
 	mv_stop_and_reset(ap);
 
-	if (!qc) {
-		printk(KERN_ERR "ata%u: BUG: timeout without command\n",
-		       ap->id);
-	} else {
-		qc->err_mask |= AC_ERR_TIMEOUT;
-		ata_eh_qc_complete(qc);
-	}
+	qc->err_mask |= AC_ERR_TIMEOUT;
+	ata_eh_qc_complete(qc);
 }
 
 /**
diff --git a/drivers/scsi/sata_promise.c b/drivers/scsi/sata_promise.c
index 5d4ed54..c9dfd93 100644
--- a/drivers/scsi/sata_promise.c
+++ b/drivers/scsi/sata_promise.c
@@ -432,11 +432,6 @@ static void pdc_eng_timeout(struct ata_p
 	spin_lock_irqsave(&host_set->lock, flags);
 
 	qc = ata_qc_from_tag(ap, ap->active_tag);
-	if (!qc) {
-		printk(KERN_ERR "ata%u: BUG: timeout without command\n",
-		       ap->id);
-		goto out;
-	}
 
 	switch (qc->tf.protocol) {
 	case ATA_PROT_DMA:
@@ -456,10 +451,8 @@ static void pdc_eng_timeout(struct ata_p
 		break;
 	}
 
-out:
 	spin_unlock_irqrestore(&host_set->lock, flags);
-	if (qc)
-		ata_eh_qc_complete(qc);
+	ata_eh_qc_complete(qc);
 	DPRINTK("EXIT\n");
 }
 
diff --git a/drivers/scsi/sata_sil24.c b/drivers/scsi/sata_sil24.c
index f4742ad..962396b 100644
--- a/drivers/scsi/sata_sil24.c
+++ b/drivers/scsi/sata_sil24.c
@@ -639,11 +639,6 @@ static void sil24_eng_timeout(struct ata
 	struct ata_queued_cmd *qc;
 
 	qc = ata_qc_from_tag(ap, ap->active_tag);
-	if (!qc) {
-		printk(KERN_ERR "ata%u: BUG: timeout without command\n",
-		       ap->id);
-		return;
-	}
 
 	printk(KERN_ERR "ata%u: command timeout\n", ap->id);
 	qc->err_mask |= AC_ERR_TIMEOUT;
diff --git a/drivers/scsi/sata_sx4.c b/drivers/scsi/sata_sx4.c
index 3319f03..212cff4 100644
--- a/drivers/scsi/sata_sx4.c
+++ b/drivers/scsi/sata_sx4.c
@@ -867,11 +867,6 @@ static void pdc_eng_timeout(struct ata_p
 	spin_lock_irqsave(&host_set->lock, flags);
 
 	qc = ata_qc_from_tag(ap, ap->active_tag);
-	if (!qc) {
-		printk(KERN_ERR "ata%u: BUG: timeout without command\n",
-		       ap->id);
-		goto out;
-	}
 
 	switch (qc->tf.protocol) {
 	case ATA_PROT_DMA:
@@ -890,10 +885,8 @@ static void pdc_eng_timeout(struct ata_p
 		break;
 	}
 
-out:
 	spin_unlock_irqrestore(&host_set->lock, flags);
-	if (qc)
-		ata_eh_qc_complete(qc);
+	ata_eh_qc_complete(qc);
 	DPRINTK("EXIT\n");
 }
 
-- 
1.1.5

Re: [PATCHSET] libata: various fixes related to EH, take #4

[Jeff Garzik]

Tejun Heo wrote:
> Hello, Jeff.
> 
> This patchset is the fourth take of 'various fixes related to EH'
> patchset.  From the previous take[1], %01 and %02 made into the tree
> and %03 got nacked and #04 acked but didn't make into the tree due to
> dependency to %03.  (% denotes patch numbers in the previous take, #
> in this take)

Applied all four patches to 'upstream', but with comments:

* Given your current implementation, ata_qc_complete() should be inline, 
defined in a header somewhere.  And __ata_qc_complete() should be 
un-inlined.

* Is ATA_FLAG_IN_EH needed in future patches?  It's not used ATM.

Re: [PATCHSET] libata: various fixes related to EH, take #4

[Tejun Heo]

Jeff Garzik wrote:
> Tejun Heo wrote:
> 
>> Hello, Jeff.
>>
>> This patchset is the fourth take of 'various fixes related to EH'
>> patchset.  From the previous take[1], %01 and %02 made into the tree
>> and %03 got nacked and #04 acked but didn't make into the tree due to
>> dependency to %03.  (% denotes patch numbers in the previous take, #
>> in this take)
> 
> 
> Applied all four patches to 'upstream', but with comments:
> 
> * Given your current implementation, ata_qc_complete() should be inline, 
> defined in a header somewhere.  And __ata_qc_complete() should be 
> un-inlined.

Will submit a patch ASAP.

> * Is ATA_FLAG_IN_EH needed in future patches?  It's not used ATM.

I don't know it depends on how we'll do EH <-> irq/pio synchronization, 
but my bet is on ATA_FLAG_IN_EH being used.

Thanks.

-- 
tejun

[PATCHSET] libata: various fixes related to EH, take #4

[Tejun Heo]

Hello, Jeff.

< %nn denotes patch nn from the last iteration, #nn in this iteration >

This is the fourth take of new reset mechanism.  4 (%01-%04) out of 11
patches made into upstream in the last iteration [1].  The rest are
acked but haven't been applied because standard reset operations need
more changes before being used.

This patchset is composed of 9 patches.

#01-02	: modify std reset operations such that they act almost
	  identically with ->phy_reset register-wise.
#03-09	: These seven patches are %05-%09 with minor changes.

After #01, the only meaningful difference of the new reset mechanism
from ->phy_reset is that SError is cleared after SATA hardreset.  This
difference is removed by #02.  #02 is separated to make the change
optional.  If #02 gets into upstream, I'll have to submit another
patch to resurrect it later for EH.  Jeff, it's your call.

After #01 and #02, the following differences remain.

1. SStatus access for sata_print_link_status() in ata_std_postreset()
   is at different postion register-wise compared to ->phy_reset.

2. ->phy_reset doesn't proceed with softreset if ata_busy_sleep()
   fails after PHY is waken, but new reset mechanism proceeds anyway.
   Note that the original code is not very consistent regarding this.
   Some drivers (e.g. ata_piix), uses ata_bus_reset() instead of
   sata_phy_reset() and thus doesn't perform ata_busy_sleep() before
   SRST in the first place.

If a low level driver uses ata_std_probeinit() but doesn't implement
softreset, it will end up performing extra phy_resume and
ata_busy_sleep before the actual hardreset sequence compared to
->phy_reset.  This is noted int the comment above ata_std_probeinit().

#08 has been modified not to use ata_std_probeinit() because it only
implements hardreset, but #09 implements softreset and adds
ata_std_probeinit() back; thus, the end result is unchanged.

Thanks.

--
tejun

[1] http://article.gmane.org/gmane.linux.ide/7804

[PATCH] libata: inline ata_qc_complete()

[Tejun Heo]

This patch inlines ata_qc_complete() and uninlines __ata_qc_complete()
as suggested by Jeff Garzik.

Signed-off-by: Tejun Heo <htejun@gmail.com>
---

Jeff, later EH patches might add one or two more condition tests to
ata_qc_complete().  I don't know whether ata_qc_complete will still
fit as an inline function after EH changes but if it doesn't, it
should be converted then, I guess.  This patch is against the current
upstream (489ff4c7d167be954f715128790bd80d3c888322).

Thanks.

 drivers/scsi/libata-core.c |   23 ++---------------------
 drivers/scsi/libata.h      |    1 -
 include/linux/libata.h     |   20 +++++++++++++++++++-
 3 files changed, 21 insertions(+), 23 deletions(-)

diff --git a/drivers/scsi/libata-core.c b/drivers/scsi/libata-core.c
index 38e72c1..fffbaa9 100644
--- a/drivers/scsi/libata-core.c
+++ b/drivers/scsi/libata-core.c
@@ -3621,7 +3621,7 @@ void ata_qc_free(struct ata_queued_cmd *
 	}
 }
 
-inline void __ata_qc_complete(struct ata_queued_cmd *qc)
+void __ata_qc_complete(struct ata_queued_cmd *qc)
 {
 	assert(qc != NULL);	/* ata_qc_from_tag _might_ return NULL */
 	assert(qc->flags & ATA_QCFLAG_ACTIVE);
@@ -3639,25 +3639,6 @@ inline void __ata_qc_complete(struct ata
 	qc->complete_fn(qc);
 }
 
-/**
- *	ata_qc_complete - Complete an active ATA command
- *	@qc: Command to complete
- *	@err_mask: ATA Status register contents
- *
- *	Indicate to the mid and upper layers that an ATA
- *	command has completed, with either an ok or not-ok status.
- *
- *	LOCKING:
- *	spin_lock_irqsave(host_set lock)
- */
-void ata_qc_complete(struct ata_queued_cmd *qc)
-{
-	if (unlikely(qc->flags & ATA_QCFLAG_EH_SCHEDULED))
-		return;
-
-	__ata_qc_complete(qc);
-}
-
 static inline int ata_should_dma_map(struct ata_queued_cmd *qc)
 {
 	struct ata_port *ap = qc->ap;
@@ -4877,7 +4858,7 @@ EXPORT_SYMBOL_GPL(ata_device_add);
 EXPORT_SYMBOL_GPL(ata_host_set_remove);
 EXPORT_SYMBOL_GPL(ata_sg_init);
 EXPORT_SYMBOL_GPL(ata_sg_init_one);
-EXPORT_SYMBOL_GPL(ata_qc_complete);
+EXPORT_SYMBOL_GPL(__ata_qc_complete);
 EXPORT_SYMBOL_GPL(ata_qc_issue_prot);
 EXPORT_SYMBOL_GPL(ata_eng_timeout);
 EXPORT_SYMBOL_GPL(ata_tf_load);
diff --git a/drivers/scsi/libata.h b/drivers/scsi/libata.h
index 1cd071a..9d76923 100644
--- a/drivers/scsi/libata.h
+++ b/drivers/scsi/libata.h
@@ -46,7 +46,6 @@ extern struct ata_queued_cmd *ata_qc_new
 extern int ata_rwcmd_protocol(struct ata_queued_cmd *qc);
 extern void ata_qc_free(struct ata_queued_cmd *qc);
 extern unsigned int ata_qc_issue(struct ata_queued_cmd *qc);
-extern void __ata_qc_complete(struct ata_queued_cmd *qc);
 extern int ata_check_atapi_dma(struct ata_queued_cmd *qc);
 extern void ata_dev_select(struct ata_port *ap, unsigned int device,
                            unsigned int wait, unsigned int can_sleep);
diff --git a/include/linux/libata.h b/include/linux/libata.h
index c1e1986..695d9ae 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -556,7 +556,7 @@ extern void ata_bmdma_start (struct ata_
 extern void ata_bmdma_stop(struct ata_queued_cmd *qc);
 extern u8   ata_bmdma_status(struct ata_port *ap);
 extern void ata_bmdma_irq_clear(struct ata_port *ap);
-extern void ata_qc_complete(struct ata_queued_cmd *qc);
+extern void __ata_qc_complete(struct ata_queued_cmd *qc);
 extern void ata_eng_timeout(struct ata_port *ap);
 extern void ata_scsi_simulate(struct ata_port *ap, struct ata_device *dev,
 			      struct scsi_cmnd *cmd,
@@ -756,6 +756,24 @@ static inline void ata_qc_reinit(struct 
 	ata_tf_init(qc->ap, &qc->tf, qc->dev->devno);
 }
 
+/**
+ *	ata_qc_complete - Complete an active ATA command
+ *	@qc: Command to complete
+ *	@err_mask: ATA Status register contents
+ *
+ *	Indicate to the mid and upper layers that an ATA
+ *	command has completed, with either an ok or not-ok status.
+ *
+ *	LOCKING:
+ *	spin_lock_irqsave(host_set lock)
+ */
+static inline void ata_qc_complete(struct ata_queued_cmd *qc)
+{
+	if (unlikely(qc->flags & ATA_QCFLAG_EH_SCHEDULED))
+		return;
+
+	__ata_qc_complete(qc);
+}
 
 /**
  *	ata_irq_on - Enable interrupts on a port.

Re: [PATCH] libata: inline ata_qc_complete()

[Jeff Garzik]

Tejun Heo wrote:
> This patch inlines ata_qc_complete() and uninlines __ata_qc_complete()
> as suggested by Jeff Garzik.
> 
> Signed-off-by: Tejun Heo <htejun@gmail.com>
> ---
> 
> Jeff, later EH patches might add one or two more condition tests to
> ata_qc_complete().  I don't know whether ata_qc_complete will still
> fit as an inline function after EH changes but if it doesn't, it
> should be converted then, I guess.  This patch is against the current
> upstream (489ff4c7d167be954f715128790bd80d3c888322).

applied