* [PATCH 0/3] ahci: fix boot/resume COMRESET failures
@ 2012-02-20 20:09 ` Brian Norris
0 siblings, 0 replies; 13+ messages in thread
From: Brian Norris @ 2012-02-20 20:09 UTC (permalink / raw)
To: Jeff Garzik
Cc: linux-ide, Linux Kernel, Tejun Heo, Kevin Cernekee, Brian Norris,
Lin Ming, Norbert Preining, Srivatsa S . Bhat, Valdis Kletnieks,
Rafael J . Wysocki
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(-)
^ permalink raw reply [flat|nested] 13+ messages in thread* [PATCH 1/3] ahci: add AHCI_HFLAG_STRICT_SPEC host flag
2012-02-20 20:09 ` Brian Norris
@ 2012-02-20 20:09 ` Brian Norris
-1 siblings, 0 replies; 13+ messages in thread
From: Brian Norris @ 2012-02-20 20:09 UTC (permalink / raw)
To: Jeff Garzik
Cc: linux-ide, Linux Kernel, Tejun Heo, Kevin Cernekee, Brian Norris,
Lin Ming, Norbert Preining, Srivatsa S . Bhat, Valdis Kletnieks,
Rafael J . Wysocki
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) {
^ permalink raw reply related [flat|nested] 13+ messages in thread* [PATCH 1/3] ahci: add AHCI_HFLAG_STRICT_SPEC host flag
@ 2012-02-20 20:09 ` Brian Norris
0 siblings, 0 replies; 13+ messages in thread
From: Brian Norris @ 2012-02-20 20:09 UTC (permalink / raw)
To: Jeff Garzik
Cc: linux-ide, Linux Kernel, Tejun Heo, Kevin Cernekee, Brian Norris,
Lin Ming, Norbert Preining, Srivatsa S . Bhat, Valdis Kletnieks,
Rafael J . Wysocki
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) {
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [PATCH 1/3] ahci: add AHCI_HFLAG_STRICT_SPEC host flag
2012-02-20 20:09 ` Brian Norris
(?)
@ 2012-02-20 20:51 ` Tejun Heo
2012-02-20 21:16 ` Brian Norris
-1 siblings, 1 reply; 13+ messages in thread
From: Tejun Heo @ 2012-02-20 20:51 UTC (permalink / raw)
To: Brian Norris
Cc: Jeff Garzik, linux-ide, Linux Kernel, Kevin Cernekee, Lin Ming,
Norbert Preining, Srivatsa S . Bhat, Valdis Kletnieks,
Rafael J . Wysocki
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/3] ahci: add AHCI_HFLAG_STRICT_SPEC host flag
2012-02-20 20:51 ` Tejun Heo
@ 2012-02-20 21:16 ` Brian Norris
2012-02-20 21:23 ` Tejun Heo
0 siblings, 1 reply; 13+ messages in thread
From: Brian Norris @ 2012-02-20 21:16 UTC (permalink / raw)
To: Tejun Heo
Cc: Jeff Garzik, linux-ide, Linux Kernel, Kevin Cernekee, Lin Ming,
Norbert Preining, Srivatsa S . Bhat, Valdis Kletnieks,
Rafael J . Wysocki
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/3] ahci: add AHCI_HFLAG_STRICT_SPEC host flag
2012-02-20 21:16 ` Brian Norris
@ 2012-02-20 21:23 ` Tejun Heo
2012-02-20 22:06 ` Brian Norris
0 siblings, 1 reply; 13+ messages in thread
From: Tejun Heo @ 2012-02-20 21:23 UTC (permalink / raw)
To: Brian Norris
Cc: Jeff Garzik, linux-ide, Linux Kernel, Kevin Cernekee, Lin Ming,
Norbert Preining, Srivatsa S . Bhat, Valdis Kletnieks,
Rafael J . Wysocki
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/3] ahci: add AHCI_HFLAG_STRICT_SPEC host flag
2012-02-20 21:23 ` Tejun Heo
@ 2012-02-20 22:06 ` Brian Norris
0 siblings, 0 replies; 13+ messages in thread
From: Brian Norris @ 2012-02-20 22:06 UTC (permalink / raw)
To: Tejun Heo
Cc: Jeff Garzik, linux-ide, Linux Kernel, Kevin Cernekee, Lin Ming,
Norbert Preining, Srivatsa S . Bhat, Valdis Kletnieks,
Rafael J . Wysocki
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 2/3] ahci: move AHCI_HFLAGS() macro to ahci.h
2012-02-20 20:09 ` Brian Norris
@ 2012-02-20 20:09 ` Brian Norris
-1 siblings, 0 replies; 13+ messages in thread
From: Brian Norris @ 2012-02-20 20:09 UTC (permalink / raw)
To: Jeff Garzik
Cc: linux-ide, Linux Kernel, Tejun Heo, Kevin Cernekee, Brian Norris,
Lin Ming, Norbert Preining, Srivatsa S . Bhat, Valdis Kletnieks,
Rafael J . Wysocki
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 */
^ permalink raw reply related [flat|nested] 13+ messages in thread* [PATCH 2/3] ahci: move AHCI_HFLAGS() macro to ahci.h
@ 2012-02-20 20:09 ` Brian Norris
0 siblings, 0 replies; 13+ messages in thread
From: Brian Norris @ 2012-02-20 20:09 UTC (permalink / raw)
To: Jeff Garzik
Cc: linux-ide, Linux Kernel, Tejun Heo, Kevin Cernekee, Brian Norris,
Lin Ming, Norbert Preining, Srivatsa S . Bhat, Valdis Kletnieks,
Rafael J . Wysocki
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 */
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH 3/3] ahci_platform: add STRICT_AHCI platform type
2012-02-20 20:09 ` Brian Norris
@ 2012-02-20 20:09 ` Brian Norris
-1 siblings, 0 replies; 13+ messages in thread
From: Brian Norris @ 2012-02-20 20:09 UTC (permalink / raw)
To: Jeff Garzik
Cc: linux-ide, Linux Kernel, Tejun Heo, Kevin Cernekee, Brian Norris,
Lin Ming, Norbert Preining, Srivatsa S . Bhat, Valdis Kletnieks,
Rafael J . Wysocki
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 = {
^ permalink raw reply related [flat|nested] 13+ messages in thread* [PATCH 3/3] ahci_platform: add STRICT_AHCI platform type
@ 2012-02-20 20:09 ` Brian Norris
0 siblings, 0 replies; 13+ messages in thread
From: Brian Norris @ 2012-02-20 20:09 UTC (permalink / raw)
To: Jeff Garzik
Cc: linux-ide, Linux Kernel, Tejun Heo, Kevin Cernekee, Brian Norris,
Lin Ming, Norbert Preining, Srivatsa S . Bhat, Valdis Kletnieks,
Rafael J . Wysocki
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 = {
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH 0/3] ahci: fix boot/resume COMRESET failures
2012-02-20 20:09 ` Brian Norris
` (3 preceding siblings ...)
(?)
@ 2012-02-20 20:36 ` Jeff Garzik
-1 siblings, 0 replies; 13+ messages in thread
From: Jeff Garzik @ 2012-02-20 20:36 UTC (permalink / raw)
To: Brian Norris
Cc: linux-ide, Linux Kernel, Tejun Heo, Kevin Cernekee, Lin Ming,
Norbert Preining, Srivatsa S . Bhat, Valdis Kletnieks,
Rafael J . Wysocki
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
^ permalink raw reply [flat|nested] 13+ messages in thread