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

login
register
mail settings
Submitter Brian Norris
Date Feb. 20, 2012, 8:09 p.m.
Message ID <1329768563-13715-2-git-send-email-computersforpeace@gmail.com>
Download mbox | patch
Permalink /patch/142182/
State Not Applicable
Delegated to: David Miller
Headers show

Comments

Brian Norris - Feb. 20, 2012, 8:09 p.m.
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(-)

--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Tejun Heo - Feb. 20, 2012, 8:51 p.m.
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.
Brian Norris - Feb. 20, 2012, 9:16 p.m.
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
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Tejun Heo - Feb. 20, 2012, 9:23 p.m.
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.
Brian Norris - Feb. 20, 2012, 10:06 p.m.
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
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

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