Skip to contentSkip to conversation

libata: various fixes related to EH, take #4

PATCH Applied Tejun Heo 2 participants 9 messages Feb 10, 2006 – Feb 11, 2006

Subsystems (2)
SCSI libata
lore
Share & tools
lore

Tejun Heo

author
#0 lore raw

PATCH 1/1 libata: various fixes related to EH, take #4

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

author
#1 lore raw

PATCH 1/4 libata: add ATA_QCFLAG_EH_SCHEDULED

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 <[email protected]>

---

 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

drivers/scsi/libata-core.c +20 -13
index 22db739..977a53d 100644
@@ -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;
drivers/scsi/libata-scsi.c +1 -1
index b007bb4..1df468e 100644
@@ -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);
drivers/scsi/libata.h +1 -0
index 9d76923..1cd071a 100644
@@ -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);
include/linux/libata.h +1 -0
index 68b3fe6..5c70a57 100644
@@ -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 */
Signature · 1 line
1.1.5
author
#2 lore raw

PATCH 2/4 libata: implement ata_scsi_timed_out()

In reply to 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 <[email protected]>.

Signed-off-by Tejun Heo <[email protected]>

---

 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

drivers/scsi/libata-core.c +1 -0
index 977a53d..d53e0bc 100644
@@ -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);
drivers/scsi/libata-scsi.c +41 -0

47 lines not on this page · raw message

include/linux/libata.h +1 -0
index 5c70a57..c1e1986 100644
@@ -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);
Signature · 1 line
1.1.5
author
#3 lore raw

PATCH 3/4 libata: use ata_scsi_timed_out()

In reply to Tejun Heo

Make all libata low level drivers use ata_scsi_timed_out().

Signed-off-by Tejun Heo <[email protected]>

---

 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

drivers/scsi/ahci.c +1 -0
index c840d5e..c67189a 100644
@@ -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,
drivers/scsi/ata_piix.c +1 -0
index 49cc420..4933ba2 100644
@@ -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,
drivers/scsi/pdc_adma.c +1 -0
index 3a6bf58..d0ad3eb 100644
@@ -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,
drivers/scsi/sata_mv.c +1 -0
index 3e91632..1db05f2 100644
@@ -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,
drivers/scsi/sata_nv.c +1 -0
index bbbb55e..cdfeb9a 100644
@@ -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,
drivers/scsi/sata_promise.c +1 -0
index 0950a8e..5d4ed54 100644
@@ -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,
drivers/scsi/sata_qstor.c +1 -0
index 2afbeb7..82c3df7 100644
@@ -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,
drivers/scsi/sata_sil.c +1 -0
index 17f74d3..f40f25e 100644
@@ -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,
drivers/scsi/sata_sil24.c +1 -0
index 7222fc7..f4742ad 100644
@@ -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,
drivers/scsi/sata_sis.c +1 -0
index 2df8c56..2f18157 100644
@@ -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,
drivers/scsi/sata_svw.c +1 -0
index d847256..f369c30 100644
@@ -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,
drivers/scsi/sata_sx4.c +1 -0
index 9f992fb..3319f03 100644
@@ -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,
drivers/scsi/sata_uli.c +1 -0
index 9635ca7..c500f24 100644
@@ -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,
drivers/scsi/sata_via.c +1 -0
index 6d5b0a7..2e20887 100644
@@ -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,
drivers/scsi/sata_vsc.c +1 -0
index 2e2c3b7..cf1f8a6 100644
@@ -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,
Signature · 1 line
1.1.5
author
#4 lore raw

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

In reply to 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 <[email protected]>

---

 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

drivers/scsi/ahci.c +3 -9
index c67189a..fa01894 100644
@@ -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)
drivers/scsi/libata-core.c +1 -11
index d53e0bc..b938c7a 100644
@@ -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");
 }
drivers/scsi/libata-scsi.c +1 -0
index d67cc2f..9d67c67 100644
@@ -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);
drivers/scsi/sata_mv.c +2 -7
index 1db05f2..6c80527 100644
@@ -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);
 }
 
 /**
drivers/scsi/sata_promise.c +1 -8
index 5d4ed54..c9dfd93 100644
@@ -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");
 }
drivers/scsi/sata_sil24.c +0 -5
index f4742ad..962396b 100644
@@ -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;
drivers/scsi/sata_sx4.c +1 -8
index 3319f03..212cff4 100644
@@ -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");
 }
Signature · 1 line
1.1.5
Applied
#5 lore raw
In reply to Tejun Heo
Quoted 7 lines · 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)

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.

author
#6 lore raw
Quoted 11 lines · Jeff Garzik
Quoted 7 lines · 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)

Quoted 3 lines

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.

Quoted 1 line

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

Signature · 1 line
tejun
author
#7 lore raw

PATCH 1/1 libata: inline ata_qc_complete()

In reply to Jeff Garzik

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

Signed-off-by Tejun Heo <[email protected]>

---

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(-)
drivers/scsi/libata-core.c +2 -21
index 38e72c1..fffbaa9 100644
@@ -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);
drivers/scsi/libata.h +0 -1
index 1cd071a..9d76923 100644
@@ -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);
include/linux/libata.h +19 -1
index c1e1986..695d9ae 100644
@@ -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.
Applied
#8 lore raw

Re: [PATCH] libata: inline ata_qc_complete()

Quoted 11 lines · Tejun Heo

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

Signed-off-by Tejun Heo <[email protected]>

---

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