[PATCH 0/5] libata: support the ata host which implements a queue depth less than 32

[PATCH 0/5] libata: support the ata host which implements a queue depth less than 32

[Kevin Hao]

Hi,

The fsl sata on mpc8315e is broken after the commit 8a4aeec8d2d6
("libata/ahci: accommodate tag ordered controllers"). This patch
serial tries to fix this problem. This is based on the next-20140702.
Boot test on the mpc8315rdb board.

Kevin Hao (5):
  libata: using ata_tag_internal() instead of open-coding
  libata: introduce ata_get_internal_tag()
  libata: add a 'struct ata_port *' argument for the ata tag help
    functions
  libata: support the ata host which implements a queue depth less than
    32
  sata_fsl: set the correct queue depth for the host controller

 drivers/ata/libata-core.c | 50 +++++++++++++++++++++++++++++++++++------------
 drivers/ata/libata-eh.c   | 32 ++++++++++++++++++------------
 drivers/ata/libata-scsi.c |  5 +++--
 drivers/ata/sata_fsl.c    | 22 +++++++++++++--------
 drivers/ata/sata_nv.c     |  9 +++++----
 drivers/ata/sata_sil24.c  | 10 +++++-----
 include/linux/libata.h    | 22 ++++++++++++++-------
 7 files changed, 100 insertions(+), 50 deletions(-)

-- 
1.9.3

[PATCH 1/5] libata: using ata_tag_internal() instead of open-coding

[Kevin Hao]

Signed-off-by: Kevin Hao <haokexin@gmail.com>
---
 drivers/ata/libata-core.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index 8f3043165048..2895254d8388 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -752,7 +752,7 @@ int ata_build_rw_tf(struct ata_taskfile *tf, struct ata_device *dev,
 	tf->flags |= ATA_TFLAG_ISADDR | ATA_TFLAG_DEVICE;
 	tf->flags |= tf_flags;
 
-	if (ata_ncq_enabled(dev) && likely(tag != ATA_TAG_INTERNAL)) {
+	if (ata_ncq_enabled(dev) && likely(!ata_tag_internal(tag))) {
 		/* yay, NCQ */
 		if (!lba_48_ok(block, n_block))
 			return -ERANGE;
@@ -4738,7 +4738,7 @@ static struct ata_queued_cmd *ata_qc_new(struct ata_port *ap)
 		tag = (i + ap->last_tag + 1) % ATA_MAX_QUEUE;
 
 		/* the last tag is reserved for internal command. */
-		if (tag == ATA_TAG_INTERNAL)
+		if (ata_tag_internal(tag))
 			continue;
 
 		if (!test_and_set_bit(tag, &ap->qc_allocated)) {
-- 
1.9.3

[PATCH 2/5] libata: introduce ata_get_internal_tag()

[Kevin Hao]

Introduce a helper function to get the tag number which is reserved
for internal use.

Signed-off-by: Kevin Hao <haokexin@gmail.com>
---
 drivers/ata/libata-core.c | 2 +-
 drivers/ata/sata_fsl.c    | 4 ++--
 include/linux/libata.h    | 5 +++++
 3 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index 2895254d8388..8807287ab7b0 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -1576,7 +1576,7 @@ unsigned ata_exec_internal_sg(struct ata_device *dev,
 	 * EH stuff without converting to it.
 	 */
 	if (ap->ops->error_handler)
-		tag = ATA_TAG_INTERNAL;
+		tag = ata_get_internal_tag();
 	else
 		tag = 0;
 
diff --git a/drivers/ata/sata_fsl.c b/drivers/ata/sata_fsl.c
index 07bc7e4dbd04..7fed5c5be426 100644
--- a/drivers/ata/sata_fsl.c
+++ b/drivers/ata/sata_fsl.c
@@ -1290,9 +1290,9 @@ static void sata_fsl_host_intr(struct ata_port *ap)
 		ata_qc_complete_multiple(ap, ap->qc_active ^ done_mask);
 		return;
 
-	} else if ((ap->qc_active & (1 << ATA_TAG_INTERNAL))) {
+	} else if ((ap->qc_active & (1 << ata_get_internal_tag()))) {
 		iowrite32(1, hcr_base + CC);
-		qc = ata_qc_from_tag(ap, ATA_TAG_INTERNAL);
+		qc = ata_qc_from_tag(ap, ata_get_internal_tag());
 
 		DPRINTK("completing non-ncq cmd, CC=0x%x\n",
 			 ioread32(hcr_base + CC));
diff --git a/include/linux/libata.h b/include/linux/libata.h
index 5ab4e3a76721..63462f1274df 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -1479,6 +1479,11 @@ static inline unsigned int ata_tag_valid(unsigned int tag)
 	return (tag < ATA_MAX_QUEUE) ? 1 : 0;
 }
 
+static inline unsigned int ata_get_internal_tag(void)
+{
+	return ATA_TAG_INTERNAL;
+}
+
 static inline unsigned int ata_tag_internal(unsigned int tag)
 {
 	return tag == ATA_TAG_INTERNAL;
-- 
1.9.3

[PATCH 3/5] libata: add a 'struct ata_port *' argument for the ata tag help functions

[Kevin Hao]

Currently we assume that all the ata host controller has a 32 queue
depth. But if a ata controller implement a queue depth less than 32,
we may need an additional 'struct ata_port *' argument to implement
these help functions. This will be used by the following patch.

Signed-off-by: Kevin Hao <haokexin@gmail.com>
---
 drivers/ata/libata-core.c | 18 ++++++++++--------
 drivers/ata/libata-eh.c   |  4 ++--
 drivers/ata/sata_fsl.c    | 17 ++++++++++-------
 drivers/ata/sata_nv.c     |  9 +++++----
 drivers/ata/sata_sil24.c  | 10 +++++-----
 include/linux/libata.h    | 11 ++++++-----
 6 files changed, 38 insertions(+), 31 deletions(-)

diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index 8807287ab7b0..f1cde0d289fa 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -752,7 +752,8 @@ int ata_build_rw_tf(struct ata_taskfile *tf, struct ata_device *dev,
 	tf->flags |= ATA_TFLAG_ISADDR | ATA_TFLAG_DEVICE;
 	tf->flags |= tf_flags;
 
-	if (ata_ncq_enabled(dev) && likely(!ata_tag_internal(tag))) {
+	if (ata_ncq_enabled(dev) &&
+			likely(!ata_tag_internal(dev->link->ap, tag))) {
 		/* yay, NCQ */
 		if (!lba_48_ok(block, n_block))
 			return -ERANGE;
@@ -1576,7 +1577,7 @@ unsigned ata_exec_internal_sg(struct ata_device *dev,
 	 * EH stuff without converting to it.
 	 */
 	if (ap->ops->error_handler)
-		tag = ata_get_internal_tag();
+		tag = ata_get_internal_tag(ap);
 	else
 		tag = 0;
 
@@ -4630,10 +4631,10 @@ int ata_std_qc_defer(struct ata_queued_cmd *qc)
 	struct ata_link *link = qc->dev->link;
 
 	if (qc->tf.protocol == ATA_PROT_NCQ) {
-		if (!ata_tag_valid(link->active_tag))
+		if (!ata_tag_valid(qc->ap, link->active_tag))
 			return 0;
 	} else {
-		if (!ata_tag_valid(link->active_tag) && !link->sactive)
+		if (!ata_tag_valid(qc->ap, link->active_tag) && !link->sactive)
 			return 0;
 	}
 
@@ -4738,7 +4739,7 @@ static struct ata_queued_cmd *ata_qc_new(struct ata_port *ap)
 		tag = (i + ap->last_tag + 1) % ATA_MAX_QUEUE;
 
 		/* the last tag is reserved for internal command. */
-		if (ata_tag_internal(tag))
+		if (ata_tag_internal(ap, tag))
 			continue;
 
 		if (!test_and_set_bit(tag, &ap->qc_allocated)) {
@@ -4797,7 +4798,7 @@ void ata_qc_free(struct ata_queued_cmd *qc)
 
 	qc->flags = 0;
 	tag = qc->tag;
-	if (likely(ata_tag_valid(tag))) {
+	if (likely(ata_tag_valid(ap, tag))) {
 		qc->tag = ATA_TAG_POISON;
 		clear_bit(tag, &ap->qc_allocated);
 	}
@@ -4906,7 +4907,7 @@ void ata_qc_complete(struct ata_queued_cmd *qc)
 		 * Finish internal commands without any further processing
 		 * and always with the result TF filled.
 		 */
-		if (unlikely(ata_tag_internal(qc->tag))) {
+		if (unlikely(ata_tag_internal(ap, qc->tag))) {
 			fill_result_tf(qc);
 			__ata_qc_complete(qc);
 			return;
@@ -5035,7 +5036,8 @@ void ata_qc_issue(struct ata_queued_cmd *qc)
 	 * check is skipped for old EH because it reuses active qc to
 	 * request ATAPI sense.
 	 */
-	WARN_ON_ONCE(ap->ops->error_handler && ata_tag_valid(link->active_tag));
+	WARN_ON_ONCE(ap->ops->error_handler &&
+					ata_tag_valid(ap, link->active_tag));
 
 	if (ata_is_ncq(prot)) {
 		WARN_ON_ONCE(link->sactive & (1 << qc->tag));
diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c
index 6760fc4e85b8..a25af5258217 100644
--- a/drivers/ata/libata-eh.c
+++ b/drivers/ata/libata-eh.c
@@ -1295,7 +1295,7 @@ static void __ata_eh_qc_complete(struct ata_queued_cmd *qc)
 	spin_lock_irqsave(ap->lock, flags);
 	qc->scsidone = ata_eh_scsidone;
 	__ata_qc_complete(qc);
-	WARN_ON(ata_tag_valid(qc->tag));
+	WARN_ON(ata_tag_valid(ap, qc->tag));
 	spin_unlock_irqrestore(ap->lock, flags);
 
 	scsi_eh_finish_cmd(scmd, &ap->eh_done_q);
@@ -3328,7 +3328,7 @@ static int ata_eh_maybe_retry_flush(struct ata_device *dev)
 	int rc = 0;
 
 	/* did flush fail for this device? */
-	if (!ata_tag_valid(link->active_tag))
+	if (!ata_tag_valid(ap, link->active_tag))
 		return 0;
 
 	qc = __ata_qc_from_tag(ap, link->active_tag);
diff --git a/drivers/ata/sata_fsl.c b/drivers/ata/sata_fsl.c
index 7fed5c5be426..bffdd96538ff 100644
--- a/drivers/ata/sata_fsl.c
+++ b/drivers/ata/sata_fsl.c
@@ -389,13 +389,16 @@ static ssize_t fsl_sata_rx_watermark_store(struct device *dev,
 	return strlen(buf);
 }
 
-static inline unsigned int sata_fsl_tag(unsigned int tag,
+static inline unsigned int sata_fsl_tag(struct ata_queued_cmd *qc,
 					void __iomem *hcr_base)
 {
+	unsigned int tag = qc->tag;
+	struct ata_port *ap = qc->ap;
+
 	/* We let libATA core do actual (queue) tag allocation */
 
 	/* all non NCQ/queued commands should have tag#0 */
-	if (ata_tag_internal(tag)) {
+	if (ata_tag_internal(ap, tag)) {
 		DPRINTK("mapping internal cmds to tag#0\n");
 		return 0;
 	}
@@ -518,7 +521,7 @@ static void sata_fsl_qc_prep(struct ata_queued_cmd *qc)
 	struct sata_fsl_port_priv *pp = ap->private_data;
 	struct sata_fsl_host_priv *host_priv = ap->host->private_data;
 	void __iomem *hcr_base = host_priv->hcr_base;
-	unsigned int tag = sata_fsl_tag(qc->tag, hcr_base);
+	unsigned int tag = sata_fsl_tag(qc, hcr_base);
 	struct command_desc *cd;
 	u32 desc_info = CMD_DESC_RES | CMD_DESC_SNOOP_ENABLE;
 	u32 num_prde = 0;
@@ -565,7 +568,7 @@ static unsigned int sata_fsl_qc_issue(struct ata_queued_cmd *qc)
 	struct ata_port *ap = qc->ap;
 	struct sata_fsl_host_priv *host_priv = ap->host->private_data;
 	void __iomem *hcr_base = host_priv->hcr_base;
-	unsigned int tag = sata_fsl_tag(qc->tag, hcr_base);
+	unsigned int tag = sata_fsl_tag(qc, hcr_base);
 
 	VPRINTK("xx_qc_issue called,CQ=0x%x,CA=0x%x,CE=0x%x,CC=0x%x\n",
 		ioread32(CQ + hcr_base),
@@ -594,7 +597,7 @@ static bool sata_fsl_qc_fill_rtf(struct ata_queued_cmd *qc)
 	struct sata_fsl_port_priv *pp = qc->ap->private_data;
 	struct sata_fsl_host_priv *host_priv = qc->ap->host->private_data;
 	void __iomem *hcr_base = host_priv->hcr_base;
-	unsigned int tag = sata_fsl_tag(qc->tag, hcr_base);
+	unsigned int tag = sata_fsl_tag(qc, hcr_base);
 	struct command_desc *cd;
 
 	cd = pp->cmdentry + tag;
@@ -1290,9 +1293,9 @@ static void sata_fsl_host_intr(struct ata_port *ap)
 		ata_qc_complete_multiple(ap, ap->qc_active ^ done_mask);
 		return;
 
-	} else if ((ap->qc_active & (1 << ata_get_internal_tag()))) {
+	} else if ((ap->qc_active & (1 << ata_get_internal_tag(ap)))) {
 		iowrite32(1, hcr_base + CC);
-		qc = ata_qc_from_tag(ap, ata_get_internal_tag());
+		qc = ata_qc_from_tag(ap, ata_get_internal_tag(ap));
 
 		DPRINTK("completing non-ncq cmd, CC=0x%x\n",
 			 ioread32(hcr_base + CC));
diff --git a/drivers/ata/sata_nv.c b/drivers/ata/sata_nv.c
index cdf99fac139a..78f7af47f0ce 100644
--- a/drivers/ata/sata_nv.c
+++ b/drivers/ata/sata_nv.c
@@ -931,7 +931,7 @@ static irqreturn_t nv_adma_interrupt(int irq, void *dev_instance)
 		if (pp->flags & NV_ADMA_PORT_REGISTER_MODE) {
 			u8 irq_stat = readb(host->iomap[NV_MMIO_BAR] + NV_INT_STATUS_CK804)
 				>> (NV_INT_PORT_SHIFT * i);
-			if (ata_tag_valid(ap->link.active_tag))
+			if (ata_tag_valid(ap, ap->link.active_tag))
 				/** NV_INT_DEV indication seems unreliable
 				    at times at least in ADMA mode. Force it
 				    on always when a command is active, to
@@ -1001,7 +1001,7 @@ static irqreturn_t nv_adma_interrupt(int irq, void *dev_instance)
 
 			if (status & NV_ADMA_STAT_CPBERR) {
 				/* check all active commands */
-				if (ata_tag_valid(ap->link.active_tag))
+				if (ata_tag_valid(ap, ap->link.active_tag))
 					check_commands = 1 <<
 						ap->link.active_tag;
 				else
@@ -1675,7 +1675,8 @@ static void nv_adma_error_handler(struct ata_port *ap)
 		int i;
 		u16 tmp;
 
-		if (ata_tag_valid(ap->link.active_tag) || ap->link.sactive) {
+		if (ata_tag_valid(ap, ap->link.active_tag) ||
+			 ap->link.sactive) {
 			u32 notifier = readl(mmio + NV_ADMA_NOTIFIER);
 			u32 notifier_error = readl(mmio + NV_ADMA_NOTIFIER_ERROR);
 			u32 gen_ctl = readl(pp->gen_block + NV_ADMA_GEN_CTL);
@@ -1692,7 +1693,7 @@ static void nv_adma_error_handler(struct ata_port *ap)
 
 			for (i = 0; i < NV_ADMA_MAX_CPBS; i++) {
 				struct nv_adma_cpb *cpb = &pp->cpb[i];
-				if ((ata_tag_valid(ap->link.active_tag) && i == ap->link.active_tag) ||
+				if ((ata_tag_valid(ap, ap->link.active_tag) && i == ap->link.active_tag) ||
 				    ap->link.sactive & (1 << i))
 					ata_port_err(ap,
 						"CPB %d: ctl_flags 0x%x, resp_flags 0x%x\n",
diff --git a/drivers/ata/sata_sil24.c b/drivers/ata/sata_sil24.c
index 0534890f118a..24bb1bd27f01 100644
--- a/drivers/ata/sata_sil24.c
+++ b/drivers/ata/sata_sil24.c
@@ -458,9 +458,9 @@ static const struct ata_port_info sil24_port_info[] = {
 	},
 };
 
-static int sil24_tag(int tag)
+static int sil24_tag(struct ata_port *ap, int tag)
 {
-	if (unlikely(ata_tag_internal(tag)))
+	if (unlikely(ata_tag_internal(ap, tag)))
 		return 0;
 	return tag;
 }
@@ -491,7 +491,7 @@ static void sil24_read_tf(struct ata_port *ap, int tag, struct ata_taskfile *tf)
 	struct sil24_prb __iomem *prb;
 	u8 fis[6 * 4];
 
-	prb = port + PORT_LRAM + sil24_tag(tag) * PORT_LRAM_SLOT_SZ;
+	prb = port + PORT_LRAM + sil24_tag(ap, tag) * PORT_LRAM_SLOT_SZ;
 	memcpy_fromio(fis, prb->fis, sizeof(fis));
 	ata_tf_from_fis(fis, tf);
 }
@@ -848,7 +848,7 @@ static void sil24_qc_prep(struct ata_queued_cmd *qc)
 	struct sil24_sge *sge;
 	u16 ctrl = 0;
 
-	cb = &pp->cmd_block[sil24_tag(qc->tag)];
+	cb = &pp->cmd_block[sil24_tag(ap, qc->tag)];
 
 	if (!ata_is_atapi(qc->tf.protocol)) {
 		prb = &cb->ata.prb;
@@ -890,7 +890,7 @@ static unsigned int sil24_qc_issue(struct ata_queued_cmd *qc)
 	struct ata_port *ap = qc->ap;
 	struct sil24_port_priv *pp = ap->private_data;
 	void __iomem *port = sil24_port_base(ap);
-	unsigned int tag = sil24_tag(qc->tag);
+	unsigned int tag = sil24_tag(ap, qc->tag);
 	dma_addr_t paddr;
 	void __iomem *activate;
 
diff --git a/include/linux/libata.h b/include/linux/libata.h
index 63462f1274df..74976a08c9bc 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -1474,17 +1474,18 @@ extern void ata_port_pbar_desc(struct ata_port *ap, int bar, ssize_t offset,
 			       const char *name);
 #endif
 
-static inline unsigned int ata_tag_valid(unsigned int tag)
+static inline unsigned int ata_tag_valid(struct ata_port *ap, unsigned int tag)
 {
 	return (tag < ATA_MAX_QUEUE) ? 1 : 0;
 }
 
-static inline unsigned int ata_get_internal_tag(void)
+static inline unsigned int ata_get_internal_tag(struct ata_port *ap)
 {
 	return ATA_TAG_INTERNAL;
 }
 
-static inline unsigned int ata_tag_internal(unsigned int tag)
+static inline unsigned int ata_tag_internal(struct ata_port *ap,
+						unsigned int tag)
 {
 	return tag == ATA_TAG_INTERNAL;
 }
@@ -1536,7 +1537,7 @@ static inline int ata_link_max_devices(const struct ata_link *link)
 
 static inline int ata_link_active(struct ata_link *link)
 {
-	return ata_tag_valid(link->active_tag) || link->sactive;
+	return ata_tag_valid(link->ap, link->active_tag) || link->sactive;
 }
 
 /*
@@ -1627,7 +1628,7 @@ static inline void ata_qc_set_polling(struct ata_queued_cmd *qc)
 static inline struct ata_queued_cmd *__ata_qc_from_tag(struct ata_port *ap,
 						       unsigned int tag)
 {
-	if (likely(ata_tag_valid(tag)))
+	if (likely(ata_tag_valid(ap, tag)))
 		return &ap->qcmd[tag];
 	return NULL;
 }
-- 
1.9.3

[PATCH 4/5] libata: support the ata host which implements a queue depth less than 32

[Kevin Hao]

The sata on fsl mpc8315e is broken after the commit 8a4aeec8d2d6
("libata/ahci: accommodate tag ordered controllers"). The reason is
that the ata controller on this SoC only implement a queue depth of
16. When issuing the commands in tag order, all the commands in tag
16 ~ 17 are mapped to tag 0 unconditionally and then causes the sata
malfunction. It makes no senses to use a 32 queue in software while
the hardware has less queue depth. This patch provides the function
for libata to adjust the queue depth for a host controller.

Signed-off-by: Kevin Hao <haokexin@gmail.com>
---
 drivers/ata/libata-core.c | 32 ++++++++++++++++++++++++++++----
 drivers/ata/libata-eh.c   | 28 ++++++++++++++++++----------
 drivers/ata/libata-scsi.c |  5 +++--
 include/linux/libata.h    | 10 ++++++----
 4 files changed, 55 insertions(+), 20 deletions(-)

diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index f1cde0d289fa..08131fe45d42 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -2115,7 +2115,8 @@ static int ata_dev_config_ncq(struct ata_device *dev,
 		return 0;
 	}
 	if (ap->flags & ATA_FLAG_NCQ) {
-		hdepth = min(ap->scsi_host->can_queue, ATA_MAX_QUEUE - 1);
+		hdepth = min((unsigned int)ap->scsi_host->can_queue,
+				 ap->host->queue_depth - 1);
 		dev->flags |= ATA_DFLAG_NCQ;
 	}
 
@@ -4729,14 +4730,14 @@ void swap_buf_le16(u16 *buf, unsigned int buf_words)
 static struct ata_queued_cmd *ata_qc_new(struct ata_port *ap)
 {
 	struct ata_queued_cmd *qc = NULL;
-	unsigned int i, tag;
+	unsigned int i, tag, max_queue = ap->host->queue_depth;
 
 	/* no command while frozen */
 	if (unlikely(ap->pflags & ATA_PFLAG_FROZEN))
 		return NULL;
 
-	for (i = 0; i < ATA_MAX_QUEUE; i++) {
-		tag = (i + ap->last_tag + 1) % ATA_MAX_QUEUE;
+	for (i = 0; i < max_queue; i++) {
+		tag = (i + ap->last_tag + 1) % max_queue;
 
 		/* the last tag is reserved for internal command. */
 		if (ata_tag_internal(ap, tag))
@@ -5689,6 +5690,27 @@ static void ata_host_release(struct device *gendev, void *res)
 }
 
 /**
+ *	ata_host_set_queue_depth - set the ATA host controller's queue depth
+ *	@host: ATA host to be set for
+ *	@queue_depth: the queue depth implemented on this host controller
+ *
+ *	We would assume that the ATA host controller has 32 queue depth and
+ *	then set the host->queue_depth to 32 by default. If this is not true
+ *	for one specific ATA host controller, you need to invoke this function
+ *	to set the correct value.
+ */
+int ata_host_set_queue_depth(struct ata_host *host, unsigned int queue_depth)
+{
+	if (!queue_depth || queue_depth > ATA_MAX_QUEUE) {
+		dev_err(host->dev, "Invalid queue depth\n");
+		return -EINVAL;
+	}
+
+	host->queue_depth = queue_depth;
+	return 0;
+}
+
+/**
  *	ata_host_alloc - allocate and init basic ATA host resources
  *	@dev: generic device this host is associated with
  *	@max_ports: maximum number of ATA ports associated with this host
@@ -5733,6 +5755,7 @@ struct ata_host *ata_host_alloc(struct device *dev, int max_ports)
 	mutex_init(&host->eh_mutex);
 	host->dev = dev;
 	host->n_ports = max_ports;
+	host->queue_depth = ATA_MAX_QUEUE;
 
 	/* allocate ports bound to this host */
 	for (i = 0; i < max_ports; i++) {
@@ -6857,6 +6880,7 @@ EXPORT_SYMBOL_GPL(ata_dev_next);
 EXPORT_SYMBOL_GPL(ata_std_bios_param);
 EXPORT_SYMBOL_GPL(ata_scsi_unlock_native_capacity);
 EXPORT_SYMBOL_GPL(ata_host_init);
+EXPORT_SYMBOL_GPL(ata_host_set_queue_depth);
 EXPORT_SYMBOL_GPL(ata_host_alloc);
 EXPORT_SYMBOL_GPL(ata_host_alloc_pinfo);
 EXPORT_SYMBOL_GPL(ata_slave_link_init);
diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c
index a25af5258217..e277022fc552 100644
--- a/drivers/ata/libata-eh.c
+++ b/drivers/ata/libata-eh.c
@@ -625,6 +625,7 @@ void ata_scsi_cmd_error_handler(struct Scsi_Host *host, struct ata_port *ap,
 {
 	int i;
 	unsigned long flags;
+	unsigned int max_queue = ap->host->queue_depth;
 
 	/* make sure sff pio task is not running */
 	ata_sff_flush_pio_task(ap);
@@ -664,14 +665,14 @@ void ata_scsi_cmd_error_handler(struct Scsi_Host *host, struct ata_port *ap,
 		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++) {
+			for (i = 0; i < max_queue; i++) {
 				qc = __ata_qc_from_tag(ap, i);
 				if (qc->flags & ATA_QCFLAG_ACTIVE &&
 				    qc->scsicmd == scmd)
 					break;
 			}
 
-			if (i < ATA_MAX_QUEUE) {
+			if (i < max_queue) {
 				/* the scmd has an associated qc */
 				if (!(qc->flags & ATA_QCFLAG_FAILED)) {
 					/* which hasn't failed yet, timeout */
@@ -870,9 +871,10 @@ static int ata_eh_nr_in_flight(struct ata_port *ap)
 {
 	unsigned int tag;
 	int nr = 0;
+	unsigned int max_queue = ap->host->queue_depth;
 
 	/* count only non-internal commands */
-	for (tag = 0; tag < ATA_MAX_QUEUE - 1; tag++)
+	for (tag = 0; tag < max_queue - 1; tag++)
 		if (ata_qc_from_tag(ap, tag))
 			nr++;
 
@@ -884,6 +886,7 @@ void ata_eh_fastdrain_timerfn(unsigned long arg)
 	struct ata_port *ap = (void *)arg;
 	unsigned long flags;
 	int cnt;
+	unsigned int max_queue = ap->host->queue_depth;
 
 	spin_lock_irqsave(ap->lock, flags);
 
@@ -899,7 +902,7 @@ void ata_eh_fastdrain_timerfn(unsigned long arg)
 		/* No progress during the last interval, tag all
 		 * in-flight qcs as timed out and freeze the port.
 		 */
-		for (tag = 0; tag < ATA_MAX_QUEUE - 1; tag++) {
+		for (tag = 0; tag < max_queue - 1; tag++) {
 			struct ata_queued_cmd *qc = ata_qc_from_tag(ap, tag);
 			if (qc)
 				qc->err_mask |= AC_ERR_TIMEOUT;
@@ -1047,13 +1050,14 @@ void ata_port_schedule_eh(struct ata_port *ap)
 static int ata_do_link_abort(struct ata_port *ap, struct ata_link *link)
 {
 	int tag, nr_aborted = 0;
+	unsigned int max_queue = ap->host->queue_depth;
 
 	WARN_ON(!ap->ops->error_handler);
 
 	/* we're gonna abort all commands, no need for fast drain */
 	ata_eh_set_pending(ap, 0);
 
-	for (tag = 0; tag < ATA_MAX_QUEUE; tag++) {
+	for (tag = 0; tag < max_queue; tag++) {
 		struct ata_queued_cmd *qc = ata_qc_from_tag(ap, tag);
 
 		if (qc && (!link || qc->dev->link == link)) {
@@ -1733,6 +1737,7 @@ void ata_eh_analyze_ncq_error(struct ata_link *link)
 	struct ata_queued_cmd *qc;
 	struct ata_taskfile tf;
 	int tag, rc;
+	unsigned int max_queue = ap->host->queue_depth;
 
 	/* if frozen, we can't do much */
 	if (ap->pflags & ATA_PFLAG_FROZEN)
@@ -1743,7 +1748,7 @@ void ata_eh_analyze_ncq_error(struct ata_link *link)
 		return;
 
 	/* has LLDD analyzed already? */
-	for (tag = 0; tag < ATA_MAX_QUEUE; tag++) {
+	for (tag = 0; tag < max_queue; tag++) {
 		qc = __ata_qc_from_tag(ap, tag);
 
 		if (!(qc->flags & ATA_QCFLAG_FAILED))
@@ -2124,6 +2129,7 @@ static void ata_eh_link_autopsy(struct ata_link *link)
 	int tag;
 	u32 serror;
 	int rc;
+	unsigned int max_queue = ap->host->queue_depth;
 
 	DPRINTK("ENTER\n");
 
@@ -2151,7 +2157,7 @@ static void ata_eh_link_autopsy(struct ata_link *link)
 
 	all_err_mask |= ehc->i.err_mask;
 
-	for (tag = 0; tag < ATA_MAX_QUEUE; tag++) {
+	for (tag = 0; tag < max_queue; tag++) {
 		struct ata_queued_cmd *qc = __ata_qc_from_tag(ap, tag);
 
 		if (!(qc->flags & ATA_QCFLAG_FAILED) ||
@@ -2405,6 +2411,7 @@ static void ata_eh_link_report(struct ata_link *link)
 	const char *frozen, *desc;
 	char tries_buf[6] = "";
 	int tag, nr_failed = 0;
+	unsigned int max_queue = ap->host->queue_depth;
 
 	if (ehc->i.flags & ATA_EHI_QUIET)
 		return;
@@ -2413,7 +2420,7 @@ static void ata_eh_link_report(struct ata_link *link)
 	if (ehc->i.desc[0] != '\0')
 		desc = ehc->i.desc;
 
-	for (tag = 0; tag < ATA_MAX_QUEUE; tag++) {
+	for (tag = 0; tag < max_queue; tag++) {
 		struct ata_queued_cmd *qc = __ata_qc_from_tag(ap, tag);
 
 		if (!(qc->flags & ATA_QCFLAG_FAILED) ||
@@ -2477,7 +2484,7 @@ static void ata_eh_link_report(struct ata_link *link)
 		  ehc->i.serror & SERR_DEV_XCHG ? "DevExch " : "");
 #endif
 
-	for (tag = 0; tag < ATA_MAX_QUEUE; tag++) {
+	for (tag = 0; tag < max_queue; tag++) {
 		struct ata_queued_cmd *qc = __ata_qc_from_tag(ap, tag);
 		struct ata_taskfile *cmd = &qc->tf, *res = &qc->result_tf;
 		const u8 *cdb = qc->cdb;
@@ -3930,9 +3937,10 @@ int ata_eh_recover(struct ata_port *ap, ata_prereset_fn_t prereset,
 void ata_eh_finish(struct ata_port *ap)
 {
 	int tag;
+	unsigned int max_queue = ap->host->queue_depth;
 
 	/* retry or finish qcs */
-	for (tag = 0; tag < ATA_MAX_QUEUE; tag++) {
+	for (tag = 0; tag < max_queue; tag++) {
 		struct ata_queued_cmd *qc = __ata_qc_from_tag(ap, tag);
 
 		if (!(qc->flags & ATA_QCFLAG_FAILED))
diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index 0586f66d70fa..76a65b652389 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -1161,9 +1161,10 @@ static int ata_scsi_dev_config(struct scsi_device *sdev,
 
 	if (dev->flags & ATA_DFLAG_NCQ) {
 		int depth;
+		struct ata_port *ap = dev->link->ap;
 
 		depth = min(sdev->host->can_queue, ata_id_queue_depth(dev->id));
-		depth = min(ATA_MAX_QUEUE - 1, depth);
+		depth = min((int)ap->host->queue_depth - 1, depth);
 		scsi_adjust_queue_depth(sdev, MSG_SIMPLE_TAG, depth);
 	}
 
@@ -1277,7 +1278,7 @@ int __ata_change_queue_depth(struct ata_port *ap, struct scsi_device *sdev,
 	/* limit and apply queue depth */
 	queue_depth = min(queue_depth, sdev->host->can_queue);
 	queue_depth = min(queue_depth, ata_id_queue_depth(dev->id));
-	queue_depth = min(queue_depth, ATA_MAX_QUEUE - 1);
+	queue_depth = min(queue_depth, (int)ap->host->queue_depth - 1);
 
 	if (sdev->queue_depth == queue_depth)
 		return -EINVAL;
diff --git a/include/linux/libata.h b/include/linux/libata.h
index 74976a08c9bc..31e2090b7c9a 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -126,7 +126,6 @@ enum {
 	ATA_DEF_QUEUE		= 1,
 	/* tag ATA_MAX_QUEUE - 1 is reserved for internal commands */
 	ATA_MAX_QUEUE		= 32,
-	ATA_TAG_INTERNAL	= ATA_MAX_QUEUE - 1,
 	ATA_SHORT_PAUSE		= 16,
 
 	ATAPI_MAX_DRAIN		= 16 << 10,
@@ -593,6 +592,7 @@ struct ata_host {
 	struct device 		*dev;
 	void __iomem * const	*iomap;
 	unsigned int		n_ports;
+	unsigned int		queue_depth;
 	void			*private_data;
 	struct ata_port_operations *ops;
 	unsigned long		flags;
@@ -1104,6 +1104,8 @@ extern int sata_std_hardreset(struct ata_link *link, unsigned int *class,
 			      unsigned long deadline);
 extern void ata_std_postreset(struct ata_link *link, unsigned int *classes);
 
+extern int ata_host_set_queue_depth(struct ata_host *host,
+					unsigned int queue_depth);
 extern struct ata_host *ata_host_alloc(struct device *dev, int max_ports);
 extern struct ata_host *ata_host_alloc_pinfo(struct device *dev,
 			const struct ata_port_info * const * ppi, int n_ports);
@@ -1476,18 +1478,18 @@ extern void ata_port_pbar_desc(struct ata_port *ap, int bar, ssize_t offset,
 
 static inline unsigned int ata_tag_valid(struct ata_port *ap, unsigned int tag)
 {
-	return (tag < ATA_MAX_QUEUE) ? 1 : 0;
+	return (tag < ap->host->queue_depth) ? 1 : 0;
 }
 
 static inline unsigned int ata_get_internal_tag(struct ata_port *ap)
 {
-	return ATA_TAG_INTERNAL;
+	return ap->host->queue_depth - 1;
 }
 
 static inline unsigned int ata_tag_internal(struct ata_port *ap,
 						unsigned int tag)
 {
-	return tag == ATA_TAG_INTERNAL;
+	return tag == (ap->host->queue_depth - 1);
 }
 
 /*
-- 
1.9.3

Re: [PATCH 4/5] libata: support the ata host which implements a queue depth less than 32

[Tejun Heo]

On Thu, Jul 03, 2014 at 08:09:41PM +0800, Kevin Hao wrote:
> The sata on fsl mpc8315e is broken after the commit 8a4aeec8d2d6
> ("libata/ahci: accommodate tag ordered controllers"). The reason is
> that the ata controller on this SoC only implement a queue depth of
> 16. When issuing the commands in tag order, all the commands in tag
> 16 ~ 17 are mapped to tag 0 unconditionally and then causes the sata
> malfunction. It makes no senses to use a 32 queue in software while
> the hardware has less queue depth. This patch provides the function
> for libata to adjust the queue depth for a host controller.

ATA_TAG_INTERNAL is an internal way of marking an EH command and
doesn't actually get used as a command tag.  After all, EH doesn't
issue any tagged command.  We can use tag 32 for it but have been
using 31 for it for historical reasons, so the only necessary part is
updating the tag allocator to wrap according to the max number of tags
supported by the controller. 

Thanks.

-- 
tejun

Re: [PATCH 4/5] libata: support the ata host which implements a queue depth less than 32

[Kevin Hao]

[-- Attachment #1: Type: text/plain, Size: 2199 bytes --]

On Thu, Jul 03, 2014 at 09:47:17AM -0400, Tejun Heo wrote:
> On Thu, Jul 03, 2014 at 08:09:41PM +0800, Kevin Hao wrote:
> > The sata on fsl mpc8315e is broken after the commit 8a4aeec8d2d6
> > ("libata/ahci: accommodate tag ordered controllers"). The reason is
> > that the ata controller on this SoC only implement a queue depth of
> > 16. When issuing the commands in tag order, all the commands in tag
> > 16 ~ 17 are mapped to tag 0 unconditionally and then causes the sata
> > malfunction. It makes no senses to use a 32 queue in software while
> > the hardware has less queue depth. This patch provides the function
> > for libata to adjust the queue depth for a host controller.
> 
> ATA_TAG_INTERNAL is an internal way of marking an EH command and
> doesn't actually get used as a command tag.  After all, EH doesn't
> issue any tagged command.  We can use tag 32 for it but have been
> using 31 for it for historical reasons, so the only necessary part is
> updating the tag allocator to wrap according to the max number of tags
> supported by the controller. 

Sorry I didn't quite follow. In some cases, we still need to iterate all
the commands in the queue (including the internal command). For example,
in drivers/ata/sata_fsl.c:
    static void sata_fsl_host_intr(struct ata_port *ap)
        	/* Workaround for data length mismatch errata */
        	if (unlikely(hstatus & INT_ON_DATA_LENGTH_MISMATCH)) {
        		for (tag = 0; tag < ATA_MAX_QUEUE; tag++) {

Since the fsl sata only support a 16 command queue, if we put the internal
command to ata_port->qcmd[16 - 1], we can easily change the above code to:
    static void sata_fsl_host_intr(struct ata_port *ap)
        	/* Workaround for data length mismatch errata */
        	if (unlikely(hstatus & INT_ON_DATA_LENGTH_MISMATCH)) {
        		for (tag = 0; tag < 16; tag++) {

But if we still put the internal command to ata_port->qcmd[31], it will
make it more complicated to iterate all the commands.

There are also several iterations of the command queue just like the above
codes in drivers/ata/libata-eh.c. They should suffer the same issue.
Did I miss something?

Thanks,
Kevin

[-- Attachment #2: Type: application/pgp-signature, Size: 490 bytes --]

Re: [PATCH 4/5] libata: support the ata host which implements a queue depth less than 32

[Tejun Heo]

Hello, Kevin.

On Fri, Jul 04, 2014 at 02:46:46PM +0800, Kevin Hao wrote:
> Since the fsl sata only support a 16 command queue, if we put the internal
> command to ata_port->qcmd[16 - 1], we can easily change the above code to:

Hmmm... something that I wanna do is using all 32 tags for regular
commands and moving ATA_TAG_INTERNAL to something out-of-bounds, say
32 or INT_MAX and let each driver use whatever command slot (most
likely zero) to actually execute internal commands.

There really is no reason to permanently reserve a command tag for
internal commands like we do now; however, it could be that using a
tag number to distinguish an internal command isn't a very good idea
to begin with and it could be better to use a qcflag instead.

Anyways, sata_fsl already has workaround for ATA_TAG_INTERNAL, right?
Given that the situation around internal tag handling may change, I'm
not sure about changing the interface now.  Is it a lot of cruft on
fsl side?

Thanks.

-- 
tejun

Re: [PATCH 4/5] libata: support the ata host which implements a queue depth less than 32

[Kevin Hao]

[-- Attachment #1: Type: text/plain, Size: 6883 bytes --]

On Fri, Jul 04, 2014 at 11:30:22AM -0400, Tejun Heo wrote:
> Hello, Kevin.
> 
> On Fri, Jul 04, 2014 at 02:46:46PM +0800, Kevin Hao wrote:
> > Since the fsl sata only support a 16 command queue, if we put the internal
> > command to ata_port->qcmd[16 - 1], we can easily change the above code to:
> 
> Hmmm... something that I wanna do is using all 32 tags for regular
> commands and moving ATA_TAG_INTERNAL to something out-of-bounds, say
> 32 or INT_MAX and let each driver use whatever command slot (most
> likely zero) to actually execute internal commands.
> 
> There really is no reason to permanently reserve a command tag for
> internal commands like we do now; however, it could be that using a
> tag number to distinguish an internal command isn't a very good idea
> to begin with and it could be better to use a qcflag instead.

I prefer to use a qcflag. How about the following codes? If it is acceptable,
I can rebase my patch on it.

diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index 2895254d8388..f3c876ca26e1 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -749,10 +749,12 @@ int ata_build_rw_tf(struct ata_taskfile *tf, struct ata_device *dev,
 		    u64 block, u32 n_block, unsigned int tf_flags,
 		    unsigned int tag)
 {
+	struct ata_queued_cmd *qc = __ata_qc_from_tag(dev->link->ap, tag);
+
 	tf->flags |= ATA_TFLAG_ISADDR | ATA_TFLAG_DEVICE;
 	tf->flags |= tf_flags;
 
-	if (ata_ncq_enabled(dev) && likely(!ata_tag_internal(tag))) {
+	if (ata_ncq_enabled(dev) && likely(!ata_tag_internal(qc))) {
 		/* yay, NCQ */
 		if (!lba_48_ok(block, n_block))
 			return -ERANGE;
@@ -1570,15 +1572,8 @@ unsigned ata_exec_internal_sg(struct ata_device *dev,
 
 	/* initialize internal qc */
 
-	/* XXX: Tag 0 is used for drivers with legacy EH as some
-	 * drivers choke if any other tag is given.  This breaks
-	 * ata_tag_internal() test for those drivers.  Don't use new
-	 * EH stuff without converting to it.
-	 */
-	if (ap->ops->error_handler)
-		tag = ATA_TAG_INTERNAL;
-	else
-		tag = 0;
+	/* Tag 0 is used as some drivers choke if any other tag is given. */
+	tag = 0;
 
 	if (test_and_set_bit(tag, &ap->qc_allocated))
 		BUG();
@@ -1609,7 +1604,7 @@ unsigned ata_exec_internal_sg(struct ata_device *dev,
 	    dma_dir == DMA_FROM_DEVICE)
 		qc->tf.feature |= ATAPI_DMADIR;
 
-	qc->flags |= ATA_QCFLAG_RESULT_TF;
+	qc->flags |= ATA_QCFLAG_RESULT_TF | ATA_QCFLAG_INTERNAL;
 	qc->dma_dir = dma_dir;
 	if (dma_dir != DMA_NONE) {
 		unsigned int i, buflen = 0;
@@ -4737,10 +4732,6 @@ static struct ata_queued_cmd *ata_qc_new(struct ata_port *ap)
 	for (i = 0; i < ATA_MAX_QUEUE; i++) {
 		tag = (i + ap->last_tag + 1) % ATA_MAX_QUEUE;
 
-		/* the last tag is reserved for internal command. */
-		if (ata_tag_internal(tag))
-			continue;
-
 		if (!test_and_set_bit(tag, &ap->qc_allocated)) {
 			qc = __ata_qc_from_tag(ap, tag);
 			qc->tag = tag;
@@ -4906,7 +4897,7 @@ void ata_qc_complete(struct ata_queued_cmd *qc)
 		 * Finish internal commands without any further processing
 		 * and always with the result TF filled.
 		 */
-		if (unlikely(ata_tag_internal(qc->tag))) {
+		if (unlikely(ata_tag_internal(qc))) {
 			fill_result_tf(qc);
 			__ata_qc_complete(qc);
 			return;
diff --git a/drivers/ata/sata_fsl.c b/drivers/ata/sata_fsl.c
index 07bc7e4dbd04..b26e15c91905 100644
--- a/drivers/ata/sata_fsl.c
+++ b/drivers/ata/sata_fsl.c
@@ -395,10 +395,6 @@ static inline unsigned int sata_fsl_tag(unsigned int tag,
 	/* We let libATA core do actual (queue) tag allocation */
 
 	/* all non NCQ/queued commands should have tag#0 */
-	if (ata_tag_internal(tag)) {
-		DPRINTK("mapping internal cmds to tag#0\n");
-		return 0;
-	}
 
 	if (unlikely(tag >= SATA_FSL_QUEUE_DEPTH)) {
 		DPRINTK("tag %d invalid : out of range\n", tag);
diff --git a/drivers/ata/sata_sil24.c b/drivers/ata/sata_sil24.c
index 0534890f118a..755459c62641 100644
--- a/drivers/ata/sata_sil24.c
+++ b/drivers/ata/sata_sil24.c
@@ -458,13 +458,6 @@ static const struct ata_port_info sil24_port_info[] = {
 	},
 };
 
-static int sil24_tag(int tag)
-{
-	if (unlikely(ata_tag_internal(tag)))
-		return 0;
-	return tag;
-}
-
 static unsigned long sil24_port_offset(struct ata_port *ap)
 {
 	return ap->port_no * PORT_REGS_SIZE;
@@ -491,7 +484,7 @@ static void sil24_read_tf(struct ata_port *ap, int tag, struct ata_taskfile *tf)
 	struct sil24_prb __iomem *prb;
 	u8 fis[6 * 4];
 
-	prb = port + PORT_LRAM + sil24_tag(tag) * PORT_LRAM_SLOT_SZ;
+	prb = port + PORT_LRAM + tag * PORT_LRAM_SLOT_SZ;
 	memcpy_fromio(fis, prb->fis, sizeof(fis));
 	ata_tf_from_fis(fis, tf);
 }
@@ -848,7 +841,7 @@ static void sil24_qc_prep(struct ata_queued_cmd *qc)
 	struct sil24_sge *sge;
 	u16 ctrl = 0;
 
-	cb = &pp->cmd_block[sil24_tag(qc->tag)];
+	cb = &pp->cmd_block[qc->tag];
 
 	if (!ata_is_atapi(qc->tf.protocol)) {
 		prb = &cb->ata.prb;
@@ -890,7 +883,7 @@ static unsigned int sil24_qc_issue(struct ata_queued_cmd *qc)
 	struct ata_port *ap = qc->ap;
 	struct sil24_port_priv *pp = ap->private_data;
 	void __iomem *port = sil24_port_base(ap);
-	unsigned int tag = sil24_tag(qc->tag);
+	unsigned int tag = qc->tag;
 	dma_addr_t paddr;
 	void __iomem *activate;
 
diff --git a/include/linux/libata.h b/include/linux/libata.h
index 5ab4e3a76721..74612875b198 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -265,6 +265,7 @@ enum {
 	ATA_QCFLAG_FAILED	= (1 << 16), /* cmd failed and is owned by EH */
 	ATA_QCFLAG_SENSE_VALID	= (1 << 17), /* sense data valid */
 	ATA_QCFLAG_EH_SCHEDULED = (1 << 18), /* EH scheduled (obsolete) */
+	ATA_QCFLAG_INTERNAL	= (1 << 19), /* Internal command */
 
 	/* host set flags */
 	ATA_HOST_SIMPLEX	= (1 << 0),	/* Host is simplex, one DMA channel per host only */
@@ -1479,11 +1480,6 @@ static inline unsigned int ata_tag_valid(unsigned int tag)
 	return (tag < ATA_MAX_QUEUE) ? 1 : 0;
 }
 
-static inline unsigned int ata_tag_internal(unsigned int tag)
-{
-	return tag == ATA_TAG_INTERNAL;
-}
-
 /*
  * device helpers
  */
@@ -1642,6 +1638,11 @@ static inline struct ata_queued_cmd *ata_qc_from_tag(struct ata_port *ap,
 	return NULL;
 }
 
+static inline unsigned int ata_tag_internal(struct ata_queued_cmd *qc)
+{
+	return (qc->flags & ATA_QCFLAG_INTERNAL);
+}
+
 static inline unsigned int ata_qc_raw_nbytes(struct ata_queued_cmd *qc)
 {
 	return qc->nbytes - min(qc->extrabytes, qc->nbytes);
> 
> Anyways, sata_fsl already has workaround for ATA_TAG_INTERNAL, right?

Yes, the internal command is mapped to tag 0 in sata_fsl.

> Given that the situation around internal tag handling may change, I'm
> not sure about changing the interface now.  Is it a lot of cruft on
> fsl side?

Yes, I agree.

Thanks,
Kevin

[-- Attachment #2: Type: application/pgp-signature, Size: 490 bytes --]

Re: [PATCH 4/5] libata: support the ata host which implements a queue depth less than 32

[Tejun Heo]

Hello, Kevin.

On Sat, Jul 05, 2014 at 09:21:34PM +0800, Kevin Hao wrote:
> @@ -1570,15 +1572,8 @@ unsigned ata_exec_internal_sg(struct ata_device *dev,
...
> +	/* Tag 0 is used as some drivers choke if any other tag is given. */
> +	tag = 0;

If we switch to using a qcflag for marking internal commands, it
probably would be better to guarantee that EH commands always use tag
0.

> diff --git a/include/linux/libata.h b/include/linux/libata.h
> index 5ab4e3a76721..74612875b198 100644
> --- a/include/linux/libata.h
> +++ b/include/linux/libata.h
> @@ -265,6 +265,7 @@ enum {
>  	ATA_QCFLAG_FAILED	= (1 << 16), /* cmd failed and is owned by EH */
>  	ATA_QCFLAG_SENSE_VALID	= (1 << 17), /* sense data valid */
>  	ATA_QCFLAG_EH_SCHEDULED = (1 << 18), /* EH scheduled (obsolete) */
> +	ATA_QCFLAG_INTERNAL	= (1 << 19), /* Internal command */

Let's make it more explicit - ATA_QCFLAG_EH.

> +static inline unsigned int ata_tag_internal(struct ata_queued_cmd *qc)
> +{
> +	return (qc->flags & ATA_QCFLAG_INTERNAL);
> +}

As we're changing the interface anyway, ata_qc_eh()?

> > Anyways, sata_fsl already has workaround for ATA_TAG_INTERNAL, right?
> 
> Yes, the internal command is mapped to tag 0 in sata_fsl.
> 
> > Given that the situation around internal tag handling may change, I'm
> > not sure about changing the interface now.  Is it a lot of cruft on
> > fsl side?
> 
> Yes, I agree.

So, yeah, I like the proposed changes.  This is a bit too much as a
fix for fsl tho.  First fix fsl and then update so that we use the
qcflag in the devel branch?

Thanks.

-- 
tejun

Re: [PATCH 4/5] libata: support the ata host which implements a queue depth less than 32

[Kevin Hao]

[-- Attachment #1: Type: text/plain, Size: 857 bytes --]

On Sat, Jul 05, 2014 at 04:00:55PM -0400, Tejun Heo wrote:
> If we switch to using a qcflag for marking internal commands, it
> probably would be better to guarantee that EH commands always use tag
> 0.

Do you mean we add a WARN_ON_ONCE() to check the tag in ata_tag_internal()?

> > +	ATA_QCFLAG_INTERNAL	= (1 << 19), /* Internal command */
> 
> Let's make it more explicit - ATA_QCFLAG_EH.

Sure.

> 
> > +static inline unsigned int ata_tag_internal(struct ata_queued_cmd *qc)
> > +{
> > +	return (qc->flags & ATA_QCFLAG_INTERNAL);
> > +}
> 
> As we're changing the interface anyway, ata_qc_eh()?

Sounds good, will change.

> So, yeah, I like the proposed changes.  This is a bit too much as a
> fix for fsl tho.  First fix fsl and then update so that we use the
> qcflag in the devel branch?

OK, will do.

Thanks,
Kevin

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH 4/5] libata: support the ata host which implements a queue depth less than 32

[Tejun Heo]

On Thu, Jul 03, 2014 at 08:09:41PM +0800, Kevin Hao wrote:
> The sata on fsl mpc8315e is broken after the commit 8a4aeec8d2d6
> ("libata/ahci: accommodate tag ordered controllers"). The reason is
> that the ata controller on this SoC only implement a queue depth of
> 16. When issuing the commands in tag order, all the commands in tag
> 16 ~ 17 are mapped to tag 0 unconditionally and then causes the sata

Shouldn't the function at least trigger WARN_ON_ONCE() if it's forcing
the tag to be 0 when it shouldn't be?  Silently mapping things to zero
doesn't look like a particularly good idea.

Thanks.

-- 
tejun

Re: [PATCH 4/5] libata: support the ata host which implements a queue depth less than 32

[Kevin Hao]

[-- Attachment #1: Type: text/plain, Size: 765 bytes --]

On Thu, Jul 03, 2014 at 09:50:58AM -0400, Tejun Heo wrote:
> On Thu, Jul 03, 2014 at 08:09:41PM +0800, Kevin Hao wrote:
> > The sata on fsl mpc8315e is broken after the commit 8a4aeec8d2d6
> > ("libata/ahci: accommodate tag ordered controllers"). The reason is
> > that the ata controller on this SoC only implement a queue depth of
> > 16. When issuing the commands in tag order, all the commands in tag
> > 16 ~ 17 are mapped to tag 0 unconditionally and then causes the sata
> 
> Shouldn't the function at least trigger WARN_ON_ONCE() if it's forcing
> the tag to be 0 when it shouldn't be?  Silently mapping things to zero
> doesn't look like a particularly good idea.

Indeed. I will add a WARN_ON_ONCE() in the fsl sata driver.

Thanks,
Kevin

[-- Attachment #2: Type: application/pgp-signature, Size: 490 bytes --]

[PATCH 5/5] sata_fsl: set the correct queue depth for the host controller

[Kevin Hao]

The fsl sata controller only implement a queue depth of 16. So use
the ata_host_set_queue_depth() to set the correct queue depth for
host controller.

Signed-off-by: Kevin Hao <haokexin@gmail.com>
---
 drivers/ata/sata_fsl.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/ata/sata_fsl.c b/drivers/ata/sata_fsl.c
index bffdd96538ff..7174998cf1bc 100644
--- a/drivers/ata/sata_fsl.c
+++ b/drivers/ata/sata_fsl.c
@@ -1219,6 +1219,7 @@ static void sata_fsl_host_intr(struct ata_port *ap)
 	u32 SError;
 	u32 tag;
 	u32 status_mask = INT_ON_ERROR;
+	unsigned int max_queue = ap->host->queue_depth;
 
 	hstatus = ioread32(hcr_base + HSTATUS);
 
@@ -1229,7 +1230,7 @@ static void sata_fsl_host_intr(struct ata_port *ap)
 
 	/* Workaround for data length mismatch errata */
 	if (unlikely(hstatus & INT_ON_DATA_LENGTH_MISMATCH)) {
-		for (tag = 0; tag < ATA_MAX_QUEUE; tag++) {
+		for (tag = 0; tag < max_queue; tag++) {
 			qc = ata_qc_from_tag(ap, tag);
 			if (qc && ata_is_atapi(qc->tf.protocol)) {
 				u32 hcontrol;
@@ -1509,6 +1510,8 @@ static int sata_fsl_probe(struct platform_device *ofdev)
 		goto error_exit_with_cleanup;
 	}
 
+	ata_host_set_queue_depth(host, SATA_FSL_QUEUE_DEPTH);
+
 	/* host->iomap is not used currently */
 	host->private_data = host_priv;
 
-- 
1.9.3