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