[PATCH 0/3] ahci: fix boot/resume COMRESET failures

[PATCH 0/3] ahci: fix boot/resume COMRESET failures

[Brian Norris]

This series addresses regression problems with

    commit 7faa33da9b7add01db9f1ad92c6a5d9145e940a7
    ahci: start engine only during soft/hard resets

Because the fix provided by the above commit caused COMRESET failures and
10 second boot-time/resume-time hangs for certain DVD drives, we need to
make the fix conditional to certain devices/platforms. Add a flag for this.

Brian

Brian Norris (3):
  ahci: add AHCI_HFLAG_STRICT_SPEC host flag
  ahci: move AHCI_HFLAGS() macro to ahci.h
  ahci_platform: add STRICT_AHCI platform type

 drivers/ata/ahci.c          |    2 --
 drivers/ata/ahci.h          |    4 ++++
 drivers/ata/ahci_platform.c |   11 +++++++++++
 drivers/ata/libahci.c       |    5 +++++
 4 files changed, 20 insertions(+), 2 deletions(-)

[PATCH 1/3] ahci: add AHCI_HFLAG_STRICT_SPEC host flag

[Brian Norris]

The following commit was intended to fix problems with some specific AHCI
platforms that would become bricks if the AHCI specification was not
followed strictly:

    commit 7faa33da9b7add01db9f1ad92c6a5d9145e940a7
    ahci: start engine only during soft/hard resets

However, some devices currently have issues with that fix, so we must
implement a flag that enables the fix only for specific controllers.

This commit simply introduces the flag, without enabling it in any driver.

Note that even when AHCI_HFLAG_STRICT_SPEC is not set, this patch does not
constitue a full revert to commit 7faa33da; there is still a change in
behavior to the ahci_port_suspend() failure path.

Signed-off-by: Brian Norris <computersforpeace@gmail.com>
---
 drivers/ata/ahci.h    |    1 +
 drivers/ata/libahci.c |    5 +++++
 2 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/drivers/ata/ahci.h b/drivers/ata/ahci.h
index b175000..9c8aed0 100644
--- a/drivers/ata/ahci.h
+++ b/drivers/ata/ahci.h
@@ -210,6 +210,7 @@ enum {
 	AHCI_HFLAG_NO_SNTF		= (1 << 12), /* no sntf */
 	AHCI_HFLAG_NO_FPDMA_AA		= (1 << 13), /* no FPDMA AA */
 	AHCI_HFLAG_YES_FBS		= (1 << 14), /* force FBS cap on */
+	AHCI_HFLAG_STRICT_SPEC		= (1 << 15), /* strict AHCI spec */
 
 	/* ap->flags bits */
 
diff --git a/drivers/ata/libahci.c b/drivers/ata/libahci.c
index a72bfd0..f59abd0 100644
--- a/drivers/ata/libahci.c
+++ b/drivers/ata/libahci.c
@@ -737,6 +737,7 @@ static void ahci_power_down(struct ata_port *ap)
 
 static void ahci_start_port(struct ata_port *ap)
 {
+	struct ahci_host_priv *hpriv = ap->host->private_data;
 	struct ahci_port_priv *pp = ap->private_data;
 	struct ata_link *link;
 	struct ahci_em_priv *emp;
@@ -746,6 +747,10 @@ static void ahci_start_port(struct ata_port *ap)
 	/* enable FIS reception */
 	ahci_start_fis_rx(ap);
 
+	/* enable DMA */
+	if (!(hpriv->flags & AHCI_HFLAG_STRICT_SPEC))
+		ahci_start_engine(ap);
+
 	/* turn on LEDs */
 	if (ap->flags & ATA_FLAG_EM) {
 		ata_for_each_link(link, ap, EDGE) {

Re: [PATCH 1/3] ahci: add AHCI_HFLAG_STRICT_SPEC host flag

[Tejun Heo]

Hello,

Overall the patchset looks good but

On Mon, Feb 20, 2012 at 12:09:21PM -0800, Brian Norris wrote:
> +	AHCI_HFLAG_STRICT_SPEC		= (1 << 15), /* strict AHCI spec */

can you please use something more specific?  e.g. something like
SKIP_ENGINE_ON_PORT_START and then explain what the flag is about in
the comment?

Thanks.

-- 
tejun

Re: [PATCH 1/3] ahci: add AHCI_HFLAG_STRICT_SPEC host flag

[Brian Norris]

On Mon, Feb 20, 2012 at 12:51 PM, Tejun Heo <tj@kernel.org> wrote:
> Overall the patchset looks good but
>
> On Mon, Feb 20, 2012 at 12:09:21PM -0800, Brian Norris wrote:
>> +     AHCI_HFLAG_STRICT_SPEC          = (1 << 15), /* strict AHCI spec */
>
> can you please use something more specific?  e.g. something like
> SKIP_ENGINE_ON_PORT_START and then explain what the flag is about in
> the comment?

So you want AHCI_HFLAG_SKIP_ENGINE_ON_PORT_START? That seems
excessively long... how about AHCI_HFLAG_DELAY_ENGINE? (since it
pushes the start engine call back to the error-handling stage)

I agree the STRICT_SPEC name is a little too generic, but I couldn't
immediately come up with something better.

Brian

Re: [PATCH 1/3] ahci: add AHCI_HFLAG_STRICT_SPEC host flag

[Tejun Heo]

On Mon, Feb 20, 2012 at 01:16:27PM -0800, Brian Norris wrote:
> On Mon, Feb 20, 2012 at 12:51 PM, Tejun Heo <tj@kernel.org> wrote:
> > Overall the patchset looks good but
> >
> > On Mon, Feb 20, 2012 at 12:09:21PM -0800, Brian Norris wrote:
> >> +     AHCI_HFLAG_STRICT_SPEC          = (1 << 15), /* strict AHCI spec */
> >
> > can you please use something more specific?  e.g. something like
> > SKIP_ENGINE_ON_PORT_START and then explain what the flag is about in
> > the comment?
> 
> So you want AHCI_HFLAG_SKIP_ENGINE_ON_PORT_START? That seems
> excessively long... how about AHCI_HFLAG_DELAY_ENGINE? (since it
> pushes the start engine call back to the error-handling stage)

Ooh, yeah, that seems better.  I'm fine with any name which ties it to
engine behavior on port start.

Thanks.

-- 
tejun

Re: [PATCH 1/3] ahci: add AHCI_HFLAG_STRICT_SPEC host flag

[Brian Norris]

On Mon, Feb 20, 2012 at 1:23 PM, Tejun Heo <tj@kernel.org> wrote:
> On Mon, Feb 20, 2012 at 01:16:27PM -0800, Brian Norris wrote:
>> On Mon, Feb 20, 2012 at 12:51 PM, Tejun Heo <tj@kernel.org> wrote:
>> > Overall the patchset looks good but
>> >
>> > On Mon, Feb 20, 2012 at 12:09:21PM -0800, Brian Norris wrote:
>> >> +     AHCI_HFLAG_STRICT_SPEC          = (1 << 15), /* strict AHCI spec */
>> >
>> > can you please use something more specific?  e.g. something like
>> > SKIP_ENGINE_ON_PORT_START and then explain what the flag is about in
>> > the comment?
>>
>> So you want AHCI_HFLAG_SKIP_ENGINE_ON_PORT_START? That seems
>> excessively long... how about AHCI_HFLAG_DELAY_ENGINE? (since it
>> pushes the start engine call back to the error-handling stage)
>
> Ooh, yeah, that seems better.  I'm fine with any name which ties it to
> engine behavior on port start.

OK. I'll wait a bit for other comments, then send a v2 series with
AHCI_HFLAG_DELAY_ENGINE.

Brian

[PATCH 2/3] ahci: move AHCI_HFLAGS() macro to ahci.h

[Brian Norris]

We will need this macro in both ahci.c and ahci_platform.c, so just move it
to the header.

Signed-off-by: Brian Norris <computersforpeace@gmail.com>
---
 drivers/ata/ahci.c |    2 --
 drivers/ata/ahci.h |    3 +++
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
index d07bf03..ddeb845 100644
--- a/drivers/ata/ahci.c
+++ b/drivers/ata/ahci.c
@@ -103,8 +103,6 @@ static struct ata_port_operations ahci_p5wdh_ops = {
 	.hardreset		= ahci_p5wdh_hardreset,
 };
 
-#define AHCI_HFLAGS(flags)	.private_data	= (void *)(flags)
-
 static const struct ata_port_info ahci_port_info[] = {
 	/* by features */
 	[board_ahci] =
diff --git a/drivers/ata/ahci.h b/drivers/ata/ahci.h
index 9c8aed0..7522dd0 100644
--- a/drivers/ata/ahci.h
+++ b/drivers/ata/ahci.h
@@ -195,6 +195,9 @@ enum {
 	PORT_FBS_EN		= (1 << 0), /* Enable FBS */
 
 	/* hpriv->flags bits */
+
+#define AHCI_HFLAGS(flags)		.private_data	= (void *)(flags)
+
 	AHCI_HFLAG_NO_NCQ		= (1 << 0),
 	AHCI_HFLAG_IGN_IRQ_IF_ERR	= (1 << 1), /* ignore IRQ_IF_ERR */
 	AHCI_HFLAG_IGN_SERR_INTERNAL	= (1 << 2), /* ignore SERR_INTERNAL */

[PATCH 3/3] ahci_platform: add STRICT_AHCI platform type

[Brian Norris]

Some platforms need to make use of the AHCI_HFLAG_STRICT_SPEC flag.

Signed-off-by: Brian Norris <computersforpeace@gmail.com>
---
 drivers/ata/ahci_platform.c |   11 +++++++++++
 1 files changed, 11 insertions(+), 0 deletions(-)

diff --git a/drivers/ata/ahci_platform.c b/drivers/ata/ahci_platform.c
index 48be4e1..50688c4 100644
--- a/drivers/ata/ahci_platform.c
+++ b/drivers/ata/ahci_platform.c
@@ -26,6 +26,7 @@
 enum ahci_type {
 	AHCI,		/* standard platform ahci */
 	IMX53_AHCI,	/* ahci on i.mx53 */
+	STRICT_AHCI,	/* stricter AHCI specification */
 };
 
 static struct platform_device_id ahci_devtype[] = {
@@ -36,6 +37,9 @@ static struct platform_device_id ahci_devtype[] = {
 		.name = "imx53-ahci",
 		.driver_data = IMX53_AHCI,
 	}, {
+		.name = "strict-ahci",
+		.driver_data = STRICT_AHCI,
+	}, {
 		/* sentinel */
 	}
 };
@@ -56,6 +60,13 @@ static const struct ata_port_info ahci_port_info[] = {
 		.udma_mask	= ATA_UDMA6,
 		.port_ops	= &ahci_pmp_retry_srst_ops,
 	},
+	[STRICT_AHCI] = {
+		AHCI_HFLAGS	(AHCI_HFLAG_STRICT_SPEC),
+		.flags		= AHCI_FLAG_COMMON,
+		.pio_mask	= ATA_PIO4,
+		.udma_mask	= ATA_UDMA6,
+		.port_ops	= &ahci_ops,
+	},
 };
 
 static struct scsi_host_template ahci_platform_sht = {

Re: [PATCH 0/3] ahci: fix boot/resume COMRESET failures

[Jeff Garzik]

On 02/20/2012 03:09 PM, Brian Norris wrote:
> This series addresses regression problems with
>
>      commit 7faa33da9b7add01db9f1ad92c6a5d9145e940a7
>      ahci: start engine only during soft/hard resets
>
> Because the fix provided by the above commit caused COMRESET failures and
> 10 second boot-time/resume-time hangs for certain DVD drives, we need to
> make the fix conditional to certain devices/platforms. Add a flag for this.
>
> Brian
>
> Brian Norris (3):
>    ahci: add AHCI_HFLAG_STRICT_SPEC host flag
>    ahci: move AHCI_HFLAGS() macro to ahci.h
>    ahci_platform: add STRICT_AHCI platform type

Patch series looks good to me.  I'll throw it into the repo today 
(unless any objections arise).

	Jeff