From patchwork Fri May 20 17:02:56 2011 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jian Peng X-Patchwork-Id: 804232 Received: from smtp1.linux-foundation.org (smtp1.linux-foundation.org [140.211.169.13]) by demeter1.kernel.org (8.14.4/8.14.3) with ESMTP id p4KH4Xdw015085 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=FAIL) for ; Fri, 20 May 2011 17:04:53 GMT Received: from daredevil.linux-foundation.org (localhost [127.0.0.1]) by smtp1.linux-foundation.org (8.14.2/8.13.5/Debian-3ubuntu1.1) with ESMTP id p4KH31wG027374; Fri, 20 May 2011 10:03:02 -0700 Received: from mail-vx0-f175.google.com (mail-vx0-f175.google.com [209.85.220.175]) by smtp1.linux-foundation.org (8.14.2/8.13.5/Debian-3ubuntu1.1) with ESMTP id p4KH2uO3027361 (version=TLSv1/SSLv3 cipher=RC4-SHA bits=128 verify=FAIL) for ; Fri, 20 May 2011 10:02:58 -0700 Received: by vxd7 with SMTP id 7so3236471vxd.6 for ; Fri, 20 May 2011 10:02:56 -0700 (PDT) MIME-Version: 1.0 Received: by 10.52.99.103 with SMTP id ep7mr1238731vdb.68.1305910976201; Fri, 20 May 2011 10:02:56 -0700 (PDT) Received: by 10.52.113.193 with HTTP; Fri, 20 May 2011 10:02:56 -0700 (PDT) In-Reply-To: <20110520154341.GL31426@htj.dyndns.org> References: <201105120025.34531.rjw@sisk.pl> <20633.1305565337@localhost> <201105182144.37713.rjw@sisk.pl> <10124.1305906020@localhost> <20110520154341.GL31426@htj.dyndns.org> Date: Fri, 20 May 2011 10:02:56 -0700 Message-ID: From: Jian Peng To: Tejun Heo Received-SPF: pass (localhost is always allowed.) X-Spam-Status: No, hits=-3.751 required=5 tests=AWL, BAYES_00, HTML_MESSAGE, OSDL_HEADER_SPF_PASS, OSDL_HEADER_SUBJECT_BRACKETED X-Spam-Checker-Version: SpamAssassin 3.2.4-osdl_revision__1.47__ X-MIMEDefang-Filter: lf$Revision: 1.188 $ X-Scanned-By: MIMEDefang 2.63 on 140.211.169.21 Cc: Valdis.Kletnieks@vt.edu, Michael Leun , LKML , linux-ide@vger.kernel.org, Linux PM mailing list , Jeff Garzik Subject: Re: [linux-pm] [PATCH v2.6.38-rc7] Revert "libata: ahci_start_engine compliant to AHCI spec" X-BeenThere: linux-pm@lists.linux-foundation.org X-Mailman-Version: 2.1.9 Precedence: list List-Id: Linux power management List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Mime-version: 1.0 Sender: linux-pm-bounces@lists.linux-foundation.org Errors-To: linux-pm-bounces@lists.linux-foundation.org X-Greylist: IP, sender and recipient auto-whitelisted, not delayed by milter-greylist-4.2.6 (demeter1.kernel.org [140.211.167.41]); Fri, 20 May 2011 17:04:53 +0000 (UTC) Hi, Tejun/Valdis, Since this is an interoperability issue of SATA host controller, the first step I want to try it to make sure the tweak that MAKE my controller WORK does not break other controllers. You are both right that adding this majic 5ms delay at this place should not be the final solution. If this magic 5ms delay works on other affected systems, I plan to post a new patch that inside ahci_start_engine(), still perform same check, and show warning message if failed, but will set a flag, then still set START bit, and if there is such failure flag, add 5ms delay. Valdis, could you apply the following patch and retest it? Tejun, please review it. On Fri, May 20, 2011 at 8:43 AM, Tejun Heo wrote: > On Fri, May 20, 2011 at 11:40:20AM -0400, Valdis.Kletnieks@vt.edu wrote: > > On Wed, 18 May 2011 17:14:56 PDT, Jian Peng said: > > > > > > @@ -1353,6 +1332,8 @@ > > > > > > > > > > > > ahci_start_engine(ap); > > > > > > > > + msleep(5); > > > > + > > > > if (online) > > > > > > > > *class = ahci_dev_classify(ap); > > > > > > > > It may very well be that adding a magic msleep(5) here just Makes It > Work, but > > I have a gut feeling that it's in the wrong place (for starters, 'online' > can't change > > during the msleep() unless somebody *else* sets it - in which case the > locking > > is screwed up as we're not forcing a re-read of the value). The msleep() > probably > > needs to be before something else further down in the code (but I have no > idea > > exactly what). > > At this point, I think it would be better to simply add a flag and > enable the check for the affected controller. > > Thanks. > > -- > tejun > --- a/drivers/ata/libahci.c 2011-05-18 14:23:36.564665643 -0700 +++ c/drivers/ata/libahci.c 2011-05-20 09:48:06.194663506 -0700 @@ -540,6 +540,7 @@ void __iomem *port_mmio = ahci_port_base(ap); u32 tmp; u8 status; + int err = 0; status = readl(port_mmio + PORT_TFDATA) & 0xFF; @@ -553,12 +554,12 @@ * specific controller will fail under this condition */ if (status & (ATA_BUSY | ATA_DRQ)) - return; + err = 1; else { ahci_scr_read(&ap->link, SCR_STATUS, &tmp); if ((tmp & 0xf) != 0x3) - return; + err = 1; } /* start DMA */ @@ -566,6 +567,13 @@ tmp |= PORT_CMD_START; writel(tmp, port_mmio + PORT_CMD); readl(port_mmio + PORT_CMD); /* flush */ + + /* Some controllers need longer time to be ready */ + if(err) { + printk(KERN_WARNING + "Controller in wrong state when setting START bit\n"); + msleep(5); + } } EXPORT_SYMBOL_GPL(ahci_start_engine);