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

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

[Kevin Hao]

Hi,

v2: Remove the changes for the ata tag helper functions.

v1:
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 (2):
  libata: introduce ata_host_set_queue_depth()
  sata_fsl: set the correct queue depth for the host controller

 drivers/ata/libata-core.c | 29 ++++++++++++++++++++++++++---
 drivers/ata/sata_fsl.c    |  3 +++
 include/linux/libata.h    |  3 +++
 3 files changed, 32 insertions(+), 3 deletions(-)

-- 
1.9.3

[PATCH v2 1/2] libata: introduce ata_host_set_queue_depth()

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

Fixes: 8a4aeec8d2d6 ("libata/ahci: accommodate tag ordered controllers")
Cc: stable@vger.kernel.org
Signed-off-by: Kevin Hao <haokexin@gmail.com>
---
v2: Remove the changes for the ata tag helper functions.

 drivers/ata/libata-core.c | 29 ++++++++++++++++++++++++++---
 include/linux/libata.h    |  3 +++
 2 files changed, 29 insertions(+), 3 deletions(-)

diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index 8f3043165048..c9c51646d8e8 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -4728,14 +4728,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 (tag == ATA_TAG_INTERNAL)
@@ -5687,6 +5687,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
@@ -5731,6 +5752,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++) {
@@ -6855,6 +6877,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/include/linux/libata.h b/include/linux/libata.h
index 5ab4e3a76721..3d912845f7df 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -593,6 +593,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 +1105,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);
-- 
1.9.3

Re: [PATCH v2 1/2] libata: introduce ata_host_set_queue_depth()

[Tejun Heo]

Hello,

On Sun, Jul 06, 2014 at 02:28:40PM +0800, Kevin Hao wrote:
> +int ata_host_set_queue_depth(struct ata_host *host, unsigned int queue_depth)

Hmmm... is there a reason we're doing this separately when the same
information is available from scsi_host_template->can_queue from
ata_host_register()?

> @@ -5731,6 +5752,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;

And shouldn't this be one for !NCQ controllers?

Thanks.

-- 
tejun

Re: [PATCH v2 1/2] libata: introduce ata_host_set_queue_depth()

[Kevin Hao]

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

On Mon, Jul 07, 2014 at 09:49:45AM -0400, Tejun Heo wrote:
> Hello,
> 
> On Sun, Jul 06, 2014 at 02:28:40PM +0800, Kevin Hao wrote:
> > +int ata_host_set_queue_depth(struct ata_host *host, unsigned int queue_depth)
> 
> Hmmm... is there a reason we're doing this separately when the same
> information is available from scsi_host_template->can_queue from
> ata_host_register()?

This was my first thought. But given that scsi_host_template are the parameters
passed to the scsi layer, it seems a bit weird to set the ata_host based a
value in that. Anyway if we decide to use can_queue, how about directly use
it in ata_qc_new(), just like the following codes?

diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index 8f3043165048..4792fea79acf 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -4728,14 +4728,17 @@ 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;
+
+	max_queue = ap->scsi_host->can_queue;
+	WARN_ON_ONCE(max_queue > ATA_MAX_QUEUE);
 
 	/* 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, tag = ap->last_tag + 1; i < max_queue; i++, tag++) {
+		tag = tag < max_queue ? tag : 0;
 
 		/* the last tag is reserved for internal command. */
 		if (tag == ATA_TAG_INTERNAL)

Thanks,
Kevin

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

Re: [PATCH v2 1/2] libata: introduce ata_host_set_queue_depth()

[Dan Williams]

On Sat, Jul 5, 2014 at 11:28 PM, Kevin Hao <haokexin@gmail.com> 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.
>
> Fixes: 8a4aeec8d2d6 ("libata/ahci: accommodate tag ordered controllers")
> Cc: stable@vger.kernel.org
> Signed-off-by: Kevin Hao <haokexin@gmail.com>
> ---
> v2: Remove the changes for the ata tag helper functions.
>
>  drivers/ata/libata-core.c | 29 ++++++++++++++++++++++++++---
>  include/linux/libata.h    |  3 +++
>  2 files changed, 29 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
> index 8f3043165048..c9c51646d8e8 100644
> --- a/drivers/ata/libata-core.c
> +++ b/drivers/ata/libata-core.c
> @@ -4728,14 +4728,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;

This change may cause the compiler to do a full divide where
previously it could optimize to power-of-2 math with the visibility of
the ATA_MAX_QUEUE constant.  How about converting
ap->host->queue_depth to ap->host->queue_depth_mask?  ...then this
becomes:

    tag = (i + ap->last_tag + 1) & max_queue_mask

Re: [PATCH v2 1/2] libata: introduce ata_host_set_queue_depth()

[Tejun Heo]

On Mon, Jul 07, 2014 at 11:19:25AM -0700, Dan Williams wrote:
> This change may cause the compiler to do a full divide where
> previously it could optimize to power-of-2 math with the visibility of
> the ATA_MAX_QUEUE constant.  How about converting
> ap->host->queue_depth to ap->host->queue_depth_mask?  ...then this
> becomes:
> 
>     tag = (i + ap->last_tag + 1) & max_queue_mask

Given how inefficient the test_and_set bit is, I don't think a single
moduls matters.  Plus, we don't even need modulus there.  We can
simply use >= max_depth.

Thanks.

-- 
tejun

Re: [PATCH v2 1/2] libata: introduce ata_host_set_queue_depth()

[Dan Williams]

On Mon, Jul 7, 2014 at 1:06 PM, Tejun Heo <tj@kernel.org> wrote:
> On Mon, Jul 07, 2014 at 11:19:25AM -0700, Dan Williams wrote:
>> This change may cause the compiler to do a full divide where
>> previously it could optimize to power-of-2 math with the visibility of
>> the ATA_MAX_QUEUE constant.  How about converting
>> ap->host->queue_depth to ap->host->queue_depth_mask?  ...then this
>> becomes:
>>
>>     tag = (i + ap->last_tag + 1) & max_queue_mask
>
> Given how inefficient the test_and_set bit is, I don't think a single
> moduls matters.  Plus, we don't even need modulus there.  We can
> simply use >= max_depth.

True, sounds good.

Re: [PATCH v2 1/2] libata: introduce ata_host_set_queue_depth()

[Kevin Hao]

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

On Mon, Jul 07, 2014 at 01:56:59PM -0700, Dan Williams wrote:
> On Mon, Jul 7, 2014 at 1:06 PM, Tejun Heo <tj@kernel.org> wrote:
> > On Mon, Jul 07, 2014 at 11:19:25AM -0700, Dan Williams wrote:
> > Given how inefficient the test_and_set bit is, I don't think a single
> > moduls matters.  Plus, we don't even need modulus there.  We can
> > simply use >= max_depth.
> 
> True, sounds good.

OK, will change.

Thanks,
Kevin

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

[PATCH v2 2/2] 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. Also add a WARN_ON_ONCE() if a tag above 16 is
used.

Fixes: 8a4aeec8d2d6 ("libata/ahci: accommodate tag ordered controllers")
Cc: stable@vger.kernel.org
Signed-off-by: Kevin Hao <haokexin@gmail.com>
---
v2: Add a WARN_ON_ONCE().

 drivers/ata/sata_fsl.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/ata/sata_fsl.c b/drivers/ata/sata_fsl.c
index 07bc7e4dbd04..ae55440af86d 100644
--- a/drivers/ata/sata_fsl.c
+++ b/drivers/ata/sata_fsl.c
@@ -402,6 +402,7 @@ static inline unsigned int sata_fsl_tag(unsigned int tag,
 
 	if (unlikely(tag >= SATA_FSL_QUEUE_DEPTH)) {
 		DPRINTK("tag %d invalid : out of range\n", tag);
+		WARN_ON_ONCE(1);
 		return 0;
 	}
 
@@ -1506,6 +1507,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