[Regression] commit 8a4aeec8d(libata/ahci: accommodate tag ordered controllers)

[Regression] commit 8a4aeec8d(libata/ahci: accommodate tag ordered controllers)

[Ming Lei]

Hi Dan and Tejun,

Looks the commit 8a4aeec8d(libata/ahci: accommodate tag ordered
controllers) causes below sata failure[1] on APM AHCI controller.

And the error does disappear after reverting the commit.

Thanks,
--
Ming Lei

[1], failure log
[   72.001579] ata4.00: exception Emask 0x40 SAct 0xff00 SErr 0x800
action 0x6 frozen
[   72.009116] ata4: SError: { HostInt }
[   72.012763] ata4.00: failed command: READ FPDMA QUEUED
[   72.017878] ata4.00: cmd 60/08:40:e0:a4:88/00:00:04:00:00/40 tag 8
ncq 4096 in
[   72.017878]          res 40/00:00:00:00:00/00:00:00:00:00/00 Emask
0x44 (timeout)
[   72.032498] ata4.00: status: { DRDY }
[   72.036139] ata4.00: failed command: READ FPDMA QUEUED
[   72.041252] ata4.00: cmd 60/18:48:b8:3e:10/00:00:09:00:00/40 tag 9
ncq 12288 in
[   72.041252]          res 40/00:00:00:00:00/00:00:00:00:00/00 Emask
0x44 (timeout)
[   72.055956] ata4.00: status: { DRDY }
[   72.059598] ata4.00: failed command: READ FPDMA QUEUED
[   72.064713] ata4.00: cmd 60/08:50:00:be:07/01:00:01:00:00/40 tag 10
ncq 135168 in
[   72.064713]          res 40/00:00:00:00:00/00:00:00:00:00/00 Emask
0x44 (timeout)
[   72.079589] ata4.00: status: { DRDY }
[   72.083233] ata4.00: failed command: READ FPDMA QUEUED
[   72.088345] ata4.00: cmd 60/08:58:00:07:09/00:00:06:00:00/40 tag 11
ncq 4096 in
[   72.088345]          res 40/00:00:00:00:00/00:00:00:00:00/00 Emask
0x44 (timeout)
[   72.103049] ata4.00: status: { DRDY }
[   72.106691] ata4.00: failed command: READ FPDMA QUEUED
[   72.111805] ata4.00: cmd 60/08:60:00:4b:f9/00:00:0d:00:00/40 tag 12
ncq 4096 in
[   72.111805]          res 40/00:00:00:00:00/00:00:00:00:00/00 Emask
0x44 (timeout)
[   72.126509] ata4.00: status: { DRDY }
[   72.130151] ata4.00: failed command: READ FPDMA QUEUED
[   72.135265] ata4.00: cmd 60/20:68:28:f5:3a/00:00:09:00:00/40 tag 13
ncq 16384 in
[   72.135265]          res 40/00:00:00:00:00/00:00:00:00:00/00 Emask
0x44 (timeout)

Re: [Regression] commit 8a4aeec8d(libata/ahci: accommodate tag ordered controllers)

[Dan Williams]

On Thu, Jun 5, 2014 at 1:53 AM, Ming Lei <ming.lei@canonical.com> wrote:
> Hi Dan and Tejun,
>
> Looks the commit 8a4aeec8d(libata/ahci: accommodate tag ordered
> controllers) causes below sata failure[1] on APM AHCI controller.
>
> And the error does disappear after reverting the commit.

Can you give some more details about the workload and the disk?  What
is the APM AHCI?

Re: [Regression] commit 8a4aeec8d(libata/ahci: accommodate tag ordered controllers)

[Ming Lei]

On Thu, Jun 5, 2014 at 9:12 PM, Dan Williams <dan.j.williams@intel.com> wrote:
> On Thu, Jun 5, 2014 at 1:53 AM, Ming Lei <ming.lei@canonical.com> wrote:
>> Hi Dan and Tejun,
>>
>> Looks the commit 8a4aeec8d(libata/ahci: accommodate tag ordered
>> controllers) causes below sata failure[1] on APM AHCI controller.
>>
>> And the error does disappear after reverting the commit.
>
> Can you give some more details about the workload and the disk?  What
> is the APM AHCI?

The commit causes the APM arm64 based system not bootable, and not
special workload, just when mounting rootfs or reading init files.

APM AHCI:
      drivers/ata/ahci_xgene.c

Thanks,
--
Ming Lei

Re: [Regression] commit 8a4aeec8d(libata/ahci: accommodate tag ordered controllers)

[Tejun Heo]

Hello,

(cc'ing ahci_xgene folks)

On Thu, Jun 05, 2014 at 09:24:04PM +0800, Ming Lei wrote:
> On Thu, Jun 5, 2014 at 9:12 PM, Dan Williams <dan.j.williams@intel.com> wrote:
> > On Thu, Jun 5, 2014 at 1:53 AM, Ming Lei <ming.lei@canonical.com> wrote:
> >> Hi Dan and Tejun,
> >>
> >> Looks the commit 8a4aeec8d(libata/ahci: accommodate tag ordered
> >> controllers) causes below sata failure[1] on APM AHCI controller.
> >>
> >> And the error does disappear after reverting the commit.
> >
> > Can you give some more details about the workload and the disk?  What
> > is the APM AHCI?
> 
> The commit causes the APM arm64 based system not bootable, and not
> special workload, just when mounting rootfs or reading init files.
> 
> APM AHCI:
>       drivers/ata/ahci_xgene.c

Urgh... so, the controller can't handle tags allocated in round-robin
instead of lowest-first?  Ming, can you please test with different
controllers / disks / ssds and see whether the problem stays with the
controller?  Loc, can you guys please look into it so that at least
the next revision hardware can fix the issue?

Provided that the tag allocation itself isn't broken, which isn't too
likely given the lack of bug reports on other controllers, we'll need
to blacklist ahci_xgene somehow.  Either disable NCQ support on it or
implement an alternative lowest-first tag allocation for it.  If this
actually is the controller freaking out about tags allocated in a
different order, I'm not sure how much confidence we can put in its
NCQ implementation tho and it might be a better idea to simply plug
NCQ support on it until we understand the details of the problem.

So, something like the following?  Ming, can you please verify whether
the following makes the machine boot again?

Thanks.

diff --git a/drivers/ata/ahci_xgene.c b/drivers/ata/ahci_xgene.c
index 77c89bf..4950600 100644
--- a/drivers/ata/ahci_xgene.c
+++ b/drivers/ata/ahci_xgene.c
@@ -302,9 +302,16 @@ static struct ata_port_operations xgene_ahci_ops = {
 	.read_id = xgene_ahci_read_id,
 };
 
+/*
+ * The controller seems to freak out depending on the order of tag
+ * allocation.  NCQ support is disabled until the problem is better
+ * understood.
+ *
+ * http://lkml.kernel.org/g/CACVXFVMnM6w5ZLX=S8fJHeVoEqxH2JG3Wg=GPMeoVnOpAjsvAA@mail.gmail.com
+ */
 static const struct ata_port_info xgene_ahci_port_info = {
-	AHCI_HFLAGS(AHCI_HFLAG_NO_PMP | AHCI_HFLAG_YES_NCQ),
-	.flags = AHCI_FLAG_COMMON | ATA_FLAG_NCQ,
+	AHCI_HFLAGS(AHCI_HFLAG_NO_PMP),
+	.flags = AHCI_FLAG_COMMON,
 	.pio_mask = ATA_PIO4,
 	.udma_mask = ATA_UDMA6,
 	.port_ops = &xgene_ahci_ops,

Re: [Regression] commit 8a4aeec8d(libata/ahci: accommodate tag ordered controllers)

[Loc Ho]

Hi Tejun,

>> >> Looks the commit 8a4aeec8d(libata/ahci: accommodate tag ordered
>> >> controllers) causes below sata failure[1] on APM AHCI controller.
>> >>
>> >> And the error does disappear after reverting the commit.
>> >
>> > Can you give some more details about the workload and the disk?  What
>> > is the APM AHCI?
>>
>> The commit causes the APM arm64 based system not bootable, and not
>> special workload, just when mounting rootfs or reading init files.
>>
>> APM AHCI:
>>       drivers/ata/ahci_xgene.c
>
> Urgh... so, the controller can't handle tags allocated in round-robin
> instead of lowest-first?  Ming, can you please test with different
> controllers / disks / ssds and see whether the problem stays with the
> controller?  Loc, can you guys please look into it so that at least
> the next revision hardware can fix the issue?
>
> Provided that the tag allocation itself isn't broken, which isn't too
> likely given the lack of bug reports on other controllers, we'll need
> to blacklist ahci_xgene somehow.  Either disable NCQ support on it or
> implement an alternative lowest-first tag allocation for it.  If this
> actually is the controller freaking out about tags allocated in a
> different order, I'm not sure how much confidence we can put in its
> NCQ implementation tho and it might be a better idea to simply plug
> NCQ support on it until we understand the details of the problem.
>
> So, something like the following?  Ming, can you please verify whether
> the following makes the machine boot again?
>
> Thanks.
>
> diff --git a/drivers/ata/ahci_xgene.c b/drivers/ata/ahci_xgene.c
> index 77c89bf..4950600 100644
> --- a/drivers/ata/ahci_xgene.c
> +++ b/drivers/ata/ahci_xgene.c
> @@ -302,9 +302,16 @@ static struct ata_port_operations xgene_ahci_ops = {
>         .read_id = xgene_ahci_read_id,
>  };
>
> +/*
> + * The controller seems to freak out depending on the order of tag
> + * allocation.  NCQ support is disabled until the problem is better
> + * understood.
> + *
> + * http://lkml.kernel.org/g/CACVXFVMnM6w5ZLX=S8fJHeVoEqxH2JG3Wg=GPMeoVnOpAjsvAA@mail.gmail.com
> + */
>  static const struct ata_port_info xgene_ahci_port_info = {
> -       AHCI_HFLAGS(AHCI_HFLAG_NO_PMP | AHCI_HFLAG_YES_NCQ),
> -       .flags = AHCI_FLAG_COMMON | ATA_FLAG_NCQ,
> +       AHCI_HFLAGS(AHCI_HFLAG_NO_PMP),
> +       .flags = AHCI_FLAG_COMMON,
>         .pio_mask = ATA_PIO4,
>         .udma_mask = ATA_UDMA6,
>         .port_ops = &xgene_ahci_ops,

Suman is very familiar with this and he will proposal an solution.
Please be aware that this is only an issue with previous revision of
the chip. With the latest revision in house, this is not an issue.

-Loc

Re: [Regression] commit 8a4aeec8d(libata/ahci: accommodate tag ordered controllers)

[Ming Lei]

Hi Tejun,

On Thu, Jun 5, 2014 at 9:41 PM, Tejun Heo <tj@kernel.org> wrote:
> Hello,
>
> (cc'ing ahci_xgene folks)
>
> On Thu, Jun 05, 2014 at 09:24:04PM +0800, Ming Lei wrote:
>> On Thu, Jun 5, 2014 at 9:12 PM, Dan Williams <dan.j.williams@intel.com> wrote:
>> > On Thu, Jun 5, 2014 at 1:53 AM, Ming Lei <ming.lei@canonical.com> wrote:
>> >> Hi Dan and Tejun,
>> >>
>> >> Looks the commit 8a4aeec8d(libata/ahci: accommodate tag ordered
>> >> controllers) causes below sata failure[1] on APM AHCI controller.
>> >>
>> >> And the error does disappear after reverting the commit.
>> >
>> > Can you give some more details about the workload and the disk?  What
>> > is the APM AHCI?
>>
>> The commit causes the APM arm64 based system not bootable, and not
>> special workload, just when mounting rootfs or reading init files.
>>
>> APM AHCI:
>>       drivers/ata/ahci_xgene.c
>
> Urgh... so, the controller can't handle tags allocated in round-robin
> instead of lowest-first?  Ming, can you please test with different
> controllers / disks / ssds and see whether the problem stays with the
> controller?  Loc, can you guys please look into it so that at least
> the next revision hardware can fix the issue?

The problem has been observed on several apm arm64 boards
with different disks.

>
> Provided that the tag allocation itself isn't broken, which isn't too
> likely given the lack of bug reports on other controllers, we'll need
> to blacklist ahci_xgene somehow.  Either disable NCQ support on it or
> implement an alternative lowest-first tag allocation for it.  If this
> actually is the controller freaking out about tags allocated in a
> different order, I'm not sure how much confidence we can put in its
> NCQ implementation tho and it might be a better idea to simply plug
> NCQ support on it until we understand the details of the problem.
>
> So, something like the following?  Ming, can you please verify whether
> the following makes the machine boot again?

Looks the problem still persists after applying your patch of disabling
NCQ.

>
> Thanks.
>
> diff --git a/drivers/ata/ahci_xgene.c b/drivers/ata/ahci_xgene.c
> index 77c89bf..4950600 100644
> --- a/drivers/ata/ahci_xgene.c
> +++ b/drivers/ata/ahci_xgene.c
> @@ -302,9 +302,16 @@ static struct ata_port_operations xgene_ahci_ops = {
>         .read_id = xgene_ahci_read_id,
>  };
>
> +/*
> + * The controller seems to freak out depending on the order of tag
> + * allocation.  NCQ support is disabled until the problem is better
> + * understood.
> + *
> + * http://lkml.kernel.org/g/CACVXFVMnM6w5ZLX=S8fJHeVoEqxH2JG3Wg=GPMeoVnOpAjsvAA@mail.gmail.com
> + */
>  static const struct ata_port_info xgene_ahci_port_info = {
> -       AHCI_HFLAGS(AHCI_HFLAG_NO_PMP | AHCI_HFLAG_YES_NCQ),
> -       .flags = AHCI_FLAG_COMMON | ATA_FLAG_NCQ,
> +       AHCI_HFLAGS(AHCI_HFLAG_NO_PMP),
> +       .flags = AHCI_FLAG_COMMON,
>         .pio_mask = ATA_PIO4,
>         .udma_mask = ATA_UDMA6,
>         .port_ops = &xgene_ahci_ops,

Thanks,
--
Ming Lei

Re: [Regression] commit 8a4aeec8d(libata/ahci: accommodate tag ordered controllers)

[Dan Williams]

On Fri, 2014-06-06 at 09:47 +0800, Ming Lei wrote:
> Hi Tejun,
> 
> On Thu, Jun 5, 2014 at 9:41 PM, Tejun Heo <tj@kernel.org> wrote:
> > Hello,
> >
> > (cc'ing ahci_xgene folks)
> >
> > On Thu, Jun 05, 2014 at 09:24:04PM +0800, Ming Lei wrote:
> >> On Thu, Jun 5, 2014 at 9:12 PM, Dan Williams <dan.j.williams@intel.com> wrote:
> >> > On Thu, Jun 5, 2014 at 1:53 AM, Ming Lei <ming.lei@canonical.com> wrote:
> >> >> Hi Dan and Tejun,
> >> >>
> >> >> Looks the commit 8a4aeec8d(libata/ahci: accommodate tag ordered
> >> >> controllers) causes below sata failure[1] on APM AHCI controller.
> >> >>
> >> >> And the error does disappear after reverting the commit.
> >> >
> >> > Can you give some more details about the workload and the disk?  What
> >> > is the APM AHCI?
> >>
> >> The commit causes the APM arm64 based system not bootable, and not
> >> special workload, just when mounting rootfs or reading init files.
> >>
> >> APM AHCI:
> >>       drivers/ata/ahci_xgene.c
> >
> > Urgh... so, the controller can't handle tags allocated in round-robin
> > instead of lowest-first?  Ming, can you please test with different
> > controllers / disks / ssds and see whether the problem stays with the
> > controller?  Loc, can you guys please look into it so that at least
> > the next revision hardware can fix the issue?
> 
> The problem has been observed on several apm arm64 boards
> with different disks.
> 
> >
> > Provided that the tag allocation itself isn't broken, which isn't too
> > likely given the lack of bug reports on other controllers, we'll need
> > to blacklist ahci_xgene somehow.  Either disable NCQ support on it or
> > implement an alternative lowest-first tag allocation for it.  If this
> > actually is the controller freaking out about tags allocated in a
> > different order, I'm not sure how much confidence we can put in its
> > NCQ implementation tho and it might be a better idea to simply plug
> > NCQ support on it until we understand the details of the problem.
> >
> > So, something like the following?  Ming, can you please verify whether
> > the following makes the machine boot again?
> 
> Looks the problem still persists after applying your patch of disabling
> NCQ.

How about the following patch?

8<---------
libata: allow xgene-ahci to opt-out of tag ordered submission

From: Dan Williams <dan.j.williams@intel.com>

Ming Lei reports:
  "Looks the commit 8a4aeec8d(libata/ahci: accommodate tag ordered
   controllers) causes below sata failure[1] on APM AHCI controller.

   And the error does disappear after reverting the commit.

     ata4.00: exception Emask 0x40 SAct 0xff00 SErr 0x800 action 0x6 frozen
     ata4: SError: { HostInt }
     ata4.00: failed command: READ FPDMA QUEUED
     ata4.00: cmd 60/08:40:e0:a4:88/00:00:04:00:00/40 tag 8 ncq 4096 in
              res 40/00:00:00:00:00/00:00:00:00:00/00 Emask 0x44 (timeout)"

Maintain tag ordered submission as the default, but allow xgene-ahci to
continue with the old behavior.

Reported-by: Ming Lei <ming.lei@canonical.com>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 drivers/ata/ahci_xgene.c  |    1 +
 drivers/ata/libata-core.c |   42 +++++++++++++++++++++++++++++++++++++++---
 drivers/ata/libata.h      |    2 ++
 include/linux/libata.h    |    1 +
 4 files changed, 43 insertions(+), 3 deletions(-)

diff --git a/drivers/ata/ahci_xgene.c b/drivers/ata/ahci_xgene.c
index 77c89bf171f1..9be069f5f58a 100644
--- a/drivers/ata/ahci_xgene.c
+++ b/drivers/ata/ahci_xgene.c
@@ -300,6 +300,7 @@ static struct ata_port_operations xgene_ahci_ops = {
 	.host_stop = xgene_ahci_host_stop,
 	.hardreset = xgene_ahci_hardreset,
 	.read_id = xgene_ahci_read_id,
+	.qc_new = ata_qc_new_fifo_order,
 };
 
 static const struct ata_port_info xgene_ahci_port_info = {
diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index 943cc8b83e59..dd554354791f 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -83,6 +83,7 @@ const struct ata_port_operations ata_base_port_ops = {
 	.error_handler		= ata_std_error_handler,
 	.sched_eh		= ata_std_sched_eh,
 	.end_eh			= ata_std_end_eh,
+	.qc_new			= ata_qc_new_tag_order,
 };
 
 const struct ata_port_operations sata_port_ops = {
@@ -4784,14 +4785,17 @@ void swap_buf_le16(u16 *buf, unsigned int buf_words)
 }
 
 /**
- *	ata_qc_new - Request an available ATA command, for queueing
+ *	ata_qc_new_tag_order - Request an available ATA command, for queueing
  *	@ap: target port
  *
+ *	Accomodates controllers that issue commands in tag order rather
+ *	than FIFO order (Intel AHCI).
+ *
  *	LOCKING:
  *	None.
  */
 
-static struct ata_queued_cmd *ata_qc_new(struct ata_port *ap)
+struct ata_queued_cmd *ata_qc_new_tag_order(struct ata_port *ap)
 {
 	struct ata_queued_cmd *qc = NULL;
 	unsigned int i, tag;
@@ -4819,6 +4823,38 @@ static struct ata_queued_cmd *ata_qc_new(struct ata_port *ap)
 }
 
 /**
+ *	ata_qc_new_fifo_order - Request an available ATA command, for queueing
+ *	@ap: target port
+ *
+ *	Allocate first available tag, for hosts that maintain fifo issue
+ *	order on the wire, or otherwise cannot handle tag order.
+ *
+ *	LOCKING:
+ *	None.
+ */
+
+struct ata_queued_cmd *ata_qc_new_fifo_order(struct ata_port *ap)
+{
+	struct ata_queued_cmd *qc = NULL;
+	unsigned int i;
+
+	/* no command while frozen */
+	if (unlikely(ap->pflags & ATA_PFLAG_FROZEN))
+		return NULL;
+
+	/* the last tag is reserved for internal command. */
+	for (i = 0; i < ATA_MAX_QUEUE - 1; i++)
+		if (!test_and_set_bit(i, &ap->qc_allocated)) {
+			qc = __ata_qc_from_tag(ap, i);
+			qc->tag = i;
+			break;
+		}
+
+	return qc;
+}
+EXPORT_SYMBOL_GPL(ata_qc_new_fifo_order);
+
+/**
  *	ata_qc_new_init - Request an available ATA command, and initialize it
  *	@dev: Device from whom we request an available command structure
  *
@@ -4831,7 +4867,7 @@ struct ata_queued_cmd *ata_qc_new_init(struct ata_device *dev)
 	struct ata_port *ap = dev->link->ap;
 	struct ata_queued_cmd *qc;
 
-	qc = ata_qc_new(ap);
+	qc = ap->ops->qc_new(ap);
 	if (qc) {
 		qc->scsicmd = NULL;
 		qc->ap = ap;
diff --git a/drivers/ata/libata.h b/drivers/ata/libata.h
index 45b5ab3a95d5..381ba41de464 100644
--- a/drivers/ata/libata.h
+++ b/drivers/ata/libata.h
@@ -90,6 +90,8 @@ extern int ata_down_xfermask_limit(struct ata_device *dev, unsigned int sel);
 extern unsigned int ata_dev_set_feature(struct ata_device *dev,
 					u8 enable, u8 feature);
 extern void ata_sg_clean(struct ata_queued_cmd *qc);
+extern struct ata_queued_cmd *ata_qc_new_tag_order(struct ata_port *ap);
+extern struct ata_queued_cmd *ata_qc_new_fifo_order(struct ata_port *ap);
 extern void ata_qc_free(struct ata_queued_cmd *qc);
 extern void ata_qc_issue(struct ata_queued_cmd *qc);
 extern void __ata_qc_complete(struct ata_queued_cmd *qc);
diff --git a/include/linux/libata.h b/include/linux/libata.h
index 5ab4e3a76721..852686837d1c 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -880,6 +880,7 @@ struct ata_port_operations {
 	void (*qc_prep)(struct ata_queued_cmd *qc);
 	unsigned int (*qc_issue)(struct ata_queued_cmd *qc);
 	bool (*qc_fill_rtf)(struct ata_queued_cmd *qc);
+	struct ata_queued_cmd *(*qc_new)(struct ata_port *ap);
 
 	/*
 	 * Configuration and exception handling

Re: [Regression] commit 8a4aeec8d(libata/ahci: accommodate tag ordered controllers)

[Ming Lei]

On Fri, Jun 6, 2014 at 10:57 AM, Dan Williams <dan.j.williams@intel.com> wrote:
> On Fri, 2014-06-06 at 09:47 +0800, Ming Lei wrote:
>> Hi Tejun,
>>
>> On Thu, Jun 5, 2014 at 9:41 PM, Tejun Heo <tj@kernel.org> wrote:
>> > Hello,
>> >
>> > (cc'ing ahci_xgene folks)
>> >
>> > On Thu, Jun 05, 2014 at 09:24:04PM +0800, Ming Lei wrote:
>> >> On Thu, Jun 5, 2014 at 9:12 PM, Dan Williams <dan.j.williams@intel.com> wrote:
>> >> > On Thu, Jun 5, 2014 at 1:53 AM, Ming Lei <ming.lei@canonical.com> wrote:
>> >> >> Hi Dan and Tejun,
>> >> >>
>> >> >> Looks the commit 8a4aeec8d(libata/ahci: accommodate tag ordered
>> >> >> controllers) causes below sata failure[1] on APM AHCI controller.
>> >> >>
>> >> >> And the error does disappear after reverting the commit.
>> >> >
>> >> > Can you give some more details about the workload and the disk?  What
>> >> > is the APM AHCI?
>> >>
>> >> The commit causes the APM arm64 based system not bootable, and not
>> >> special workload, just when mounting rootfs or reading init files.
>> >>
>> >> APM AHCI:
>> >>       drivers/ata/ahci_xgene.c
>> >
>> > Urgh... so, the controller can't handle tags allocated in round-robin
>> > instead of lowest-first?  Ming, can you please test with different
>> > controllers / disks / ssds and see whether the problem stays with the
>> > controller?  Loc, can you guys please look into it so that at least
>> > the next revision hardware can fix the issue?
>>
>> The problem has been observed on several apm arm64 boards
>> with different disks.
>>
>> >
>> > Provided that the tag allocation itself isn't broken, which isn't too
>> > likely given the lack of bug reports on other controllers, we'll need
>> > to blacklist ahci_xgene somehow.  Either disable NCQ support on it or
>> > implement an alternative lowest-first tag allocation for it.  If this
>> > actually is the controller freaking out about tags allocated in a
>> > different order, I'm not sure how much confidence we can put in its
>> > NCQ implementation tho and it might be a better idea to simply plug
>> > NCQ support on it until we understand the details of the problem.
>> >
>> > So, something like the following?  Ming, can you please verify whether
>> > the following makes the machine boot again?
>>
>> Looks the problem still persists after applying your patch of disabling
>> NCQ.
>
> How about the following patch?
>
> 8<---------
> libata: allow xgene-ahci to opt-out of tag ordered submission
>
> From: Dan Williams <dan.j.williams@intel.com>
>
> Ming Lei reports:
>   "Looks the commit 8a4aeec8d(libata/ahci: accommodate tag ordered
>    controllers) causes below sata failure[1] on APM AHCI controller.
>
>    And the error does disappear after reverting the commit.
>
>      ata4.00: exception Emask 0x40 SAct 0xff00 SErr 0x800 action 0x6 frozen
>      ata4: SError: { HostInt }
>      ata4.00: failed command: READ FPDMA QUEUED
>      ata4.00: cmd 60/08:40:e0:a4:88/00:00:04:00:00/40 tag 8 ncq 4096 in
>               res 40/00:00:00:00:00/00:00:00:00:00/00 Emask 0x44 (timeout)"
>
> Maintain tag ordered submission as the default, but allow xgene-ahci to
> continue with the old behavior.
>
> Reported-by: Ming Lei <ming.lei@canonical.com>
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> ---
>  drivers/ata/ahci_xgene.c  |    1 +
>  drivers/ata/libata-core.c |   42 +++++++++++++++++++++++++++++++++++++++---
>  drivers/ata/libata.h      |    2 ++
>  include/linux/libata.h    |    1 +
>  4 files changed, 43 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/ata/ahci_xgene.c b/drivers/ata/ahci_xgene.c
> index 77c89bf171f1..9be069f5f58a 100644
> --- a/drivers/ata/ahci_xgene.c
> +++ b/drivers/ata/ahci_xgene.c

+#include "libata.h" is needed, otherwise build will fail.

> @@ -300,6 +300,7 @@ static struct ata_port_operations xgene_ahci_ops = {
>         .host_stop = xgene_ahci_host_stop,
>         .hardreset = xgene_ahci_hardreset,
>         .read_id = xgene_ahci_read_id,
> +       .qc_new = ata_qc_new_fifo_order,
>  };
>
>  static const struct ata_port_info xgene_ahci_port_info = {
> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
> index 943cc8b83e59..dd554354791f 100644
> --- a/drivers/ata/libata-core.c
> +++ b/drivers/ata/libata-core.c
> @@ -83,6 +83,7 @@ const struct ata_port_operations ata_base_port_ops = {
>         .error_handler          = ata_std_error_handler,
>         .sched_eh               = ata_std_sched_eh,
>         .end_eh                 = ata_std_end_eh,
> +       .qc_new                 = ata_qc_new_tag_order,
>  };
>
>  const struct ata_port_operations sata_port_ops = {
> @@ -4784,14 +4785,17 @@ void swap_buf_le16(u16 *buf, unsigned int buf_words)
>  }
>
>  /**
> - *     ata_qc_new - Request an available ATA command, for queueing
> + *     ata_qc_new_tag_order - Request an available ATA command, for queueing
>   *     @ap: target port
>   *
> + *     Accomodates controllers that issue commands in tag order rather
> + *     than FIFO order (Intel AHCI).
> + *
>   *     LOCKING:
>   *     None.
>   */
>
> -static struct ata_queued_cmd *ata_qc_new(struct ata_port *ap)
> +struct ata_queued_cmd *ata_qc_new_tag_order(struct ata_port *ap)
>  {
>         struct ata_queued_cmd *qc = NULL;
>         unsigned int i, tag;
> @@ -4819,6 +4823,38 @@ static struct ata_queued_cmd *ata_qc_new(struct ata_port *ap)
>  }
>
>  /**
> + *     ata_qc_new_fifo_order - Request an available ATA command, for queueing
> + *     @ap: target port
> + *
> + *     Allocate first available tag, for hosts that maintain fifo issue
> + *     order on the wire, or otherwise cannot handle tag order.
> + *
> + *     LOCKING:
> + *     None.
> + */
> +
> +struct ata_queued_cmd *ata_qc_new_fifo_order(struct ata_port *ap)
> +{
> +       struct ata_queued_cmd *qc = NULL;
> +       unsigned int i;
> +
> +       /* no command while frozen */
> +       if (unlikely(ap->pflags & ATA_PFLAG_FROZEN))
> +               return NULL;
> +
> +       /* the last tag is reserved for internal command. */
> +       for (i = 0; i < ATA_MAX_QUEUE - 1; i++)
> +               if (!test_and_set_bit(i, &ap->qc_allocated)) {
> +                       qc = __ata_qc_from_tag(ap, i);
> +                       qc->tag = i;
> +                       break;
> +               }
> +
> +       return qc;
> +}
> +EXPORT_SYMBOL_GPL(ata_qc_new_fifo_order);
> +
> +/**
>   *     ata_qc_new_init - Request an available ATA command, and initialize it
>   *     @dev: Device from whom we request an available command structure
>   *
> @@ -4831,7 +4867,7 @@ struct ata_queued_cmd *ata_qc_new_init(struct ata_device *dev)
>         struct ata_port *ap = dev->link->ap;
>         struct ata_queued_cmd *qc;
>
> -       qc = ata_qc_new(ap);
> +       qc = ap->ops->qc_new(ap);
>         if (qc) {
>                 qc->scsicmd = NULL;
>                 qc->ap = ap;
> diff --git a/drivers/ata/libata.h b/drivers/ata/libata.h
> index 45b5ab3a95d5..381ba41de464 100644
> --- a/drivers/ata/libata.h
> +++ b/drivers/ata/libata.h
> @@ -90,6 +90,8 @@ extern int ata_down_xfermask_limit(struct ata_device *dev, unsigned int sel);
>  extern unsigned int ata_dev_set_feature(struct ata_device *dev,
>                                         u8 enable, u8 feature);
>  extern void ata_sg_clean(struct ata_queued_cmd *qc);
> +extern struct ata_queued_cmd *ata_qc_new_tag_order(struct ata_port *ap);
> +extern struct ata_queued_cmd *ata_qc_new_fifo_order(struct ata_port *ap);
>  extern void ata_qc_free(struct ata_queued_cmd *qc);
>  extern void ata_qc_issue(struct ata_queued_cmd *qc);
>  extern void __ata_qc_complete(struct ata_queued_cmd *qc);
> diff --git a/include/linux/libata.h b/include/linux/libata.h
> index 5ab4e3a76721..852686837d1c 100644
> --- a/include/linux/libata.h
> +++ b/include/linux/libata.h
> @@ -880,6 +880,7 @@ struct ata_port_operations {
>         void (*qc_prep)(struct ata_queued_cmd *qc);
>         unsigned int (*qc_issue)(struct ata_queued_cmd *qc);
>         bool (*qc_fill_rtf)(struct ata_queued_cmd *qc);
> +       struct ata_queued_cmd *(*qc_new)(struct ata_port *ap);
>
>         /*
>          * Configuration and exception handling

After applying this patch again -next tree, the apm mustang board
can boot again.

Tested-by: Ming Lei <ming.lei@canonical.com>

Thanks,
--
Ming Lei

Re: [Regression] commit 8a4aeec8d(libata/ahci: accommodate tag ordered controllers)

[Ming Lei]

Hi Suman,

Could you forward the patches to me so that I can give a test?

I don't subscribe to linux-ide list.

Thanks,
--
Ming Lei

On Sat, Jun 7, 2014 at 5:33 AM, Suman Tripathi <stripathi@apm.com> wrote:
> Hi ,
>
> I have  just posted 3 patches "ata: Fix the dma state machine lockup for APM
> X-Gene SoC SATA controller" . This patch set has fixed this tag ordering
> issue.
> Tested in apm  mustang board.
>
> Thanks,
> with regards,
> suman
>
>
> On Fri, Jun 6, 2014 at 11:41 AM, Ming Lei <ming.lei@canonical.com> wrote:
>>
>> On Fri, Jun 6, 2014 at 10:57 AM, Dan Williams <dan.j.williams@intel.com>
>> wrote:
>> > On Fri, 2014-06-06 at 09:47 +0800, Ming Lei wrote:
>> >> Hi Tejun,
>> >>
>> >> On Thu, Jun 5, 2014 at 9:41 PM, Tejun Heo <tj@kernel.org> wrote:
>> >> > Hello,
>> >> >
>> >> > (cc'ing ahci_xgene folks)
>> >> >
>> >> > On Thu, Jun 05, 2014 at 09:24:04PM +0800, Ming Lei wrote:
>> >> >> On Thu, Jun 5, 2014 at 9:12 PM, Dan Williams
>> >> >> <dan.j.williams@intel.com> wrote:
>> >> >> > On Thu, Jun 5, 2014 at 1:53 AM, Ming Lei <ming.lei@canonical.com>
>> >> >> > wrote:
>> >> >> >> Hi Dan and Tejun,
>> >> >> >>
>> >> >> >> Looks the commit 8a4aeec8d(libata/ahci: accommodate tag ordered
>> >> >> >> controllers) causes below sata failure[1] on APM AHCI controller.
>> >> >> >>
>> >> >> >> And the error does disappear after reverting the commit.
>> >> >> >
>> >> >> > Can you give some more details about the workload and the disk?
>> >> >> > What
>> >> >> > is the APM AHCI?
>> >> >>
>> >> >> The commit causes the APM arm64 based system not bootable, and not
>> >> >> special workload, just when mounting rootfs or reading init files.
>> >> >>
>> >> >> APM AHCI:
>> >> >>       drivers/ata/ahci_xgene.c
>> >> >
>> >> > Urgh... so, the controller can't handle tags allocated in round-robin
>> >> > instead of lowest-first?  Ming, can you please test with different
>> >> > controllers / disks / ssds and see whether the problem stays with the
>> >> > controller?  Loc, can you guys please look into it so that at least
>> >> > the next revision hardware can fix the issue?
>> >>
>> >> The problem has been observed on several apm arm64 boards
>> >> with different disks.
>> >>
>> >> >
>> >> > Provided that the tag allocation itself isn't broken, which isn't too
>> >> > likely given the lack of bug reports on other controllers, we'll need
>> >> > to blacklist ahci_xgene somehow.  Either disable NCQ support on it or
>> >> > implement an alternative lowest-first tag allocation for it.  If this
>> >> > actually is the controller freaking out about tags allocated in a
>> >> > different order, I'm not sure how much confidence we can put in its
>> >> > NCQ implementation tho and it might be a better idea to simply plug
>> >> > NCQ support on it until we understand the details of the problem.
>> >> >
>> >> > So, something like the following?  Ming, can you please verify
>> >> > whether
>> >> > the following makes the machine boot again?
>> >>
>> >> Looks the problem still persists after applying your patch of disabling
>> >> NCQ.
>> >
>> > How about the following patch?
>> >
>> > 8<---------
>> > libata: allow xgene-ahci to opt-out of tag ordered submission
>> >
>> > From: Dan Williams <dan.j.williams@intel.com>
>> >
>> > Ming Lei reports:
>> >   "Looks the commit 8a4aeec8d(libata/ahci: accommodate tag ordered
>> >    controllers) causes below sata failure[1] on APM AHCI controller.
>> >
>> >    And the error does disappear after reverting the commit.
>> >
>> >      ata4.00: exception Emask 0x40 SAct 0xff00 SErr 0x800 action 0x6
>> > frozen
>> >      ata4: SError: { HostInt }
>> >      ata4.00: failed command: READ FPDMA QUEUED
>> >      ata4.00: cmd 60/08:40:e0:a4:88/00:00:04:00:00/40 tag 8 ncq 4096 in
>> >               res 40/00:00:00:00:00/00:00:00:00:00/00 Emask 0x44
>> > (timeout)"
>> >
>> > Maintain tag ordered submission as the default, but allow xgene-ahci to
>> > continue with the old behavior.
>> >
>> > Reported-by: Ming Lei <ming.lei@canonical.com>
>> > Signed-off-by: Dan Williams <dan.j.williams@intel.com>
>> > ---
>> >  drivers/ata/ahci_xgene.c  |    1 +
>> >  drivers/ata/libata-core.c |   42
>> > +++++++++++++++++++++++++++++++++++++++---
>> >  drivers/ata/libata.h      |    2 ++
>> >  include/linux/libata.h    |    1 +
>> >  4 files changed, 43 insertions(+), 3 deletions(-)
>> >
>> > diff --git a/drivers/ata/ahci_xgene.c b/drivers/ata/ahci_xgene.c
>> > index 77c89bf171f1..9be069f5f58a 100644
>> > --- a/drivers/ata/ahci_xgene.c
>> > +++ b/drivers/ata/ahci_xgene.c
>>
>> +#include "libata.h" is needed, otherwise build will fail.
>>
>> > @@ -300,6 +300,7 @@ static struct ata_port_operations xgene_ahci_ops = {
>> >         .host_stop = xgene_ahci_host_stop,
>> >         .hardreset = xgene_ahci_hardreset,
>> >         .read_id = xgene_ahci_read_id,
>> > +       .qc_new = ata_qc_new_fifo_order,
>> >  };
>> >
>> >  static const struct ata_port_info xgene_ahci_port_info = {
>> > diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
>> > index 943cc8b83e59..dd554354791f 100644
>> > --- a/drivers/ata/libata-core.c
>> > +++ b/drivers/ata/libata-core.c
>> > @@ -83,6 +83,7 @@ const struct ata_port_operations ata_base_port_ops = {
>> >         .error_handler          = ata_std_error_handler,
>> >         .sched_eh               = ata_std_sched_eh,
>> >         .end_eh                 = ata_std_end_eh,
>> > +       .qc_new                 = ata_qc_new_tag_order,
>> >  };
>> >
>> >  const struct ata_port_operations sata_port_ops = {
>> > @@ -4784,14 +4785,17 @@ void swap_buf_le16(u16 *buf, unsigned int
>> > buf_words)
>> >  }
>> >
>> >  /**
>> > - *     ata_qc_new - Request an available ATA command, for queueing
>> > + *     ata_qc_new_tag_order - Request an available ATA command, for
>> > queueing
>> >   *     @ap: target port
>> >   *
>> > + *     Accomodates controllers that issue commands in tag order rather
>> > + *     than FIFO order (Intel AHCI).
>> > + *
>> >   *     LOCKING:
>> >   *     None.
>> >   */
>> >
>> > -static struct ata_queued_cmd *ata_qc_new(struct ata_port *ap)
>> > +struct ata_queued_cmd *ata_qc_new_tag_order(struct ata_port *ap)
>> >  {
>> >         struct ata_queued_cmd *qc = NULL;
>> >         unsigned int i, tag;
>> > @@ -4819,6 +4823,38 @@ static struct ata_queued_cmd *ata_qc_new(struct
>> > ata_port *ap)
>> >  }
>> >
>> >  /**
>> > + *     ata_qc_new_fifo_order - Request an available ATA command, for
>> > queueing
>> > + *     @ap: target port
>> > + *
>> > + *     Allocate first available tag, for hosts that maintain fifo issue
>> > + *     order on the wire, or otherwise cannot handle tag order.
>> > + *
>> > + *     LOCKING:
>> > + *     None.
>> > + */
>> > +
>> > +struct ata_queued_cmd *ata_qc_new_fifo_order(struct ata_port *ap)
>> > +{
>> > +       struct ata_queued_cmd *qc = NULL;
>> > +       unsigned int i;
>> > +
>> > +       /* no command while frozen */
>> > +       if (unlikely(ap->pflags & ATA_PFLAG_FROZEN))
>> > +               return NULL;
>> > +
>> > +       /* the last tag is reserved for internal command. */
>> > +       for (i = 0; i < ATA_MAX_QUEUE - 1; i++)
>> > +               if (!test_and_set_bit(i, &ap->qc_allocated)) {
>> > +                       qc = __ata_qc_from_tag(ap, i);
>> > +                       qc->tag = i;
>> > +                       break;
>> > +               }
>> > +
>> > +       return qc;
>> > +}
>> > +EXPORT_SYMBOL_GPL(ata_qc_new_fifo_order);
>> > +
>> > +/**
>> >   *     ata_qc_new_init - Request an available ATA command, and
>> > initialize it
>> >   *     @dev: Device from whom we request an available command structure
>> >   *
>> > @@ -4831,7 +4867,7 @@ struct ata_queued_cmd *ata_qc_new_init(struct
>> > ata_device *dev)
>> >         struct ata_port *ap = dev->link->ap;
>> >         struct ata_queued_cmd *qc;
>> >
>> > -       qc = ata_qc_new(ap);
>> > +       qc = ap->ops->qc_new(ap);
>> >         if (qc) {
>> >                 qc->scsicmd = NULL;
>> >                 qc->ap = ap;
>> > diff --git a/drivers/ata/libata.h b/drivers/ata/libata.h
>> > index 45b5ab3a95d5..381ba41de464 100644
>> > --- a/drivers/ata/libata.h
>> > +++ b/drivers/ata/libata.h
>> > @@ -90,6 +90,8 @@ extern int ata_down_xfermask_limit(struct ata_device
>> > *dev, unsigned int sel);
>> >  extern unsigned int ata_dev_set_feature(struct ata_device *dev,
>> >                                         u8 enable, u8 feature);
>> >  extern void ata_sg_clean(struct ata_queued_cmd *qc);
>> > +extern struct ata_queued_cmd *ata_qc_new_tag_order(struct ata_port
>> > *ap);
>> > +extern struct ata_queued_cmd *ata_qc_new_fifo_order(struct ata_port
>> > *ap);
>> >  extern void ata_qc_free(struct ata_queued_cmd *qc);
>> >  extern void ata_qc_issue(struct ata_queued_cmd *qc);
>> >  extern void __ata_qc_complete(struct ata_queued_cmd *qc);
>> > diff --git a/include/linux/libata.h b/include/linux/libata.h
>> > index 5ab4e3a76721..852686837d1c 100644
>> > --- a/include/linux/libata.h
>> > +++ b/include/linux/libata.h
>> > @@ -880,6 +880,7 @@ struct ata_port_operations {
>> >         void (*qc_prep)(struct ata_queued_cmd *qc);
>> >         unsigned int (*qc_issue)(struct ata_queued_cmd *qc);
>> >         bool (*qc_fill_rtf)(struct ata_queued_cmd *qc);
>> > +       struct ata_queued_cmd *(*qc_new)(struct ata_port *ap);
>> >
>> >         /*
>> >          * Configuration and exception handling
>>
>> After applying this patch again -next tree, the apm mustang board
>> can boot again.
>>
>> Tested-by: Ming Lei <ming.lei@canonical.com>
>>
>> Thanks,
>> --
>> Ming Lei
>
>
> CONFIDENTIALITY NOTICE: This e-mail message, including any attachments,
> is for the sole use of the intended recipient(s) and contains information
> that is confidential and proprietary to Applied Micro Circuits Corporation
> or its subsidiaries.
> It is to be used solely for the purpose of furthering the parties' business
> relationship.
> All unauthorized review, use, disclosure or distribution is prohibited.
> If you are not the intended recipient, please contact the sender by reply
> e-mail
> and destroy all copies of the original message.
>

Re: [Regression] commit 8a4aeec8d(libata/ahci: accommodate tag ordered controllers)

[Ming Lei]

Hi Suman,

On Sun, Jun 8, 2014 at 12:51 PM, Suman Tripathi <stripathi@apm.com> wrote:
> Hi Ming,
>
> I have attached the  patches but it is not accepted by Tejun yet, So I will
> post another version.

OK, thanks.

Since Tejun didn't think it is a workable approach, I am not going test
the attachment, but I will look at / test your next version.

Also, Loc mentioned that only your early chip has the problem,
I am wondering why you don't take Dan's approach just
for the affected chips?


Thanks,
--
Ming Lei