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

[PATCH v3] 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 ~ 31 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. So consider the queue depth
implemented by the hardware when requesting a command tag.

Fixes: 8a4aeec8d2d6 ("libata/ahci: accommodate tag ordered controllers")
Cc: stable@vger.kernel.org
Signed-off-by: Kevin Hao <haokexin@gmail.com>
---
v3: Use ap->scsi_host->can_queue for the queue depth implemented by hardware.
    Patch 2 in v2 is also dropped due to this change.

v2: Remove the changes for the ata tag helper functions

Hi Tejun,

I didn't get explicit objection for the codes at http://marc.info/?l=linux-ide&m=140478830920334&w=2
So I assume that you are OK wit it. The code in this patch is the same as that,
just add the commit log.

 drivers/ata/libata-core.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

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)
-- 
1.9.3

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

[Sergei Shtylyov]

Hello.

On 11-07-2014 10:10, 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 ~ 31 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. So consider the queue depth
> implemented by the hardware when requesting a command tag.

> Fixes: 8a4aeec8d2d6 ("libata/ahci: accommodate tag ordered controllers")
> Cc: stable@vger.kernel.org
> Signed-off-by: Kevin Hao <haokexin@gmail.com>
> ---
> v3: Use ap->scsi_host->can_queue for the queue depth implemented by hardware.
>      Patch 2 in v2 is also dropped due to this change.

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

> Hi Tejun,

> I didn't get explicit objection for the codes at http://marc.info/?l=linux-ide&m=140478830920334&w=2
> So I assume that you are OK wit it. The code in this patch is the same as that,
> just add the commit log.

>   drivers/ata/libata-core.c | 9 ++++++---
>   1 file changed, 6 insertions(+), 3 deletions(-)

> 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;

    Assigning 'tag' back to 'tag' is quite stupid, don't you think? Why not:

		if (tag >= max_queue)
			tag = 0;

MBR, Sergei

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

[Tejun Heo]

On Fri, Jul 11, 2014 at 03:57:01PM +0400, Sergei Shtylyov wrote:
> >-	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;
> 
>    Assigning 'tag' back to 'tag' is quite stupid, don't you think? Why not:
> 
> 		if (tag >= max_queue)
> 			tag = 0;

I don't really mind it and the explicit if block isn't more readable
in any way.

Thanks.

-- 
tejun

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

[Tejun Heo]

On Fri, Jul 11, 2014 at 02:10:26PM +0800, Kevin Hao wrote:
>  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);

Let's move the WARN_ON_ONCE() to ata_host_register().  Also, let's
please update the function comment explaining why we're allocating
tags the way we do.

Other than that, it looks good to me.

Thanks.

-- 
tejun

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

[Bartlomiej Zolnierkiewicz]

Hi,

On Friday, July 11, 2014 02:10:26 PM 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 ~ 31 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. So consider the queue depth
> implemented by the hardware when requesting a command tag.
> 
> Fixes: 8a4aeec8d2d6 ("libata/ahci: accommodate tag ordered controllers")
> Cc: stable@vger.kernel.org
> Signed-off-by: Kevin Hao <haokexin@gmail.com>
> ---
> v3: Use ap->scsi_host->can_queue for the queue depth implemented by hardware.
>     Patch 2 in v2 is also dropped due to this change.
> 
> v2: Remove the changes for the ata tag helper functions
> 
> Hi Tejun,
> 
> I didn't get explicit objection for the codes at http://marc.info/?l=linux-ide&m=140478830920334&w=2
> So I assume that you are OK wit it. The code in this patch is the same as that,
> just add the commit log.
> 
>  drivers/ata/libata-core.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> 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);

Why not handle this properly by just doing what ata_dev_config_ncq() does and
using

	min(ap->scsi_host->can_queue, ATA_MAX_QUEUE - 1);

for obtaining 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)

Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics

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

[Bartlomiej Zolnierkiewicz]

On Friday, July 11, 2014 06:28:33 PM Bartlomiej Zolnierkiewicz wrote:
> 
> Hi,
> 
> On Friday, July 11, 2014 02:10:26 PM 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 ~ 31 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. So consider the queue depth
> > implemented by the hardware when requesting a command tag.
> > 
> > Fixes: 8a4aeec8d2d6 ("libata/ahci: accommodate tag ordered controllers")
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Kevin Hao <haokexin@gmail.com>
> > ---
> > v3: Use ap->scsi_host->can_queue for the queue depth implemented by hardware.
> >     Patch 2 in v2 is also dropped due to this change.
> > 
> > v2: Remove the changes for the ata tag helper functions
> > 
> > Hi Tejun,
> > 
> > I didn't get explicit objection for the codes at http://marc.info/?l=linux-ide&m=140478830920334&w=2
> > So I assume that you are OK wit it. The code in this patch is the same as that,
> > just add the commit log.
> > 
> >  drivers/ata/libata-core.c | 9 ++++++---
> >  1 file changed, 6 insertions(+), 3 deletions(-)
> > 
> > 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);
> 
> Why not handle this properly by just doing what ata_dev_config_ncq() does and
> using
> 
> 	min(ap->scsi_host->can_queue, ATA_MAX_QUEUE - 1);

actually this should be

	min(ap->scsi_host->can_queue, ATA_MAX_QUEUE);

in this function

> for obtaining 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)

Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics

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

[Kevin Hao]

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

On Fri, Jul 11, 2014 at 03:57:01PM +0400, Sergei Shtylyov wrote:
> >-	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;
> 
>    Assigning 'tag' back to 'tag' is quite stupid, don't you think? Why not:
> 
> 		if (tag >= max_queue)
> 			tag = 0;

Since I am an idiot, it is very possible for me to write stupid code.
But I don't believe this is one. I don't think the above code is more readable.

Thanks,
Kevin

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

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

[Kevin Hao]

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

On Fri, Jul 11, 2014 at 10:51:05AM -0400, Tejun Heo wrote:
> On Fri, Jul 11, 2014 at 02:10:26PM +0800, Kevin Hao wrote:
> >  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);
> 
> Let's move the WARN_ON_ONCE() to ata_host_register().  Also, let's
> please update the function comment explaining why we're allocating
> tags the way we do.

OK, will do.

Thanks,
Kevin

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

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

[Sergei Shtylyov]

On 07/12/2014 04:26 AM, Kevin Hao wrote:

>>> -	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;

>>     Assigning 'tag' back to 'tag' is quite stupid, don't you think? Why not:

>> 		if (tag >= max_queue)
>> 			tag = 0;

> Since I am an idiot, it is very possible for me to write stupid code.
> But I don't believe this is one. I don't think the above code is more readable.

    Perhaps I'm an idiot (I even seldom doubt it) but to me it looks like 
clear abuse of the ?: operator. But that's probably just me...

> Thanks,
> Kevin

MBR, Sergei

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

[Kevin Hao]

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

On Fri, Jul 11, 2014 at 06:33:46PM +0200, Bartlomiej Zolnierkiewicz wrote:
> On Friday, July 11, 2014 06:28:33 PM Bartlomiej Zolnierkiewicz wrote:
> > > +	max_queue = ap->scsi_host->can_queue;
> > > +	WARN_ON_ONCE(max_queue > ATA_MAX_QUEUE);
> > 
> > Why not handle this properly by just doing what ata_dev_config_ncq() does and
> > using
> > 
> > 	min(ap->scsi_host->can_queue, ATA_MAX_QUEUE - 1);
> 
> actually this should be
> 
> 	min(ap->scsi_host->can_queue, ATA_MAX_QUEUE);
> 
> in this function
> 
> > for obtaining max_queue?

Actually the ap->scsi_host->can_queue is the max queue supported by the
hardware. It should be a bug if it is greater than ATA_MAX_QUEUE. So we don't
need to use min() to get the max queue here. As pointed out by Tejun, it makes
no sense to do the sanity check for each qc requesting, so I will move the
WARN_ON_ONCE() to ata_host_register().

I have a pending patch to free the command tag reserved by the internal
command. In that patch the above code in ata_dev_config_ncq() will change
to:

  hdepth = ap->scsi_host->can_queue;

Thanks,
Kevin

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